Re: [PATCH v5 1/3] security: rework inode_init_security xattr handling

On Jul  7, 2026 David Windsor <[email protected]> wrote:
> 
> In preparation for bpf_init_inode_xattr(), a kfunc that lets bpf LSM
> programs atomically label new inodes, rework how inode_init_security
> xattrs are managed.
> 
> inode_init_security receives the LSM xattr array and its count as
> separate parameters. For better compatibility with the bpf verifier,
> update inode_init_security and its callers to consolidate these
> parameters into a single context object: struct lsm_xattrs.
> 
> Also, add security_lsmxattr_add(), which claims a slot in the
> inode_init_security xattr array on behalf of the calling LSM and
> fills it with a copy of the given name and value.
> 
> Suggested-by: Paul Moore <[email protected]>
> Signed-off-by: David Windsor <[email protected]>
> ---
>  include/linux/bpf_lsm.h           |   3 +
>  include/linux/evm.h               |   9 +--
>  include/linux/lsm_hook_defs.h     |   4 +-
>  include/linux/lsm_hooks.h         |  16 ++---
>  include/linux/security.h          |  15 +++++
>  security/bpf/hooks.c              |   1 +
>  security/integrity/evm/evm_main.c |   8 ++-
>  security/security.c               | 108 ++++++++++++++++++++++++++----
>  security/selinux/hooks.c          |   4 +-
>  security/smack/smack_lsm.c        |  27 ++++----
>  10 files changed, 148 insertions(+), 47 deletions(-)

...

> diff --git a/include/linux/security.h b/include/linux/security.h
> index 153e9043058f..647f7b88358b 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -68,6 +68,11 @@ struct watch;
>  struct watch_notification;
>  struct lsm_ctx;
>  
> +struct lsm_xattrs {
> +	struct xattr *xattrs;
> +	unsigned int xattr_count;
> +};

Please separate out the 'struct lsm_xattrs' related changes into a
separate patch from the security_lsmxattr_add() changes.  I know they
are related, but they are different things.

> diff --git a/security/security.c b/security/security.c
> index 71aea8fdf014..261f68e17cfd 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -12,6 +12,7 @@
>  #define pr_fmt(fmt) "LSM: " fmt
>  
>  #include <linux/bpf.h>
> +#include <linux/bpf_lsm.h>
>  #include <linux/capability.h>
>  #include <linux/dcache.h>
>  #include <linux/export.h>
> @@ -1333,8 +1334,8 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>  				 const initxattrs initxattrs, void *fs_data)
>  {
>  	struct lsm_static_call *scall;
> -	struct xattr *new_xattrs = NULL;
> -	int ret = -EOPNOTSUPP, xattr_count = 0;
> +	struct lsm_xattrs xattrs = {};
> +	int ret = -EOPNOTSUPP;
>  
>  	if (unlikely(IS_PRIVATE(inode)))
>  		return 0;
> @@ -1344,15 +1345,15 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>  
>  	if (initxattrs) {
>  		/* Allocate +1 as terminator. */
> -		new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 1,
> -				     sizeof(*new_xattrs), GFP_NOFS);
> -		if (!new_xattrs)
> +		xattrs.xattrs = kcalloc(blob_sizes.lbs_xattr_count + 1,
> +					sizeof(*xattrs.xattrs), GFP_NOFS);
> +		if (!xattrs.xattrs)
>  			return -ENOMEM;
>  	}
>  
>  	lsm_for_each_hook(scall, inode_init_security) {
> -		ret = scall->hl->hook.inode_init_security(inode, dir, qstr, new_xattrs,
> -						  &xattr_count);
> +		ret = scall->hl->hook.inode_init_security(inode, dir, qstr,
> +							  &xattrs);
>  		if (ret && ret != -EOPNOTSUPP)
>  			goto out;
>  		/*
> @@ -1364,18 +1365,101 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>  	}
>  
>  	/* If initxattrs() is NULL, xattr_count is zero, skip the call. */
> -	if (!xattr_count)
> +	if (!xattrs.xattr_count)
>  		goto out;
>  
> -	ret = initxattrs(inode, new_xattrs, fs_data);
> +	ret = initxattrs(inode, xattrs.xattrs, fs_data);
>  out:
> -	for (; xattr_count > 0; xattr_count--)
> -		kfree(new_xattrs[xattr_count - 1].value);
> -	kfree(new_xattrs);
> +	for (; xattrs.xattr_count > 0; xattrs.xattr_count--)
> +		kfree(xattrs.xattrs[xattrs.xattr_count - 1].value);
> +	kfree(xattrs.xattrs);
>  	return (ret == -EOPNOTSUPP) ? 0 : ret;
>  }
>  EXPORT_SYMBOL(security_inode_init_security);
>  
> +#ifdef CONFIG_BPF_LSM
> +static unsigned int lsm_xattrs_used(const struct lsm_xattrs *xattrs,
> +				    const char *prefix)
> +{
> +	size_t prefix_len = strlen(prefix);
> +	unsigned int i, n = 0;
> +
> +	for (i = 0; i < xattrs->xattr_count; i++) {
> +		const char *name = xattrs->xattrs[i].name;
> +
> +		if (name && !strncmp(name, prefix, prefix_len))
> +			n++;
> +	}
> +	return n;
> +}
> +#endif /* CONFIG_BPF_LSM */

