[PATCH RFC 1/9] LSM: Add /sys/kernel/security/lsm

[PATCH RFC 1/9] LSM: Add /sys/kernel/security/lsm

[Tetsuo Handa]

Casey Schaufler wrote:
> Subject: [PATCH RFC 1/9] LSM: Add /sys/kernel/security/lsm
> 
> This has been accepted into security next - provided for completeness

Before going to "[PATCH RFC 5/9] LSM: General but not extreme module
stacking", what do you think about below change?

----------------------------------------

>From 8fe80e4b6a479a81d720571ad3b5979f6fd1e6ae Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Fri, 17 Mar 2017 21:34:51 +0900
Subject: [PATCH] LSM: Pass module name via "union security_list_options".

Commit d69dece5f5b6bc7a ("LSM: Add /sys/kernel/security/lsm") added
"char *lsm" field to "struct security_hook_list". But it is currently
never used after set at initialization time. While there is a plan that
this field will be used by "LSM: General but not extreme module stacking"
patch, there is no point with setting the same value to each element of
"struct security_hook_list" array. We can keep "struct security_hook_list"
4 * sizeof(void *) bytes if we keep this field in "union
security_list_options".

The "char *" argument in security_add_hooks() was removed because we can
fetch it from security_list_options"->module_name field. This implies
that setting security_list_options"->module_name field is mandatory.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 include/linux/lsm_hooks.h  |  8 +++++---
 security/apparmor/lsm.c    |  4 ++--
 security/commoncap.c       |  4 ++--
 security/loadpin/loadpin.c |  3 ++-
 security/security.c        | 13 +++++++------
 security/selinux/hooks.c   |  3 ++-
 security/smack/smack_lsm.c |  3 ++-
 security/tomoyo/tomoyo.c   |  3 ++-
 security/yama/yama_lsm.c   |  3 ++-
 9 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index a4193c3..d10b4f7 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -29,6 +29,8 @@
 #include <linux/rculist.h>
 
 /**
+ * @module_name: the name of the security module
+ *
  * Security hooks for program execution operations.
  *
  * @bprm_set_creds:
@@ -1340,6 +1342,7 @@
  */
 
 union security_list_options {
+	const char *module_name;
 	int (*binder_set_context_mgr)(struct task_struct *mgr);
 	int (*binder_transaction)(struct task_struct *from,
 					struct task_struct *to);
@@ -1664,6 +1667,7 @@
 };
 
 struct security_hook_heads {
+	struct list_head module_name;
 	struct list_head binder_set_context_mgr;
 	struct list_head binder_transaction;
 	struct list_head binder_transfer_binder;
@@ -1886,7 +1890,6 @@ struct security_hook_list {
 	struct list_head		list;
 	struct list_head		*head;
 	union security_list_options	hook;
-	char				*lsm;
 };
 
 /*
@@ -1901,8 +1904,7 @@ struct security_hook_list {
 extern struct security_hook_heads security_hook_heads;
 extern char *lsm_names;
 
-extern void security_add_hooks(struct security_hook_list *hooks, int count,
-				char *lsm);
+extern void security_add_hooks(struct security_hook_list *hooks, int count);
 
 #ifdef CONFIG_SECURITY_SELINUX_DISABLE
 /*
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index e287b69..dc21835 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -588,6 +588,7 @@ static int apparmor_task_setrlimit(struct task_struct *task,
 }
 
 static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
+	LSM_HOOK_INIT(module_name, "apparmor"),
 	LSM_HOOK_INIT(ptrace_access_check, apparmor_ptrace_access_check),
 	LSM_HOOK_INIT(ptrace_traceme, apparmor_ptrace_traceme),
 	LSM_HOOK_INIT(capget, apparmor_capget),
@@ -996,8 +997,7 @@ static int __init apparmor_init(void)
 		aa_free_root_ns();
 		goto buffers_out;
 	}
-	security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
-				"apparmor");
+	security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks));
 
 	/* Report that AppArmor successfully initialized */
 	apparmor_initialized = 1;
diff --git a/security/commoncap.c b/security/commoncap.c
index 7abebd7..1ced4ee 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -1072,6 +1072,7 @@ int cap_mmap_file(struct file *file, unsigned long reqprot,
 #ifdef CONFIG_SECURITY
 
 struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
+	LSM_HOOK_INIT(module_name, "capability"),
 	LSM_HOOK_INIT(capable, cap_capable),
 	LSM_HOOK_INIT(settime, cap_settime),
 	LSM_HOOK_INIT(ptrace_access_check, cap_ptrace_access_check),
@@ -1094,8 +1095,7 @@ struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
 
 void __init capability_add_hooks(void)
 {
-	security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks),
-				"capability");
+	security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks));
 }
 
 #endif /* CONFIG_SECURITY */
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index dbe6efd..c126c74 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -175,6 +175,7 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
 }
 
 static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
+	LSM_HOOK_INIT(module_name, "loadpin"),
 	LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
 	LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
 };
@@ -182,7 +183,7 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
 void __init loadpin_add_hooks(void)
 {
 	pr_info("ready to pin (currently %sabled)", enabled ? "en" : "dis");
-	security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
+	security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks));
 }
 
 /* Should not be mutable after boot, so not listed in sysfs (perm == 0). */
diff --git a/security/security.c b/security/security.c
index 5b6cdd0..ae57a84 100644
--- a/security/security.c
+++ b/security/security.c
@@ -79,7 +79,7 @@ static int __init choose_lsm(char *str)
 }
 __setup("security=", choose_lsm);
 
