[PATCH net] Drop packets with invalid headers to prevent KMSAN infoleak

[PATCH net] Drop packets with invalid headers to prevent KMSAN infoleak

[Daniel Yang]

KMSAN detects uninitialized memory stored to memory by
bpf_clone_redirect(). Adding a check to the transmission path to find
malformed headers prevents this issue. Specifically, we check if the length
of the data stored in skb is less than the minimum device header length.
If so, drop the packet since the skb cannot contain a valid device header.
Also check if mac_header_len(skb) is outside the range provided of valid
device header lengths.

Testing this patch with syzbot removes the bug.

Fixes: 88264981f208 ("Merge tag 'sched_ext-for-6.12' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/sched_ext")
Reported-by: syzbot+346474e3bf0b26bd3090@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=346474e3bf0b26bd3090
Signed-off-by: Daniel Yang <danielyangkang@gmail.com>
---
 net/core/filter.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index cd3524cb3..92d8f2098 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2191,6 +2191,13 @@ static int __bpf_redirect_common(struct sk_buff *skb, struct net_device *dev,
 		return -ERANGE;
 	}
 
+	if (unlikely(skb->len < dev->min_header_len ||
+		     skb_mac_header_len(skb) < dev->min_header_len ||
+		     skb_mac_header_len(skb) > dev->hard_header_len)) {
+		kfree_skb(skb);
+		return -ERANGE;
+	}
+
 	bpf_push_mac_rcsum(skb);
 	return flags & BPF_F_INGRESS ?
 	       __bpf_rx_skb(dev, skb) : __bpf_tx_skb(dev, skb);
-- 
2.39.2

Re: [PATCH net] Drop packets with invalid headers to prevent KMSAN infoleak

[Martin KaFai Lau]

On 10/19/24 12:11 AM, Daniel Yang wrote:
> KMSAN detects uninitialized memory stored to memory by
> bpf_clone_redirect(). Adding a check to the transmission path to find
> malformed headers prevents this issue. Specifically, we check if the length
> of the data stored in skb is less than the minimum device header length.
> If so, drop the packet since the skb cannot contain a valid device header.
> Also check if mac_header_len(skb) is outside the range provided of valid
> device header lengths.
> 
> Testing this patch with syzbot removes the bug.
> 
> Fixes: 88264981f208 ("Merge tag 'sched_ext-for-6.12' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/sched_ext")

I am pretty sure this is the wrong tag.

A test in selftests/bpf is needed to reproduce and better understand this. Only 
bpf_clone_redirect() is needed to reproduce or other bpf_skb_*() helpers calls 
are needed to reproduce?


> Reported-by: syzbot+346474e3bf0b26bd3090@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=346474e3bf0b26bd3090
> Signed-off-by: Daniel Yang <danielyangkang@gmail.com>
> ---
>   net/core/filter.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index cd3524cb3..92d8f2098 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2191,6 +2191,13 @@ static int __bpf_redirect_common(struct sk_buff *skb, struct net_device *dev,
>   		return -ERANGE;
>   	}
>   
> +	if (unlikely(skb->len < dev->min_header_len ||
> +		     skb_mac_header_len(skb) < dev->min_header_len ||
> +		     skb_mac_header_len(skb) > dev->hard_header_len)) {
> +		kfree_skb(skb);
> +		return -ERANGE;
> +	}
> +
>   	bpf_push_mac_rcsum(skb);
>   	return flags & BPF_F_INGRESS ?
>   	       __bpf_rx_skb(dev, skb) : __bpf_tx_skb(dev, skb);

Re: [PATCH net] Drop packets with invalid headers to prevent KMSAN infoleak

[Daniel Yang]

> A test in selftests/bpf is needed to reproduce and better understand this.
I don't know much about self tests but I've just been using the syzbot
repro and #syz test at the link in the patch:
https://syzkaller.appspot.com/bug?extid=346474e3bf0b26bd3090. Testing
the patch showed that the uninitialized memory was not getting written
to memory.

