netdev - Re: Self-XORing BPF registers is undefined behavior

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 27 May 2020 19:12:45 +0200
From:   Alexander Potapenko <glider@...gle.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Michal Kubecek <mkubecek@...e.cz>,
        Alexei Starovoitov <ast@...nel.org>,
        Dmitriy Vyukov <dvyukov@...gle.com>,
        Networking <netdev@...r.kernel.org>
Subject: Re: Self-XORing BPF registers is undefined behavior

On Wed, May 27, 2020 at 6:58 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Wed, May 27, 2020 at 8:52 AM Alexander Potapenko <glider@...gle.com> wrote:
> >
> > This basically means that BPF's output register was uninitialized when
> > ___bpf_prog_run() returned.
> >
> > When I replace the lines initializing registers A and X in net/core/filter.c:
> >
> > -               *new_insn++ = BPF_ALU32_REG(BPF_XOR, BPF_REG_A, BPF_REG_A);
> > -               *new_insn++ = BPF_ALU32_REG(BPF_XOR, BPF_REG_X, BPF_REG_X);
> >
> > with
> >
> > +               *new_insn++ = BPF_MOV32_IMM(BPF_REG_A, 0);
> > +               *new_insn++ = BPF_MOV32_IMM(BPF_REG_X, 0);
> >
> > , the bug goes away, therefore I think it's being caused by XORing the
> > initially uninitialized registers with themselves.
> >
> > kernel/bpf/core.c:1408, where the uninitialized value was stored to
> > memory, points to the "ALU(ADD,  +)" macro in ___bpf_prog_run().
> > But the debug info seems to be incorrect here: if I comment this line
> > out and unroll the macro manually, KMSAN points to "ALU(SUB,  -)".
> > Most certainly it's actually one of the XOR instruction declarations.
> >
> > Do you think it makes sense to use the UB-proof BPF_MOV32_IMM
> > instructions to initialize the registers?
>
> I think it's better for UBsan to get smarter about xor-ing.

Could you please elaborate on this? How exactly should KMSAN handle
this situation?
Note that despite the source says "BPF_ALU32_REG(BPF_XOR, BPF_REG_A,
BPF_REG_A);", it doesn't necessarily boil down to an expression like A
= A ^ A. It's more likely that temporary values will be involved,
making it quite hard to figure out whether the two operands are really
the same.
For an expression like A = B ^ C, KMSAN just calculates A's shadow
bits based on the values and the shadow bits of B and C. If either B
or C is uninitialized, the result will be uninitialized as well, even
if both B and C contain the same values. It's therefore strange to
expect the tool to behave differently if B and C are the same
variable.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg