Hi Matt,

On Jan 30, 2025, at 2:57 AM, Matt Bobrowski [off-list ref] wrote:

On Wed, Jan 29, 2025 at 12:59:51PM -0800, Song Liu wrote:

quoted

Introduct new xattr name prefix security.bpf., and enable reading these
xattrs from bpf kfuncs bpf_get_[file|dentry]_xattr().

As we are on it, correct the comments for return value of
bpf_get_[file|dentry]_xattr(), i.e. return length the xattr value on
success.

Reviewed-by: Matt Bobrowski <mattbobrowski@google.com>

Thanks for the review!

[...]

quoted

- * Return: 0 on success, a negative value on error.
+ * Return: length of the xattr value on success, a negative value on error.
 */
__bpf_kfunc int bpf_get_dentry_xattr(struct dentry *dentry, const char *name__str,
    struct bpf_dynptr *value_p)

@@ -117,7 +123,9 @@ __bpf_kfunc int bpf_get_dentry_xattr(struct dentry *dentry, const char *name__st

if (WARN_ON(!inode))
return -EINVAL;

- if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN))
+ /* Allow reading xattr with user. and security.bpf. prefix */
+ if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN) &&
+    !match_security_bpf_prefix(name__str))

I think it would be cleaner to have single function
i.e. is_allowed_xattr_prefix(const char *name__str) which simply
checks all the allowed xattr prefixes that can be read by this BPF
kfunc.

Sure, we can add bpf_xattr_read_permission() which pairs with 
bpf_xattr_write_permission(). 

Thanks,
Song

quoted

return -EPERM;

value_len = __bpf_dynptr_size(value_ptr);

@@ -139,9 +147,10 @@ __bpf_kfunc int bpf_get_dentry_xattr(struct dentry *dentry, const char *name__st
 *
 * Get xattr *name__str* of *file* and store the output in *value_ptr*.
 *
- * For security reasons, only *name__str* with prefix "user." is allowed.
+ * For security reasons, only *name__str* with prefix "user." or

                     ^ prefixes

quoted

+ * "security.bpf." is allowed.

           ^ are

quoted

- * Return: 0 on success, a negative value on error.
+ * Return: length of the xattr value on success, a negative value on error.
 */
__bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str,
  struct bpf_dynptr *value_p)

diff --git a/include/uapi/linux/xattr.h b/include/uapi/linux/xattr.h
index 9854f9cff3c6..c7c85bb504ba 100644
--- a/include/uapi/linux/xattr.h
+++ b/include/uapi/linux/xattr.h

@@ -83,6 +83,10 @@ struct xattr_args {

#define XATTR_CAPS_SUFFIX "capability"
#define XATTR_NAME_CAPS XATTR_SECURITY_PREFIX XATTR_CAPS_SUFFIX

+#define XATTR_BPF_LSM_SUFFIX "bpf."
+#define XATTR_NAME_BPF_LSM (XATTR_SECURITY_PREFIX XATTR_BPF_LSM_SUFFIX)
+#define XATTR_NAME_BPF_LSM_LEN (sizeof(XATTR_NAME_BPF_LSM) - 1)
+
#define XATTR_POSIX_ACL_ACCESS  "posix_acl_access"
#define XATTR_NAME_POSIX_ACL_ACCESS XATTR_SYSTEM_PREFIX XATTR_POSIX_ACL_ACCESS
#define XATTR_POSIX_ACL_DEFAULT  "posix_acl_default"
-- 
2.43.5