Message ID | 1439297802-32088-1-git-send-email-luis.henriques@canonical.com |
---|---|
State | New |
Headers | show |
On 11.08.2015 15:56, Luis Henriques wrote: > This is a note to let you know that I have just added a patch titled > > net: Clone skb before setting peeked flag > > to the linux-3.16.y-queue branch of the 3.16.y-ckt extended stable tree > which can be found at: > > http://kernel.ubuntu.com/git/ubuntu/linux.git/log/?h=linux-3.16.y-queue > > This patch is scheduled to be released in version 3.16.7-ckt16. > > If you, or anyone else, feels it should not be added to this tree, please > reply to this email. This patch buggy. Please wait for "net: Fix skb_set_peeked use-after-free" https://lkml.org/lkml/2015/8/10/721 > > For more information about the 3.16.y-ckt tree, see > https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable > > Thanks. > -Luis > > ------ > > From 1f0d52ed21163c5e29fa7c5f7a834eaeaffbf5ea Mon Sep 17 00:00:00 2001 > From: Herbert Xu <herbert@gondor.apana.org.au> > Date: Mon, 13 Jul 2015 16:04:13 +0800 > Subject: net: Clone skb before setting peeked flag > > commit 738ac1ebb96d02e0d23bc320302a6ea94c612dec upstream. > > Shared skbs must not be modified and this is crucial for broadcast > and/or multicast paths where we use it as an optimisation to avoid > unnecessary cloning. > > The function skb_recv_datagram breaks this rule by setting peeked > without cloning the skb first. This causes funky races which leads > to double-free. > > This patch fixes this by cloning the skb and replacing the skb > in the list when setting skb->peeked. > > Fixes: a59322be07c9 ("[UDP]: Only increment counter on first peek/recv") > Reported-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > Signed-off-by: David S. Miller <davem@davemloft.net> > Signed-off-by: Luis Henriques <luis.henriques@canonical.com> > --- > net/core/datagram.c | 41 ++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 38 insertions(+), 3 deletions(-) > > diff --git a/net/core/datagram.c b/net/core/datagram.c > index 488dd1a825c0..4e07eaef9949 100644 > --- a/net/core/datagram.c > +++ b/net/core/datagram.c > @@ -130,6 +130,35 @@ out_noerr: > goto out; > } > > +static int skb_set_peeked(struct sk_buff *skb) > +{ > + struct sk_buff *nskb; > + > + if (skb->peeked) > + return 0; > + > + /* We have to unshare an skb before modifying it. */ > + if (!skb_shared(skb)) > + goto done; > + > + nskb = skb_clone(skb, GFP_ATOMIC); > + if (!nskb) > + return -ENOMEM; > + > + skb->prev->next = nskb; > + skb->next->prev = nskb; > + nskb->prev = skb->prev; > + nskb->next = skb->next; > + > + consume_skb(skb); > + skb = nskb; > + > +done: > + skb->peeked = 1; > + > + return 0; > +} > + > /** > * __skb_recv_datagram - Receive a datagram skbuff > * @sk: socket > @@ -164,7 +193,9 @@ out_noerr: > struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags, > int *peeked, int *off, int *err) > { > + struct sk_buff_head *queue = &sk->sk_receive_queue; > struct sk_buff *skb, *last; > + unsigned long cpu_flags; > long timeo; > /* > * Caller is allowed not to check sk->sk_err before skb_recv_datagram() > @@ -183,8 +214,6 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags, > * Look at current nfs client by the way... > * However, this function was correct in any case. 8) > */ > - unsigned long cpu_flags; > - struct sk_buff_head *queue = &sk->sk_receive_queue; > int _off = *off; > > last = (struct sk_buff *)queue; > @@ -198,7 +227,11 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags, > _off -= skb->len; > continue; > } > - skb->peeked = 1; > + > + error = skb_set_peeked(skb); > + if (error) > + goto unlock_err; > + > atomic_inc(&skb->users); > } else > __skb_unlink(skb, queue); > @@ -222,6 +255,8 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags, > > return NULL; > > +unlock_err: > + spin_unlock_irqrestore(&queue->lock, cpu_flags); > no_packet: > *err = error; > return NULL; >
On Tue, Aug 11, 2015 at 05:56:50PM +0300, Konstantin Khlebnikov wrote: > On 11.08.2015 15:56, Luis Henriques wrote: > >This is a note to let you know that I have just added a patch titled > > > > net: Clone skb before setting peeked flag > > > >to the linux-3.16.y-queue branch of the 3.16.y-ckt extended stable tree > >which can be found at: > > > > http://kernel.ubuntu.com/git/ubuntu/linux.git/log/?h=linux-3.16.y-queue > > > >This patch is scheduled to be released in version 3.16.7-ckt16. > > > >If you, or anyone else, feels it should not be added to this tree, please > >reply to this email. > > This patch buggy. Please wait for "net: Fix skb_set_peeked use-after-free" > https://lkml.org/lkml/2015/8/10/721 > Thank you, Konstantin. I'll drop this patch for now and wait for that fix. Cheers, -- Luís
diff --git a/net/core/datagram.c b/net/core/datagram.c index 488dd1a825c0..4e07eaef9949 100644 --- a/net/core/datagram.c +++ b/net/core/datagram.c @@ -130,6 +130,35 @@ out_noerr: goto out; } +static int skb_set_peeked(struct sk_buff *skb) +{ + struct sk_buff *nskb; + + if (skb->peeked) + return 0; + + /* We have to unshare an skb before modifying it. */ + if (!skb_shared(skb)) + goto done; + + nskb = skb_clone(skb, GFP_ATOMIC); + if (!nskb) + return -ENOMEM; + + skb->prev->next = nskb; + skb->next->prev = nskb; + nskb->prev = skb->prev; + nskb->next = skb->next; + + consume_skb(skb); + skb = nskb; + +done: + skb->peeked = 1; + + return 0; +} + /** * __skb_recv_datagram - Receive a datagram skbuff * @sk: socket @@ -164,7 +193,9 @@ out_noerr: struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags, int *peeked, int *off, int *err) { + struct sk_buff_head *queue = &sk->sk_receive_queue; struct sk_buff *skb, *last; + unsigned long cpu_flags; long timeo; /* * Caller is allowed not to check sk->sk_err before skb_recv_datagram() @@ -183,8 +214,6 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags, * Look at current nfs client by the way... * However, this function was correct in any case. 8) */ - unsigned long cpu_flags; - struct sk_buff_head *queue = &sk->sk_receive_queue; int _off = *off; last = (struct sk_buff *)queue; @@ -198,7 +227,11 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags, _off -= skb->len; continue; } - skb->peeked = 1; + + error = skb_set_peeked(skb); + if (error) + goto unlock_err; + atomic_inc(&skb->users); } else __skb_unlink(skb, queue); @@ -222,6 +255,8 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags, return NULL; +unlock_err: + spin_unlock_irqrestore(&queue->lock, cpu_flags); no_packet: *err = error; return NULL;