linux-kernel - Re: [PATCH 1/6] ebpf: add a seccomp program type

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:	Wed, 09 Sep 2015 18:07:57 +0200
From:	Daniel Borkmann <daniel@...earbox.net>
To:	Tycho Andersen <tycho.andersen@...onical.com>,
	Kees Cook <keescook@...omium.org>
CC:	Alexei Starovoitov <ast@...nel.org>,
	Will Drewry <wad@...omium.org>,
	Oleg Nesterov <oleg@...hat.com>,
	Andy Lutomirski <luto@...capital.net>,
	Pavel Emelyanov <xemul@...allels.com>,
	"Serge E. Hallyn" <serge.hallyn@...ntu.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH 1/6] ebpf: add a seccomp program type

On 09/09/2015 05:50 PM, Tycho Andersen wrote:
> On Fri, Sep 04, 2015 at 02:08:37PM -0700, Kees Cook wrote:
>> On Fri, Sep 4, 2015 at 2:06 PM, Tycho Andersen
[...]
>>>> I was expecting to see a validator, similar to the existing BPF
>>>> validator that is called when creating seccomp filters currently. Can
>>>> we add a similar validator for new BPF_PROG_TYPE_SECCOMP?
>>>
>>> That's effectively what this patch does; when the eBPF is loaded via
>>> bpf(), you tell bpf() you want a BPF_PROG_TYPE_SECCOMP, and it invokes
>>> this validation/translation code, i.e. it uses
>>> seccomp_is_valid_access() to check and make sure access are aligned
>>> and inside struct seccomp_data.
>>
>> What about limiting the possible instructions?
>
> I totally overlooked this. A quick glance through the eBPF verifier
> makes me think that we can just add another function to struct
> bpf_verifier_ops called valid_instruction, which shouldn't be too
> hard. Perhaps a more interesting question is what to allow:

It's possible, but keep in mind that when you disallow various
instructions from the base insns set, you won't be able to leverage
filter creation in the minimal C subset via clang/llvm anymore, so
usability would suffer from this side, even if you just use clang/llvm
to create the raw insns and later keep them in your application
directly.

And if you later on decide to allow maps, etc, hacking this together
by hand is a bit of a pain. ;)

[ Restricting helper functions and ctx access, etc via bpf_verifier_ops
   (as you can currently do) should not affect this. ]

> BPF_LD(X) and BPF_ST(X): it looks like all types of stores are
>    allowed, and only BPF_MEM and BPF_IMM loads are allowed; I think
>    these can stay the same. BPF_XADD is new in eBPF, and I don't think
>    we need it for seccomp (yet), since we don't have any shared memory
>    via maps.
>
> BPF_ALU: It looks like we're also not allowing regular BPF_ALU
>    instruction BPF_MOD; eBPF adds a few ones: BPF_MOV (register move),
>    BPF_ARSH (sign extended right shift), and BPF_END (endianness
>    conversion), wich I think should all be safe. In particular, we need
>    to allow BPF_MOV at least, since that's how the converter implements
>    BPF_MISC | BPF_TAX from classic.
>
> BPF_ALU64: I think we can safely allow all these as above, since
>    they're just the 64-bit versions.
>
> BPF_JMP: eBPF adds BPF_JNE, BPF_JSGT, BPF_JSGE, BPF_CALL, and
>    BPF_EXIT, which I think all should be safe (except maybe BPF_CALL
>    since we're not allowing functions really). Again we have to allow
>    one of the new eBPF codes, as the converter implements BPF_RET as
>    BPF_JMP | BPF_EXIT.
>
> Thoughts?
>
> Tycho

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/