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",
> },
