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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Calling kfree() on the sysctl buffer can cause a kernel panic because the buffer is originally allocated with kvzalloc() and may be backed by vmalloc().
- [High] The BPF-provided sysctl replacement buffer is not explicitly NUL-terminated, which violates the VFS sysctl contract and risks out-of-bounds reads in downstream proc handlers.
--

commit 87d6f30bc7d66708176be678dd7992a7d205d7b2
Author: Dawei Feng <dawei.feng@seu.edu.cn>

bpf: cgroup: fix sysctl new value replacement

This commit changes the success check from ret == 1 to !ret in
__cgroup_bpf_run_filter_sysctl(). This allows cgroup/sysctl BPF programs to
correctly replace the pending sysctl buffer after bpf_prog_run_array_cg()
returns 0 on success.

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 876f6a81a9b61..8715a014c21d9 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -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);

[Severity: Critical]
This is a pre-existing issue, but by activating this code path, could this
kfree() trigger a kernel panic?

Looking at proc_sys_call_handler(), the original sysctl write buffer is
allocated using kvzalloc(), which means it might be backed by vmalloc()
for larger allocations. If kfree() is called on a vmalloc address, it would
cause an invalid virtual-to-physical translation.

I noticed this is fixed in a later commit (bpf: cgroup: Use kvfree instead
of kfree in __cgroup_bpf_run_filter_sysctl), but should this patch be
self-contained to avoid exposing this bug?

>  		*buf = ctx.new_val;

[Severity: High]
This isn't a bug introduced by this patch, but now that this buffer
replacement path is reachable, does this violate the VFS sysctl contract
for null-termination?

The VFS sysctl contract guarantees that the write buffer passed to proc
handlers is a null-terminated string. Since ctx.new_val is allocated with
kmalloc_track_caller() which does not zero-initialize memory, and the
helper copies the buffer without explicitly appending a null byte, the
memory past the new length could contain uninitialized garbage.

Could this lead to out-of-bounds reads when downstream handlers pass this
buffer to string-parsing functions like kstrtoint()?

>  		*pcount = ctx.new_len;
>  	} else {
>  		kfree(ctx.new_val);
>  	}
>  
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526131035.1312864-1-dawei.feng@seu.edu.cn?part=1