Date: Thu, 8 Jan 2009 05:16:23 GMT From: Patrick Lamaiziere <patfbsd@davenulle.org> To: freebsd-gnats-submit@FreeBSD.org Subject: kern/130286: [patch] hifn(4) changes Message-ID: <200901080516.n085GNsW095581@www.freebsd.org> Resent-Message-ID: <200901080520.n085K2OZ001366@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 130286 >Category: kern >Synopsis: [patch] hifn(4) changes >Confidential: no >Severity: non-critical >Priority: low >Responsible: freebsd-bugs >State: open >Quarter: >Keywords: >Date-Required: >Class: change-request >Submitter-Id: current-users >Arrival-Date: Thu Jan 08 05:20:01 UTC 2009 >Closed-Date: >Last-Modified: >Originator: Patrick Lamaiziere >Release: 8.0-CURRENT/i386 >Organization: >Environment: FreeBSD malpractice.lamaiziere.net 8.0-CURRENT FreeBSD 8.0-CURRENT #5: Mon Dec 29 21:20:22 CET 2008 root@:/usr/obj/usr/src/sys/NET5501 i386 >Description: I would like to suggest few small changes in the hifn(4) driver: Please see the attached patch. Description. The main change is the use of a rwlock (sc->sc_sessions_lock) to protect the sessions. We don't need to use the main driver lock for this. While i'm here, fix small others problems. hifn_attach() - Useless bzero of softc hifn_detach() - Return EBUSY if there is active session - Free the sessions - Use callout_drain() instead callout_stop() hifn_rng() - Remove the unused macro RANDOM_BITS hifn_newsession() - Add and use a rwlock to lock the sessions (sc->sc_sessions_lock). - Remove useless bzero of the sessions (use malloc with M_ZERO) - Use arc4rand() instead read_random() hifn_freesession() - Use a rwlock to lock the sessions hifn_process() - Use a rwlock to lock the sessions. In the current driver there is no lock to protect the sessions in hifn_process() but the sessions can be reallocated in hifn_newsession(). I think this is the cause of PR kern/91407: http://www.FreeBSD.org/cgi/query-pr.cgi?pr=kern/91407 Anyway we should protect the sessions in hifn_process(). hifn_callback() - Use a rwlock to lock the sessions. I've tested on a Soekris vpn 1411 (hifn 7955) with ipsec and openssl. Regards. (with big thanks to Mike Tancsa) >How-To-Repeat: >Fix: Patch attached with submission follows: --- /home/patrick/src/sys/dev/hifn/hifn7751.c 2007-03-21 04:42:49.000000000 +0100 +++ /usr/src/sys/dev/hifn/hifn7751.c 2009-01-01 02:46:18.000000000 +0100 @@ -58,6 +58,7 @@ __FBSDID("$FreeBSD: src/sys/dev/hifn/hif #include <sys/mbuf.h> #include <sys/lock.h> #include <sys/mutex.h> +#include <sys/rwlock.h> #include <sys/sysctl.h> #include <vm/vm.h> @@ -362,9 +363,9 @@ hifn_attach(device_t dev) u_int16_t ena, rev; KASSERT(sc != NULL, ("hifn_attach: null software carrier!")); - bzero(sc, sizeof (*sc)); sc->sc_dev = dev; + rw_init(&sc->sc_sessions_lock, device_get_nameunit(dev)); mtx_init(&sc->sc_mtx, device_get_nameunit(dev), "hifn driver", MTX_DEF); /* XXX handle power management */ @@ -637,6 +638,7 @@ fail_io1: fail_io0: bus_release_resource(dev, SYS_RES_MEMORY, HIFN_BAR0, sc->sc_bar0res); fail_pci: + rw_destroy(&sc->sc_sessions_lock); mtx_destroy(&sc->sc_mtx); return (ENXIO); } @@ -648,15 +650,32 @@ static int hifn_detach(device_t dev) { struct hifn_softc *sc = device_get_softc(dev); + struct hifn_session *ses; + int i; KASSERT(sc != NULL, ("hifn_detach: null software carrier!")); + rw_wlock(&sc->sc_sessions_lock); + for (i = 0; i < sc->sc_nsessions; i++) { + ses = &sc->sc_sessions[i]; + if (ses->hs_used) { + rw_wunlock(&sc->sc_sessions_lock); + device_printf(dev, + "cannot detach, sessions still active.\n"); + return (EBUSY); + } + } + if (sc->sc_sessions != NULL) + free(sc->sc_sessions, M_DEVBUF); + rw_wunlock(&sc->sc_sessions_lock); + crypto_unregister_all(sc->sc_cid); + /* disable interrupts */ WRITE_REG_1(sc, HIFN_1_DMA_IER, 0); /*XXX other resources */ - callout_stop(&sc->sc_tickto); - callout_stop(&sc->sc_rngto); + callout_drain(&sc->sc_tickto); + callout_drain(&sc->sc_rngto); #ifdef HIFN_RNDTEST if (sc->sc_rndtest) rndtest_detach(sc->sc_rndtest); @@ -666,8 +685,6 @@ hifn_detach(device_t dev) WRITE_REG_1(sc, HIFN_1_DMA_CNFG, HIFN_DMACNFG_MSTRESET | HIFN_DMACNFG_DMARESET | HIFN_DMACNFG_MODE); - crypto_unregister_all(sc->sc_cid); - bus_generic_detach(dev); /*XXX should be no children, right? */ bus_teardown_intr(dev, sc->sc_irq, sc->sc_intrhand); @@ -682,6 +699,7 @@ hifn_detach(device_t dev) bus_release_resource(dev, SYS_RES_MEMORY, HIFN_BAR1, sc->sc_bar1res); bus_release_resource(dev, SYS_RES_MEMORY, HIFN_BAR0, sc->sc_bar0res); + rw_destroy(&sc->sc_sessions_lock); mtx_destroy(&sc->sc_mtx); return (0); @@ -817,7 +835,6 @@ hifn_init_pubrng(struct hifn_softc *sc) static void hifn_rng(void *vsc) { -#define RANDOM_BITS(n) (n)*sizeof (u_int32_t), (n)*sizeof (u_int32_t)*NBBY, 0 struct hifn_softc *sc = vsc; u_int32_t sts, num[2]; int i; @@ -2354,12 +2371,12 @@ hifn_newsession(device_t dev, u_int32_t if (sidp == NULL || cri == NULL || sc == NULL) return (EINVAL); - HIFN_LOCK(sc); + rw_wlock(&sc->sc_sessions_lock); if (sc->sc_sessions == NULL) { ses = sc->sc_sessions = (struct hifn_session *)malloc( - sizeof(*ses), M_DEVBUF, M_NOWAIT); + sizeof(*ses), M_DEVBUF, M_NOWAIT | M_ZERO); if (ses == NULL) { - HIFN_UNLOCK(sc); + rw_wunlock(&sc->sc_sessions_lock); return (ENOMEM); } sesn = 0; @@ -2375,9 +2392,9 @@ hifn_newsession(device_t dev, u_int32_t if (ses == NULL) { sesn = sc->sc_nsessions; ses = (struct hifn_session *)malloc((sesn + 1) * - sizeof(*ses), M_DEVBUF, M_NOWAIT); + sizeof(*ses), M_DEVBUF, M_NOWAIT | M_ZERO); if (ses == NULL) { - HIFN_UNLOCK(sc); + rw_wunlock(&sc->sc_sessions_lock); return (ENOMEM); } bcopy(sc->sc_sessions, ses, sesn * sizeof(*ses)); @@ -2388,9 +2405,7 @@ hifn_newsession(device_t dev, u_int32_t sc->sc_nsessions++; } } - HIFN_UNLOCK(sc); - bzero(ses, sizeof(*ses)); ses->hs_used = 1; for (c = cri; c != NULL; c = c->cri_next) { @@ -2399,8 +2414,10 @@ hifn_newsession(device_t dev, u_int32_t case CRYPTO_SHA1: case CRYPTO_MD5_HMAC: case CRYPTO_SHA1_HMAC: - if (mac) + if (mac) { + rw_wunlock(&sc->sc_sessions_lock); return (EINVAL); + } mac = 1; ses->hs_mlen = c->cri_mlen; if (ses->hs_mlen == 0) { @@ -2419,20 +2436,23 @@ hifn_newsession(device_t dev, u_int32_t case CRYPTO_DES_CBC: case CRYPTO_3DES_CBC: case CRYPTO_AES_CBC: - /* XXX this may read fewer, does it matter? */ - read_random(ses->hs_iv, - c->cri_alg == CRYPTO_AES_CBC ? - HIFN_AES_IV_LENGTH : HIFN_IV_LENGTH); + arc4rand(ses->hs_iv, + c->cri_alg == CRYPTO_AES_CBC ? + HIFN_AES_IV_LENGTH : HIFN_IV_LENGTH, 0); /*FALLTHROUGH*/ case CRYPTO_ARC4: - if (cry) + if (cry) { + rw_wunlock(&sc->sc_sessions_lock); return (EINVAL); + } cry = 1; break; default: + rw_wunlock(&sc->sc_sessions_lock); return (EINVAL); } } + rw_wunlock(&sc->sc_sessions_lock); if (mac == 0 && cry == 0) return (EINVAL); @@ -2457,14 +2477,14 @@ hifn_freesession(device_t dev, u_int64_t if (sc == NULL) return (EINVAL); - HIFN_LOCK(sc); session = HIFN_SESSION(sid); + rw_wlock(&sc->sc_sessions_lock); if (session < sc->sc_nsessions) { bzero(&sc->sc_sessions[session], sizeof(struct hifn_session)); error = 0; } else error = EINVAL; - HIFN_UNLOCK(sc); + rw_wunlock(&sc->sc_sessions_lock); return (error); } @@ -2483,11 +2503,17 @@ hifn_process(device_t dev, struct crypto } session = HIFN_SESSION(crp->crp_sid); - if (sc == NULL || session >= sc->sc_nsessions) { + if (sc == NULL) { err = EINVAL; goto errout; } - + rw_rlock(&sc->sc_sessions_lock); + if (session >= sc->sc_nsessions) { + rw_runlock(&sc->sc_sessions_lock); + err = EINVAL; + goto errout; + } + rw_runlock(&sc->sc_sessions_lock); cmd = malloc(sizeof(struct hifn_command), M_DEVBUF, M_NOWAIT | M_ZERO); if (cmd == NULL) { hifnstats.hst_nomem++; @@ -2597,10 +2623,12 @@ hifn_process(device_t dev, struct crypto if (enccrd->crd_flags & CRD_F_ENCRYPT) { if (enccrd->crd_flags & CRD_F_IV_EXPLICIT) bcopy(enccrd->crd_iv, cmd->iv, ivlen); - else + else { + rw_rlock(&sc->sc_sessions_lock); bcopy(sc->sc_sessions[session].hs_iv, cmd->iv, ivlen); - + rw_runlock(&sc->sc_sessions_lock); + } if ((enccrd->crd_flags & CRD_F_IV_PRESENT) == 0) { crypto_copyback(crp->crp_flags, @@ -2856,9 +2884,11 @@ hifn_callback(struct hifn_softc *sc, str continue; ivlen = ((crd->crd_alg == CRYPTO_AES_CBC) ? HIFN_AES_IV_LENGTH : HIFN_IV_LENGTH); + rw_rlock(&sc->sc_sessions_lock); crypto_copydata(crp->crp_flags, crp->crp_buf, crd->crd_skip + crd->crd_len - ivlen, ivlen, cmd->softc->sc_sessions[cmd->session_num].hs_iv); + rw_runlock(&sc->sc_sessions_lock); break; } } @@ -2873,7 +2903,9 @@ hifn_callback(struct hifn_softc *sc, str crd->crd_alg != CRYPTO_SHA1_HMAC) { continue; } + rw_rlock(&sc->sc_sessions_lock); len = cmd->softc->sc_sessions[cmd->session_num].hs_mlen; + rw_runlock(&sc->sc_sessions_lock); crypto_copyback(crp->crp_flags, crp->crp_buf, crd->crd_inject, len, macbuf); break; --- /home/patrick/src/sys/dev/hifn/hifn7751var.h 2007-03-21 04:42:49.000000000 +0100 +++ /usr/src/sys/dev/hifn/hifn7751var.h 2008-12-29 18:04:24.000000000 +0100 @@ -161,6 +161,7 @@ struct hifn_softc { int sc_maxses; int sc_nsessions; struct hifn_session *sc_sessions; + struct rwlock sc_sessions_lock;/* sessions lock */ int sc_ramsize; int sc_flags; #define HIFN_HAS_RNG 0x1 /* includes random number generator */ >Release-Note: >Audit-Trail: >Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200901080516.n085GNsW095581>