Re: [PATCH bpf-next v5 10/14] bpf: Fix dynptr ref counting to scan all call frames

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The macro `bpf_for_each_reg_in_vstate_mask` incorrectly computes the `stack` pointer when iterating over outgoing stack arguments. The new `dynptr_ref_cnt` function uses this macro and dereferences the invalid pointer, leading to a verifier crash or security bypass.
--

commit 2fbeda3e91d76b77c3a4fe5a3269d7a89c9d8588
Author: Amery Hung <ameryhung@gmail.com>

bpf: Fix dynptr ref counting to scan all call frames
    
This commit extracts dynptr reference counting into a new dynptr_ref_cnt() 
function to check whether a referenced dynptr can be overwritten. It uses 
bpf_for_each_reg_in_vstate_mask() to scan dynptr stack slots across all 
call frames instead of just the current frame.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index b7984c42ca145..97eb8033d732e 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -786,10 +786,29 @@ static void mark_reg_invalid(const struct bpf_verifier_env *env, struct bpf_reg_
[ ... ]
> +static int dynptr_ref_cnt(struct bpf_verifier_env *env, int v_parent_id)
> +{
> +	struct bpf_stack_state *stack;
> +	struct bpf_func_state *state;
> +	struct bpf_reg_state *reg;
> +	int ref_cnt = 0;
> +
> +	bpf_for_each_reg_in_vstate_mask(env->cur_state, state, reg, stack, 1 << STACK_DYNPTR, ({
> +		if (!stack || stack->slot_type[0] != STACK_DYNPTR)
> +			continue;

[Severity: Critical]
Does bpf_for_each_reg_in_vstate_mask() safely provide the stack pointer when
iterating over outgoing stack arguments?

Looking at the macro definition in include/linux/bpf_verifier.h:

    bpf_for_each_spilled_stack_arg(___j, __state, __reg) { \
        if (!__reg)                              \
            continue;                        \
        __stack = &__state->stack[___j];         \
        (void)(__expr);                          \
    }

When iterating over outgoing stack arguments, it calculates __stack using the
local stack array (__state->stack). Since out_stack_arg_cnt is independent
of allocated_stack, __state->stack could be NULL.

If ___j > 0 and __state->stack is NULL, __stack would evaluate to a non-NULL
offset. Would this bypass the !stack check above and result in an
out-of-bounds read or page fault when evaluating stack->slot_type[0]?

Additionally, if __state->stack is valid, could this evaluate the same local
stack slot twice, artificially inflating ref_cnt and potentially bypassing
the ref_cnt <= 1 check?

> +		if (!stack->spilled_ptr.dynptr.first_slot)
> +			continue;
> +		if (stack->spilled_ptr.parent_id == v_parent_id)
> +			ref_cnt++;
> +	}));
> +
> +	return ref_cnt;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519181314.2731658-1-ameryhung@gmail.com?part=10