Re: [RFC PATCH bpf-next 08/12] bpf: Add a few bpf_cgroup_array_* helper functions

[sashiko-bot@kernel.org]

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

New issues:
- [High] bpf_cgroup_array_copy_to_user() incorrectly returns -ENOSPC when the requested count is zero.

Pre-existing issues:
- [Critical] purge_effective_progs() calculates linear indices that do not account for BPF_F_PREORDER physical reordering, leading to Use-After-Free.
--

commit 23e774d89cf8101cef3b2e8789a63f7fb930be02
Author: Martin KaFai Lau <martin.lau@kernel.org>

bpf: Add a few bpf_cgroup_array_* helper functions

This patch adds bpf_cgroup_array_*() helper functions to prepare for
supporting struct_ops maps in the cgroup bpf array. These helpers currently
skip the dummy_bpf_prog, which is the same as the existing behavior.
The callers are changed to use these new helpers as a no-op change.

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index b5769f5401e64..a033aa479ab6e 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c

[ ... ]

> +static int bpf_cgroup_array_copy_to_user(struct bpf_prog_array *array,
> +					 __u32 __user *prog_ids, int cnt,
> +					 enum cgroup_bpf_attach_type atype)
> +{
> +	struct bpf_prog_array_item *item;
> +	int i = 0;
> +	u32 id;
> +
> +	for (item = array->items; item->prog && i < cnt; item++) {
> +		if (item->prog == bpf_cgroup_array_dummy(atype))
> +			continue;
> +		id = item->prog->aux->id;
> +		if (copy_to_user(prog_ids + i, &id, sizeof(id)))
> +			return -EFAULT;
> +		i++;
> +	}
> +	return item->prog ? -ENOSPC : 0;
> +}

[Severity: High]
If cnt is 0 (which happens if the attach type only has dummy programs,
causing bpf_cgroup_array_length() to return 0), the loop condition i < cnt
will fail immediately. At this point, item->prog still points to a dummy
program (which is not NULL).

Does this incorrectly return -ENOSPC instead of 0?

If this occurs during __cgroup_bpf_query(), it looks like the entire
syscall could fail with -ENOSPC even when the user provided a large enough
buffer.

[ ... ]

> @@ -1124,7 +1185,7 @@ static void purge_effective_progs(struct cgroup *cgrp, struct bpf_prog *prog,
>  				lockdep_is_held(&cgroup_mutex));
>  
>  		/* Remove the program from the array */
> -		WARN_ONCE(bpf_prog_array_delete_safe_at(progs, pos),
> +		WARN_ONCE(bpf_cgroup_array_delete_safe_at(progs, pos, atype),

[Severity: Critical]
This is a pre-existing issue, but does pos correctly reflect the program's
physical index in the effective array?

When building the effective program array, compute_effective_progs() places
programs with BPF_F_PREORDER at the beginning of the array. However,
purge_effective_progs() calculates pos by sequentially iterating through
the hierarchy's hlists, which does not account for this reordering.

If update_effective_progs() fails (for example, due to OOM) during detach,
purge_effective_progs() is called as a fallback.

With the incorrect pos, could bpf_cgroup_array_delete_safe_at() overwrite
the wrong program with a dummy, leaving the intended program in the
effective array?

Since __cgroup_bpf_detach() subsequently calls bpf_prog_put() on the
detached program, the effective array would be left holding a dangling
pointer to a freed bpf_prog, which could result in a use-after-free when
the cgroup hook runs.

>  			  "Failed to purge a prog from array at index %d", pos);
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519215841.2984970-1-martin.lau@linux.dev?part=8