From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754722AbbIIQHw (ORCPT ); Wed, 9 Sep 2015 12:07:52 -0400 Received: from mail-pa0-f45.google.com ([209.85.220.45]:34068 "EHLO mail-pa0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753606AbbIIQHs (ORCPT ); Wed, 9 Sep 2015 12:07:48 -0400 Date: Wed, 9 Sep 2015 09:07:45 -0700 From: Alexei Starovoitov To: Tycho Andersen Cc: Kees Cook , Alexei Starovoitov , Will Drewry , Oleg Nesterov , Andy Lutomirski , Pavel Emelyanov , "Serge E. Hallyn" , Daniel Borkmann , LKML , Network Development Subject: Re: [PATCH 1/6] ebpf: add a seccomp program type Message-ID: <20150909160744.GA3526@Alexeis-MBP-2.westell.com> References: <1441382664-17437-1-git-send-email-tycho.andersen@canonical.com> <1441382664-17437-2-git-send-email-tycho.andersen@canonical.com> <20150904210615.GR26679@smitten> <20150909155035.GA26679@smitten> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150909155035.GA26679@smitten> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Sep 09, 2015 at 09:50:35AM -0600, Tycho Andersen wrote: > > > > > > 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: > > 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? Please do not add any per-instruction hacks. None of them are necessary. Classic had to do extra ugly checks in seccomp only because verifier wasn't flexible enough. If you don't want to see any BPF_CALL in seccomp, just have empty get_func_proto() callback for BPF_PROG_TYPE_SECCOMP and verifier will reject all calls. Currently we have only two non-generic instrucitons LD_ABS and LD_IND that are avaialable for sockets/TC only, because these are legacy instructions and we had to make exceptions for them.