2023-06-29 20:36:23

by Casey Schaufler

[permalink] [raw]
Subject: [PATCH v12 03/11] proc: Use lsmids instead of lsm names for attrs

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 <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
Reviewed-by: Serge Hallyn <[email protected]>
Cc: [email protected]
---
fs/proc/base.c | 29 +++++++++++++++--------------
fs/proc/internal.h | 2 +-
include/linux/security.h | 11 +++++------
security/security.c | 15 +++++++--------
4 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 05452c3b9872..f999bb5c497b 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -97,6 +97,7 @@
#include <linux/resctrl.h>
#include <linux/cn_proc.h>
#include <linux/ksm.h>
+#include <uapi/linux/lsm.h>
#include <trace/events/oom.h>
#include "internal.h"
#include "fd.h"
@@ -146,10 +147,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(LSM_ID_UNDEF, "current", 0666),
+ ATTR(LSM_ID_UNDEF, "prev", 0444),
+ ATTR(LSM_ID_UNDEF, "exec", 0666),
+ ATTR(LSM_ID_UNDEF, "fscreate", 0666),
+ ATTR(LSM_ID_UNDEF, "keycreate", 0666),
+ ATTR(LSM_ID_UNDEF, "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 9dda7e54b2d0..a889d9ef9584 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 569b1d8ab002..945101b0d404 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -470,10 +470,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);
@@ -1332,14 +1331,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 5a699e47478b..d942b0c8e32f 100644
--- a/security/security.c
+++ b/security/security.c
@@ -3801,7 +3801,7 @@ EXPORT_SYMBOL(security_d_instantiate);
/**
* security_getprocattr() - Read an attribute for a task
* @p: the task
- * @lsm: LSM name
+ * @lsmid: LSM identification
* @name: attribute name
* @value: attribute value
*
@@ -3809,13 +3809,13 @@ EXPORT_SYMBOL(security_d_instantiate);
*
* Return: Returns the length of @value on success, a negative value otherwise.
*/
-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->name))
+ if (lsmid != 0 && lsmid != hp->lsmid->id)
continue;
return hp->hook.getprocattr(p, name, value);
}
@@ -3824,7 +3824,7 @@ int security_getprocattr(struct task_struct *p, const char *lsm,

/**
* security_setprocattr() - Set an attribute for a task
- * @lsm: LSM name
+ * @lsmid: LSM identification
* @name: attribute name
* @value: attribute value
* @size: attribute value size
@@ -3834,13 +3834,12 @@ int security_getprocattr(struct task_struct *p, const char *lsm,
*
* Return: Returns bytes written on success, a negative value otherwise.
*/
-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->name))
+ if (lsmid != 0 && lsmid != hp->lsmid->id)
continue;
return hp->hook.setprocattr(name, value, size);
}
--
2.40.1



2023-07-11 15:50:51

by Mickaël Salaün

[permalink] [raw]
Subject: Re: [PATCH v12 03/11] proc: Use lsmids instead of lsm names for attrs


On 29/06/2023 21:55, Casey Schaufler 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 <[email protected]>
> Reviewed-by: Kees Cook <[email protected]>
> Reviewed-by: Serge Hallyn <[email protected]>
> Cc: [email protected]

Reviewed-by: Mickaël Salaün <[email protected]>

> ---
> fs/proc/base.c | 29 +++++++++++++++--------------
> fs/proc/internal.h | 2 +-
> include/linux/security.h | 11 +++++------
> security/security.c | 15 +++++++--------
> 4 files changed, 28 insertions(+), 29 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 05452c3b9872..f999bb5c497b 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -97,6 +97,7 @@
> #include <linux/resctrl.h>
> #include <linux/cn_proc.h>
> #include <linux/ksm.h>
> +#include <uapi/linux/lsm.h>
> #include <trace/events/oom.h>
> #include "internal.h"
> #include "fd.h"
> @@ -146,10 +147,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(LSM_ID_UNDEF, "current", 0666),
> + ATTR(LSM_ID_UNDEF, "prev", 0444),
> + ATTR(LSM_ID_UNDEF, "exec", 0666),
> + ATTR(LSM_ID_UNDEF, "fscreate", 0666),
> + ATTR(LSM_ID_UNDEF, "keycreate", 0666),
> + ATTR(LSM_ID_UNDEF, "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 9dda7e54b2d0..a889d9ef9584 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 569b1d8ab002..945101b0d404 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -470,10 +470,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);
> @@ -1332,14 +1331,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 5a699e47478b..d942b0c8e32f 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -3801,7 +3801,7 @@ EXPORT_SYMBOL(security_d_instantiate);
> /**
> * security_getprocattr() - Read an attribute for a task
> * @p: the task
> - * @lsm: LSM name
> + * @lsmid: LSM identification
> * @name: attribute name
> * @value: attribute value
> *
> @@ -3809,13 +3809,13 @@ EXPORT_SYMBOL(security_d_instantiate);
> *
> * Return: Returns the length of @value on success, a negative value otherwise.
> */
> -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->name))
> + if (lsmid != 0 && lsmid != hp->lsmid->id)
> continue;
> return hp->hook.getprocattr(p, name, value);
> }
> @@ -3824,7 +3824,7 @@ int security_getprocattr(struct task_struct *p, const char *lsm,
>
> /**
> * security_setprocattr() - Set an attribute for a task
> - * @lsm: LSM name
> + * @lsmid: LSM identification
> * @name: attribute name
> * @value: attribute value
> * @size: attribute value size
> @@ -3834,13 +3834,12 @@ int security_getprocattr(struct task_struct *p, const char *lsm,
> *
> * Return: Returns bytes written on success, a negative value otherwise.
> */
> -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->name))
> + if (lsmid != 0 && lsmid != hp->lsmid->id)
> continue;
> return hp->hook.setprocattr(name, value, size);
> }