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

[Jan Kara]

On Tue 10-12-24 14:06:25, 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.
> 
> Signed-off-by: Song Liu <song@xxxxxxxxxx>

A few comments below.

> @@ -161,6 +162,160 @@ __bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str,
>  	return bpf_get_dentry_xattr(dentry, name__str, value_p);
>  }
>  
> +static int bpf_xattr_write_permission(const char *name, struct inode *inode)
> +{
> +	if (WARN_ON(!inode))
> +		return -EINVAL;
> +
> +	/* Only allow setting and removing security.bpf. xattrs */
> +	if (!match_security_bpf_prefix(name))
> +		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.

> +
> +	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.

...

> +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);
> +
> +		/* This xattr is removed by BPF LSM, so we do not call
> +		 * security_inode_post_removexattr.
> +		 */
> +	}
> +	if (lock_inode)
> +		inode_unlock(inode);
> +	return ret;
> +}

									Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR