linux-kernel - Re: [PATCH v9 bpf-next 6/7] bpf: fs/xattr: Add BPF kfuncs to set and remove xattrs

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] [day] [month] [year] [list]

Message-ID: <79B1F200-403F-427A-8FAF-01D1DC485452@fb.com>
Date: Fri, 24 Jan 2025 17:22:23 +0000
From: Song Liu <songliubraving@...a.com>
To: Christian Brauner <brauner@...nel.org>
CC: Song Liu <song@...nel.org>, "bpf@...r.kernel.org" <bpf@...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>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "martin.lau@...ux.dev"
	<martin.lau@...ux.dev>,
        "kpsingh@...nel.org" <kpsingh@...nel.org>,
        "mattbobrowski@...gle.com" <mattbobrowski@...gle.com>,
        "paul@...l-moore.com"
	<paul@...l-moore.com>,
        "jmorris@...ei.org" <jmorris@...ei.org>,
        "serge@...lyn.com" <serge@...lyn.com>,
        "memxor@...il.com" <memxor@...il.com>
Subject: Re: [PATCH v9 bpf-next 6/7] bpf: fs/xattr: Add BPF kfuncs to set and
 remove xattrs

Hi Christian, 

Thanks for the review!

> On Jan 24, 2025, at 1:05 AM, Christian Brauner <brauner@...nel.org> wrote:
> 
> On Thu, Jan 09, 2025 at 05:13:41PM -0800, Song Liu wrote:
>> Add the following kfuncs to set and remove xattrs from BPF programs:
>> 
>>  bpf_set_dentry_xattr
>>  bpf_remove_dentry_xattr
>>  bpf_set_dentry_xattr_locked
>>  bpf_remove_dentry_xattr_locked
>> 
>> The _locked version of these kfuncs are called from hooks where
>> dentry->d_inode is already locked. Instead of requiring the user
>> to know which version of the kfuncs to use, the verifier will pick
>> the proper kfunc based on the calling hook.
>> 
>> Signed-off-by: Song Liu <song@...nel.org>

[...]

>> +
>> + ret = __vfs_setxattr(&nop_mnt_idmap, dentry, inode, name,
>> +      value, value_len, flags);
>> + if (!ret) {
>> + fsnotify_xattr(dentry);
>> +
>> + /* This xattr is set by BPF LSM, so we do not call
>> +  * security_inode_post_setxattr. This is the same as
>> +  * security_inode_setsecurity().
>> +  */
> 
> If you did you would risk deadlocks as you could end up calling yourself
> again afaict.

Exactly. Let state it more clearly in the comment. 

> 
>> + }
>> +out:
>> + if (lock_inode)
>> + inode_unlock(inode);
>> + return ret;
>> +}
>> +
>> +/**
>> + * bpf_set_dentry_xattr - set a xattr of a dentry
>> + * @dentry: dentry to get xattr from
>> + * @name__str: name of the xattr
>> + * @value_p: xattr value
>> + * @flags: flags to pass into filesystem operations
>> + *
>> + * Set xattr *name__str* of *dentry* to the value in *value_ptr*.
>> + *
>> + * For security reasons, only *name__str* with prefix "security.bpf."
>> + * is allowed.
>> + *
>> + * The caller has not locked dentry->d_inode.
>> + *
>> + * Return: 0 on success, a negative value on error.
>> + */
>> +__bpf_kfunc int bpf_set_dentry_xattr(struct dentry *dentry, const char *name__str,
>> +      const struct bpf_dynptr *value_p, int flags)
>> +{
>> + return __bpf_set_dentry_xattr(dentry, name__str, value_p, flags, true);
>> +}
>> +
>> +/**
>> + * bpf_set_dentry_xattr_locked - set a xattr of a dentry
>> + * @dentry: dentry to get xattr from
>> + * @name__str: name of the xattr
>> + * @value_p: xattr value
>> + * @flags: flags to pass into filesystem operations
>> + *
>> + * Set xattr *name__str* of *dentry* to the value in *value_ptr*.
>> + *
>> + * For security reasons, only *name__str* with prefix "security.bpf."
>> + * is allowed.
>> + *
>> + * The caller already locked dentry->d_inode.
>> + *
>> + * Return: 0 on success, a negative value on error.
>> + */
>> +__bpf_kfunc int bpf_set_dentry_xattr_locked(struct dentry *dentry, const char *name__str,
>> +     const struct bpf_dynptr *value_p, int flags)
>> +{
>> + return __bpf_set_dentry_xattr(dentry, name__str, value_p, flags, false);
> 
> That boolean argument is not needed if you pull
> 
> value_len = __bpf_dynptr_size(value_ptr);
> value = __bpf_dynptr_data(value_ptr, value_len);
> if (!value)
> return -EINVAL;
> 
> into the two functions and then put:
> 
> inode_lock()
> bpf_set_dentry_xattr_unlocked();
> inode_unlock()
> 
> for the locked variant. Similar comment applied to the remove functions.

Sounds good. Let me update them in the next version. 

Song


[...]