bpf: Ensure BPF programs testing skb context initialization

Commit Message

Michal Switala July 10, 2024, 8:46 a.m. UTC

This commit addresses an issue where a netdevice was found to be uninitialized.
To mitigate this case, the change ensures that BPF programs designed to test
skb context initialization thoroughly verify the availability of a fully
initialized context before execution.The root cause of a NULL ctx stems from
the initialization process in bpf_ctx_init(). This function returns NULL if
the user initializes the bpf_attr variables ctx_in and ctx_out with invalid
pointers or sets them to NULL. These variables are directly controlled by user
input, and if both are NULL, the context cannot be initialized, resulting in a
NULL ctx.

Reported-by: [e-mail]
Closes: https://syzkaller.appspot.com/bug?extid=cca39e6e84a367a7e6f6
Link: https://lore.kernel.org/all/[e-mail]/
Signed-off-by: Michal Switala <[e-mail]>
---
 net/bpf/test_run.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

Comments

Alexei Starovoitov July 10, 2024, 6:38 p.m. UTC | #1

On Wed, Jul 10, 2024 at 4:58 AM Michal Switala
<[e-mail]> wrote:
>
> This commit addresses an issue where a netdevice was found to be uninitialized.
> To mitigate this case, the change ensures that BPF programs designed to test
> skb context initialization thoroughly verify the availability of a fully
> initialized context before execution.The root cause of a NULL ctx stems from
> the initialization process in bpf_ctx_init(). This function returns NULL if
> the user initializes the bpf_attr variables ctx_in and ctx_out with invalid
> pointers or sets them to NULL. These variables are directly controlled by user
> input, and if both are NULL, the context cannot be initialized, resulting in a
> NULL ctx.
>
> Reported-by: [e-mail]
> Closes: https://syzkaller.appspot.com/bug?extid=cca39e6e84a367a7e6f6
> Link: https://lore.kernel.org/all/[e-mail]/

Something doesn't add up.
This syzbot report is about:

dev_map_enqueue+0x31/0x3e0 kernel/bpf/devmap.c:539
__xdp_do_redirect_frame net/core/filter.c:4397 [inline]
bpf_prog_test_run_xdp

while you're fixing bpf_prog_test_run_skb ?

pw-bot: cr

