[bpf-next,1/2] bpf: Implement bpf_getxattr helper

Commit Message

KP Singh May 12, 2022, 4:50 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 helper wraps around vfs_getxattr which can sleep and is, therefore,
limited to sleepable programs.

Signed-off-by: KP Singh <[e-mail]>
---
 include/uapi/linux/bpf.h       |  8 ++++++++
 kernel/trace/bpf_trace.c       | 26 ++++++++++++++++++++++++++
 scripts/bpf_doc.py             |  5 +++++
 tools/include/uapi/linux/bpf.h |  8 ++++++++
 4 files changed, 47 insertions(+)

Comments

Alexei Starovoitov May 12, 2022, 5:29 p.m. UTC | #1

On Thu, May 12, 2022 at 9:50 AM KP Singh <[e-mail]> wrote:
>
> +BPF_CALL_5(bpf_getxattr, struct user_namespace *, mnt_userns, struct dentry *,
> +          dentry, void *, name, void *, value, size_t, value_size)
> +{
> +       return vfs_getxattr(mnt_userns, dentry, name, value, value_size);
> +}

It will deadlock in tracing, since it grabs all kinds of locks
and calls lsm hooks (potentially calling other bpf progs).
It probably should be sleepable only.
Also there is no need to make it uapi.
kfunc is a better interface here.
__vfs_getxattr() is probably better too,
since vfs_getxattr() calls xattr_permission which calls
a bunch of capable*() which will return "random values"
depending on the current task, since it's called from bpf prog.

KP Singh May 13, 2022, 12:20 a.m. UTC | #2

On Thu, May 12, 2022 at 10:30 AM Alexei Starovoitov
<[e-mail]> wrote:
>
> On Thu, May 12, 2022 at 9:50 AM KP Singh <[e-mail]> wrote:
> >
> > +BPF_CALL_5(bpf_getxattr, struct user_namespace *, mnt_userns, struct dentry *,
> > +          dentry, void *, name, void *, value, size_t, value_size)
> > +{
> > +       return vfs_getxattr(mnt_userns, dentry, name, value, value_size);
> > +}
>
> It will deadlock in tracing, since it grabs all kinds of locks
> and calls lsm hooks (potentially calling other bpf progs).

I wonder if we can limit these to just sleepable LSM programs
and for sleepable hooks + programs.

> It probably should be sleepable only.

Yes, it's currently sleepable only.

> Also there is no need to make it uapi.
> kfunc is a better interface here.

Sure, let me try with kfunc, simple wrappers like
these are a good use-case for kfuncs.

> __vfs_getxattr() is probably better too,
> since vfs_getxattr() calls xattr_permission which calls
> a bunch of capable*() which will return "random values"

Agreed.



> depending on the current task, since it's called from bpf prog.

Patch

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 0210f85131b3..ebd361c2e351 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -5172,6 +5172,13 @@  union bpf_attr {
  * 	Return
  * 		Map value associated to *key* on *cpu*, or **NULL** if no entry
  * 		was found or *cpu* is invalid.
+ *
+ * long bpf_getxattr(struct user_namespace *mnt_userns, struct dentry *dentry, const char *name, void *value, u64 value_size)
+ *	Description
+ *		Get the *value* of the xattr with the given *name*
+ *		where *value_size* is the size of the *value* buffer.
+ *	Return
+ *		The number of bytes copied into *value*.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5370,6 +5377,7 @@  union bpf_attr {
 	FN(ima_file_hash),		\
 	FN(kptr_xchg),			\
 	FN(map_lookup_percpu_elem),     \
+	FN(getxattr),			\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 7141ca8a1c2d..185adbb9b1b6 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,29 @@  static const struct bpf_func_proto bpf_get_func_arg_cnt_proto = {
 	.arg1_type	= ARG_PTR_TO_CTX,
 };
 
+BPF_CALL_5(bpf_getxattr, struct user_namespace *, mnt_userns, struct dentry *,
+	   dentry, void *, name, void *, value, size_t, value_size)
+{
+	return vfs_getxattr(mnt_userns, dentry, name, value, value_size);
+}
+
+BTF_ID_LIST(bpf_getxattr_btf_ids)
+BTF_ID(struct, user_namespace)
+BTF_ID(struct, dentry)
+
+static const struct bpf_func_proto bpf_getxattr_proto = {
+	.func = bpf_getxattr,
+	.gpl_only = false,
+	.ret_type = RET_INTEGER,
+	.arg1_type = ARG_PTR_TO_BTF_ID,
+	.arg1_btf_id = &bpf_getxattr_btf_ids[0],
+	.arg2_type = ARG_PTR_TO_BTF_ID,
+	.arg2_btf_id = &bpf_getxattr_btf_ids[1],
+	.arg3_type = ARG_PTR_TO_CONST_STR,
+	.arg4_type = ARG_PTR_TO_UNINIT_MEM,
+	.arg5_type = ARG_CONST_SIZE,
+};
+
 static const struct bpf_func_proto *
 bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
@@ -1304,6 +1328,8 @@  bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_find_vma_proto;
 	case BPF_FUNC_trace_vprintk:
 		return bpf_get_trace_vprintk_proto();
+	case BPF_FUNC_getxattr:
+		return prog->aux->sleepable ? &bpf_getxattr_proto : NULL;
 	default:
 		return bpf_base_func_proto(func_id);
 	}
diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
index 096625242475..be601c75c96c 100755
--- a/scripts/bpf_doc.py
+++ b/scripts/bpf_doc.py
@@ -633,6 +633,8 @@  class PrinterHelpers(Printer):
             'struct socket',
             'struct file',
             'struct bpf_timer',
+            'struct user_namespace',
+            'struct dentry',
     ]
     known_types = {
             '...',
@@ -682,6 +684,9 @@  class PrinterHelpers(Printer):
             'struct socket',
             'struct file',
             'struct bpf_timer',
+            'struct user_namespace',
+            'struct dentry',
+
     }
     mapped_types = {
             'u8': '__u8',
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 0210f85131b3..ebd361c2e351 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -5172,6 +5172,13 @@  union bpf_attr {
  * 	Return
  * 		Map value associated to *key* on *cpu*, or **NULL** if no entry
  * 		was found or *cpu* is invalid.
+ *
+ * long bpf_getxattr(struct user_namespace *mnt_userns, struct dentry *dentry, const char *name, void *value, u64 value_size)
+ *	Description
+ *		Get the *value* of the xattr with the given *name*
+ *		where *value_size* is the size of the *value* buffer.
+ *	Return
+ *		The number of bytes copied into *value*.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5370,6 +5377,7 @@  union bpf_attr {
 	FN(ima_file_hash),		\
 	FN(kptr_xchg),			\
 	FN(map_lookup_percpu_elem),     \
+	FN(getxattr),			\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper