Alexei Starovoitov wrote: > From: Alexei Starovoitov <ast@xxxxxxxxxx> > > The llvm register allocator may use two different registers representing the > same virtual register. In such case the following pattern can be observed: > 1047: (bf) r9 = r6 > 1048: (a5) if r6 < 0x1000 goto pc+1 > 1050: ... > 1051: (a5) if r9 < 0x2 goto pc+66 > 1052: ... > 1053: (bf) r2 = r9 /* r2 needs to have upper and lower bounds */ > > This is normal behavior of greedy register allocator. > The slides 137+ explain why regalloc introduces such register copy: > http://llvm.org/devmtg/2018-04/slides/Yatsina-LLVM%20Greedy%20Register%20Allocator.pdf > There is no way to tell llvm 'not to do this'. > Hence the verifier has to recognize such patterns. > > In order to track this information without backtracking allocate ID > for scalars in a similar way as it's done for find_good_pkt_pointers(). > > When the verifier encounters r9 = r6 assignment it will assign the same ID > to both registers. Later if either register range is narrowed via conditional > jump propagate the register state into the other register. > > Clear register ID in adjust_reg_min_max_vals() for any alu instruction. The > register ID is ignored for scalars in regsafe() and doesn't affect state > pruning. mark_reg_unknown() clears the ID. It's used to process call, endian > and other instructions. Hence ID is explicitly cleared only in > adjust_reg_min_max_vals() and in 32-bit mov. > > Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx> > Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > --- > kernel/bpf/verifier.c | 50 +++++++++++++++++++ > .../testing/selftests/bpf/prog_tests/align.c | 16 +++--- > .../bpf/verifier/direct_packet_access.c | 2 +- > 3 files changed, 59 insertions(+), 9 deletions(-) > I also walked through the stack read case and I don't see any issues there either. We will run check_mem_access on the ld with src_reg->type set to PTR_TO_STACK. So we run through check_stack_read() with a value_regno set to the dst_reg (>=0). The case I was thinking of was if we had a size != BPF_REG_SIZE. But, this case is handled by marking the dst reg unknown and never copying over the stack[spi]. If size == BPF_REG_SIZE its a full read and we also ensure all stype[] are STACK_SPILL. Then everything looks safe to do state->regs[value_reno] = *reg e.g. assign register state from stack complete with reg.id. Looking the other way at stack writes. We only allow SCALAR type bounds to be saved on the stack if they are constant and size = BPF_REG_SIZE so no problems there. LGTM! Acked-by: John Fastabend <john.fastabend@xxxxxxxxx>
