diff mbox series

[v3] af_key: Fix send_acquire race with pfkey_register

Message ID Y1d8+FdfgtVCaTDS@gondor.apana.org.au
State New
Headers
Series [v3] af_key: Fix send_acquire race with pfkey_register |

Commit Message

Herbert Xu Oct. 25, 2022, 6:06 a.m. UTC 
  On Mon, Oct 24, 2022 at 09:20:00AM +0200, Sabrina Dubroca wrote:
> 2022-10-24, 14:06:12 +0800, Herbert Xu wrote:
> > @@ -1697,11 +1699,11 @@ static int pfkey_register(struct sock *sk, struct sk_buff *skb, const struct sad
> >  		pfk->registered |= (1<<hdr->sadb_msg_satype);
> >  	}
> >  
> > -	mutex_lock(&pfkey_mutex);
> > +	spin_lock_bh(&pfkey_alg_lock);
> >  	xfrm_probe_algs();
> 
> I don't think we can do that:
> 
> void xfrm_probe_algs(void)
> {
> 	int i, status;
> 
> 	BUG_ON(in_softirq());

Indeed.  I was also wrong in stating that this bug was created by
namespaces.  This race has always existed since this code was first
added.

---8<---
The function pfkey_send_acquire may race with pfkey_register
(which could even be in a different name space).  This may result
in a buffer overrun.

Allocating the maximum amount of memory that could be used prevents
this.

Reported-by: syzbot+1e9af9185d8850e2c2fa@syzkaller.appspotmail.com
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Comments

Sabrina Dubroca Oct. 26, 2022, 1:38 p.m. UTC | #1 
2022-10-25, 14:06:48 +0800, Herbert Xu wrote:
> On Mon, Oct 24, 2022 at 09:20:00AM +0200, Sabrina Dubroca wrote:
> > 2022-10-24, 14:06:12 +0800, Herbert Xu wrote:
> > > @@ -1697,11 +1699,11 @@ static int pfkey_register(struct sock *sk, struct sk_buff *skb, const struct sad
> > >  		pfk->registered |= (1<<hdr->sadb_msg_satype);
> > >  	}
> > >  
> > > -	mutex_lock(&pfkey_mutex);
> > > +	spin_lock_bh(&pfkey_alg_lock);
> > >  	xfrm_probe_algs();
> > 
> > I don't think we can do that:
> > 
> > void xfrm_probe_algs(void)
> > {
> > 	int i, status;
> > 
> > 	BUG_ON(in_softirq());
> 
> Indeed.  I was also wrong in stating that this bug was created by
> namespaces.  This race has always existed since this code was first
> added.
> 
> ---8<---
> The function pfkey_send_acquire may race with pfkey_register
> (which could even be in a different name space).  This may result
> in a buffer overrun.
> 
> Allocating the maximum amount of memory that could be used prevents
> this.
> 
> Reported-by: syzbot+1e9af9185d8850e2c2fa@syzkaller.appspotmail.com
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

LGTM, thanks.

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
Herbert Xu Oct. 27, 2022, 3:42 a.m. UTC | #2 
On Wed, Oct 26, 2022 at 03:38:23PM +0200, Sabrina Dubroca wrote:
>
> LGTM, thanks.
> 
> Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

Thanks for the review and comments!
Eric Dumazet Oct. 27, 2022, 3:45 a.m. UTC | #3 
On Wed, Oct 26, 2022 at 8:42 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Wed, Oct 26, 2022 at 03:38:23PM +0200, Sabrina Dubroca wrote:
> >
> > LGTM, thanks.
> >
> > Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
>
> Thanks for the review and comments!

SGTM, thanks for the fix.

Reviewed-by: Eric Dumazet <edumazet@google.com>
Herbert Xu Oct. 27, 2022, 3:55 a.m. UTC | #4 
On Wed, Oct 26, 2022 at 08:45:57PM -0700, Eric Dumazet wrote:
>
> SGTM, thanks for the fix.
> 
> Reviewed-by: Eric Dumazet <edumazet@google.com>

Thanks Eric!
Steffen Klassert Oct. 28, 2022, 11:26 a.m. UTC | #5 
On Wed, Oct 26, 2022 at 08:45:57PM -0700, Eric Dumazet wrote:
> On Wed, Oct 26, 2022 at 8:42 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >
> > On Wed, Oct 26, 2022 at 03:38:23PM +0200, Sabrina Dubroca wrote:
> > >
> > > LGTM, thanks.
> > >
> > > Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
> >
> > Thanks for the review and comments!
> 
> SGTM, thanks for the fix.
> 
> Reviewed-by: Eric Dumazet <edumazet@google.com>

Applied, thanks everyone!
diff mbox series

Patch

diff --git a/net/key/af_key.c b/net/key/af_key.c
index c85df5b958d2..213287814328 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -2905,7 +2905,7 @@  static int count_ah_combs(const struct xfrm_tmpl *t)
 			break;
 		if (!aalg->pfkey_supported)
 			continue;
-		if (aalg_tmpl_set(t, aalg) && aalg->available)
+		if (aalg_tmpl_set(t, aalg))
 			sz += sizeof(struct sadb_comb);
 	}
 	return sz + sizeof(struct sadb_prop);
@@ -2923,7 +2923,7 @@  static int count_esp_combs(const struct xfrm_tmpl *t)
 		if (!ealg->pfkey_supported)
 			continue;
 
-		if (!(ealg_tmpl_set(t, ealg) && ealg->available))
+		if (!(ealg_tmpl_set(t, ealg)))
 			continue;
 
 		for (k = 1; ; k++) {
@@ -2934,16 +2934,17 @@  static int count_esp_combs(const struct xfrm_tmpl *t)
 			if (!aalg->pfkey_supported)
 				continue;
 
-			if (aalg_tmpl_set(t, aalg) && aalg->available)
+			if (aalg_tmpl_set(t, aalg))
 				sz += sizeof(struct sadb_comb);
 		}
 	}
 	return sz + sizeof(struct sadb_prop);
 }
 
