Re: [PATCH net] udp: fix segmentation crash for GRO packet without fraglist

[Willem de Bruijn]

> > >  struct sk_buff *tail = NULL;
> > >  struct sk_buff *nskb, *tmp;
> > >  int len_diff, err;
> > > @@ -4504,6 +4505,9 @@ struct sk_buff *skb_segment_list(struct
> > sk_buff
> > > *skb,
> > >  if (err)
> > >  goto err_linearize;
> > >  
> > > +if (mss != GSO_BY_FRAGS && mss != skb_headlen(skb))
> > > +return ERR_PTR(-EFAULT);
> > > +
> > 
> > Do this precondition integrity check before the skb_unclone path?
> 
> After return error, the skb will enter into kfree_skb, not consume_skb.
> It may meet same crash problem which has been resolved by skb_unclone.
> 
> Or kfree_skb could well handle the cloned skb's release?

Since this is an error path it should reach kfree_skb rather than
consume_skb.

> 
> Other changes are updated as below:
> 
> From 301da5c9d65652bac6091d4cd64b751b3338f8bb Mon Sep 17 00:00:00 2001
> From: Shiming Cheng <shiming.cheng@xxxxxxxxxxxx>
> Date: Wed, 24 Apr 2024 13:42:35 +0800
> Subject: [PATCH net] net: prevent BPF pulling SKB_GSO_FRAGLIST skb
> 
> A SKB_GSO_FRAGLIST skb can't be pulled data
> from its fraglist as it may result an invalid
> segmentation or kernel exception.
> 
> For such structured skb we limit the BPF pulling
> data length smaller than skb_headlen() and return
> error if exceeding.
> 
> Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.")
> Signed-off-by: Shiming Cheng <shiming.cheng@xxxxxxxxxxxx>
> Signed-off-by: Lena Wang <lena.wang@xxxxxxxxxxxx>
> ---
>  net/core/filter.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 8adf95765cdd..8ed4d5d87167 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -1662,6 +1662,11 @@ static DEFINE_PER_CPU(struct bpf_scratchpad,
> bpf_sp);
>  static inline int __bpf_try_make_writable(struct sk_buff *skb,
>  					  unsigned int write_len)
>  {
> +	if (skb_is_gso(skb) &&
> +	    (skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST) &&
> +	     write_len > skb_headlen(skb)) {
> +		return -ENOMEM;
> +	}
>  	return skb_ensure_writable(skb, write_len);
>  }
>  
> -- 
> 2.18.0
> 
> 
> From 64d55392debbc90ef2e9c33441024d612075bdd7 Mon Sep 17 00:00:00 2001
> From: Shiming Cheng <shiming.cheng@xxxxxxxxxxxx>
> Date: Wed, 24 Apr 2024 14:43:45 +0800
> Subject: [PATCH net] net: drop pulled SKB_GSO_FRAGLIST skb
> 
> A SKB_GSO_FRAGLIST skb without GSO_BY_FRAGS is
> expected to have all segments except the last
> to be gso_size long. If this does not hold, the
> skb has been modified and the fraglist gso integrity
> is lost. Drop the packet, as it cannot be segmented
> correctly by skb_segment_list.
> 
> The skb could be salvaged, though, right?
> By linearizing, dropping the SKB_GSO_FRAGLIST bit
> and entering the normal skb_segment path rather than
> the skb_segment_list path.

Drop the "though, right?"
> 
> That choice is currently made in the protocol caller,
> __udp_gso_segment. It's not trivial to add such a
> backup path here. So let's add this backstop against
> kernel crashes.
> 
> If the gso_size does not match skb_headlen(),
> it means part of or the entire fraglist has been pulled.
> It has been messed with and we should return error to
> free this skb.

This paragraph is now duplicative. Drop.
> 
> Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.")
> Signed-off-by: Shiming Cheng <shiming.cheng@xxxxxxxxxxxx>
> Signed-off-by: Lena Wang <lena.wang@xxxxxxxxxxxx>
> ---
>  net/core/skbuff.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index b99127712e67..4777f5fea6c3 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4491,6 +4491,7 @@ struct sk_buff *skb_segment_list(struct sk_buff
> *skb,
>  {
>  	struct sk_buff *list_skb = skb_shinfo(skb)->frag_list;
>  	unsigned int tnl_hlen = skb_tnl_header_len(skb);
> +	unsigned int mss = skb_shinfo(skb)->gso_size;
>  	unsigned int delta_truesize = 0;
>  	unsigned int delta_len = 0;
>  	struct sk_buff *tail = NULL;
> @@ -4504,6 +4505,9 @@ struct sk_buff *skb_segment_list(struct sk_buff
> *skb,
>  	if (err)
>  		goto err_linearize;
>  
> +	if (mss != GSO_BY_FRAGS && mss != skb_headlen(skb))
> +		return ERR_PTR(-EFAULT);
> +
>  	skb_shinfo(skb)->frag_list = NULL;
>  
>  	while (list_skb) {
> -- 
> 2.18.0