| Message ID | 20150804074247.GA29951@gondor.apana.org.au |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Herbert Xu > Sent: 04 August 2015 08:43 > Brenden Blanco <bblanco@plumgrid.com> wrote: > >> [ 318.244596] BUG: unable to handle kernel NULL pointer dereference > >> at 000000000000008e > >> [ 318.245182] IP: [<ffffffff81455e7c>] __skb_recv_datagram+0xbc/0x5a0 > > > > Replying to myself, and adding commit interested parties... > > > > I went through the git log for the function in question, and > > positively identified that the following commit introduces the crash: > > > > 738ac1e net: Clone skb before setting peeked flag > > > > Null dereference is at line 224 of net/core/datagram.c (according to > > my objdump dis-assembly): > > Sorry, brain fart on my part. Please try this patch. You've introduced a memory leak if skb_clone() fails. > The commit 738ac1ebb96d02e0d23bc320302a6ea94c612dec ("net: Clone > skb before setting peeked flag") introduced a use-after-free bug > in skb_recv_datagram. This is because skb_set_peeked may create > a new skb and free the existing one. As it stands the caller will > continue to use the old freed skb. > > This patch fixes it by making skb_set_peeked return the new skb > (or the old one if unchanged). ... > nskb = skb_clone(skb, GFP_ATOMIC); > if (!nskb) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); Here the original skb is still allocated. > - error = skb_set_peeked(skb); > - if (error) > + skb = skb_set_peeked(skb); You've now lost the address of the original skb. > + error = PTR_ERR(skb); > + if (IS_ERR(skb)) > goto unlock_err; David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 04, 2015 at 09:15:13AM +0000, David Laight wrote: > > You've introduced a memory leak if skb_clone() fails. No I have not. > > nskb = skb_clone(skb, GFP_ATOMIC); > > if (!nskb) > > - return -ENOMEM; > > + return ERR_PTR(-ENOMEM); > > Here the original skb is still allocated. > > > - error = skb_set_peeked(skb); > > - if (error) > > + skb = skb_set_peeked(skb); > > You've now lost the address of the original skb. It doesn't matter because we will take the error path and return the ENOMEM error. We must not free the skb as it's still on the recv queue. Cheers,
From: Herbert Xu > Sent: 04 August 2015 10:21 > On Tue, Aug 04, 2015 at 09:15:13AM +0000, David Laight wrote: > > > > You've introduced a memory leak if skb_clone() fails. > > No I have not. > > > > nskb = skb_clone(skb, GFP_ATOMIC); > > > if (!nskb) > > > - return -ENOMEM; > > > + return ERR_PTR(-ENOMEM); > > > > Here the original skb is still allocated. > > > > > - error = skb_set_peeked(skb); > > > - if (error) > > > + skb = skb_set_peeked(skb); > > > > You've now lost the address of the original skb. > > It doesn't matter because we will take the error path and return > the ENOMEM error. We must not free the skb as it's still on the > recv queue. In that case, what happens to the receive queue when skb_clone() takes a copy of the skb - freeing the original one? David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Laight <David.Laight@aculab.com> wrote: > > In that case, what happens to the receive queue when skb_clone() > takes a copy of the skb - freeing the original one? The new skb is inserted into the recv queue and replacing the existing skb.
> The commit 738ac1ebb96d02e0d23bc320302a6ea94c612dec ("net: Clone > skb before setting peeked flag") introduced a use-after-free bug > in skb_recv_datagram. This is because skb_set_peeked may create > a new skb and free the existing one. As it stands the caller will > continue to use the old freed skb. > > This patch fixes it by making skb_set_peeked return the new skb > (or the old one if unchanged). > > Fixes: 738ac1ebb96d ("net: Clone skb before setting peeked flag") > Reported-by: Brenden Blanco <bblanco@plumgrid.com> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > diff --git a/net/core/datagram.c b/net/core/datagram.c > index 4967262..617088a 100644 > --- a/net/core/datagram.c > +++ b/net/core/datagram.c > @@ -131,12 +131,12 @@ out_noerr: > goto out; > } > > -static int skb_set_peeked(struct sk_buff *skb) > +static struct sk_buff *skb_set_peeked(struct sk_buff *skb) > { > struct sk_buff *nskb; > > if (skb->peeked) > - return 0; > + return skb; > > /* We have to unshare an skb before modifying it. */ > if (!skb_shared(skb)) > @@ -144,7 +144,7 @@ static int skb_set_peeked(struct sk_buff *skb) > > nskb = skb_clone(skb, GFP_ATOMIC); > if (!nskb) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > > skb->prev->next = nskb; > skb->next->prev = nskb; > @@ -157,7 +157,7 @@ static int skb_set_peeked(struct sk_buff *skb) > done: > skb->peeked = 1; > > - return 0; > + return skb; > } > > /** > @@ -229,8 +229,9 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags, > continue; > } > > - error = skb_set_peeked(skb); > - if (error) > + skb = skb_set_peeked(skb); > + error = PTR_ERR(skb); > + if (IS_ERR(skb)) > goto unlock_err; > > atomic_inc(&skb->users); This patch holds good in my testing. Thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 04, 2015 at 08:30:23AM -0700, Brenden Blanco wrote: > > This patch holds good in my testing. Thanks! Thanks for testing. Let's add a tag for patchwork: Tested-by: Brenden Blanco <bblanco@plumgrid.com>
On 04.08.2015 10:42, Herbert Xu wrote: > Brenden Blanco <bblanco@plumgrid.com> wrote: >>> [ 318.244596] BUG: unable to handle kernel NULL pointer dereference >>> at 000000000000008e >>> [ 318.245182] IP: [<ffffffff81455e7c>] __skb_recv_datagram+0xbc/0x5a0 >> >> Replying to myself, and adding commit interested parties... >> >> I went through the git log for the function in question, and >> positively identified that the following commit introduces the crash: >> >> 738ac1e net: Clone skb before setting peeked flag >> >> Null dereference is at line 224 of net/core/datagram.c (according to >> my objdump dis-assembly): > > Sorry, brain fart on my part. Please try this patch. > > ---8<--- > The commit 738ac1ebb96d02e0d23bc320302a6ea94c612dec ("net: Clone > skb before setting peeked flag") introduced a use-after-free bug > in skb_recv_datagram. This is because skb_set_peeked may create > a new skb and free the existing one. As it stands the caller will > continue to use the old freed skb. > > This patch fixes it by making skb_set_peeked return the new skb > (or the old one if unchanged). > > Fixes: 738ac1ebb96d ("net: Clone skb before setting peeked flag") > Reported-by: Brenden Blanco <bblanco@plumgrid.com> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> Seems correct. You can add: Reviewed-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> Your skb_set_peeked() doesn't set prev/next to NULL when unlinks old skb from the queue unlike to __skb_unlink(). Isn't big deal but nulling might be useful. > > diff --git a/net/core/datagram.c b/net/core/datagram.c > index 4967262..617088a 100644 > --- a/net/core/datagram.c > +++ b/net/core/datagram.c > @@ -131,12 +131,12 @@ out_noerr: > goto out; > } > > -static int skb_set_peeked(struct sk_buff *skb) > +static struct sk_buff *skb_set_peeked(struct sk_buff *skb) > { > struct sk_buff *nskb; > > if (skb->peeked) > - return 0; > + return skb; > > /* We have to unshare an skb before modifying it. */ > if (!skb_shared(skb)) > @@ -144,7 +144,7 @@ static int skb_set_peeked(struct sk_buff *skb) > > nskb = skb_clone(skb, GFP_ATOMIC); > if (!nskb) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > > skb->prev->next = nskb; > skb->next->prev = nskb; > @@ -157,7 +157,7 @@ static int skb_set_peeked(struct sk_buff *skb) > done: > skb->peeked = 1; > > - return 0; > + return skb; > } > > /** > @@ -229,8 +229,9 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags, > continue; > } > > - error = skb_set_peeked(skb); > - if (error) > + skb = skb_set_peeked(skb); > + error = PTR_ERR(skb); > + if (IS_ERR(skb)) > goto unlock_err; > > atomic_inc(&skb->users); >
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Tue, 4 Aug 2015 15:42:47 +0800 > The commit 738ac1ebb96d02e0d23bc320302a6ea94c612dec ("net: Clone > skb before setting peeked flag") introduced a use-after-free bug > in skb_recv_datagram. This is because skb_set_peeked may create > a new skb and free the existing one. As it stands the caller will > continue to use the old freed skb. > > This patch fixes it by making skb_set_peeked return the new skb > (or the old one if unchanged). > > Fixes: 738ac1ebb96d ("net: Clone skb before setting peeked flag") > Reported-by: Brenden Blanco <bblanco@plumgrid.com> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> Applied, and since the commit this is fixing is queued up for -stable I'll of course queue this up too. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/datagram.c b/net/core/datagram.c index 4967262..617088a 100644 --- a/net/core/datagram.c +++ b/net/core/datagram.c @@ -131,12 +131,12 @@ out_noerr: goto out; } -static int skb_set_peeked(struct sk_buff *skb) +static struct sk_buff *skb_set_peeked(struct sk_buff *skb) { struct sk_buff *nskb; if (skb->peeked) - return 0; + return skb; /* We have to unshare an skb before modifying it. */ if (!skb_shared(skb)) @@ -144,7 +144,7 @@ static int skb_set_peeked(struct sk_buff *skb) nskb = skb_clone(skb, GFP_ATOMIC); if (!nskb) - return -ENOMEM; + return ERR_PTR(-ENOMEM); skb->prev->next = nskb; skb->next->prev = nskb; @@ -157,7 +157,7 @@ static int skb_set_peeked(struct sk_buff *skb) done: skb->peeked = 1; - return 0; + return skb; } /** @@ -229,8 +229,9 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags, continue; } - error = skb_set_peeked(skb); - if (error) + skb = skb_set_peeked(skb); + error = PTR_ERR(skb); + if (IS_ERR(skb)) goto unlock_err; atomic_inc(&skb->users);