diff options
| author | riastradh <riastradh@NetBSD.org> | 2022-05-22 11:25:14 +0000 |
|---|---|---|
| committer | riastradh <riastradh@NetBSD.org> | 2022-05-22 11:25:14 +0000 |
| commit | dd25b8ea7dd4819f30919b4488b399444396e9eb (patch) | |
| tree | 49983d87c5b8ec13be4ffe58aaab9ac67f0ac55e /sys | |
| parent | 44d77602effd8862fbc1063cad5ff4cde6566355 (diff) | |
opencrypto: Make sid=0 always invalid, but OK to free.
Previously, crypto_newsession could sometimes return 0 as the
driver-specific part of the session id, and 0 as the hid, for sid=0.
But netipsec assumes that it is always safe to free sid=0 from
zero-initialized memory even if crypto_newsession has never
succeeded. So it was up to every driver in tree to gracefully handle
sid=0, if it happened to get assigned hid=0. And, as long as the
freesession callback was expected to just return an error code when
given a bogus session id, that worked out fine...because nothing ever
used the error code.
That was a terrible fragile system that should never have been
invented. Instead, let's just ensure that valid session ids are
nonzero, and make crypto_freesession with sid=0 be a no-op.
Diffstat (limited to 'sys')
| -rw-r--r-- | sys/opencrypto/crypto.c | 31 | ||||
| -rw-r--r-- | sys/opencrypto/cryptodev.h | 4 |
2 files changed, 29 insertions, 6 deletions
diff --git a/sys/opencrypto/crypto.c b/sys/opencrypto/crypto.c index dc135cf8d9f..7ed9dc95fb6 100644 --- a/sys/opencrypto/crypto.c +++ b/sys/opencrypto/crypto.c @@ -1,4 +1,4 @@ -/* $NetBSD: crypto.c,v 1.119 2022/05/19 20:51:59 riastradh Exp $ */ +/* $NetBSD: crypto.c,v 1.120 2022/05/22 11:25:14 riastradh Exp $ */ /* $FreeBSD: src/sys/opencrypto/crypto.c,v 1.4.2.5 2003/02/26 00:14:05 sam Exp $ */ /* $OpenBSD: crypto.c,v 1.41 2002/07/17 23:52:38 art Exp $ */ @@ -53,7 +53,7 @@ */ #include <sys/cdefs.h> -__KERNEL_RCSID(0, "$NetBSD: crypto.c,v 1.119 2022/05/19 20:51:59 riastradh Exp $"); +__KERNEL_RCSID(0, "$NetBSD: crypto.c,v 1.120 2022/05/22 11:25:14 riastradh Exp $"); #include <sys/param.h> #include <sys/reboot.h> @@ -800,6 +800,16 @@ crypto_newsession(u_int64_t *sid, struct cryptoini *cri, int hard) struct cryptocap *cap; int err = EINVAL; + /* + * On failure, leave *sid initialized to a sentinel value that + * crypto_freesession will ignore. This is the same as what + * you get from zero-initialized memory -- some callers (I'm + * looking at you, netipsec!) have paths that lead from + * zero-initialized memory into crypto_freesession without any + * crypto_newsession. + */ + *sid = 0; + mutex_enter(&crypto_drv_mtx); cap = crypto_select_driver_lock(cri, hard); @@ -807,6 +817,7 @@ crypto_newsession(u_int64_t *sid, struct cryptoini *cri, int hard) u_int32_t hid, lid; hid = cap - crypto_drivers; + KASSERT(hid < 0xffffff); /* * Can't do everything in one session. * @@ -820,10 +831,11 @@ crypto_newsession(u_int64_t *sid, struct cryptoini *cri, int hard) err = cap->cc_newsession(cap->cc_arg, &lid, cri); crypto_driver_lock(cap); if (err == 0) { - (*sid) = hid; + (*sid) = hid + 1; (*sid) <<= 32; (*sid) |= (lid & 0xffffffff); - (cap->cc_sessions)++; + KASSERT(*sid != 0); + cap->cc_sessions++; } else { DPRINTF("crypto_drivers[%d].cc_newsession() failed. error=%d\n", hid, err); @@ -846,6 +858,17 @@ crypto_freesession(u_int64_t sid) struct cryptocap *cap; int err = 0; + /* + * crypto_newsession never returns 0 as a sid (by virtue of + * never returning 0 as a hid, which is part of the sid). + * However, some callers assume that freeing zero is safe. + * Previously this relied on all drivers to agree that freeing + * invalid sids is a no-op, but that's a terrible API contract + * that we're getting rid of. + */ + if (sid == 0) + return; + /* Determine two IDs. */ cap = crypto_checkdriver_lock(CRYPTO_SESID2HID(sid)); if (cap == NULL) diff --git a/sys/opencrypto/cryptodev.h b/sys/opencrypto/cryptodev.h index fc5b2ab77e2..c14d7b7fb2e 100644 --- a/sys/opencrypto/cryptodev.h +++ b/sys/opencrypto/cryptodev.h @@ -1,4 +1,4 @@ -/* $NetBSD: cryptodev.h,v 1.43 2022/05/19 20:51:46 riastradh Exp $ */ +/* $NetBSD: cryptodev.h,v 1.44 2022/05/22 11:25:14 riastradh Exp $ */ /* $FreeBSD: src/sys/opencrypto/cryptodev.h,v 1.2.2.6 2003/07/02 17:04:50 sam Exp $ */ /* $OpenBSD: cryptodev.h,v 1.33 2002/07/17 23:52:39 art Exp $ */ @@ -589,7 +589,7 @@ struct cryptocap { * a copy of the driver's capabilities that can be used by client code to * optimize operation. */ -#define CRYPTO_SESID2HID(_sid) (((_sid) >> 32) & 0xffffff) +#define CRYPTO_SESID2HID(_sid) ((((_sid) >> 32) & 0xffffff) - 1) #define CRYPTO_SESID2CAPS(_sid) (((_sid) >> 56) & 0xff) #define CRYPTO_SESID2LID(_sid) (((u_int32_t) (_sid)) & 0xffffffff) |
