hOn Thu, 2022-11-10 at 10:46 +0100, 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 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.
Perhaps supporting per LSM multiple xattrs is a nice idea, but EVM
doesn't currently support it. The LSM xattrs are hard coded in
evm_config_default_xattrnames[], based on whether the LSM is
configured. Additional security xattrs may be included in the
security.evm calculation, by extending the list via
security/integrity/evm/evm_xattrs.
>
> 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.
>
> 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_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 the xattr 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.
>
> 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.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> ---
> diff --git a/security/security.c b/security/security.c
> index a0e9b4ce2341..b62f192de6da 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -30,8 +30,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)
>
> @@ -210,6 +208,7 @@ 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, &blob_sizes.lbs_xattr);
> }
>
> /* Prepare LSM for initialization. */
> @@ -346,6 +345,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);
>
> /*
> * Create any kmem_caches needed for blobs
> @@ -1100,34 +1100,78 @@ static int security_initxattrs(struct inode *inode, const struct xattr *xattrs,
> return 0;
> }
> +static int security_check_compact_xattrs(struct xattr *xattrs,
> + int num_xattrs, int *checked_xattrs)
Perhaps the variable naming is off, making it difficult to read. So
although this is a static function, which normally doesn't require a
comment, it's definitely needs one.
> +{
> + int i;
> +
> + for (i = *checked_xattrs; i < num_xattrs; i++) {
If the number of "checked" xattrs was kept up to date, removing the
empty xattr gaps wouldn't require a loop. Is the purpose of this loop
to support multiple per LSM xattrs?
> + if ((!xattrs[i].name && xattrs[i].value) ||
> + (xattrs[i].name && !xattrs[i].value))
> + return -EINVAL;
> +
> + if (!xattrs[i].name)
> + continue;
> +
> + if (i == *checked_xattrs) {
> + (*checked_xattrs)++;
> + continue;
> + }
> +
> + memcpy(xattrs + (*checked_xattrs)++, xattrs + i,
> + sizeof(*xattrs));
> + memset(xattrs + i, 0, sizeof(*xattrs));
> + }
> +
> + return 0;
> +}
> +
> 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 = -EOPNOTSUPP;
> + struct security_hook_list *P;
> + struct xattr *new_xattrs;
> + struct xattr *xattr;
> + int ret = -EOPNOTSUPP, cur_xattrs = 0;
>
> if (unlikely(IS_PRIVATE(inode)))
> goto out_exit;
>
> + if (!blob_sizes.lbs_xattr)
> + goto out_exit;
> +
> if (!initxattrs ||
> (initxattrs == &security_initxattrs && !fs_data)) {
> ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
> - dir, qstr, NULL, NULL, NULL);
> + dir, qstr, NULL);
> goto out_exit;
> }
> - 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)
> - goto out;
> + /* Allocate +1 for EVM and +1 as terminator. */
> + new_xattrs = kcalloc(blob_sizes.lbs_xattr + 2, sizeof(*new_xattrs),
> + GFP_NOFS);
> + if (!new_xattrs) {
> + ret = -ENOMEM;
> + goto out_exit;
> + }
> + 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;
> + if (ret == -EOPNOTSUPP)
> + continue;
> + ret = security_check_compact_xattrs(new_xattrs,
> + blob_sizes.lbs_xattr,
> + &cur_xattrs);
Defining a variable named "cur_xattrs" to indicate the number of xattrs
compressed is off. Perhaps use cur_num_xattrs? Similarly,
"checked_xattrs" should be num_checked_xattrs. Or change the existing
num_xattrs to max_num_xattrs and rename checked_xattrs to num_xattrs.
thanks,
Mimi
> + if (ret < 0) {
> + ret = -ENOMEM;
> + 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 + cur_xattrs);
> if (ret)
> goto out;
> ret = initxattrs(inode, new_xattrs, fs_data);
> @@ -1142,6 +1186,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> continue;
> kfree(xattr->value);
> }
> + kfree(new_xattrs);
> out_exit:
> if (initxattrs == &security_initxattrs)
> return ret;
