On Wed, May 27, 2020 at 6:58 PM Alexei Starovoitov <alexei.starovoi...@gmail.com> wrote: > > On Wed, May 27, 2020 at 8:52 AM Alexander Potapenko <gli...@google.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
