Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1204182

Re: [PATCH 3.2 089/110] net: Clone skb before setting peeked flag

Show all headers | View raw

On 10.08.2015 13:12, Ben Hutchings wrote:
> 3.2.71-rc1 review patch.  If anyone has any objections, please let me know.

Here is important fix: https://patchwork.ozlabs.org/patch/503374/
"net: Fix skb_set_peeked use-after-free". not in upstream yet.

>
> ------------------
>
> From: Herbert Xu <herbert@gondor.apana.org.au>
>
> 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>
> [bwh: Backported to 3.2: adjust context]
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
>   net/core/datagram.c | 41 ++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 38 insertions(+), 3 deletions(-)
>
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -128,6 +128,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
> @@ -160,7 +189,9 @@ out_noerr:
>   struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned flags,
>   				    int *peeked, int *err)
>   {
> +	struct sk_buff_head *queue = &sk->sk_receive_queue;
>   	struct sk_buff *skb;
> +	unsigned long cpu_flags;
>   	long timeo;
>   	/*
>   	 * Caller is allowed not to check sk->sk_err before skb_recv_datagram()
> @@ -179,15 +210,16 @@ struct sk_buff *__skb_recv_datagram(stru
>   		 * 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;
> -
>   		spin_lock_irqsave(&queue->lock, cpu_flags);
>   		skb = skb_peek(queue);
>   		if (skb) {
>   			*peeked = skb->peeked;
>   			if (flags & MSG_PEEK) {
> -				skb->peeked = 1;
> +
> +				error = skb_set_peeked(skb);
> +				if (error)
> +					goto unlock_err;
> +
>   				atomic_inc(&skb->users);
>   			} else
>   				__skb_unlink(skb, queue);
> @@ -206,6 +238,8 @@ struct sk_buff *__skb_recv_datagram(stru
>
>   	return NULL;
>
> +unlock_err:
> +	spin_unlock_irqrestore(&queue->lock, cpu_flags);
>   no_packet:
>   	*err = error;
>   	return NULL;
>


-- 
Konstantin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 3.2 089/110] net: Clone skb before setting peeked flag Ben Hutchings <ben@decadent.org.uk> - 2015-08-10 12:40 +0200
  Re: [PATCH 3.2 089/110] net: Clone skb before setting peeked flag Konstantin Khlebnikov <khlebnikov@yandex-team.ru> - 2015-08-10 13:40 +0200
    Re: [PATCH 3.2 089/110] net: Clone skb before setting peeked flag David Miller <davem@davemloft.net> - 2015-08-10 20:20 +0200
    Re: [PATCH 3.2 089/110] net: Clone skb before setting peeked flag Ben Hutchings <ben@decadent.org.uk> - 2015-08-11 20:30 +0200

csiph-web