linux-kernel - Re: bpf: incorrect value spill in check_stack_write_fixed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CACkBjsb3mxW4FJx2U9_jWFZFogNxXtBcd9PpeFphHGVk7vRhTA@mail.gmail.com>
Date:   Thu, 26 Oct 2023 17:23:06 +0200
From:   Hao Sun <sunhao.th@...il.com>
To:     Eduard Zingerman <eddyz87@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>,
        Yonghong Song <yonghong.song@...ux.dev>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
        bpf <bpf@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: bpf: incorrect value spill in check_stack_write_fixed_off()

On Wed, Oct 25, 2023 at 2:48 PM Eduard Zingerman <eddyz87@...il.com> 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",
> },