linux-kernel - Re: [RFC PATCH net-next 3/5] bpf/verifier: feed pointer-to-unknown-scalar casts into scalar ALU path

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]

Date:   Thu, 8 Jun 2017 11:41:09 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Edward Cree <ecree@...arflare.com>
Cc:     davem@...emloft.net, Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        iovisor-dev <iovisor-dev@...ts.iovisor.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH net-next 3/5] bpf/verifier: feed
 pointer-to-unknown-scalar casts into scalar ALU path

On Thu, Jun 08, 2017 at 06:12:39PM +0100, Edward Cree wrote:
> On 08/06/17 17:50, Alexei Starovoitov wrote:
> > On Thu, Jun 08, 2017 at 04:25:39PM +0100, Edward Cree wrote:
> >> On 08/06/17 03:35, Alexei Starovoitov wrote:
> >>> such large back and forth move doesn't help reviewing.
> >>> may be just merge it into previous patch?
> >>> Or keep that function in the right place in patch 2 already?
> >> I think 'diff' got a bit confused, and maybe with different options I could
> >>  have got it to produce something more readable.  But I think I will just
> >>  merge this into patch 2; it's only separate because it started out as an
> >>  experiment.
> > after sleeping on it I'm not sure we should be allowing such pointer
> > arithmetic. In normal C code people do fancy tricks with lower 3 bits
> > of the pointer, but in bpf code I cannot see such use case.
> > What kind of realistic code will be doing ptr & 0x40 ?
> Well, I didn't support it because I saw a use case.  I supported it because
>  it seemed easy to do and the code came out reasonably elegant-looking.
> Since this is guarded by env->allow_ptr_leaks, I can't see any reason _not_
>  to let people try fancy tricks with the low bits of pointers.
> I agree ptr & 0x40 is a crazy thing with no imaginable use case, but...
> "Unix was not designed to stop its users from doing stupid things, as that
>  would also stop them from doing clever things." ;-)

well, I agree with the philosophy :) but I also see few reasons not to allow it:
1. it immediately becomes uapi and if later we find out that it's preventing us
to do something we actually really need we'll be stuck looking for workaround
2. it's the same pruning concern. probably doesn't fully apply here, but
the reason we don't track 'if (reg == 1) ...' is if we mark that
register as known const_imm in the true branch, it will screw up
pruning quite badly. It's trivial to track and may seem useful,
but hurts instead.