| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <880367ab-e9a2-d9b4-c6d6-9e2efdf04a0f@iogearbox.net>
Date: Mon, 29 Apr 2024 17:11:39 +0200
From: Daniel Borkmann <daniel@...earbox.net>
To: Lena Wang (王娜) <Lena.Wang@...iatek.com>,
"maze@...gle.com" <maze@...gle.com>,
"willemdebruijn.kernel@...il.com" <willemdebruijn.kernel@...il.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"bpf@...r.kernel.org" <bpf@...r.kernel.org>,
"steffen.klassert@...unet.com" <steffen.klassert@...unet.com>,
"kuba@...nel.org" <kuba@...nel.org>,
Shiming Cheng (成诗明)
<Shiming.Cheng@...iatek.com>, "pabeni@...hat.com" <pabeni@...hat.com>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"matthias.bgg@...il.com" <matthias.bgg@...il.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"yan@...udflare.com" <yan@...udflare.com>
Subject: Re: [PATCH net] udp: fix segmentation crash for GRO packet without
fraglist
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@...iatek.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@...iatek.com>
>>>>>>>> Signed-off-by: Lena Wang <lena.wang@...iatek.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@...iatek.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@...iatek.com>
>>> Signed-off-by: Lena Wang <lena.wang@...iatek.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
Powered by blists - more mailing lists