> Only bpf_clone_redirect() is needed to reproduce or other bpf_skb_*() helpers calls
> are needed to reproduce?

From what I can see in the crash report here:
https://syzkaller.appspot.com/text?tag=CrashReport&x=10ba3ca9980000,
only bpf_clone_redirect() is needed to trigger this issue. The issue
seems to be that bpf_try_make_head_writable clones the skb and creates
uninitialized memory but __bpf_tx_skb() gets called and the ethernet
header never got written, resulting in the skb having a data section
without a proper mac header. Current check:

if (unlikely(skb->mac_header >= skb->network_header || skb->len == 0))
{
**drop packet**
}

in __bpf_redirect_common() is insufficient since it only checks if the
mac header is misordered or if the data length is 0. So, any packet
with a malformed MAC header that is not 14 bytes but is not 0 doesn't
get dropped. Adding bounds checks for mac header size should fix this.
And from what I see in the syz test of this patch, it does.

Are there any possible unexpected issues that can be caused by this?

Re: [PATCH net] Drop packets with invalid headers to prevent KMSAN infoleak

[Paolo Abeni]

On 10/22/24 03:37, Daniel Yang wrote:
> Are there any possible unexpected issues that can be caused by this?

This patch is apparently the cause of BPF self-tests failures:

test_empty_skb:FAIL:ret: veth ETH_HLEN+1 packet ingress
[redirect_ingress] unexpected ret: veth ETH_HLEN+1 packet ingress
[redirect_ingress]: actual -34 != expected 0
test_empty_skb:PASS:err: veth ETH_HLEN+1 packet ingress
[redirect_egress] 0 nsec
test_empty_skb:FAIL:ret: veth ETH_HLEN+1 packet ingress
[redirect_egress] unexpected ret: veth ETH_HLEN+1 packet ingress
[redirect_egress]: actual -34 != expected 1

Before submitting an eventual next revision, please very that BPF
self-tests are happy.

Thanks,

Paolo

Re: [PATCH net] Drop packets with invalid headers to prevent KMSAN infoleak

[Martin KaFai Lau]

On 10/21/24 6:37 PM, Daniel Yang wrote:
>> A test in selftests/bpf is needed to reproduce and better understand this.
> I don't know much about self tests but I've just been using the syzbot
> repro and #syz test at the link in the patch:
> https://syzkaller.appspot.com/bug?extid=346474e3bf0b26bd3090. Testing
> the patch showed that the uninitialized memory was not getting written
> to memory.
> 
>> Only bpf_clone_redirect() is needed to reproduce or other bpf_skb_*() helpers calls
>> are needed to reproduce?

If only bpf_clone_redirect() is needed, it should be simple to write a selftest 
to reproduce it. It also helps to catch future regression.

Please tag the next respin as "bpf" also.

> 
>  From what I can see in the crash report here:
> https://syzkaller.appspot.com/text?tag=CrashReport&x=10ba3ca9980000,
> only bpf_clone_redirect() is needed to trigger this issue. The issue
> seems to be that bpf_try_make_head_writable clones the skb and creates
> uninitialized memory but __bpf_tx_skb() gets called and the ethernet
> header never got written, resulting in the skb having a data section
> without a proper mac header. Current check:
> 
> if (unlikely(skb->mac_header >= skb->network_header || skb->len == 0))
> {
> **drop packet**
> }
> 
> in __bpf_redirect_common() is insufficient since it only checks if the
> mac header is misordered or if the data length is 0. So, any packet
> with a malformed MAC header that is not 14 bytes but is not 0 doesn't
> get dropped. Adding bounds checks for mac header size should fix this.
> And from what I see in the syz test of this patch, it does.
> 
> Are there any possible unexpected issues that can be caused by this?

Re: [PATCH net] Drop packets with invalid headers to prevent KMSAN infoleak

[Daniel Yang]