-static void dump_ah_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
+static int dump_ah_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
 {
 	struct sadb_prop *p;
+	int sz = 0;
 	int i;
 
 	p = skb_put(skb, sizeof(struct sadb_prop));
@@ -2971,13 +2972,17 @@  static void dump_ah_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
 			c->sadb_comb_soft_addtime = 20*60*60;
 			c->sadb_comb_hard_usetime = 8*60*60;
 			c->sadb_comb_soft_usetime = 7*60*60;
+			sz += sizeof(*c);
 		}
 	}
+
+	return sz + sizeof(*p);
 }
 
-static void dump_esp_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
+static int dump_esp_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
 {
 	struct sadb_prop *p;
+	int sz = 0;
 	int i, k;
 
 	p = skb_put(skb, sizeof(struct sadb_prop));
@@ -3019,8 +3024,11 @@  static void dump_esp_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
 			c->sadb_comb_soft_addtime = 20*60*60;
 			c->sadb_comb_hard_usetime = 8*60*60;
 			c->sadb_comb_soft_usetime = 7*60*60;
+			sz += sizeof(*c);
 		}
 	}
+
+	return sz + sizeof(*p);
 }
 
 static int key_notify_policy_expire(struct xfrm_policy *xp, const struct km_event *c)
@@ -3150,6 +3158,7 @@  static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
 	struct sadb_x_sec_ctx *sec_ctx;
 	struct xfrm_sec_ctx *xfrm_ctx;
 	int ctx_size = 0;
+	int alg_size = 0;
 
 	sockaddr_size = pfkey_sockaddr_size(x->props.family);
 	if (!sockaddr_size)
@@ -3161,16 +3170,16 @@  static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
 		sizeof(struct sadb_x_policy);
 
 	if (x->id.proto == IPPROTO_AH)
-		size += count_ah_combs(t);
+		alg_size = count_ah_combs(t);
 	else if (x->id.proto == IPPROTO_ESP)
-		size += count_esp_combs(t);
+		alg_size = count_esp_combs(t);
 
 	if ((xfrm_ctx = x->security)) {
 		ctx_size = PFKEY_ALIGN8(xfrm_ctx->ctx_len);
 		size +=  sizeof(struct sadb_x_sec_ctx) + ctx_size;
 	}
 
-	skb =  alloc_skb(size + 16, GFP_ATOMIC);
+	skb =  alloc_skb(size + alg_size + 16, GFP_ATOMIC);
 	if (skb == NULL)
 		return -ENOMEM;
 
@@ -3224,10 +3233,13 @@  static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
 	pol->sadb_x_policy_priority = xp->priority;
 
 	/* Set sadb_comb's. */
+	alg_size = 0;
 	if (x->id.proto == IPPROTO_AH)
-		dump_ah_combs(skb, t);
+		alg_size = dump_ah_combs(skb, t);
 	else if (x->id.proto == IPPROTO_ESP)
-		dump_esp_combs(skb, t);
+		alg_size = dump_esp_combs(skb, t);
+
+	hdr->sadb_msg_len += alg_size / 8;
 
 	/* security context */
 	if (xfrm_ctx) {