From 479de1f7b7a1746fa617adc3cfb902d6e73224ae Mon Sep 17 00:00:00 2001 From: riastradh Date: Sun, 22 May 2022 11:25:14 +0000 Subject: [PATCH] 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. --- sys/opencrypto/crypto.c | 31 +++++++++++++++++++++++++++---- 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 dc135cf8d9f7..7ed9dc95fb60 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 -__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 #include @@ -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 fc5b2ab77e21..c14d7b7fb2e2 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)