From 887150002f94aee74032969844bb81f8b7f87fe3 Mon Sep 17 00:00:00 2001 From: k-nakahara Date: Fri, 2 Jun 2017 10:54:16 +0900 Subject: [PATCH 1/3] introduce localcount(9) to avoid to free using cryptocap --- sys/opencrypto/crypto.c | 196 ++++++++++++++++++++++++++++++++++++++------- sys/opencrypto/cryptodev.h | 5 ++ 2 files changed, 171 insertions(+), 30 deletions(-) diff --git a/sys/opencrypto/crypto.c b/sys/opencrypto/crypto.c index d0fe8c62a04..dde7707a07d 100644 --- a/sys/opencrypto/crypto.c +++ b/sys/opencrypto/crypto.c @@ -375,6 +375,9 @@ static int crypto_invoke(struct cryptop *crp, int hint); static int crypto_kinvoke(struct cryptkop *krp, int hint); static struct cryptocap *crypto_checkdriver(u_int32_t); +static struct cryptocap *crypto_checkdriver_ref(u_int32_t); +static void crypto_driver_ref(struct cryptocap *); +static void crypto_driver_unref(struct cryptocap *); static struct cryptostats cryptostats; #ifdef CRYPTO_TIMING @@ -449,10 +452,13 @@ crypto_destroy(bool exit_kthread) cap = crypto_checkdriver(i); if (cap == NULL) continue; + crypto_driver_ref(cap); if (cap->cc_sessions != 0) break; + crypto_driver_unref(cap); } if (cap != NULL) { + crypto_driver_unref(cap); mutex_spin_exit(&crypto_ret_q_mtx); return EBUSY; } @@ -515,20 +521,28 @@ crypto_newsession(u_int64_t *sid, struct cryptoini *cri, int hard) if (cap == NULL) continue; + crypto_driver_ref(cap); + /* * If it's not initialized or has remaining sessions * referencing it, skip. */ if (cap->cc_newsession == NULL || - (cap->cc_flags & CRYPTOCAP_F_CLEANUP)) + (cap->cc_flags & CRYPTOCAP_F_CLEANUP)) { + crypto_driver_unref(cap); continue; + } /* Hardware required -- ignore software drivers. */ - if (hard > 0 && (cap->cc_flags & CRYPTOCAP_F_SOFTWARE)) + if (hard > 0 && (cap->cc_flags & CRYPTOCAP_F_SOFTWARE)) { + crypto_driver_unref(cap); continue; + } /* Software required -- ignore hardware drivers. */ - if (hard < 0 && (cap->cc_flags & CRYPTOCAP_F_SOFTWARE) == 0) + if (hard < 0 && (cap->cc_flags & CRYPTOCAP_F_SOFTWARE) == 0) { + crypto_driver_unref(cap); continue; + } /* See if all the algorithms are supported. */ for (cr = cri; cr; cr = cr->cri_next) @@ -581,12 +595,15 @@ crypto_freesession(u_int64_t sid) mutex_enter(&crypto_drv_mtx); /* Determine two IDs. */ + /* avoid to clear cryptocap before cap->cc_freesession() */ cap = crypto_checkdriver(CRYPTO_SESID2HID(sid)); if (cap == NULL) { err = ENOENT; goto done; } + /* must hold referenced by crypto_newsession() */ + if (cap->cc_sessions) (cap->cc_sessions)--; @@ -596,6 +613,9 @@ crypto_freesession(u_int64_t sid) else err = 0; + /* release reference which acquired by crypto_newsession() */ + crypto_driver_unref(cap); + /* * If this was the last session of a driver marked as invalid, * make the entry available for reuse. @@ -664,6 +684,9 @@ crypto_get_driverid(u_int32_t flags) /* NB: state is zero'd on free */ cap->cc_sessions = 1; /* Mark */ cap->cc_flags = flags; + localcount_init(&cap->cc_ref); + cv_init(&cap->cc_cv, "cryptocap"); + mutex_init(&cap->cc_lock, MUTEX_DEFAULT, IPL_NET); if (bootverbose) printf("crypto: assign driver %u, flags %u\n", i, flags); @@ -673,14 +696,71 @@ crypto_get_driverid(u_int32_t flags) return i; } +/* + * Below functions can access uninitialized cryptocap. + * - crypto_get_driverid() + * - crypto_newsession() + * - crypto_kinvoke() + * - crypto_getfeat() + * - crypto_destroy() + * So, they cannot get reference to localcount before NULL check. + */ static struct cryptocap * crypto_checkdriver(u_int32_t hid) { + if (crypto_drivers == NULL) return NULL; return (hid >= crypto_drivers_num ? NULL : &crypto_drivers[hid]); } +static struct cryptocap * +crypto_checkdriver_ref(u_int32_t hid) +{ + struct cryptocap *cap; + + if (crypto_drivers == NULL) + return NULL; + + if (hid >= crypto_drivers_num) + return NULL; + + cap = &crypto_drivers[hid]; + /* + * sanity check. + * This check means the "cap" is done crypto_get_driverid() and + * crypto_register(), therefore, the "cap" is not done + * crypto_register() yet. + */ + if (cap->cc_process == NULL && cap->cc_sessions == 0) + return NULL; + + crypto_driver_ref(cap); + return cap; +} + +static void +crypto_driver_ref(struct cryptocap *cap) +{ + + if (cap == NULL) + return; + + DPRINTF("acquire ref of drivers[%u] addr=%p\n", hid, &crypto_drivers[hid]); + localcount_acquire(&cap->cc_ref); +} + +static void +crypto_driver_unref(struct cryptocap *cap) +{ + + if (cap == NULL) + return; + + DPRINTF("release ref of drivers[] addr=%p\n", cap); + localcount_release(&cap->cc_ref, &cap->cc_cv, &cap->cc_lock); +} + /* * Register support for a key-related algorithm. This routine * is called once for each algorithm supported a driver. @@ -696,8 +776,13 @@ crypto_kregister(u_int32_t driverid, int kalg, u_int32_t flags, mutex_enter(&crypto_drv_mtx); cap = crypto_checkdriver(driverid); - if (cap != NULL && - (CRK_ALGORITM_MIN <= kalg && kalg <= CRK_ALGORITHM_MAX)) { + if (cap == NULL) { + return EINVAL; + mutex_exit(&crypto_drv_mtx); + } + crypto_driver_ref(cap); + + if ((CRK_ALGORITM_MIN <= kalg && kalg <= CRK_ALGORITHM_MAX)) { /* * XXX Do some performance testing to determine placing. * XXX We probably need an auxiliary data structure that @@ -722,6 +807,7 @@ crypto_kregister(u_int32_t driverid, int kalg, u_int32_t flags, } else err = EINVAL; + crypto_driver_unref(cap); mutex_exit(&crypto_drv_mtx); return err; } @@ -744,9 +830,14 @@ crypto_register(u_int32_t driverid, int alg, u_int16_t maxoplen, mutex_enter(&crypto_drv_mtx); cap = crypto_checkdriver(driverid); + if (cap == NULL) { + mutex_exit(&crypto_drv_mtx); + return EINVAL; + } + crypto_driver_ref(cap); + /* NB: algorithms are in the range [1..max] */ - if (cap != NULL && - (CRYPTO_ALGORITHM_MIN <= alg && alg <= CRYPTO_ALGORITHM_MAX)) { + if ((CRYPTO_ALGORITHM_MIN <= alg && alg <= CRYPTO_ALGORITHM_MAX)) { /* * XXX Do some performance testing to determine placing. * XXX We probably need an auxiliary data structure that @@ -776,6 +867,7 @@ crypto_register(u_int32_t driverid, int alg, u_int16_t maxoplen, } else err = EINVAL; + crypto_driver_unref(cap); mutex_exit(&crypto_drv_mtx); return err; } @@ -793,10 +885,15 @@ crypto_unregister_locked(u_int32_t driverid, int alg, bool all) if (alg < CRYPTO_ALGORITHM_MIN || CRYPTO_ALGORITHM_MAX < alg) return EINVAL; - cap = crypto_checkdriver(driverid); - if (cap == NULL || (!all && cap->cc_alg[alg] == 0)) + cap = crypto_checkdriver_ref(driverid); + if (cap == NULL) return EINVAL; + if (!all && cap->cc_alg[alg] == 0) { + crypto_driver_unref(cap); + return EINVAL; + } + cap->cc_alg[alg] = 0; cap->cc_max_op_len[alg] = 0; @@ -811,7 +908,16 @@ crypto_unregister_locked(u_int32_t driverid, int alg, bool all) break; } } + crypto_driver_unref(cap); + if (lastalg) { + mutex_enter(&cap->cc_lock); + mutex_exit(&crypto_drv_mtx); /* XXX */ + localcount_drain(&cap->cc_ref, &cap->cc_cv, &cap->cc_lock); + mutex_exit(&cap->cc_lock); + localcount_fini(&cap->cc_ref); + mutex_destroy(&cap->cc_lock); + ses = cap->cc_sessions; memset(cap, 0, sizeof(struct cryptocap)); if (ses != 0) { @@ -821,6 +927,7 @@ crypto_unregister_locked(u_int32_t driverid, int alg, bool all) cap->cc_flags |= CRYPTOCAP_F_CLEANUP; cap->cc_sessions = ses; } + mutex_enter(&crypto_drv_mtx); /* XXX */ } return 0; @@ -879,11 +986,10 @@ crypto_unblock(u_int32_t driverid, int what) mutex_spin_enter(&crypto_q_mtx); cap = crypto_checkdriver(driverid); - if (cap == NULL) { - mutex_spin_exit(&crypto_q_mtx); - return EINVAL; - } + KASSERTMSG(cap != NULL, + "You have not registered yet or already unregistered."); + crypto_driver_ref(cap); if (what & CRYPTO_SYMQ) { needwakeup |= cap->cc_qblocked; cap->cc_qblocked = 0; @@ -892,6 +998,8 @@ crypto_unblock(u_int32_t driverid, int what) needwakeup |= cap->cc_kqblocked; cap->cc_kqblocked = 0; } + crypto_driver_unref(cap); + mutex_spin_exit(&crypto_q_mtx); if (needwakeup) setsoftcrypto(softintr_cookie); @@ -939,7 +1047,7 @@ crypto_dispatch(struct cryptop *crp) mutex_spin_enter(&crypto_q_mtx); - cap = crypto_checkdriver(CRYPTO_SESID2HID(crp->crp_sid)); + cap = crypto_checkdriver_ref(CRYPTO_SESID2HID(crp->crp_sid)); /* * TODO: * If we can ensure the driver has been valid until the driver is @@ -968,6 +1076,7 @@ crypto_dispatch(struct cryptop *crp) */ TAILQ_INSERT_TAIL(&crp_q, crp, crp_next); mutex_spin_exit(&crypto_q_mtx); + /* keep cryptocap reference for this crp */ return 0; } @@ -997,6 +1106,13 @@ crypto_dispatch(struct cryptop *crp) * not return error. */ result = 0; + /* keep cryptocap reference for this crp */ + } else { + /* + * crypto_done() and cryptoret() does not touch cryptocap. + * so, decrement cryptocap reference. + */ + crypto_driver_unref(cap); } return result; @@ -1017,7 +1133,7 @@ crypto_kdispatch(struct cryptkop *krp) mutex_spin_enter(&crypto_q_mtx); cryptostats.cs_kops++; - cap = crypto_checkdriver(krp->krp_hid); + cap = crypto_checkdriver_ref(krp->krp_hid); /* * TODO: * If we can ensure the driver has been valid until the driver is @@ -1037,6 +1153,7 @@ crypto_kdispatch(struct cryptkop *krp) */ TAILQ_INSERT_TAIL(&crp_kq, krp, krp_next); mutex_spin_exit(&crypto_q_mtx); + /* keep cryptocap reference for this krp */ return 0; } @@ -1061,6 +1178,13 @@ crypto_kdispatch(struct cryptkop *krp) * not return error. */ result = 0; + /* keep cryptocap reference for this krp */ + } else { + /* + * crypto_kdone() and cryptoret() does not touch cryptocap. + * so, decrement cryptocap reference. + */ + crypto_driver_unref(cap); } return result; @@ -1086,20 +1210,27 @@ crypto_kinvoke(struct cryptkop *krp, int hint) } mutex_enter(&crypto_drv_mtx); - for (hid = 0; hid < crypto_drivers_num; hid++) { - cap = crypto_checkdriver(hid); - if (cap == NULL) - continue; - if ((cap->cc_flags & CRYPTOCAP_F_SOFTWARE) && - crypto_devallowsoft == 0) - continue; - if (cap->cc_kprocess == NULL) - continue; - if ((cap->cc_kalg[krp->krp_op] & - CRYPTO_ALG_FLAG_SUPPORTED) == 0) - continue; - break; - } + cap = crypto_checkdriver_ref(krp->krp_hid); + if (cap == NULL) { + for (hid = 0; hid < crypto_drivers_num; hid++) { + cap = crypto_checkdriver(hid); + if (cap == NULL) + continue; + if ((cap->cc_flags & CRYPTOCAP_F_SOFTWARE) && + crypto_devallowsoft == 0) + continue; + if (cap->cc_kprocess == NULL) + continue; + if ((cap->cc_kalg[krp->krp_op] & + CRYPTO_ALG_FLAG_SUPPORTED) == 0) + continue; + break; + } + if (cap != NULL) { + crypto_driver_ref(cap); + krp->krp_hid = hid; + } + } /* else, divert reference got in crypto_dispatch() */ if (cap != NULL) { int (*process)(void *, struct cryptkop *, int); void *arg; @@ -1107,8 +1238,8 @@ crypto_kinvoke(struct cryptkop *krp, int hint) process = cap->cc_kprocess; arg = cap->cc_karg; mutex_exit(&crypto_drv_mtx); - krp->krp_hid = hid; error = (*process)(arg, krp, hint); + crypto_driver_unref(cap); } else { mutex_exit(&crypto_drv_mtx); error = ENODEV; @@ -1169,6 +1300,7 @@ crypto_invoke(struct cryptop *crp, int hint) return 0; } + /* divert reference got in crypto_dispatch() */ cap = crypto_checkdriver(CRYPTO_SESID2HID(crp->crp_sid)); if (cap != NULL && (cap->cc_flags & CRYPTOCAP_F_CLEANUP) == 0) { int (*process)(void *, struct cryptop *, int); @@ -1492,6 +1624,7 @@ cryptointr(void) TAILQ_FOREACH_SAFE(crp, &crp_q, crp_next, cnext) { u_int32_t hid = CRYPTO_SESID2HID(crp->crp_sid); cap = crypto_checkdriver(hid); + /* divert ref got in crypto_dispatch() */ if (cap == NULL || cap->cc_process == NULL) { /* Op needs to be migrated, process it. */ submit = crp; @@ -1547,6 +1680,7 @@ cryptointr(void) * it at the end does not work. */ /* validate sid again */ + /* divert ref got in crypto_dispatch() */ cap = crypto_checkdriver(CRYPTO_SESID2HID(submit->crp_sid)); if (cap == NULL) { /* migrate again, sigh... */ @@ -1561,6 +1695,7 @@ cryptointr(void) /* As above, but for key ops */ TAILQ_FOREACH_SAFE(krp, &crp_kq, krp_next, knext) { + /* divert ref got in crypto_kdispatch() */ cap = crypto_checkdriver(krp->krp_hid); if (cap == NULL || cap->cc_kprocess == NULL) { /* Op needs to be migrated, process it. */ @@ -1586,6 +1721,7 @@ cryptointr(void) * it at the end does not work. */ /* validate sid again */ + /* divert reference got in crypto_dispatch() */ cap = crypto_checkdriver(krp->krp_hid); if (cap == NULL) { /* migrate again, sigh... */ diff --git a/sys/opencrypto/cryptodev.h b/sys/opencrypto/cryptodev.h index cf86edfb38b..95a3af10ccf 100644 --- a/sys/opencrypto/cryptodev.h +++ b/sys/opencrypto/cryptodev.h @@ -88,6 +88,7 @@ #include #include #include +#include #if defined(_KERNEL_OPT) #include "opt_ocf.h" @@ -560,6 +561,10 @@ struct cryptocap { int (*cc_freesession) (void*, u_int64_t); void *cc_karg; /* callback argument */ int (*cc_kprocess) (void*, struct cryptkop *, int); + + kmutex_t cc_lock; + struct localcount cc_ref; + kcondvar_t cc_cv; }; /* -- 2.11.0