netdev - Re: [PATCH v7 bpf-next 01/16] bpf: verifier: mark verified-insn with sub-register zext flag

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] [day] [month] [year] [list]

Date:   Thu, 23 May 2019 13:42:43 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Jiong Wang <jiong.wang@...ronome.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, oss-drivers@...ronome.com,
        davem@...emloft.net, paul.burton@...s.com, udknight@...il.com,
        zlim.lnx@...il.com, illusionist.neo@...il.com,
        naveen.n.rao@...ux.ibm.com, sandipan@...ux.ibm.com,
        schwidefsky@...ibm.com, heiko.carstens@...ibm.com,
        jakub.kicinski@...ronome.com
Subject: Re: [PATCH v7 bpf-next 01/16] bpf: verifier: mark verified-insn with
 sub-register zext flag

On Thu, May 23, 2019 at 09:20:15PM +0100, Jiong Wang wrote:
> 
> Alexei Starovoitov writes:
> 
> <snip>
> 
> > well, it made me realize that we're probably doing it wrong,
> > since after calling check_reg_arg() we need to re-parse insn encoding.
> > How about we change check_reg_arg()'s enum reg_arg_type instead?
> 
> This is exactly what I had implemented in my initial internal version.

it was long ago :) shrinkers purged it.

> > The caller has much more context and no need to parse insn opcode again.
> 
> And I had exactly the same thoughts, is_reg64 is duplicating what has been
> done.
> 
> The code evolved into the current shape mostly because I agreed if we
> re-centre all checks inside check_reg_arg, then we don't need to touch
> quite a few call sites of check_reg_arg, the change/patch looks simpler,
> and I was thinking is_reg64 is a quick check, so the overhead is not big.
> 
> > Something like:
> > enum reg_arg_type {
> >         SRC_OP64,        
> >         DST_OP64,       
> >         DST_OP_NO_MARK, // probably no need to split this one ?
> >         SRC_OP32,      
> >         DST_OP32,      
> > };
> >
> 
> Yeah, no need to split DST_OP_NO_MARK, my split was
> 
> enum reg_arg_type {
>    SRC_OP,
> +  SRC32_OP,
>    DST_OP,
> +  DST32_OP,
>    DST_OP_NO_MARK 
> }
> 
> No renaming on existing SRC/DST_OP, they mean 64-bit, the changes are
> smaller, looks better?
> 
> But, we also need to know whether one patch-insn define sub-register, if it
> is, we then conservatively mark it as needing zero extension. patch-insn
> doesn't go through check_reg_arg analysis, so still need separate insn
> parsing.

Good point.
Then let's stick with your last is_reg64().
Re-parsing is annoying, but looks like there is already use case for it
and more can appear in the future.