-static int lsm_append(char *new, char **result)
+static int lsm_append(const char *new, char **result)
 {
 	char *cp;
 
@@ -118,20 +118,20 @@ int __init security_module_enable(const char *module)
  * security_add_hooks - Add a modules hooks to the hook lists.
  * @hooks: the hooks to add
  * @count: the number of hooks to add
- * @lsm: the name of the security module
  *
  * Each LSM has to register its hooks with the infrastructure.
  */
-void __init security_add_hooks(struct security_hook_list *hooks, int count,
-				char *lsm)
+void __init security_add_hooks(struct security_hook_list *hooks, int count)
 {
 	int i;
+	const char *lsm = NULL;
 
 	for (i = 0; i < count; i++) {
-		hooks[i].lsm = lsm;
 		list_add_tail_rcu(&hooks[i].list, hooks[i].head);
+		if (hooks[i].head == &security_hook_heads.module_name)
+			lsm = hooks[i].hook.module_name;
 	}
-	if (lsm_append(lsm, &lsm_names) < 0)
+	if (!lsm || lsm_append(lsm, &lsm_names) < 0)
 		panic("%s - Cannot get early memory.\n", __func__);
 }
 
@@ -1634,6 +1634,7 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule,
 #endif /* CONFIG_AUDIT */
 
 struct security_hook_heads security_hook_heads __lsm_ro_after_init = {
+	.module_name = LIST_HEAD_INIT(security_hook_heads.module_name),
 	.binder_set_context_mgr =
 		LIST_HEAD_INIT(security_hook_heads.binder_set_context_mgr),
 	.binder_transaction =
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d37a723..cd7befe 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6124,6 +6124,7 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer)
 #endif
 
 static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
+	LSM_HOOK_INIT(module_name, "selinux"),
 	LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
 	LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
 	LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
@@ -6366,7 +6367,7 @@ static __init int selinux_init(void)
 					    0, SLAB_PANIC, NULL);
 	avc_init();
 
-	security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
+	security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
 
 	if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
 		panic("SELinux: Unable to register AVC netcache callback\n");
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 927e60e..f2457b8 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4634,6 +4634,7 @@ static int smack_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
 }
 
 static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
+	LSM_HOOK_INIT(module_name, "smack"),
 	LSM_HOOK_INIT(ptrace_access_check, smack_ptrace_access_check),
 	LSM_HOOK_INIT(ptrace_traceme, smack_ptrace_traceme),
 	LSM_HOOK_INIT(syslog, smack_syslog),
@@ -4850,7 +4851,7 @@ static __init int smack_init(void)
 	/*
 	 * Register with LSM
 	 */
-	security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack");
+	security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks));
 
 	return 0;
 }
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 0de150d..6ba6139 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -488,6 +488,7 @@ static int tomoyo_socket_sendmsg(struct socket *sock, struct msghdr *msg,
  * registering TOMOYO.
  */
 static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
+	LSM_HOOK_INIT(module_name, "tomoyo"),
 	LSM_HOOK_INIT(task_alloc, tomoyo_task_alloc),
 	LSM_HOOK_INIT(task_free, tomoyo_task_free),
 	LSM_HOOK_INIT(bprm_set_creds, tomoyo_bprm_set_creds),
@@ -534,7 +535,7 @@ static int __init tomoyo_init(void)
 	if (!security_module_enable("tomoyo"))
 		return 0;
 	/* register ourselves with the security framework */
-	security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo");
+	security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks));
 	printk(KERN_INFO "TOMOYO Linux initialized\n");
 	current->security = &ts;
 	tomoyo_mm_init();
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index 8298e09..3290d33 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -429,6 +429,7 @@ int yama_ptrace_traceme(struct task_struct *parent)
 }
 
 static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
+	LSM_HOOK_INIT(module_name, "yama"),
 	LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
 	LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
 	LSM_HOOK_INIT(task_prctl, yama_task_prctl),
@@ -485,6 +486,6 @@ static inline void yama_init_sysctl(void) { }
 void __init yama_add_hooks(void)
 {
 	pr_info("Yama: becoming mindful.\n");
-	security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), "yama");
+	security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks));
 	yama_init_sysctl();
 }
-- 
1.8.3.1
--
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

[PATCH RFC 1/9] LSM: Add /sys/kernel/security/lsm

[Casey Schaufler]

On 3/17/2017 5:52 AM, Tetsuo Handa wrote:
> Casey Schaufler wrote:
>> Subject: [PATCH RFC 1/9] LSM: Add /sys/kernel/security/lsm
>>
>> This has been accepted into security next - provided for completeness
> Before going to "[PATCH RFC 5/9] LSM: General but not extreme module
> stacking", what do you think about below change?

It does clean things up a bit. I will incorporate the change.
Thank you.

