diff mbox series

[v5,bpf-next,4/5] bpf: Add a bpf_getxattr kfunc

Message ID 20220628161948.475097-5-kpsingh@kernel.org (mailing list archive)
State New
Headers show
Series Add bpf_getxattr | expand

Commit Message

KP Singh June 28, 2022, 4:19 p.m. UTC
LSMs like SELinux store security state in xattrs. bpf_getxattr enables
BPF LSM to implement similar functionality. In combination with
bpf_local_storage, xattrs can be used to develop more complex security
policies.

This kfunc wraps around __vfs_getxattr which can sleep and is,
therefore, limited to sleepable programs using the newly added
sleepable_set for kfuncs.

Signed-off-by: KP Singh <kpsingh@kernel.org>
---
 kernel/trace/bpf_trace.c | 42 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

Comments

Christian Brauner June 28, 2022, 5:22 p.m. UTC | #1
On Tue, Jun 28, 2022 at 04:19:47PM +0000, KP Singh wrote:
> LSMs like SELinux store security state in xattrs. bpf_getxattr enables
> BPF LSM to implement similar functionality. In combination with
> bpf_local_storage, xattrs can be used to develop more complex security
> policies.
> 
> This kfunc wraps around __vfs_getxattr which can sleep and is,
> therefore, limited to sleepable programs using the newly added
> sleepable_set for kfuncs.
> 
> Signed-off-by: KP Singh <kpsingh@kernel.org>
> ---
>  kernel/trace/bpf_trace.c | 42 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 4be976cf7d63..87496d57b099 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -20,6 +20,7 @@
>  #include <linux/fprobe.h>
>  #include <linux/bsearch.h>
>  #include <linux/sort.h>
> +#include <linux/xattr.h>
>  
>  #include <net/bpf_sk_storage.h>
>  
> @@ -1181,6 +1182,47 @@ static const struct bpf_func_proto bpf_get_func_arg_cnt_proto = {
>  	.arg1_type	= ARG_PTR_TO_CTX,
>  };
>  
> +__diag_push();
> +__diag_ignore_all("-Wmissing-prototypes",
> +		  "kfuncs that are used in tracing/LSM BPF programs");
> +
> +ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
> +		     const char *name, void *value, int value__sz)
> +{
> +	return __vfs_getxattr(dentry, inode, name, value, value__sz);

So this might all be due to my ignorance where and how this is supposed
to be used but using __vfs_getxattr() is performing _zero_ permission
checks. That means every eBPF program will be able to retrieve whatever
extended attribute it likes.

In addition to generic permission checking your code also assumes that
every caller is located in the initial user namespace. Is that a valid
assumption?

POSIX ACLs can store additional [u,g]ids on disk that need to be
translated according to the caller's user namespace.

Looking at your selftest example you have a current task and you also
have access to a struct file which makes me doubt that this assumption
is correct. But I'm happy to be convinced otherwise.

Also, if the current task is retrieving extended attributes from an
idmapped mount you also need to take the mount's idmapping into account.
Otherwise again, you'll retrieve misleading [g,u]id values...

Could you explain to me why that is safe and how this is going to be
used, please? As it stands I can't make heads nor tails of this.
Al Viro June 28, 2022, 5:23 p.m. UTC | #2
On Tue, Jun 28, 2022 at 04:19:47PM +0000, KP Singh wrote:
> LSMs like SELinux store security state in xattrs. bpf_getxattr enables
> BPF LSM to implement similar functionality. In combination with
> bpf_local_storage, xattrs can be used to develop more complex security
> policies.
> 
> This kfunc wraps around __vfs_getxattr which can sleep and is,
> therefore, limited to sleepable programs using the newly added
> sleepable_set for kfuncs.

"Sleepable" is nowhere near enough - for a trivial example, consider
what e.g. ext2_xattr_get() does.
        down_read(&EXT2_I(inode)->xattr_sem);
in there means that having that thing executed in anything that happens
to hold ->xattr_sem is a deadlock fodder.

"Can't use that in BPF program executed in non-blocking context" is
*not* sufficient to make it safe.
KP Singh June 28, 2022, 5:29 p.m. UTC | #3
On Tue, Jun 28, 2022 at 7:23 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Tue, Jun 28, 2022 at 04:19:47PM +0000, KP Singh wrote:
> > LSMs like SELinux store security state in xattrs. bpf_getxattr enables
> > BPF LSM to implement similar functionality. In combination with
> > bpf_local_storage, xattrs can be used to develop more complex security
> > policies.
> >
> > This kfunc wraps around __vfs_getxattr which can sleep and is,
> > therefore, limited to sleepable programs using the newly added
> > sleepable_set for kfuncs.
>
> "Sleepable" is nowhere near enough - for a trivial example, consider
> what e.g. ext2_xattr_get() does.
>         down_read(&EXT2_I(inode)->xattr_sem);
> in there means that having that thing executed in anything that happens
> to hold ->xattr_sem is a deadlock fodder.
>

We could limit this to sleepable LSM hooks:

https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/bpf_lsm.c#n169

and when we have abilities to tag
kernel functions and pointers with the work Yonghong did
(e.g. https://reviews.llvm.org/D113496) we can expand the set.


> "Can't use that in BPF program executed in non-blocking context" is
> *not* sufficient to make it safe.
diff mbox series

Patch

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 4be976cf7d63..87496d57b099 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -20,6 +20,7 @@ 
 #include <linux/fprobe.h>
 #include <linux/bsearch.h>
 #include <linux/sort.h>
+#include <linux/xattr.h>
 
 #include <net/bpf_sk_storage.h>
 
@@ -1181,6 +1182,47 @@  static const struct bpf_func_proto bpf_get_func_arg_cnt_proto = {
 	.arg1_type	= ARG_PTR_TO_CTX,
 };
 
+__diag_push();
+__diag_ignore_all("-Wmissing-prototypes",
+		  "kfuncs that are used in tracing/LSM BPF programs");
+
+ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
+		     const char *name, void *value, int value__sz)
+{
+	return __vfs_getxattr(dentry, inode, name, value, value__sz);
+}
+
+__diag_pop();
+
+BTF_SET_START(bpf_trace_kfunc_ids)
+BTF_ID(func, bpf_getxattr)
+BTF_SET_END(bpf_trace_kfunc_ids)
+
+BTF_SET_START(bpf_trace_sleepable_kfunc_ids)
+BTF_ID(func, bpf_getxattr)
+BTF_SET_END(bpf_trace_sleepable_kfunc_ids)
+
+static const struct btf_kfunc_id_set bpf_trace_kfunc_set = {
+	.owner = THIS_MODULE,
+	.check_set = &bpf_trace_kfunc_ids,
+	.sleepable_set = &bpf_trace_sleepable_kfunc_ids,
+};
+
+static int __init bpf_trace_kfunc_init(void)
+{
+	int ret;
+
+	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING,
+					&bpf_trace_kfunc_set);
+	if (!ret)
+		return ret;
+
+	return register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM,
+					&bpf_trace_kfunc_set);
+
+}
+late_initcall(bpf_trace_kfunc_init);
+
 static const struct bpf_func_proto *
 bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {