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? > #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 -- paul-moore.com
