Skip to contentSkip to conversation

Avoid dummy bpf_offload_netdev in __bpf_prog_dev_bound_init

PATCH Applied Eduard Zingerman 3 participants 6 messages Sep 12, 2023

Subsystems (4)
Kselftest BPF BPF selftests XDP
lore
Share & tools
lore

Eduard Zingerman

author maintainer
#0 lore raw

cover Avoid dummy bpf_offload_netdev in __bpf_prog_dev_bound_init

For a device bound BPF program with flag BPF_F_XDP_DEV_BOUND_ONLY, in case if device does not support offload, __bpf_prog_dev_bound_init() creates a dummy bpf_offload_netdev struct with .offdev field set to NULL.

This dummy struct might be reused for programs without this flag bound to the same device. However, bpf_prog_offload_verifier_prep() that uses bpf_offload_netdev assumes that .offdev field cannot be NULL.

This bug was reported by syzbot in [1].

[1] https://lore.kernel.org/bpf/[email protected]/

Eduard Zingerman (2):
bpf: Avoid dummy bpf_offload_netdev in __bpf_prog_dev_bound_init selftests/bpf: Offloaded prog after non-offloaded should not cause BUG

 kernel/bpf/offload.c                          | 12 ++--
 .../bpf/prog_tests/xdp_dev_bound_only.c       | 58 +++++++++++++++++++
 2 files changed, 65 insertions(+), 5 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c
Signature · 1 line
2.41.0
author maintainer
#1 lore raw

PATCH 1/2 bpf: Avoid dummy bpf_offload_netdev in __bpf_prog_dev_bound_init

Fix for a bug observable under the following sequence of events:
1. Create a network device that does not support XDP offload.
2. Load a device bound XDP program with BPF_F_XDP_DEV_BOUND_ONLY flag (such programs are not offloaded).
3. Load a device bound XDP program with zero flags
(such programs are offloaded).

At step (2) __bpf_prog_dev_bound_init() associates with device (1) a dummy bpf_offload_netdev struct with .offdev field set to NULL. At step (3) __bpf_prog_dev_bound_init() would reuse dummy struct allocated at step (2).
However, downstream usage of the bpf_offload_netdev assumes that .offdev field can't be NULL, e.g. in bpf_prog_offload_verifier_prep().

Adjust __bpf_prog_dev_bound_init() to require bpf_offload_netdev with non-NULL .offdev for offloaded BPF programs.

Fixes 2b3486bc2d23 ("bpf: Introduce device-bound XDP programs")
Reported-by [email protected]
Closes https://lore.kernel.org/bpf/[email protected]/ open here
Signed-off-by Eduard Zingerman <[email protected]>
---
 kernel/bpf/offload.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
