Re: [PATCH AUTOSEL 6.18-5.10] bpf: NUL-terminate replaced sysctl value

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The original sysctl buffer is allocated with `kvzalloc()` but erroneously freed with `kfree()` during buffer replacement, leading to potential kernel panics.
- [High] The sysctl buffer replacement logic is completely broken because it relies on `ret == 1`, but the BPF execution engine returns 0 on success. Furthermore, a non-zero return aborts the sysctl write.
--

commit 499368353ad755feaab0984797529c1470873194
Author: Dawei Feng <dawei.feng@seu.edu.cn>

bpf: NUL-terminate replaced sysctl value

This commit ensures that BPF-replaced sysctl write buffers are
NUL-terminated before downstream proc handlers parse them. It appends
a '\0' to the buffer in bpf_sysctl_set_new_value() to prevent out-of-bounds
reads when parsing the modified sysctl values.

Link: https://lore.kernel.org/r/20260603105317.944304-2-dawei.feng@seu.edu.cn

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 248f517d66d04..aed2f087cfc4e 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -2346,6 +2346,7 @@ BPF_CALL_3(bpf_sysctl_set_new_value, struct bpf_sysctl_kern *, ctx,
>  		return -E2BIG;
>  
>  	memcpy(ctx->new_val, buf, buf_len);
> +	((char *)ctx->new_val)[buf_len] = '\0';

[Severity: High]
This isn't a bug introduced by this patch, but there is a pre-existing issue
in the sysctl replacement path that bpf_sysctl_set_new_value() relies on.

Is it possible that replacing the sysctl buffer leads to a kernel panic
due to mismatched allocation and free functions?

In fs/proc/proc_sysctl.c:proc_sys_call_handler(), the user buffer is
allocated using kvzalloc():

    kbuf = kvzalloc(count + 1, GFP_KERNEL);

Under memory pressure for allocations larger than PAGE_SIZE, this can fall
back to vmalloc().

However, if a BPF program successfully updates the sysctl value via
bpf_sysctl_set_new_value(), __cgroup_bpf_run_filter_sysctl() in 
kernel/bpf/cgroup.c frees the original user buffer using kfree():

    kfree(*buf);

Could freeing a vmalloc pointer with kfree() cause memory corruption or a
kernel panic in the slab allocator?


[Severity: High]
This is also a pre-existing issue, but the sysctl buffer replacement logic
appears to be functionally broken.

Can bpf_sysctl_set_new_value() ever successfully modify a sysctl value?

In kernel/bpf/cgroup.c:__cgroup_bpf_run_filter_sysctl(), the replacement
branch is only taken if the return value is exactly 1:

    if (ret == 1 && ctx.new_updated) {

But the BPF execution engine in bpf_prog_run_array_cg() normally returns 0
on success, making the replacement branch unreachable:

    if (!func_ret && !IS_ERR_VALUE((long)run_ctx.retval))
        run_ctx.retval = -EPERM;

If a BPF program uses bpf_set_retval(1) to force a return value of 1,
__cgroup_bpf_run_filter_sysctl() will bubble up 1 as an error to
fs/proc/proc_sysctl.c:proc_sys_call_handler():

    if (error)
        goto out_free_buf;

Does this abort the sysctl write and skip calling the downstream
table->proc_handler, entirely breaking the replacement functionality?

>  	ctx->new_len = buf_len;
>  	ctx->new_updated = 1;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-556-sashal@kernel.org?part=1