On 4/22/2021 6:46 AM, Roberto Sassu wrote: >> 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. You can address that by expanding the call_int_hook MACRO in security_old_inode_init_security() in place and change it to stop after the first call. The two callers of security_old_inode_init_security() are going to need to be converted to security_inode_init_security() when the "complete" stacking (i.e. SELinux + Smack) anyway, so I don't see that as an issue. Is anyone concerned that ocfs2 and reiserfs aren't EVM capable? > > 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;
