On 4/4/2023 11:54 AM, Paul Moore wrote: > On Fri, Mar 31, 2023 at 8:33 AM Roberto Sassu > <roberto.sassu@xxxxxxxxxxxxxxx> wrote: >> From: Roberto Sassu <roberto.sassu@xxxxxxxxxx> >> >> Currently, security_inode_init_security() supports only one LSM providing >> an xattr and EVM calculating the HMAC on that xattr, plus other inode >> metadata. >> >> Allow all LSMs to provide one or multiple xattrs, by extending the security >> blob reservation mechanism. Introduce the new lbs_xattr_count field of the >> lsm_blob_sizes structure, so that each LSM can specify how many xattrs it >> needs, and the LSM infrastructure knows how many xattr slots it should >> allocate. >> >> Dynamically allocate the new_xattrs array to be populated by LSMs with the >> inode_init_security hook, and pass it to the latter instead of the >> name/value/len triple. Unify the !initxattrs and initxattrs case, simply >> don't allocate the new_xattrs array in the former. >> >> Also, pass to the hook the number of xattrs filled by each LSM, so that >> there are no gaps when the next LSM fills the array. Gaps might occur >> because an LSM can legitimately request xattrs to the LSM infrastructure, >> but not fill the reserved slots, if it was not initialized. >> >> Update the documentation of security_inode_init_security() to reflect the >> changes, and fix the description of the xattr name, as it is not allocated >> anymore. >> >> Finally, adapt both SELinux and Smack to use the new definition of the >> inode_init_security hook, and to fill the reserved slots in the xattr >> array. Introduce the lsm_get_xattr_slot() helper to retrieve an available >> slot to fill, and to increment the number of filled slots. >> >> Move the xattr->name assignment after the xattr->value one, so that it is >> done only in case of successful memory allocation. For Smack, also reserve >> space for the other defined xattrs although they are not set yet in >> smack_inode_init_security(). >> >> Reported-by: Nicolas Bouchinet <nicolas.bouchinet@xxxxxxxxxxx> (EVM crash) >> Link: https://lore.kernel.org/linux-integrity/Y1FTSIo+1x+4X0LS@archlinux/ >> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx> >> --- >> include/linux/lsm_hook_defs.h | 6 +-- >> include/linux/lsm_hooks.h | 20 ++++++++++ >> security/security.c | 71 +++++++++++++++++++++++------------ >> security/selinux/hooks.c | 17 +++++---- >> security/smack/smack_lsm.c | 32 ++++++++++------ >> 5 files changed, 99 insertions(+), 47 deletions(-) > This looks good aside from a few small things (below). From what I > can see, there are only two outstanding issues to answer: the number > of Smack xattrs, sign-off from Casey for the Smack bits. > >> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h >> index 6bb55e61e8e..a1896f90089 100644 >> --- a/include/linux/lsm_hook_defs.h >> +++ b/include/linux/lsm_hook_defs.h >> @@ -111,9 +111,9 @@ LSM_HOOK(int, 0, path_notify, const struct path *path, u64 mask, >> unsigned int obj_type) >> 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) >> +LSM_HOOK(int, -EOPNOTSUPP, inode_init_security, struct inode *inode, >> + struct inode *dir, const struct qstr *qstr, struct xattr *xattrs, >> + int *xattr_count) >> 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 c2be66c669a..9eb9b686493 100644 >> --- a/include/linux/lsm_hooks.h >> +++ b/include/linux/lsm_hooks.h >> @@ -28,6 +28,7 @@ >> #include <linux/security.h> >> #include <linux/init.h> >> #include <linux/rculist.h> >> +#include <linux/xattr.h> >> >> union security_list_options { >> #define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__); >> @@ -63,8 +64,27 @@ struct lsm_blob_sizes { >> int lbs_ipc; >> int lbs_msg_msg; >> int lbs_task; >> + int lbs_xattr_count; /* number of xattr slots in new_xattrs array */ >> }; >> >> +/** >> + * lsm_get_xattr_slot - Return the next available slot and increment the index >> + * @xattrs: array storing LSM-provided xattrs >> + * @xattr_count: number of already stored xattrs (updated) >> + * >> + * Retrieve the first available slot in the @xattrs array to fill with an xattr, >> + * and increment @xattr_count. >> + * >> + * Return: The slot to fill in @xattrs if non-NULL, NULL otherwise. >> + */ >> +static inline struct xattr *lsm_get_xattr_slot(struct xattr *xattrs, >> + int *xattr_count) >> +{ >> + if (unlikely(!xattrs)) >> + return NULL; >> + return xattrs + (*xattr_count)++; >> +} >> + >> /* >> * LSM_RET_VOID is used as the default value in LSM_HOOK definitions for void >> * LSM hooks (in include/linux/lsm_hook_defs.h). >> diff --git a/security/security.c b/security/security.c >> index f4170efcddd..1aeaa8ce449 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -31,8 +31,6 @@ >> #include <linux/msg.h> >> #include <net/flow.h> >> >> -#define MAX_LSM_EVM_XATTR 2 >> - >> /* How many LSMs were built into the kernel? */ >> #define LSM_COUNT (__end_lsm_info - __start_lsm_info) >> >> @@ -212,6 +210,8 @@ static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed) >> lsm_set_blob_size(&needed->lbs_msg_msg, &blob_sizes.lbs_msg_msg); >> lsm_set_blob_size(&needed->lbs_superblock, &blob_sizes.lbs_superblock); >> lsm_set_blob_size(&needed->lbs_task, &blob_sizes.lbs_task); >> + lsm_set_blob_size(&needed->lbs_xattr_count, >> + &blob_sizes.lbs_xattr_count); >> } >> >> /* Prepare LSM for initialization. */ >> @@ -378,6 +378,7 @@ static void __init ordered_lsm_init(void) >> init_debug("msg_msg blob size = %d\n", blob_sizes.lbs_msg_msg); >> init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock); >> init_debug("task blob size = %d\n", blob_sizes.lbs_task); >> + init_debug("xattr slots = %d\n", blob_sizes.lbs_xattr_count); >> >> /* >> * Create any kmem_caches needed for blobs >> @@ -1591,11 +1592,15 @@ EXPORT_SYMBOL(security_dentry_create_files_as); >> * created inode and set up the incore security field for the new inode. This >> * hook is called by the fs code as part of the inode creation transaction and >> * provides for atomic labeling of the inode, unlike the post_create/mkdir/... >> - * hooks called by the VFS. The hook function is expected to allocate the name >> - * and value via kmalloc, with the caller being responsible for calling kfree >> - * after using them. If the security module does not use security attributes >> - * or does not wish to put a security attribute on this particular inode, then >> - * it should return -EOPNOTSUPP to skip this processing. >> + * hooks called by the VFS. The hook function is expected to populate the >> + * @xattrs array, by calling lsm_get_xattr_slot() to retrieve the slots >> + * reserved by the security module with the lbs_xattr_count field of the >> + * lsm_blob_sizes structure. For each slot, the hook function should set ->name >> + * to the attribute name suffix (e.g. selinux), to allocate ->value (will be >> + * freed by the caller) and set it to the attribute value, to set ->value_len to >> + * the length of the value. If the security module does not use security >> + * attributes or does not wish to put a security attribute on this particular >> + * inode, then it should return -EOPNOTSUPP to skip this processing. >> * >> * Return: Returns 0 on success, -EOPNOTSUPP if no security attribute is >> * needed, or -ENOMEM on memory allocation failure. >> @@ -1604,33 +1609,51 @@ int security_inode_init_security(struct inode *inode, struct inode *dir, >> const struct qstr *qstr, >> const initxattrs initxattrs, void *fs_data) >> { >> - struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1]; >> - struct xattr *lsm_xattr, *evm_xattr, *xattr; >> - int ret; >> + struct security_hook_list *P; >> + struct xattr *new_xattrs = NULL; >> + int ret = -EOPNOTSUPP, xattr_count = 0; >> >> if (unlikely(IS_PRIVATE(inode))) >> return 0; >> >> - if (!initxattrs) >> - return call_int_hook(inode_init_security, -EOPNOTSUPP, inode, >> - dir, qstr, NULL, NULL, NULL); >> - 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); >> - if (ret) >> + if (!blob_sizes.lbs_xattr_count) >> + return 0; >> + >> + if (initxattrs) { >> + /* Allocate +1 for EVM and +1 as terminator. */ >> + new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 2, >> + sizeof(*new_xattrs), GFP_NOFS); >> + if (!new_xattrs) >> + return -ENOMEM; >> + } >> + >> + hlist_for_each_entry(P, &security_hook_heads.inode_init_security, >> + list) { >> + ret = P->hook.inode_init_security(inode, dir, qstr, new_xattrs, >> + &xattr_count); >> + if (ret && ret != -EOPNOTSUPP) >> + goto out; >> + /* >> + * As documented in lsm_hooks.h, -EOPNOTSUPP in this context >> + * means that the LSM is not willing to provide an xattr, not >> + * that it wants to signal an error. Thus, continue to invoke >> + * the remaining LSMs. >> + */ >> + } >> + >> + /* If initxattrs() is NULL, xattr_count is zero, skip the call. */ >> + if (!xattr_count) >> 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, >> + new_xattrs + xattr_count); > I think it's cleaner to write '&new_xattrs[xattr_count]' for the third > parameter above (no concerns around pointer math), and stylistically > it matches better with the for-kfree loop below. > >> if (ret) >> goto out; >> ret = initxattrs(inode, new_xattrs, fs_data); >> out: >> - for (xattr = new_xattrs; xattr->value != NULL; xattr++) >> - kfree(xattr->value); >> + for (; xattr_count > 0; xattr_count--) >> + kfree(new_xattrs[xattr_count - 1].value); >> + kfree(new_xattrs); >> return (ret == -EOPNOTSUPP) ? 0 : ret; >> } >> EXPORT_SYMBOL(security_inode_init_security); > .. > >> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c >> index cfcbb748da2..8392983334b 100644 >> --- a/security/smack/smack_lsm.c >> +++ b/security/smack/smack_lsm.c >> @@ -52,6 +52,15 @@ >> #define SMK_RECEIVING 1 >> #define SMK_SENDING 2 >> >> +/* >> + * Smack uses multiple xattrs. >> + * SMACK64 - for access control, SMACK64EXEC - label for the program, > I think it would be good to move SMACK64EXEC to its own line; it took > me a minute to figure out why SMACK_INODE_INIT_XATTRS was set to '4' > when I only say three comment lines ... ;) > >> + * SMACK64MMAP - controls library loading, >> + * SMACK64TRANSMUTE - label initialization, >> + * Not saved on files - SMACK64IPIN and SMACK64IPOUT >> + */ >> +#define SMACK_INODE_INIT_XATTRS 4 > If smack_inode_init_security() only ever populates a single xattr, and > that is the only current user of SMACK_INODE_INIT_XATTRS, can we make > this '1' and shrink the xattr allocation a bit? If the parent directory is marked with SMACK64_TRANSMUTE, the access rule allowing the access has the "t" mode, and the object being initialized is a directory, the new inode should get the SMACK64_TRANSMUTE attribute. The callers of security_inode_init_security() don't seem to care. I can't say if the evm code is getting SMACK64_TRANSMUTE or, for that matter, SMACK64_EXEC and SMACK64_MMAP, some other way. The older system allowed for multiple Smack xattrs, but I'm not clear on exactly how. >> #ifdef SMACK_IPV6_PORT_LABELING >> static DEFINE_MUTEX(smack_ipv6_lock); >> static LIST_HEAD(smk_ipv6_port_list); >> @@ -939,26 +948,23 @@ 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 attributes >> + * @xattr_count: current number of LSM-provided xattrs (updated) >> * >> * 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, int *xattr_count) >> { >> 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_get_xattr_slot(xattrs, xattr_count); >> int may; >> >> - if (name) >> - *name = XATTR_SMACK_SUFFIX; >> - >> - if (value && len) { >> + if (xattr) { >> rcu_read_lock(); >> may = smk_access_entry(skp->smk_known, dsp->smk_known, >> &skp->smk_rules); >> @@ -976,11 +982,12 @@ 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); >> + xattr->name = XATTR_SMACK_SUFFIX; >> } >> >> return 0; >> @@ -4854,6 +4861,7 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = { >> .lbs_ipc = sizeof(struct smack_known *), >> .lbs_msg_msg = sizeof(struct smack_known *), >> .lbs_superblock = sizeof(struct superblock_smack), >> + .lbs_xattr_count = SMACK_INODE_INIT_XATTRS, >> }; >> >> static struct security_hook_list smack_hooks[] __lsm_ro_after_init = { >> -- >> 2.25.1
