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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Lena Wang (王娜) wrote:
> On Sat, 2024-04-27 at 09:28 -0400, Willem de Bruijn wrote:
> >  	 
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> >  
> > Daniel Borkmann wrote:
> > > On 4/26/24 11:52 AM, Lena Wang (王娜) wrote:
> > > [...]
> > > >>>  From 301da5c9d65652bac6091d4cd64b751b3338f8bb Mon Sep 17
> > 00:00:00
> > > >> 2001
> > > >>> From: Shiming Cheng <shiming.cheng@mediatek.com>
> > > >>> 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@mediatek.com>
> > > >>> Signed-off-by: Lena Wang <lena.wang@mediatek.com>
> > > >>> ---
> > > >>>   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);
> > > 
> > > Dumb question, but should this guard be more generically part of
> > skb_ensure_writable()
> > > internals, presumably that would be inside pskb_may_pull_reason(),
> > or only if we ever
> > > see more code instances similar to this?
> > 
> > Good point. Most callers of skb_ensure_writable correctly pull only
> > headers, so wouldn't cause this problem. But it also adds coverage to
> > things like tc pedit.
> 
> Updated:
> 
> From 3be30b8cf6e629f2615ef4eafe3b2a1c0d68c530 Mon Sep 17 00:00:00 2001
> From: Shiming Cheng <shiming.cheng@mediatek.com>
> Date: Sun, 28 Apr 2024 15:03:12 +0800
> Subject: [PATCH net] net: prevent pulling SKB_GSO_FRAGLIST skb
> 
> BPF or TC callers may pull in a length longer than skb_headlen()
> for a SKB_GSO_FRAGLIST skb. The data in fraglist will be pulled
> into the linear space. However it destroys the skb's structure
> and may result in an invalid segmentation or kernel exception.
> 
> So we should add protection to stop the operation and return
> error to remind callers.
> 
> Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.")
> Signed-off-by: Shiming Cheng <shiming.cheng@mediatek.com>
> Signed-off-by: Lena Wang <lena.wang@mediatek.com>
> ---
>  include/linux/skbuff.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 9d24aec064e8..3eef65b3db24 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -2740,6 +2740,12 @@ pskb_may_pull_reason(struct sk_buff *skb,
> unsigned int len)


pskb_may_pull is called in far too many locations. Daniel's
suggestion was to amend skb_ensure_writable

Please start sending the patches as regular patches. They are close
enough to review normally.

>  	if (unlikely(len > skb->len))
>  		return SKB_DROP_REASON_PKT_TOO_SMALL;
>  
> +	if (skb_is_gso(skb) &&
> +	    (skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST) &&
> +	     write_len > skb_headlen(skb)) {
> +		return SKB_DROP_REASON_NOMEM;
> +	}
> +
>  	if (unlikely(!__pskb_pull_tail(skb, len - skb_headlen(skb))))
>  		return SKB_DROP_REASON_NOMEM;
>  
> -- 
> 2.18.0