More on this below, but this function isn't strictly BPF LSM related so
let's drop the CONFIG_BPF_LSM macro bracketing.

> +/**
> + * security_lsmxattr_add() - Add an xattr during inode_init_security
> + * @xattrs: xattr state shared by inode_init_security hooks
> + * @lsm_id: LSM_ID_* value identifying the calling LSM
> + * @name: xattr name suffix

For BPF you currently pass "bpf.foo" for the name and other LSMs would
pass their own xattr name, likely without a LSM specific sub-namespace
(for example, SELinux would be just "selinux").  In both cases the
'name' parameter always starts with a well known suffix as defined by
the 'lsm_id" parameter.

Since we are already passing the lsm_id parameter, let's do away with
the standard LSM suffixes, e.g. XATTR_BPF_LSM_SUFFIX, and just pass in
any additional name components.  For example, instead of passing
"bpf.foo" in the BPF LSM case, you would just pass "foo"; LSMs without
their own sub-namespace, e.g. SELinux, would pass NULL for the name
parameter as XATTR_SELINUX_SUFFIX is all that is needed.  While doing
this I would also suggest changing the name of the 'name' parameter to
'name_extra', 'namespace_extra', or something similar to indicate that
it isn't the full name, but rather an additional suffix beyond the
standard suffix associated with the given LSM.

> + * @value: xattr value
> + * @value_len: length of @value
> + *
> + * Claim an xattr slot in @xattrs on behalf of the LSM identified by
> + * @lsm_id and fill it with a copy of @name and @value. Callers can invoke
> + * this function from non-sleepable context.
> + *
> + * Return: Returns 0 on success, -ENOSPC if the calling LSM's slot budget
> + *         is exhausted, negative values on other errors.
> + */
> +int security_lsmxattr_add(struct lsm_xattrs *xattrs, u64 lsm_id,
> +			  const char *name, const void *value,
> +			  size_t value_len)
> +{
> +	struct xattr *xattr;
> +	void *xattr_value;
> +	size_t name_len;
> +
> +	if (!xattrs || !xattrs->xattrs || !name || !value)
> +		return -EINVAL;

Sashiko raised a good point about xattrs->xattrs being NULL not
necessarily being a good reason for -EINVAL.  If xattrs is NULL, yes,
something has gone wrong and -EINVAL seems reasonable, but the
xattr->xattrs NULL case does seem like it should simply return early
with a value of 0 (see SELinux's handling of this case as an example).

> +	name_len = strlen(name);
> +	if (name_len == 0 || name_len > XATTR_NAME_MAX)
> +		return -EINVAL;
> +	if (value_len == 0 || value_len > XATTR_SIZE_MAX)
> +		return -EINVAL;
> +
> +	switch (lsm_id) {
> +#ifdef CONFIG_BPF_LSM
> +	case LSM_ID_BPF:
> +		if (lsm_xattrs_used(xattrs, XATTR_BPF_LSM_SUFFIX) >=
> +		    BPF_LSM_INODE_INIT_XATTRS)
> +			return -ENOSPC;
> +		break;
> +#endif /* CONFIG_BPF_LSM */

I like to avoid macro conditional code inside functions whenever
possible, and I think this is a case where we could avoid the conditional
block with a little work.

The LSM_ID_BPF macro is already defined as part of the UAPI so that will
always be available.  While BPF_LSM_INODE_INIT_XATTRS is dependent on
CONFIG_BPF_LSM in this revision, that should be easy enough to move
outside the CONFIG_BPF_LSM conditional in bpf_lsm.h.  Eventually we
should probably expand the lsm_id struct to carry this info, likely just
a permanent/local copy of the LSM's lsm_blob_sizes passed during
registration, but I can take care of that later; just make the
BPF_LSM_INODE_INIT_XATTRS macro always accessible now.

We should also probably record the xattr suffix/length when the LSM is
registered, but that can also be done later with the other lsm_id
additions.

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* Combine xattr value + name into one allocation. */
> +	xattr_value = kmalloc(value_len + name_len + 1, GFP_NOWAIT);
> +	if (!xattr_value)
> +		return -ENOMEM;
> +
> +	memcpy(xattr_value, value, value_len);
> +	memcpy(xattr_value + value_len, name, name_len);
> +	((char *)xattr_value)[value_len + name_len] = '\0';

You'll need to add an additional memcpy() here, likely in a switch
statement to handle the different LSMs, to copy over the LSM specific
prefix.  It's a little ugly, but when we have things captured in the
lsm_id struct it will get a lot cleaner.

> +	xattr = lsm_get_xattr_slot(xattrs);
> +	if (!xattr) {
> +		kfree(xattr_value);
> +		return -ENOSPC;
> +	}
> +
> +	xattr->value = xattr_value;
> +	xattr->name = (const char *)xattr_value + value_len;
> +	xattr->value_len = value_len;
> +
> +	return 0;
> +}

--
paul-moore.com