Re: [PATCH bpf-next v2 4/4] selftests/bpf: Exercise kptr map update lifetime

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The selftests incorrectly codify a resource leak and cross-key reference vulnerability for BPF kptrs in map updates.
--

commit b8d5d36d1cdd1bc97594f89c513eb183613ab05f
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

selftests/bpf: Exercise kptr map update lifetime

This commit adds selftests for BPF-side map updates that touch values
containing referenced kptrs in array and hash maps. It verifies that the
refcount behaves as expected when updating existing map elements from BPF.

> diff --git a/tools/testing/selftests/bpf/progs/map_kptr.c b/tools/testing/selftests/bpf/progs/map_kptr.c
> index e708ffbe1f610..3fbefc568e0a6 100644
> --- a/tools/testing/selftests/bpf/progs/map_kptr.c
> +++ b/tools/testing/selftests/bpf/progs/map_kptr.c

[ ... ]

> @@ -500,11 +499,95 @@ int count_ref(void *ctx)

[ ... ]

> +SEC("syscall")
> +int test_array_map_update_kptr(void *ctx)
> +{
> +	struct map_value init = {}, *v;
> +	int key = 0, ret;
> +
> +	v = bpf_map_lookup_elem(&array_map, &key);
> +	if (!v)
> +		return 1;
> +	ret = stash_ref_ptr(v);
> +	if (ret)
> +		return ret;
> +	ret = check_refs(3);
> +	if (ret)
> +		return ret;
> +	ret = bpf_map_update_elem(&array_map, &key, &init, BPF_EXIST);
> +	if (ret)
> +		return 4;
> +	return check_refs(3);

[Severity: Critical]
This isn't a bug introduced by this patch, but are these tests codifying a
kernel resource leak and cross-key reference vulnerability?

In a prior commit, bpf_obj_free_fields() was replaced with
bpf_obj_cancel_fields() in kernel/bpf/arraymap.c:array_map_update_elem()
and check_and_cancel_fields() in kernel/bpf/hashtab.c:free_htab_elem().

Because those cancel functions do not release or zero kptrs, overwriting an
array element or freeing a preallocated hash map element to the freelist
leaves the old kptr intact.

For preallocated hash maps, when a new element is popped from the freelist
and updated via copy_map_value() (which skips special BTF fields), the new
element silently inherits the stale kptr from the deleted key.

By asserting check_refs(3) here and in DEFINE_HASH_UPDATE_KPTR_TEST below
after updating the map with an empty init value, do these tests accidentally
codify this broken kernel behavior where the refcount incorrectly remains
elevated?

> +}
> +
> +#define DEFINE_HASH_UPDATE_KPTR_TEST(name, map)			\
> +SEC("syscall")							\
> +int name(void *ctx)						\
> +{								\
> +	struct map_value init = {}, *v;				\
> +	int key = 0, ret;					\
> +								\
> +	ret = bpf_map_update_elem(&map, &key, &init, BPF_NOEXIST); \
> +	if (ret)						\
> +		return 1;					\
> +	v = bpf_map_lookup_elem(&map, &key);			\
> +	if (!v)							\
> +		return 2;					\
> +	ret = stash_ref_ptr(v);					\
> +	if (ret)						\
> +		return ret;					\
> +	ret = check_refs(3);					\
> +	if (ret)						\
> +		return ret;					\
> +	ret = bpf_map_update_elem(&map, &key, &init, BPF_EXIST); \
> +	if (ret)						\
> +		return 4;					\
> +	return check_refs(3);					\
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609093719.2858096-1-memxor@gmail.com?part=4