Re: [PATCH bpf-next v2 07/12] bpf: Prepare kfunc arguments for the JIT from an ABI description

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

On 9/8/26 11:46 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Stack buffer overflow in `bpf_jit_plan_arg_moves` and `kfunc_abi_slots` due to undersized arrays based on maximum arguments rather than maximum slots.
> --
>
> commit 180655f49c406c35435ba5ebc05f2900a3c56656
> Author: Yonghong Song <yonghong.song@linux.dev>
>
> bpf: Prepare kfunc arguments for the JIT from an ABI description
>      
> The previous patch refuses a kfunc argument of more than one eightbyte.
> This patch allows up to 16 byte kfunc arguments.
>
>> diff --git a/include/linux/filter.h b/include/linux/filter.h
>> index 00ad8b63aa477..01b52d0259d18 100644
>> --- a/include/linux/filter.h
>> +++ b/include/linux/filter.h
> [ ... ]
>> @@ -1248,6 +1248,38 @@ bool bpf_jit_supports_insn(struct bpf_insn *insn, bool in_arena);
>> +
>> +/* The JIT's scratch register, in place of an argument slot. */
>> +#define BPF_JIT_ARG_TMP		0xff
>> +
>> +/* Every argument slot moves at most once, and the scratch goes out and back. */
>> +#define BPF_JIT_MAX_ARG_MOVES	(MAX_BPF_FUNC_ARGS + 2)
> [Severity: Medium]
> Since arguments can now take up to 2 slots (for 16-byte arguments), could the
> maximum number of slot moves actually be up to 26 (24 slots + 2) instead of 14?

In jit, for x86_64, we have

         /* x86-64 supports up to MAX_BPF_FUNC_ARGS arguments. 1-6
          * are passed through regs, the remains are through stack.
          */
         if (nr_regs > MAX_BPF_FUNC_ARGS)
                 return -ENOTSUPP;

So the maximum number of regs is 12, so we are okay for x86_64.

But arm64 is different. It has

static int calc_arg_aux(const struct btf_func_model *m,
                          struct arg_aux *a)
{
         int stack_slots, nregs, slots, i;

         /* verifier ensures m->nr_args <= MAX_BPF_FUNC_ARGS */
         for (i = 0, nregs = 0; i < m->nr_args; i++) {
                 slots = (m->arg_size[i] + 7) / 8;
                 if (nregs + slots <= 8) /* passed through register ? */
                         nregs += slots;
                 else
                         break;
         }

	...
}

So the maximum number of reg arguments can be 24. I think I will
add a patch similar to x86_64 above.

>
>> +
>> +struct bpf_jit_arg_move {
>> +	u8 dst;
>> +	u8 src;
>> +};
> [ ... ]
>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
>> index c673b02d55a65..d4bd2ba9aadee 100644
>> --- a/kernel/bpf/core.c
>> +++ b/kernel/bpf/core.c
> [ ... ]
>> @@ -3287,6 +3287,100 @@ bool __weak bpf_jit_supports_kfunc_ret_reg_pair(void)
>> +
>> +u32 bpf_jit_place_args(const struct bpf_jit_arg_abi *abi,
>> +		       const struct btf_func_model *fm, u8 *pos_of_slot)
>> +{
>> +	u32 i, k, nslots, slot = 0, nregs_used = 0, stack_off = 0;
>> +	bool on_stack = false;
>> +
>> +	for (i = 0; i < fm->nr_args; i++) {
>> +		bool align16 = fm->arg_flags[i] & BTF_FMODEL_ALIGN16_ARG;
>> +		u32 pos;
>> +
>> +		nslots = btf_func_model_arg_slots(fm, i);
> [ ... ]
>> +		for (k = 0; k < nslots; k++)
>> +			pos_of_slot[slot + k] = pos + k;
> [Severity: Medium]
> Will this overflow the pos_of_slot array if an architecture adopts this API
> for BPF trampolines?
>
> The callers currently size the array based on MAX_BPF_FUNC_ARGS (12), but
> slot + k can reach up to 23 because some arguments can require multiple slots.

I will add a check to ensure maximum reg arguments is 12.

>
>> +		slot += nslots;
>> +	}
>> +
>> +	return slot;
>> +}
>> +
>> +u32 bpf_jit_plan_arg_moves(const struct bpf_jit_arg_abi *abi,
>> +			   const struct btf_func_model *fm,
>> +			   struct bpf_jit_arg_move *moves)
>> +{
>> +	u8 pos_of_slot[MAX_BPF_FUNC_ARGS];
> [Severity: Medium]
> Does this stack array need to be sized for the maximum number of slots
> (e.g., MAX_BPF_FUNC_ARGS * 2) instead of the maximum number of arguments?

This is for kfunc. I will check whether we need to do
   MAX_BPF_FUNC_ARGS * 2
or not.

>
>> +	u32 nslots, n = 0, s, back;
>> +
>> +	nslots = bpf_jit_place_args(abi, fm, pos_of_slot);
> [ ... ]
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 94c359351bb69..2b8df8c7f098c 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
> [ ... ]
>> @@ -12172,6 +12172,58 @@ static u32 kfunc_proto_slots(const struct btf *btf, const struct btf_type *func_
>>   	return slots_used;
>>   }
>>   
>> +static u32 kfunc_abi_slots(const struct btf_func_model *fm)
>> +{
>> +	const struct bpf_jit_arg_abi *abi = bpf_jit_arg_abi();
>> +	u8 pos_of_slot[MAX_BPF_FUNC_ARGS];
> [Severity: Medium]
> Similarly, does this stack array also need to be sized to accommodate up to
> 24 slots rather than 12 arguments to prevent an out-of-bounds write in
> bpf_jit_place_args()?

Yes, I will check whether we need to have 24 slots or not.

>
>> +	u32 i, nslots, slots = 0;
>> +
>> +	for (i = 0; i < fm->nr_args; i++)
>> +		slots += btf_func_model_arg_slots(fm, i);
>> +
>> +	if (!abi)
>> +		return slots;
>> +
>> +	nslots = bpf_jit_place_args(abi, fm, pos_of_slot);