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

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

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;
+	}
+
 	if (unlikely(!__pskb_pull_tail(skb, len - skb_headlen(skb))))
 		return SKB_DROP_REASON_NOMEM;
 
-- 
2.18.0