linux-kernel - Re: [PATCH v11 bpf-next 1/7] fs/xattr: bpf: Introduce security.bpf. xattr name prefix

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7F32DAA4-1B0D-4702-AAE3-AA742092F192@fb.com>
Date: Thu, 30 Jan 2025 18:08:14 +0000
From: Song Liu <songliubraving@...a.com>
To: Matt Bobrowski <mattbobrowski@...gle.com>
CC: Song Liu <song@...nel.org>, "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-security-module@...r.kernel.org"
	<linux-security-module@...r.kernel.org>,
        Kernel Team <kernel-team@...a.com>,
        "andrii@...nel.org" <andrii@...nel.org>,
        "eddyz87@...il.com"
	<eddyz87@...il.com>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "martin.lau@...ux.dev"
	<martin.lau@...ux.dev>,
        "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
        "brauner@...nel.org" <brauner@...nel.org>,
        "jack@...e.cz" <jack@...e.cz>,
        "kpsingh@...nel.org" <kpsingh@...nel.org>,
        Liam Wisehart
	<liamwisehart@...a.com>,
        Shankaran Gnanashanmugam <shankaran@...a.com>
Subject: Re: [PATCH v11 bpf-next 1/7] fs/xattr: bpf: Introduce security.bpf.
 xattr name prefix

Hi Matt,

> On Jan 30, 2025, at 2:57 AM, Matt Bobrowski <mattbobrowski@...gle.com> wrote:
> 
> On Wed, Jan 29, 2025 at 12:59:51PM -0800, Song Liu wrote:
>> 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@...gle.com>

Thanks for the review!

[...]
> 
>> - * 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

> 
>> 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
> 
>> + * "security.bpf." is allowed.
>            ^ are
> 
>> - * 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
>>