Author: jhb
Date: Mon Mar 11 22:17:53 2019
New Revision: 345033
URL: https://svnweb.freebsd.org/changeset/base/345033
Log:
MFC 328453: 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
Sponsored by: Chelsio Communications
Modified:
stable/11/sys/opencrypto/cryptodev.c
Directory Properties:
stable/11/ (props changed)
Modified: stable/11/sys/opencrypto/cryptodev.c
==============================================================================
--- stable/11/sys/opencrypto/cryptodev.c Mon Mar 11 22:05:34 2019
(r345032)
+++ stable/11/sys/opencrypto/cryptodev.c Mon Mar 11 22:17:53 2019
(r345033)
@@ -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 {
@@ -707,9 +711,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,
@@ -717,6 +749,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;
@@ -733,20 +766,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) {
@@ -773,7 +796,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;
@@ -805,10 +828,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) {
@@ -851,19 +874,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;
}
@@ -873,21 +897,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;
@@ -896,8 +914,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);
}
@@ -909,7 +927,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;
@@ -925,19 +943,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;
@@ -953,13 +961,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;
@@ -996,10 +1004,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)) {
@@ -1019,7 +1027,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;
@@ -1033,19 +1041,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;
}
@@ -1055,20 +1064,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;
@@ -1076,21 +1079,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);
}
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
