Fix a race condition in opencrypto where the crypto request could be
completed by the crypto device, queued on the retq, but freed by the
ioctl lwp. The problem manifests as various panics relating to the
condvar inside the request. The problem can occur whenever the crypto
device completes the request immediately and the ioctl skips the cv_wait().
The problem can be reproduced by enabling cryptosoft and running an openssl
speed test. E.g.
sysctl -w kern.cryptodevallowsoft=-1
openssl speed -engine cryptodev -evp des-ede3-cbc -multi 64
Add a macro for TAILQ_FOREACH_REVERSE_SAFE() to queue.h, since this
was missing and the opencrypto code removes requests from a list while
iterating with TAILQ_FOREACH_REVERSE().
Add missing cv_destroy() calls for the key request cleanup.
Reviewed by Thor Lancelot Simon.
This commit is contained in:
darran
parent
72b66cb31e
commit
d6a1889de6
@ -1,4 +1,4 @@
|
||||
/* $NetBSD: crypto.c,v 1.29 2008/08/03 10:18:12 degroote Exp $ */
|
||||
/* $NetBSD: crypto.c,v 1.30 2008/11/18 12:59:58 darran 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.29 2008/08/03 10:18:12 degroote Exp $");
|
||||
__KERNEL_RCSID(0, "$NetBSD: crypto.c,v 1.30 2008/11/18 12:59:58 darran Exp $");
|
||||
|
||||
#include <sys/param.h>
|
||||
#include <sys/reboot.h>
|
||||
@ -82,6 +82,8 @@ kmutex_t crypto_mtx;
|
||||
|
||||
#define SESID2HID(sid) (((sid) >> 32) & 0xffffffff)
|
||||
|
||||
int crypto_ret_q_check(struct cryptop *);
|
||||
|
||||
/*
|
||||
* Crypto drivers register themselves by allocating a slot in the
|
||||
* crypto_drivers table with crypto_get_driverid() and then registering
|
||||
@ -124,9 +126,9 @@ static TAILQ_HEAD(krprethead, cryptkop) crp_ret_kq =
|
||||
int
|
||||
crypto_ret_q_remove(struct cryptop *crp)
|
||||
{
|
||||
struct cryptop * acrp;
|
||||
struct cryptop * acrp, *next;
|
||||
|
||||
TAILQ_FOREACH_REVERSE(acrp, &crp_ret_q, crprethead, crp_next) {
|
||||
TAILQ_FOREACH_REVERSE_SAFE(acrp, &crp_ret_q, crprethead, crp_next, next) {
|
||||
if (acrp == crp) {
|
||||
TAILQ_REMOVE(&crp_ret_q, crp, crp_next);
|
||||
crp->crp_flags &= (~CRYPTO_F_ONRETQ);
|
||||
@ -139,9 +141,9 @@ crypto_ret_q_remove(struct cryptop *crp)
|
||||
int
|
||||
crypto_ret_kq_remove(struct cryptkop *krp)
|
||||
{
|
||||
struct cryptkop * akrp;
|
||||
struct cryptkop * akrp, *next;
|
||||
|
||||
TAILQ_FOREACH_REVERSE(akrp, &crp_ret_kq, krprethead, krp_next) {
|
||||
TAILQ_FOREACH_REVERSE_SAFE(akrp, &crp_ret_kq, krprethead, krp_next, next) {
|
||||
if (akrp == krp) {
|
||||
TAILQ_REMOVE(&crp_ret_kq, krp, krp_next);
|
||||
krp->krp_flags &= (~CRYPTO_F_ONRETQ);
|
||||
@ -830,6 +832,7 @@ crypto_kinvoke(struct cryptkop *krp, int hint)
|
||||
if (krp == NULL)
|
||||
return EINVAL;
|
||||
if (krp->krp_callback == NULL) {
|
||||
cv_destroy(&krp->krp_cv);
|
||||
pool_put(&cryptkop_pool, krp);
|
||||
return EINVAL;
|
||||
}
|
||||
@ -901,7 +904,6 @@ crypto_invoke(struct cryptop *crp, int hint)
|
||||
if (crp == NULL)
|
||||
return EINVAL;
|
||||
if (crp->crp_callback == NULL) {
|
||||
crypto_freereq(crp);
|
||||
return EINVAL;
|
||||
}
|
||||
if (crp->crp_desc == NULL) {
|
||||
@ -912,11 +914,11 @@ crypto_invoke(struct cryptop *crp, int hint)
|
||||
|
||||
hid = SESID2HID(crp->crp_sid);
|
||||
if (hid < crypto_drivers_num) {
|
||||
mutex_enter(&crypto_mtx);
|
||||
mutex_spin_enter(&crypto_mtx);
|
||||
if (crypto_drivers[hid].cc_flags & CRYPTOCAP_F_CLEANUP)
|
||||
crypto_freesession(crp->crp_sid);
|
||||
process = crypto_drivers[hid].cc_process;
|
||||
mutex_exit(&crypto_mtx);
|
||||
mutex_spin_exit(&crypto_mtx);
|
||||
} else {
|
||||
process = NULL;
|
||||
}
|
||||
@ -929,7 +931,7 @@ crypto_invoke(struct cryptop *crp, int hint)
|
||||
* Driver has unregistered; migrate the session and return
|
||||
* an error to the caller so they'll resubmit the op.
|
||||
*/
|
||||
mutex_enter(&crypto_mtx);
|
||||
mutex_spin_enter(&crypto_mtx);
|
||||
for (crd = crp->crp_desc; crd->crd_next; crd = crd->crd_next)
|
||||
crd->CRD_INI.cri_next = &(crd->crd_next->CRD_INI);
|
||||
|
||||
@ -937,7 +939,7 @@ crypto_invoke(struct cryptop *crp, int hint)
|
||||
crp->crp_sid = nid;
|
||||
|
||||
crp->crp_etype = EAGAIN;
|
||||
mutex_exit(&crypto_mtx);
|
||||
mutex_spin_exit(&crypto_mtx);
|
||||
|
||||
crypto_done(crp);
|
||||
return 0;
|
||||
@ -961,6 +963,11 @@ crypto_freereq(struct cryptop *crp)
|
||||
if (crp == NULL)
|
||||
return;
|
||||
|
||||
/* sanity check */
|
||||
if (crp->crp_flags & CRYPTO_F_ONRETQ) {
|
||||
panic("crypto_freereq() freeing crp on RETQ\n");
|
||||
}
|
||||
|
||||
while ((crd = crp->crp_desc) != NULL) {
|
||||
crp->crp_desc = crd->crd_next;
|
||||
pool_put(&cryptodesc_pool, crd);
|
||||
@ -1015,8 +1022,6 @@ crypto_done(struct cryptop *crp)
|
||||
crypto_tstat(&cryptostats.cs_done, &crp->crp_tstamp);
|
||||
#endif
|
||||
|
||||
crp->crp_flags |= CRYPTO_F_DONE;
|
||||
|
||||
/*
|
||||
* Normal case; queue the callback for the thread.
|
||||
*
|
||||
@ -1031,6 +1036,10 @@ crypto_done(struct cryptop *crp)
|
||||
* callback routine does very little (e.g. the
|
||||
* /dev/crypto callback method just does a wakeup).
|
||||
*/
|
||||
mutex_spin_enter(&crypto_mtx);
|
||||
crp->crp_flags |= CRYPTO_F_DONE;
|
||||
mutex_spin_exit(&crypto_mtx);
|
||||
|
||||
#ifdef CRYPTO_TIMING
|
||||
if (crypto_timing) {
|
||||
/*
|
||||
@ -1047,14 +1056,27 @@ crypto_done(struct cryptop *crp)
|
||||
crp->crp_callback(crp);
|
||||
} else {
|
||||
mutex_spin_enter(&crypto_mtx);
|
||||
wasempty = TAILQ_EMPTY(&crp_ret_q);
|
||||
DPRINTF(("crypto_done: queueing %08x\n", (uint32_t)crp));
|
||||
crp->crp_flags |= CRYPTO_F_ONRETQ;
|
||||
TAILQ_INSERT_TAIL(&crp_ret_q, crp, crp_next);
|
||||
if (wasempty) {
|
||||
DPRINTF(("crypto_done: waking cryptoret, %08x " \
|
||||
"hit empty queue\n.", (uint32_t)crp));
|
||||
cv_signal(&cryptoret_cv);
|
||||
crp->crp_flags |= CRYPTO_F_DONE;
|
||||
|
||||
if (crp->crp_flags & CRYPTO_F_USER) {
|
||||
/* the request has completed while
|
||||
* running in the user context
|
||||
* so don't queue it - the user
|
||||
* thread won't sleep when it sees
|
||||
* the CRYPTO_F_DONE flag.
|
||||
* This is an optimization to avoid
|
||||
* unecessary context switches.
|
||||
*/
|
||||
} else {
|
||||
wasempty = TAILQ_EMPTY(&crp_ret_q);
|
||||
DPRINTF(("crypto_done: queueing %08x\n", (uint32_t)crp));
|
||||
crp->crp_flags |= CRYPTO_F_ONRETQ;
|
||||
TAILQ_INSERT_TAIL(&crp_ret_q, crp, crp_next);
|
||||
if (wasempty) {
|
||||
DPRINTF(("crypto_done: waking cryptoret, %08x " \
|
||||
"hit empty queue\n.", (uint32_t)crp));
|
||||
cv_signal(&cryptoret_cv);
|
||||
}
|
||||
}
|
||||
mutex_spin_exit(&crypto_mtx);
|
||||
}
|
||||
@ -1126,12 +1148,11 @@ out:
|
||||
static void
|
||||
cryptointr(void)
|
||||
{
|
||||
struct cryptop *crp, *submit;
|
||||
struct cryptkop *krp;
|
||||
struct cryptop *crp, *submit, *cnext;
|
||||
struct cryptkop *krp, *knext;
|
||||
struct cryptocap *cap;
|
||||
int result, hint;
|
||||
|
||||
printf("crypto softint\n");
|
||||
cryptostats.cs_intrs++;
|
||||
mutex_spin_enter(&crypto_mtx);
|
||||
do {
|
||||
@ -1142,7 +1163,7 @@ cryptointr(void)
|
||||
*/
|
||||
submit = NULL;
|
||||
hint = 0;
|
||||
TAILQ_FOREACH(crp, &crp_q, crp_next) {
|
||||
TAILQ_FOREACH_SAFE(crp, &crp_q, crp_next, cnext) {
|
||||
u_int32_t hid = SESID2HID(crp->crp_sid);
|
||||
cap = crypto_checkdriver(hid);
|
||||
if (cap == NULL || cap->cc_process == NULL) {
|
||||
@ -1197,7 +1218,7 @@ cryptointr(void)
|
||||
}
|
||||
|
||||
/* As above, but for key ops */
|
||||
TAILQ_FOREACH(krp, &crp_kq, krp_next) {
|
||||
TAILQ_FOREACH_SAFE(krp, &crp_kq, krp_next, knext) {
|
||||
cap = crypto_checkdriver(krp->krp_hid);
|
||||
if (cap == NULL || cap->cc_kprocess == NULL) {
|
||||
/* Op needs to be migrated, process it. */
|
||||
|
||||
@ -1,4 +1,4 @@
|
||||
/* $NetBSD: cryptodev.c,v 1.44 2008/05/24 16:42:00 christos Exp $ */
|
||||
/* $NetBSD: cryptodev.c,v 1.45 2008/11/18 12:59:58 darran Exp $ */
|
||||
/* $FreeBSD: src/sys/opencrypto/cryptodev.c,v 1.4.2.4 2003/06/03 00:09:02 sam Exp $ */
|
||||
/* $OpenBSD: cryptodev.c,v 1.53 2002/07/10 22:21:30 mickey Exp $ */
|
||||
|
||||
@ -64,7 +64,7 @@
|
||||
*/
|
||||
|
||||
#include <sys/cdefs.h>
|
||||
__KERNEL_RCSID(0, "$NetBSD: cryptodev.c,v 1.44 2008/05/24 16:42:00 christos Exp $");
|
||||
__KERNEL_RCSID(0, "$NetBSD: cryptodev.c,v 1.45 2008/11/18 12:59:58 darran Exp $");
|
||||
|
||||
#include <sys/param.h>
|
||||
#include <sys/systm.h>
|
||||
@ -452,7 +452,12 @@ cryptodev_op(struct csession *cse, struct crypt_op *cop, struct lwp *l)
|
||||
}
|
||||
|
||||
crp->crp_ilen = cop->len;
|
||||
crp->crp_flags = CRYPTO_F_IOV | (cop->flags & COP_F_BATCH);
|
||||
/* The reqest is flagged as CRYPTO_F_USER as long as it is running
|
||||
* in the user IOCTL thread. This flag lets us skip using the retq for
|
||||
* the request if it completes immediately. If the request ends up being
|
||||
* delayed or is not completed immediately the flag is removed.
|
||||
*/
|
||||
crp->crp_flags = CRYPTO_F_IOV | (cop->flags & COP_F_BATCH) | CRYPTO_F_USER;
|
||||
crp->crp_buf = (void *)&cse->uio;
|
||||
crp->crp_callback = (int (*) (struct cryptop *)) cryptodev_cb;
|
||||
crp->crp_sid = cse->sid;
|
||||
@ -507,6 +512,14 @@ eagain:
|
||||
error = crypto_dispatch(crp);
|
||||
mutex_spin_enter(&crypto_mtx);
|
||||
|
||||
/*
|
||||
* If the request was going to be completed by the
|
||||
* ioctl thread then it would have been done by now.
|
||||
* Remove the F_USER flag it so crypto_done() is not confused
|
||||
* if the crypto device calls it after this point.
|
||||
*/
|
||||
crp->crp_flags &= ~(CRYPTO_F_USER);
|
||||
|
||||
switch (error) {
|
||||
#ifdef notyet /* don't loop forever -- but EAGAIN not possible here yet */
|
||||
case EAGAIN:
|
||||
@ -521,13 +534,16 @@ eagain:
|
||||
mutex_spin_exit(&crypto_mtx);
|
||||
goto bail;
|
||||
}
|
||||
|
||||
|
||||
while (!(crp->crp_flags & CRYPTO_F_DONE)) {
|
||||
DPRINTF(("cryptodev_op: sleeping on cv %08x for crp %08x\n", \
|
||||
(uint32_t)&crp->crp_cv, (uint32_t)crp));
|
||||
cv_wait(&crp->crp_cv, &crypto_mtx); /* XXX cv_wait_sig? */
|
||||
}
|
||||
if (crp->crp_flags & CRYPTO_F_ONRETQ) {
|
||||
/* XXX this should never happen now with the CRYPTO_F_USER flag
|
||||
* changes.
|
||||
*/
|
||||
DPRINTF(("cryptodev_op: DONE, not woken by cryptoret.\n"));
|
||||
(void)crypto_ret_q_remove(crp);
|
||||
}
|
||||
@ -559,8 +575,9 @@ eagain:
|
||||
}
|
||||
|
||||
bail:
|
||||
if (crp)
|
||||
if (crp) {
|
||||
crypto_freereq(crp);
|
||||
}
|
||||
if (cse->uio.uio_iov[0].iov_base)
|
||||
kmem_free(cse->uio.uio_iov[0].iov_base,
|
||||
cse->uio.uio_iov[0].iov_len);
|
||||
@ -771,6 +788,7 @@ fail:
|
||||
kmem_free(kp->crp_p, size);
|
||||
}
|
||||
}
|
||||
cv_destroy(&krp->krp_cv);
|
||||
pool_put(&cryptkop_pool, krp);
|
||||
DPRINTF(("cryptodev_key: error=0x%08x\n", error));
|
||||
return error;
|
||||
@ -800,10 +818,10 @@ cryptof_close(struct file *fp)
|
||||
static struct csession *
|
||||
csefind(struct fcrypt *fcr, u_int ses)
|
||||
{
|
||||
struct csession *cse, *ret = NULL;
|
||||
struct csession *cse, *cnext, *ret = NULL;
|
||||
|
||||
KASSERT(mutex_owned(&crypto_mtx));
|
||||
TAILQ_FOREACH(cse, &fcr->csessions, next)
|
||||
TAILQ_FOREACH_SAFE(cse, &fcr->csessions, next, cnext)
|
||||
if (cse->ses == ses)
|
||||
ret = cse;
|
||||
|
||||
@ -814,11 +832,11 @@ csefind(struct fcrypt *fcr, u_int ses)
|
||||
static int
|
||||
csedelete(struct fcrypt *fcr, struct csession *cse_del)
|
||||
{
|
||||
struct csession *cse;
|
||||
struct csession *cse, *cnext;
|
||||
int ret = 0;
|
||||
|
||||
KASSERT(mutex_owned(&crypto_mtx));
|
||||
TAILQ_FOREACH(cse, &fcr->csessions, next) {
|
||||
TAILQ_FOREACH_SAFE(cse, &fcr->csessions, next, cnext) {
|
||||
if (cse == cse_del) {
|
||||
TAILQ_REMOVE(&fcr->csessions, cse, next);
|
||||
ret = 1;
|
||||
@ -1274,6 +1292,7 @@ fail:
|
||||
kmem_free(kp->crp_p, size);
|
||||
}
|
||||
}
|
||||
cv_destroy(&krp->krp_cv);
|
||||
pool_put(&cryptkop_pool, krp);
|
||||
}
|
||||
}
|
||||
@ -1284,7 +1303,8 @@ fail:
|
||||
}
|
||||
|
||||
static int
|
||||
cryptodev_session(struct fcrypt *fcr, struct session_op *sop) {
|
||||
cryptodev_session(struct fcrypt *fcr, struct session_op *sop)
|
||||
{
|
||||
struct cryptoini cria, crie;
|
||||
struct enc_xform *txform = NULL;
|
||||
struct auth_hash *thash = NULL;
|
||||
@ -1616,6 +1636,7 @@ fail:
|
||||
}
|
||||
}
|
||||
SLIST_REMOVE_HEAD(&krp_delfree_l, krp_qun.krp_lnext);
|
||||
cv_destroy(&krp->krp_cv);
|
||||
pool_put(&cryptkop_pool, krp);
|
||||
}
|
||||
return completed;
|
||||
@ -1624,15 +1645,15 @@ fail:
|
||||
static int
|
||||
cryptodev_getstatus (struct fcrypt *fcr, struct crypt_result *crypt_res)
|
||||
{
|
||||
struct cryptop *crp = NULL;
|
||||
struct cryptkop *krp = NULL;
|
||||
struct cryptop *crp = NULL, *cnext;
|
||||
struct cryptkop *krp = NULL, *knext;
|
||||
struct csession *cse;
|
||||
int i, size, req = 0;
|
||||
|
||||
mutex_spin_enter(&crypto_mtx);
|
||||
/* Here we dont know for which request the user is requesting the
|
||||
* response so checking in both the queues */
|
||||
TAILQ_FOREACH(crp, &fcr->crp_ret_mq, crp_next) {
|
||||
TAILQ_FOREACH_SAFE(crp, &fcr->crp_ret_mq, crp_next, cnext) {
|
||||
if(crp && (crp->crp_reqid == crypt_res->reqid)) {
|
||||
cse = (struct csession *)crp->crp_opaque;
|
||||
crypt_res->opaque = crp->crp_usropaque;
|
||||
@ -1671,7 +1692,7 @@ bail:
|
||||
}
|
||||
}
|
||||
|
||||
TAILQ_FOREACH(krp, &fcr->crp_ret_mkq, krp_next) {
|
||||
TAILQ_FOREACH_SAFE(krp, &fcr->crp_ret_mkq, krp_next, knext) {
|
||||
if(krp && (krp->krp_reqid == crypt_res->reqid)) {
|
||||
crypt_res[req].opaque = krp->krp_usropaque;
|
||||
if (krp->krp_status != 0) {
|
||||
@ -1712,6 +1733,7 @@ fail:
|
||||
kmem_free(kp->crp_p, size);
|
||||
}
|
||||
}
|
||||
cv_destroy(&krp->krp_cv);
|
||||
pool_put(&cryptkop_pool, krp);
|
||||
return 0;
|
||||
}
|
||||
|
||||
@ -1,4 +1,4 @@
|
||||
/* $NetBSD: cryptodev.h,v 1.14 2008/04/28 20:24:10 martin Exp $ */
|
||||
/* $NetBSD: cryptodev.h,v 1.15 2008/11/18 12:59:58 darran 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 $ */
|
||||
|
||||
@ -441,7 +441,7 @@ struct cryptop {
|
||||
* should always check and use the new
|
||||
* value on future requests.
|
||||
*/
|
||||
int crp_flags;
|
||||
int crp_flags; /* Note: must hold mutext to modify */
|
||||
|
||||
#define CRYPTO_F_IMBUF 0x0001 /* Input/output are mbuf chains */
|
||||
#define CRYPTO_F_IOV 0x0002 /* Input/output are uio */
|
||||
@ -451,6 +451,7 @@ struct cryptop {
|
||||
#define CRYPTO_F_DONE 0x0020 /* Operation completed */
|
||||
#define CRYPTO_F_CBIFSYNC 0x0040 /* Do CBIMM if op is synchronous */
|
||||
#define CRYPTO_F_ONRETQ 0x0080 /* Request is on return queue */
|
||||
#define CRYPTO_F_USER 0x0100 /* Request is in user context */
|
||||
|
||||
void * crp_buf; /* Data to be processed */
|
||||
void * crp_opaque; /* Opaque pointer, passed along */
|
||||
@ -590,7 +591,7 @@ extern int crypto_devallowsoft; /* only use hardware crypto */
|
||||
* Asymmetric operations are allocated in cryptodev.c but can be
|
||||
* freed in crypto.c.
|
||||
*/
|
||||
extern struct pool cryptkop_pool;
|
||||
extern struct pool cryptkop_pool;
|
||||
|
||||
/*
|
||||
* Mutual exclusion and its unwelcome friends.
|
||||
|
||||
@ -1,4 +1,4 @@
|
||||
/* $NetBSD: queue.h,v 1.49 2008/06/15 16:42:18 christos Exp $ */
|
||||
/* $NetBSD: queue.h,v 1.50 2008/11/18 12:59:58 darran Exp $ */
|
||||
|
||||
/*
|
||||
* Copyright (c) 1991, 1993
|
||||
@ -520,6 +520,11 @@ struct { \
|
||||
(var); \
|
||||
(var) = (*(((struct headname *)((var)->field.tqe_prev))->tqh_last)))
|
||||
|
||||
#define TAILQ_FOREACH_REVERSE_SAFE(var, head, headname, field, prev) \
|
||||
for ((var) = TAILQ_LAST((head), headname); \
|
||||
(var) && ((prev) = TAILQ_PREV((var), headname, field), 1);\
|
||||
(var) = (prev))
|
||||
|
||||
#define TAILQ_CONCAT(head1, head2, field) do { \
|
||||
if (!TAILQ_EMPTY(head2)) { \
|
||||
*(head1)->tqh_last = (head2)->tqh_first; \
|
||||
|
||||
Loading…
Reference in New Issue
Block a user