Date: Mon, 11 Mar 2019 22:17:53 +0000 (UTC) From: John Baldwin <jhb@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org Subject: svn commit: r345033 - stable/11/sys/opencrypto Message-ID: <201903112217.x2BMHrwo060526@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
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); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201903112217.x2BMHrwo060526>