Messages in this thread | | | Subject | Re: [PATCH net] udp: fix segmentation crash for GRO packet without fraglist | From | Daniel Borkmann <> | Date | Mon, 29 Apr 2024 12:15:20 +0200 |
| |
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;
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; > >
| |