>
> ----------------------------------------
>
> >From 8fe80e4b6a479a81d720571ad3b5979f6fd1e6ae Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Fri, 17 Mar 2017 21:34:51 +0900
> Subject: [PATCH] LSM: Pass module name via "union security_list_options".
>
> Commit d69dece5f5b6bc7a ("LSM: Add /sys/kernel/security/lsm") added
> "char *lsm" field to "struct security_hook_list". But it is currently
> never used after set at initialization time. While there is a plan that
> this field will be used by "LSM: General but not extreme module stacking"
> patch, there is no point with setting the same value to each element of
> "struct security_hook_list" array. We can keep "struct security_hook_list"
> 4 * sizeof(void *) bytes if we keep this field in "union
> security_list_options".
>
> The "char *" argument in security_add_hooks() was removed because we can
> fetch it from security_list_options"->module_name field. This implies
> that setting security_list_options"->module_name field is mandatory.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  include/linux/lsm_hooks.h  |  8 +++++---
>  security/apparmor/lsm.c    |  4 ++--
>  security/commoncap.c       |  4 ++--
>  security/loadpin/loadpin.c |  3 ++-
>  security/security.c        | 13 +++++++------
>  security/selinux/hooks.c   |  3 ++-
>  security/smack/smack_lsm.c |  3 ++-
>  security/tomoyo/tomoyo.c   |  3 ++-
>  security/yama/yama_lsm.c   |  3 ++-
>  9 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index a4193c3..d10b4f7 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -29,6 +29,8 @@
>  #include <linux/rculist.h>
>  
>  /**
> + * @module_name: the name of the security module
> + *
>   * Security hooks for program execution operations.
>   *
>   * @bprm_set_creds:
> @@ -1340,6 +1342,7 @@
>   */
>  
>  union security_list_options {
> +	const char *module_name;
>  	int (*binder_set_context_mgr)(struct task_struct *mgr);
>  	int (*binder_transaction)(struct task_struct *from,
>  					struct task_struct *to);
> @@ -1664,6 +1667,7 @@
>  };
>  
>  struct security_hook_heads {
> +	struct list_head module_name;
>  	struct list_head binder_set_context_mgr;
>  	struct list_head binder_transaction;
>  	struct list_head binder_transfer_binder;
> @@ -1886,7 +1890,6 @@ struct security_hook_list {
>  	struct list_head		list;
>  	struct list_head		*head;
>  	union security_list_options	hook;
> -	char				*lsm;
>  };
>  
>  /*
> @@ -1901,8 +1904,7 @@ struct security_hook_list {
>  extern struct security_hook_heads security_hook_heads;
>  extern char *lsm_names;
>  
> -extern void security_add_hooks(struct security_hook_list *hooks, int count,
> -				char *lsm);
> +extern void security_add_hooks(struct security_hook_list *hooks, int count);
>  
>  #ifdef CONFIG_SECURITY_SELINUX_DISABLE
>  /*
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index e287b69..dc21835 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -588,6 +588,7 @@ static int apparmor_task_setrlimit(struct task_struct *task,
>  }
>  
>  static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
> +	LSM_HOOK_INIT(module_name, "apparmor"),
>  	LSM_HOOK_INIT(ptrace_access_check, apparmor_ptrace_access_check),
>  	LSM_HOOK_INIT(ptrace_traceme, apparmor_ptrace_traceme),
>  	LSM_HOOK_INIT(capget, apparmor_capget),
> @@ -996,8 +997,7 @@ static int __init apparmor_init(void)
>  		aa_free_root_ns();
>  		goto buffers_out;
>  	}
> -	security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
> -				"apparmor");
> +	security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks));
>  
>  	/* Report that AppArmor successfully initialized */
>  	apparmor_initialized = 1;
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 7abebd7..1ced4ee 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -1072,6 +1072,7 @@ int cap_mmap_file(struct file *file, unsigned long reqprot,
>  #ifdef CONFIG_SECURITY
>  
>  struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
> +	LSM_HOOK_INIT(module_name, "capability"),
>  	LSM_HOOK_INIT(capable, cap_capable),
>  	LSM_HOOK_INIT(settime, cap_settime),
>  	LSM_HOOK_INIT(ptrace_access_check, cap_ptrace_access_check),
> @@ -1094,8 +1095,7 @@ struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
>  
>  void __init capability_add_hooks(void)
>  {
> -	security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks),
> -				"capability");
> +	security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks));
>  }
>  
>  #endif /* CONFIG_SECURITY */
> diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
> index dbe6efd..c126c74 100644
> --- a/security/loadpin/loadpin.c
> +++ b/security/loadpin/loadpin.c
> @@ -175,6 +175,7 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
>  }
>  
>  static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
> +	LSM_HOOK_INIT(module_name, "loadpin"),
>  	LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
>  	LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
>  };
> @@ -182,7 +183,7 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
>  void __init loadpin_add_hooks(void)
>  {
>  	pr_info("ready to pin (currently %sabled)", enabled ? "en" : "dis");
> -	security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
> +	security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks));
>  }
>  
>  /* Should not be mutable after boot, so not listed in sysfs (perm == 0). */
> diff --git a/security/security.c b/security/security.c
> index 5b6cdd0..ae57a84 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -79,7 +79,7 @@ static int __init choose_lsm(char *str)
>  }
>  __setup("security=", choose_lsm);
>  
> -static int lsm_append(char *new, char **result)
> +static int lsm_append(const char *new, char **result)
>  {
>  	char *cp;
>  
> @@ -118,20 +118,20 @@ int __init security_module_enable(const char *module)
>   * security_add_hooks - Add a modules hooks to the hook lists.
>   * @hooks: the hooks to add
>   * @count: the number of hooks to add
> - * @lsm: the name of the security module
>   *
>   * Each LSM has to register its hooks with the infrastructure.
>   */
> -void __init security_add_hooks(struct security_hook_list *hooks, int count,
> -				char *lsm)
> +void __init security_add_hooks(struct security_hook_list *hooks, int count)
>  {
>  	int i;
> +	const char *lsm = NULL;
>  
>  	for (i = 0; i < count; i++) {
> -		hooks[i].lsm = lsm;
>  		list_add_tail_rcu(&hooks[i].list, hooks[i].head);
> +		if (hooks[i].head == &security_hook_heads.module_name)
> +			lsm = hooks[i].hook.module_name;
>  	}
> -	if (lsm_append(lsm, &lsm_names) < 0)
> +	if (!lsm || lsm_append(lsm, &lsm_names) < 0)
>  		panic("%s - Cannot get early memory.\n", __func__);
>  }
>  
> @@ -1634,6 +1634,7 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule,
>  #endif /* CONFIG_AUDIT */
>  
>  struct security_hook_heads security_hook_heads __lsm_ro_after_init = {
> +	.module_name = LIST_HEAD_INIT(security_hook_heads.module_name),
>  	.binder_set_context_mgr =
>  		LIST_HEAD_INIT(security_hook_heads.binder_set_context_mgr),
>  	.binder_transaction =
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index d37a723..cd7befe 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6124,6 +6124,7 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer)
>  #endif
>  
>  static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> +	LSM_HOOK_INIT(module_name, "selinux"),
>  	LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
>  	LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
>  	LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
> @@ -6366,7 +6367,7 @@ static __init int selinux_init(void)
>  					    0, SLAB_PANIC, NULL);
>  	avc_init();
>  
> -	security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
> +	security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
>  
>  	if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
>  		panic("SELinux: Unable to register AVC netcache callback\n");
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 927e60e..f2457b8 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4634,6 +4634,7 @@ static int smack_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
>  }
>  
>  static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
> +	LSM_HOOK_INIT(module_name, "smack"),
>  	LSM_HOOK_INIT(ptrace_access_check, smack_ptrace_access_check),
>  	LSM_HOOK_INIT(ptrace_traceme, smack_ptrace_traceme),
>  	LSM_HOOK_INIT(syslog, smack_syslog),
> @@ -4850,7 +4851,7 @@ static __init int smack_init(void)
>  	/*
>  	 * Register with LSM
>  	 */
> -	security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack");
> +	security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks));
>  
>  	return 0;
>  }
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 0de150d..6ba6139 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -488,6 +488,7 @@ static int tomoyo_socket_sendmsg(struct socket *sock, struct msghdr *msg,
>   * registering TOMOYO.
>   */
>  static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
> +	LSM_HOOK_INIT(module_name, "tomoyo"),
>  	LSM_HOOK_INIT(task_alloc, tomoyo_task_alloc),
>  	LSM_HOOK_INIT(task_free, tomoyo_task_free),
>  	LSM_HOOK_INIT(bprm_set_creds, tomoyo_bprm_set_creds),
> @@ -534,7 +535,7 @@ static int __init tomoyo_init(void)
>  	if (!security_module_enable("tomoyo"))
>  		return 0;
>  	/* register ourselves with the security framework */
> -	security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo");
> +	security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks));
>  	printk(KERN_INFO "TOMOYO Linux initialized\n");
>  	current->security = &ts;
>  	tomoyo_mm_init();
> diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
> index 8298e09..3290d33 100644
> --- a/security/yama/yama_lsm.c
> +++ b/security/yama/yama_lsm.c
> @@ -429,6 +429,7 @@ int yama_ptrace_traceme(struct task_struct *parent)
>  }
>  
>  static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
> +	LSM_HOOK_INIT(module_name, "yama"),
>  	LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
>  	LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
>  	LSM_HOOK_INIT(task_prctl, yama_task_prctl),
> @@ -485,6 +486,6 @@ static inline void yama_init_sysctl(void) { }
>  void __init yama_add_hooks(void)
>  {
>  	pr_info("Yama: becoming mindful.\n");
> -	security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), "yama");
> +	security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks));
>  	yama_init_sysctl();
>  }

--
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

[PATCH RFC 1/9] LSM: Add /sys/kernel/security/lsm

[Casey Schaufler]

On 3/17/2017 5:52 AM, Tetsuo Handa wrote:
> Casey Schaufler wrote:
>> Subject: [PATCH RFC 1/9] LSM: Add /sys/kernel/security/lsm
>>
>> This has been accepted into security next - provided for completeness
> Before going to "[PATCH RFC 5/9] LSM: General but not extreme module
> stacking", what do you think about below change?
>
> ----------------------------------------
>
> >From 8fe80e4b6a479a81d720571ad3b5979f6fd1e6ae Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Fri, 17 Mar 2017 21:34:51 +0900
> Subject: [PATCH] LSM: Pass module name via "union security_list_options".
>
> Commit d69dece5f5b6bc7a ("LSM: Add /sys/kernel/security/lsm") added
> "char *lsm" field to "struct security_hook_list". But it is currently
> never used after set at initialization time. While there is a plan that
> this field will be used by "LSM: General but not extreme module stacking"
> patch, there is no point with setting the same value to each element of
> "struct security_hook_list" array. We can keep "struct security_hook_list"
> 4 * sizeof(void *) bytes if we keep this field in "union
> security_list_options".
>
> The "char *" argument in security_add_hooks() was removed because we can
> fetch it from security_list_options"->module_name field. This implies
> that setting security_list_options"->module_name field is mandatory.

On further reflection, and spending a little time back
in the code, it turns out that this won't work at all.
The lsm field needs to be in the "struct security_hook_list"
so that hook processing that uses the module name such
as security_getprocattr has it. It wouldn't hurt to have
the module name in "union security_list_options", but
a list of module names doesn't help anything, either.


