Re: [PATCH v3 sched_ext/for-7.3 34/36] 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] The `fmt` argument in the newly added `scx_bpf_sub_kill_bstr` kfunc lacks the `__str` suffix, bypassing the BPF verifier's constant string checks and allowing a malicious BPF program to trigger an out-of-bounds read.
--

--- Patch [34]: [PATCH v3 sched_ext/for-7.3 34/36] sched_ext: Add scx_bpf_sub_kill() to evict a child sub-scheduler ---
commit a7bc8fe4077dcf82d7a123efd11e286c81b3c177
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() and its corresponding kfunc to allow a
cid-form scheduler to evict a direct child sub-scheduler. It disables the
child asynchronously and formats a printf-style string reason to reach the
child's scx_exit_info.

[ ... ]

> diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
> index 55cd1f9a597a0..e550cfb366ad2 100644
> --- a/kernel/sched/ext/sub.c
> +++ b/kernel/sched/ext/sub.c

[ ... ]

> @@ -1685,6 +1685,63 @@ __bpf_kfunc s32 scx_bpf_sub_caps(u64 cgroup_id, u64 caps, struct scx_cmask *out_
>  	return 0;
>  }
>  
> +/**
> + * scx_bpf_sub_kill_bstr - Kill a direct child sub-scheduler
> + * @cgroup_id: cgroup id of the direct child to kill
> + * @fmt: reason message format string
> + * @data: format string parameters packaged using ___bpf_fill() macro
> + * @data__sz: @data len, must end in '__sz' for the verifier
> + * @aux: implicit BPF argument to access bpf_prog_aux hidden from BPF progs
> + *
> + * Evict a direct child sub-scheduler, disabling it with the supplied reason.
> + * The child and its subtree are torn down asynchronously through the usual
> + * disable path.
> + *
> + * Unlike scx_bpf_exit(), no exit code is taken: the child is a separate
> + * scheduler with its own exit-code semantics, so a code chosen by the parent
> + * would have no defined meaning. The reason string carries the intent.
> + *
> + * Return 0 on success or -ENODEV if @cgroup_id names no sub-scheduler, which
> + * can race with the child detaching on its own and so is not a scheduler error.
> + * Naming a sched that exists but is not a direct child aborts the parent.
> + */
> +__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]
Should the fmt parameter be named fmt__str here?

Without the __str suffix, the BPF verifier treats the char *fmt argument as a
KF_ARG_PTR_TO_MEM of size 1 (the size of char), rather than enforcing the
KF_ARG_PTR_TO_CONST_STR checks.

If a BPF program directly calls scx_bpf_sub_kill_bstr() with a 1-byte pointer
that is not null-terminated, scx_bstr_format() will call
bpf_bprintf_prepare(fmt, UINT_MAX, ...) which eventually calls
strnchr(fmt, UINT_MAX, 0).

Could this allow a BPF program to read beyond the boundary of the provided
memory, potentially causing a page fault or out-of-bounds read?

> +	struct scx_sched *parent, *child;
> +	s32 ret;
> +
> +	guard(rcu)();

[ ... ]

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