netdev - Re: [PATCH bpf] bpf: do not modify min/max bounds on scalars with constant values

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 13 Jan 2018 02:01:18 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Edward Cree <ecree@...arflare.com>, ast@...com
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH bpf] bpf: do not modify min/max bounds on scalars with
 constant values

On 01/12/2018 08:52 PM, Edward Cree wrote:
> On 12/01/18 19:23, 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,
> reg_set_min_max() refines the existing bounds, it doesn't replace
>  them, so all that's happened is that the jsle handling has
>  demonstrated that this branch can't be taken.

Correct, on the one hand one could argue that for known constants
it cannot get any more refined than this already since we know
the precise constant value the register holds (thus we make the
range worse in this case fwiw), on the other hand why should they
be treated any different than registers with unknown scalars.
True that smin > smax is a result that the branch won't be taken
at runtime.

> That AFAICT isn't confined to known constants, one could e.g.
>  obtain inconsistent bounds with two js* insns.  Updating the
>  bounds in reg_set_min_max() is right, it's where we try to use
>  those sbounds in adjust_ptr_min_max_vals() that's wrong imho;
>  instead the 'known' paths should be using off_reg->var_off.value
>  rather than smin_val everywhere.

Ok, I'll check further on the side-effects and evaluate this
option of using var_off.value for a v2 as well, thanks!

> Alternatively we could consider not following jumps/lack-thereof
>  that produce inconsistent bounds, but that can make insns
>  unreachable that previously weren't and thus reject programs
>  that we previously considered valid, so we probably can't get
>  away with that.

Agree.

Thanks,
Daniel