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

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

Date:   Fri, 22 Sep 2017 21:49:10 -0700
From:   Y Song <ys114321@...il.com>
To:     Edward Cree <ecree@...arflare.com>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.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 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.

Download attachment "0001-bpf-add-support-for-neg-insn-and-change-format-of-bs.patch" of type "application/octet-stream" (8464 bytes)