>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  include/linux/lsm_hooks.h  |  8 +++++---
>  security/apparmor/lsm.c    |  4 ++--
>  security/commoncap.c       |  4 ++--
>  security/loadpin/loadpin.c |  3 ++-
>  security/security.c        | 13 +++++++------
>  security/selinux/hooks.c   |  3 ++-
>  security/smack/smack_lsm.c |  3 ++-
>  security/tomoyo/tomoyo.c   |  3 ++-
>  security/yama/yama_lsm.c   |  3 ++-
>  9 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index a4193c3..d10b4f7 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -29,6 +29,8 @@
>  #include <linux/rculist.h>
>  
>  /**
> + * @module_name: the name of the security module
> + *
>   * Security hooks for program execution operations.
>   *
>   * @bprm_set_creds:
> @@ -1340,6 +1342,7 @@
>   */
>  
>  union security_list_options {
> +	const char *module_name;
>  	int (*binder_set_context_mgr)(struct task_struct *mgr);
>  	int (*binder_transaction)(struct task_struct *from,
>  					struct task_struct *to);
> @@ -1664,6 +1667,7 @@
>  };
>  
>  struct security_hook_heads {
> +	struct list_head module_name;
>  	struct list_head binder_set_context_mgr;
>  	struct list_head binder_transaction;
>  	struct list_head binder_transfer_binder;
> @@ -1886,7 +1890,6 @@ struct security_hook_list {
>  	struct list_head		list;
>  	struct list_head		*head;
>  	union security_list_options	hook;
> -	char				*lsm;
>  };
>  
>  /*
> @@ -1901,8 +1904,7 @@ struct security_hook_list {
>  extern struct security_hook_heads security_hook_heads;
>  extern char *lsm_names;
>  
> -extern void security_add_hooks(struct security_hook_list *hooks, int count,
> -				char *lsm);
> +extern void security_add_hooks(struct security_hook_list *hooks, int count);
>  
>  #ifdef CONFIG_SECURITY_SELINUX_DISABLE
>  /*
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index e287b69..dc21835 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -588,6 +588,7 @@ static int apparmor_task_setrlimit(struct task_struct *task,
>  }
>  
>  static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
> +	LSM_HOOK_INIT(module_name, "apparmor"),
>  	LSM_HOOK_INIT(ptrace_access_check, apparmor_ptrace_access_check),
>  	LSM_HOOK_INIT(ptrace_traceme, apparmor_ptrace_traceme),
>  	LSM_HOOK_INIT(capget, apparmor_capget),
> @@ -996,8 +997,7 @@ static int __init apparmor_init(void)
>  		aa_free_root_ns();
>  		goto buffers_out;
>  	}
> -	security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
> -				"apparmor");
> +	security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks));
>  
>  	/* Report that AppArmor successfully initialized */
>  	apparmor_initialized = 1;
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 7abebd7..1ced4ee 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -1072,6 +1072,7 @@ int cap_mmap_file(struct file *file, unsigned long reqprot,
>  #ifdef CONFIG_SECURITY
>  
>  struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
> +	LSM_HOOK_INIT(module_name, "capability"),
>  	LSM_HOOK_INIT(capable, cap_capable),
>  	LSM_HOOK_INIT(settime, cap_settime),
>  	LSM_HOOK_INIT(ptrace_access_check, cap_ptrace_access_check),
> @@ -1094,8 +1095,7 @@ struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
>  
>  void __init capability_add_hooks(void)
>  {
> -	security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks),
> -				"capability");
> +	security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks));
>  }
>  
>  #endif /* CONFIG_SECURITY */
> diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
> index dbe6efd..c126c74 100644
> --- a/security/loadpin/loadpin.c
> +++ b/security/loadpin/loadpin.c
> @@ -175,6 +175,7 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
>  }
>  
>  static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
> +	LSM_HOOK_INIT(module_name, "loadpin"),
>  	LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
>  	LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
>  };
> @@ -182,7 +183,7 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
>  void __init loadpin_add_hooks(void)
>  {
>  	pr_info("ready to pin (currently %sabled)", enabled ? "en" : "dis");
> -	security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
> +	security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks));
>  }
>  
>  /* Should not be mutable after boot, so not listed in sysfs (perm == 0). */
> diff --git a/security/security.c b/security/security.c
> index 5b6cdd0..ae57a84 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -79,7 +79,7 @@ static int __init choose_lsm(char *str)
>  }
>  __setup("security=", choose_lsm);
>  
> -static int lsm_append(char *new, char **result)
> +static int lsm_append(const char *new, char **result)
>  {
>  	char *cp;
>  
> @@ -118,20 +118,20 @@ int __init security_module_enable(const char *module)
>   * security_add_hooks - Add a modules hooks to the hook lists.
>   * @hooks: the hooks to add
>   * @count: the number of hooks to add
> - * @lsm: the name of the security module
>   *
>   * Each LSM has to register its hooks with the infrastructure.
>   */
> -void __init security_add_hooks(struct security_hook_list *hooks, int count,
> -				char *lsm)
> +void __init security_add_hooks(struct security_hook_list *hooks, int count)
>  {
>  	int i;
> +	const char *lsm = NULL;
>  
>  	for (i = 0; i < count; i++) {
> -		hooks[i].lsm = lsm;
>  		list_add_tail_rcu(&hooks[i].list, hooks[i].head);
> +		if (hooks[i].head == &security_hook_heads.module_name)
> +			lsm = hooks[i].hook.module_name;
>  	}
> -	if (lsm_append(lsm, &lsm_names) < 0)
> +	if (!lsm || lsm_append(lsm, &lsm_names) < 0)
>  		panic("%s - Cannot get early memory.\n", __func__);
>  }
>  
> @@ -1634,6 +1634,7 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule,
>  #endif /* CONFIG_AUDIT */
>  
>  struct security_hook_heads security_hook_heads __lsm_ro_after_init = {
> +	.module_name = LIST_HEAD_INIT(security_hook_heads.module_name),
>  	.binder_set_context_mgr =
>  		LIST_HEAD_INIT(security_hook_heads.binder_set_context_mgr),
>  	.binder_transaction =
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index d37a723..cd7befe 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6124,6 +6124,7 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer)
>  #endif
>  
>  static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> +	LSM_HOOK_INIT(module_name, "selinux"),
>  	LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
>  	LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
>  	LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
> @@ -6366,7 +6367,7 @@ static __init int selinux_init(void)
>  					    0, SLAB_PANIC, NULL);
>  	avc_init();
>  
> -	security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
> +	security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
>  
>  	if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
>  		panic("SELinux: Unable to register AVC netcache callback\n");
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 927e60e..f2457b8 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4634,6 +4634,7 @@ static int smack_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
>  }
>  
>  static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
> +	LSM_HOOK_INIT(module_name, "smack"),
>  	LSM_HOOK_INIT(ptrace_access_check, smack_ptrace_access_check),
>  	LSM_HOOK_INIT(ptrace_traceme, smack_ptrace_traceme),
>  	LSM_HOOK_INIT(syslog, smack_syslog),
> @@ -4850,7 +4851,7 @@ static __init int smack_init(void)
>  	/*
>  	 * Register with LSM
>  	 */
> -	security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack");
> +	security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks));
>  
>  	return 0;
>  }
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 0de150d..6ba6139 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -488,6 +488,7 @@ static int tomoyo_socket_sendmsg(struct socket *sock, struct msghdr *msg,
>   * registering TOMOYO.
>   */
>  static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
> +	LSM_HOOK_INIT(module_name, "tomoyo"),
>  	LSM_HOOK_INIT(task_alloc, tomoyo_task_alloc),
>  	LSM_HOOK_INIT(task_free, tomoyo_task_free),
>  	LSM_HOOK_INIT(bprm_set_creds, tomoyo_bprm_set_creds),
> @@ -534,7 +535,7 @@ static int __init tomoyo_init(void)
>  	if (!security_module_enable("tomoyo"))
>  		return 0;
>  	/* register ourselves with the security framework */
> -	security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo");
> +	security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks));
>  	printk(KERN_INFO "TOMOYO Linux initialized\n");
>  	current->security = &ts;
>  	tomoyo_mm_init();
> diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
> index 8298e09..3290d33 100644
> --- a/security/yama/yama_lsm.c
> +++ b/security/yama/yama_lsm.c
> @@ -429,6 +429,7 @@ int yama_ptrace_traceme(struct task_struct *parent)
>  }
>  
>  static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
> +	LSM_HOOK_INIT(module_name, "yama"),
>  	LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
>  	LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
>  	LSM_HOOK_INIT(task_prctl, yama_task_prctl),
> @@ -485,6 +486,6 @@ static inline void yama_init_sysctl(void) { }
>  void __init yama_add_hooks(void)
>  {
>  	pr_info("Yama: becoming mindful.\n");
> -	security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), "yama");
> +	security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks));
>  	yama_init_sysctl();
>  }

