Module Name: src
Committed By: riastradh
Date: Sun May 22 11:25:14 UTC 2022
Modified Files:
src/sys/opencrypto: crypto.c cryptodev.h
Log Message:
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.
To generate a diff of this commit:
cvs rdiff -u -r1.119 -r1.120 src/sys/opencrypto/crypto.c
cvs rdiff -u -r1.43 -r1.44 src/sys/opencrypto/cryptodev.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/opencrypto/crypto.c
diff -u src/sys/opencrypto/crypto.c:1.119 src/sys/opencrypto/crypto.c:1.120
--- src/sys/opencrypto/crypto.c:1.119 Thu May 19 20:51:59 2022
+++ src/sys/opencrypto/crypto.c Sun May 22 11:25:14 2022
@@ -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
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
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
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)
Index: src/sys/opencrypto/cryptodev.h
diff -u src/sys/opencrypto/cryptodev.h:1.43 src/sys/opencrypto/cryptodev.h:1.44
--- src/sys/opencrypto/cryptodev.h:1.43 Thu May 19 20:51:46 2022
+++ src/sys/opencrypto/cryptodev.h Sun May 22 11:25:14 2022
@@ -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)
