* Re: [PATCH bpf-next v5 08/16] bpf: Support stack arguments for bpf functions
[not found] ` <20260417043525.6DAF1C2BCB6@smtp.kernel.org>
@ 2026-04-18 17:10 ` Yonghong Song
0 siblings, 0 replies; only message in thread
From: Yonghong Song @ 2026-04-18 17:10 UTC (permalink / raw)
To: sashiko; +Cc: bpf
On 4/16/26 9:35 PM, sashiko-bot@kernel.org wrote:
> Sashiko AI review found 5 potential issue(s):
> - [Critical] Verifier State Leakage in `copy_stack_state` when reusing states.
> - [Critical] Security / Liveness Tracking Bypass via `stack_arg_regs`.
> - [Critical] Use-After-Free via Type Mismatch in `bpf_get_spilled_stack_arg()`.
> - [Medium] Unnecessary Rejection of Valid Programs due to Dead Code Elimination.
> - [Low] Missing Parentheses in `bpf_get_spilled_stack_arg` Macro.
> --
>
> commit 6ec4a3fcb5e83a95626cd2bb13a60b2283e1c3e2
> Author: Yonghong Song <yonghong.song@linux.dev>
> bpf: Support stack arguments for bpf functions
>
> This commit adds support for passing more than 5 arguments to BPF subprograms
> by utilizing a dedicated stack area accessed via register r11. It introduces
> new fields to track incoming and outgoing stack arguments for bounds checking
> and precise verifier path tracking.
>
> Link: https://github.com/llvm/llvm-project/pull/189060
>
>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>> --- a/include/linux/bpf_verifier.h
>> +++ b/include/linux/bpf_verifier.h
>> @@ -508,6 +513,17 @@ struct bpf_verifier_state {
>> iter < frame->allocated_stack / BPF_REG_SIZE; \
>> iter++, reg = bpf_get_spilled_reg(iter, frame, mask))
>>
>> +#define bpf_get_spilled_stack_arg(slot, frame, mask) \
>> + (((slot < frame->stack_arg_depth / BPF_REG_SIZE) && \
>> + ((1 << frame->stack_arg_regs[slot].type) & (mask))) \
>> + ? &frame->stack_arg_regs[slot] : NULL)
> Is there a type mismatch in this check?
>
> The mask parameter here is passed as 1 << STACK_SPILL, but
> frame->stack_arg_regs[slot].type is an enum bpf_reg_type, not an enum
> bpf_stack_slot_type.
>
> For pointer types, this expression can evaluate to 0. This would cause the
> iterator to silently skip pointers stored in the stack argument area. If an
> acquired reference is dropped via release_reference(), the copy in the stack
> argument area wouldn't be invalidated, potentially allowing a use-after-free
> if a program subsequently reads the spilled pointer back from r11.
>
> Also, this isn't a bug, but should the slot parameter be enclosed in
> parentheses to prevent operator precedence issues if a complex expression
> is passed in the future?
Yes, it is a bug. Will fix.
>
> [ ... ]
>> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
>> --- a/kernel/bpf/fixups.c
>> +++ b/kernel/bpf/fixups.c
>> @@ -1266,9 +1275,20 @@ int bpf_fixup_call_args(struct bpf_verifier_env *env)
> [ ... ]
>> + for (i = 0; i < env->subprog_cnt; i++) {
>> + struct bpf_subprog_info *subprog = &env->subprog_info[i];
>> +
>> + if (subprog->max_out_stack_arg_depth > subprog->outgoing_stack_arg_depth) {
>> + verbose(env,
>> + "func#%d writes stack arg slot at depth %u, but calls only require %u bytes\n",
>> + i, subprog->max_out_stack_arg_depth, subprog->outgoing_stack_arg_depth);
>> + return -EINVAL;
>> + }
>> + }
> Could this reject valid programs if the compiler hoists a stack argument
> store above a branch that is later eliminated as dead code?
>
> If the compiler optimizes a store above a conditional check,
> max_out_stack_arg_depth would be updated. However, if the configuration
> constants cause the branch containing the actual function call to be
> eliminated, outgoing_stack_arg_depth would remain 0, leading to a rejection
> of a safe verification path.
We should be okay here. btf_check_func_arg_match() ensures func prototype
matches the number of outgoing parameters. Otherwise, it will fail:
if (i >= MAX_BPF_FUNC_REG_ARGS) {
struct bpf_func_state *caller = cur_func(env);
int spi = out_arg_spi(caller, i - MAX_BPF_FUNC_REG_ARGS);
/*
* The compiler may constant-fold stack arg values into the
* callee, eliminating the r11 stores. The BTF still declares
* these parameters, but no outgoing stack slots exist.
*/
if (spi >= (caller->stack_arg_depth / BPF_REG_SIZE)) {
verbose(env, "stack %s not found in caller state\n",
reg_arg_name(env, argno));
return -EINVAL;
}
}
>
> [ ... ]
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -1340,6 +1340,20 @@ static int copy_stack_state(struct bpf_func_state *dst, const struct bpf_func_st
>> return -ENOMEM;
>>
>> dst->allocated_stack = src->allocated_stack;
>> +
>> + /* copy stack args state */
>> + n = src->stack_arg_depth / BPF_REG_SIZE;
>> + if (n) {
>> + dst->stack_arg_regs = copy_array(dst->stack_arg_regs, src->stack_arg_regs, n,
>> + sizeof(struct bpf_reg_state),
>> + GFP_KERNEL_ACCOUNT);
>> + if (!dst->stack_arg_regs)
>> + return -ENOMEM;
>> +
>> + dst->stack_arg_depth = src->stack_arg_depth;
>> + dst->incoming_stack_arg_depth = src->incoming_stack_arg_depth;
>> + dst->out_stack_arg_mask = src->out_stack_arg_mask;
>> + }
>> return 0;
>> }
> Does this skip clearing the destination's stack argument fields if the source
> state has no stack arguments?
>
> If src->stack_arg_depth is 0, dst->stack_arg_depth, out_stack_arg_mask, and
> incoming_stack_arg_depth are not updated. When the verifier reuses an
> existing state during pop_stack() via bpf_copy_verifier_state(), the
> destination state might incorrectly inherit a stale stack argument
> initialization mask from a previously evaluated path.
>
> Could this allow a program to initialize stack arguments on one branch, and
> then safely perform a function call on a different branch without proper
> initialization?
Yes, you are right. dst->stack_arg_depth etc. should not under condition 'if (n)'.
>
> [ ... ]
>> @@ -4424,6 +4467,123 @@ static int check_stack_write(struct bpf_verifier_env *env,
> [ ... ]
>> +static int check_stack_arg_write(struct bpf_verifier_env *env, struct bpf_func_state *state,
>> + int off, int value_regno)
>> +{
> [ ... ]
>> + cur = env->cur_state->frame[env->cur_state->curframe];
>> + if (value_regno >= 0) {
>> + state->stack_arg_regs[spi] = cur->regs[value_regno];
>> + } else {
>> + /* BPF_ST: store immediate, treat as scalar */
>> + struct bpf_reg_state *arg = &state->stack_arg_regs[spi];
>> +
>> + arg->type = SCALAR_VALUE;
>> + __mark_reg_known(arg, env->prog->insnsi[env->insn_idx].imm);
>> + }
>> + state->out_stack_arg_mask |= BIT(idx);
>> + return 0;
>> +}
>> +
>> +/*
>> + * Read a value from the incoming stack arg area.
>> + * off is a positive offset from r11 (e.g. +8 for arg6, +16 for arg7).
>> + * Callers ensure off > 0, 8-byte aligned, and size is BPF_REG_SIZE.
>> + */
>> +static int check_stack_arg_read(struct bpf_verifier_env *env, struct bpf_func_state *state,
>> + int off, int dst_regno)
>> +{
> [ ... ]
>> + arg = &state->stack_arg_regs[spi];
>> + cur = env->cur_state->frame[env->cur_state->curframe];
>> +
>> + if (is_spillable_regtype(arg->type))
>> + copy_register_state(&cur->regs[dst_regno], arg);
>> + else
>> + mark_reg_unknown(env, cur->regs, dst_regno);
>> + return 0;
>> +}
> Does this bypass liveness tracking for stack arguments?
This will be addressed in liveness.c.
>
> Because the state is copied directly without calling mark_reg_read(), the
> live field of stack arguments is never marked as read.
>
> During state pruning, stack_arg_safe() delegates to regsafe(), which
> contains a fast-path that unconditionally returns true if the old state's
> register was never marked as read. Could this cause the verifier to
> aggressively prune verification paths even if they contain completely
> different and potentially unsafe stack arguments?
>
I think we should be okay. mark_reg_unknown will mark register as SCALAR_VALUE.
It will do pruning properly in regsafe().
^ permalink raw reply [flat|nested] only message in thread