Re: [PATCH net] udp: fix segmentation crash for GRO packet without fraglist
From: Daniel Borkmann
Date: Mon Apr 29 2024 - 11:33:34 EST
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@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
Signed-off-by: Lena Wang <lena.wang@xxxxxxxxxxxx>
---
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@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
Signed-off-by: Lena Wang <lena.wang@xxxxxxxxxxxx>
---
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