netdev - Re: [PATCH bpf v2] bpf: mark dst unknown on inconsistent {s,u}bounds adjustments

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]

Date:   Wed, 17 Jan 2018 16:33:12 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     ast@...com, ecree@...arflare.com, netdev@...r.kernel.org
Subject: Re: [PATCH bpf v2] bpf: mark dst unknown on inconsistent {s,u}bounds
 adjustments

On Thu, Jan 18, 2018 at 01:15:21AM +0100, Daniel Borkmann wrote:
> syzkaller generated a BPF proglet and triggered a warning with
> the following:
> 
>   0: (b7) r0 = 0
>   1: (d5) if r0 s<= 0x0 goto pc+0
>    R0=inv0 R1=ctx(id=0,off=0,imm=0) R10=fp0
>   2: (1f) r0 -= r1
>    R0=inv0 R1=ctx(id=0,off=0,imm=0) R10=fp0
>   verifier internal error: known but bad sbounds
> 
> What happens is that in the first insn, r0's min/max value
> are both 0 due to the immediate assignment, later in the jsle
> test the bounds are updated for the min value in the false
> path, meaning, they yield smin_val = 1, smax_val = 0, and when
> ctx pointer is subtracted from r0, verifier bails out with the
> internal error and throwing a WARN since smin_val != smax_val
> for the known constant.
> 
> For min_val > max_val scenario it means that reg_set_min_max()
> and reg_set_min_max_inv() (which both refine existing bounds)
> demonstrated that such branch cannot be taken at runtime.
> 
> In above scenario for the case where it will be taken, the
> existing [0, 0] bounds are kept intact. Meaning, the rejection
> is not due to a verifier internal error, and therefore the
> WARN() is not necessary either.
> 
> We could just reject such cases in adjust_{ptr,scalar}_min_max_vals()
> when either known scalars have smin_val != smax_val or
> umin_val != umax_val or any scalar reg with bounds
> smin_val > smax_val or umin_val > umax_val. However, there
> may be a small risk of breakage of buggy programs, so handle
> this more gracefully and in adjust_{ptr,scalar}_min_max_vals()
> just taint the dst reg as unknown scalar when we see ops with
> such kind of src reg.
> 
> Reported-by: syzbot+6d362cadd45dc0a12ba4@...kaller.appspotmail.com
> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>

there are several ways to fix it and this approach looks
to be the safest way to do it since we're so late into the release.
Applied, Thank you Daniel.