Add a reference count to cryptodev sessions.
ClosedPublic
Actions

Authored by jhb on Jan 8 2020, 12:26 AM.

Details

Reviewers

cem
jmg

Commits

rS356507: Add a reference count to cryptodev sessions.

Summary

This prevents use-after-free races with crypto requests (which may
sleep) and CIOCFSESSION as well as races from current CIOCFSESSION
requests.

Test Plan

tested cryptocheck still works which doesn't exercise the race, but shows that normal operation still works

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Attempting to test this but ran into https://reviews.freebsd.org/D23073 sigh.

sys/opencrypto/cryptodev.c
329 ↗	(On Diff #66470)	cseadd is no longer defined / used

cem accepted this revision.Jan 8 2020, 1:02 AM

cem added inline comments.

sys/opencrypto/cryptodev.c
1379–1381 ↗	(On Diff #66470)	I'm not sure how this is valid. But I did not trip it in my testing.

This revision is now accepted and ready to land.Jan 8 2020, 1:02 AM

jhb added inline comments.Jan 8 2020, 1:28 AM

sys/opencrypto/cryptodev.c
1379–1381 ↗	(On Diff #66470)	By the time `cryptof_close()` is called, there should be no other references on the `fp` (it's called from the last `fdrop()`). Any existing ioctl calls would hold a reference on the `fp`, so no threads can be in `cryptof_ioctl()` (including sleeping on a crypto request) when this function is invoked.

cem added inline comments.Jan 8 2020, 2:03 AM

sys/opencrypto/cryptodev.c
1379–1381 ↗	(On Diff #66470)	Oh, I see. An attacker can create as many `cse` objects with refcount exactly 1 as it likes using `CIOCGSESSION2` (`csecreate`); any additional refs are ephemeral. So wait, why do we need reference counts at all? Is the lock alone sufficient?

jhb added inline comments.Jan 8 2020, 4:43 PM

sys/opencrypto/cryptodev.c
1379–1381 ↗	(On Diff #66470)	CIOCCRYPT and CIOCAEAD can sleep. You could use an sx lock instead I suppose, but this seems more straightforward?

Lgtm

sys/opencrypto/cryptodev.c
1379–1381 ↗	(On Diff #66470)	Oh, and the race is against concurrent FSESSION? Ok. We could also take the relevant cse off-list under lock but then we’re changing semantics. And we can’t use epoch because they sleep. I’m on board. We should delete more of this driver when we can :-/. (Or start from scratch? We know the abi, roughly.)