On 3/23/2023 5:09 PM, Paul Moore wrote: > On Tue, Mar 14, 2023 at 4:19 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 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 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. Update the documentation accordingly, and fix the >> description of the xattr name, as it is not allocated anymore. >> >> Since the LSM infrastructure, at initialization time, updates the number of >> the requested xattrs provided by each LSM with a corresponding offset in >> the security blob (in this case the xattr array), it makes straightforward >> for an LSM to access the right position in the xattr array. >> >> There is still the issue that an LSM might not fill the xattr, even if it >> requests it (legitimate case, for example it might have been loaded but not >> initialized with a policy). Since users of the xattr array (e.g. the >> initxattrs() callbacks) detect the end of the xattr array by checking if >> the xattr name is NULL, not filling an xattr would cause those users to >> stop scanning xattrs prematurely. >> >> Solve that issue by introducing security_check_compact_filled_xattrs(), >> which does a basic check of the xattr array (if the xattr name is filled, >> the xattr value should be too, and viceversa), and compacts the xattr array >> by removing the holes. >> >> An alternative solution would be to let users of the xattr array know the >> number of elements of that array, so that they don't have to check the >> termination. However, this seems more invasive, compared to a simple move >> of few array elements. >> >> security_check_compact_filled_xattrs() also determines how many xattrs in >> the xattr array have been filled. If there is none, skip >> evm_inode_init_security() and initxattrs(). Skipping the former also avoids >> EVM to crash the kernel, as it is expecting a filled xattr. >> >> Finally, adapt both SELinux and Smack to use the new definition of the >> inode_init_security hook, and to correctly fill the designated slots in the >> xattr array. For Smack, 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> >> Reviewed-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx> >> Reviewed-by: Mimi Zohar <zohar@xxxxxxxxxxxxx> >> --- >> include/linux/lsm_hook_defs.h | 3 +- >> include/linux/lsm_hooks.h | 1 + >> security/security.c | 119 +++++++++++++++++++++++++++++----- >> security/selinux/hooks.c | 19 ++++-- >> security/smack/smack_lsm.c | 33 ++++++---- >> 5 files changed, 137 insertions(+), 38 deletions(-) >> >> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h >> index 6bb55e61e8e..b814955ae70 100644 >> --- a/include/linux/lsm_hook_defs.h >> +++ b/include/linux/lsm_hook_defs.h >> @@ -112,8 +112,7 @@ 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) >> 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..75a2f85b49d 100644 >> --- a/include/linux/lsm_hooks.h >> +++ b/include/linux/lsm_hooks.h >> @@ -63,6 +63,7 @@ struct lsm_blob_sizes { >> int lbs_ipc; >> int lbs_msg_msg; >> int lbs_task; >> + int lbs_xattr; /* number of xattr slots in new_xattrs array */ > No need for the comment, we don't do it for the other fields. I asked for the comment. lbs_xattr is the number of entries, which is different from the other fields. The other fields contain blob sizes in bytes. Inconsistent behavior should be noted. > >> }; >> >> /* >> diff --git a/security/security.c b/security/security.c >> index f4170efcddd..f1f5f62f7fa 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -1579,6 +1579,52 @@ int security_dentry_create_files_as(struct dentry *dentry, int mode, >> } >> EXPORT_SYMBOL(security_dentry_create_files_as); >> >> +/** >> + * security_check_compact_filled_xattrs - check xattrs and make array contiguous >> + * @xattrs: xattr array filled by LSMs >> + * @num_xattrs: length of xattr array >> + * @num_filled_xattrs: number of already processed xattrs >> + * >> + * Ensure that each xattr slot is correctly filled and close the gaps in the >> + * xattr array if an LSM didn't provide an xattr for which it asked space >> + * (legitimate case, it might have been loaded but not initialized). An LSM >> + * might request space in the xattr array for one or multiple xattrs. The LSM >> + * infrastructure ensures that all requests by LSMs are satisfied. >> + * >> + * Track the number of filled xattrs in @num_filled_xattrs, so that it is easy >> + * to determine whether the currently processed xattr is fine in its position >> + * (if all previous xattrs were filled) or it should be moved after the last >> + * filled xattr. >> + * >> + * Return: zero if all xattrs are valid, -EINVAL otherwise. >> + */ >> +static int security_check_compact_filled_xattrs(struct xattr *xattrs, >> + int num_xattrs, >> + int *num_filled_xattrs) > That is one long name :) > > Since you're making some other changes to this patch, can you rename > this to security_xattr_compact() or something like that? > >> +{ >> + int i; >> + >> + for (i = *num_filled_xattrs; i < num_xattrs; i++) { >> + if ((!xattrs[i].name && xattrs[i].value) || >> + (xattrs[i].name && !xattrs[i].value)) >> + return -EINVAL; >> + >> + if (!xattrs[i].name) >> + continue; >> + >> + if (i == *num_filled_xattrs) { >> + (*num_filled_xattrs)++; >> + continue; >> + } >> + >> + memcpy(xattrs + (*num_filled_xattrs)++, xattrs + i, >> + sizeof(*xattrs)); >> + memset(xattrs + i, 0, sizeof(*xattrs)); >> + } >> + >> + return 0; >> +} >> + >> /** >> * security_inode_init_security() - Initialize an inode's LSM context >> * @inode: the inode >> @@ -1591,9 +1637,13 @@ 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 >> + * hooks called by the VFS. The hook function is expected to populate the >> + * @xattrs array, depending on how many xattrs have been specified by the >> + * security module in the lbs_xattr field of the lsm_blob_sizes structure. For >> + * each array element, the hook function is expected to 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. >> * >> @@ -1604,33 +1654,66 @@ 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; >> + struct xattr *xattr; >> + int ret = -EOPNOTSUPP, num_filled_xattrs = 0; >> >> if (unlikely(IS_PRIVATE(inode))) >> return 0; >> >> + if (!blob_sizes.lbs_xattr) >> + 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) >> + dir, qstr, NULL); >> + /* Allocate +1 for EVM and +1 as terminator. */ >> + new_xattrs = kcalloc(blob_sizes.lbs_xattr + 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); >> + 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 (ret == -EOPNOTSUPP) >> + continue; >> + /* >> + * As the number of xattrs reserved by LSMs is not directly >> + * available, directly use the total number blob_sizes.lbs_xattr >> + * to keep the code simple, while being not the most efficient >> + * way. >> + */ > Is there a good reason why the LSM can't return the number of xattrs > it is adding to the xattr array? It seems like it should be fairly > trivial for the individual LSMs to determine and it could save a lot > of work. However, given we're at v8 on this patchset I'm sure I'm > missing something obvious, can you help me understand why the idea > above is crazy stupid? ;) > >> + ret = security_check_compact_filled_xattrs(new_xattrs, >> + blob_sizes.lbs_xattr, >> + &num_filled_xattrs); >> + if (ret < 0) { >> + ret = -ENOMEM; >> + goto out; >> + } >> + } >> + >> + if (!num_filled_xattrs) >> 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 + num_filled_xattrs); >> if (ret) >> goto out; >> ret = initxattrs(inode, new_xattrs, fs_data); >> out: >> for (xattr = new_xattrs; xattr->value != NULL; xattr++) >> kfree(xattr->value); >> + kfree(new_xattrs); >> return (ret == -EOPNOTSUPP) ? 0 : ret; >> } >> EXPORT_SYMBOL(security_inode_init_security); >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> index 9a5bdfc2131..3e4308dd336 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -104,6 +104,8 @@ >> #include "audit.h" >> #include "avc_ss.h" >> >> +#define SELINUX_INODE_INIT_XATTRS 1 >> + >> struct selinux_state selinux_state; >> >> /* SECMARK reference count */ >> @@ -2868,11 +2870,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) >> { >> const struct task_security_struct *tsec = selinux_cred(current_cred()); >> struct superblock_security_struct *sbsec; >> + struct xattr *xattr = NULL; >> u32 newsid, clen; >> int rc; >> char *context; >> @@ -2899,16 +2901,18 @@ 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 (xattrs) >> + xattr = xattrs + selinux_blob_sizes.lbs_xattr; > Please abstract that away to an inline function similar to > selinux_cred(), selinux_file(), selinux_inode(), etc. > >> + if (xattr) { >> + xattr->name = XATTR_SELINUX_SUFFIX; > I'm guessing the xattr->name assignment is always done, regardless of > if security_sid_to_context_force() is successful, due to the -EINVAL > check in security_check_compact_filled_xattrs()? If yes, it would be > good to make note of that here in the code. If not, it would be nice > to move this down the function to go with the other xattr->XXX > assignments, unless there is another reason for its placement that I'm > missing. > >> - 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; >> @@ -6918,6 +6922,7 @@ struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = { >> .lbs_ipc = sizeof(struct ipc_security_struct), >> .lbs_msg_msg = sizeof(struct msg_security_struct), >> .lbs_superblock = sizeof(struct superblock_security_struct), >> + .lbs_xattr = SELINUX_INODE_INIT_XATTRS, >> }; >> >> #ifdef CONFIG_PERF_EVENTS >> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c >> index cfcbb748da2..c8cf8df268b 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, >> + * SMACK64MMAP - controls library loading, >> + * SMACK64TRANSMUTE - label initialization, >> + * Not saved on files - SMACK64IPIN and SMACK64IPOUT >> + */ >> +#define SMACK_INODE_INIT_XATTRS 4 >> + >> #ifdef SMACK_IPV6_PORT_LABELING >> static DEFINE_MUTEX(smack_ipv6_lock); >> static LIST_HEAD(smk_ipv6_port_list); >> @@ -939,26 +948,27 @@ 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 >> * >> * 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) >> { >> 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 = NULL; >> int may; >> >> - if (name) >> - *name = XATTR_SMACK_SUFFIX; >> + if (xattrs) >> + xattr = xattrs + smack_blob_sizes.lbs_xattr; >> + >> + 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); >> @@ -976,11 +986,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; >> @@ -4854,6 +4864,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 = SMACK_INODE_INIT_XATTRS, >> }; >> >> static struct security_hook_list smack_hooks[] __lsm_ro_after_init = { >> -- >> 2.25.1
