[v12,bpf-next,4/9] bpf: Add kfunc bpf_get_file_xattr

Commit Message

Song Liu Nov. 4, 2023, 12:13 a.m. UTC

It is common practice for security solutions to store tags/labels in
xattrs. To implement similar functionalities in BPF LSM, add new kfunc
bpf_get_file_xattr().

The first use case of bpf_get_file_xattr() is to implement file
verifications with asymmetric keys. Specificially, security applications
could use fsverity for file hashes and use xattr to store file signatures.
(kfunc for fsverity hash will be added in a separate commit.)

Currently, only xattrs with "user." prefix can be read with kfunc
bpf_get_file_xattr(). As use cases evolve, we may add a dedicated prefix
for bpf_get_file_xattr().

To avoid recursion, bpf_get_file_xattr can be only called from LSM hooks.

Signed-off-by: Song Liu <[e-mail]>
---
 kernel/trace/bpf_trace.c | 67 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

Comments

Alexei Starovoitov Nov. 4, 2023, 9:11 a.m. UTC | #1

On Fri, Nov 3, 2023 at 5:13 PM Song Liu <[e-mail]> wrote:
>
> It is common practice for security solutions to store tags/labels in
> xattrs. To implement similar functionalities in BPF LSM, add new kfunc
> bpf_get_file_xattr().
>
> The first use case of bpf_get_file_xattr() is to implement file
> verifications with asymmetric keys. Specificially, security applications
> could use fsverity for file hashes and use xattr to store file signatures.
> (kfunc for fsverity hash will be added in a separate commit.)
>
> Currently, only xattrs with "user." prefix can be read with kfunc
> bpf_get_file_xattr(). As use cases evolve, we may add a dedicated prefix
> for bpf_get_file_xattr().
>
> To avoid recursion, bpf_get_file_xattr can be only called from LSM hooks.

Song,

have you studied the prior attempt to add this kfunc?
https://lore.kernel.org/bpf/[e-mail]/

file instead of (dentry,inode) pair makes sense,
and restricting to "user." prefix makes sense to me as well,
but you need to cc vfs experts in the future.

> + * For security reasons, only *name__str* with prefix "user." is allowed.
> + *
> + * Return: 0 on success, a negative value on error.
> + */
> +__bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str,
> +                                  struct bpf_dynptr_kern *value_ptr)
> +{
> +       struct dentry *dentry;
> +       u32 value_len;
> +       void *value;
> +
> +       if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN))
> +               return -EPERM;
> +
> +       value_len = __bpf_dynptr_size(value_ptr);
> +       value = __bpf_dynptr_data_rw(value_ptr, value_len);
> +       if (!value)
> +               return -EINVAL;
> +
> +       dentry = file_dentry(file);
> +       return __vfs_getxattr(dentry, dentry->d_inode, name__str, value, value_len);
> +}
> +
> +__diag_pop();
> +
> +BTF_SET8_START(fs_kfunc_set_ids)
> +BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)

See earlier Al's point. Just sleepable is not enough.

Song Liu Nov. 4, 2023, 2:20 p.m. UTC | #2

Hi Alexei,

On Sat, Nov 4, 2023 at 2:11 AM Alexei Starovoitov
<[e-mail]> wrote:
>
> On Fri, Nov 3, 2023 at 5:13 PM Song Liu <[e-mail]> wrote:
> >
> > It is common practice for security solutions to store tags/labels in
> > xattrs. To implement similar functionalities in BPF LSM, add new kfunc
> > bpf_get_file_xattr().
> >
> > The first use case of bpf_get_file_xattr() is to implement file
> > verifications with asymmetric keys. Specificially, security applications
> > could use fsverity for file hashes and use xattr to store file signatures.
> > (kfunc for fsverity hash will be added in a separate commit.)
> >
> > Currently, only xattrs with "user." prefix can be read with kfunc
> > bpf_get_file_xattr(). As use cases evolve, we may add a dedicated prefix
> > for bpf_get_file_xattr().
> >
> > To avoid recursion, bpf_get_file_xattr can be only called from LSM hooks.
>
> Song,
>
> have you studied the prior attempt to add this kfunc?
> https://lore.kernel.org/bpf/[e-mail]/

I studied this thread, and I think I addressed the concerns.

