netdev - RE: [PATCH v2 bpf-next 1/4] bpf: Propagate scalar ranges through register assignments.

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]

Message-ID: <5f80bd081008f_ed742083e@john-XPS-13-9370.notmuch>
Date:   Fri, 09 Oct 2020 12:42:00 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        davem@...emloft.net
Cc:     daniel@...earbox.net, john.fastabend@...il.com,
        netdev@...r.kernel.org, bpf@...r.kernel.org, kernel-team@...com
Subject: RE: [PATCH v2 bpf-next 1/4] bpf: Propagate scalar ranges through
 register assignments.

Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@...nel.org>
> 
> 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@...nel.org>
> Acked-by: Andrii Nakryiko <andrii@...nel.org>
> ---
>  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@...il.com>