Hi, crypto_getreq() and crypto_freereq() don’t require kernel lock.
> On 16 Jun 2021, at 23:05, Alexander Bluhm <[email protected]> wrote: > > Hi, > > I have seen a kernel crash with while forwarding and encrypting > much traffic through OpenBSD IPsec gateways running iked. > > kernel: protection fault trap, code=0 > Stopped at aes_ctr_crypt+0x1e: addb $0x1,0x2e3(%rdi) > > ddb{2}> trace > aes_ctr_crypt(16367ed4be021a53,ffff8000246e1db0) at aes_ctr_crypt+0x1e > swcr_authenc(fffffd8132a21b08) at swcr_authenc+0x5c3 > swcr_process(fffffd8132a21b08) at swcr_process+0x1e8 > crypto_invoke(fffffd8132a21b08) at crypto_invoke+0xde > taskq_thread(ffff800000200500) at taskq_thread+0x81 > end trace frame: 0x0, count: -5 > > *64926 109760 0 0 7 0x14200 crypto > > swcr_authenc() passes swe->sw_kschedule to aes_ctr_crypt(), swe has > been freed and contains deaf006cdeaf4152, which looks like some > sort of poison. I suspect a use after free. > > The swe value comes from the swcr_sessions global pointers. Its > content looks sane in ddb. Noone touches it in swcr_authenc(). So > I guess that an other CPU changes the global structures while > swcr_authenc() is working with it. > > The crypto thread is protected by kernel lock, both network stack > and pfkey use net lock. The kernel lock has been recently removed > from pfkey. > > I think the required lock for the crypto framework is the kernel > lock. If crypto_ functions are called, IPsec must grab the kernel > lock. pfkey accesses crypto only via tdb_ functions, so this diff > also covers that case. > > ok? > > bluhm > > Index: netinet/ip_ah.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ah.c,v > retrieving revision 1.146 > diff -u -p -r1.146 ip_ah.c > --- netinet/ip_ah.c 25 Feb 2021 02:48:21 -0000 1.146 > +++ netinet/ip_ah.c 16 Jun 2021 17:29:37 -0000 > @@ -98,6 +98,7 @@ ah_init(struct tdb *tdbp, struct xformsw > { > struct auth_hash *thash = NULL; > struct cryptoini cria, crin; > + int error; > > /* Authentication operation. */ > switch (ii->ii_authalg) { > @@ -162,7 +163,10 @@ ah_init(struct tdb *tdbp, struct xformsw > cria.cri_next = &crin; > } > > - return crypto_newsession(&tdbp->tdb_cryptoid, &cria, 0); > + KERNEL_LOCK(); > + error = crypto_newsession(&tdbp->tdb_cryptoid, &cria, 0); > + KERNEL_UNLOCK(); > + return error; > } > > /* > @@ -171,7 +175,7 @@ ah_init(struct tdb *tdbp, struct xformsw > int > ah_zeroize(struct tdb *tdbp) > { > - int err; > + int error; > > if (tdbp->tdb_amxkey) { > explicit_bzero(tdbp->tdb_amxkey, tdbp->tdb_amxkeylen); > @@ -179,9 +183,11 @@ ah_zeroize(struct tdb *tdbp) > tdbp->tdb_amxkey = NULL; > } > > - err = crypto_freesession(tdbp->tdb_cryptoid); > + KERNEL_LOCK(); > + error = crypto_freesession(tdbp->tdb_cryptoid); > + KERNEL_UNLOCK(); > tdbp->tdb_cryptoid = 0; > - return err; > + return error; > } > > /* > @@ -626,7 +632,9 @@ ah_input(struct mbuf *m, struct tdb *tdb > } > > /* Get crypto descriptors. */ > + KERNEL_LOCK(); > crp = crypto_getreq(1); > + KERNEL_UNLOCK(); > if (crp == NULL) { > DPRINTF(("%s: failed to acquire crypto descriptors\n", > __func__)); > @@ -696,11 +704,16 @@ ah_input(struct mbuf *m, struct tdb *tdb > tc->tc_rdomain = tdb->tdb_rdomain; > memcpy(&tc->tc_dst, &tdb->tdb_dst, sizeof(union sockaddr_union)); > > - return crypto_dispatch(crp); > + KERNEL_LOCK(); > + error = crypto_dispatch(crp); > + KERNEL_UNLOCK(); > + return error; > > drop: > m_freem(m); > + KERNEL_LOCK(); > crypto_freereq(crp); > + KERNEL_UNLOCK(); > free(tc, M_XDATA, 0); > return error; > } > @@ -1047,7 +1060,9 @@ ah_output(struct mbuf *m, struct tdb *td > #endif > > /* Get crypto descriptors. */ > + KERNEL_LOCK(); > crp = crypto_getreq(1); > + KERNEL_UNLOCK(); > if (crp == NULL) { > DPRINTF(("%s: failed to acquire crypto descriptors\n", > __func__)); > @@ -1144,11 +1159,16 @@ ah_output(struct mbuf *m, struct tdb *td > tc->tc_rdomain = tdb->tdb_rdomain; > memcpy(&tc->tc_dst, &tdb->tdb_dst, sizeof(union sockaddr_union)); > > - return crypto_dispatch(crp); > + KERNEL_LOCK(); > + error = crypto_dispatch(crp); > + KERNEL_UNLOCK(); > + return error; > > drop: > m_freem(m); > + KERNEL_LOCK(); > crypto_freereq(crp); > + KERNEL_UNLOCK(); > free(tc, M_XDATA, 0); > return error; > } > Index: netinet/ip_esp.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_esp.c,v > retrieving revision 1.162 > diff -u -p -r1.162 ip_esp.c > --- netinet/ip_esp.c 25 Feb 2021 02:48:21 -0000 1.162 > +++ netinet/ip_esp.c 16 Jun 2021 17:29:55 -0000 > @@ -93,6 +93,7 @@ esp_init(struct tdb *tdbp, struct xforms > struct enc_xform *txform = NULL; > struct auth_hash *thash = NULL; > struct cryptoini cria, crie, crin; > + int error; > > if (!ii->ii_encalg && !ii->ii_authalg) { > DPRINTF(("esp_init(): neither authentication nor encryption " > @@ -294,8 +295,11 @@ esp_init(struct tdb *tdbp, struct xforms > cria.cri_key = ii->ii_authkey; > } > > - return crypto_newsession(&tdbp->tdb_cryptoid, > + KERNEL_LOCK(); > + error = crypto_newsession(&tdbp->tdb_cryptoid, > (tdbp->tdb_encalgxform ? &crie : &cria), 0); > + KERNEL_UNLOCK(); > + return error; > } > > /* > @@ -304,7 +308,7 @@ esp_init(struct tdb *tdbp, struct xforms > int > esp_zeroize(struct tdb *tdbp) > { > - int err; > + int error; > > if (tdbp->tdb_amxkey) { > explicit_bzero(tdbp->tdb_amxkey, tdbp->tdb_amxkeylen); > @@ -318,9 +322,11 @@ esp_zeroize(struct tdb *tdbp) > tdbp->tdb_emxkey = NULL; > } > > - err = crypto_freesession(tdbp->tdb_cryptoid); > + KERNEL_LOCK(); > + error = crypto_freesession(tdbp->tdb_cryptoid); > + KERNEL_UNLOCK(); > tdbp->tdb_cryptoid = 0; > - return err; > + return error; > } > > #define MAXBUFSIZ (AH_ALEN_MAX > ESP_MAX_IVS ? AH_ALEN_MAX : ESP_MAX_IVS) > @@ -438,7 +444,9 @@ esp_input(struct mbuf *m, struct tdb *td > } > > /* Get crypto descriptors */ > + KERNEL_LOCK(); > crp = crypto_getreq(esph && espx ? 2 : 1); > + KERNEL_UNLOCK(); > if (crp == NULL) { > DPRINTF(("%s: failed to acquire crypto descriptors\n", > __func__)); > espstat_inc(esps_crypto); > @@ -519,11 +527,16 @@ esp_input(struct mbuf *m, struct tdb *td > crde->crd_len = m->m_pkthdr.len - (skip + hlen + alen); > } > > - return crypto_dispatch(crp); > + KERNEL_LOCK(); > + error = crypto_dispatch(crp); > + KERNEL_UNLOCK(); > + return error; > > drop: > m_freem(m); > + KERNEL_LOCK(); > crypto_freereq(crp); > + KERNEL_UNLOCK(); > free(tc, M_XDATA, 0); > return error; > } > @@ -919,7 +932,9 @@ esp_output(struct mbuf *m, struct tdb *t > m_copyback(m, protoff, sizeof(u_int8_t), &prot, M_NOWAIT); > > /* Get crypto descriptors. */ > + KERNEL_LOCK(); > crp = crypto_getreq(esph && espx ? 2 : 1); > + KERNEL_UNLOCK(); > if (crp == NULL) { > DPRINTF(("%s: failed to acquire crypto descriptors\n", > __func__)); > @@ -1006,11 +1021,16 @@ esp_output(struct mbuf *m, struct tdb *t > crda->crd_len = m->m_pkthdr.len - (skip + alen); > } > > - return crypto_dispatch(crp); > + KERNEL_LOCK(); > + error = crypto_dispatch(crp); > + KERNEL_UNLOCK(); > + return error; > > drop: > m_freem(m); > + KERNEL_LOCK(); > crypto_freereq(crp); > + KERNEL_UNLOCK(); > free(tc, M_XDATA, 0); > return error; > } > Index: netinet/ip_ipcomp.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipcomp.c,v > retrieving revision 1.67 > diff -u -p -r1.67 ip_ipcomp.c > --- netinet/ip_ipcomp.c 30 Sep 2019 01:53:05 -0000 1.67 > +++ netinet/ip_ipcomp.c 16 Jun 2021 20:02:28 -0000 > @@ -79,6 +79,7 @@ ipcomp_init(struct tdb *tdbp, struct xfo > { > struct comp_algo *tcomp = NULL; > struct cryptoini cric; > + int error; > > switch (ii->ii_compalg) { > case SADB_X_CALG_DEFLATE: > @@ -105,7 +106,10 @@ ipcomp_init(struct tdb *tdbp, struct xfo > memset(&cric, 0, sizeof(cric)); > cric.cri_alg = tdbp->tdb_compalgxform->type; > > - return crypto_newsession(&tdbp->tdb_cryptoid, &cric, 0); > + KERNEL_LOCK(); > + error = crypto_newsession(&tdbp->tdb_cryptoid, &cric, 0); > + KERNEL_UNLOCK(); > + return error; > } > > /* > @@ -114,11 +118,13 @@ ipcomp_init(struct tdb *tdbp, struct xfo > int > ipcomp_zeroize(struct tdb *tdbp) > { > - int err; > + int error; > > - err = crypto_freesession(tdbp->tdb_cryptoid); > + KERNEL_LOCK(); > + error = crypto_freesession(tdbp->tdb_cryptoid); > + KERNEL_UNLOCK(); > tdbp->tdb_cryptoid = 0; > - return err; > + return error; > } > > /* > @@ -129,7 +135,7 @@ ipcomp_input(struct mbuf *m, struct tdb > { > struct comp_algo *ipcompx = (struct comp_algo *) tdb->tdb_compalgxform; > struct tdb_crypto *tc; > - int hlen; > + int hlen, error; > > struct cryptodesc *crdc = NULL; > struct cryptop *crp; > @@ -137,7 +143,9 @@ ipcomp_input(struct mbuf *m, struct tdb > hlen = IPCOMP_HLENGTH; > > /* Get crypto descriptors */ > + KERNEL_LOCK(); > crp = crypto_getreq(1); > + KERNEL_UNLOCK(); > if (crp == NULL) { > m_freem(m); > DPRINTF(("%s: failed to acquire crypto descriptors\n", > __func__)); > @@ -148,7 +156,9 @@ ipcomp_input(struct mbuf *m, struct tdb > tc = malloc(sizeof(*tc), M_XDATA, M_NOWAIT | M_ZERO); > if (tc == NULL) { > m_freem(m); > + KERNEL_LOCK(); > crypto_freereq(crp); > + KERNEL_UNLOCK(); > DPRINTF(("%s: failed to allocate tdb_crypto\n", __func__)); > ipcompstat_inc(ipcomps_crypto); > return ENOBUFS; > @@ -178,7 +188,10 @@ ipcomp_input(struct mbuf *m, struct tdb > tc->tc_rdomain = tdb->tdb_rdomain; > tc->tc_dst = tdb->tdb_dst; > > - return crypto_dispatch(crp); > + KERNEL_LOCK(); > + error = crypto_dispatch(crp); > + KERNEL_UNLOCK(); > + return error; > } > > int > @@ -431,7 +444,9 @@ ipcomp_output(struct mbuf *m, struct tdb > /* Ok now, we can pass to the crypto processing */ > > /* Get crypto descriptors */ > + KERNEL_LOCK(); > crp = crypto_getreq(1); > + KERNEL_UNLOCK(); > if (crp == NULL) { > DPRINTF(("%s: failed to acquire crypto descriptors\n", > __func__)); > ipcompstat_inc(ipcomps_crypto); > @@ -472,11 +487,16 @@ ipcomp_output(struct mbuf *m, struct tdb > crp->crp_opaque = (caddr_t)tc; > crp->crp_sid = tdb->tdb_cryptoid; > > - return crypto_dispatch(crp); > + KERNEL_LOCK(); > + error = crypto_dispatch(crp); > + KERNEL_UNLOCK(); > + return error; > > drop: > m_freem(m); > + KERNEL_LOCK(); > crypto_freereq(crp); > + KERNEL_UNLOCK(); > return error; > } > > Index: netinet/ipsec_input.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ipsec_input.c,v > retrieving revision 1.173 > diff -u -p -r1.173 ipsec_input.c > --- netinet/ipsec_input.c 1 Sep 2020 01:53:34 -0000 1.173 > +++ netinet/ipsec_input.c 16 Jun 2021 12:37:54 -0000 > @@ -376,6 +376,8 @@ ipsec_input_cb(struct cryptop *crp) > struct tdb *tdb = NULL; > int clen, error; > > + KERNEL_ASSERT_LOCKED(); > + > if (m == NULL) { > DPRINTF(("%s: bogus returned buffer from crypto\n", __func__)); > ipsecstat_inc(ipsec_crypto); > Index: netinet/ipsec_output.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ipsec_output.c,v > retrieving revision 1.79 > diff -u -p -r1.79 ipsec_output.c > --- netinet/ipsec_output.c 10 Mar 2021 10:21:49 -0000 1.79 > +++ netinet/ipsec_output.c 16 Jun 2021 12:37:53 -0000 > @@ -391,6 +391,8 @@ ipsec_output_cb(struct cryptop *crp) > struct tdb *tdb = NULL; > int error, ilen, olen; > > + KERNEL_ASSERT_LOCKED(); > + > if (m == NULL) { > DPRINTF(("%s: bogus returned buffer from crypto\n", __func__)); > ipsecstat_inc(ipsec_crypto); >
