[PATCH bpf-next 0/2] Add bpf_getxattr

[PATCH bpf-next 0/2] Add bpf_getxattr

[KP Singh]

Foundation for building more complex security policies using the
BPF LSM as presented in LSF/MM/BPF:

 http://vger.kernel.org/bpfconf2022_material/lsfmmbpf2022-xattr.pdf

KP Singh (2):
  bpf: Implement bpf_getxattr helper
  bpf/selftests: Add a selftest for bpf_getxattr

 include/uapi/linux/bpf.h                      |  8 +++
 kernel/trace/bpf_trace.c                      | 26 +++++++++
 scripts/bpf_doc.py                            |  5 ++
 tools/include/uapi/linux/bpf.h                |  8 +++
 .../testing/selftests/bpf/prog_tests/xattr.c  | 58 +++++++++++++++++++
 tools/testing/selftests/bpf/progs/xattr.c     | 34 +++++++++++
 6 files changed, 139 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xattr.c
 create mode 100644 tools/testing/selftests/bpf/progs/xattr.c

-- 
2.36.0.550.gb090851708-goog

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

[KP Singh]

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 <kpsingh@kernel.org>
---
 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(+)

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
-- 
2.36.0.550.gb090851708-goog

[PATCH bpf-next 2/2] bpf/selftests: Add a selftest for bpf_getxattr

[KP Singh]

A simple test that adds an xattr on a copied /bin/ls and reads it back
when the copied ls is executed.

Signed-off-by: KP Singh <kpsingh@kernel.org>
---
 .../testing/selftests/bpf/prog_tests/xattr.c  | 58 +++++++++++++++++++
 tools/testing/selftests/bpf/progs/xattr.c     | 34 +++++++++++
 2 files changed, 92 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xattr.c
 create mode 100644 tools/testing/selftests/bpf/progs/xattr.c

diff --git a/tools/testing/selftests/bpf/prog_tests/xattr.c b/tools/testing/selftests/bpf/prog_tests/xattr.c
new file mode 100644
index 000000000000..442b6c1aed0e
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/xattr.c
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright 2022 Google LLC.
+ */
+
+#include <test_progs.h>
+#include <sys/xattr.h>
+#include "xattr.skel.h"
+
+#define XATTR_NAME "security.bpf"
+#define XATTR_VALUE "test_progs"
+
+static unsigned int duration;
+
+void test_xattr(void)
+{
+	struct xattr *skel = NULL;
+	char tmp_dir_path[] = "/tmp/xattrXXXXXX";
+	char tmp_exec_path[64];
+	char cmd[256];
+	int err;
+
+	if (CHECK(!mkdtemp(tmp_dir_path), "mkdtemp",
+		  "unable to create tmpdir: %d\n", errno))
+		goto close_prog;
+
+	snprintf(tmp_exec_path, sizeof(tmp_exec_path), "%s/copy_of_ls",
+		 tmp_dir_path);
+	snprintf(cmd, sizeof(cmd), "cp /bin/ls %s", tmp_exec_path);
+	if (CHECK_FAIL(system(cmd)))
+		goto close_prog_rmdir;
+
+	if (CHECK(setxattr(tmp_exec_path, XATTR_NAME, XATTR_VALUE,
+			   sizeof(XATTR_VALUE), 0),
+		  "setxattr", "unable to setxattr: %d", errno))
+		goto close_prog_rmdir;
+
+	skel = xattr__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel_load"))
+		goto close_prog_rmdir;
+
+	err = xattr__attach(skel);
+	if (!ASSERT_OK(err, "xattr__attach failed"))
+		goto close_prog_rmdir;
+
+	snprintf(cmd, sizeof(cmd), "%s -l", tmp_exec_path);
+	if (CHECK_FAIL(system(cmd)))
+		goto close_prog_rmdir;
+
+	ASSERT_EQ(skel->bss->result, 1, "xattr result");
+
+close_prog_rmdir:
+	snprintf(cmd, sizeof(cmd), "rm -rf %s", tmp_dir_path);
+	system(cmd);
+close_prog:
+	xattr__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/xattr.c b/tools/testing/selftests/bpf/progs/xattr.c
new file mode 100644
index 000000000000..3bf6966a7900
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/xattr.c
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright 2022 Google LLC.
+ */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+#define XATTR_NAME "security.bpf"
+#define XATTR_VALUE "test_progs"
+
+__u64 result = 0;
+
+SEC("lsm.s/bprm_committed_creds")
+void BPF_PROG(bprm_cc, struct linux_binprm *bprm)
+{
+	struct task_struct *current = bpf_get_current_task_btf();
+	char dir_xattr_value[64];
+	int xattr_sz;
+
+	xattr_sz = bpf_getxattr(bprm->file->f_path.mnt->mnt_userns,
+				bprm->file->f_path.dentry, XATTR_NAME,
+				dir_xattr_value, 64);
+
+	if (xattr_sz <= 0)
+		return;
+
+	if (!bpf_strncmp(dir_xattr_value, sizeof(XATTR_VALUE), XATTR_VALUE))
+		result = 1;
+}
-- 
2.36.0.550.gb090851708-goog

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

[Alexei Starovoitov]

On Thu, May 12, 2022 at 9:50 AM KP Singh <kpsingh@kernel.org> 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.

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

[KP Singh]

On Thu, May 12, 2022 at 10:30 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, May 12, 2022 at 9:50 AM KP Singh <kpsingh@kernel.org> 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.