diff mbox series

[net-next] skbuff: revert "skbuff: remove some unnecessary operation in skb_segment_list()"

Message ID f092ecf89336221af04310c9feac800e49d4647f.1618397249.git.pabeni@redhat.com
State New
Headers show
Series [net-next] skbuff: revert "skbuff: remove some unnecessary operation in skb_segment_list()" | expand

Commit Message

Paolo Abeni April 14, 2021, 10:48 a.m. UTC
the commit 1ddc3229ad3c ("skbuff: remove some unnecessary operation
in skb_segment_list()") introduces an issue very similar to the
one already fixed by commit 53475c5dd856 ("net: fix use-after-free when
UDP GRO with shared fraglist").

If the GSO skb goes though skb_clone() and pskb_expand_head() before
entering skb_segment_list(), the latter  will unshare the frag_list
skbs and will release the old list. With the reverted commit in place,
when skb_segment_list() completes, skb->next points to the just
released list, and later on the kernel will hit UaF.

Note that since commit e0e3070a9bc9 ("udp: properly complete L4 GRO
over UDP tunnel packet") the critical scenario can be reproduced also
receiving UDP over vxlan traffic with:

NIC (NETIF_F_GRO_FRAGLIST enabled) -> vxlan -> UDP sink

Attaching a packet socket to the NIC will cause skb_clone() and the
tunnel decapsulation will call pskb_expand_head().

Fixes: 1ddc3229ad3c ("skbuff: remove some unnecessary operation in skb_segment_list()")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/core/skbuff.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

patchwork-bot+netdevbpf@kernel.org April 14, 2021, 9:10 p.m. UTC | #1
Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Wed, 14 Apr 2021 12:48:48 +0200 you wrote:
> the commit 1ddc3229ad3c ("skbuff: remove some unnecessary operation
> in skb_segment_list()") introduces an issue very similar to the
> one already fixed by commit 53475c5dd856 ("net: fix use-after-free when
> UDP GRO with shared fraglist").
> 
> If the GSO skb goes though skb_clone() and pskb_expand_head() before
> entering skb_segment_list(), the latter  will unshare the frag_list
> skbs and will release the old list. With the reverted commit in place,
> when skb_segment_list() completes, skb->next points to the just
> released list, and later on the kernel will hit UaF.
> 
> [...]

Here is the summary with links:
  - [net-next] skbuff: revert "skbuff: remove some unnecessary operation in skb_segment_list()"
    https://git.kernel.org/netdev/net-next/c/17c3df7078e3

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Yunsheng Lin April 15, 2021, 2:23 a.m. UTC | #2
On 2021/4/14 18:48, Paolo Abeni wrote:
> the commit 1ddc3229ad3c ("skbuff: remove some unnecessary operation

> in skb_segment_list()") introduces an issue very similar to the

> one already fixed by commit 53475c5dd856 ("net: fix use-after-free when

> UDP GRO with shared fraglist").

> 

> If the GSO skb goes though skb_clone() and pskb_expand_head() before

> entering skb_segment_list(), the latter  will unshare the frag_list

> skbs and will release the old list. With the reverted commit in place,

> when skb_segment_list() completes, skb->next points to the just

> released list, and later on the kernel will hit UaF.


In that case, is "nskb->next = list_skb" needed before jumpping to
error when __skb_linearize() fails? As there is "nskb->next = list_skb"
before jumpping to error handling when skb_clone() fails.

The inconsistency above is the reason I sent the reverted patch:)

> 

> Note that since commit e0e3070a9bc9 ("udp: properly complete L4 GRO

> over UDP tunnel packet") the critical scenario can be reproduced also

> receiving UDP over vxlan traffic with:

> 

> NIC (NETIF_F_GRO_FRAGLIST enabled) -> vxlan -> UDP sink

> 

> Attaching a packet socket to the NIC will cause skb_clone() and the

> tunnel decapsulation will call pskb_expand_head().

> 

> Fixes: 1ddc3229ad3c ("skbuff: remove some unnecessary operation in skb_segment_list()")

> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

> ---

>  net/core/skbuff.c | 15 ++++++++++++---

>  1 file changed, 12 insertions(+), 3 deletions(-)

> 

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c

> index 3ad9e8425ab2..14010c0eec48 100644

> --- a/net/core/skbuff.c

> +++ b/net/core/skbuff.c

> @@ -3773,13 +3773,13 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,

>  	unsigned int tnl_hlen = skb_tnl_header_len(skb);

>  	unsigned int delta_truesize = 0;

>  	unsigned int delta_len = 0;

> +	struct sk_buff *tail = NULL;

>  	struct sk_buff *nskb, *tmp;

>  	int err;

>  

>  	skb_push(skb, -skb_network_offset(skb) + offset);

>  

>  	skb_shinfo(skb)->frag_list = NULL;

> -	skb->next = list_skb;

>  

>  	do {

>  		nskb = list_skb;

> @@ -3797,8 +3797,17 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,

>  			}

>  		}

>  

> -		if (unlikely(err))

> +		if (!tail)

> +			skb->next = nskb;

> +		else

> +			tail->next = nskb;

> +

> +		if (unlikely(err)) {

> +			nskb->next = list_skb;

>  			goto err_linearize;

> +		}

> +

> +		tail = nskb;

>  

>  		delta_len += nskb->len;

>  		delta_truesize += nskb->truesize;

> @@ -3825,7 +3834,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,

>  

>  	skb_gso_reset(skb);

>  

> -	skb->prev = nskb;

> +	skb->prev = tail;

>  

>  	if (skb_needs_linearize(skb, features) &&

>  	    __skb_linearize(skb))

>
Dongseok Yi April 15, 2021, 4:10 a.m. UTC | #3
On Thu, Apr 15, 2021 at 10:23:17AM +0800, Yunsheng Lin wrote:
> On 2021/4/14 18:48, Paolo Abeni wrote:

> > the commit 1ddc3229ad3c ("skbuff: remove some unnecessary operation

> > in skb_segment_list()") introduces an issue very similar to the

> > one already fixed by commit 53475c5dd856 ("net: fix use-after-free when

> > UDP GRO with shared fraglist").

> >

> > If the GSO skb goes though skb_clone() and pskb_expand_head() before

> > entering skb_segment_list(), the latter  will unshare the frag_list

> > skbs and will release the old list. With the reverted commit in place,

> > when skb_segment_list() completes, skb->next points to the just

> > released list, and later on the kernel will hit UaF.

> 

> In that case, is "nskb->next = list_skb" needed before jumpping to

> error when __skb_linearize() fails? As there is "nskb->next = list_skb"

> before jumpping to error handling when skb_clone() fails.

> 

> The inconsistency above is the reason I sent the reverted patch:)


Yes, It is needed. skb_clone clears the next pointer.

> 

> >

> > Note that since commit e0e3070a9bc9 ("udp: properly complete L4 GRO

> > over UDP tunnel packet") the critical scenario can be reproduced also

> > receiving UDP over vxlan traffic with:

> >

> > NIC (NETIF_F_GRO_FRAGLIST enabled) -> vxlan -> UDP sink

> >

> > Attaching a packet socket to the NIC will cause skb_clone() and the

> > tunnel decapsulation will call pskb_expand_head().

> >

> > Fixes: 1ddc3229ad3c ("skbuff: remove some unnecessary operation in skb_segment_list()")

> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>

> > ---

> >  net/core/skbuff.c | 15 ++++++++++++---

> >  1 file changed, 12 insertions(+), 3 deletions(-)

> >

> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c

> > index 3ad9e8425ab2..14010c0eec48 100644

> > --- a/net/core/skbuff.c

> > +++ b/net/core/skbuff.c

> > @@ -3773,13 +3773,13 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,

> >  	unsigned int tnl_hlen = skb_tnl_header_len(skb);

> >  	unsigned int delta_truesize = 0;

> >  	unsigned int delta_len = 0;

> > +	struct sk_buff *tail = NULL;

> >  	struct sk_buff *nskb, *tmp;

> >  	int err;

> >

> >  	skb_push(skb, -skb_network_offset(skb) + offset);

> >

> >  	skb_shinfo(skb)->frag_list = NULL;

> > -	skb->next = list_skb;

> >

> >  	do {

> >  		nskb = list_skb;

> > @@ -3797,8 +3797,17 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,

> >  			}

> >  		}

> >

> > -		if (unlikely(err))

> > +		if (!tail)

> > +			skb->next = nskb;

> > +		else

> > +			tail->next = nskb;

> > +

> > +		if (unlikely(err)) {

> > +			nskb->next = list_skb;

> >  			goto err_linearize;

> > +		}

> > +

> > +		tail = nskb;

> >

> >  		delta_len += nskb->len;

> >  		delta_truesize += nskb->truesize;

> > @@ -3825,7 +3834,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,

> >

> >  	skb_gso_reset(skb);

> >

> > -	skb->prev = nskb;

> > +	skb->prev = tail;

> >

> >  	if (skb_needs_linearize(skb, features) &&

> >  	    __skb_linearize(skb))

> >
diff mbox series

Patch

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 3ad9e8425ab2..14010c0eec48 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3773,13 +3773,13 @@  struct sk_buff *skb_segment_list(struct sk_buff *skb,
 	unsigned int tnl_hlen = skb_tnl_header_len(skb);
 	unsigned int delta_truesize = 0;
 	unsigned int delta_len = 0;
+	struct sk_buff *tail = NULL;
 	struct sk_buff *nskb, *tmp;
 	int err;
 
 	skb_push(skb, -skb_network_offset(skb) + offset);
 
 	skb_shinfo(skb)->frag_list = NULL;
-	skb->next = list_skb;
 
 	do {
 		nskb = list_skb;
@@ -3797,8 +3797,17 @@  struct sk_buff *skb_segment_list(struct sk_buff *skb,
 			}
 		}
 
-		if (unlikely(err))
+		if (!tail)
+			skb->next = nskb;
+		else
+			tail->next = nskb;
+
+		if (unlikely(err)) {
+			nskb->next = list_skb;
 			goto err_linearize;
+		}
+
+		tail = nskb;
 
 		delta_len += nskb->len;
 		delta_truesize += nskb->truesize;
@@ -3825,7 +3834,7 @@  struct sk_buff *skb_segment_list(struct sk_buff *skb,
 
 	skb_gso_reset(skb);
 
-	skb->prev = nskb;
+	skb->prev = tail;
 
 	if (skb_needs_linearize(skb, features) &&
 	    __skb_linearize(skb))