Re: [PATCH bpf-next 01/18] bpf: Support stack arguments for bpf functions

Yonghong Song
2026-04-25T05:09:09Z

On 4/24/26 11:13 AM, bot+bpf-ci@kernel.org wrote:
>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>> index d5b4303315dd..2cc349d7fc17 100644
>> --- a/include/linux/bpf_verifier.h
>> +++ b/include/linux/bpf_verifier.h
> [ ... ]
>
>> @@ -739,10 +759,13 @@ struct bpf_subprog_info {
>> bool keep_fastcall_stack: 1;
>> bool changes_pkt_data: 1;
>> bool might_sleep: 1;
>> - u8 arg_cnt:3;
>> + u8 arg_cnt:4;
>>
>> enum priv_stack_mode priv_stack_mode;
>> - struct bpf_subprog_arg_info args[MAX_BPF_FUNC_REG_ARGS];
>> + struct bpf_subprog_arg_info args[MAX_BPF_FUNC_ARGS];
>> + u16 incoming_stack_arg_depth;
>> + u16 stack_arg_depth; /* incoming + max outgoing */
>> + u16 max_out_stack_arg_depth;
>> };
> Alexei Starovoitov raised a concern in the v6 review about duplicated
> fields. The incoming_stack_arg_depth, stack_arg_depth, and
> max_out_stack_arg_depth fields are present in both bpf_subprog_info and
> bpf_prog_aux structures. His comment was:
>
> "but you already have them in prog_aux?! another copy in
> bpf_subprog_info?! Remove one of them. JIT only need one set."
>
> Looking at include/linux/bpf.h, these fields appear in bpf_prog_aux:
>
> struct bpf_prog_aux {
> ...
> u16 incoming_stack_arg_depth;
> u16 stack_arg_depth;
> ...
> };
>
> Does the verifier actually need both copies? The concern was that having
> duplicate state creates maintenance burden.

Before jit_subprogs(), bpf_subprog_info stores the information for each subprog.
At this point, the subprog has not been allocated and only main prog is available.
Therefore, it is not possible to copy bpf_subprog_info to each subprog.

So I think it might be necessary to have both fields in bpf_prog_aux and
bpf_subprog_info.

This version made some changes so max_out_stack_arg_depth is not needed
any more. Will remove it in the next revision.