--
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

[PATCH RFC 1/9] LSM: Add /sys/kernel/security/lsm

[Tetsuo Handa]

Casey Schaufler wrote:
> On 3/17/2017 5:52 AM, Tetsuo Handa wrote:
> > Casey Schaufler wrote:
> >> Subject: [PATCH RFC 1/9] LSM: Add /sys/kernel/security/lsm
> >>
> >> This has been accepted into security next - provided for completeness
> > Before going to "[PATCH RFC 5/9] LSM: General but not extreme module
> > stacking", what do you think about below change?
> >
> > ----------------------------------------
> >
> > >From 8fe80e4b6a479a81d720571ad3b5979f6fd1e6ae Mon Sep 17 00:00:00 2001
> > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Date: Fri, 17 Mar 2017 21:34:51 +0900
> > Subject: [PATCH] LSM: Pass module name via "union security_list_options".
> >
> > Commit d69dece5f5b6bc7a ("LSM: Add /sys/kernel/security/lsm") added
> > "char *lsm" field to "struct security_hook_list". But it is currently
> > never used after set at initialization time. While there is a plan that
> > this field will be used by "LSM: General but not extreme module stacking"
> > patch, there is no point with setting the same value to each element of
> > "struct security_hook_list" array. We can keep "struct security_hook_list"
> > 4 * sizeof(void *) bytes if we keep this field in "union
> > security_list_options".
> >
> > The "char *" argument in security_add_hooks() was removed because we can
> > fetch it from security_list_options"->module_name field. This implies
> > that setting security_list_options"->module_name field is mandatory.
> 
> On further reflection, and spending a little time back
> in the code, it turns out that this won't work at all.
> The lsm field needs to be in the "struct security_hook_list"
> so that hook processing that uses the module name such
> as security_getprocattr has it. It wouldn't hurt to have
> the module name in "union security_list_options", but
> a list of module names doesn't help anything, either.
> 

Oops. I didn't expect you to use a list of module names for
getprocattr()/setprocattr(). I assumed you pass module name to
individual module and let individual module return a value or -ENOENT.

But looking at PATCH 5/9, do we really want to use "context" format?
TOMOYO uses $name=$value encoding which can include arbitrary characters
and thus allows simple tokenizing using a whitespace like

  lsm1=v1 lsm2=v2 lsm3=v3

but you are trying to introduce

  lsm1='v1'lsm2='v2'lsm3='v3'

format at the cost of giving up ability to include arbitrary characters
(e.g. '\n'), aren't you?

If userspace has to concatenate strings like

  lsm1='v1'lsm2='v2'lsm3='v3'

and write to single file at one time like

  echo lsm1='v1'lsm2='v2'lsm3='v3' > /proc/$pid/attr/context

, userspace can simply do

  echo $v1 > /proc/$pid/attr/$lsm1/$name
  echo $v2 > /proc/$pid/attr/$lsm2/$name
  echo $v3 > /proc/$pid/attr/$lsm3/$name

which will not need to exclude characters like '\n'.

If there were hundreds of modules, the overhead might matter.
But given that there will be only few modules which use these
interfaces, I think userspace can tolerate such multiple writes.
--
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

[PATCH RFC 1/9] LSM: Add /sys/kernel/security/lsm

[John Johansen]

On 03/18/2017 12:53 AM, Tetsuo Handa wrote:
> Casey Schaufler wrote:
>> On 3/17/2017 5:52 AM, Tetsuo Handa wrote:
>>> Casey Schaufler wrote:
>>>> Subject: [PATCH RFC 1/9] LSM: Add /sys/kernel/security/lsm
>>>>
>>>> This has been accepted into security next - provided for completeness
>>> Before going to "[PATCH RFC 5/9] LSM: General but not extreme module
>>> stacking", what do you think about below change?
>>>
>>> ----------------------------------------
>>>
>>> >From 8fe80e4b6a479a81d720571ad3b5979f6fd1e6ae Mon Sep 17 00:00:00 2001
>>> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>>> Date: Fri, 17 Mar 2017 21:34:51 +0900
>>> Subject: [PATCH] LSM: Pass module name via "union security_list_options".
>>>
>>> Commit d69dece5f5b6bc7a ("LSM: Add /sys/kernel/security/lsm") added
>>> "char *lsm" field to "struct security_hook_list". But it is currently
>>> never used after set at initialization time. While there is a plan that
>>> this field will be used by "LSM: General but not extreme module stacking"
>>> patch, there is no point with setting the same value to each element of
>>> "struct security_hook_list" array. We can keep "struct security_hook_list"
>>> 4 * sizeof(void *) bytes if we keep this field in "union
>>> security_list_options".
>>>
>>> The "char *" argument in security_add_hooks() was removed because we can
>>> fetch it from security_list_options"->module_name field. This implies
>>> that setting security_list_options"->module_name field is mandatory.
>>
>> On further reflection, and spending a little time back
>> in the code, it turns out that this won't work at all.
>> The lsm field needs to be in the "struct security_hook_list"
>> so that hook processing that uses the module name such
>> as security_getprocattr has it. It wouldn't hurt to have
>> the module name in "union security_list_options", but
>> a list of module names doesn't help anything, either.
>>
> 
> Oops. I didn't expect you to use a list of module names for
> getprocattr()/setprocattr(). I assumed you pass module name to
> individual module and let individual module return a value or -ENOENT.
> 
> But looking at PATCH 5/9, do we really want to use "context" format?
> TOMOYO uses $name=$value encoding which can include arbitrary characters
> and thus allows simple tokenizing using a whitespace like
> 
If its arbitrary characters does that not include spaces? In which case
simple tokenizing based on whitespacing would not work.

Regardless as much as I would like to go back and change it, the apparmor
context stuff really does use arbitrary characters including ws, quotes
and if we are talking writes \000 is used as well.

We can update libapparmor to hit a different interface for writing,
and work with lxc and a few other projects that are using the interface
directly, so that stop hitting the proc interface.

But we would very much like the read side to work correctly. The apparmor
context sadly currently always includes ws. So simple ws tokenizing won't
work, but I am open to changing that. Individual files would work better
for apparmor, but that has its own issues.

So I am some what ambivalent about the interface chosen. As long as
we can standardize on something, I'll make the effort on the apparmor
side to make sure it works. It may mean apparmor has to break some
things but its not like apparmor doesn't already have work arounds in
place that can be used to remove the arbitrary character issue, we just
have to force people to add them to their existing policy.

>   lsm1=v1 lsm2=v2 lsm3=v3
> 
> but you are trying to introduce
> 
>   lsm1='v1'lsm2='v2'lsm3='v3'
> 
> format at the cost of giving up ability to include arbitrary characters
> (e.g. '\n'), aren't you?
> 
> If userspace has to concatenate strings like
> 
>   lsm1='v1'lsm2='v2'lsm3='v3'
> 
> and write to single file at one time like
> 
>   echo lsm1='v1'lsm2='v2'lsm3='v3' > /proc/$pid/attr/context
> 
> , userspace can simply do
> 
>   echo $v1 > /proc/$pid/attr/$lsm1/$name
>   echo $v2 > /proc/$pid/attr/$lsm2/$name
>   echo $v3 > /proc/$pid/attr/$lsm3/$name
> 
> which will not need to exclude characters like '\n'.
> 
sure but then userspace also has to iterate those files, not such an
issue for writes, but for reads the the application is going to have
to try to organize the data into some form.

> If there were hundreds of modules, the overhead might matter.
> But given that there will be only few modules which use these
> interfaces, I think userspace can tolerate such multiple writes.
> 
I'm not worried about the writes, those will largely be coming from
the tools for a given project. It is the generic tools doing reads (ps,
top, ...) that should have a standard format to work with, and
should not have to worry about the details of each lsm. This is what
makes me lean in the direction of the single file.

--
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

[PATCH RFC 1/9] LSM: Add /sys/kernel/security/lsm

[Casey Schaufler]

On 3/18/2017 12:53 AM, Tetsuo Handa wrote:
> Casey Schaufler wrote:
>> On 3/17/2017 5:52 AM, Tetsuo Handa wrote:
>>> Casey Schaufler wrote:
>>>> Subject: [PATCH RFC 1/9] LSM: Add /sys/kernel/security/lsm
>>>>
>>>> This has been accepted into security next - provided for completeness
>>> Before going to "[PATCH RFC 5/9] LSM: General but not extreme module
>>> stacking", what do you think about below change?
>>>
>>> ----------------------------------------
>>>
>>> >From 8fe80e4b6a479a81d720571ad3b5979f6fd1e6ae Mon Sep 17 00:00:00 2001
>>> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>>> Date: Fri, 17 Mar 2017 21:34:51 +0900
>>> Subject: [PATCH] LSM: Pass module name via "union security_list_options".
>>>
>>> Commit d69dece5f5b6bc7a ("LSM: Add /sys/kernel/security/lsm") added
>>> "char *lsm" field to "struct security_hook_list". But it is currently
>>> never used after set at initialization time. While there is a plan that
>>> this field will be used by "LSM: General but not extreme module stacking"
>>> patch, there is no point with setting the same value to each element of
>>> "struct security_hook_list" array. We can keep "struct security_hook_list"
>>> 4 * sizeof(void *) bytes if we keep this field in "union
>>> security_list_options".
>>>
>>> The "char *" argument in security_add_hooks() was removed because we can
>>> fetch it from security_list_options"->module_name field. This implies
>>> that setting security_list_options"->module_name field is mandatory.
>> On further reflection, and spending a little time back
>> in the code, it turns out that this won't work at all.
>> The lsm field needs to be in the "struct security_hook_list"
>> so that hook processing that uses the module name such
>> as security_getprocattr has it. It wouldn't hurt to have
>> the module name in "union security_list_options", but
>> a list of module names doesn't help anything, either.
>>
> Oops. I didn't expect you to use a list of module names for
> getprocattr()/setprocattr(). I assumed you pass module name to
> individual module and let individual module return a value or -ENOENT.

