Re: [PATCH] sched_ext: Gate cid kfuncs behind the SCX struct_ops check

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

Pre-existing issues:
- [High] Missing negative bounds checks in memory cgroup kfuncs (`bpf_mem_cgroup_memory_events` and `bpf_mem_cgroup_vm_events`) allow out-of-bounds array access.
--

--- Patch [1]: [PATCH] sched_ext: Gate cid kfuncs behind the SCX struct_ops check ---
commit 44e99221673a68d8565292bb70385d8d0629f96c
Author: fangqiurong <[email protected]>
sched_ext: Gate cid kfuncs behind the SCX struct_ops check

This commit updates scx_kfunc_context_filter() to recognize scx_kfunc_ids_cid.
It ensures that cid kfuncs are properly restricted and cannot be called from
arbitrary struct_ops programs.

[ ... ]

While reviewing this, I noticed a pre-existing issue in the BPF memory
cgroup kfuncs.

In mm/bpf_memcontrol.c:

> __bpf_kfunc unsigned long bpf_mem_cgroup_memory_events(struct mem_cgroup *memcg,
> 						       enum memcg_memory_event event)
> {
> 	if (unlikely(event >= MEMCG_NR_MEMORY_EVENTS))
> 		return (unsigned long)-1;
> 
> 	return atomic_long_read(&memcg->memory_events[event]);
> }

[Severity: High]
This is a pre-existing issue, but can a negative enum value bypass this bounds
check?

If a BPF program passes a negative integer for the event argument, the BPF
verifier does not enforce enum ranges, allowing the negative value to reach
the kfunc.

Because the check event >= MEMCG_NR_MEMORY_EVENTS performs a signed comparison,
would it evaluate to false for negative values and lead to an out-of-bounds
read of memcg->memory_events[event]?

In mm/memcontrol.c:

> bool memcg_vm_event_item_valid(enum vm_event_item idx)
> {
> 	if (idx >= NR_VM_EVENT_ITEMS)
> 		return false;
> 
> 	return !BAD_STAT_IDX(memcg_events_index(idx));
> }

And in mm/bpf_memcontrol.c:

> __bpf_kfunc unsigned long bpf_mem_cgroup_vm_events(struct mem_cgroup *memcg,
> 						   enum vm_event_item event)
> {
> 	if (unlikely(!memcg_vm_event_item_valid(event)))
> 		return (unsigned long)-1;
> 
> 	return memcg_events(memcg, event);
> }

[Severity: High]
This isn't a bug introduced by this patch, but does memcg_vm_event_item_valid()
also miss negative enum values?

Since the bounds check uses a signed comparison, a negative idx evaluates to
false for idx >= NR_VM_EVENT_ITEMS.

When bpf_mem_cgroup_vm_events() passes an unfiltered enum to this helper, could
this result in an out-of-bounds read in memcg_events_index(idx) or
memcg_events()?

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