From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757962AbdDRXQ2 (ORCPT ); Tue, 18 Apr 2017 19:16:28 -0400 Received: from nm1-vm2.bullet.mail.ne1.yahoo.com ([98.138.91.17]:36628 "EHLO nm1-vm2.bullet.mail.ne1.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752357AbdDRXQ0 (ORCPT ); Tue, 18 Apr 2017 19:16:26 -0400 X-Yahoo-Newman-Id: 827447.10993.bm@smtp205.mail.ne1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: yI357GsVM1lsg7JpXc6VdA8HOUS7mFk1Q3U3OSfF2EVJ06u uiq7toT2ADyTMyrugMSpj3y6Ue2WoqTmcdI5jiCQ.ZXYZETAecMc70c69Upw nC6FMFOyBG6nHJJvTD8pZ7l2mEHWaw35aRxsug210xijtSmFooZrGpDIRxu7 DYjlbc0r5V9E0TaUd1N0O0y6XkYzZsCwRwd2rKjTh4twTllcAjSM0Qm5ISPD VRB50xwngHCTOc.wQeOhIk7Bg__q5rox2SEg.9t3KVhYhrIbtN135dBLIGbc XHu7GbPGxUTmgIC8Y_Pu2sVj9oIhXkuLLfmFbgrbPdAUxSJNQtZfu3qu9CwV iEk4QeBBynGdOSnDgsxbXmMGcHMX5Dh1diH8rTLuec6YK.dfcORNtUwoOPB4 wBCt.OsWlaB.R54z9_qG6RTTlvel7vYBknMdHTz17i.LYizdDs.VA_8QX_HP Z1mbrXq6ZegsuOdhg8yCyrk9XHAs9Amgv7WyqKffcpqD54xLUoa8jfjBkyKP uiJk5x4bo.kZk X-Yahoo-SMTP: OIJXglSswBDfgLtXluJ6wiAYv6_cnw-- Subject: Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem To: =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= , Kees Cook References: <20170328234650.19695-1-mic@digikod.net> <20170328234650.19695-5-mic@digikod.net> <9a69055a-b4cf-00b0-da5e-2e45ff88059c@digikod.net> Cc: LKML , Alexei Starovoitov , Andy Lutomirski , Arnaldo Carvalho de Melo , Daniel Borkmann , David Drysdale , "David S . Miller" , "Eric W . Biederman" , James Morris , Jann Horn , Jonathan Corbet , Matthew Garrett , Michael Kerrisk , Paul Moore , Sargun Dhillon , "Serge E . Hallyn" , Shuah Khan , Tejun Heo , Thomas Graf , Will Drewry , "kernel-hardening@lists.openwall.com" , Linux API , linux-security-module , Network Development , Casey Schaufler From: Casey Schaufler Message-ID: Date: Tue, 18 Apr 2017 16:16:18 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <9a69055a-b4cf-00b0-da5e-2e45ff88059c@digikod.net> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 4/18/2017 3:44 PM, Mickaël Salaün wrote: > On 19/04/2017 00:17, Kees Cook wrote: >> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün wrote: >>> Handle 33 filesystem-related LSM hooks for the Landlock filesystem >>> event: LANDLOCK_SUBTYPE_EVENT_FS. >>> >>> A Landlock event wrap LSM hooks for similar kernel object types (e.g. >>> struct file, struct path...). Multiple LSM hooks can trigger the same >>> Landlock event. >>> >>> Landlock handle nine coarse-grained actions: read, write, execute, new, >>> get, remove, ioctl, lock and fcntl. Each of them abstract LSM hook >>> access control in a way that can be extended in the future. >>> >>> The Landlock LSM hook registration is done after other LSM to only run >>> actions from user-space, via eBPF programs, if the access was granted by >>> major (privileged) LSMs. >>> >>> Changes since v5: >>> * split hooks.[ch] into hooks.[ch] and hooks_fs.[ch] >>> * add more documentation >>> * cosmetic fixes >>> >>> Changes since v4: >>> * add LSM hook abstraction called Landlock event >>> * use the compiler type checking to verify hooks use by an event >>> * handle all filesystem related LSM hooks (e.g. file_permission, >>> mmap_file, sb_mount...) >>> * register BPF programs for Landlock just after LSM hooks registration >>> * move hooks registration after other LSMs >>> * add failsafes to check if a hook is not used by the kernel >>> * allow partial raw value access form the context (needed for programs >>> generated by LLVM) >>> >>> Changes since v3: >>> * split commit >>> * add hooks dealing with struct inode and struct path pointers: >>> inode_permission and inode_getattr >>> * add abstraction over eBPF helper arguments thanks to wrapping structs >>> >>> Signed-off-by: Mickaël Salaün >>> Cc: Alexei Starovoitov >>> Cc: Andy Lutomirski >>> Cc: Daniel Borkmann >>> Cc: David S. Miller >>> Cc: James Morris >>> Cc: Kees Cook >>> Cc: Serge E. Hallyn >>> --- >>> include/linux/lsm_hooks.h | 5 + >>> security/landlock/Makefile | 4 +- >>> security/landlock/hooks.c | 115 +++++++++ >>> security/landlock/hooks.h | 177 ++++++++++++++ >>> security/landlock/hooks_fs.c | 563 +++++++++++++++++++++++++++++++++++++++++++ >>> security/landlock/hooks_fs.h | 19 ++ >>> security/landlock/init.c | 13 + >>> security/security.c | 7 +- >>> 8 files changed, 901 insertions(+), 2 deletions(-) >>> create mode 100644 security/landlock/hooks.c >>> create mode 100644 security/landlock/hooks.h >>> create mode 100644 security/landlock/hooks_fs.c >>> create mode 100644 security/landlock/hooks_fs.h >>> >>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >>> index e29d4c62a3c8..884289166a0e 100644 >>> --- a/include/linux/lsm_hooks.h >>> +++ b/include/linux/lsm_hooks.h >>> @@ -1920,5 +1920,10 @@ void __init loadpin_add_hooks(void); >>> #else >>> static inline void loadpin_add_hooks(void) { }; >>> #endif >>> +#ifdef CONFIG_SECURITY_LANDLOCK >>> +extern void __init landlock_add_hooks(void); >>> +#else >>> +static inline void __init landlock_add_hooks(void) { } >>> +#endif /* CONFIG_SECURITY_LANDLOCK */ >>> >>> #endif /* ! __LINUX_LSM_HOOKS_H */ >>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile >>> index 7205f9a7a2ee..c0db504a6335 100644 >>> --- a/security/landlock/Makefile >>> +++ b/security/landlock/Makefile >>> @@ -1,3 +1,5 @@ >>> +ccflags-$(CONFIG_SECURITY_LANDLOCK) += -Werror=unused-function >> Why is this needed? If it can't be avoided, a comment should exist >> here explaining why. > This is useful to catch defined but unused hooks: error out if a > HOOK_NEW_FS(foo) is not used with a HOOK_INIT_FS(foo) in the struct > security_hook_list landlock_hooks. > >>> [...] >>> @@ -127,3 +132,11 @@ static struct bpf_prog_type_list bpf_landlock_type __ro_after_init = { >>> .ops = &bpf_landlock_ops, >>> .type = BPF_PROG_TYPE_LANDLOCK, >>> }; >>> + >>> +void __init landlock_add_hooks(void) >>> +{ >>> + pr_info("landlock: Version %u", LANDLOCK_VERSION); >>> + landlock_add_hooks_fs(); >>> + security_add_hooks(NULL, 0, "landlock"); >>> + bpf_register_prog_type(&bpf_landlock_type); >> I'm confused by the separation of hook registration here. The call to >> security_add_hooks is with count=0 is especially weird. Why isn't this >> just a single call with security_add_hooks(landlock_hooks, >> ARRAY_SIZE(landlock_hooks), "landlock")? > Yes, this is ugly with the new security_add_hooks() with three arguments > but I wanted to split the hooks definition in multiple files. Why? I'll buy a good argument, but there are dangers in allowing multiple calls to security_add_hooks(). > > The current security_add_hooks() use lsm_append(lsm, &lsm_names) which > is not exported. Unfortunately, calling multiple security_add_hooks() > with the same LSM name would register multiple names for the same LSM… > Is it OK if I modify this function to not add duplicated entries? It may seem absurd, but it's conceivable that a module might have two hooks it wants called. My example is a module that counts the number of times SELinux denies a process access to things (which needs to be called before and after SELinux in order to detect denials) and takes "appropriate action" if too many denials occur. It would be weird, wonky and hackish, but that never stopped anybody before. > > >>> +} >>> diff --git a/security/security.c b/security/security.c >>> index d0e07f269b2d..a3e9f4625991 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -64,10 +64,15 @@ int __init security_init(void) >>> loadpin_add_hooks(); >>> >>> /* >>> - * Load all the remaining security modules. >>> + * Load all remaining privileged security modules. >>> */ >>> do_security_initcalls(); >>> >>> + /* >>> + * Load potentially-unprivileged security modules at the end. >>> + */ >>> + landlock_add_hooks(); >> Oh, is this to make it last in the list? Is there a reason it has to be last? > Right, this is the intend. I'm not sure it is the only way to register > hooks, though. > > For an unprivileged access-control, we don't want to give the ability to > any process to do some checks, through an eBPF program, on kernel > objects (e.g. files) if they should not be accessible (because of a > following LSM hook check). > > Mickaël > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Casey Schaufler Subject: Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem Date: Tue, 18 Apr 2017 16:16:18 -0700 Message-ID: References: <20170328234650.19695-1-mic@digikod.net> <20170328234650.19695-5-mic@digikod.net> <9a69055a-b4cf-00b0-da5e-2e45ff88059c@digikod.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: LKML , Alexei Starovoitov , Andy Lutomirski , Arnaldo Carvalho de Melo , Daniel Borkmann , David Drysdale , "David S . Miller" , "Eric W . Biederman" , James Morris , Jann Horn , Jonathan Corbet , Matthew Garrett , Michael Kerrisk , Paul Moore , Sargun Dhillon , "Serge E . Hallyn" , Shuah Khan , Tejun Heo , Thomas Graf , Will Drewry , "kernel-hardening@lists.o To: =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= , Kees Cook Return-path: In-Reply-To: <9a69055a-b4cf-00b0-da5e-2e45ff88059c-WFhQfpSGs3bR7s880joybQ@public.gmane.org> Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: netdev.vger.kernel.org On 4/18/2017 3:44 PM, Mickaël Salaün wrote: > On 19/04/2017 00:17, Kees Cook wrote: >> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün wrote: >>> Handle 33 filesystem-related LSM hooks for the Landlock filesystem >>> event: LANDLOCK_SUBTYPE_EVENT_FS. >>> >>> A Landlock event wrap LSM hooks for similar kernel object types (e.g. >>> struct file, struct path...). Multiple LSM hooks can trigger the same >>> Landlock event. >>> >>> Landlock handle nine coarse-grained actions: read, write, execute, new, >>> get, remove, ioctl, lock and fcntl. Each of them abstract LSM hook >>> access control in a way that can be extended in the future. >>> >>> The Landlock LSM hook registration is done after other LSM to only run >>> actions from user-space, via eBPF programs, if the access was granted by >>> major (privileged) LSMs. >>> >>> Changes since v5: >>> * split hooks.[ch] into hooks.[ch] and hooks_fs.[ch] >>> * add more documentation >>> * cosmetic fixes >>> >>> Changes since v4: >>> * add LSM hook abstraction called Landlock event >>> * use the compiler type checking to verify hooks use by an event >>> * handle all filesystem related LSM hooks (e.g. file_permission, >>> mmap_file, sb_mount...) >>> * register BPF programs for Landlock just after LSM hooks registration >>> * move hooks registration after other LSMs >>> * add failsafes to check if a hook is not used by the kernel >>> * allow partial raw value access form the context (needed for programs >>> generated by LLVM) >>> >>> Changes since v3: >>> * split commit >>> * add hooks dealing with struct inode and struct path pointers: >>> inode_permission and inode_getattr >>> * add abstraction over eBPF helper arguments thanks to wrapping structs >>> >>> Signed-off-by: Mickaël Salaün >>> Cc: Alexei Starovoitov >>> Cc: Andy Lutomirski >>> Cc: Daniel Borkmann >>> Cc: David S. Miller >>> Cc: James Morris >>> Cc: Kees Cook >>> Cc: Serge E. Hallyn >>> --- >>> include/linux/lsm_hooks.h | 5 + >>> security/landlock/Makefile | 4 +- >>> security/landlock/hooks.c | 115 +++++++++ >>> security/landlock/hooks.h | 177 ++++++++++++++ >>> security/landlock/hooks_fs.c | 563 +++++++++++++++++++++++++++++++++++++++++++ >>> security/landlock/hooks_fs.h | 19 ++ >>> security/landlock/init.c | 13 + >>> security/security.c | 7 +- >>> 8 files changed, 901 insertions(+), 2 deletions(-) >>> create mode 100644 security/landlock/hooks.c >>> create mode 100644 security/landlock/hooks.h >>> create mode 100644 security/landlock/hooks_fs.c >>> create mode 100644 security/landlock/hooks_fs.h >>> >>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >>> index e29d4c62a3c8..884289166a0e 100644 >>> --- a/include/linux/lsm_hooks.h >>> +++ b/include/linux/lsm_hooks.h >>> @@ -1920,5 +1920,10 @@ void __init loadpin_add_hooks(void); >>> #else >>> static inline void loadpin_add_hooks(void) { }; >>> #endif >>> +#ifdef CONFIG_SECURITY_LANDLOCK >>> +extern void __init landlock_add_hooks(void); >>> +#else >>> +static inline void __init landlock_add_hooks(void) { } >>> +#endif /* CONFIG_SECURITY_LANDLOCK */ >>> >>> #endif /* ! __LINUX_LSM_HOOKS_H */ >>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile >>> index 7205f9a7a2ee..c0db504a6335 100644 >>> --- a/security/landlock/Makefile >>> +++ b/security/landlock/Makefile >>> @@ -1,3 +1,5 @@ >>> +ccflags-$(CONFIG_SECURITY_LANDLOCK) += -Werror=unused-function >> Why is this needed? If it can't be avoided, a comment should exist >> here explaining why. > This is useful to catch defined but unused hooks: error out if a > HOOK_NEW_FS(foo) is not used with a HOOK_INIT_FS(foo) in the struct > security_hook_list landlock_hooks. > >>> [...] >>> @@ -127,3 +132,11 @@ static struct bpf_prog_type_list bpf_landlock_type __ro_after_init = { >>> .ops = &bpf_landlock_ops, >>> .type = BPF_PROG_TYPE_LANDLOCK, >>> }; >>> + >>> +void __init landlock_add_hooks(void) >>> +{ >>> + pr_info("landlock: Version %u", LANDLOCK_VERSION); >>> + landlock_add_hooks_fs(); >>> + security_add_hooks(NULL, 0, "landlock"); >>> + bpf_register_prog_type(&bpf_landlock_type); >> I'm confused by the separation of hook registration here. The call to >> security_add_hooks is with count=0 is especially weird. Why isn't this >> just a single call with security_add_hooks(landlock_hooks, >> ARRAY_SIZE(landlock_hooks), "landlock")? > Yes, this is ugly with the new security_add_hooks() with three arguments > but I wanted to split the hooks definition in multiple files. Why? I'll buy a good argument, but there are dangers in allowing multiple calls to security_add_hooks(). > > The current security_add_hooks() use lsm_append(lsm, &lsm_names) which > is not exported. Unfortunately, calling multiple security_add_hooks() > with the same LSM name would register multiple names for the same LSM… > Is it OK if I modify this function to not add duplicated entries? It may seem absurd, but it's conceivable that a module might have two hooks it wants called. My example is a module that counts the number of times SELinux denies a process access to things (which needs to be called before and after SELinux in order to detect denials) and takes "appropriate action" if too many denials occur. It would be weird, wonky and hackish, but that never stopped anybody before. > > >>> +} >>> diff --git a/security/security.c b/security/security.c >>> index d0e07f269b2d..a3e9f4625991 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -64,10 +64,15 @@ int __init security_init(void) >>> loadpin_add_hooks(); >>> >>> /* >>> - * Load all the remaining security modules. >>> + * Load all remaining privileged security modules. >>> */ >>> do_security_initcalls(); >>> >>> + /* >>> + * Load potentially-unprivileged security modules at the end. >>> + */ >>> + landlock_add_hooks(); >> Oh, is this to make it last in the list? Is there a reason it has to be last? > Right, this is the intend. I'm not sure it is the only way to register > hooks, though. > > For an unprivileged access-control, we don't want to give the ability to > any process to do some checks, through an eBPF program, on kernel > objects (e.g. files) if they should not be accessible (because of a > following LSM hook check). > > Mickaël > From mboxrd@z Thu Jan 1 00:00:00 1970 From: casey@schaufler-ca.com (Casey Schaufler) Date: Tue, 18 Apr 2017 16:16:18 -0700 Subject: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem In-Reply-To: <9a69055a-b4cf-00b0-da5e-2e45ff88059c@digikod.net> References: <20170328234650.19695-1-mic@digikod.net> <20170328234650.19695-5-mic@digikod.net> <9a69055a-b4cf-00b0-da5e-2e45ff88059c@digikod.net> Message-ID: To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On 4/18/2017 3:44 PM, Micka?l Sala?n wrote: > On 19/04/2017 00:17, Kees Cook wrote: >> On Tue, Mar 28, 2017 at 4:46 PM, Micka?l Sala?n wrote: >>> Handle 33 filesystem-related LSM hooks for the Landlock filesystem >>> event: LANDLOCK_SUBTYPE_EVENT_FS. >>> >>> A Landlock event wrap LSM hooks for similar kernel object types (e.g. >>> struct file, struct path...). Multiple LSM hooks can trigger the same >>> Landlock event. >>> >>> Landlock handle nine coarse-grained actions: read, write, execute, new, >>> get, remove, ioctl, lock and fcntl. Each of them abstract LSM hook >>> access control in a way that can be extended in the future. >>> >>> The Landlock LSM hook registration is done after other LSM to only run >>> actions from user-space, via eBPF programs, if the access was granted by >>> major (privileged) LSMs. >>> >>> Changes since v5: >>> * split hooks.[ch] into hooks.[ch] and hooks_fs.[ch] >>> * add more documentation >>> * cosmetic fixes >>> >>> Changes since v4: >>> * add LSM hook abstraction called Landlock event >>> * use the compiler type checking to verify hooks use by an event >>> * handle all filesystem related LSM hooks (e.g. file_permission, >>> mmap_file, sb_mount...) >>> * register BPF programs for Landlock just after LSM hooks registration >>> * move hooks registration after other LSMs >>> * add failsafes to check if a hook is not used by the kernel >>> * allow partial raw value access form the context (needed for programs >>> generated by LLVM) >>> >>> Changes since v3: >>> * split commit >>> * add hooks dealing with struct inode and struct path pointers: >>> inode_permission and inode_getattr >>> * add abstraction over eBPF helper arguments thanks to wrapping structs >>> >>> Signed-off-by: Micka?l Sala?n >>> Cc: Alexei Starovoitov >>> Cc: Andy Lutomirski >>> Cc: Daniel Borkmann >>> Cc: David S. Miller >>> Cc: James Morris >>> Cc: Kees Cook >>> Cc: Serge E. Hallyn >>> --- >>> include/linux/lsm_hooks.h | 5 + >>> security/landlock/Makefile | 4 +- >>> security/landlock/hooks.c | 115 +++++++++ >>> security/landlock/hooks.h | 177 ++++++++++++++ >>> security/landlock/hooks_fs.c | 563 +++++++++++++++++++++++++++++++++++++++++++ >>> security/landlock/hooks_fs.h | 19 ++ >>> security/landlock/init.c | 13 + >>> security/security.c | 7 +- >>> 8 files changed, 901 insertions(+), 2 deletions(-) >>> create mode 100644 security/landlock/hooks.c >>> create mode 100644 security/landlock/hooks.h >>> create mode 100644 security/landlock/hooks_fs.c >>> create mode 100644 security/landlock/hooks_fs.h >>> >>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >>> index e29d4c62a3c8..884289166a0e 100644 >>> --- a/include/linux/lsm_hooks.h >>> +++ b/include/linux/lsm_hooks.h >>> @@ -1920,5 +1920,10 @@ void __init loadpin_add_hooks(void); >>> #else >>> static inline void loadpin_add_hooks(void) { }; >>> #endif >>> +#ifdef CONFIG_SECURITY_LANDLOCK >>> +extern void __init landlock_add_hooks(void); >>> +#else >>> +static inline void __init landlock_add_hooks(void) { } >>> +#endif /* CONFIG_SECURITY_LANDLOCK */ >>> >>> #endif /* ! __LINUX_LSM_HOOKS_H */ >>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile >>> index 7205f9a7a2ee..c0db504a6335 100644 >>> --- a/security/landlock/Makefile >>> +++ b/security/landlock/Makefile >>> @@ -1,3 +1,5 @@ >>> +ccflags-$(CONFIG_SECURITY_LANDLOCK) += -Werror=unused-function >> Why is this needed? If it can't be avoided, a comment should exist >> here explaining why. > This is useful to catch defined but unused hooks: error out if a > HOOK_NEW_FS(foo) is not used with a HOOK_INIT_FS(foo) in the struct > security_hook_list landlock_hooks. > >>> [...] >>> @@ -127,3 +132,11 @@ static struct bpf_prog_type_list bpf_landlock_type __ro_after_init = { >>> .ops = &bpf_landlock_ops, >>> .type = BPF_PROG_TYPE_LANDLOCK, >>> }; >>> + >>> +void __init landlock_add_hooks(void) >>> +{ >>> + pr_info("landlock: Version %u", LANDLOCK_VERSION); >>> + landlock_add_hooks_fs(); >>> + security_add_hooks(NULL, 0, "landlock"); >>> + bpf_register_prog_type(&bpf_landlock_type); >> I'm confused by the separation of hook registration here. The call to >> security_add_hooks is with count=0 is especially weird. Why isn't this >> just a single call with security_add_hooks(landlock_hooks, >> ARRAY_SIZE(landlock_hooks), "landlock")? > Yes, this is ugly with the new security_add_hooks() with three arguments > but I wanted to split the hooks definition in multiple files. Why? I'll buy a good argument, but there are dangers in allowing multiple calls to security_add_hooks(). > > The current security_add_hooks() use lsm_append(lsm, &lsm_names) which > is not exported. Unfortunately, calling multiple security_add_hooks() > with the same LSM name would register multiple names for the same LSM? > Is it OK if I modify this function to not add duplicated entries? It may seem absurd, but it's conceivable that a module might have two hooks it wants called. My example is a module that counts the number of times SELinux denies a process access to things (which needs to be called before and after SELinux in order to detect denials) and takes "appropriate action" if too many denials occur. It would be weird, wonky and hackish, but that never stopped anybody before. > > >>> +} >>> diff --git a/security/security.c b/security/security.c >>> index d0e07f269b2d..a3e9f4625991 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -64,10 +64,15 @@ int __init security_init(void) >>> loadpin_add_hooks(); >>> >>> /* >>> - * Load all the remaining security modules. >>> + * Load all remaining privileged security modules. >>> */ >>> do_security_initcalls(); >>> >>> + /* >>> + * Load potentially-unprivileged security modules at the end. >>> + */ >>> + landlock_add_hooks(); >> Oh, is this to make it last in the list? Is there a reason it has to be last? > Right, this is the intend. I'm not sure it is the only way to register > hooks, though. > > For an unprivileged access-control, we don't want to give the ability to > any process to do some checks, through an eBPF program, on kernel > objects (e.g. files) if they should not be accessible (because of a > following LSM hook check). > > Micka?l > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 References: <20170328234650.19695-1-mic@digikod.net> <20170328234650.19695-5-mic@digikod.net> <9a69055a-b4cf-00b0-da5e-2e45ff88059c@digikod.net> From: Casey Schaufler Message-ID: Date: Tue, 18 Apr 2017 16:16:18 -0700 MIME-Version: 1.0 In-Reply-To: <9a69055a-b4cf-00b0-da5e-2e45ff88059c@digikod.net> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: [kernel-hardening] Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem To: =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= , Kees Cook Cc: LKML , Alexei Starovoitov , Andy Lutomirski , Arnaldo Carvalho de Melo , Daniel Borkmann , David Drysdale , "David S . Miller" , "Eric W . Biederman" , James Morris , Jann Horn , Jonathan Corbet , Matthew Garrett , Michael Kerrisk , Paul Moore , Sargun Dhillon , "Serge E . Hallyn" , Shuah Khan , Tejun Heo , Thomas Graf , Will Drewry , "kernel-hardening@lists.openwall.com" , Linux API , linux-security-module , Network Development , Casey Schaufler List-ID: On 4/18/2017 3:44 PM, Mickaël Salaün wrote: > On 19/04/2017 00:17, Kees Cook wrote: >> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün wrote: >>> Handle 33 filesystem-related LSM hooks for the Landlock filesystem >>> event: LANDLOCK_SUBTYPE_EVENT_FS. >>> >>> A Landlock event wrap LSM hooks for similar kernel object types (e.g. >>> struct file, struct path...). Multiple LSM hooks can trigger the same >>> Landlock event. >>> >>> Landlock handle nine coarse-grained actions: read, write, execute, new, >>> get, remove, ioctl, lock and fcntl. Each of them abstract LSM hook >>> access control in a way that can be extended in the future. >>> >>> The Landlock LSM hook registration is done after other LSM to only run >>> actions from user-space, via eBPF programs, if the access was granted by >>> major (privileged) LSMs. >>> >>> Changes since v5: >>> * split hooks.[ch] into hooks.[ch] and hooks_fs.[ch] >>> * add more documentation >>> * cosmetic fixes >>> >>> Changes since v4: >>> * add LSM hook abstraction called Landlock event >>> * use the compiler type checking to verify hooks use by an event >>> * handle all filesystem related LSM hooks (e.g. file_permission, >>> mmap_file, sb_mount...) >>> * register BPF programs for Landlock just after LSM hooks registration >>> * move hooks registration after other LSMs >>> * add failsafes to check if a hook is not used by the kernel >>> * allow partial raw value access form the context (needed for programs >>> generated by LLVM) >>> >>> Changes since v3: >>> * split commit >>> * add hooks dealing with struct inode and struct path pointers: >>> inode_permission and inode_getattr >>> * add abstraction over eBPF helper arguments thanks to wrapping structs >>> >>> Signed-off-by: Mickaël Salaün >>> Cc: Alexei Starovoitov >>> Cc: Andy Lutomirski >>> Cc: Daniel Borkmann >>> Cc: David S. Miller >>> Cc: James Morris >>> Cc: Kees Cook >>> Cc: Serge E. Hallyn >>> --- >>> include/linux/lsm_hooks.h | 5 + >>> security/landlock/Makefile | 4 +- >>> security/landlock/hooks.c | 115 +++++++++ >>> security/landlock/hooks.h | 177 ++++++++++++++ >>> security/landlock/hooks_fs.c | 563 +++++++++++++++++++++++++++++++++++++++++++ >>> security/landlock/hooks_fs.h | 19 ++ >>> security/landlock/init.c | 13 + >>> security/security.c | 7 +- >>> 8 files changed, 901 insertions(+), 2 deletions(-) >>> create mode 100644 security/landlock/hooks.c >>> create mode 100644 security/landlock/hooks.h >>> create mode 100644 security/landlock/hooks_fs.c >>> create mode 100644 security/landlock/hooks_fs.h >>> >>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >>> index e29d4c62a3c8..884289166a0e 100644 >>> --- a/include/linux/lsm_hooks.h >>> +++ b/include/linux/lsm_hooks.h >>> @@ -1920,5 +1920,10 @@ void __init loadpin_add_hooks(void); >>> #else >>> static inline void loadpin_add_hooks(void) { }; >>> #endif >>> +#ifdef CONFIG_SECURITY_LANDLOCK >>> +extern void __init landlock_add_hooks(void); >>> +#else >>> +static inline void __init landlock_add_hooks(void) { } >>> +#endif /* CONFIG_SECURITY_LANDLOCK */ >>> >>> #endif /* ! __LINUX_LSM_HOOKS_H */ >>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile >>> index 7205f9a7a2ee..c0db504a6335 100644 >>> --- a/security/landlock/Makefile >>> +++ b/security/landlock/Makefile >>> @@ -1,3 +1,5 @@ >>> +ccflags-$(CONFIG_SECURITY_LANDLOCK) += -Werror=unused-function >> Why is this needed? If it can't be avoided, a comment should exist >> here explaining why. > This is useful to catch defined but unused hooks: error out if a > HOOK_NEW_FS(foo) is not used with a HOOK_INIT_FS(foo) in the struct > security_hook_list landlock_hooks. > >>> [...] >>> @@ -127,3 +132,11 @@ static struct bpf_prog_type_list bpf_landlock_type __ro_after_init = { >>> .ops = &bpf_landlock_ops, >>> .type = BPF_PROG_TYPE_LANDLOCK, >>> }; >>> + >>> +void __init landlock_add_hooks(void) >>> +{ >>> + pr_info("landlock: Version %u", LANDLOCK_VERSION); >>> + landlock_add_hooks_fs(); >>> + security_add_hooks(NULL, 0, "landlock"); >>> + bpf_register_prog_type(&bpf_landlock_type); >> I'm confused by the separation of hook registration here. The call to >> security_add_hooks is with count=0 is especially weird. Why isn't this >> just a single call with security_add_hooks(landlock_hooks, >> ARRAY_SIZE(landlock_hooks), "landlock")? > Yes, this is ugly with the new security_add_hooks() with three arguments > but I wanted to split the hooks definition in multiple files. Why? I'll buy a good argument, but there are dangers in allowing multiple calls to security_add_hooks(). > > The current security_add_hooks() use lsm_append(lsm, &lsm_names) which > is not exported. Unfortunately, calling multiple security_add_hooks() > with the same LSM name would register multiple names for the same LSM… > Is it OK if I modify this function to not add duplicated entries? It may seem absurd, but it's conceivable that a module might have two hooks it wants called. My example is a module that counts the number of times SELinux denies a process access to things (which needs to be called before and after SELinux in order to detect denials) and takes "appropriate action" if too many denials occur. It would be weird, wonky and hackish, but that never stopped anybody before. > > >>> +} >>> diff --git a/security/security.c b/security/security.c >>> index d0e07f269b2d..a3e9f4625991 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -64,10 +64,15 @@ int __init security_init(void) >>> loadpin_add_hooks(); >>> >>> /* >>> - * Load all the remaining security modules. >>> + * Load all remaining privileged security modules. >>> */ >>> do_security_initcalls(); >>> >>> + /* >>> + * Load potentially-unprivileged security modules at the end. >>> + */ >>> + landlock_add_hooks(); >> Oh, is this to make it last in the list? Is there a reason it has to be last? > Right, this is the intend. I'm not sure it is the only way to register > hooks, though. > > For an unprivileged access-control, we don't want to give the ability to > any process to do some checks, through an eBPF program, on kernel > objects (e.g. files) if they should not be accessible (because of a > following LSM hook check). > > Mickaël >