On Tue, Oct 22, 2024 at 11:14 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 10/21/24 6:37 PM, Daniel Yang wrote:
> >> A test in selftests/bpf is needed to reproduce and better understand this.
> > I don't know much about self tests but I've just been using the syzbot
> > repro and #syz test at the link in the patch:
> > https://syzkaller.appspot.com/bug?extid=346474e3bf0b26bd3090. Testing
> > the patch showed that the uninitialized memory was not getting written
> > to memory.
> >
> >> Only bpf_clone_redirect() is needed to reproduce or other bpf_skb_*() helpers calls
> >> are needed to reproduce?
>
> If only bpf_clone_redirect() is needed, it should be simple to write a selftest
> to reproduce it. It also helps to catch future regression.
>
> Please tag the next respin as "bpf" also.

I have a problem. I can't seem to build the bpf kselftests for some
reason. There is always a struct definition error:
In file included from progs/profiler1.c:5:
progs/profiler.inc.h:599:49: error: declaration of 'struct
syscall_trace_enter' will not be visible outside of t]
  599 | int tracepoint__syscalls__sys_enter_kill(struct
syscall_trace_enter* ctx)
      |                                                 ^
progs/profiler.inc.h:604:15: error: incomplete definition of type
'struct syscall_trace_enter'
  604 |         int pid = ctx->args[0];
      |                   ~~~^
progs/profiler.inc.h:599:49: note: forward declaration of 'struct
syscall_trace_enter'
  599 | int tracepoint__syscalls__sys_enter_kill(struct
syscall_trace_enter* ctx)
      |                                                 ^
progs/profiler.inc.h:605:15: error: incomplete definition of type
'struct syscall_trace_enter'
  605 |         int sig = ctx->args[1];
      |                   ~~~^
progs/profiler.inc.h:599:49: note: forward declaration of 'struct
syscall_trace_enter'
  599 | int tracepoint__syscalls__sys_enter_kill(struct
syscall_trace_enter* ctx)

I just run the following to build:
$ cd tools/testing/selftests/bpf/
$ make

I can't find anyone else encountering the same error.

Re: [PATCH net] Drop packets with invalid headers to prevent KMSAN infoleak

[Yonghong Song]

On 10/27/24 1:49 AM, Daniel Yang wrote:
> On Tue, Oct 22, 2024 at 11:14 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>> On 10/21/24 6:37 PM, Daniel Yang wrote:
>>>> A test in selftests/bpf is needed to reproduce and better understand this.
>>> I don't know much about self tests but I've just been using the syzbot
>>> repro and #syz test at the link in the patch:
>>> https://syzkaller.appspot.com/bug?extid=346474e3bf0b26bd3090. Testing
>>> the patch showed that the uninitialized memory was not getting written
>>> to memory.
>>>
>>>> Only bpf_clone_redirect() is needed to reproduce or other bpf_skb_*() helpers calls
>>>> are needed to reproduce?
>> If only bpf_clone_redirect() is needed, it should be simple to write a selftest
>> to reproduce it. It also helps to catch future regression.
>>
>> Please tag the next respin as "bpf" also.
> I have a problem. I can't seem to build the bpf kselftests for some
> reason. There is always a struct definition error:
> In file included from progs/profiler1.c:5:
> progs/profiler.inc.h:599:49: error: declaration of 'struct
> syscall_trace_enter' will not be visible outside of t]
>    599 | int tracepoint__syscalls__sys_enter_kill(struct
> syscall_trace_enter* ctx)
>        |                                                 ^
> progs/profiler.inc.h:604:15: error: incomplete definition of type
> 'struct syscall_trace_enter'
>    604 |         int pid = ctx->args[0];
>        |                   ~~~^
> progs/profiler.inc.h:599:49: note: forward declaration of 'struct
> syscall_trace_enter'
>    599 | int tracepoint__syscalls__sys_enter_kill(struct
> syscall_trace_enter* ctx)
>        |                                                 ^
> progs/profiler.inc.h:605:15: error: incomplete definition of type
> 'struct syscall_trace_enter'
>    605 |         int sig = ctx->args[1];
>        |                   ~~~^
> progs/profiler.inc.h:599:49: note: forward declaration of 'struct
> syscall_trace_enter'
>    599 | int tracepoint__syscalls__sys_enter_kill(struct
> syscall_trace_enter* ctx)
>
> I just run the following to build:
> $ cd tools/testing/selftests/bpf/
> $ make

