On Thu, Jul 16, 2026 at 5:55 PM Paul Moore <[email protected]> wrote:
>
> > +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.
>
Will do.
> > +#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.
>
Ack
> > +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).
>
The problem is, SELinux has already set the sid by before calling
lsm_get_xattr_slot, so we do indeed get an in-core label for the inode
before returning early here. Returning early here from BPF would not
mean the same thing: there is no label anywhere, in memory or on disk.
> > + 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.
>
Sounds good, will refactor to remove the conditional.
> > + 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.
>
Yup
>
> --
> paul-moore.com
Thanks,
David
