Re: ipsec crypto kernel lock
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 - 1.146
> > > +++ netinet/ip_ah.c 16 Jun 2021 17:29:37 -
> > > @@ -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 =
> > > }
> > >
> > > - return crypto_newsession(>tdb_cryptoid, , 0);
> > > + KERNEL_LOCK();
> > > + error = crypto_newsession(>tdb_cryptoid, , 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_dst, >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_dst, >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 - 1.162
> > > +++ netinet/ip_esp.c 16 Jun 2021 17:29:55 -
> >
Re: ipsec crypto kernel lock
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 - 1.146
> > +++ netinet/ip_ah.c 16 Jun 2021 17:29:37 -
> > @@ -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 =
> > }
> >
> > - return crypto_newsession(>tdb_cryptoid, , 0);
> > + KERNEL_LOCK();
> > + error = crypto_newsession(>tdb_cryptoid, , 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_dst, >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_dst, >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.c25 Feb 2021 02:48:21 - 1.162
> > +++ netinet/ip_esp.c16 Jun 2021 17:29:55 -
> > @@ -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 =
Re: ipsec crypto kernel lock
On 16/06/21(Wed) 22:05, Alexander Bluhm 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,8000246e1db0) at aes_ctr_crypt+0x1e
> swcr_authenc(fd8132a21b08) at swcr_authenc+0x5c3
> swcr_process(fd8132a21b08) at swcr_process+0x1e8
> crypto_invoke(fd8132a21b08) at crypto_invoke+0xde
> taskq_thread(80200500) at taskq_thread+0x81
> end trace frame: 0x0, count: -5
>
> *64926 109760 0 0 7 0x14200crypto
>
> 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.
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.
Could you annotate which field is being protected by the KERNEL_LOCK()?
> 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 - 1.146
> +++ netinet/ip_ah.c 16 Jun 2021 17:29:37 -
> @@ -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 =
> }
>
> - return crypto_newsession(>tdb_cryptoid, , 0);
> + KERNEL_LOCK();
> + error = crypto_newsession(>tdb_cryptoid, , 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_dst, >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_dst, >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
>
Re: ipsec crypto kernel lock
ok mvs@
> On 17 Jun 2021, at 01:06, Alexander Bluhm wrote:
>
> On Wed, Jun 16, 2021 at 11:58:48PM +0300, Vitaliy Makkoveev wrote:
>> crypto_getreq() and crypto_freereq() don???t require kernel lock.
>
> Indeed, new diff.
>
> 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 - 1.146
> +++ netinet/ip_ah.c 16 Jun 2021 21:59:38 -
> @@ -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 =
> }
>
> - return crypto_newsession(>tdb_cryptoid, , 0);
> + KERNEL_LOCK();
> + error = crypto_newsession(>tdb_cryptoid, , 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;
> }
>
> /*
> @@ -696,7 +702,10 @@ ah_input(struct mbuf *m, struct tdb *tdb
> tc->tc_rdomain = tdb->tdb_rdomain;
> memcpy(>tc_dst, >tdb_dst, sizeof(union sockaddr_union));
>
> - return crypto_dispatch(crp);
> + KERNEL_LOCK();
> + error = crypto_dispatch(crp);
> + KERNEL_UNLOCK();
> + return error;
>
> drop:
> m_freem(m);
> @@ -1144,7 +1153,10 @@ ah_output(struct mbuf *m, struct tdb *td
> tc->tc_rdomain = tdb->tdb_rdomain;
> memcpy(>tc_dst, >tdb_dst, sizeof(union sockaddr_union));
>
> - return crypto_dispatch(crp);
> + KERNEL_LOCK();
> + error = crypto_dispatch(crp);
> + KERNEL_UNLOCK();
> + return error;
>
> drop:
> m_freem(m);
> 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 - 1.162
> +++ netinet/ip_esp.c 16 Jun 2021 22:00:08 -
> @@ -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(>tdb_cryptoid,
> + KERNEL_LOCK();
> + error = crypto_newsession(>tdb_cryptoid,
> (tdbp->tdb_encalgxform ? : ), 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)
> @@ -519,7 +525,10 @@ 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);
> @@ -1006,7 +1015,10 @@ 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);
> 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
> ---
Re: ipsec crypto kernel lock
On Wed, Jun 16, 2021 at 11:58:48PM +0300, Vitaliy Makkoveev wrote:
> crypto_getreq() and crypto_freereq() don???t require kernel lock.
Indeed, new diff.
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 - 1.146
+++ netinet/ip_ah.c 16 Jun 2021 21:59:38 -
@@ -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 =
}
- return crypto_newsession(>tdb_cryptoid, , 0);
+ KERNEL_LOCK();
+ error = crypto_newsession(>tdb_cryptoid, , 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;
}
/*
@@ -696,7 +702,10 @@ ah_input(struct mbuf *m, struct tdb *tdb
tc->tc_rdomain = tdb->tdb_rdomain;
memcpy(>tc_dst, >tdb_dst, sizeof(union sockaddr_union));
- return crypto_dispatch(crp);
+ KERNEL_LOCK();
+ error = crypto_dispatch(crp);
+ KERNEL_UNLOCK();
+ return error;
drop:
m_freem(m);
@@ -1144,7 +1153,10 @@ ah_output(struct mbuf *m, struct tdb *td
tc->tc_rdomain = tdb->tdb_rdomain;
memcpy(>tc_dst, >tdb_dst, sizeof(union sockaddr_union));
- return crypto_dispatch(crp);
+ KERNEL_LOCK();
+ error = crypto_dispatch(crp);
+ KERNEL_UNLOCK();
+ return error;
drop:
m_freem(m);
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.c25 Feb 2021 02:48:21 - 1.162
+++ netinet/ip_esp.c16 Jun 2021 22:00:08 -
@@ -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(>tdb_cryptoid,
+ KERNEL_LOCK();
+ error = crypto_newsession(>tdb_cryptoid,
(tdbp->tdb_encalgxform ? : ), 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)
@@ -519,7 +525,10 @@ 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);
@@ -1006,7 +1015,10 @@ 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);
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 - 1.67
+++ netinet/ip_ipcomp.c 16 Jun 2021 22:00:45 -
@@ -79,6 +79,7 @@ ipcomp_init(struct tdb *tdbp, struct xfo
{
Re: ipsec crypto kernel lock
Hi,
crypto_getreq() and crypto_freereq() don’t require kernel lock.
> On 16 Jun 2021, at 23:05, Alexander Bluhm 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,8000246e1db0) at aes_ctr_crypt+0x1e
> swcr_authenc(fd8132a21b08) at swcr_authenc+0x5c3
> swcr_process(fd8132a21b08) at swcr_process+0x1e8
> crypto_invoke(fd8132a21b08) at crypto_invoke+0xde
> taskq_thread(80200500) at taskq_thread+0x81
> end trace frame: 0x0, count: -5
>
> *64926 109760 0 0 7 0x14200crypto
>
> 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 - 1.146
> +++ netinet/ip_ah.c 16 Jun 2021 17:29:37 -
> @@ -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 =
> }
>
> - return crypto_newsession(>tdb_cryptoid, , 0);
> + KERNEL_LOCK();
> + error = crypto_newsession(>tdb_cryptoid, , 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_dst, >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_dst, >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 - 1.162
> +++
ipsec crypto kernel lock
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,8000246e1db0) at aes_ctr_crypt+0x1e
swcr_authenc(fd8132a21b08) at swcr_authenc+0x5c3
swcr_process(fd8132a21b08) at swcr_process+0x1e8
crypto_invoke(fd8132a21b08) at crypto_invoke+0xde
taskq_thread(80200500) at taskq_thread+0x81
end trace frame: 0x0, count: -5
*64926 109760 0 0 7 0x14200crypto
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 - 1.146
+++ netinet/ip_ah.c 16 Jun 2021 17:29:37 -
@@ -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 =
}
- return crypto_newsession(>tdb_cryptoid, , 0);
+ KERNEL_LOCK();
+ error = crypto_newsession(>tdb_cryptoid, , 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_dst, >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_dst, >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.c25 Feb 2021 02:48:21 - 1.162
+++ netinet/ip_esp.c16 Jun 2021 17:29:55 -
@@ -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
