Hi Jan,
Thanks for your review!
> On Dec 12, 2024, at 2:24 AM, Jan Kara <jack@xxxxxxx> 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
