[PATCH bpf v1 1/2] bpf: Disallow tail call to programs that use cgroup storage

[PATCH bpf v1 1/2] bpf: Disallow tail call to programs that use cgroup storage

[Amery Hung]

Mitigate a possible NULL pointer dereference in bpf_get_local_storage()
by disallowing tail call to programs that use cgroup storage. Cgroup
storage is allocated lazily when attaching a cgroup bpf program. With
tail call, it is possible for a callee BPF program to see a NULL
storage pointer if the caller prorgam does not use cgroup storage.

Reported-by: Yinhao Hu <dddddd@hust.edu.cn>
Reported-by: Kaiyan Mei <M202472210@hust.edu.cn>
Reported-by: Dongliang Mu <dzm91@hust.edu.cn>
Closes: https://lore.kernel.org/bpf/c9ac63d7-73be-49c5-a4ac-eb07f7521adb@hust.edu.cn/
Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 kernel/bpf/arraymap.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 1eeb31c5b317..9c3f86ef9d16 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -884,8 +884,9 @@ int bpf_fd_array_map_update_elem(struct bpf_map *map, struct file *map_file,
 				 void *key, void *value, u64 map_flags)
 {
 	struct bpf_array *array = container_of(map, struct bpf_array, map);
+	u32 i, index = *(u32 *)key, ufd;
 	void *new_ptr, *old_ptr;
-	u32 index = *(u32 *)key, ufd;
+	struct bpf_prog *prog;
 
 	if (map_flags != BPF_ANY)
 		return -EINVAL;
@@ -898,6 +899,14 @@ int bpf_fd_array_map_update_elem(struct bpf_map *map, struct file *map_file,
 	if (IS_ERR(new_ptr))
 		return PTR_ERR(new_ptr);
 
+	if (map->map_type == BPF_MAP_TYPE_PROG_ARRAY) {
+		prog = (struct bpf_prog *)new_ptr;
+
+		for_each_cgroup_storage_type(i)
+			if (prog->aux->cgroup_storage[i])
+				return -EINVAL;
+	}
+
 	if (map->ops->map_poke_run) {
 		mutex_lock(&array->aux->poke_mutex);
 		old_ptr = xchg(array->ptrs + index, new_ptr);
-- 
2.47.3

[PATCH bpf v1 2/2] selftests/bpf: Test using cgroup storage in a tail call callee program

[Amery Hung]

Check that a BPF program that uses cgroup storage cannot be added to
a program array map.

Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 .../selftests/bpf/prog_tests/tailcalls.c      | 25 ++++++++++++
 .../bpf/progs/tailcall_cgrp_storage.c         | 39 +++++++++++++++++++
 2 files changed, 64 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/tailcall_cgrp_storage.c

diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
index 0ab36503c3b2..e4a5287f10b1 100644
--- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
+++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
@@ -8,6 +8,7 @@
 #include "tailcall_freplace.skel.h"
 #include "tc_bpf2bpf.skel.h"
 #include "tailcall_fail.skel.h"
+#include "tailcall_cgrp_storage.skel.h"
 
 /* test_tailcall_1 checks basic functionality by patching multiple locations
  * in a single program for a single tail call slot with nop->jmp, jmp->nop
@@ -1648,6 +1649,28 @@ static void test_tailcall_bpf2bpf_freplace(void)
 	tc_bpf2bpf__destroy(tc_skel);
 }
 
+/*
+ * test_tail_call_cgrp_storage makes sure that callee programs cannot
+ * use cgroup storage
+ */
+static void test_tailcall_cgrp_storage(void)
+{
+	int err, prog_fd, prog_array_fd, key = 0;
+	struct tailcall_cgrp_storage *skel;
+
+	skel = tailcall_cgrp_storage__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "tailcall_cgrp_storage__open_and_load"))
+		return;
+
+	prog_fd = bpf_program__fd(skel->progs.callee_prog);
+	prog_array_fd = bpf_map__fd(skel->maps.prog_array);
+
+	err = bpf_map_update_elem(prog_array_fd, &key, &prog_fd, BPF_ANY);
+	ASSERT_ERR(err, "bpf_map_update_elem");
+
+	tailcall_cgrp_storage__destroy(skel);
+}
+
 static void test_tailcall_failure()
 {
 	RUN_TESTS(tailcall_fail);
@@ -1705,6 +1728,8 @@ void test_tailcalls(void)
 		test_tailcall_freplace();
 	if (test__start_subtest("tailcall_bpf2bpf_freplace"))
 		test_tailcall_bpf2bpf_freplace();
+	if (test__start_subtest("tailcall_cgrp_storage"))
+		test_tailcall_cgrp_storage();
 	if (test__start_subtest("tailcall_failure"))
 		test_tailcall_failure();
 }
diff --git a/tools/testing/selftests/bpf/progs/tailcall_cgrp_storage.c b/tools/testing/selftests/bpf/progs/tailcall_cgrp_storage.c
new file mode 100644
index 000000000000..e4f277d2c4fe
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tailcall_cgrp_storage.c
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE);
+	__type(key, struct bpf_cgroup_storage_key);
+	__type(value, __u64);
+} storage_map SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
+	__uint(max_entries, 1);
+	__uint(key_size, sizeof(__u32));
+	__uint(value_size, sizeof(__u32));
+} prog_array SEC(".maps");
+
+SEC("cgroup_skb/egress")
+int caller_prog(struct __sk_buff *skb)
+{
+	bpf_tail_call(skb, &prog_array, 0);
+
+	return 1;
+}
+
+SEC("cgroup_skb/egress")
+int callee_prog(struct __sk_buff *skb)
+{
+	__u64 *storage;
+
+	storage = bpf_get_local_storage(&storage_map, 0);
+	if (storage)
+		*storage = 1;
+
+	return 1;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.47.3

Re: [PATCH bpf v1 2/2] selftests/bpf: Test using cgroup storage in a tail call callee program

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1800 bytes --]

> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> index 0ab36503c..e4a5287f1 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c

[ ... ]

> @@ -1648,6 +1649,28 @@ static void test_tailcall_bpf2bpf_freplace(void)
>  	tc_bpf2bpf__destroy(tc_skel);
>  }
>
> +/*
> + * test_tail_call_cgrp_storage makes sure that callee programs cannot
> + * use cgroup storage
> + */
> +static void test_tailcall_cgrp_storage(void)
> +{
> +	int err, prog_fd, prog_array_fd, key = 0;
> +	struct tailcall_cgrp_storage *skel;
> +
> +	skel = tailcall_cgrp_storage__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "tailcall_cgrp_storage__open_and_load"))
> +		return;
> +
> +	prog_fd = bpf_program__fd(skel->progs.callee_prog);
> +	prog_array_fd = bpf_map__fd(skel->maps.prog_array);
                     ^^^^

Should the return values of bpf_program__fd() and bpf_map__fd() be
checked before use? Other tests in this file validate these return
values (see test_tailcall_1 and similar tests which check for < 0).

Without checking, if either function returns a negative error value,
bpf_map_update_elem() could fail for the wrong reason (invalid FD),
and ASSERT_ERR would still pass, potentially masking issues with the
actual kernel restriction being tested.

> +
> +	err = bpf_map_update_elem(prog_array_fd, &key, &prog_fd, BPF_ANY);
> +	ASSERT_ERR(err, "bpf_map_update_elem");
> +
> +	tailcall_cgrp_storage__destroy(skel);
> +}

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19842130628

Re: [PATCH bpf v1 2/2] selftests/bpf: Test using cgroup storage in a tail call callee program

[Alexei Starovoitov]

