Re: [PATCH 1/2] bpf: cgroup: fix sysctl new value replacement

[bot+bpf-ci]

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 876f6a81a9b6..8715a014c21d 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c

[ ... ]

> @@ -1878,7 +1878,7 @@ int __cgroup_bpf_run_filter_sysctl(struct ctl_table_header *head,
>   * to provided by user buffer, so that next BPF program in the chain
>   * can treat it as a replacement.
>   *
> - * This function will return %-EPERM if an attached program is found and
> - * returned value != 1 during execution. In all other cases 0 is returned.
> + * This function will return %-EPERM if an attached program is found and
> + * returned value != 1 during execution. In all other cases 0 is returned.
>   */

Should the function documentation be updated to reflect the actual return
value convention?

After commit 4e63acdff864, bpf_prog_run_array_cg() returns 0 on success,
not 1. The comment still states "returned value != 1 during execution" but
the code now checks for '!ret' (i.e., ret == 0). The documentation should
say "returned value != 0" to match the actual behavior.

> @@ -1935,7 +1935,7 @@ int __cgroup_bpf_run_filter_sysctl(struct ctl_table_header *head,
>
>  	kfree(ctx.cur_val);
>
> -	if (ret == 1 && ctx.new_updated) {
> +	if (!ret && ctx.new_updated) {
>  		kfree(*buf);
>  		*buf = ctx.new_val;
>  		*pcount = ctx.new_len;

Can this cause memory corruption?

The caller proc_sys_call_handler() allocates the buffer with kvzalloc():

fs/proc/proc_sysctl.c:proc_sys_call_handler() {
    ...
    kbuf = kvzalloc(count + 1, GFP_KERNEL);
    ...
}

kvzalloc() may fall back to vmalloc() for large allocations, but this
code uses kfree() instead of kvfree() to free *buf. Using kfree() on
vmalloc memory corrupts memory.

This is fixed in a later commit 155dfbd42ab8 ("bpf: cgroup: Use kvfree
instead of kfree in __cgroup_bpf_run_filter_sysctl") which includes a
KASAN crash report demonstrating the bug.

Should this patch use kvfree() instead of kfree()?


Also, Martin KaFai Lau raised a concern about the null terminator handling
during the May 2024 review of a similar fix.

In the discussion at:
https://lore.kernel.org/bpf/ca8136e0-5d2a-402b-ad03-cc8a218affd4@xxxxxxxxx/

Martin requested that the patch adjust *pcount to exclude the null
terminator if present. The concern is that proc_sys_call_handler may reject
the value with EINVAL if the null terminator is included in the count.

The suggested fix from that discussion was:

    if (!(*buf)[ctx.new_len])
        *pcount = ctx.new_len - 1;
    else
        *pcount = ctx.new_len;

Should this patch include the null terminator handling improvement?


> commit f089fd0540601290c72ecfd2ff3f0c817952970a
> Author: Dawei Feng <dawei.feng@xxxxxxxxxx>
>
>     bpf: cgroup: fix sysctl new value replacement
>
>     Commit 4e63acdff864 ("bpf: Introduce bpf_sysctl_{get,set}_new_value
>     helpers") changed the success return value to 0, but failed to update the
>     corresponding check in __cgroup_bpf_run_filter_sysctl(). Since
>     bpf_prog_run_array_cg() now returns 0 on success, the legacy ret == 1
>     condition is never satisfied. As a result, the modified value is ignored,
>     and bpf_sysctl_set_new_value() fails to replace the write buffer.
>
>     Fix this by checking for a return value of 0 instead, so cgroup/sysctl
>     programs can correctly replace the pending sysctl buffer.
>
>     This bug was discovered during a manual code review. Tested via a
>     cgroup/sysctl BPF reproducer overriding writes to a target sysctl.
>     Pre-fix, bpf_sysctl_set_new_value("foo") was silently ignored: the write
>     returned 8192 and the value remained "600". Post-fix, the BPF replacement
>     buffer properly propagates: the write returns 3 and the value updates to
>     "foo".
>
>     Fixes: 4e63acdff864 ("bpf: Introduce bpf_sysctl_{get,set}_new_value helpers")

Is the Fixes: tag pointing to the correct commit?

Commit 4e63acdff864 (2019) originally used 'ret == 1' correctly, as
BPF_PROG_RUN_ARRAY_CG returned 1 on success at that time.

However, commit f10d05966196 ("bpf: Make BPF_PROG_RUN_ARRAY return -err
instead of allow boolean") from 2022 changed BPF_PROG_RUN_ARRAY_CG to
return 0 on success instead of 1. That commit updated the final return
statement from 'return ret == 1 ? 0 : -EPERM;' to 'return ret;', but
forgot to update the 'if (ret == 1 && ctx.new_updated)' check.

Should the Fixes: tag be:

  Fixes: f10d05966196 ("bpf: Make BPF_PROG_RUN_ARRAY return -err instead of allow boolean")


---
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/26451027650