Re: bpf: incorrect value spill in check_stack_write_fixed_off()

[Hao Sun]

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