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

Commit Message

Song Liu Dec. 17, 2024, 6:38 a.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 | 243 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 242 insertions(+), 1 deletion(-)

Comments

Alexei Starovoitov Dec. 17, 2024, 4:50 p.m. UTC | #1

On Mon, Dec 16, 2024 at 10:38 PM Song Liu <[e-mail]> 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.

...

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

the inode locking rules might change, so let's hide this
implementation detail from the bpf progs by making kfunc polymorphic.

To struct bpf_prog_aux add:
bool use_locked_kfunc:1;
and set it in bpf_check_attach_target() if it's attaching
to one of d_inode_locked_hooks

Then in fixup_kfunc_call() call some helper that
if (prog->aux->use_locked_kfunc &&
    insn->imm == special_kfunc_list[KF_bpf_remove_dentry_xattr])
     insn->imm = special_kfunc_list[KF_bpf_remove_dentry_xattr_locked];

The progs will be simpler and will suffer less churn
when the kernel side changes.

Song Liu Dec. 17, 2024, 6:24 p.m. UTC | #2

Hi Alexei, 

Thanks for the review!

> On Dec 17, 2024, at 8:50 AM, Alexei Starovoitov <[e-mail]> wrote:
> 
> On Mon, Dec 16, 2024 at 10:38 PM Song Liu <[e-mail]> 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.
> 
> ...
> 
>> + *
>> + * 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.
>> + */
> 
> the inode locking rules might change, so let's hide this
> implementation detail from the bpf progs by making kfunc polymorphic.
> 
> To struct bpf_prog_aux add:
> bool use_locked_kfunc:1;
> and set it in bpf_check_attach_target() if it's attaching
> to one of d_inode_locked_hooks
> 
> Then in fixup_kfunc_call() call some helper that
> if (prog->aux->use_locked_kfunc &&
>    insn->imm == special_kfunc_list[KF_bpf_remove_dentry_xattr])
>     insn->imm = special_kfunc_list[KF_bpf_remove_dentry_xattr_locked];
> 
> The progs will be simpler and will suffer less churn
> when the kernel side changes.

I was thinking about something in similar direction. 

If we do this, shall we somehow hide the _locked version of the 
kfuncs, so that the user cannot use it? If so, what's the best 
way to do it? 

Thanks,
Song

Kumar Kartikeya Dwivedi Dec. 17, 2024, 6:32 p.m. UTC | #3

On Tue, 17 Dec 2024 at 19:25, Song Liu <[e-mail]> wrote:
>
> Hi Alexei,
>
> Thanks for the review!
>
> > On Dec 17, 2024, at 8:50 AM, Alexei Starovoitov <[e-mail]> wrote:
> >
> > On Mon, Dec 16, 2024 at 10:38 PM Song Liu <[e-mail]> 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.
> >
> > ...
> >
> >> + *
> >> + * 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.
> >> + */
> >
> > the inode locking rules might change, so let's hide this
> > implementation detail from the bpf progs by making kfunc polymorphic.
> >
> > To struct bpf_prog_aux add:
> > bool use_locked_kfunc:1;
> > and set it in bpf_check_attach_target() if it's attaching
> > to one of d_inode_locked_hooks
> >
> > Then in fixup_kfunc_call() call some helper that
> > if (prog->aux->use_locked_kfunc &&
> >    insn->imm == special_kfunc_list[KF_bpf_remove_dentry_xattr])
> >     insn->imm = special_kfunc_list[KF_bpf_remove_dentry_xattr_locked];
> >
> > The progs will be simpler and will suffer less churn
> > when the kernel side changes.
>
> I was thinking about something in similar direction.
>
> If we do this, shall we somehow hide the _locked version of the
> kfuncs, so that the user cannot use it? If so, what's the best
> way to do it?

Just don't add BTF_ID_FLAGS entries for them.
You'd also need to make an extra call to add_kfunc_call to add its
details before you can do the fixup.
That allows find_kfunc_desc to work.
I did something similar in earlier versions of resilient locks.
In add_kfunc_call's end (instead of directly returning):
func_id = get_shadow_kfunc_id(func_id, offset);
if (!func_id)
  return err;
return add_kfunc_call(env, func_id, offset);

Then check in fixup_kfunc_call to find shadow kfunc id and substitute imm.
Can use some other naming instead of "shadow".
Probably need to take a prog pointer to make a decision to find the
underlying kfunc id in your case.

>
> Thanks,
> Song
>

Song Liu Dec. 18, 2024, 4:38 a.m. UTC | #4

> On Dec 17, 2024, at 10:32 AM, Kumar Kartikeya Dwivedi <[e-mail]> wrote:
> 
> On Tue, 17 Dec 2024 at 19:25, Song Liu <[e-mail]> wrote:
>> 
>> Hi Alexei,
>> 
>> Thanks for the review!
>> 
>>> On Dec 17, 2024, at 8:50 AM, Alexei Starovoitov <[e-mail]> wrote:
>>> 
>>> On Mon, Dec 16, 2024 at 10:38 PM Song Liu <[e-mail]> 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.
>>> 
>>> ...
>>> 
>>>> + *
>>>> + * 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.
>>>> + */
>>> 
>>> the inode locking rules might change, so let's hide this
>>> implementation detail from the bpf progs by making kfunc polymorphic.
>>> 
>>> To struct bpf_prog_aux add:
>>> bool use_locked_kfunc:1;
>>> and set it in bpf_check_attach_target() if it's attaching
>>> to one of d_inode_locked_hooks
>>> 
>>> Then in fixup_kfunc_call() call some helper that
>>> if (prog->aux->use_locked_kfunc &&
>>>   insn->imm == special_kfunc_list[KF_bpf_remove_dentry_xattr])
>>>    insn->imm = special_kfunc_list[KF_bpf_remove_dentry_xattr_locked];
>>> 
>>> The progs will be simpler and will suffer less churn
>>> when the kernel side changes.
>> 
>> I was thinking about something in similar direction.
>> 
>> If we do this, shall we somehow hide the _locked version of the
>> kfuncs, so that the user cannot use it? If so, what's the best
>> way to do it?
> 
> Just don't add BTF_ID_FLAGS entries for them.
> You'd also need to make an extra call to add_kfunc_call to add its
> details before you can do the fixup.
> That allows find_kfunc_desc to work.
> I did something similar in earlier versions of resilient locks.
> In add_kfunc_call's end (instead of directly returning):
> func_id = get_shadow_kfunc_id(func_id, offset);
> if (!func_id)
>  return err;
> return add_kfunc_call(env, func_id, offset);
> 
> Then check in fixup_kfunc_call to find shadow kfunc id and substitute imm.
> Can use some other naming instead of "shadow".
> Probably need to take a prog pointer to make a decision to find the
> underlying kfunc id in your case.

Thanks for the hints! They helped a lot. 

I ended up doing this with a slightly different logic, which I 
think is cleaner. I will send v5 shortly. 

Song

Patch

diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
index 8a65184c8c2c..3261324e956c 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,164 @@  __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;
+
+	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 = bpf_xattr_write_permission(name, inode);
+	if (ret)
+		goto out;
+
+	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().
+		 */
+	}
+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);
+}
+
+static int __bpf_remove_dentry_xattr(struct dentry *dentry, const char *name__str,
+				     bool lock_inode)
+{
+	struct inode *inode = d_inode(dentry);
+	int ret;
+
+	if (lock_inode)
+		inode_lock(inode);
+
+	ret = bpf_xattr_write_permission(name__str, inode);
+	if (ret)
+		goto out;
+
+	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.
+		 */
+	}
+out:
+	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 +345,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);