Skip site navigation (1)Skip section navigation (2) 
Date:      Fri, 26 Jan 2018 23:21:50 +0000 (UTC)
From:      John Baldwin <jhb@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r328453 - head/sys/opencrypto
Message-ID:  <201801262321.w0QNLoI4068958@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: jhb
Date: Fri Jan 26 23:21:50 2018
New Revision: 328453
URL: https://svnweb.freebsd.org/changeset/base/328453

Log:
  Move per-operation data out of the csession structure.
  
  Create a struct cryptop_data which contains state needed for a single
  symmetric crypto operation and move that state out of the session. This
  closes a race with the CRYPTO_F_DONE flag that can result in use after
  free.
  
  While here, remove the 'cse->error' member.  It was just a copy of
  'crp->crp_etype' and cryptodev_op() and cryptodev_aead() checked both
  'crp->crp_etype' and 'cse->error'.  Similarly, do not check for an
  error from mtx_sleep() since it is not used with PCATCH or a timeout
  so cannot fail with an error.
  
  PR:		218597
  Reviewed by:	kib
  Sponsored by:	Chelsio Communications
  Differential Revision:	https://reviews.freebsd.org/D13928

Modified:
  head/sys/opencrypto/cryptodev.c

Modified: head/sys/opencrypto/cryptodev.c
==============================================================================
--- head/sys/opencrypto/cryptodev.c	Fri Jan 26 23:14:46 2018	(r328452)
+++ head/sys/opencrypto/cryptodev.c	Fri Jan 26 23:21:50 2018	(r328453)
@@ -281,10 +281,14 @@ struct csession {
 
 	caddr_t		mackey;
 	int		mackeylen;
+};
 
-	struct iovec	iovec;
+struct cryptop_data {
+	struct csession *cse;
+
+	struct iovec	iovec[1];
 	struct uio	uio;
-	int		error;
+	bool		done;
 };
 
 struct fcrypt {
@@ -708,9 +712,37 @@ bail:
 #undef SES2
 }
 
-static int cryptodev_cb(void *);
+static int cryptodev_cb(struct cryptop *);
 
