On Wed, Oct 25, 2023 at 2:48 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > On Wed, 2023-10-25 at 15:14 +0300, Eduard Zingerman wrote: > > On Wed, 2023-10-25 at 11:16 +0200, Hao Sun 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. > > > > This bug is on me. > > Thank you for reporting it along with the example program. > > Looks like the patch below is sufficient to fix the issue. > > Have no idea at the moment why I used u32 cast there. > > Let me think a bit more about it and I'll submit an official patch. > > Yeap, I see no drawbacks in that patch, imm field is declared as s32, > so it would be correctly sign extended by compiler before cast to u64, > so there is no need for additional casts. > It would be wrong if I submit the fix, because you've done all the work. Done. Besides, users or binaries with CAP_BPF can exploit this bug. > Here is a refined test-case to be placed in verifier/bpf_st_mem.c > (be careful with \t, test_verifier uses those as glob marks inside errstr). > > { > "BPF_ST_MEM stack imm sign", > /* Check if verifier correctly reasons about sign of an > * immediate spilled to stack by BPF_ST instruction. > * > * fp[-8] = -44; > * r0 = fp[-8]; > * if r0 s< 0 goto ret0; > * r0 = -1; > * exit; > * ret0: > * r0 = 0; > * exit; > */ > .insns = { > BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, -44), > BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_10, -8), > BPF_JMP_IMM(BPF_JSLT, BPF_REG_0, 0, 2), > BPF_MOV64_IMM(BPF_REG_0, -1), > BPF_EXIT_INSN(), > BPF_MOV64_IMM(BPF_REG_0, 0), > BPF_EXIT_INSN(), > }, > /* Use prog type that requires return value in range [0, 1] */ > .prog_type = BPF_PROG_TYPE_SK_LOOKUP, > .expected_attach_type = BPF_SK_LOOKUP, > .result = VERBOSE_ACCEPT, > .runs = -1, > .errstr = "0: (7a) *(u64 *)(r10 -8) = -44 ; R10=fp0 fp-8_w=-44\ > 2: (c5) if r0 s< 0x0 goto pc+2\ > 2: R0_w=-44", > },