Either way works, it's just a bit cleaner for the infrastructure
to call the one module that can provide the information than to
loop through calling everyone unnecessarily.

Yes, I've coded it both ways.

> But looking at PATCH 5/9, do we really want to use "context" format?
> TOMOYO uses $name=$value encoding which can include arbitrary characters
> and thus allows simple tokenizing using a whitespace like
>
>   lsm1=v1 lsm2=v2 lsm3=v3
>
> but you are trying to introduce
>
>   lsm1='v1'lsm2='v2'lsm3='v3'
>
> format at the cost of giving up ability to include arbitrary characters
> (e.g. '\n'), aren't you?

I am open (yet again, I've asked for input several times before)
to suggestions, but there has to be a way to represent the complete
"context" in a finite way. This is my best effort at doing so.

> If userspace has to concatenate strings like
>
>   lsm1='v1'lsm2='v2'lsm3='v3'
>
> and write to single file at one time like
>
>   echo lsm1='v1'lsm2='v2'lsm3='v3' > /proc/$pid/attr/context
>
> , userspace can simply do
>
>   echo $v1 > /proc/$pid/attr/$lsm1/$name
>   echo $v2 > /proc/$pid/attr/$lsm2/$name
>   echo $v3 > /proc/$pid/attr/$lsm3/$name

This isn't atomic, and while it is possible to
do it thus, there can be races in global policy.
Also, it would require user-space to know about
the available modules, and part of what
/proc/$pid/attr/context gets you is the ability
to fetch the process security data the same way
in all cases. Very useful for "id".

> which will not need to exclude characters like '\n'.

I can't see ps, ls or id being at all happy
with security attributes containing newlines,
formfeeds, bells or backspaces. There's a reason
Smack labels have a restricted character set.

> If there were hundreds of modules, the overhead might matter.
> But given that there will be only few modules which use these
> interfaces, I think userspace can tolerate such multiple writes.
>
Multiple writes aren't atomic.
I'm much more concerned with multiple reads.
lsm-1='data-1',...,lsm-n='data-n'
is as good a representation as I can come up with.
Please propose a better one. I don't think that 
anything requiring sophisticated parsing is viable.

--
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

[PATCH RFC 1/9] LSM: Add /sys/kernel/security/lsm

[Tetsuo Handa]

Casey Schaufler wrote:
> On 3/18/2017 12:53 AM, Tetsuo Handa wrote:
> > But looking at PATCH 5/9, do we really want to use "context" format?
> > TOMOYO uses $name=$value encoding which can include arbitrary characters
> > and thus allows simple tokenizing using a whitespace like
> >
> >   lsm1=v1 lsm2=v2 lsm3=v3
> >
> > but you are trying to introduce
> >
> >   lsm1='v1'lsm2='v2'lsm3='v3'
> >
> > format at the cost of giving up ability to include arbitrary characters
> > (e.g. '\n'), aren't you?
> 
> I am open (yet again, I've asked for input several times before)
> to suggestions, but there has to be a way to represent the complete
> "context" in a finite way. This is my best effort at doing so.
> 
> > If userspace has to concatenate strings like
> >
> >   lsm1='v1'lsm2='v2'lsm3='v3'
> >
> > and write to single file at one time like
> >
> >   echo lsm1='v1'lsm2='v2'lsm3='v3' > /proc/$pid/attr/context
> >
> > , userspace can simply do
> >
> >   echo $v1 > /proc/$pid/attr/$lsm1/$name
> >   echo $v2 > /proc/$pid/attr/$lsm2/$name
> >   echo $v3 > /proc/$pid/attr/$lsm3/$name
> 
> This isn't atomic, and while it is possible to
> do it thus, there can be races in global policy.

If I didn't overlook something, PATCH 5/9 does not make
security_setprocattr() do something like

  cred = prepare_creds();
  for_each_module_which_matches_given_names() {
    cred->per_module_data = new_value_for_that_module;
  }
  commit_creds(cred);

. Therefore, while copying data to /proc interface is atomic,
processing the copied data is not atomic. Same thing is possible
by doing

  base_fd = open("/proc/$pid/attr/");
  fd_lsm1 = openat(base_fd, "$lsm1/$name");
  write(fd_lsm1, "$v1");
  close(fd_lsm1);
  fd_lsm2 = openat(base_fd, "$lsm2/$name");
  write(fd_lsm2, "$v2");
  close(fd_lsm2);
  fd_lsm3 = openat(base_fd, "$lsm3/$name");
  write(fd_lsm3, "$v3");
  close(fd_lsm3);
  close(base_fd);

.

> Also, it would require user-space to know about
> the available modules, and part of what
> /proc/$pid/attr/context gets you is the ability
> to fetch the process security data the same way
> in all cases. Very useful for "id".
> 
> > which will not need to exclude characters like '\n'.
> 
> I can't see ps, ls or id being at all happy
> with security attributes containing newlines,
> formfeeds, bells or backspaces. There's a reason
> Smack labels have a restricted character set.
> 
> > If there were hundreds of modules, the overhead might matter.
> > But given that there will be only few modules which use these
> > interfaces, I think userspace can tolerate such multiple writes.
> >
> Multiple writes aren't atomic.

PATCH 5/9 does multiple writes which aren't atomic.

> I'm much more concerned with multiple reads.
> lsm-1='data-1',...,lsm-n='data-n'
> is as good a representation as I can come up with.
> Please propose a better one. I don't think that 
> anything requiring sophisticated parsing is viable.

TOMOYO uses /sys/kernel/security/tomoyo/.process_status for reading all
process's information without opening every /proc/$pid/attr/ interface.

If you put priority on readers, I think prctl() interface might fit better.
When there are hundreds/thousands of processes, opening each
/proc/$pid/attr/context interface only for one read() request is a big
overhead.
--
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