> From: Paul Moore [mailto:paul@xxxxxxxxxxxxxx] > Sent: Friday, July 7, 2023 3:44 AM > On Jun 10, 2023 Roberto Sassu <roberto.sassu@xxxxxxxxxxxxxxx> wrote: > > > > Currently, the LSM infrastructure 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. > > > > Modify the inode_init_security hook definition, by passing the full > > xattr array allocated in security_inode_init_security(), and the current > > number of xattr slots in that array filled by LSMs. The first parameter > > would allow EVM to access and calculate the HMAC on xattrs supplied by > > other LSMs, the second to not leave gaps in the xattr array, when an LSM > > requested but did not provide xattrs (e.g. if it is not initialized). > > > > Introduce lsm_get_xattr_slot(), which LSMs can call as many times as the > > number specified in the lbs_xattr_count field of the lsm_blob_sizes > > structure. During each call, lsm_get_xattr_slot() increments the number of > > filled xattrs, so that at the next invocation it returns the next xattr > > slot to fill. > > > > Cleanup security_inode_init_security(). Unify the !initxattrs and > > initxattrs case by simply not allocating the new_xattrs array in the > > former. Update the documentation to reflect the changes, and fix the > > description of the xattr name, as it is not allocated anymore. > > > > Adapt both SELinux and Smack to use the new definition of the > > inode_init_security hook, and to call lsm_get_xattr_slot() to obtain and > > fill the reserved slots in the xattr array. > > > > Move the xattr->name assignment after the xattr->value one, so that it is > > done only in case of successful memory allocation. > > > > Finally, change the default return value of the inode_init_security hook > > from zero to -EOPNOTSUPP, so that BPF LSM correctly follows the hook > > conventions. > > > > Reported-by: Nicolas Bouchinet <nicolas.bouchinet@xxxxxxxxxxx> > > 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 | 25 ++++++------ > > 5 files changed, 92 insertions(+), 47 deletions(-) > > Two *very* small suggestions below, but I can make those during the > merge if you are okay with that Roberto? Hi Paul yes, sure, I'm ok with them. Please make them during the merge. Thanks Roberto > I'm also going to assume that Casey is okay with the Smack portion of > this patchset? It looks fine to me, and considering his ACK on the > other Smack patch in this patchset I'm assuming he is okay with this > one as well ... ? > > > diff --git a/security/security.c b/security/security.c > > index ee4f1cc4902..d5ef7df1ce4 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -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 > > I think we want to change "@xattrs array" to just "xattrs array" as > there is no function parameter named "xattrs" in the LSM/security_XXX > hook itself, just in the 'inode_init_security' hook implementation. > > I might also break the new text describing the hook implementation > into a new paragraph. > > > + * 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; > > The above comments were nitpicky, this one is even more so ... > convention within security/security.c is to call the > security_hook_list pointer "hp", not "P" (although I recognize P is > used in the macro). > > > + struct xattr *new_xattrs = NULL; > > + int ret = -EOPNOTSUPP, xattr_count = 0; > > -- > paul-moore.com
