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

Groups > linux.kernel > #1204103 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [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

#1204103 — [PATCH 3.2 089/110] net: Clone skb before setting peeked flag

3.2.71-rc1 review patch.  If anyone has any objections, please let me know.

------------------

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;

--
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/

[toc] | [next] | [standalone]

#1204182

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/

[toc] | [prev] | [next] | [standalone]

#1204460

From: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Date: Mon, 10 Aug 2015 14:37:05 +0300

> 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.

It is in my -stable queue and will be submitted to the stable trees
when I deem it appropriate.  I wanted the fix to sit and soak in
Linus's tree before just throwing it to the stable folks.
--
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/

[toc] | [prev] | [next] | [standalone]

#1205358

[Multipart message — attachments visible in raw view] — view raw

On Mon, 2015-08-10 at 14:37 +0300, Konstantin Khlebnikov wrote:
> 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.
[...]

Thanks, I'll defer this and wait for that to show up.

Ben.

-- 
Ben Hutchings
Theory and practice are closer in theory than in practice.
                                - John Levine, moderator of comp.compilers

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web