Re: [PATCH v6 bpf-next 2/4] security: add security_lsmxattr_add()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The dynamically allocated `xattr->name` pointer is used after being freed by `security_inode_init_security()` in the ocfs2 filesystem.
--

--- Patch [2]: [PATCH v6 bpf-next 2/4] security: add security_lsmxattr_add() ---
commit cefa37d110516b84cf53133f4f60bba93ba53641
Author: David Windsor <[email protected]>

security: add security_lsmxattr_add()
    
This commit adds security_lsmxattr_add(), which claims a slot in the
inode_init_security xattr array on behalf of the calling LSM. It fills the
slot with a dynamically allocated copy of the given name and value.

> diff --git a/security/security.c b/security/security.c
> index 2ad7f09c1a611..ae72102cd29b8 100644
> --- a/security/security.c
> +++ b/security/security.c
[ ... ]
> @@ -1376,6 +1377,101 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
[ ... ]
> +int security_lsmxattr_add(struct lsm_xattrs *xattrs, u64 lsm_id,
> +			  const char *name_extra, const void *value,
> +			  size_t value_len)
> +{
[ ... ]
> +	/* 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, suffix, suffix_len);
> +	if (extra_len)
> +		memcpy(xattr_value + value_len + suffix_len, name_extra,
> +		       extra_len);
> +	((char *)xattr_value)[value_len + name_len] = '\0';
> +
> +	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;

[Severity: High]
Does this code cause a use-after-free for filesystems like ocfs2?

Looking at ocfs2, the ocfs2_initxattrs() callback saves the xattr->name
pointer directly into si->name, assuming it has a static lifetime (which
was true for SELinux and Smack):

fs/ocfs2/xattr.c:ocfs2_initxattrs() {
    ...
    si->name = xattr_array->name;
    ...
}

But here xattr->name points inside the dynamically allocated xattr->value
block. When security_inode_init_security() completes, it frees
xattr->value:

security/security.c:security_inode_init_security() {
    ...
    ret = initxattrs(inode, xattrs.xattrs, fs_data);
out:
    for (; xattrs.xattr_count > 0; xattrs.xattr_count--)
        kfree(xattrs.xattrs[xattrs.xattr_count - 1].value);
    ...
}

After this free, ocfs2_mknod() will call ocfs2_calc_xattr_init(), which
dereferences the now-dangling si->name pointer:

fs/ocfs2/xattr.c:ocfs2_calc_xattr_init() {
    ...
    s_size = ocfs2_xattr_entry_real_size(strlen(si->name),
                                         si->value_len);
    ...
}

Could the dynamically allocated name trigger a use-after-free here?

> +	xattr->value_len = value_len;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2