> From: Casey Schaufler [mailto:casey@xxxxxxxxxxxxxxxx] > Sent: Thursday, April 22, 2021 12:44 AM > On 4/21/2021 9:19 AM, Roberto Sassu wrote: > > In preparation for moving EVM to the LSM infrastructure, this patch > > replaces the name, value, len triple with the xattr array pointer provided > > by security_inode_init_security(). LSMs are expected to call the new > > function lsm_find_xattr_slot() to find the first unused slot of the array > > where the xattr should be written. > > > > This patch modifies also SELinux and Smack to search for an unused slot, to > > have a consistent behavior across LSMs (the unmodified version would > > overwrite the xattr set by the first LSM in the chain). It is also > > desirable to have the modification in those LSMs, as they are likely used > > as a reference for the development of new LSMs. > > This looks better than V1. One safety comment below. > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx> > > --- > > include/linux/lsm_hook_defs.h | 4 ++-- > > include/linux/lsm_hooks.h | 18 +++++++++++++++--- > > security/security.c | 13 +++++++------ > > security/selinux/hooks.c | 13 ++++++------- > > security/smack/smack_lsm.c | 20 +++++++++----------- > > 5 files changed, 39 insertions(+), 29 deletions(-) > > > > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > > index 477a597db013..afb9dd122f60 100644 > > --- a/include/linux/lsm_hook_defs.h > > +++ b/include/linux/lsm_hook_defs.h > > @@ -111,8 +111,8 @@ LSM_HOOK(int, 0, path_notify, const struct path > *path, u64 mask, > > LSM_HOOK(int, 0, inode_alloc_security, struct inode *inode) > > LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode *inode) > > LSM_HOOK(int, 0, inode_init_security, struct inode *inode, > > - struct inode *dir, const struct qstr *qstr, const char **name, > > - void **value, size_t *len) > > + struct inode *dir, const struct qstr *qstr, struct xattr *xattrs, > > + void *fs_data) > > LSM_HOOK(int, 0, inode_init_security_anon, struct inode *inode, > > const struct qstr *name, const struct inode *context_inode) > > LSM_HOOK(int, 0, inode_create, struct inode *dir, struct dentry *dentry, > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > > index c5498f5174ce..e8c9bac29b9d 100644 > > --- a/include/linux/lsm_hooks.h > > +++ b/include/linux/lsm_hooks.h > > @@ -27,6 +27,7 @@ > > > > #include <linux/security.h> > > #include <linux/init.h> > > +#include <linux/xattr.h> > > #include <linux/rculist.h> > > > > /** > > @@ -227,9 +228,11 @@ > > * @inode contains the inode structure of the newly created inode. > > * @dir contains the inode structure of the parent directory. > > * @qstr contains the last path component of the new object > > - * @name will be set to the allocated name suffix (e.g. selinux). > > - * @value will be set to the allocated attribute value. > > - * @len will be set to the length of the value. > > + * @xattrs contains the full array of xattrs allocated by LSMs where > > + * ->name will be set to the allocated name suffix (e.g. selinux). > > + * ->value will be set to the allocated attribute value. > > + * ->len will be set to the length of the value. > > + * @fs_data contains filesystem-specific data. > > * Returns 0 if @name and @value have been successfully set, > > * -EOPNOTSUPP if no security attribute is needed, or > > * -ENOMEM on memory allocation failure. > > @@ -1661,4 +1664,13 @@ static inline void security_delete_hooks(struct > security_hook_list *hooks, > > > > extern int lsm_inode_alloc(struct inode *inode); > > > > Some "security researcher" with a fuzz tester is going to manage to dump junk > into the slots and ruin your week. I suggest a simple change to make bounds > checking > possible. It should never happen, but if that was sufficient people would > love C > string processing better. > > > +static inline struct xattr *lsm_find_xattr_slot(struct xattr *xattrs) > > +static inline struct xattr *lsm_find_xattr_slot(struct xattr *xattrs, int available) Ok. I looked at how I should do that. Initially, I thought that I could use a global variable storing the number of inode_init_security implementations, determined at LSM registration time. Then, I realized that this would not work, as the number of array elements when security_old_inode_init_security() is called is 1. I modified the patch set to pass also the number of array elements. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli > > +{ > > + struct xattr *slot; > > + > > + for (slot = xattrs; slot && slot->name != NULL; slot++) > > + for (slot = xattrs; slot && slot->name != NULL; slot++) > if (WARN_ON(slot > xattrs[available])) > return NULL; > > > + ; > > + > > + return slot; > > +} > > #endif /* ! __LINUX_LSM_HOOKS_H */ > > diff --git a/security/security.c b/security/security.c > > index 7f14e59c4f8e..2c1fe1496069 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -1037,18 +1037,16 @@ int security_inode_init_security(struct inode > *inode, struct inode *dir, > > > > if (!initxattrs) > > return call_int_hook(inode_init_security, -EOPNOTSUPP, > inode, > > - dir, qstr, NULL, NULL, NULL); > > + dir, qstr, NULL, fs_data); > > memset(new_xattrs, 0, sizeof(new_xattrs)); > > lsm_xattr = new_xattrs; > > ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr, > > - &lsm_xattr->name, > > - &lsm_xattr->value, > > - &lsm_xattr->value_len); > > + lsm_xattr, fs_data); > > if (ret) > > goto out; > > > > evm_xattr = lsm_xattr + 1; > > - ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr); > > + ret = evm_inode_init_security(inode, new_xattrs, evm_xattr); > > if (ret) > > goto out; > > ret = initxattrs(inode, new_xattrs, fs_data); > > @@ -1071,10 +1069,13 @@ int security_old_inode_init_security(struct inode > *inode, struct inode *dir, > > const struct qstr *qstr, const char **name, > > void **value, size_t *len) > > { > > + struct xattr xattr = { .name = NULL, .value = NULL, .value_len = 0 }; > > + struct xattr *lsm_xattr = (name && value && len) ? &xattr : NULL; > > + > > if (unlikely(IS_PRIVATE(inode))) > > return -EOPNOTSUPP; > > return call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, > > - qstr, name, value, len); > > + qstr, lsm_xattr, NULL); > > } > > EXPORT_SYMBOL(security_old_inode_init_security); > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index ddd097790d47..806827eb132a 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -2916,11 +2916,11 @@ static int selinux_dentry_create_files_as(struct > dentry *dentry, int mode, > > > > static int selinux_inode_init_security(struct inode *inode, struct inode *dir, > > const struct qstr *qstr, > > - const char **name, > > - void **value, size_t *len) > > + struct xattr *xattrs, void *fs_data) > > { > > const struct task_security_struct *tsec = selinux_cred(current_cred()); > > struct superblock_security_struct *sbsec; > > + struct xattr *xattr = lsm_find_xattr_slot(xattrs); > > u32 newsid, clen; > > int rc; > > char *context; > > @@ -2947,16 +2947,15 @@ static int selinux_inode_init_security(struct > inode *inode, struct inode *dir, > > !(sbsec->flags & SBLABEL_MNT)) > > return -EOPNOTSUPP; > > > > - if (name) > > - *name = XATTR_SELINUX_SUFFIX; > > + if (xattr) { > > + xattr->name = XATTR_SELINUX_SUFFIX; > > > > - if (value && len) { > > rc = security_sid_to_context_force(&selinux_state, newsid, > > &context, &clen); > > if (rc) > > return rc; > > - *value = context; > > - *len = clen; > > + xattr->value = context; > > + xattr->value_len = clen; > > } > > > > return 0; > > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > > index 12a45e61c1a5..af7eee0fee52 100644 > > --- a/security/smack/smack_lsm.c > > +++ b/security/smack/smack_lsm.c > > @@ -962,26 +962,24 @@ static int smack_inode_alloc_security(struct inode > *inode) > > * @inode: the newly created inode > > * @dir: containing directory object > > * @qstr: unused > > - * @name: where to put the attribute name > > - * @value: where to put the attribute value > > - * @len: where to put the length of the attribute > > + * @xattrs: where to put the attribute > > * > > * Returns 0 if it all works out, -ENOMEM if there's no memory > > */ > > static int smack_inode_init_security(struct inode *inode, struct inode > *dir, > > - const struct qstr *qstr, const char **name, > > - void **value, size_t *len) > > + const struct qstr *qstr, > > + struct xattr *xattrs, void *fs_data) > > { > > struct inode_smack *issp = smack_inode(inode); > > struct smack_known *skp = smk_of_current(); > > struct smack_known *isp = smk_of_inode(inode); > > struct smack_known *dsp = smk_of_inode(dir); > > + struct xattr *xattr = lsm_find_xattr_slot(xattrs); > > int may; > > > > - if (name) > > - *name = XATTR_SMACK_SUFFIX; > > + if (xattr) { > > + xattr->name = XATTR_SMACK_SUFFIX; > > > > - if (value && len) { > > rcu_read_lock(); > > may = smk_access_entry(skp->smk_known, dsp->smk_known, > > &skp->smk_rules); > > @@ -999,11 +997,11 @@ static int smack_inode_init_security(struct inode > *inode, struct inode *dir, > > issp->smk_flags |= SMK_INODE_CHANGED; > > } > > > > - *value = kstrdup(isp->smk_known, GFP_NOFS); > > - if (*value == NULL) > > + xattr->value = kstrdup(isp->smk_known, GFP_NOFS); > > + if (xattr->value == NULL) > > return -ENOMEM; > > > > - *len = strlen(isp->smk_known); > > + xattr->value_len = strlen(isp->smk_known); > > } > > > > return 0;