> Signed-off-by: Michal Switala <[e-mail]>
> ---
>  net/bpf/test_run.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 36ae54f57bf5..8b2efcee059f 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -970,7 +970,7 @@ static struct proto bpf_dummy_proto = {
>  int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>                           union bpf_attr __user *uattr)
>  {
> -       bool is_l2 = false, is_direct_pkt_access = false;
> +       bool is_l2 = false, is_direct_pkt_access = false, ctx_needed = false;
>         struct net *net = current->nsproxy->net_ns;
>         struct net_device *dev = net->loopback_dev;
>         u32 size = kattr->test.data_size_in;
> @@ -998,6 +998,34 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>                 return PTR_ERR(ctx);
>         }
>
> +       switch (prog->type) {
> +       case BPF_PROG_TYPE_SOCKET_FILTER:
> +       case BPF_PROG_TYPE_SCHED_CLS:
> +       case BPF_PROG_TYPE_SCHED_ACT:
> +       case BPF_PROG_TYPE_XDP:
> +       case BPF_PROG_TYPE_CGROUP_SKB:
> +       case BPF_PROG_TYPE_CGROUP_SOCK:
> +       case BPF_PROG_TYPE_SOCK_OPS:
> +       case BPF_PROG_TYPE_SK_SKB:
> +       case BPF_PROG_TYPE_SK_MSG:
> +       case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
> +       case BPF_PROG_TYPE_LWT_SEG6LOCAL:
> +       case BPF_PROG_TYPE_SK_REUSEPORT:
> +       case BPF_PROG_TYPE_NETFILTER:
> +       case BPF_PROG_TYPE_LWT_IN:
> +       case BPF_PROG_TYPE_LWT_OUT:
> +       case BPF_PROG_TYPE_LWT_XMIT:
> +               ctx_needed = true;
> +               break;
> +       default:
> +               break;
> +       }
> +
> +       if (!ctx && ctx_needed) {
> +               kfree(data);
> +               return -EINVAL;
> +       }
> +
>         switch (prog->type) {
>         case BPF_PROG_TYPE_SCHED_CLS:
>         case BPF_PROG_TYPE_SCHED_ACT:
> --
> 2.43.0
>
>

Michal Switala July 15, 2024, 6:13 p.m. UTC | #2

Hi,

The reproducer calls the methods bpf_prog_test_run_xdp and
bpf_prog_test_run_skb. Both lead to the invocation of dev_map_enqueue, in the
case of the former, the backtrace is recorded in its entirety, whereas for the
latter it is not. I think the bug might be incorrectly reported on syzkaller, as
during GDB debugging, the problem occurred in functions called from
bpf_prog_test_run_skb. I also ran testing of my patch on syzkaller and the tests
passed.

Regards
Michal

Martin KaFai Lau July 15, 2024, 9:59 p.m. UTC | #3

On 7/15/24 11:13 AM, Michal Switala wrote:

 >> Reported-by: [e-mail]
 >> Closes: https://syzkaller.appspot.com/bug?extid=cca39e6e84a367a7e6f6
 >> Link: https://lore.kernel.org/all/[e-mail]/
 >
 > Something doesn't add up.
 > This syzbot report is about:
 >
 > dev_map_enqueue+0x31/0x3e0 kernel/bpf/devmap.c:539
 > __xdp_do_redirect_frame net/core/filter.c:4397 [inline]
 > bpf_prog_test_run_xdp
 >
 > why you're fixing bpf_prog_test_run_skb ?


[ Please keep the relevant email context in the reply ]


> The reproducer calls the methods bpf_prog_test_run_xdp and
> bpf_prog_test_run_skb. Both lead to the invocation of dev_map_enqueue, in the

The syzbot report is triggering from the bpf_prog_test_run_xdp. I agree with 
Alexei that fixing the bpf_prog_test_run_skb does not make sense. At least I 
don't see how dev_map_enqueue can be used from bpf_prog_test_run_skb.

It looks very similar to 
https://lore.kernel.org/bpf/[e-mail]/. It has 
been fixed in commit 5bcf0dcbf906 ("xdp: use flags field to disambiguate 
broadcast redirect")

I tried the C repro. I can reproduce in the bpf tree also which should have the 
fix. I cannot reproduce in the bpf-next though.

Cc Toke who knows more details here.

> case of the former, the backtrace is recorded in its entirety, whereas for the
> latter it is not. I think the bug might be incorrectly reported on syzkaller, as
> during GDB debugging, the problem occurred in functions called from
> bpf_prog_test_run_skb. I also ran testing of my patch on syzkaller and the tests
> passed.

Toke Høiland-Jørgensen July 17, 2024, 1:28 p.m. UTC | #4

Martin KaFai Lau <[e-mail]> writes:

> On 7/15/24 11:13 AM, Michal Switala wrote:
>
>  >> Reported-by: [e-mail]
>  >> Closes: https://syzkaller.appspot.com/bug?extid=cca39e6e84a367a7e6f6
>  >> Link: https://lore.kernel.org/all/[e-mail]/
>  >
>  > Something doesn't add up.
>  > This syzbot report is about:
>  >
>  > dev_map_enqueue+0x31/0x3e0 kernel/bpf/devmap.c:539
>  > __xdp_do_redirect_frame net/core/filter.c:4397 [inline]
>  > bpf_prog_test_run_xdp
>  >
>  > why you're fixing bpf_prog_test_run_skb ?
>
>
> [ Please keep the relevant email context in the reply ]
>
>
>> The reproducer calls the methods bpf_prog_test_run_xdp and
>> bpf_prog_test_run_skb. Both lead to the invocation of dev_map_enqueue, in the
>
> The syzbot report is triggering from the bpf_prog_test_run_xdp. I agree with 
> Alexei that fixing the bpf_prog_test_run_skb does not make sense. At least I 
> don't see how dev_map_enqueue can be used from bpf_prog_test_run_skb.

Me neither.

> It looks very similar to 
> https://lore.kernel.org/bpf/[e-mail]/. It has 
> been fixed in commit 5bcf0dcbf906 ("xdp: use flags field to disambiguate 
> broadcast redirect")
>
> I tried the C repro. I can reproduce in the bpf tree also which should have the 
> fix. I cannot reproduce in the bpf-next though.
>
> Cc Toke who knows more details here.

Hmm, yeah, it does look kinda similar. Do you mean that the C repro from
this new report triggers the crash for you on the current -bpf tree?

-Toke

Martin KaFai Lau July 17, 2024, 7:16 p.m. UTC | #5

On 7/17/24 6:28 AM, Toke Høiland-Jørgensen wrote:
>> It looks very similar to
>> https://lore.kernel.org/bpf/[e-mail]/. It has
>> been fixed in commit 5bcf0dcbf906 ("xdp: use flags field to disambiguate
>> broadcast redirect")
>>
>> I tried the C repro. I can reproduce in the bpf tree also which should have the
>> fix. I cannot reproduce in the bpf-next though.
>>
>> Cc Toke who knows more details here.
> 
> Hmm, yeah, it does look kinda similar. Do you mean that the C repro from
> this new report triggers the crash for you on the current -bpf tree?

I was able to repro in bpf tree ~two days ago but not now. The bpf tree has been 
fast forwarded and has the 6.10 changes. I just tried linux-stable/linux-6.9.y 
which has the fix in the commit 5bcf0dcbf906. The syzbot report (against the 
36534d3c5453) also has that fix.

In particular, the syzbot repro I tried:
https://syzkaller.appspot.com/text?tag=ReproC&x=17caa30a980000

Patch

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 36ae54f57bf5..8b2efcee059f 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -970,7 +970,7 @@  static struct proto bpf_dummy_proto = {
 int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 			  union bpf_attr __user *uattr)
 {
-	bool is_l2 = false, is_direct_pkt_access = false;
+	bool is_l2 = false, is_direct_pkt_access = false, ctx_needed = false;
 	struct net *net = current->nsproxy->net_ns;
 	struct net_device *dev = net->loopback_dev;
 	u32 size = kattr->test.data_size_in;
@@ -998,6 +998,34 @@  int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 		return PTR_ERR(ctx);
 	}
 
+	switch (prog->type) {
+	case BPF_PROG_TYPE_SOCKET_FILTER:
+	case BPF_PROG_TYPE_SCHED_CLS:
+	case BPF_PROG_TYPE_SCHED_ACT:
+	case BPF_PROG_TYPE_XDP:
+	case BPF_PROG_TYPE_CGROUP_SKB:
+	case BPF_PROG_TYPE_CGROUP_SOCK:
+	case BPF_PROG_TYPE_SOCK_OPS:
+	case BPF_PROG_TYPE_SK_SKB:
+	case BPF_PROG_TYPE_SK_MSG:
+	case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
+	case BPF_PROG_TYPE_LWT_SEG6LOCAL:
+	case BPF_PROG_TYPE_SK_REUSEPORT:
+	case BPF_PROG_TYPE_NETFILTER:
+	case BPF_PROG_TYPE_LWT_IN:
+	case BPF_PROG_TYPE_LWT_OUT:
+	case BPF_PROG_TYPE_LWT_XMIT:
+		ctx_needed = true;
+		break;
+	default:
+		break;
+	}
+
+	if (!ctx && ctx_needed) {
+		kfree(data);
+		return -EINVAL;
+	}
+
 	switch (prog->type) {
 	case BPF_PROG_TYPE_SCHED_CLS:
 	case BPF_PROG_TYPE_SCHED_ACT: