Re: [PATCH v3 1/3] bpf: NUL-terminate replaced sysctl value

[Yonghong Song <yonghong.song@linux.dev>]

On 6/3/26 4:23 PM, Alexei Starovoitov wrote:
> On Wed, Jun 3, 2026 at 7:37 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>>
>> On 6/3/26 3:53 AM, Dawei Feng wrote:
>>> When writing to sysctls, proc_sys_call_handler() guarantees that the
>>> buffer passed to proc handlers is NUL-terminated. If
>>> bpf_sysctl_set_new_value() replaces the pending sysctl value, it can
>>> hand a replacement buffer directly to proc handlers. However, the
>>> helper currently copies only buf_len bytes into that buffer without
>>> appending a NUL terminator, leaving downstream parsers vulnerable to
>>> out-of-bounds access.
>>>
>>> Fix this by appending a '\0' after the replaced value to restore the
>>> expected sysctl semantics. Since the helper already rejects buf_len
>>> greater than PAGE_SIZE - 1, there is always room for the extra byte.
>>>
>>> Reproduced in a QEMU x86_64 guest booted with KASAN while exercising
>>> the sysctl replacement path with a cgroup/sysctl BPF program. The
>>> reproducer targets `/proc/sys/net/core/flow_limit_cpu_bitmap`, fills
>>> the original user write buffer with non-zero bytes, and overrides the
>>> sysctl value so the replacement buffer lacks a terminating NUL. Under
>>> that setup, the pre-fix kernel reported:
>>>
>>>     BUG: KASAN: slab-out-of-bounds in strnchrnul+0x72/0x90
>>>     Read of size 1 at addr ffff88800de57000 by task repro_patch3/66
>>>     CPU: 0 UID: 0 PID: 66 Comm: repro_patch3 Not tainted 7.1.0-rc3-00269-g8370ca1f87cc #6 PREEMPT(lazy)
>>>     Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
>>>     Call Trace:
>>>      <TASK>
>>>      dump_stack_lvl+0x68/0xa0
>>>      print_report+0xcb/0x5e0
>>>      ? __virt_addr_valid+0x21d/0x3f0
>>>      ? strnchrnul+0x72/0x90
>>>      ? strnchrnul+0x72/0x90
>>>      kasan_report+0xca/0x100
>>>      ? strnchrnul+0x72/0x90
>>>      strnchrnul+0x72/0x90
>>>      bitmap_parse+0x37/0x2e0
>>>      flow_limit_cpu_sysctl+0xc6/0x840
>>>      ? __pfx_flow_limit_cpu_sysctl+0x10/0x10
>>>      ? __kvmalloc_node_noprof+0x5ba/0x870
>>>      proc_sys_call_handler+0x31d/0x480
>>>      ? __pfx_proc_sys_call_handler+0x10/0x10
>>>      ? selinux_file_permission+0x39f/0x500
>>>      ? lock_is_held_type+0x9e/0x120
>>>      vfs_write+0x98e/0x1000
>>>      ...
>>>      </TASK>
>>>     The buggy address is located 0 bytes to the right of
>>>     allocated 4096-byte region [ffff88800de56000, ffff88800de57000)
>>> With this fix applied, rerunning the same sysctl-targeted path yields
>>> no corresponding KASAN reports.
>>>
>>> Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
>>> Signed-off-by: Dawei Feng <dawei.feng@seu.edu.cn>
>>> ---
>>>    kernel/bpf/cgroup.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>>> index 876f6a81a9b6..2c7f72d3fb11 100644
>>> --- a/kernel/bpf/cgroup.c
>>> +++ b/kernel/bpf/cgroup.c
>>> @@ -2342,6 +2342,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';
>> In v2 (https://lore.kernel.org/bpf/bf25d653-d856-4ad7-a751-b97d38f38892@linux.dev/)
>> I suggested
>>       memcpy(ctx->new_val, buf, buf_len + 1);
>> Does it work?
> may be it should be strscpy()?
> The input is a string, right?

The following is the bpf_sysctl_set_new_value proto:

static const struct bpf_func_proto bpf_sysctl_set_new_value_proto = {
         .func           = bpf_sysctl_set_new_value,
         .gpl_only       = false,
         .ret_type       = RET_INTEGER,
         .arg1_type      = ARG_PTR_TO_CTX,
         .arg2_type      = ARG_PTR_TO_MEM | MEM_RDONLY,
         .arg3_type      = ARG_CONST_SIZE,
};

So the input may not be a string.