[3.16.y-ckt stable] Patch "net: Clone skb before setting peeked flag" has been added to staging queue

[Konstantin Khlebnikov]

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 at 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 at yandex-team.ru>
> Signed-off-by: Herbert Xu <herbert at gondor.apana.org.au>
> Signed-off-by: David S. Miller <davem at davemloft.net>
> Signed-off-by: Luis Henriques <luis.henriques at 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;
>


-- 
Konstantin