Re: [PATCH bpf v2 1/2] bpf: Fix check_stack_write_fixed_off() to correctly spill imm

[Shung-Hsi Yu]

On Wed, Nov 01, 2023 at 08:33:22AM +0100, Hao Sun wrote:
> In check_stack_write_fixed_off(), imm value is cast to u32 before being
> spilled to the stack. Therefore, the sign information is lost, and the
> range information is incorrect when load from the stack again.
> 
> For the following prog:
> 0: r2 = r10
> 1: *(u64*)(r2 -40) = -44
> 2: r0 = *(u64*)(r2 - 40)
> 3: if r0 s<= 0xa goto +2
> 4: r0 = 1
> 5: exit
> 6: r0  = 0
> 7: exit
> 
> The verifier gives:
> func#0 @0
> 0: R1=ctx(off=0,imm=0) R10=fp0
> 0: (bf) r2 = r10                      ; R2_w=fp0 R10=fp0
> 1: (7a) *(u64 *)(r2 -40) = -44        ; R2_w=fp0 fp-40_w=4294967252
> 2: (79) r0 = *(u64 *)(r2 -40)         ; R0_w=4294967252 R2_w=fp0
> fp-40_w=4294967252
> 3: (c5) if r0 s< 0xa goto pc+2
> mark_precise: frame0: last_idx 3 first_idx 0 subseq_idx -1
> mark_precise: frame0: regs=r0 stack= before 2: (79) r0 = *(u64 *)(r2 -40)
> 3: R0_w=4294967252
> 4: (b7) r0 = 1                        ; R0_w=1
> 5: (95) exit
> verification time 7971 usec
> stack depth 40
> processed 6 insns (limit 1000000) max_states_per_insn 0 total_states 0
> peak_states 0 mark_read 0
> 
> So remove the incorrect cast, since imm field is declared as s32, and
> __mark_reg_known() takes u64, so imm would be correctly sign extended
> by compiler.
> 
> Fixes: ecdf985d7615 ("bpf: track immediate values written to stack by BPF_ST instruction")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Hao Sun <sunhao.th@xxxxxxxxx>

Acked-by: Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx>