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;
Also, was this patch even compile tested since there is no write_len var ?
if (unlikely(!__pskb_pull_tail(skb, len - skb_headlen(skb))))
return SKB_DROP_REASON_NOMEM;