kernel/bpf/offload.c +7 -5
index 3e4f2ec1af06..87d6693d8233 100644
@@ -199,12 +199,14 @@ static int __bpf_prog_dev_bound_init(struct bpf_prog *prog, struct net_device *n
 	offload->netdev = netdev;
 
 	ondev = bpf_offload_find_netdev(offload->netdev);
+	/* When program is offloaded require presence of "true"
+	 * bpf_offload_netdev, avoid the one created for !ondev case below.
+	 */
+	if (bpf_prog_is_offloaded(prog->aux) && (!ondev || !ondev->offdev)) {
+		err = -EINVAL;
+		goto err_free;
+	}
 	if (!ondev) {
-		if (bpf_prog_is_offloaded(prog->aux)) {
-			err = -EINVAL;
-			goto err_free;
-		}
-
 		/* When only binding to the device, explicitly
 		 * create an entry in the hashtable.
 		 */
Signature · 1 line
2.41.0
author maintainer
#2 lore raw

PATCH 2/2 selftests/bpf: Offloaded prog after non-offloaded should not cause BUG

In reply to Eduard Zingerman

Check what happens if non-offloaded dev bound BPF program is followed by offloaded dev bound program. Test case adapated from syzbot report [1].

[1] https://lore.kernel.org/bpf/[email protected]/

Signed-off-by Eduard Zingerman <[email protected]>
---
 .../bpf/prog_tests/xdp_dev_bound_only.c       | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c
tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c new file +58 -0
new file mode 100644
index 000000000000..5ee4c16d2e21
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <net/if.h>
+#include <test_progs.h>
+#include <network_helpers.h>
+
+#define LOCAL_NETNS "xdp_dev_bound_only_netns"
+
+int load_dummy_prog(char *name, __u32 ifindex, __u32 flags)
+{
+	struct bpf_insn insns[] = { BPF_MOV64_IMM(BPF_REG_0, 0), BPF_EXIT_INSN() };
+	LIBBPF_OPTS(bpf_prog_load_opts, opts);
+
+	opts.prog_flags = flags;
+	opts.prog_ifindex = ifindex;
+	return bpf_prog_load(BPF_PROG_TYPE_XDP, name, "GPL", insns, ARRAY_SIZE(insns), &opts);
+}
+
+/* A test case for bpf_offload_netdev->offload handling bug:
+ * - create a veth device (does not support offload);
+ * - create a device bound XDP program with BPF_F_XDP_DEV_BOUND_ONLY flag
+ *   (such programs are not offloaded);
+ * - create a device bound XDP program without flags (such programs are offloaded).
+ * This might lead to 'BUG: kernel NULL pointer dereference'.
+ */
+void test_xdp_dev_bound_only_offdev(void)
+{
+	struct nstoken *tok = NULL;
+	__u32 ifindex;
+	int fd1 = -1;
+	int fd2 = -1;
+
+	SYS(out, "ip netns add " LOCAL_NETNS);
+	tok = open_netns(LOCAL_NETNS);
+	SYS(out, "ip link add eth42 type veth");
+	ifindex = if_nametoindex("eth42");
+	if (!ASSERT_NEQ(ifindex, 0, "if_nametoindex")) {
+		perror("if_nametoindex");
+		goto out;
+	}
+	fd1 = load_dummy_prog("dummy1", ifindex, BPF_F_XDP_DEV_BOUND_ONLY);
+	if (!ASSERT_GE(fd1, 0, "load_dummy_prog #1")) {
+		perror("load_dummy_prog #1");
+		goto out;
+	}
+	/* Program with ifindex is considered offloaded, however veth
+	 * does not support offload => error should be reported.
+	 */
+	fd2 = load_dummy_prog("dummy2", ifindex, 0);
+	ASSERT_EQ(fd2, -EINVAL, "load_dummy_prog #2 (offloaded)");
+
+out:
+	close(fd1);
+	close(fd2);
+	SYS_NOFAIL("ip link delete eth42");
+	SYS_NOFAIL("ip netns del " LOCAL_NETNS);
+	if (tok)
+		close_netns(tok);
+}
Signature · 1 line
2.41.0
reviewer
#3 lore raw

Re: [PATCH bpf-next 2/2] selftests/bpf: Offloaded prog after non-offloaded should not cause BUG

Quoted 18 lines · Eduard Zingerman

Check what happens if non-offloaded dev bound BPF program is followed by offloaded dev bound program. Test case adapated from syzbot report [1].

[1] https://lore.kernel.org/bpf/[email protected]/

Signed-off-by Eduard Zingerman <[email protected]>
---
  .../bpf/prog_tests/xdp_dev_bound_only.c       | 58 +++++++++++++++++++
  1 file changed, 58 insertions(+)

create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c

tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c new file +0 -0
new file mode 100644
index 000000000000..5ee4c16d2e21

tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c line 8

Quoted 8 lines
tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c new file +8 -0
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <net/if.h>
+#include <test_progs.h>
+#include <network_helpers.h>
+
+#define LOCAL_NETNS "xdp_dev_bound_only_netns"
+
+int load_dummy_prog(char *name, __u32 ifindex, __u32 flags)

I added static.

Quoted 15 lines
tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c new file +15 -0
+{
+	struct bpf_insn insns[] = { BPF_MOV64_IMM(BPF_REG_0, 0), BPF_EXIT_INSN() };
+	LIBBPF_OPTS(bpf_prog_load_opts, opts);
+
+	opts.prog_flags = flags;
+	opts.prog_ifindex = ifindex;
+	return bpf_prog_load(BPF_PROG_TYPE_XDP, name, "GPL", insns, ARRAY_SIZE(insns), &opts);
+}
+
+/* A test case for bpf_offload_netdev->offload handling bug:
+ * - create a veth device (does not support offload);
+ * - create a device bound XDP program with BPF_F_XDP_DEV_BOUND_ONLY flag
+ *   (such programs are not offloaded);
+ * - create a device bound XDP program without flags (such programs are offloaded).
+ * This might lead to 'BUG: kernel NULL pointer dereference'.

tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c line 33

Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c new file +10 -0
+ */
+void test_xdp_dev_bound_only_offdev(void)
+{
+	struct nstoken *tok = NULL;
+	__u32 ifindex;
+	int fd1 = -1;
+	int fd2 = -1;
+
+	SYS(out, "ip netns add " LOCAL_NETNS);
+	tok = open_netns(LOCAL_NETNS);

Also added NULL check for tok.

Quoted 13 lines
tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c new file +13 -0
+	SYS(out, "ip link add eth42 type veth");
+	ifindex = if_nametoindex("eth42");
+	if (!ASSERT_NEQ(ifindex, 0, "if_nametoindex")) {
+		perror("if_nametoindex");
+		goto out;
+	}
+	fd1 = load_dummy_prog("dummy1", ifindex, BPF_F_XDP_DEV_BOUND_ONLY);
+	if (!ASSERT_GE(fd1, 0, "load_dummy_prog #1")) {
+		perror("load_dummy_prog #1");
+		goto out;
+	}
+	/* Program with ifindex is considered offloaded, however veth
+	 * does not support offload => error should be reported.

tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c line 56

Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c new file +10 -0
+	 */
+	fd2 = load_dummy_prog("dummy2", ifindex, 0);
+	ASSERT_EQ(fd2, -EINVAL, "load_dummy_prog #2 (offloaded)");
+
+out:
+	close(fd1);
+	close(fd2);
+	SYS_NOFAIL("ip link delete eth42");
+	SYS_NOFAIL("ip netns del " LOCAL_NETNS);
+	if (tok)

close_netns() can handle NULL, so removed this tok check.

Applied. Thanks for the fix and test!

Quoted 2 lines
tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c new file +2 -0
+		close_netns(tok);
+}
author maintainer
#4 lore raw

Re: [PATCH bpf-next 2/2] selftests/bpf: Offloaded prog after non-offloaded should not cause BUG

Quoted 1 line · Martin KaFai Lau

On 9/11/23 5:55 PM, Eduard Zingerman wrote:

[...]

Quoted 3 lines
Quoted 1 line

+int load_dummy_prog(char *name, __u32 ifindex, __u32 flags)

I added static.

[...]

Quoted 3 lines

tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c line 33

Quoted 1 line
tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c new file +1 -0
+	tok = open_netns(LOCAL_NETNS);

Also added NULL check for tok.

[...]

Quoted 5 lines

tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c line 56

Quoted 1 line
tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c new file +1 -0
+	if (tok)

close_netns() can handle NULL, so removed this tok check.

Applied. Thanks for the fix and test!

Sorry, I should have noticed these issues before sending. Thank you for fixing it up.

CI Applied
#5 lore raw
In reply to Eduard Zingerman
This series was applied to bpf/bpf.git (master) by Martin KaFai Lau <[email protected]>:

Hello:

This series was applied to bpf/bpf.git (master)
by Martin KaFai Lau <[email protected]>:

Quoted 9 lines · On Tue, 12 Sep 2023 03:55:36 +0300 you wrote:

For a device bound BPF program with flag BPF_F_XDP_DEV_BOUND_ONLY, in case if device does not support offload, __bpf_prog_dev_bound_init() creates a dummy bpf_offload_netdev struct with .offdev field set to NULL.

This dummy struct might be reused for programs without this flag bound to the same device. However, bpf_prog_offload_verifier_prep() that uses bpf_offload_netdev assumes that .offdev field cannot be NULL.

[...]

Here is the summary with links:
- [bpf-next,1/2] bpf: Avoid dummy bpf_offload_netdev in __bpf_prog_dev_bound_init

    https://git.kernel.org/bpf/bpf/c/1a49f4195d34
  - [bpf-next,2/2] selftests/bpf: Offloaded prog after non-offloaded should not cause BUG
    https://git.kernel.org/bpf/bpf/c/e4c31164737e

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html