Re: [PATCH v4 sched_ext/for-7.3 37/40] sched_ext: Add scx_bpf_sub_kill() to evict a child sub-scheduler

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing `__str` suffix on the `fmt` parameter in `scx_bpf_sub_kill_bstr()` allows out-of-bounds kernel memory read.
--

--- Patch [37]: [PATCH v4 sched_ext/for-7.3 37/40] sched_ext: Add scx_bpf_sub_kill() to evict a child sub-scheduler ---
commit dffeca7158a063ab8ec3cb4f59c9c43d77050c68
Author: Tejun Heo <[email protected]>
sched_ext: Add scx_bpf_sub_kill() to evict a child sub-scheduler

This commit adds scx_bpf_sub_kill() to allow a cid-form scheduler to evict
a direct child sub-scheduler. The child is torn down asynchronously via the
exit kind SCX_EXIT_PARENT_KILL. It exposes bstr formatting infrastructure
to support this.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 44622dcc8b4f3..5c9cd3682f086 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c

[ ... ]

> @@ -9362,7 +9358,7 @@ __bpf_kfunc void scx_bpf_exit_bstr(s64 exit_code, char *fmt,
>  	raw_spin_lock_irqsave(&scx_exit_bstr_buf_lock, flags);
>  	sch = scx_prog_sched(aux);
>  	if (likely(sch) &&
> -	    bstr_format(sch, &scx_exit_bstr_buf, fmt, data, data__sz) >= 0)
> +	    scx_bstr_format(sch, &scx_exit_bstr_buf, fmt, data, data__sz) >= 0)
>  		scx_exit(sch, SCX_EXIT_UNREG_BPF, exit_code, "%s", scx_exit_bstr_buf.line);

[Severity: High]
This is a pre-existing issue, but does scx_bpf_exit_bstr() allow an
out-of-bounds kernel memory read by omitting the __str suffix from the fmt
parameter?

Since fmt does not have the __str suffix (i.e. fmt__str), the BPF verifier
treats it as KF_ARG_PTR_TO_MEM instead of KF_ARG_PTR_TO_CONST_STR and does
not check for a NUL terminator at load time.

If a BPF program passes a non-NUL-terminated buffer, the fmt pointer is
ultimately passed to bpf_bprintf_prepare() in __bstr_format() with
fmt_size = UINT_MAX, which calls strnchr(fmt, UINT_MAX, 0). This could loop
over unmapped memory boundaries and cause a page fault.

> @@ -9387,7 +9383,7 @@ __bpf_kfunc void scx_bpf_error_bstr(char *fmt, unsigned long long *data,
>  	raw_spin_lock_irqsave(&scx_exit_bstr_buf_lock, flags);
>  	sch = scx_prog_sched(aux);
>  	if (likely(sch) &&
> -	    bstr_format(sch, &scx_exit_bstr_buf, fmt, data, data__sz) >= 0)
> +	    scx_bstr_format(sch, &scx_exit_bstr_buf, fmt, data, data__sz) >= 0)
>  		scx_exit(sch, SCX_EXIT_ERROR_BPF, 0, "%s", scx_exit_bstr_buf.line);

[Severity: High]
This is a pre-existing issue, but does scx_bpf_error_bstr() also suffer from
the same missing __str suffix on the fmt parameter?

[ ... ]

> diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
> index 77cb7f3d7826b..b3e72fffb0f6c 100644
> --- a/kernel/sched/ext/sub.c
> +++ b/kernel/sched/ext/sub.c

[ ... ]

> +__printf(2, 0)
> +__bpf_kfunc s32 scx_bpf_sub_kill_bstr(u64 cgroup_id, char *fmt,
> +				      unsigned long long *data, u32 data__sz,
> +				      const struct bpf_prog_aux *aux)
> +{

[Severity: High]
Does scx_bpf_sub_kill_bstr() introduce the same vulnerability by missing the
__str suffix on the fmt parameter?

Could we change this parameter to fmt__str to ensure the BPF verifier
enforces a NUL-terminated constant string?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=37