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

Commit Message

Song Liu Dec. 10, 2024, 10:06 p.m. UTC

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 <[e-mail]>
---
 fs/bpf_fs_kfuncs.c | 239 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 238 insertions(+), 1 deletion(-)

Comments

Jan Kara Dec. 12, 2024, 10:24 a.m. UTC | #1

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 <[e-mail]>

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

Song Liu Dec. 12, 2024, 6:01 p.m. UTC | #2

Hi Jan,

Thanks for your review!

> On Dec 12, 2024, at 2:24 AM, Jan Kara <[e-mail]> 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

Patch

diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
index 8a65184c8c2c..2b6e5be57ff3 100644
--- a/fs/bpf_fs_kfuncs.c
+++ b/fs/bpf_fs_kfuncs.c
@@ -6,6 +6,7 @@ 
 #include <linux/btf_ids.h>
 #include <linux/dcache.h>
 #include <linux/fs.h>
+#include <linux/fsnotify.h>
 #include <linux/file.h>
 #include <linux/mm.h>
 #include <linux/xattr.h>
@@ -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;
+
+	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);
+
+		/* 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 (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);
+}
+
+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);
+	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;
+}
+
+/**
+ * bpf_remove_dentry_xattr - remove a xattr of a dentry
+ * @dentry: dentry to get xattr from
+ * @name__str: name of the xattr
+ *
+ * Rmove xattr *name__str* of *dentry*.
+ *
+ * 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_remove_dentry_xattr(struct dentry *dentry, const char *name__str)
+{
+	return __bpf_remove_dentry_xattr(dentry, name__str, true);
+}
+
+/**
+ * bpf_remove_dentry_xattr_locked - remove a xattr of a dentry
+ * @dentry: dentry to get xattr from
+ * @name__str: name of the xattr
+ *
+ * Rmove xattr *name__str* of *dentry*.
+ *
+ * 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_remove_dentry_xattr_locked(struct dentry *dentry, const char *name__str)
+{
+	return __bpf_remove_dentry_xattr(dentry, name__str, false);
+}
+
 __bpf_kfunc_end_defs();
 
 BTF_KFUNCS_START(bpf_fs_kfunc_set_ids)
@@ -186,9 +341,91 @@  static const struct btf_kfunc_id_set bpf_fs_kfunc_set = {
 	.filter = bpf_fs_kfuncs_filter,
 };
 
+/* bpf_[set|remove]_dentry_xattr.* hooks have KF_TRUSTED_ARGS and
+ * KF_SLEEPABLE, so they are only available to sleepable hooks with
+ * dentry arguments.
+ *
+ * Setting and removing xattr requires exclusive lock on dentry->d_inode.
+ * Some hooks already locked d_inode, while some hooks have not locked
+ * d_inode. Therefore, we need different kfuncs for different hooks.
+ * Specifically, hooks in the following list (d_inode_locked_hooks)
+ * should call bpf_[set|remove]_dentry_xattr_locked; while other hooks
+ * should call bpf_[set|remove]_dentry_xattr.
+ */
+BTF_SET_START(d_inode_locked_hooks)
+BTF_ID(func, bpf_lsm_inode_post_removexattr)
+BTF_ID(func, bpf_lsm_inode_post_setattr)
+BTF_ID(func, bpf_lsm_inode_post_setxattr)
+BTF_ID(func, bpf_lsm_inode_removexattr)
+BTF_ID(func, bpf_lsm_inode_rmdir)
+BTF_ID(func, bpf_lsm_inode_setattr)
+BTF_ID(func, bpf_lsm_inode_setxattr)
+BTF_ID(func, bpf_lsm_inode_unlink)
+#ifdef CONFIG_SECURITY_PATH
+BTF_ID(func, bpf_lsm_path_unlink)
+BTF_ID(func, bpf_lsm_path_rmdir)
+#endif /* CONFIG_SECURITY_PATH */
+BTF_SET_END(d_inode_locked_hooks)
+
+static bool bpf_lsm_has_d_inode_locked(const struct bpf_prog *prog)
+{
+	return btf_id_set_contains(&d_inode_locked_hooks, prog->aux->attach_btf_id);
+}
+
+BTF_KFUNCS_START(bpf_write_xattr_set_ids)
+BTF_ID_FLAGS(func, bpf_set_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_remove_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
+BTF_KFUNCS_END(bpf_write_xattr_set_ids)
+
+static int bpf_write_xattr_filter(const struct bpf_prog *prog, u32 kfunc_id)
+{
+	if (!btf_id_set8_contains(&bpf_write_xattr_set_ids, kfunc_id))
+		return 0;
+	if (prog->type != BPF_PROG_TYPE_LSM)
+		return -EACCES;
+
+	if (bpf_lsm_has_d_inode_locked(prog))
+		return -EINVAL;
+	return 0;
+}
+
+static const struct btf_kfunc_id_set bpf_write_xattr_set = {
+	.owner = THIS_MODULE,
+	.set = &bpf_write_xattr_set_ids,
+	.filter = bpf_write_xattr_filter,
+};
+
+BTF_KFUNCS_START(bpf_write_xattr_locked_set_ids)
+BTF_ID_FLAGS(func, bpf_set_dentry_xattr_locked, KF_SLEEPABLE | KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_remove_dentry_xattr_locked, KF_SLEEPABLE | KF_TRUSTED_ARGS)
+BTF_KFUNCS_END(bpf_write_xattr_locked_set_ids)
+
+static int bpf_write_xattr_locked_filter(const struct bpf_prog *prog, u32 kfunc_id)
+{
+	if (!btf_id_set8_contains(&bpf_write_xattr_locked_set_ids, kfunc_id))
+		return 0;
+	if (prog->type != BPF_PROG_TYPE_LSM)
+		return -EACCES;
+
+	if (!bpf_lsm_has_d_inode_locked(prog))
+		return -EINVAL;
+	return 0;
+}
+
+static const struct btf_kfunc_id_set bpf_write_xattr_locked_set = {
+	.owner = THIS_MODULE,
+	.set = &bpf_write_xattr_locked_set_ids,
+	.filter = bpf_write_xattr_locked_filter,
+};
+
 static int __init bpf_fs_kfuncs_init(void)
 {
-	return register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_fs_kfunc_set);
+	int ret;
+
+	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_fs_kfunc_set);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_write_xattr_set);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_write_xattr_locked_set);
+	return ret;
 }
 
 late_initcall(bpf_fs_kfuncs_init);