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:   Fri, 22 Sep 2017 07:27:29 -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>
Subject: Re: [PATCH net-next] bpf/verifier: improve disassembly of BPF_END instructions

On Fri, Sep 22, 2017 at 7:11 AM, Y Song <ys114321@...il.com> wrote:
> On Fri, Sep 22, 2017 at 6:46 AM, Edward Cree <ecree@...arflare.com> wrote:
>> On 22/09/17 00:11, Y Song wrote:
>>> On Thu, Sep 21, 2017 at 12:58 PM, Edward Cree <ecree@...arflare.com> wrote:
>>>> On 21/09/17 20:44, Alexei Starovoitov wrote:
>>>>> On Thu, Sep 21, 2017 at 09:29:33PM +0200, Daniel Borkmann wrote:
>>>>>> More intuitive, but agree on the from_be/le. Maybe we should
>>>>>> just drop the "to_" prefix altogether, and leave the rest as is since
>>>>>> it's not surrounded by braces, it's also not a cast but rather an op.
>>>> That works for me.
>>>>> 'be16 r4' is ambiguous regarding upper bits.
>>>>>
>>>>> what about my earlier suggestion:
>>>>> r4 = (be16) (u16) r4
>>>>> r4 = (le64) (u64) r4
>>>>>
>>>>> It will be pretty clear what instruction is doing (that upper bits become zero).
>>>> Trouble with that is that's very *not* what C will do with those casts
>>>>  and it doesn't really capture the bidirectional/symmetry thing.  The
>>>>  closest I could see with that is something like `r4 = (be16/u16) r4`,
>>>>  but that's quite an ugly mongrel.
>>>> I think Daniel's idea of `be16`, `le32` etc one-arg opcodes is the
>>>>  cleanest and clearest.  Should it be
>>>>     r4 = be16 r4
>>>>  or just
>>>>     be16 r4
>>>> ?  Personally I incline towards the latter, but admit it doesn't really
>>>>  match the syntax of other opcodes.
>>> I did some quick prototyping in llvm to make sure we have a syntax
>>> llvm is happy. Apparently, llvm does not like the syntax
>>>    r4 = be16 r4    or    r4 = (be16) (u16) r4.
>>>
>>> In llvm:utils/TableGen/AsmMatcherEmitter.cpp:
>>>
>>>     // Verify that any operand is only mentioned once.
>> Wait, how do you deal with (totally legal) r4 += r4?
>> Or r4 = *(r4 +0)?
>> Even jumps can have src_reg == dst_reg, though it doesn't seem useful.
>
> We are talking about dag node here. The above "r4", although using the same
> register, will be different dag nodes. So it will be okay.
>
> The "r4 = be16 r4" tries to use the *same* dag node as both source and
> destination
> in the asm output which is prohibited.

With second thought, we may allow "r4 = be16 r4" by using different dag nodes.
(I need to do experiment for this.) But we do have constraints that
the two "r4" must
be the same register.  "r5 = be16 r4"  is not allowed. So from that
perspective, referencing
"r4" only once is a good idea and less confusing.