[PATCH] bpf: make sure mac_header was set before using it

[PATCH] bpf: make sure mac_header was set before using it

[Eric Dumazet]

Classic BPF has a way to load bytes starting from the mac header.

Some skbs do not have a mac header, and skb_mac_header()
in this case is returning a pointer that 65535 bytes after
skb->head.

Existing range check in bpf_internal_load_pointer_neg_helper()
was properly kicking and no illegal access was happening.

New sanity check in skb_mac_header() is firing, so we need
to avoid it.

WARNING: CPU: 1 PID: 28990 at include/linux/skbuff.h:2785 skb_mac_header include/linux/skbuff.h:2785 [inline]
WARNING: CPU: 1 PID: 28990 at include/linux/skbuff.h:2785 bpf_internal_load_pointer_neg_helper+0x1b1/0x1c0 kernel/bpf/core.c:74
Modules linked in:
CPU: 1 PID: 28990 Comm: syz-executor.0 Not tainted 5.19.0-rc4-syzkaller-00865-g4874fb9484be #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 06/29/2022
RIP: 0010:skb_mac_header include/linux/skbuff.h:2785 [inline]
RIP: 0010:bpf_internal_load_pointer_neg_helper+0x1b1/0x1c0 kernel/bpf/core.c:74
Code: ff ff 45 31 f6 e9 5a ff ff ff e8 aa 27 40 00 e9 3b ff ff ff e8 90 27 40 00 e9 df fe ff ff e8 86 27 40 00 eb 9e e8 2f 2c f3 ff <0f> 0b eb b1 e8 96 27 40 00 e9 79 fe ff ff 90 41 57 41 56 41 55 41
RSP: 0018:ffffc9000309f668 EFLAGS: 00010216
RAX: 0000000000000118 RBX: ffffffffffeff00c RCX: ffffc9000e417000
RDX: 0000000000040000 RSI: ffffffff81873f21 RDI: 0000000000000003
RBP: ffff8880842878c0 R08: 0000000000000003 R09: 000000000000ffff
R10: 000000000000ffff R11: 0000000000000001 R12: 0000000000000004
R13: ffff88803ac56c00 R14: 000000000000ffff R15: dffffc0000000000
FS: 00007f5c88a16700(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fdaa9f6c058 CR3: 000000003a82c000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
____bpf_skb_load_helper_32 net/core/filter.c:276 [inline]
bpf_skb_load_helper_32+0x191/0x220 net/core/filter.c:264

Fixes: f9aefd6b2aa3 ("net: warn if mac header was not set")
Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 kernel/bpf/core.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index b5ffebcce6ccae22ad3d252c823b7a61d7bfd206..9d17baac1144227f61bdbd69294a3ad70bf26389 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -68,11 +68,13 @@ void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, uns
 {
 	u8 *ptr = NULL;
 
-	if (k >= SKF_NET_OFF)
+	if (k >= SKF_NET_OFF) {
 		ptr = skb_network_header(skb) + k - SKF_NET_OFF;
-	else if (k >= SKF_LL_OFF)
+	} else if (k >= SKF_LL_OFF) {
+		if (unlikely(!skb_mac_header_was_set(skb)))
+			return NULL;
 		ptr = skb_mac_header(skb) + k - SKF_LL_OFF;
-
+	}
 	if (ptr >= skb->head && ptr + size <= skb_tail_pointer(skb))
 		return ptr;
 
-- 
2.37.0.rc0.161.g10f37bed90-goog

Re: [PATCH] bpf: make sure mac_header was set before using it

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Thu,  7 Jul 2022 12:39:00 +0000 you wrote:
> Classic BPF has a way to load bytes starting from the mac header.
> 
> Some skbs do not have a mac header, and skb_mac_header()
> in this case is returning a pointer that 65535 bytes after
> skb->head.
> 
> Existing range check in bpf_internal_load_pointer_neg_helper()
> was properly kicking and no illegal access was happening.
> 
> [...]

Here is the summary with links:
  - bpf: make sure mac_header was set before using it
    https://git.kernel.org/bpf/bpf/c/0326195f523a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH] bpf: make sure mac_header was set before using it

[Alexei Starovoitov]

On Thu, Jul 7, 2022 at 11:20 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
>
> Hello:
>
> This patch was applied to bpf/bpf.git (master)

Are we sure it's bpf tree material?
The fixes tag points to net-next tree.

> by Daniel Borkmann <daniel@iogearbox.net>:
>
> On Thu,  7 Jul 2022 12:39:00 +0000 you wrote:
> > Classic BPF has a way to load bytes starting from the mac header.
> >
> > Some skbs do not have a mac header, and skb_mac_header()
> > in this case is returning a pointer that 65535 bytes after
> > skb->head.
> >
> > Existing range check in bpf_internal_load_pointer_neg_helper()
> > was properly kicking and no illegal access was happening.
> >
> > [...]
>
> Here is the summary with links:
>   - bpf: make sure mac_header was set before using it
>     https://git.kernel.org/bpf/bpf/c/0326195f523a
>
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html
>
>

Re: [PATCH] bpf: make sure mac_header was set before using it

[Eric Dumazet]

On Thu, Jul 7, 2022 at 8:31 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Jul 7, 2022 at 11:20 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
> >
> > Hello:
> >
> > This patch was applied to bpf/bpf.git (master)
>
> Are we sure it's bpf tree material?
> The fixes tag points to net-next tree.

Fix is generic and should not harm bpf tree, or any tree if that matters.

Sorry for not adding the net-next tag in the [PATCH].

>
> > by Daniel Borkmann <daniel@iogearbox.net>:
> >
> > On Thu,  7 Jul 2022 12:39:00 +0000 you wrote:
> > > Classic BPF has a way to load bytes starting from the mac header.
> > >
> > > Some skbs do not have a mac header, and skb_mac_header()
> > > in this case is returning a pointer that 65535 bytes after
> > > skb->head.
> > >
> > > Existing range check in bpf_internal_load_pointer_neg_helper()
> > > was properly kicking and no illegal access was happening.
> > >
> > > [...]
> >
> > Here is the summary with links:
> >   - bpf: make sure mac_header was set before using it
> >     https://git.kernel.org/bpf/bpf/c/0326195f523a
> >
> > You are awesome, thank you!
> > --
> > Deet-doot-dot, I am a bot.
> > https://korg.docs.kernel.org/patchwork/pwbot.html
> >
> >

Re: [PATCH] bpf: make sure mac_header was set before using it

[Alexei Starovoitov]

On Thu, Jul 7, 2022 at 11:36 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Jul 7, 2022 at 8:31 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Thu, Jul 7, 2022 at 11:20 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
> > >
> > > Hello:
> > >
> > > This patch was applied to bpf/bpf.git (master)
> >
> > Are we sure it's bpf tree material?
> > The fixes tag points to net-next tree.
>
> Fix is generic and should not harm bpf tree, or any tree if that matters.

Right. Just trying to understand the urgency/severity
considering we're at rc5.

> Sorry for not adding the net-next tag in the [PATCH].
>
> >
> > > by Daniel Borkmann <daniel@iogearbox.net>:
> > >
> > > On Thu,  7 Jul 2022 12:39:00 +0000 you wrote:
> > > > Classic BPF has a way to load bytes starting from the mac header.
> > > >
> > > > Some skbs do not have a mac header, and skb_mac_header()
> > > > in this case is returning a pointer that 65535 bytes after
> > > > skb->head.
> > > >
> > > > Existing range check in bpf_internal_load_pointer_neg_helper()
> > > > was properly kicking and no illegal access was happening.
> > > >
> > > > [...]
> > >
> > > Here is the summary with links:
> > >   - bpf: make sure mac_header was set before using it
> > >     https://git.kernel.org/bpf/bpf/c/0326195f523a
> > >
> > > You are awesome, thank you!
> > > --
> > > Deet-doot-dot, I am a bot.
> > > https://korg.docs.kernel.org/patchwork/pwbot.html
> > >
> > >

Re: [PATCH] bpf: make sure mac_header was set before using it

[Daniel Borkmann]

On 7/7/22 8:40 PM, Alexei Starovoitov wrote:
> On Thu, Jul 7, 2022 at 11:36 AM Eric Dumazet <edumazet@google.com> wrote:
>> On Thu, Jul 7, 2022 at 8:31 PM Alexei Starovoitov
>> <alexei.starovoitov@gmail.com> wrote:
>>> On Thu, Jul 7, 2022 at 11:20 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
>>>>
>>>> Hello:
>>>>
>>>> This patch was applied to bpf/bpf.git (master)
>>>
>>> Are we sure it's bpf tree material?
>>> The fixes tag points to net-next tree.
>>
>> Fix is generic and should not harm bpf tree, or any tree if that matters.
> 
> Right. Just trying to understand the urgency/severity
> considering we're at rc5.

The Fixes tag points to the warning that has been added. I understand more
as a reference rather than the actual underlying bug that syzkaller was
able to trigger w/ classic bpf. So yeah, bpf tree seems reasonable, imho,
but also low risk from patch diff itself.

Thanks,
Daniel