On Thu, Jun 17, 2021 at 10:09:47AM +0200, Martin Pieuchot wrote:
> It's not clear to me which field is the KERNEL_LOCK() protecting. Is it
> the access to `swcr_sessions'? Is it a reference? If so grabbing/releasing
> the lock might not be enough to fix the use-after-free.
I am running the fix for nearly a day now. Seems to be stable. I
guess the crash is a regression from unlocking the pfkey socket.
Rekeying had the kernel lock before, now it does not. Calling
crypto operations without kernel lock looks wrong. There is no
other locking in crypto.
> Could you annotate which field is being protected by the KERNEL_LOCK()?
No. I do not want to invest into fine grained crypto locking. I
need a stable test machine. If you think my aporoach of throwing
the kernel lock between the boundary of network stack, pfkey and
crypto is too naive, I would suggest that we backoout the pfkey
unlocking.
But my aproach is stable and has less locks than before. I think
it is an improvement.
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);
> >
