Page MenuHomeFreeBSD

Add a reference count to cryptodev sessions.
ClosedPublic

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

Details

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jhb created this revision.Jan 8 2020, 12:26 AM
cem added a comment.Jan 8 2020, 12:51 AM

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?

cem added a comment.Jan 8 2020, 6:19 PM

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.)

This revision was automatically updated to reflect the committed changes.