diff mbox series

[v5,3/8] proc: Use lsmids instead of lsm names for attrs

Message ID 20230109180717.58855-4-casey@schaufler-ca.com
State New
Headers
Series LSM: Three basic syscalls |

Commit Message

Casey Schaufler Jan. 9, 2023, 6:07 p.m. UTC 
  Use the LSM ID number instead of the LSM name to identify which
security module's attibute data should be shown in /proc/self/attr.
The security_[gs]etprocattr() functions have been changed to expect
the LSM ID. The change from a string comparison to an integer comparison
in these functions will provide a minor performance improvement.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Cc: linux-fsdevel@vger.kernel.org
---
 fs/proc/base.c           | 29 +++++++++++++++--------------
 fs/proc/internal.h       |  2 +-
 include/linux/security.h | 11 +++++------
 security/security.c      | 11 +++++------
 4 files changed, 26 insertions(+), 27 deletions(-)

Comments

Paul Moore Jan. 11, 2023, 9:01 p.m. UTC | #1 
On Mon, Jan 9, 2023 at 1:09 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Use the LSM ID number instead of the LSM name to identify which
> security module's attibute data should be shown in /proc/self/attr.
> The security_[gs]etprocattr() functions have been changed to expect
> the LSM ID. The change from a string comparison to an integer comparison
> in these functions will provide a minor performance improvement.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Cc: linux-fsdevel@vger.kernel.org
> ---
>  fs/proc/base.c           | 29 +++++++++++++++--------------
>  fs/proc/internal.h       |  2 +-
>  include/linux/security.h | 11 +++++------
>  security/security.c      | 11 +++++------
>  4 files changed, 26 insertions(+), 27 deletions(-)

...

> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 9e479d7d202b..9328b6b07dfc 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2837,27 +2838,27 @@ static const struct inode_operations proc_##LSM##_attr_dir_inode_ops = { \
>
>  #ifdef CONFIG_SECURITY_SMACK
>  static const struct pid_entry smack_attr_dir_stuff[] = {
> -       ATTR("smack", "current",        0666),
> +       ATTR(LSM_ID_SMACK, "current",   0666),
>  };
>  LSM_DIR_OPS(smack);
>  #endif
>
>  #ifdef CONFIG_SECURITY_APPARMOR
>  static const struct pid_entry apparmor_attr_dir_stuff[] = {
> -       ATTR("apparmor", "current",     0666),
> -       ATTR("apparmor", "prev",        0444),
> -       ATTR("apparmor", "exec",        0666),
> +       ATTR(LSM_ID_APPARMOR, "current",        0666),
> +       ATTR(LSM_ID_APPARMOR, "prev",           0444),
> +       ATTR(LSM_ID_APPARMOR, "exec",           0666),
>  };
>  LSM_DIR_OPS(apparmor);
>  #endif
>
>  static const struct pid_entry attr_dir_stuff[] = {
> -       ATTR(NULL, "current",           0666),
> -       ATTR(NULL, "prev",              0444),
> -       ATTR(NULL, "exec",              0666),
> -       ATTR(NULL, "fscreate",          0666),
> -       ATTR(NULL, "keycreate",         0666),
> -       ATTR(NULL, "sockcreate",        0666),
> +       ATTR(0, "current",      0666),
> +       ATTR(0, "prev",         0444),
> +       ATTR(0, "exec",         0666),
> +       ATTR(0, "fscreate",     0666),
> +       ATTR(0, "keycreate",    0666),
> +       ATTR(0, "sockcreate",   0666),

See the discussion in patch 1/8, we should use a macro instead of a 0
here (although the exact macro definition is very much up for
discussion):

  ATTR(LSM_ID_UNDEF, "current", 0666),

--
paul-moore.com
Casey Schaufler Jan. 12, 2023, 12:37 a.m. UTC | #2 
On 1/11/2023 1:01 PM, Paul Moore wrote:
> On Mon, Jan 9, 2023 at 1:09 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> Use the LSM ID number instead of the LSM name to identify which
>> security module's attibute data should be shown in /proc/self/attr.
>> The security_[gs]etprocattr() functions have been changed to expect
>> the LSM ID. The change from a string comparison to an integer comparison
>> in these functions will provide a minor performance improvement.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> Cc: linux-fsdevel@vger.kernel.org
>> ---
>>  fs/proc/base.c           | 29 +++++++++++++++--------------
>>  fs/proc/internal.h       |  2 +-
>>  include/linux/security.h | 11 +++++------
>>  security/security.c      | 11 +++++------
>>  4 files changed, 26 insertions(+), 27 deletions(-)
> ..
>
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index 9e479d7d202b..9328b6b07dfc 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -2837,27 +2838,27 @@ static const struct inode_operations proc_##LSM##_attr_dir_inode_ops = { \
>>
>>  #ifdef CONFIG_SECURITY_SMACK
>>  static const struct pid_entry smack_attr_dir_stuff[] = {
>> -       ATTR("smack", "current",        0666),
>> +       ATTR(LSM_ID_SMACK, "current",   0666),
>>  };
>>  LSM_DIR_OPS(smack);
>>  #endif
>>
>>  #ifdef CONFIG_SECURITY_APPARMOR
>>  static const struct pid_entry apparmor_attr_dir_stuff[] = {
>> -       ATTR("apparmor", "current",     0666),
>> -       ATTR("apparmor", "prev",        0444),
>> -       ATTR("apparmor", "exec",        0666),
>> +       ATTR(LSM_ID_APPARMOR, "current",        0666),
>> +       ATTR(LSM_ID_APPARMOR, "prev",           0444),
>> +       ATTR(LSM_ID_APPARMOR, "exec",           0666),
>>  };
>>  LSM_DIR_OPS(apparmor);
>>  #endif
>>
>>  static const struct pid_entry attr_dir_stuff[] = {
>> -       ATTR(NULL, "current",           0666),
>> -       ATTR(NULL, "prev",              0444),
>> -       ATTR(NULL, "exec",              0666),
>> -       ATTR(NULL, "fscreate",          0666),
>> -       ATTR(NULL, "keycreate",         0666),
>> -       ATTR(NULL, "sockcreate",        0666),
>> +       ATTR(0, "current",      0666),
>> +       ATTR(0, "prev",         0444),
>> +       ATTR(0, "exec",         0666),
>> +       ATTR(0, "fscreate",     0666),
>> +       ATTR(0, "keycreate",    0666),
>> +       ATTR(0, "sockcreate",   0666),
> See the discussion in patch 1/8, we should use a macro instead of a 0
> here (although the exact macro definition is very much up for
> discussion):
>
>   ATTR(LSM_ID_UNDEF, "current", 0666),

Or LSM_ID_NALSMID, or whatever. Agreed.

>
> --
> paul-moore.com
diff mbox series

Patch

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 9e479d7d202b..9328b6b07dfc 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -96,6 +96,7 @@ 
 #include <linux/time_namespace.h>
 #include <linux/resctrl.h>
 #include <linux/cn_proc.h>
+#include <uapi/linux/lsm.h>
 #include <trace/events/oom.h>
 #include "internal.h"
 #include "fd.h"
@@ -145,10 +146,10 @@  struct pid_entry {
 	NOD(NAME, (S_IFREG|(MODE)),			\
 		NULL, &proc_single_file_operations,	\
 		{ .proc_show = show } )
-#define ATTR(LSM, NAME, MODE)				\
+#define ATTR(LSMID, NAME, MODE)				\
 	NOD(NAME, (S_IFREG|(MODE)),			\
 		NULL, &proc_pid_attr_operations,	\
-		{ .lsm = LSM })
+		{ .lsmid = LSMID })
 
 /*
  * Count the number of hardlinks for the pid_entry table, excluding the .
@@ -2730,7 +2731,7 @@  static ssize_t proc_pid_attr_read(struct file * file, char __user * buf,
 	if (!task)
 		return -ESRCH;
 
-	length = security_getprocattr(task, PROC_I(inode)->op.lsm,
+	length = security_getprocattr(task, PROC_I(inode)->op.lsmid,
 				      file->f_path.dentry->d_name.name,
 				      &p);
 	put_task_struct(task);
@@ -2788,7 +2789,7 @@  static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
 	if (rv < 0)
 		goto out_free;
 
-	rv = security_setprocattr(PROC_I(inode)->op.lsm,
+	rv = security_setprocattr(PROC_I(inode)->op.lsmid,
 				  file->f_path.dentry->d_name.name, page,
 				  count);
 	mutex_unlock(&current->signal->cred_guard_mutex);
@@ -2837,27 +2838,27 @@  static const struct inode_operations proc_##LSM##_attr_dir_inode_ops = { \
 
 #ifdef CONFIG_SECURITY_SMACK
 static const struct pid_entry smack_attr_dir_stuff[] = {
-	ATTR("smack", "current",	0666),
+	ATTR(LSM_ID_SMACK, "current",	0666),
 };
 LSM_DIR_OPS(smack);
 #endif
 
 #ifdef CONFIG_SECURITY_APPARMOR
 static const struct pid_entry apparmor_attr_dir_stuff[] = {
-	ATTR("apparmor", "current",	0666),
-	ATTR("apparmor", "prev",	0444),
-	ATTR("apparmor", "exec",	0666),
+	ATTR(LSM_ID_APPARMOR, "current",	0666),
+	ATTR(LSM_ID_APPARMOR, "prev",		0444),
+	ATTR(LSM_ID_APPARMOR, "exec",		0666),
 };
 LSM_DIR_OPS(apparmor);
 #endif
 
 static const struct pid_entry attr_dir_stuff[] = {
-	ATTR(NULL, "current",		0666),
-	ATTR(NULL, "prev",		0444),
-	ATTR(NULL, "exec",		0666),
-	ATTR(NULL, "fscreate",		0666),
-	ATTR(NULL, "keycreate",		0666),
-	ATTR(NULL, "sockcreate",	0666),
+	ATTR(0, "current",	0666),
+	ATTR(0, "prev",		0444),
+	ATTR(0, "exec",		0666),
+	ATTR(0, "fscreate",	0666),
+	ATTR(0, "keycreate",	0666),
+	ATTR(0, "sockcreate",	0666),
 #ifdef CONFIG_SECURITY_SMACK
 	DIR("smack",			0555,
 	    proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index b701d0207edf..18db9722c81b 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -92,7 +92,7 @@  union proc_op {
 	int (*proc_show)(struct seq_file *m,
 		struct pid_namespace *ns, struct pid *pid,
 		struct task_struct *task);
-	const char *lsm;
+	int lsmid;
 };
 
 struct proc_inode {
diff --git a/include/linux/security.h b/include/linux/security.h
index 33ed1860b96f..2d09e818a7d1 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -475,10 +475,9 @@  int security_sem_semctl(struct kern_ipc_perm *sma, int cmd);
 int security_sem_semop(struct kern_ipc_perm *sma, struct sembuf *sops,
 			unsigned nsops, int alter);
 void security_d_instantiate(struct dentry *dentry, struct inode *inode);
-int security_getprocattr(struct task_struct *p, const char *lsm, const char *name,
+int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
 			 char **value);
-int security_setprocattr(const char *lsm, const char *name, void *value,
-			 size_t size);
+int security_setprocattr(int lsmid, const char *name, void *value, size_t size);
 int security_netlink_send(struct sock *sk, struct sk_buff *skb);
 int security_ismaclabel(const char *name);
 int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
@@ -1346,14 +1345,14 @@  static inline void security_d_instantiate(struct dentry *dentry,
 					  struct inode *inode)
 { }
 
-static inline int security_getprocattr(struct task_struct *p, const char *lsm,
+static inline int security_getprocattr(struct task_struct *p, int lsmid,
 				       const char *name, char **value)
 {
 	return -EINVAL;
 }
 
-static inline int security_setprocattr(const char *lsm, char *name,
-				       void *value, size_t size)
+static inline int security_setprocattr(int lsmid, char *name, void *value,
+				       size_t size)
 {
 	return -EINVAL;
 }
diff --git a/security/security.c b/security/security.c
index a590fa98ddd6..a0f4af2da5f3 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2169,26 +2169,25 @@  void security_d_instantiate(struct dentry *dentry, struct inode *inode)
 }
 EXPORT_SYMBOL(security_d_instantiate);
 
-int security_getprocattr(struct task_struct *p, const char *lsm,
-			 const char *name, char **value)
+int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
+			 char **value)
 {
 	struct security_hook_list *hp;
 
 	hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
-		if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
+		if (lsmid != 0 && lsmid != hp->lsmid->id)
 			continue;
 		return hp->hook.getprocattr(p, name, value);
 	}
 	return LSM_RET_DEFAULT(getprocattr);
 }
 
-int security_setprocattr(const char *lsm, const char *name, void *value,
-			 size_t size)
+int security_setprocattr(int lsmid, const char *name, void *value, size_t size)
 {
 	struct security_hook_list *hp;
 
 	hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
-		if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
+		if (lsmid != 0 && lsmid != hp->lsmid->id)
 			continue;
 		return hp->hook.setprocattr(name, value, size);
 	}