From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755079AbcH0U4L (ORCPT ); Sat, 27 Aug 2016 16:56:11 -0400 Received: from mail-pf0-f180.google.com ([209.85.192.180]:36535 "EHLO mail-pf0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752002AbcH0U4H (ORCPT ); Sat, 27 Aug 2016 16:56:07 -0400 Date: Sat, 27 Aug 2016 13:56:01 -0700 From: Alexei Starovoitov To: =?iso-8859-1?Q?Micka=EBl_Sala=FCn?= Cc: linux-kernel@vger.kernel.org, Alexei Starovoitov , Andy Lutomirski , Daniel Borkmann , Daniel Mack , "David S . Miller" , Kees Cook , Sargun Dhillon , kernel-hardening@lists.openwall.com, linux-api@vger.kernel.org, linux-security-module@vger.kernel.org, netdev@vger.kernel.org, Tejun Heo Subject: Re: [RFC v2 09/10] landlock: Handle cgroups (program types) Message-ID: <20160827205559.GA43880@ast-mbp.thefacebook.com> References: <1472121165-29071-1-git-send-email-mic@digikod.net> <1472121165-29071-10-git-send-email-mic@digikod.net> <20160826021432.GA8291@ast-mbp.thefacebook.com> <57C05BF0.8000706@digikod.net> <20160826230539.GA26683@ast-mbp.thefacebook.com> <57C1A50F.60605@digikod.net> <20160827181939.GC38754@ast-mbp.thefacebook.com> <57C1F015.1000301@digikod.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <57C1F015.1000301@digikod.net> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Aug 27, 2016 at 09:55:01PM +0200, Mickaël Salaün wrote: > > On 27/08/2016 20:19, Alexei Starovoitov wrote: > > On Sat, Aug 27, 2016 at 04:34:55PM +0200, Mickaël Salaün wrote: > >> > >> On 27/08/2016 01:05, Alexei Starovoitov wrote: > >>> On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote: > >>> > >>>>> As far as safety and type checking that bpf programs has to do, > >>>>> I like the approach of patch 06/10: > >>>>> +LANDLOCK_HOOK2(file_open, FILE_OPEN, > >>>>> + PTR_TO_STRUCT_FILE, struct file *, file, > >>>>> + PTR_TO_STRUCT_CRED, const struct cred *, cred > >>>>> +) > >>>>> teaching verifier to recognize struct file, cred, sockaddr > >>>>> will let bpf program access them naturally without any overhead. > >>>>> Though: > >>>>> @@ -102,6 +102,9 @@ enum bpf_prog_type { > >>>>> BPF_PROG_TYPE_SCHED_CLS, > >>>>> BPF_PROG_TYPE_SCHED_ACT, > >>>>> BPF_PROG_TYPE_TRACEPOINT, > >>>>> + BPF_PROG_TYPE_LANDLOCK_FILE_OPEN, > >>>>> + BPF_PROG_TYPE_LANDLOCK_FILE_PERMISSION, > >>>>> + BPF_PROG_TYPE_LANDLOCK_MMAP_FILE, > >>>>> }; > >>>>> is a bit of overkill. > >>>>> I think it would be cleaner to have single > >>>>> BPF_PROG_TYPE_LSM and at program load time pass > >>>>> lsm_hook_id as well, so that verifier can do safety checks > >>>>> based on type info provided in LANDLOCK_HOOKs > >>>> > >>>> I first started with a unique BPF_PROG_TYPE but, the thing is, the BPF > >>>> verifier check programs according to their types. If we need to check > >>>> specific context value types (e.g. PTR_TO_STRUCT_FILE), we need a > >>>> dedicated program types. I don't see any other way to do it with the > >>>> current verifier code. Moreover it's the purpose of program types, right? > >>> > >>> Adding new bpf program type for every lsm hook is not acceptable. > >>> Either do one new program type + pass lsm_hook_id as suggested > >>> or please come up with an alternative approach. > >> > >> OK, so we have to modify the verifier to not only rely on the program > >> type but on another value to check the context accesses. Do you have a > >> hint from where this value could come from? Do we need to add a new bpf > >> command to associate a program to a subtype? > > > > It's another field prog_subtype (or prog_hook_id) in union bpf_attr. > > Both prog_type and prog_hook_id are used during verification. > > prog_type distinguishes the main aspects whereas prog_hook_id selects > > which lsm_hook's argument definition to apply. > > At the time of attaching to a hook, the prog_hook_id passed at the > > load time should match lsm's hook_id. > > OK, so this new prog_subtype field should be use/set by a new bpf_cmd, > right? Something like BPF_PROG_SUBTYPE or BPF_PROG_METADATA? No new command. It will be an optional field to existing BPF_PROG_LOAD. In other words instead of replicating everything that different bpf_prog_type-s need, we can pass this hook_id field to fine tune the purpose (and applicability) of the program. And one BPF_PROG_TYPE_LANDLOCK type will cover all lsm hooks. For example existing BPF_PROG_TYPE_TRACEPOINT checks the safety for all tracepoints while they all have different arguments. For tracepoints it's easier, since the only difference between them is the range of ctx access. Here we need strong type safety of arguments therefore need extra hook_id at load time.