Re: [PATCH bpf-next v4 01/15] bpf: Read a kfunc's __sz argument only when it is in a register

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

On 9/12/26 1:06 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Missing negative bounds check for enum parameters in memcg kfuncs allows out-of-bounds array read
> --
>
> commit 5816e44e0d469f2be29753390131ba5c9f8fd073
> Author: Yonghong Song <yonghong.song@linux.dev>
>
> bpf: Read a kfunc's __sz argument only when it is in a register
>
> This commit fixes an issue where an out-of-bounds index could be used to read
> from const_reg_vals[] when a kfunc receives a stack argument pair of a
> pointer and size. It ensures size_reg is bounded by MAX_BPF_FUNC_REG_ARGS
> to prevent reading unrelated callee-saved registers.
>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 617a277c3558c..0c6a404167ca4 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -13641,13 +13641,14 @@ s64 bpf_kfunc_stack_access_bytes(struct bpf_verifier_env *env, struct bpf_insn *
>>   		goto out;
>>   	}
>>   
>> -	/* ptr + __sz/__szk pair: size is in the next register */
>> +	/* ptr + __sz/__szk pair: the size follows the pointer */
>>   	if (arg + 1 < nargs &&
>>   	    (btf_param_match_suffix(btf, &args[arg + 1], "__sz") ||
>>   	     btf_param_match_suffix(btf, &args[arg + 1], "__szk"))) {
>>   		int size_reg = BPF_REG_1 + arg + 1;
>>   
>> -		if (aux->const_reg_mask & BIT(size_reg)) {
>> +		if (size_reg <= MAX_BPF_FUNC_REG_ARGS &&
>> +		    (aux->const_reg_mask & BIT(size_reg))) {
>>   			size = (s64)aux->const_reg_vals[size_reg];
>>   			goto out;
>>   		}
> [Severity: High]
> This is a pre-existing issue, but I noticed a potential problem with how the
> verifier handles enum arguments for kfuncs, specifically in the memcg
> kfuncs.
>
> Since the BPF verifier treats enum types as plain signed integers and does not
> enforce enum ranges natively, BPF programs can pass negative scalar values
> to kfuncs.
>
> In mm/bpf_memcontrol.c:bpf_mem_cgroup_memory_events():
>
> 	if (unlikely(event >= MEMCG_NR_MEMORY_EVENTS))
> 		return (unsigned long)-1;
>
> 	return atomic_long_read(&memcg->memory_events[event]);
>
> Does this signed comparison without a negative bound allow a negative
> event value to bypass the check and cause an out-of-bounds read on the
> memory_events array?
>
> Similarly, in mm/bpf_memcontrol.c:bpf_mem_cgroup_vm_events():
>
> 	if (unlikely(!memcg_vm_event_item_valid(event)))
> 		return (unsigned long)-1;
>
> 	return memcg_events(memcg, event);
>
> And the check in mm/memcontrol.c:memcg_vm_event_item_valid():
>
> 	if (idx >= NR_VM_EVENT_ITEMS)
> 		return false;
>
> 	return !BAD_STAT_IDX(memcg_events_index(idx));
>
> Could a negative idx here bypass the bounds check and lead to an
> out-of-bounds access on the mem_cgroup_events_index array?

I think we should be okay here. The kfunc itself is responsible to do 
proper bound checking before doing other stuff, as examples in the above.