On Mon, Dec 1, 2025 at 4:35 PM <bot+bpf-ci@kernel.org> wrote:
>
> > diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> > index 0ab36503c..e4a5287f1 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>
> [ ... ]
>
> > @@ -1648,6 +1649,28 @@ static void test_tailcall_bpf2bpf_freplace(void)
> >       tc_bpf2bpf__destroy(tc_skel);
> >  }
> >
> > +/*
> > + * test_tail_call_cgrp_storage makes sure that callee programs cannot
> > + * use cgroup storage
> > + */
> > +static void test_tailcall_cgrp_storage(void)
> > +{
> > +     int err, prog_fd, prog_array_fd, key = 0;
> > +     struct tailcall_cgrp_storage *skel;
> > +
> > +     skel = tailcall_cgrp_storage__open_and_load();
> > +     if (!ASSERT_OK_PTR(skel, "tailcall_cgrp_storage__open_and_load"))
> > +             return;
> > +
> > +     prog_fd = bpf_program__fd(skel->progs.callee_prog);
> > +     prog_array_fd = bpf_map__fd(skel->maps.prog_array);
>                      ^^^^
>
> Should the return values of bpf_program__fd() and bpf_map__fd() be
> checked before use? Other tests in this file validate these return
> values (see test_tailcall_1 and similar tests which check for < 0).
>
> Without checking, if either function returns a negative error value,
> bpf_map_update_elem() could fail for the wrong reason (invalid FD),
> and ASSERT_ERR would still pass, potentially masking issues with the
> actual kernel restriction being tested.

Chris,

note... AI is wrong here.
We don't check FDs returned by these getters because skeleton open_and_load()
succeeded.

Re: [PATCH bpf v1 2/2] selftests/bpf: Test using cgroup storage in a tail call callee program

[Chris Mason]

On 12/1/25 8:09 PM, Alexei Starovoitov wrote:
> On Mon, Dec 1, 2025 at 4:35 PM <bot+bpf-ci@kernel.org> wrote:
>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>>> index 0ab36503c..e4a5287f1 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>>
>> [ ... ]
>>
>>> @@ -1648,6 +1649,28 @@ static void test_tailcall_bpf2bpf_freplace(void)
>>>       tc_bpf2bpf__destroy(tc_skel);
>>>  }
>>>
>>> +/*
>>> + * test_tail_call_cgrp_storage makes sure that callee programs cannot
>>> + * use cgroup storage
>>> + */
>>> +static void test_tailcall_cgrp_storage(void)
>>> +{
>>> +     int err, prog_fd, prog_array_fd, key = 0;
>>> +     struct tailcall_cgrp_storage *skel;
>>> +
>>> +     skel = tailcall_cgrp_storage__open_and_load();
>>> +     if (!ASSERT_OK_PTR(skel, "tailcall_cgrp_storage__open_and_load"))
>>> +             return;
>>> +
>>> +     prog_fd = bpf_program__fd(skel->progs.callee_prog);
>>> +     prog_array_fd = bpf_map__fd(skel->maps.prog_array);
>>                      ^^^^
>>
>> Should the return values of bpf_program__fd() and bpf_map__fd() be
>> checked before use? Other tests in this file validate these return
>> values (see test_tailcall_1 and similar tests which check for < 0).
>>
>> Without checking, if either function returns a negative error value,
>> bpf_map_update_elem() could fail for the wrong reason (invalid FD),
>> and ASSERT_ERR would still pass, potentially masking issues with the
>> actual kernel restriction being tested.
> 
> Chris,
> 
> note... AI is wrong here.
> We don't check FDs returned by these getters because skeleton open_and_load()
> succeeded.

Thanks Alexei, I'll fix this up.

-chris

Re: [PATCH bpf v1 2/2] selftests/bpf: Test using cgroup storage in a tail call callee program

[Eduard Zingerman]

On Mon, 2025-12-01 at 16:18 -0800, Amery Hung wrote:
> Check that a BPF program that uses cgroup storage cannot be added to
> a program array map.
> 
> Signed-off-by: Amery Hung <ameryhung@gmail.com>
> ---

Hi Amery,

Mabye I'm making some silly systematic mistake, but when I pick this
test w/o picking patch #1 the test still passes.
I'm at ff34657aa72a ("bpf: optimize bpf_map_update_elem() for map-in-map types").
Inserting some printk shows that -EINVAL is propagated for map update
from kernel/bpf/core.c:__bpf_prog_map_compatible() line 2406
(`ret = map->owner->storage_cookie[i] == cookie || !cookie;`).

[...]

Re: [PATCH bpf v1 2/2] selftests/bpf: Test using cgroup storage in a tail call callee program

[Amery Hung]

On Mon, Dec 1, 2025 at 5:08 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Mon, 2025-12-01 at 16:18 -0800, Amery Hung wrote:
> > Check that a BPF program that uses cgroup storage cannot be added to
> > a program array map.
> >
> > Signed-off-by: Amery Hung <ameryhung@gmail.com>
> > ---
>
> Hi Amery,
>
> Mabye I'm making some silly systematic mistake, but when I pick this
> test w/o picking patch #1 the test still passes.
> I'm at ff34657aa72a ("bpf: optimize bpf_map_update_elem() for map-in-map types").
> Inserting some printk shows that -EINVAL is propagated for map update
> from kernel/bpf/core.c:__bpf_prog_map_compatible() line 2406
> (`ret = map->owner->storage_cookie[i] == cookie || !cookie;`).

Thanks for the double check! I simplified the selftest too much and
introduced the false negative.

There should be another program using the prog array and the same
cgroup storage in the first place so that the check here passes.

I will fix and resubmit.

>
> [...]

Re: [PATCH bpf v1 1/2] bpf: Disallow tail call to programs that use cgroup storage

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1824 bytes --]

> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 80b1765a3..fbfc45ed9 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -892,8 +892,9 @@ int bpf_fd_array_map_update_elem(struct bpf_map *map, struct file *map_file,
>  				 void *key, void *value, u64 map_flags)
>  {
>  	struct bpf_array *array = container_of(map, struct bpf_array, map);
> +	u32 i, index = *(u32 *)key, ufd;
>  	void *new_ptr, *old_ptr;
> -	u32 index = *(u32 *)key, ufd;
> +	struct bpf_prog *prog;
>
>  	if (map_flags != BPF_ANY)
>  		return -EINVAL;
> @@ -906,6 +907,14 @@ int bpf_fd_array_map_update_elem(struct bpf_map *map, struct file *map_file,
>  	if (IS_ERR(new_ptr))
>  		return PTR_ERR(new_ptr);
>
> +	if (map->map_type == BPF_MAP_TYPE_PROG_ARRAY) {
> +		prog = (struct bpf_prog *)new_ptr;
> +
> +		for_each_cgroup_storage_type(i)
> +			if (prog->aux->cgroup_storage[i])
> +				return -EINVAL;
                               ^^^^^^

Does this leak prog's refcount? In bpf_fd_array_map_update_elem(), the
call to map->ops->map_fd_get_ptr() acquires a reference by calling
bpf_prog_get() in prog_fd_array_get_ptr(), which increments both the
prog refcount and prog->aux->prog_array_member_cnt. When the new
validation check returns -EINVAL, the error path never calls
map->ops->map_fd_put_ptr() to release these references.

The normal path releases old_ptr at the end of the function, but the
early return here leaks new_ptr.

> +	}
> +
>  	if (map->ops->map_poke_run) {
>  		mutex_lock(&array->aux->poke_mutex);
>  		old_ptr = xchg(array->ptrs + index, new_ptr);


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19842130628