Re: [PATCH bpf-next v2 09/23] bpf: Extend liveness analysis to track stack argument slots

On 5/9/26 2:59 AM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] record_load_store_access() mistakenly uses the tracking state of stack argument slot 0 as the base pointer for R11-based memory accesses, leading to spurious liveness marks.
> - [Low] Missing verifier logging for stack argument liveness state transitions.
> --
>
> commit 00a5fce117daa13f26c61ec5ffa3622241608fc1
> Author: Yonghong Song <[email protected]>
>
> bpf: Extend liveness analysis to track stack argument slots
>
> This commit extends liveness analysis to handle stack argument slots, preventing
> pointers passed through stack args from incorrectly poisoning the pointed-to
> stack slots. It explicitly handles BPF_REG_PARAMS (R11) to avoid out-of-bounds
> accesses and extends tracking arrays.
>
>> diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c
>> index de0b4c6263ff2..eed7260f2bb0a 100644
>> --- a/kernel/bpf/liveness.c
>> +++ b/kernel/bpf/liveness.c
>> @@ -610,6 +610,24 @@ enum arg_track_state {
>>   /* Track callee stack slots fp-8 through fp-512 (64 slots of 8 bytes each) */
>>   #define MAX_ARG_SPILL_SLOTS 64
>>   
>> +/* Track stack arg slots: outgoing starts at -(i+1)*8, incoming at +(i+1)*8 */
>> +#define MAX_STACK_ARG_SLOTS (MAX_BPF_FUNC_ARGS - MAX_BPF_FUNC_REG_ARGS)
>> +
>> +/*
>> + * Combined register + stack arg tracking: R0-R10 at indices 0-10,
>> + * outgoing stack arg slots at indices MAX_BPF_REG..MAX_BPF_REG+6.
>> + */
>> +#define MAX_AT_TRACK_REGS (MAX_BPF_REG + MAX_STACK_ARG_SLOTS)
> Could this repurposed index cause a spurious liveness mark in
> record_load_store_access()?
>
> Because at[MAX_BPF_REG] is repurposed to track the value stored in outgoing
> stack argument slot 0, what happens when record_load_store_access() processes
> stack argument instructions like is_stack_arg_stx()?
>
> record_load_store_access() does this:
>
> 	case BPF_STX:
> 		if (BPF_MODE(insn->code) == BPF_ATOMIC) {
> 			...
> 		} else {
> 			ptr = &at[insn->dst_reg];
> 			sz = -sz;
> 		}
>
> If the instruction is a stack argument access, insn->dst_reg is BPF_REG_PARAMS
> (which equals MAX_BPF_REG). It will fetch &at[MAX_BPF_REG]. If a BPF program
> previously stored a frame pointer (FP) into slot 0, at[MAX_BPF_REG] will reflect
> that FP.
>
> Does this mean record_load_store_access() will spuriously treat this FP as the
> base pointer and incorrectly mark the resulting local stack frame slot as
> accessed, corrupting the local stack liveness mask?

In this case, if the insn is r11-based insn in record_load_store_access(),
we should just return since record_load_store_access() is for fp based stacks.

>
> [ ... ]
>> @@ -1616,7 +1663,8 @@ static int compute_subprog_args(struct bpf_verifier_env *env,
>>   		memcpy(at_out, at_in[i], sizeof(at_out));
>>   		memcpy(at_stack_out, at_stack_in[i], MAX_ARG_SPILL_SLOTS * sizeof(*at_stack_out));
>>   
>> -		arg_track_xfer(env, insn, idx, at_out, at_stack_out, instance, callsites);
>> +		arg_track_xfer(env, insn, idx, at_out, at_stack_out,
>> +			       at_stack_arg_entry, instance, callsites);
>>   		arg_track_log(env, insn, idx, at_in[i], at_stack_in[i], at_out, at_stack_out);
> Are the state transitions for stack arguments logged here?
>
> In arg_track_log(), the loop for logging register transitions is strictly
> bounded by MAX_BPF_REG:
>
> 	for (i = 0; i < MAX_BPF_REG; i++) {
> 		if (arg_track_eq(&at_out[i], &at_in[i]))
> 			continue;
>
> Since the tracking arrays at_in and at_out were extended to MAX_AT_TRACK_REGS to
> include state for outgoing stack arguments at indices MAX_BPF_REG through
> MAX_AT_TRACK_REGS - 1, does arg_track_log() silently omit logging their state
> transitions?

Missed the logging issue for stack argument tracking. Will fix.