Hi Roberto, Sorry for the delay in responding... The patch description reads as though support for per LSM multiple xattrs is being added in this patch, though lsm_get_xattr_slot() only ever is incremented once for each LSM. To simplify review, it would be nice to mention that lsm_get_xattr_slot() would be called multiple times per LSM xattr. On Fri, 2023-03-31 at 14:32 +0200, Roberto Sassu 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. The number of security xattrs permitted per LSM was discussed in the second paragraph. The first line of this paragraph needs to be updated to reflect the current number of security xattrs used, though that is more related to the new lsm_get_xattr_slot(). Or perhaps the entire paragraph is unnecessary, a remnant from security_check_compact_filled_xattrs(), and should be removed. > > 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(). This Smack comment should be moved to the previous paragraph and even expanded explaining that lsm_get_xattr_slot() will be called for each additional security xattr. > > 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> > --- > 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)++; At some point, since lsm_get_xattr_slot() could be called multiple times from the same LSM, shouldn't there be some sort of bounds checking? -- thanks, Mimi
