summaryrefslogtreecommitdiff
path: root/sys
diff options
context:
space:
mode:
authorriastradh <riastradh@NetBSD.org>2022-05-22 11:25:14 +0000
committerriastradh <riastradh@NetBSD.org>2022-05-22 11:25:14 +0000
commitdd25b8ea7dd4819f30919b4488b399444396e9eb (patch)
tree49983d87c5b8ec13be4ffe58aaab9ac67f0ac55e /sys
parent44d77602effd8862fbc1063cad5ff4cde6566355 (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.c31
-rw-r--r--sys/opencrypto/cryptodev.h4
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)