+static struct cryptop_data *
+cod_alloc(struct csession *cse, size_t len, struct thread *td)
+{
+	struct cryptop_data *cod;
+	struct uio *uio;
 
+	cod = malloc(sizeof(struct cryptop_data), M_XDATA, M_WAITOK | M_ZERO);
+
+	cod->cse = cse;
+	uio = &cod->uio;
+	uio->uio_iov = cod->iovec;
+	uio->uio_iovcnt = 1;
+	uio->uio_resid = len;
+	uio->uio_segflg = UIO_SYSSPACE;
+	uio->uio_rw = UIO_WRITE;
+	uio->uio_td = td;
+	uio->uio_iov[0].iov_len = len;
+	uio->uio_iov[0].iov_base = malloc(len, M_XDATA, M_WAITOK);
+	return (cod);
+}
+
+static void
+cod_free(struct cryptop_data *cod)
+{
+
+	free(cod->uio.uio_iov[0].iov_base, M_XDATA);
+	free(cod, M_XDATA);
+}
+
 static int
 cryptodev_op(
 	struct csession *cse,
@@ -718,6 +750,7 @@ cryptodev_op(
 	struct ucred *active_cred,
 	struct thread *td)
 {
+	struct cryptop_data *cod = NULL;
 	struct cryptop *crp = NULL;
 	struct cryptodesc *crde = NULL, *crda = NULL;
 	int error;
@@ -734,20 +767,10 @@ cryptodev_op(
 		}
 	}
 
-	cse->uio.uio_iov = &cse->iovec;
-	cse->uio.uio_iovcnt = 1;
-	cse->uio.uio_offset = 0;
-	cse->uio.uio_resid = cop->len;
-	cse->uio.uio_segflg = UIO_SYSSPACE;
-	cse->uio.uio_rw = UIO_WRITE;
-	cse->uio.uio_td = td;
-	cse->uio.uio_iov[0].iov_len = cop->len;
-	if (cse->thash) {
-		cse->uio.uio_iov[0].iov_len += cse->thash->hashsize;
-		cse->uio.uio_resid += cse->thash->hashsize;
-	}
-	cse->uio.uio_iov[0].iov_base = malloc(cse->uio.uio_iov[0].iov_len,
-	    M_XDATA, M_WAITOK);
+	if (cse->thash)
+		cod = cod_alloc(cse, cop->len + cse->thash->hashsize, td);
+	else
+		cod = cod_alloc(cse, cop->len, td);
 
 	crp = crypto_getreq((cse->txform != NULL) + (cse->thash != NULL));
 	if (crp == NULL) {
@@ -774,7 +797,7 @@ cryptodev_op(
 		goto bail;
 	}
 
-	if ((error = copyin(cop->src, cse->uio.uio_iov[0].iov_base,
+	if ((error = copyin(cop->src, cod->uio.uio_iov[0].iov_base,
 	    cop->len))) {
 		SDT_PROBE1(opencrypto, dev, ioctl, error, __LINE__);
 		goto bail;
@@ -806,10 +829,10 @@ cryptodev_op(
 	crp->crp_ilen = cop->len;
 	crp->crp_flags = CRYPTO_F_IOV | CRYPTO_F_CBIMM
 		       | (cop->flags & COP_F_BATCH);
-	crp->crp_buf = (caddr_t)&cse->uio;
-	crp->crp_callback = (int (*) (struct cryptop *)) cryptodev_cb;
+	crp->crp_uio = &cod->uio;
+	crp->crp_callback = cryptodev_cb;
 	crp->crp_sid = cse->sid;
-	crp->crp_opaque = (void *)cse;
+	crp->crp_opaque = cod;
 
 	if (cop->iv) {
 		if (crde == NULL) {
@@ -852,19 +875,20 @@ again:
 	 * entry and the crypto_done callback into us.
 	 */
 	error = crypto_dispatch(crp);
-	mtx_lock(&cse->lock);
-	if (error == 0 && (crp->crp_flags & CRYPTO_F_DONE) == 0)
-		error = msleep(crp, &cse->lock, PWAIT, "crydev", 0);
-	mtx_unlock(&cse->lock);
-
 	if (error != 0) {
 		SDT_PROBE1(opencrypto, dev, ioctl, error, __LINE__);
 		goto bail;
 	}
 
+	mtx_lock(&cse->lock);
+	while (!cod->done)
+		mtx_sleep(cod, &cse->lock, PWAIT, "crydev", 0);
+	mtx_unlock(&cse->lock);
+
 	if (crp->crp_etype == EAGAIN) {
 		crp->crp_etype = 0;
 		crp->crp_flags &= ~CRYPTO_F_DONE;
+		cod->done = false;
 		goto again;
 	}
 
@@ -874,21 +898,15 @@ again:
 		goto bail;
 	}
 
-	if (cse->error) {
-		SDT_PROBE1(opencrypto, dev, ioctl, error, __LINE__);
-		error = cse->error;
-		goto bail;
-	}
-
 	if (cop->dst &&
-	    (error = copyout(cse->uio.uio_iov[0].iov_base, cop->dst,
+	    (error = copyout(cod->uio.uio_iov[0].iov_base, cop->dst,
 	    cop->len))) {
 		SDT_PROBE1(opencrypto, dev, ioctl, error, __LINE__);
 		goto bail;
 	}
 
 	if (cop->mac &&
-	    (error = copyout((caddr_t)cse->uio.uio_iov[0].iov_base + cop->len,
+	    (error = copyout((caddr_t)cod->uio.uio_iov[0].iov_base + cop->len,
 	    cop->mac, cse->thash->hashsize))) {
 		SDT_PROBE1(opencrypto, dev, ioctl, error, __LINE__);
 		goto bail;
@@ -897,8 +915,8 @@ again:
 bail:
 	if (crp)
 		crypto_freereq(crp);
-	if (cse->uio.uio_iov[0].iov_base)
-		free(cse->uio.uio_iov[0].iov_base, M_XDATA);
+	if (cod)
+		cod_free(cod);
 
 	return (error);
 }
@@ -910,7 +928,7 @@ cryptodev_aead(
 	struct ucred *active_cred,
 	struct thread *td)
 {
-	struct uio *uio;
+	struct cryptop_data *cod = NULL;
 	struct cryptop *crp = NULL;
 	struct cryptodesc *crde = NULL, *crda = NULL;
 	int error;
@@ -926,19 +944,9 @@ cryptodev_aead(
 		return (EINVAL);
 	}
 
-	uio = &cse->uio;
-	uio->uio_iov = &cse->iovec;
-	uio->uio_iovcnt = 1;
-	uio->uio_offset = 0;
-	uio->uio_resid = caead->aadlen + caead->len + cse->thash->hashsize;
-	uio->uio_segflg = UIO_SYSSPACE;
-	uio->uio_rw = UIO_WRITE;
-	uio->uio_td = td;
-	uio->uio_iov[0].iov_len = uio->uio_resid;
+	cod = cod_alloc(cse, caead->aadlen + caead->len + cse->thash->hashsize,
+	    td);
 
-	uio->uio_iov[0].iov_base = malloc(uio->uio_iov[0].iov_len,
-	    M_XDATA, M_WAITOK);
-
 	crp = crypto_getreq(2);
 	if (crp == NULL) {
 		error = ENOMEM;
@@ -954,13 +962,13 @@ cryptodev_aead(
 		crde = crda->crd_next;
 	}
 
-	if ((error = copyin(caead->aad, cse->uio.uio_iov[0].iov_base,
+	if ((error = copyin(caead->aad, cod->uio.uio_iov[0].iov_base,
 	    caead->aadlen))) {
 		SDT_PROBE1(opencrypto, dev, ioctl, error, __LINE__);
 		goto bail;
 	}
 
-	if ((error = copyin(caead->src, (char *)cse->uio.uio_iov[0].iov_base +
+	if ((error = copyin(caead->src, (char *)cod->uio.uio_iov[0].iov_base +
 	    caead->aadlen, caead->len))) {
 		SDT_PROBE1(opencrypto, dev, ioctl, error, __LINE__);
 		goto bail;
@@ -997,10 +1005,10 @@ cryptodev_aead(
 	crp->crp_ilen = caead->aadlen + caead->len;
 	crp->crp_flags = CRYPTO_F_IOV | CRYPTO_F_CBIMM
 		       | (caead->flags & COP_F_BATCH);
-	crp->crp_buf = (caddr_t)&cse->uio.uio_iov;
-	crp->crp_callback = (int (*) (struct cryptop *)) cryptodev_cb;
+	crp->crp_uio = &cod->uio;
+	crp->crp_callback = cryptodev_cb;
 	crp->crp_sid = cse->sid;
-	crp->crp_opaque = (void *)cse;
+	crp->crp_opaque = cod;
 
 	if (caead->iv) {
 		if (caead->ivlen > sizeof(crde->crd_iv)) {
@@ -1020,7 +1028,7 @@ cryptodev_aead(
 		crde->crd_len -= cse->txform->blocksize;
 	}
 
-	if ((error = copyin(caead->tag, (caddr_t)cse->uio.uio_iov[0].iov_base +
+	if ((error = copyin(caead->tag, (caddr_t)cod->uio.uio_iov[0].iov_base +
 	    caead->len + caead->aadlen, cse->thash->hashsize))) {
 		SDT_PROBE1(opencrypto, dev, ioctl, error, __LINE__);
 		goto bail;
@@ -1034,19 +1042,20 @@ again:
 	 * entry and the crypto_done callback into us.
 	 */
 	error = crypto_dispatch(crp);
-	mtx_lock(&cse->lock);
-	if (error == 0 && (crp->crp_flags & CRYPTO_F_DONE) == 0)
-		error = msleep(crp, &cse->lock, PWAIT, "crydev", 0);
-	mtx_unlock(&cse->lock);
-
 	if (error != 0) {
 		SDT_PROBE1(opencrypto, dev, ioctl, error, __LINE__);
 		goto bail;
 	}
 
+	mtx_lock(&cse->lock);
+	while (!cod->done)
+		mtx_sleep(cod, &cse->lock, PWAIT, "crydev", 0);
+	mtx_unlock(&cse->lock);
+
 	if (crp->crp_etype == EAGAIN) {
 		crp->crp_etype = 0;
 		crp->crp_flags &= ~CRYPTO_F_DONE;
+		cod->done = false;
 		goto again;
 	}
 
@@ -1056,20 +1065,14 @@ again:
 		goto bail;
 	}
 
-	if (cse->error) {
-		error = cse->error;
-		SDT_PROBE1(opencrypto, dev, ioctl, error, __LINE__);
-		goto bail;
-	}
-
 	if (caead->dst && (error = copyout(
-	    (caddr_t)cse->uio.uio_iov[0].iov_base + caead->aadlen, caead->dst,
+	    (caddr_t)cod->uio.uio_iov[0].iov_base + caead->aadlen, caead->dst,
 	    caead->len))) {
 		SDT_PROBE1(opencrypto, dev, ioctl, error, __LINE__);
 		goto bail;
 	}
 
-	if ((error = copyout((caddr_t)cse->uio.uio_iov[0].iov_base +
+	if ((error = copyout((caddr_t)cod->uio.uio_iov[0].iov_base +
 	    caead->aadlen + caead->len, caead->tag, cse->thash->hashsize))) {
 		SDT_PROBE1(opencrypto, dev, ioctl, error, __LINE__);
 		goto bail;
@@ -1077,21 +1080,26 @@ again:
 
 bail:
 	crypto_freereq(crp);
-	free(cse->uio.uio_iov[0].iov_base, M_XDATA);
+	if (cod)
+		cod_free(cod);
 
 	return (error);
 }
 
 static int
-cryptodev_cb(void *op)
+cryptodev_cb(struct cryptop *crp)
 {
-	struct cryptop *crp = (struct cryptop *) op;
-	struct csession *cse = (struct csession *)crp->crp_opaque;
+	struct cryptop_data *cod = crp->crp_opaque;
 
-	mtx_lock(&cse->lock);
-	cse->error = crp->crp_etype;
-	wakeup_one(crp);
-	mtx_unlock(&cse->lock);
+	/*
+	 * Lock to ensure the wakeup() is not missed by the loops
+	 * waiting on cod->done in cryptodev_op() and
+	 * cryptodev_aead().
+	 */
+	mtx_lock(&cod->cse->lock);
+	cod->done = true;
+	mtx_unlock(&cod->cse->lock);
+	wakeup(cod);
 	return (0);
 }

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201801262321.w0QNLoI4068958>