From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756734Ab3HFWZp (ORCPT ); Tue, 6 Aug 2013 18:25:45 -0400 Received: from smtp102.biz.mail.gq1.yahoo.com ([98.137.12.177]:20053 "HELO smtp102.biz.mail.gq1.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1756175Ab3HFWZn (ORCPT ); Tue, 6 Aug 2013 18:25:43 -0400 X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: 8.llrKQVM1lsY1f1GIGOcHGU4jHVzM7L2lZ2mLIEKzcDqJm pXsbCiyakLFjVznaar33bHjfKC4XpzQ8cusLy0PAdXs2.3PeB5g8pZ9SauxH iHOHr84IaIDEWJ1j.IpO792ZrLzwYhzsrZChUt2q6EmieZBfnzqzQuQrD2Gc jH9zryD79_KpnQSntOIIZTfaffHuBEUr8fxcKAsVVfzSo4FMVDe.lOb36HWs 0qZuDZM0kcuY0KFoZXdUVF8TsJ0Qhkb4LpIVToSMmFh2EmjI4slkgPqKd9Rc Sf2eVIJnPImT0l8ZLMVPYSd1KmPEuPO2nzMK8em8..Ki52hK96kWMcwaJky3 HB7oIyPYoej4k60SoJdV146pmqS8O6PG3w2i.OPZzQB.A5TkA8Q0ohOktHih llxPq.auCzECxaIuFoEEzBruDFsOsahzj3QZ9rLihY6EJ.DQDDnvxYTX_57A me6qfwbjOVydrB5pBx3vgfmhqFSlakbpFRXUH5A2.7zYo7NlYkk8_IJ5HWuc yiVDjk4uwtrHdXE13awE6wb5sSq0ckJB.FlHo5cxtF2MzB28HD2PpMMlT6mI cDXEg5ZbtAM9APNUkUgjPQ1nQ7PrxfSrqSrD0gczJFf3wohD9KxCr X-Yahoo-SMTP: OIJXglSswBDfgLtXluJ6wiAYv6_cnw-- X-Rocket-Received: from [192.168.0.103] (casey@24.6.250.25 with ) by smtp102.biz.mail.gq1.yahoo.com with SMTP; 06 Aug 2013 15:25:42 -0700 PDT Message-ID: <520177E8.5080906@schaufler-ca.com> Date: Tue, 06 Aug 2013 15:25:44 -0700 From: Casey Schaufler User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20130620 Thunderbird/17.0.7 MIME-Version: 1.0 To: Kees Cook CC: LKLM , LSM , SE Linux , James Morris , John Johansen , Eric Paris , Tetsuo Handa , Casey Schaufler Subject: Re: [PATCH v14 0/6] LSM: Multiple concurrent LSMs References: <20130806063002.GF2280@outflux.net> In-Reply-To: <20130806063002.GF2280@outflux.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 8/5/2013 11:30 PM, Kees Cook wrote: > On Thu, Jul 25, 2013 at 11:52 PM, Casey Schaufler 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 .current instead of /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 > --- > 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; > }