netdev - Re: [RFCv2 07/16] bpf: enable non-core use of the verfier

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]

Message-ID: <57C5D986.2000402@iogearbox.net>
Date:   Tue, 30 Aug 2016 21:07:50 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jakub Kicinski <kubakici@...pl>
CC:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        netdev@...r.kernel.org, ast@...nel.org,
        dinan.gunawardena@...ronome.com, jiri@...nulli.us,
        john.fastabend@...il.com
Subject: Re: [RFCv2 07/16] bpf: enable non-core use of the verfier

On 08/30/2016 12:48 PM, Jakub Kicinski wrote:
> On Mon, 29 Aug 2016 22:17:10 +0200, Daniel Borkmann wrote:
>> On 08/29/2016 10:13 PM, Daniel Borkmann wrote:
>>> On 08/27/2016 07:32 PM, Alexei Starovoitov wrote:
>>>> On Sat, Aug 27, 2016 at 12:40:04PM +0100, Jakub Kicinski wrote:
>>>> probably array_of_insn_aux_data[num_insns] should do it.
>>>> Unlike reg_state that is forked on branches, this array
>>>> is only one.
>>>
>>> This would be for struct nfp_insn_meta, right? So, struct
>>> bpf_ext_parser_ops could become:
>>>
>>> static const struct bpf_ext_parser_ops nfp_bpf_pops = {
>>>       .insn_hook = nfp_verify_insn,
>>>       .insn_size = sizeof(struct nfp_insn_meta),
>>> };
>>>
>>> ... where bpf_parse() would prealloc that f.e. in env->insn_meta[].
>
> Hm.. this is tempting, I will have to store the pointer type in
> nfp_insn_meta soon, anyway.
>
>> (Well, actually everything can live in env->private_data.)
>
> We are discussing changing the place verifier keep its pointer type
> annotation, I don't think we could put that in the private_data.
>
>>> Agree, was also my concern when I read patch 5 and 6. It would
>>> not only be related to types, but also different imm values,
>>> where the memcmp() could fail on. Potentially the latter can be
>>> avoided by only checking types which should be sufficient. Hmm,
>>> maybe only bpf_parse() should go through this stricter mode since
>>> only relevant for drivers (otoh downside would be that bugs
>>> would end up less likely to be found).
>
> I don't want only checking types because it would defeat my exit code
> validation :)  I was thinking about doing a lazy evaluation -
> registering branches to explored_states with UNKNOWN and only upgrading
> to CONST when someone actually needed the imm value.  I'm not sure the
> complexity would be justified, though.
>
> Having two modes seems more straight forward and I think we would only
> need to pay attention in the LD_IMM64 case, I don't think I've seen
> LLVM generating XORs, it's just the cBPF -> eBPF conversion.

Okay, though, I think that the cBPF to eBPF migration wouldn't even
pass through the bpf_parse() handling, since verifier is not aware on
some of their aspects such as emitting calls directly (w/o *proto) or
arg mappings. Probably make sense to reject these (bpf_prog_was_classic())
if they cannot be handled anyway?

>>>> I see. Indeed then you'd need the verifier to walk all paths
>>>> to make sure constant return values.
>>>
>>> I think this would still not cover the cases where you'd fetch
>>> a return value/verdict from a map, but this should be ignored/
>>> rejected for now, also since majority of programs are not written
>>> in such a way.
>>>
>>>> If you only need yes/no check then such info can probably be
>>>> collected unconditionally during initial program load.
>>>> Like prog->cb_access flag.
>>>
>>> One other comment wrt the header, when you move these things
>>> there, would be good to prefix with bpf_* so that this doesn't
>>> clash in future with other header files.
>
> Good point!