It might be due to your .config file.
The 'struct syscall_trace_enter' is defined in kernel/trace/trace.h,
which is used in kernel/trace/trace_syscalls.c. Maybe your config
does not have CONFIG_FTRACE_SYSCALLS?

>
> I can't find anyone else encountering the same error.

Re: [PATCH net] Drop packets with invalid headers to prevent KMSAN infoleak

[Daniel Yang]

On Sun, Oct 27, 2024 at 10:42 PM Yonghong Song <yonghong.song@linux.dev> wrote:
> It might be due to your .config file.
> The 'struct syscall_trace_enter' is defined in kernel/trace/trace.h,
> which is used in kernel/trace/trace_syscalls.c. Maybe your config
> does not have CONFIG_FTRACE_SYSCALLS?
I did add it to my config but another error popped up after I tried to
rerun the tests. I just ended up using the vmtest.sh script and got
the tests working. Seems like there should be a template config file
or something to make building locally more convenient. Anyways, thanks
for the help.

- Daniel

Re: [PATCH net] Drop packets with invalid headers to prevent KMSAN infoleak

[Alexander Lobakin]

From: Daniel Yang <danielyangkang@gmail.com>
Date: Sat, 19 Oct 2024 00:11:39 -0700

> KMSAN detects uninitialized memory stored to memory by
> bpf_clone_redirect(). Adding a check to the transmission path to find
> malformed headers prevents this issue. Specifically, we check if the length
> of the data stored in skb is less than the minimum device header length.
> If so, drop the packet since the skb cannot contain a valid device header.
> Also check if mac_header_len(skb) is outside the range provided of valid
> device header lengths.
> 
> Testing this patch with syzbot removes the bug.
> 
> Fixes: 88264981f208 ("Merge tag 'sched_ext-for-6.12' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/sched_ext")
> Reported-by: syzbot+346474e3bf0b26bd3090@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=346474e3bf0b26bd3090
> Signed-off-by: Daniel Yang <danielyangkang@gmail.com>
> ---
>  net/core/filter.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index cd3524cb3..92d8f2098 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2191,6 +2191,13 @@ static int __bpf_redirect_common(struct sk_buff *skb, struct net_device *dev,
>  		return -ERANGE;
>  	}
>  
> +	if (unlikely(skb->len < dev->min_header_len ||
> +		     skb_mac_header_len(skb) < dev->min_header_len ||
> +		     skb_mac_header_len(skb) > dev->hard_header_len)) {
> +		kfree_skb(skb);
> +		return -ERANGE;
> +	}

I believe this should go under IS_ENABLED(CONFIG_KMSAN) or
CONFIG_DEBUG_NET or so to not affect the regular configurations.
Or does this fix some real bug?

> +
>  	bpf_push_mac_rcsum(skb);
>  	return flags & BPF_F_INGRESS ?
>  	       __bpf_rx_skb(dev, skb) : __bpf_tx_skb(dev, skb);

Thanks,
Olek

Re: [PATCH net] Drop packets with invalid headers to prevent KMSAN infoleak

[Daniel Yang]

On Tue, Oct 29, 2024 at 9:41 AM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
> > +     if (unlikely(skb->len < dev->min_header_len ||
> > +                  skb_mac_header_len(skb) < dev->min_header_len ||
> > +                  skb_mac_header_len(skb) > dev->hard_header_len)) {
> > +             kfree_skb(skb);
> > +             return -ERANGE;
> > +     }
>
> I believe this should go under IS_ENABLED(CONFIG_KMSAN) or
> CONFIG_DEBUG_NET or so to not affect the regular configurations.
> Or does this fix some real bug?

