Re: [PATCH bpf-next v5 08/16] bpf: Support stack arguments for bpf functions

On 4/16/26 9:35 PM, [email protected] 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 <[email protected]>
> 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().