On Thu, Jun 17, 2021 at 03:19:11PM +0200, Alexander Bluhm wrote:
> 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.
We perform crypto(9) access from net stack without kernel lock held, so
pfkey unlocking backout could not be enough and 'tdbp->tdb_xform->xf_*'
routines should be also wrapped by kernel lock. So this diff is welcomed
anyway.
>
> 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);
> > >
>
