On 7/6/2023 6:43 PM, Paul Moore wrote: > 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? > > 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 ... ? Yes, please feel free to add my Acked-by as needed. > >> 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
