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

[Daniel Borkmann <daniel@iogearbox.net>]

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