[src/trunk]: src/sys/opencrypto opencrypto: Make sid=0 always invalid, but OK...

[riastradh <riastradh%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/fc5232bd13f5
branches:  trunk
changeset: 366307:fc5232bd13f5
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Sun May 22 11:25:14 2022 +0000

description:
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:

 sys/opencrypto/crypto.c    |  31 +++++++++++++++++++++++++++----
 sys/opencrypto/cryptodev.h |   4 ++--
 2 files changed, 29 insertions(+), 6 deletions(-)

diffs (93 lines):

diff -r 2f2db50da923 -r fc5232bd13f5 sys/opencrypto/crypto.c
--- a/sys/opencrypto/crypto.c   Sun May 22 10:45:02 2022 +0000
+++ b/sys/opencrypto/crypto.c   Sun May 22 11:25:14 2022 +0000
@@ -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 @@
        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 @@
                u_int32_t hid, lid;
 
                hid = cap - crypto_drivers;
+               KASSERT(hid < 0xffffff);
                /*
                 * Can't do everything in one session.
                 *
@@ -820,10 +831,11 @@
                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 @@
        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 -r 2f2db50da923 -r fc5232bd13f5 sys/opencrypto/cryptodev.h
--- a/sys/opencrypto/cryptodev.h        Sun May 22 10:45:02 2022 +0000
+++ b/sys/opencrypto/cryptodev.h        Sun May 22 11:25:14 2022 +0000
@@ -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 @@
  * 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)