>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index ff6ff1c27517..bcf81692a22b 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
> [ ... ]
>
>> +/*
>> + * Write a value to the outgoing stack arg area.
>> + * off is a negative offset from r11 (e.g. -8 for arg6, -16 for arg7).
>> + */
>> +static int check_stack_arg_write(struct bpf_verifier_env *env, struct bpf_func_state *state,
>> + int off, int value_regno)
>> +{
>> + int max_stack_arg_regs = MAX_BPF_FUNC_ARGS - MAX_BPF_FUNC_REG_ARGS;
>> + struct bpf_subprog_info *subprog = &env->subprog_info[state->subprogno];
>> + int spi = -off / BPF_REG_SIZE - 1;
>> + struct bpf_func_state *cur;
>> + struct bpf_reg_state *arg;
>> + int err;
>> +
>> + if (spi >= max_stack_arg_regs) {
>> + verbose(env, "stack arg write offset %d exceeds max %d stack args\n",
>> + off, max_stack_arg_regs);
>> + return -EINVAL;
>> + }
>> +
>> + err = grow_stack_arg_slots(env, state, -off);
>> + if (err)
>> + return err;
>> +
>> + /* Track the max outgoing stack arg access depth. */
>> + if (-off > subprog->max_out_stack_arg_depth)
>> + subprog->max_out_stack_arg_depth = -off;
>> +
>> + cur = env->cur_state->frame[env->cur_state->curframe];
>> + if (value_regno >= 0) {
>> + state->stack_arg_regs[spi] = cur->regs[value_regno];
> Can this code handle offset=0 correctly? For an instruction like

This is not possible for this one and below other questions.
See commit 'bpf: Enable r11 based insns'. For load, the offset
must be 8, 16, .... For store, the offset must be -8, -16, etc.
And the size must be 8 (BPF_DW).

Also, at this point, r11 usage is not enabled and it will fail
at check_and_resolve_insns() way before any actual verification
(do_check_main()). So we should be okay.

> '*(u64 *)(r11 + 0) = r6', the computation spi = -0 / 8 - 1 = -1. The
> bounds check 'spi >= max_stack_arg_regs' would be false since -1 < 7,
> so execution proceeds to 'state->stack_arg_regs[-1]' which is an
> out-of-bounds array access.
>
> This also affects misaligned negative offsets. For off=-4, the result is
> spi = 4/8 - 1 = -1, causing the same OOB write.
>
> Additionally, there's no validation of the access size. A 4-byte write
> '*(u32 *)(r11 - 8) = r6' would be accepted and recorded as an 8-byte
> register spill in stack_arg_regs[0], but the JIT would emit a 4-byte
> store. The verifier's tracked state would not match the machine state.
>
> Looking at the referenced commit 5a36f6a2934e ("bpf: Enable r11 based
> insns") in the patch series, it adds validation in
> check_and_resolve_insns() that rejects r11 stores with off >= 0, non
> 8-byte aligned offsets, and non-BPF_DW sizes. Should this validation be
> in the current commit instead?
>
>> +/*
>> + * Read a value from the incoming stack arg area.
>> + * off is a positive offset from r11 (e.g. +8 for arg6, +16 for arg7).
>> + */
>> +static int check_stack_arg_read(struct bpf_verifier_env *env, struct bpf_func_state *state,
>> + int off, int dst_regno)
>> +{
>> + struct bpf_subprog_info *subprog = &env->subprog_info[state->subprogno];
>> + struct bpf_verifier_state *vstate = env->cur_state;
>> + int spi = off / BPF_REG_SIZE - 1;
>> + struct bpf_func_state *caller, *cur;
>> + struct bpf_reg_state *arg;
>> +
>> + if (state->no_stack_arg_load) {
>> + verbose(env, "r11 load must be before any r11 store or call insn\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (off > subprog->incoming_stack_arg_depth) {
>> + verbose(env, "invalid read from stack arg off %d depth %d\n",
>> + off, subprog->incoming_stack_arg_depth);
>> + return -EACCES;
>> + }
>> +
>> + caller = vstate->frame[vstate->curframe - 1];
>> + arg = &caller->stack_arg_regs[spi];
> Can this handle off=0 safely? For a load '*(u64 *)(r11 + 0)', spi would
> be 0/8 - 1 = -1, and the check 'off > incoming_stack_arg_depth' (0 >
> non-negative) would be false. Execution would continue to
> 'caller->stack_arg_regs[-1]', which is out-of-bounds.
>
> What about when this runs in the main program where curframe==0? The
> expression 'vstate->frame[vstate->curframe - 1]' would compute
> frame[(u32)-1], accessing frame[0xFFFFFFFF] which is far beyond the
> bounds of the frame[] array (MAX_CALL_FRAMES entries).
>
> Additionally, the code does not validate access size. A 4-byte load
> 'r0 = *(u32 *)(r11 + 8)' would be accepted and the verifier would copy
> the full 8-byte register from caller->stack_arg_regs[0], but the JIT
> would emit a 4-byte read.
>
> The later commit 5a36f6a2934e ("bpf: Enable r11 based insns") adds
> validation requiring loads to be BPF_LDX|BPF_MEM|BPF_DW with off > 0 and
> 8-byte aligned. Should that be part of this commit?
>
>> @@ -6605,10 +6737,20 @@ static int check_load_mem(struct bpf_verifier_env *env, struct bpf_insn *insn,
>> bool strict_alignment_once, bool is_ldsx,
>> bool allow_trust_mismatch, const char *ctx)
>> {
>> + struct bpf_verifier_state *vstate = env->cur_state;
>> + struct bpf_func_state *state = vstate->frame[vstate->curframe];
>> struct bpf_reg_state *regs = cur_regs(env);
>> enum bpf_reg_type src_reg_type;
>> int err;
>>
>> + /* Handle stack arg read */
>> + if (insn->src_reg == BPF_REG_PARAMS) {
>> + err = check_reg_arg(env, insn->dst_reg, DST_OP_NO_MARK);
>> + if (err)
>> + return err;
>> + return check_stack_arg_read(env, state, insn->off, insn->dst_reg);
>> + }
> Does this dispatch check the BPF_SIZE? A non-BPF_DW load through r11
> (e.g., 'r0 = *(u32 *)(r11 + 8)' which is BPF_W) goes to
> check_stack_arg_read, which treats it as a full 8-byte register copy
> from the caller. The verifier state would not match what the JIT emits.
>
>> @@ -6637,10 +6779,20 @@ static int check_load_mem(struct bpf_verifier_env *env, struct bpf_insn *insn,
>> static int check_store_reg(struct bpf_verifier_env *env, struct bpf_insn *insn,
>> bool strict_alignment_once)
>> {
>> + struct bpf_verifier_state *vstate = env->cur_state;
>> + struct bpf_func_state *state = vstate->frame[vstate->curframe];
>> struct bpf_reg_state *regs = cur_regs(env);
>> enum bpf_reg_type dst_reg_type;
>> int err;
>>
>> + /* Handle stack arg write */
>> + if (insn->dst_reg == BPF_REG_PARAMS) {
>> + err = check_reg_arg(env, insn->src_reg, SRC_OP);
>> + if (err)
>> + return err;
>> + return check_stack_arg_write(env, state, insn->off, insn->src_reg);
>> + }
> Similarly for stores, does this gate on BPF_SIZE? A 4-byte store
> '*(u32 *)(r11 - 8) = r6' (BPF_W) would be accepted here and recorded as
> an 8-byte spill, creating the same verifier/JIT mismatch.
>
>> @@ -17627,6 +17804,14 @@ static int do_check_insn(struct bpf_verifier_env *env, bool *do_print_state)
>> return check_store_reg(env, insn, false);
>>
>> case BPF_ST: {
>> + /* Handle stack arg write (store immediate) */
>> + if (insn->dst_reg == BPF_REG_PARAMS) {
>> + struct bpf_verifier_state *vstate = env->cur_state;
>> + struct bpf_func_state *state = vstate->frame[vstate->curframe];
>> +
>> + return check_stack_arg_write(env, state, insn->off, -1);
>> + }
> Does this check BPF_SIZE as well? A BPF_ST with dst_reg=BPF_REG_PARAMS
> and size != BPF_DW would have the same issue.
>
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/24902767240