> file instead of (dentry,inode) pair makes sense,
> and restricting to "user." prefix makes sense to me as well,
> but you need to cc vfs experts in the future.

I will cc vfs experts and lists for future versions.

>
> > + * For security reasons, only *name__str* with prefix "user." is allowed.
> > + *
> > + * Return: 0 on success, a negative value on error.
> > + */
> > +__bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str,
> > +                                  struct bpf_dynptr_kern *value_ptr)
> > +{
> > +       struct dentry *dentry;
> > +       u32 value_len;
> > +       void *value;
> > +
> > +       if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN))
> > +               return -EPERM;
> > +
> > +       value_len = __bpf_dynptr_size(value_ptr);
> > +       value = __bpf_dynptr_data_rw(value_ptr, value_len);
> > +       if (!value)
> > +               return -EINVAL;
> > +
> > +       dentry = file_dentry(file);
> > +       return __vfs_getxattr(dentry, dentry->d_inode, name__str, value, value_len);
> > +}
> > +
> > +__diag_pop();
> > +
> > +BTF_SET8_START(fs_kfunc_set_ids)
> > +BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
>
> See earlier Al's point. Just sleepable is not enough.

In bpf_get_file_xattr_filter() we only allow calling
bpf_get_file_xattr from LSM hooks.
AFAICT, this is enough to avoid deadlock. We still need KF_SLEEPABLE here
because xattr_handler->get() may block (lock xattr_sem).

Did I miss something?

Thanks,
Song

Patch

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index d525a22b8d56..ec2db88416af 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -24,6 +24,7 @@ 
 #include <linux/key.h>
 #include <linux/verification.h>
 #include <linux/namei.h>
+#include <linux/fileattr.h>
 
 #include <net/bpf_sk_storage.h>
 
@@ -1433,6 +1434,72 @@  static int __init bpf_key_sig_kfuncs_init(void)
 late_initcall(bpf_key_sig_kfuncs_init);
 #endif /* CONFIG_KEYS */
 
+/* filesystem kfuncs */
+__diag_push();
+__diag_ignore_all("-Wmissing-prototypes",
+		  "kfuncs which will be used in BPF programs");
+__diag_ignore_all("-Wmissing-declarations",
+		  "Global kfuncs as their definitions will be in BTF");
+
+/**
+ * bpf_get_file_xattr - get xattr of a file
+ * @file: file to get xattr from
+ * @name__str: name of the xattr
+ * @value_ptr: output buffer of the xattr value
+ *
+ * Get xattr *name__str* of *file* and store the output in *value_ptr*.
+ *
+ * For security reasons, only *name__str* with prefix "user." is allowed.
+ *
+ * Return: 0 on success, a negative value on error.
+ */
+__bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str,
+				   struct bpf_dynptr_kern *value_ptr)
+{
+	struct dentry *dentry;
+	u32 value_len;
+	void *value;
+
+	if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN))
+		return -EPERM;
+
+	value_len = __bpf_dynptr_size(value_ptr);
+	value = __bpf_dynptr_data_rw(value_ptr, value_len);
+	if (!value)
+		return -EINVAL;
+
+	dentry = file_dentry(file);
+	return __vfs_getxattr(dentry, dentry->d_inode, name__str, value, value_len);
+}
+
+__diag_pop();
+
+BTF_SET8_START(fs_kfunc_set_ids)
+BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
+BTF_SET8_END(fs_kfunc_set_ids)
+
+static int bpf_get_file_xattr_filter(const struct bpf_prog *prog, u32 kfunc_id)
+{
+	if (!btf_id_set8_contains(&fs_kfunc_set_ids, kfunc_id))
+		return 0;
+
+	/* Only allow to attach from LSM hooks, to avoid recursion */
+	return prog->type != BPF_PROG_TYPE_LSM ? -EACCES : 0;
+}
+
+const struct btf_kfunc_id_set bpf_fs_kfunc_set = {
+	.owner = THIS_MODULE,
+	.set = &fs_kfunc_set_ids,
+	.filter = bpf_get_file_xattr_filter,
+};
+
+static int __init bpf_fs_kfuncs_init(void)
+{
+	return register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_fs_kfunc_set);
+}
+
+late_initcall(bpf_fs_kfuncs_init);
+
 static const struct bpf_func_proto *
 bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {