Prevent races in accesses of the software crypto session array.

swcr_newsession can change the pointer for swcr_sessions which races with
swcr_process which is looking up entries in this array.

Add a rwlock that protects changes to the array pointer so that
swcr_newsession and swcr_process no longer race.

Original patch by:	Steve O'Hara-Smith <Steve.OHaraSmith@isilon.com>
Reviewed by:		jmg
Sponsored by:		EMC / Isilon Storage Division

1 files changed, 55 insertions, 12 deletions

diff --git a/sys/opencrypto/cryptosoft.c b/sys/opencrypto/cryptosoft.c
index d73f462..4095e89 100644
--- a/sys/opencrypto/cryptosoft.c
+++ b/sys/opencrypto/cryptosoft.c
@@ -35,6 +35,8 @@ __FBSDID("$FreeBSD$");
 #include <sys/random.h>
 #include <sys/kernel.h>
 #include <sys/uio.h>
+#include <sys/lock.h>
+#include <sys/rwlock.h>
 
 #include <crypto/blowfish/blowfish.h>
 #include <crypto/sha1.h>
@@ -54,6 +56,8 @@ __FBSDID("$FreeBSD$");
 static	int32_t swcr_id;
 static	struct swcr_data **swcr_sessions = NULL;
 static	u_int32_t swcr_sesnum;
+/* Protects swcr_sessions pointer, not data. */
+static	struct rwlock swcr_sessions_lock;
 
 u_int8_t hmac_ipad_buffer[HMAC_MAX_BLOCK_LEN];
 u_int8_t hmac_opad_buffer[HMAC_MAX_BLOCK_LEN];
@@ -62,6 +66,7 @@ static	int swcr_encdec(struct cryptodesc *, struct swcr_data *, caddr_t, int);
 static	int swcr_authcompute(struct cryptodesc *, struct swcr_data *, caddr_t, int);
 static	int swcr_compdec(struct cryptodesc *, struct swcr_data *, caddr_t, int);
 static	int swcr_freesession(device_t dev, u_int64_t tid);
+static	int swcr_freesession_locked(device_t dev, u_int64_t tid);
 
 /*
  * Apply a symmetric encryption/decryption algorithm.
@@ -670,6 +675,7 @@ swcr_newsession(device_t dev, u_int32_t *sid, struct cryptoini *cri)
 	if (sid == NULL || cri == NULL)
 		return EINVAL;
 
+	rw_wlock(&swcr_sessions_lock);
 	if (swcr_sessions) {
 		for (i = 1; i < swcr_sesnum; i++)
 			if (swcr_sessions[i] == NULL)
@@ -692,6 +698,7 @@ swcr_newsession(device_t dev, u_int32_t *sid, struct cryptoini *cri)
 				swcr_sesnum = 0;
 			else
 				swcr_sesnum /= 2;
+			rw_wunlock(&swcr_sessions_lock);
 			return ENOBUFS;
 		}
 
@@ -705,6 +712,7 @@ swcr_newsession(device_t dev, u_int32_t *sid, struct cryptoini *cri)
 		swcr_sessions = swd;
 	}
 
+	rw_downgrade(&swcr_sessions_lock);
 	swd = &swcr_sessions[i];
 	*sid = i;
 
@@ -712,7 +720,8 @@ swcr_newsession(device_t dev, u_int32_t *sid, struct cryptoini *cri)
 		*swd = malloc(sizeof(struct swcr_data),
 		    M_CRYPTO_DATA, M_NOWAIT|M_ZERO);
 		if (*swd == NULL) {
-			swcr_freesession(dev, i);
+			swcr_freesession_locked(dev, i);
+			rw_runlock(&swcr_sessions_lock);
 			return ENOBUFS;
 		}
 
@@ -749,7 +758,8 @@ swcr_newsession(device_t dev, u_int32_t *sid, struct cryptoini *cri)
 				error = txf->setkey(&((*swd)->sw_kschedule),
 				    cri->cri_key, cri->cri_klen / 8);
 				if (error) {
-					swcr_freesession(dev, i);
+					swcr_freesession_locked(dev, i);
+					rw_runlock(&swcr_sessions_lock);
 					return error;
 				}
 			}
@@ -780,14 +790,16 @@ swcr_newsession(device_t dev, u_int32_t *sid, struct cryptoini *cri)
 			(*swd)->sw_ictx = malloc(axf->ctxsize, M_CRYPTO_DATA,
 			    M_NOWAIT);
 			if ((*swd)->sw_ictx == NULL) {
-				swcr_freesession(dev, i);
+				swcr_freesession_locked(dev, i);
+				rw_runlock(&swcr_sessions_lock);
 				return ENOBUFS;
 			}
 	
 			(*swd)->sw_octx = malloc(axf->ctxsize, M_CRYPTO_DATA,
 			    M_NOWAIT);
 			if ((*swd)->sw_octx == NULL) {
-				swcr_freesession(dev, i);
+				swcr_freesession_locked(dev, i);
+				rw_runlock(&swcr_sessions_lock);
 				return ENOBUFS;
 			}
 
@@ -810,14 +822,16 @@ swcr_newsession(device_t dev, u_int32_t *sid, struct cryptoini *cri)
 			(*swd)->sw_ictx = malloc(axf->ctxsize, M_CRYPTO_DATA,
 			    M_NOWAIT);
 			if ((*swd)->sw_ictx == NULL) {
-				swcr_freesession(dev, i);
+				swcr_freesession_locked(dev, i);
+				rw_runlock(&swcr_sessions_lock);
 				return ENOBUFS;
 			}
 	
 			(*swd)->sw_octx = malloc(cri->cri_klen / 8,
 			    M_CRYPTO_DATA, M_NOWAIT);
 			if ((*swd)->sw_octx == NULL) {
-				swcr_freesession(dev, i);
+				swcr_freesession_locked(dev, i);
+				rw_runlock(&swcr_sessions_lock);
 				return ENOBUFS;
 			}
 
@@ -841,7 +855,8 @@ swcr_newsession(device_t dev, u_int32_t *sid, struct cryptoini *cri)
 			(*swd)->sw_ictx = malloc(axf->ctxsize, M_CRYPTO_DATA,
 			    M_NOWAIT);
 			if ((*swd)->sw_ictx == NULL) {
-				swcr_freesession(dev, i);
+				swcr_freesession_locked(dev, i);
+				rw_runlock(&swcr_sessions_lock);
 				return ENOBUFS;
 			}
 
@@ -855,7 +870,8 @@ swcr_newsession(device_t dev, u_int32_t *sid, struct cryptoini *cri)
 			(*swd)->sw_cxf = cxf;
 			break;
 		default:
-			swcr_freesession(dev, i);
+			swcr_freesession_locked(dev, i);
+			rw_runlock(&swcr_sessions_lock);
 			return EINVAL;
 		}
 	
@@ -863,14 +879,26 @@ swcr_newsession(device_t dev, u_int32_t *sid, struct cryptoini *cri)
 		cri = cri->cri_next;
 		swd = &((*swd)->sw_next);
 	}
+	rw_runlock(&swcr_sessions_lock);
 	return 0;
 }
 
+static int
+swcr_freesession(device_t dev, u_int64_t tid)
+{
+	int error;
+
+	rw_rlock(&swcr_sessions_lock);
+	error = swcr_freesession_locked(dev, tid);
+	rw_runlock(&swcr_sessions_lock);
+	return error;
+}
+
 /*
  * Free a session.
  */
 static int
-swcr_freesession(device_t dev, u_int64_t tid)
+swcr_freesession_locked(device_t dev, u_int64_t tid)
 {
 	struct swcr_data *swd;
 	struct enc_xform *txf;
@@ -976,10 +1004,14 @@ swcr_process(device_t dev, struct cryptop *crp, int hint)
 	}
 
 	lid = crp->crp_sid & 0xffffffff;
-	if (lid >= swcr_sesnum || lid == 0 || swcr_sessions[lid] == NULL) {
+	rw_rlock(&swcr_sessions_lock);
+	if (swcr_sessions == NULL || lid >= swcr_sesnum || lid == 0 ||
+	    swcr_sessions[lid] == NULL) {
+		rw_runlock(&swcr_sessions_lock);
 		crp->crp_etype = ENOENT;
 		goto done;
 	}
+	rw_runlock(&swcr_sessions_lock);
 
 	/* Go through crypto descriptors, processing as we go */
 	for (crd = crp->crp_desc; crd; crd = crd->crd_next) {
@@ -993,10 +1025,17 @@ swcr_process(device_t dev, struct cryptop *crp, int hint)
 		 * XXX between the various instances of an algorithm (so we can
 		 * XXX locate the correct crypto context).
 		 */
+		rw_rlock(&swcr_sessions_lock);
+		if (swcr_sessions == NULL) {
+			rw_runlock(&swcr_sessions_lock);
+			crp->crp_etype = ENOENT;
+			goto done;
+		}
 		for (sw = swcr_sessions[lid];
 		    sw && sw->sw_alg != crd->crd_alg;
 		    sw = sw->sw_next)
 			;
+		rw_runlock(&swcr_sessions_lock);
 
 		/* No such context ? */
 		if (sw == NULL) {
@@ -1074,6 +1113,7 @@ swcr_probe(device_t dev)
 static int
 swcr_attach(device_t dev)
 {
+	rw_init(&swcr_sessions_lock, "swcr_sessions_lock");
 	memset(hmac_ipad_buffer, HMAC_IPAD_VAL, HMAC_MAX_BLOCK_LEN);
 	memset(hmac_opad_buffer, HMAC_OPAD_VAL, HMAC_MAX_BLOCK_LEN);
 
@@ -1115,8 +1155,11 @@ static int
 swcr_detach(device_t dev)
 {
 	crypto_unregister_all(swcr_id);
-	if (swcr_sessions != NULL)
-		free(swcr_sessions, M_CRYPTO_DATA);
+	rw_wlock(&swcr_sessions_lock);
+	free(swcr_sessions, M_CRYPTO_DATA);
+	swcr_sessions = NULL;
+	rw_wunlock(&swcr_sessions_lock);
+	rw_destroy(&swcr_sessions_lock);
 	return 0;
 }