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

["Lena Wang (王娜)" <Lena.Wang@mediatek.com>]

On Mon, 2024-04-29 at 12:15 +0200, Daniel Borkmann wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 4/28/24 9:48 AM, 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)
> >   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;
> > +}
> 
> The 'write_len > skb_headlen(skb)' test is redundant, no ?
> 
> It is covered by the earlier test :
> 
>          if (likely(len <= skb_headlen(skb)))
>                  return SKB_NOT_DROPPED_YET;
> 
Daniel, it is not redundant. The bpf pulls a len between
skb_headlen(skb) and skb->len that results in error. Here it will stop
this operation. For other skbs(not SKB_GSO_FRAGLIST) it could be a
normal behaviour and will continue to do next pulling.

> Also, was this patch even compile tested since there is no write_len
> var ?
> 
The check is moved into skb_ensure_writable as advice and a new mail
loop is created to start upstream:

From 36307cb8706653a63a389faadbb1324cda5c0445 Mon Sep 17 00:00:00 2001
F
rom: Shiming Cheng <shiming.cheng@mediatek.com>
Date: Sun, 28 Apr 2024
21:50:11 +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>
---
 net/core/skbuff.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f68f2679b086..2d35e009e814 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -6100,6 +6100,12 @@ EXPORT_SYMBOL(skb_vlan_untag);
 
 int skb_ensure_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;
+	}
+
 	if (!pskb_may_pull(skb, write_len))
 		return -ENOMEM;
 
-- 
2.18.0

> >   if (unlikely(!__pskb_pull_tail(skb, len - skb_headlen(skb))))
> >   return SKB_DROP_REASON_NOMEM;
> >   
> > 
>