Re: [PATCH 2/2] sched_ext: Make scx_bpf_events() read the calling scheduler's counters

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

Pre-existing issues:
- [High] A tracing or syscall BPF program can cause an out-of-bounds memory read by associating with a non-sched_ext struct_ops map (like `tcp_congestion_ops`), which is then incorrectly cast to `struct sched_ext_ops *` by `scx_prog_sched()`.
--

--- Patch [2]: [PATCH 2/2] sched_ext: Make scx_bpf_events() read the calling scheduler's counters ---
commit c53feaf881922f5f2912216d45a7b765df97ad06
Author: Tejun Heo <[email protected]>

sched_ext: Make scx_bpf_events() read the calling scheduler's counters

scx_bpf_events() now resolves the scheduler from the calling program with
scx_prog_sched(), instead of always reading the root scheduler's event
counters.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 40c9467d8b186..ee865d0f65112 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c

[ ... ]

> @@ -10418,19 +10418,23 @@ static void scx_read_events(struct scx_sched *sch, struct scx_event_stats *event
>  	}
>  }
>  
> -/*
> - * scx_bpf_events - Get a system-wide event counter to
> +/**
> + * scx_bpf_events - Read the event counters of the calling scheduler
>   * @events: output buffer from a BPF program
> - * @events__sz: @events len, must end in '__sz'' for the verifier
> + * @events__sz: @events len, must end in '__sz' for the verifier
> + * @aux: implicit BPF argument to access bpf_prog_aux hidden from BPF progs
> + *
> + * Read the event counters of the scheduler associated with the calling program.
> + * @events is zeroed when no scheduler can be resolved.
>   */
> -__bpf_kfunc void scx_bpf_events(struct scx_event_stats *events,
> -				size_t events__sz)
> +__bpf_kfunc void scx_bpf_events(struct scx_event_stats *events, size_t events__sz,
> +				const struct bpf_prog_aux *aux)
>  {
>  	struct scx_sched *sch;
>  	struct scx_event_stats e_sys;
>  
>  	rcu_read_lock();
> -	sch = rcu_dereference(scx_root);
> +	sch = scx_prog_sched(aux);

[Severity: High]
This is a pre-existing issue, but does this expose us to a type confusion
and out-of-bounds read?

Looking at scx_prog_sched() in kernel/sched/ext/internal.h, it retrieves
associated struct_ops:

kernel/sched/ext/internal.h:scx_prog_sched() {
    ...
    ops = bpf_prog_get_assoc_struct_ops(aux);
    if (likely(ops))
        return rcu_dereference_all(ops->priv);
    ...
}

If a BPF_PROG_TYPE_TRACING or BPF_PROG_TYPE_SYSCALL program is associated
with a different struct_ops map (like tcp_congestion_ops), the pointer is
blindly cast to struct sched_ext_ops *.

Since struct sched_ext_ops is much larger than other struct_ops structures,
reading ops->priv could read memory well past the end of the map allocation
and cause a kernel panic when the garbage value is dereferenced as a
struct scx_sched *.

Does scx_prog_sched() need to verify the type of the associated struct_ops
before accessing ops->priv?

>  	if (sch)
>  		scx_read_events(sch, &e_sys);
>  	else

[ ... ]

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