Re: bpf: incorrect value spill in check_stack_write_fixed_off()

[Hao Sun]

On Wed, Oct 25, 2023 at 11:16 AM Hao Sun <sunhao.th@xxxxxxxxx> wrote:
>
> Hi,
>
> In check_stack_write_fixed_off(), the verifier creates a fake reg to store the
> imm in a BPF_ST_MEM:
> ...
> else if (!reg && !(off % BPF_REG_SIZE) && is_bpf_st_mem(insn) &&
> insn->imm != 0 && env->bpf_capable) {
>         struct bpf_reg_state fake_reg = {};
>
>         __mark_reg_known(&fake_reg, (u32)insn->imm);
>         fake_reg.type = SCALAR_VALUE;
>         save_register_state(state, spi, &fake_reg, size);
>
> Here, insn->imm is cast to u32, and used to mark fake_reg, which is incorrect
> and may lose sign information. Consider the following program:
>
> r2 = r10
> *(u64*)(r2 -40) = -44
> r0 = *(u64*)(r2 - 40)
> if r0 s<= 0xa goto +2
> r0 = 0
> exit
> r0  = 1
> exit
>

Sorry, the program should be:

 r2 = r10
 *(u64*)(r2 -40) = -44
 r0 = *(u64*)(r2 - 40)
 if r0 s<= 0xa goto +2
 r0 = 1
 exit
 r0  = 0
 exit

Here is the C macros for the following verifier's log:

BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
BPF_ST_MEM(BPF_DW, BPF_REG_2, -40, -44),
BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_2, -40),
BPF_JMP_IMM(BPF_JSLT, BPF_REG_0, 0xa, 2),
BPF_MOV64_IMM(BPF_REG_0, 1),
BPF_EXIT_INSN(),
BPF_MOV64_IMM(BPF_REG_0, 0),
BPF_EXIT_INSN()

> The verifier gives the following log:
>
> -------- Verifier Log --------
> 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
>
> Here, the verifier incorrectly thinks R0 is 0xffffffd4, which should
> be 0xffffffffffffffd4,
> due to the u32 cast in check_stack_write_fixed_off(). This makes the verifier
> collect incorrect reg scalar range.
>
> Since insn->imm is i32, we should cast it to the signed integer with
> correct size
> according to BPF_MEM, then promoting the imm to u64 to mark fake reg as
> known, right?
>
> Best
> Hao