Messages in this thread Patch in this message | | | Subject | Re: [PATCH net] udp: fix segmentation crash for GRO packet without fraglist | From | Daniel Borkmann <> | Date | Mon, 29 Apr 2024 17:11:39 +0200 |
| |
On 4/29/24 1:45 PM, Lena Wang (王娜) wrote: > 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.
I meant something like the below. The len <= skb_headlen(skb) case you already return earlier with SKB_NOT_DROPPED_YET. Willem, do you see a case where this should not live in pskb_may_pull_reason() but rather specifically in skb_ensure_writable()?
Thanks, Daniel
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 517e546a120a..ef2a0328ff2b 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -1687,6 +1687,11 @@ int skb_zerocopy_iter_stream(struct sock *sk, struct sk_buff *skb, /* Internal */ #define skb_shinfo(SKB) ((struct skb_shared_info *)(skb_end_pointer(SKB)))
+static inline bool skb_is_gso(const struct sk_buff *skb) +{ + return skb_shinfo(skb)->gso_size; +} + static inline struct skb_shared_hwtstamps *skb_hwtstamps(struct sk_buff *skb) { return &skb_shinfo(skb)->hwtstamps; @@ -2740,6 +2745,10 @@ pskb_may_pull_reason(struct sk_buff *skb, unsigned int len) if (unlikely(len > skb->len)) return SKB_DROP_REASON_PKT_TOO_SMALL;
+ if (unlikely(skb_is_gso(skb) && + (skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST))) + return SKB_DROP_REASON_FRAGLIST_PULL; + if (unlikely(!__pskb_pull_tail(skb, len - skb_headlen(skb)))) return SKB_DROP_REASON_NOMEM;
@@ -4953,11 +4962,6 @@ static inline struct sec_path *skb_sec_path(const struct sk_buff *skb) #endif }
-static inline bool skb_is_gso(const struct sk_buff *skb) -{ - return skb_shinfo(skb)->gso_size; -} - /* Note: Should be called only if skb_is_gso(skb) is true */ static inline bool skb_is_gso_v6(const struct sk_buff *skb) { diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h index 9707ab54fdd5..9d6c97a6b2b6 100644 --- a/include/net/dropreason-core.h +++ b/include/net/dropreason-core.h @@ -418,6 +418,12 @@ enum skb_drop_reason { * iterations. */ SKB_DROP_REASON_TC_RECLASSIFY_LOOP, + /** + * @SKB_DROP_REASON_FRAGLIST_PULL: attempting to pull GSO fraglist + * which destroys the skb's structure and may then result in an + * invalid segmentation or kernel exception. + */ + SKB_DROP_REASON_FRAGLIST_PULL, /** * @SKB_DROP_REASON_MAX: the maximum of core drop reasons, which * shouldn't be used as a real 'reason' - only for tracing code gen -- 2.21.0
| |