[PATCH bpf-next 1/2] bpf: Fix mprog detachment for empty mprog entry

[PATCH bpf-next 1/2] bpf: Fix mprog detachment for empty mprog entry

[Daniel Borkmann]

syzbot reported an UBSAN array-index-out-of-bounds access in bpf_mprog_read()
upon bpf_mprog_detach(). While it did not have a reproducer, I was able to
manually reproduce through an empty mprog entry which just has miniq present.

The latter is important given otherwise we get an ENOENT error as tcx detaches
the whole mprog entry. The index 4294967295 was triggered via NULL dtuple.prog
which then attempts to detach from the back. bpf_mprog_fetch() in this case
did hit the idx == total and therefore tried to grab the entry at idx -1.

Fix it by adding an explicit bpf_mprog_total() check in bpf_mprog_detach() and
bail out early with ENOENT.

Fixes: 053c8e1f235d ("bpf: Add generic attach/detach/query API for multi-progs")
Reported-by: syzbot+0c06ba0f831fe07a8f27@syzkaller.appspotmail.com
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 kernel/bpf/mprog.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/bpf/mprog.c b/kernel/bpf/mprog.c
index f7816d2bc3e4..32d2c4829eb8 100644
--- a/kernel/bpf/mprog.c
+++ b/kernel/bpf/mprog.c
@@ -337,6 +337,8 @@ int bpf_mprog_detach(struct bpf_mprog_entry *entry,
 		return -EINVAL;
 	if (revision && revision != bpf_mprog_revision(entry))
 		return -ESTALE;
+	if (!bpf_mprog_total(entry))
+		return -ENOENT;
 	ret = bpf_mprog_tuple_relative(&rtuple, id_or_fd, flags,
 				       prog ? prog->type :
 				       BPF_PROG_TYPE_UNSPEC);
-- 
2.34.1

[PATCH bpf-next 2/2] selftests/bpf: Add test for detachment on empty mprog entry

[Daniel Borkmann]

Add a detachment test case with miniq present to assert that with and
without the miniq we get the same error.

  # ./test_progs -t tc_opts
  #244     tc_opts_after:OK
  #245     tc_opts_append:OK
  #246     tc_opts_basic:OK
  #247     tc_opts_before:OK
  #248     tc_opts_chain_classic:OK
  #249     tc_opts_delete_empty:OK
  #250     tc_opts_demixed:OK
  #251     tc_opts_detach:OK
  #252     tc_opts_detach_after:OK
  #253     tc_opts_detach_before:OK
  #254     tc_opts_dev_cleanup:OK
  #255     tc_opts_invalid:OK
  #256     tc_opts_mixed:OK
  #257     tc_opts_prepend:OK
  #258     tc_opts_replace:OK
  #259     tc_opts_revision:OK
  Summary: 16/0 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 .../selftests/bpf/prog_tests/tc_opts.c        | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/tc_opts.c b/tools/testing/selftests/bpf/prog_tests/tc_opts.c
index 7914100f9b46..39bd253e41aa 100644
--- a/tools/testing/selftests/bpf/prog_tests/tc_opts.c
+++ b/tools/testing/selftests/bpf/prog_tests/tc_opts.c
@@ -2237,3 +2237,34 @@ void serial_test_tc_opts_detach_after(void)
 	test_tc_opts_detach_after_target(BPF_TCX_INGRESS);
 	test_tc_opts_detach_after_target(BPF_TCX_EGRESS);
 }
+
+static void test_tc_opts_delete_empty(int target, bool chain_tc_old)
+{
+	LIBBPF_OPTS(bpf_tc_hook, tc_hook, .ifindex = loopback);
+	LIBBPF_OPTS(bpf_prog_detach_opts, optd);
+	int err;
+
+	assert_mprog_count(target, 0);
+	if (chain_tc_old) {
+		tc_hook.attach_point = target == BPF_TCX_INGRESS ?
+				       BPF_TC_INGRESS : BPF_TC_EGRESS;
+		err = bpf_tc_hook_create(&tc_hook);
+		ASSERT_OK(err, "bpf_tc_hook_create");
+		__assert_mprog_count(target, 0, true, loopback);
+	}
+	err = bpf_prog_detach_opts(0, loopback, target, &optd);
+	ASSERT_EQ(err, -ENOENT, "prog_detach");
+	if (chain_tc_old) {
+		tc_hook.attach_point = BPF_TC_INGRESS | BPF_TC_EGRESS;
+		bpf_tc_hook_destroy(&tc_hook);
+	}
+	assert_mprog_count(target, 0);
+}
+
+void serial_test_tc_opts_delete_empty(void)
+{
+	test_tc_opts_delete_empty(BPF_TCX_INGRESS, false);
+	test_tc_opts_delete_empty(BPF_TCX_EGRESS, false);
+	test_tc_opts_delete_empty(BPF_TCX_INGRESS, true);
+	test_tc_opts_delete_empty(BPF_TCX_EGRESS, true);
+}
-- 
2.34.1

Re: [PATCH bpf-next 1/2] bpf: Fix mprog detachment for empty mprog entry

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:

On Fri,  4 Aug 2023 15:11:11 +0200 you wrote:
> syzbot reported an UBSAN array-index-out-of-bounds access in bpf_mprog_read()
> upon bpf_mprog_detach(). While it did not have a reproducer, I was able to
> manually reproduce through an empty mprog entry which just has miniq present.
> 
> The latter is important given otherwise we get an ENOENT error as tcx detaches
> the whole mprog entry. The index 4294967295 was triggered via NULL dtuple.prog
> which then attempts to detach from the back. bpf_mprog_fetch() in this case
> did hit the idx == total and therefore tried to grab the entry at idx -1.
> 
> [...]

Here is the summary with links:
  - [bpf-next,1/2] bpf: Fix mprog detachment for empty mprog entry
    https://git.kernel.org/bpf/bpf-next/c/d210f9735e13
  - [bpf-next,2/2] selftests/bpf: Add test for detachment on empty mprog entry
    https://git.kernel.org/bpf/bpf-next/c/21ce6abe178a

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