On 8/5/2013 11:30 PM, Kees Cook wrote:
> On Thu, Jul 25, 2013 at 11:52 PM, Casey Schaufler <ca...@schaufler-ca.com>
> wrote:
>> The /proc/*/attr interfaces are given to one LSM. This can be
>> done by setting CONFIG_SECURITY_PRESENT. Additional interfaces
>> have been created in /proc/*/attr so that each LSM has its own
>> named interfaces. The name of the presenting LSM can be read from
> For me, this is one problem that was bothering me, but it was a cosmetic
> one that I'd mentioned before: I really disliked the /proc/$pid/attr
> interface being named "$lsm.$file". I feel it's important to build
> directories in attr/ for each LSM. So, I spent time to figure out a way to
> do this. This patch changes the interface to /proc/$pid/attr/$lsm/$file
> instead, which I feel has a much more appealing organizational structure.
I will confess that the reason I went with <lsm>.current instead of
<lsm>/current was that the former was easier to implement.
> -Kees
>
> ---
> Subject: [PATCH] LSM: use subdirectories for LSM attr files
>
> Instead of filling the /proc/$pid/attr/ directory with every LSM's needed
> attr files, insert a directory entry for each LSM which contains the
> needed files.
>
> Signed-off-by: Kees Cook <keesc...@chromium.org>
> ---
> fs/proc/base.c | 95
> ++++++++++++++++++++++++++++++++++++----------
> fs/proc/internal.h | 1 +
> include/linux/security.h | 11 +++---
> security/security.c | 67 ++++++++++++++------------------
> 4 files changed, 112 insertions(+), 62 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 941fe83..4c80ffd 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -138,6 +138,10 @@ struct pid_entry {
> NOD(NAME, (S_IFREG|(MODE)), \
> NULL, &proc_single_file_operations, \
> { .proc_show = show } )
> +#define ATTR(LSM, NAME, MODE) \
> + NOD(NAME, (S_IFREG|(MODE)), \
> + NULL, &proc_pid_attr_operations, \
> + { .lsm = LSM } )
>
> /*
> * Count the number of hardlinks for the pid_entry table, excluding the .
> @@ -2292,7 +2296,7 @@ static ssize_t proc_pid_attr_read(struct file * file,
> char __user * buf,
> if (!task)
> return -ESRCH;
>
> - length = security_getprocattr(task,
> + length = security_getprocattr(task, PROC_I(inode)->op.lsm,
> (char*)file->f_path.dentry->d_name.name,
> &p);
> put_task_struct(task);
> @@ -2335,7 +2339,7 @@ static ssize_t proc_pid_attr_write(struct file * file,
> const char __user * buf,
> if (length < 0)
> goto out_free;
>
> - length = security_setprocattr(task,
> + length = security_setprocattr(task, PROC_I(inode)->op.lsm,
> (char*)file->f_path.dentry->d_name.name,
> (void*)page, count);
> mutex_unlock(&task->signal->cred_guard_mutex);
> @@ -2353,29 +2357,82 @@ static const struct file_operations
> proc_pid_attr_operations = {
> .llseek = generic_file_llseek,
> };
>
> +#define LSM_DIR_OPS(LSM) \
> +static int proc_##LSM##_attr_dir_readdir(struct file * filp, \
> + void * dirent, filldir_t filldir) \
> +{ \
> + return proc_pident_readdir(filp, dirent, filldir, \
> + LSM##_attr_dir_stuff, \
> + ARRAY_SIZE(LSM##_attr_dir_stuff)); \
> +} \
> +\
> +static const struct file_operations proc_##LSM##_attr_dir_ops = { \
> + .read = generic_read_dir, \
> + .readdir = proc_##LSM##_attr_dir_readdir, \
> + .llseek = default_llseek, \
> +}; \
> +\
> +static struct dentry *proc_##LSM##_attr_dir_lookup(struct inode *dir, \
> + struct dentry *dentry, unsigned int flags) \
> +{ \
> + return proc_pident_lookup(dir, dentry, \
> + LSM##_attr_dir_stuff, \
> + ARRAY_SIZE(LSM##_attr_dir_stuff)); \
> +} \
> +\
> +static const struct inode_operations proc_##LSM##_attr_dir_inode_ops = { \
> + .lookup = proc_##LSM##_attr_dir_lookup, \
> + .getattr = pid_getattr, \
> + .setattr = proc_setattr, \
> +};
That's one hell of a macro you got there, Kees.
I'm not saying it's bad, but it is quite the mouthful.
> +
> +#ifdef CONFIG_SECURITY_SELINUX
> +static const struct pid_entry selinux_attr_dir_stuff[] = {
> + ATTR("selinux", "current", S_IRUGO|S_IWUGO),
> + ATTR("selinux", "prev", S_IRUGO),
> + ATTR("selinux", "exec", S_IRUGO|S_IWUGO),
> + ATTR("selinux", "fscreate", S_IRUGO|S_IWUGO),
> + ATTR("selinux", "keycreate", S_IRUGO|S_IWUGO),
> + ATTR("selinux", "sockcreate", S_IRUGO|S_IWUGO),
> +};
> +LSM_DIR_OPS(selinux);
> +#endif
> +
> +#ifdef CONFIG_SECURITY_SMACK
> +static const struct pid_entry smack_attr_dir_stuff[] = {
> + ATTR("smack", "current", S_IRUGO|S_IWUGO),
> +};
> +LSM_DIR_OPS(smack);
> +#endif
> +
> +#ifdef CONFIG_SECURITY_APPARMOR
> +static const struct pid_entry apparmor_attr_dir_stuff[] = {
> + ATTR("apparmor", "current", S_IRUGO|S_IWUGO),
> + ATTR("apparmor", "prev", S_IRUGO),
> + ATTR("apparmor", "exec", S_IRUGO|S_IWUGO),
> +};
> +LSM_DIR_OPS(apparmor);
> +#endif
> +
%s/dir_stuff/dir_entries/g
It doesn't have to be "entries", but "stuff" is horribly non-descriptive.
> static const struct pid_entry attr_dir_stuff[] = {
> - REG("current", S_IRUGO|S_IWUGO, proc_pid_attr_operations),
> - REG("prev", S_IRUGO, proc_pid_attr_operations),
> - REG("exec", S_IRUGO|S_IWUGO, proc_pid_attr_operations),
> - REG("fscreate", S_IRUGO|S_IWUGO, proc_pid_attr_operations),
> - REG("keycreate", S_IRUGO|S_IWUGO, proc_pid_attr_operations),
> - REG("sockcreate", S_IRUGO|S_IWUGO, proc_pid_attr_operations),
> - REG("context", S_IRUGO|S_IWUGO, proc_pid_attr_operations),
> + ATTR(NULL, "current", S_IRUGO|S_IWUGO),
> + ATTR(NULL, "prev", S_IRUGO),
> + ATTR(NULL, "exec", S_IRUGO|S_IWUGO),
> + ATTR(NULL, "fscreate", S_IRUGO|S_IWUGO),
> + ATTR(NULL, "keycreate", S_IRUGO|S_IWUGO),
> + ATTR(NULL, "sockcreate", S_IRUGO|S_IWUGO),
> + ATTR(NULL, "context", S_IRUGO|S_IWUGO),
> #ifdef CONFIG_SECURITY_SELINUX
> - REG("selinux.current", S_IRUGO|S_IWUGO, proc_pid_attr_operations),
> - REG("selinux.prev", S_IRUGO, proc_pid_attr_operations),
> - REG("selinux.exec", S_IRUGO|S_IWUGO, proc_pid_attr_operations),
> - REG("selinux.fscreate", S_IRUGO|S_IWUGO, proc_pid_attr_operations),
> - REG("selinux.keycreate", S_IRUGO|S_IWUGO, proc_pid_attr_operations),
> - REG("selinux.sockcreate", S_IRUGO|S_IWUGO, proc_pid_attr_operations),
> + DIR("selinux", S_IRUGO|S_IXUGO,
> + proc_selinux_attr_dir_inode_ops, proc_selinux_attr_dir_ops),
> #endif
> #ifdef CONFIG_SECURITY_SMACK
> - REG("smack.current", S_IRUGO|S_IWUGO, proc_pid_attr_operations),
> + DIR("smack", S_IRUGO|S_IXUGO,
> + proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
> #endif
> #ifdef CONFIG_SECURITY_APPARMOR
> - REG("apparmor.current", S_IRUGO|S_IWUGO, proc_pid_attr_operations),
> - REG("apparmor.prev", S_IRUGO, proc_pid_attr_operations),
> - REG("apparmor.exec", S_IRUGO|S_IWUGO, proc_pid_attr_operations),
> + DIR("apparmor", S_IRUGO|S_IXUGO,
> + proc_apparmor_attr_dir_inode_ops, proc_apparmor_attr_dir_ops),
> #endif
>
> };
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index d600fb0..795f649 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -56,6 +56,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;
> };
>
> struct proc_inode {
> diff --git a/include/linux/security.h b/include/linux/security.h
> index d60e21c..fa89618 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -2115,9 +2115,10 @@ int security_sem_semctl(struct sem_array *sma, int
> cmd);
> int security_sem_semop(struct sem_array *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, char *name, char **value);
> -int security_setprocattr(struct task_struct *p, char *name, void *value,
> - size_t size);
> +int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> + char **value);
> +int security_setprocattr(struct task_struct *p, const char *lsm, char *name,
> + void *value, size_t size);
> int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> int security_secid_to_secctx(struct secids *secid, char **secdata, u32
> *seclen,
> struct security_operations **secops);
> @@ -2801,8 +2802,8 @@ static inline void security_d_instantiate(struct dentry
> *dentry,
> struct inode *inode)
> { }
>
> -static inline int security_getprocattr(struct task_struct *p, char *name,
> - char **value)
> +static inline int security_getprocattr(struct task_struct *p, char *lsm,
> + char *name, char **value)
> {
> return -EINVAL;
> }
> diff --git a/security/security.c b/security/security.c
> index 119a377..499af30 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1897,74 +1897,65 @@ void security_d_instantiate(struct dentry *dentry,
> struct inode *inode)
> }
> EXPORT_SYMBOL(security_d_instantiate);
>
> -int security_getprocattr(struct task_struct *p, char *name, char **value)
> +int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> + char **value)
> {
> struct security_operations *sop = NULL;
> struct secids secid;
> - char *lsm;
> - int lsmlen;
> int ret;
>
> /*
> - * Names will either be in the legacy form containing
> - * no periods (".") or they will be the LSM name followed
> - * by the legacy suffix. "current" or "selinux.current"
> - * The exception is "context", which gets all of the LSMs.
> - *
> - * Legacy names are handled by the presenting LSM.
> - * Suffixed names are handled by the named LSM.
> + * Target LSM will be either NULL or looked up by name. Names with
> + * a NULL LSM (legacy) are handled by the presenting LSM. The
> + * exception is "context", which gets all of the LSMs.
> */
> if (strcmp(name, "context") == 0) {
> + char *lsmname;
> + int lsmlen;
> +
> security_task_getsecid(p, &secid);
> - ret = security_secid_to_secctx(&secid, &lsm, &lsmlen, &sop);
> + ret = security_secid_to_secctx(&secid, &lsmname, &lsmlen, &sop);
> if (ret == 0) {
> - *value = kstrdup(lsm, GFP_KERNEL);
> + *value = kstrdup(lsmname, GFP_KERNEL);
> if (*value == NULL)
> ret = -ENOMEM;
> else
> ret = strlen(*value);
> - security_release_secctx(lsm, lsmlen, sop);
> + security_release_secctx(lsmname, lsmlen, sop);
> }
> return ret;
> }
>
> - if (present_ops && !strchr(name, '.'))
> - return present_getprocattr(p, name, value);
> -
> - for_each_hook(sop, getprocattr) {
> - lsm = sop->name;
> - lsmlen = strlen(lsm);
> - if (!strncmp(name, lsm, lsmlen) && name[lsmlen] == '.')
> - return sop->getprocattr(p, name + lsmlen + 1, value);
> + if (!lsm) {
> + if (present_ops)
> + return present_getprocattr(p, name, value);
> + } else {
> + for_each_hook(sop, getprocattr) {
> + if (!strcmp(lsm, sop->name))
> + return sop->getprocattr(p, name, value);
> + }
> }
> return -EINVAL;
> }
>
> -int security_setprocattr(struct task_struct *p, char *name, void *value,
> - size_t size)
> +int security_setprocattr(struct task_struct *p, const char *lsm, char *name,
> + void *value, size_t size)
> {
> struct security_operations *sop;
> - char *lsm;
> - int lsmlen;
>
> /*
> - * Names will either be in the legacy form containing
> - * no periods (".") or they will be the LSM name followed
> - * by the legacy suffix.
> - * "current" or "selinux.current"
> - *
> - * Legacy names are handled by the presenting LSM.
> - * Suffixed names are handled by the named LSM.
> + * Target LSM will be either NULL or looked up by name. Names with
> + * a NULL LSM (legacy) are handled by the presenting LSM. The
> */
> if (present_ops && !strchr(name, '.'))
> return present_setprocattr(p, name, value, size);
We'll want to loose the preceding two lines, and add some later.
> - for_each_hook(sop, setprocattr) {
> - lsm = sop->name;
> - lsmlen = strlen(lsm);
> - if (!strncmp(name, lsm, lsmlen) && name[lsmlen] == '.')
> - return sop->setprocattr(p, name + lsmlen + 1, value,
> - size);
> + if (lsm) {
> + for_each_hook(sop, setprocattr) {
> + if (!strcmp(lsm, sop->name))
> + return sop->setprocattr(p, name, value,
> + size);
> + }
> - }
+ } else if (present_ops)
+ return present_setprocattr(p, name, value, size);
> return -EINVAL;
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
