netdev - Re: [PATCH net-next] bpf/verifier: improve disassembly of BPF

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:   Sat, 23 Sep 2017 22:50:18 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Y Song <ys114321@...il.com>
Cc:     Edward Cree <ecree@...arflare.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Jiong Wang <jiong.wang@...ronome.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>
Subject: Re: [PATCH net-next] bpf/verifier: improve disassembly of BPF_END
 instructions

On Fri, Sep 22, 2017 at 09:49:10PM -0700, Y Song wrote:
> On Fri, Sep 22, 2017 at 9:23 AM, Edward Cree <ecree@...arflare.com> wrote:
> > On 22/09/17 16:16, Alexei Starovoitov wrote:
> >> looks like we're converging on
> >> "be16/be32/be64/le16/le32/le64 #register" for BPF_END.
> >> I guess it can live with that. I would prefer more C like syntax
> >> to match the rest, but llvm parsing point is a strong one.
> > Yep, agreed.  I'll post a v2 once we've settled BPF_NEG.
> >> For BPG_NEG I prefer to do it in C syntax like interpreter does:
> >>         ALU_NEG:
> >>                 DST = (u32) -DST;
> >>         ALU64_NEG:
> >>                 DST = -DST;
> >> Yonghong, does it mean that asmparser will equally suffer?
> > Correction to my earlier statements: verifier will currently disassemble
> >  neg as:
> > (87) r0 neg 0
> > (84) (u32) r0 neg (u32) 0
> >  because it pretends 'neg' is a compound-assignment operator like +=.
> > The analogy with be16 and friends would be to use
> >     neg64 r0
> >     neg32 r0
> >  whereas the analogy with everything else would be
> >     r0 = -r0
> >     r0 = (u32) -r0
> >  as Alexei says.
> > I'm happy to go with Alexei's version if it doesn't cause problems for llvm.
> 
> I got some time to do some prototyping in llvm and it looks like that
> I am able to
> resolve the issue and we are able to use more C-like syntax. That is:
> for bswap:
>      r1 = (be16) (u16) r1
>      or
>      r1 = (be16) r1
>      or
>      r1 = be16 r1
> for neg:
>      r0 = -r0
>      (for 32bit support, llvm may output "w0 = -w0" in the future. But
> since it is not
>       enabled yet, you can continue to output "r0 = (u32) -r0".)
> 
> Not sure which syntax is best for bswap. The "r1 = (be16) (u16) r1" is most
> explicit in its intention.
> 
> Attaching my llvm patch as well and cc'ing Jiong and Jakub so they can see my
> implementation and the relative discussion here. (In this patch, I did
> not implement
> bswap for little endian yet.) Maybe they can provide additional comments.

This is awesome. In such case I'd like to swing back to the C syntax for bpf_end :)
Any of these
  r1 = (be16) (u16) r1
  or
  r1 = (be16) r1
  or
  r1 = be16 r1
are better than just
  be16 r1
I like 1st the most, since it's explicit in terms of what happens with upper bits,
but 2nd is also ok. 3rd is not quite C-like.