Re: [PATCH v3 bpf-next 4/6] bpf: fs/xattr: Add BPF kfuncs to set and remove xattrs

[Song Liu <songliubraving@meta.com>]

Hi Jan,

Thanks for your review!

> On Dec 12, 2024, at 2:24 AM, Jan Kara <jack@suse.cz> wrote:
> 
> > 
> On Tue 10-12-24 14:06:25, Song Liu wrote:
>> Add the following kfuncs to set and remove xattrs from BPF programs:

[...]

>> + return -EPERM;
>> +
>> + return inode_permission(&nop_mnt_idmap, inode, MAY_WRITE);
>> +}
>> +
>> +static int __bpf_set_dentry_xattr(struct dentry *dentry, const char *name,
>> +  const struct bpf_dynptr *value_p, int flags, bool lock_inode)
>> +{
>> + struct bpf_dynptr_kern *value_ptr = (struct bpf_dynptr_kern *)value_p;
>> + struct inode *inode = d_inode(dentry);
>> + const void *value;
>> + u32 value_len;
>> + int ret;
>> +
>> + ret = bpf_xattr_write_permission(name, inode);
>> + if (ret)
>> + return ret;
> 
> The permission checking should already happen under inode lock. Otherwise
> you'll have TTCTTU races.

Great catch! I will fix this in the next version. 

> 
>> +
>> + value_len = __bpf_dynptr_size(value_ptr);
>> + value = __bpf_dynptr_data(value_ptr, value_len);
>> + if (!value)
>> + return -EINVAL;
>> +
>> + if (lock_inode)
>> + inode_lock(inode);
>> + ret = __vfs_setxattr(&nop_mnt_idmap, dentry, inode, name,
>> +     value, value_len, flags);
>> + if (!ret) {
>> + fsnotify_xattr(dentry);
> 
> Do we really want to generate fsnotify event for this? I expect
> security.bpf is an internal bookkeeping of a BPF security module and
> generating fsnotify event for it seems a bit like generating it for
> filesystem metadata modifications. On the other hand as I'm checking IMA
> generates fsnotify events when modifying its xattrs as well. So probably
> this fine. OK.

Both SELinux and smack generate fsnotify events when setting xattrs:
[selinux|smack]_inode_setsecctx() -> __vfs_setxattr_locked(). So I
add the same logic here. 

> 
> ...
> 
>> +static int __bpf_remove_dentry_xattr(struct dentry *dentry, const char *name__str,
>> +     bool lock_inode)
>> +{
>> + struct inode *inode = d_inode(dentry);
>> + int ret;
>> +
>> + ret = bpf_xattr_write_permission(name__str, inode);
>> + if (ret)
>> + return ret;
>> +
>> + if (lock_inode)
> + inode_lock(inode);
> 
> The same comment WRT inode lock as above.
> 
>> + ret = __vfs_removexattr(&nop_mnt_idmap, dentry, name__str);
>> + if (!ret) {
>> + fsnotify_xattr(dentry);
>> +

Thanks again, 
Song