Aren
/
NetBSD
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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.
This commit is contained in:
riastradh 2022-05-22 11:25:14 +00:00
parent fbd9c37871
commit 479de1f7b7
2 changed files with 29 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
sys/opencrypto/crypto.c
View File

@ -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)

4
sys/opencrypto/cryptodev.h
View File

@ -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)