Well in my opinion, an infoleak is still an infoleak. But, this would
likely not get triggered as long as an skb with a properly initialized
eth header is passed into the bpf_clone_redirect function. We could
initialize the memory to 0 but the performance hit would be too much.
If the bpf_clone_redirect() function cannot be called from user space
with user-crafted skbs as input, I don't think this is really an issue
and we can just put it under the macros to get rid of the syzbot
error.

- Daniel

[PATCH net] Drop packets with invalid headers to prevent KMSAN infoleak

[Daniel Yang]

KMSAN detects uninitialized memory stored to memory by
bpf_clone_redirect(). Adding a check to the transmission path to find
malformed headers prevents this issue. Specifically, we check if the length
of the data stored in skb is less than the minimum device header length. If
so, drop the packet since the skb cannot contain a valid device header.
Also check if mac_header_len(skb) is outside the range provided of valid
device header lengths.

Testing this patch with syzbot removes the bug.

Macro added to not affect normal builds.

Fixes: 88264981f208 ("Merge tag 'sched_ext-for-6.12' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/sched_ext")
Reported-by: syzbot+346474e3bf0b26bd3090@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=346474e3bf0b26bd3090
Signed-off-by: Daniel Yang <danielyangkang@gmail.com>
---
v1: Enclosed in macro to not affect normal builds

 net/core/filter.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index cd3524cb3..9c5786f9c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2191,6 +2191,14 @@ static int __bpf_redirect_common(struct sk_buff *skb, struct net_device *dev,
 		return -ERANGE;
 	}
 
+#if IS_ENABLED(CONFIG_KMSAN)
+	if (unlikely(skb->len < dev->min_header_len ||
+		     skb_mac_header_len(skb) < dev->min_header_len ||
+		     skb_mac_header_len(skb) > dev->hard_header_len)) {
+		kfree_skb(skb);
+		return -ERANGE;
+	}
+#endif
 	bpf_push_mac_rcsum(skb);
 	return flags & BPF_F_INGRESS ?
 	       __bpf_rx_skb(dev, skb) : __bpf_tx_skb(dev, skb);
-- 
2.39.2

Re: [PATCH net] Drop packets with invalid headers to prevent KMSAN infoleak

[Eric Dumazet]

On Mon, Nov 4, 2024 at 5:02 AM Daniel Yang <danielyangkang@gmail.com> wrote:
>
> KMSAN detects uninitialized memory stored to memory by
> bpf_clone_redirect(). Adding a check to the transmission path to find
> malformed headers prevents this issue. Specifically, we check if the length
> of the data stored in skb is less than the minimum device header length. If
> so, drop the packet since the skb cannot contain a valid device header.
> Also check if mac_header_len(skb) is outside the range provided of valid
> device header lengths.
>
> Testing this patch with syzbot removes the bug.
>
> Macro added to not affect normal builds.
>
> Fixes: 88264981f208 ("Merge tag 'sched_ext-for-6.12' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/sched_ext")
> Reported-by: syzbot+346474e3bf0b26bd3090@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=346474e3bf0b26bd3090
> Signed-off-by: Daniel Yang <danielyangkang@gmail.com>
> ---
> v1: Enclosed in macro to not affect normal builds
>
>  net/core/filter.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index cd3524cb3..9c5786f9c 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2191,6 +2191,14 @@ static int __bpf_redirect_common(struct sk_buff *skb, struct net_device *dev,
>                 return -ERANGE;
>         }
>
> +#if IS_ENABLED(CONFIG_KMSAN)
> +       if (unlikely(skb->len < dev->min_header_len ||
> +                    skb_mac_header_len(skb) < dev->min_header_len ||
> +                    skb_mac_header_len(skb) > dev->hard_header_len)) {
> +               kfree_skb(skb);
> +               return -ERANGE;
> +       }
> +#endif
>         bpf_push_mac_rcsum(skb);
>         return flags & BPF_F_INGRESS ?
>                __bpf_rx_skb(dev, skb) : __bpf_tx_skb(dev, skb);
> --
> 2.39.2
>

I am not a BPF maintainer, but for the record I think it is wrong to
silence KMSAN and give the impression a bug is 'removed'.