[PATCH v13 bpf-next 0/6] bpf: File verification with LSM and fsverity

[PATCH v13 bpf-next 0/6] bpf: File verification with LSM and fsverity

[Song Liu]

Changes v12 => v13:
1. Only keep 4/9 through 9/9 of v12, as the first 3 patches already
   applied;
2. Use new macro __bpf_kfunc_[start|end]_defs().

Changes v11 => v12:
1. Fix typo (data_ptr => sig_ptr) in bpf_get_file_xattr().

Changes v10 => v11:
1. Let __bpf_dynptr_data() return const void *. (Andrii)
2. Optimize code to reuse output from __bpf_dynptr_size(). (Andrii)
3. Add __diag_ignore_all("-Wmissing-declarations") for kfunc definition.
4. Fix an off indentation. (Andrii)

Changes v9 => v10:
1. Remove WARN_ON_ONCE() from check_reg_const_str. (Alexei)

Changes v8 => v9:
1. Fix test_progs kfunc_dynptr_param/dynptr_data_null.

Changes v7 => v8:
1. Do not use bpf_dynptr_slice* in the kernel. Add __bpf_dynptr_data* and
   use them in ther kernel. (Andrii)

Changes v6 => v7:
1. Change "__const_str" annotation to "__str". (Alexei, Andrii)
2. Add KF_TRUSTED_ARGS flag for both new kfuncs. (KP)
3. Only allow bpf_get_file_xattr() to read xattr with "user." prefix.
4. Add Acked-by from Eric Biggers.

Changes v5 => v6:
1. Let fsverity_init_bpf() return void. (Eric Biggers)
2. Sort things in alphabetic orders. (Eric Biggers)

Changes v4 => v5:
1. Revise commit logs. (Alexei)

Changes v3 => v4:
1. Fix error reported by CI.
2. Update comments of bpf_dynptr_slice* that they may return error pointer.

Changes v2 => v3:
1. Rebase and resolve conflicts.

Changes v1 => v2:
1. Let bpf_get_file_xattr() use const string for arg "name". (Alexei)
2. Add recursion prevention with allowlist. (Alexei)
3. Let bpf_get_file_xattr() use __vfs_getxattr() to avoid recursion,
   as vfs_getxattr() calls into other LSM hooks.
4. Do not use dynptr->data directly, use helper insteadd. (Andrii)
5. Fixes with bpf_get_fsverity_digest. (Eric Biggers)
6. Add documentation. (Eric Biggers)
7. Fix some compile warnings. (kernel test robot)

This set enables file verification with BPF LSM and fsverity.

In this solution, fsverity is used to provide reliable and efficient hash
of files; and BPF LSM is used to implement signature verification (against
asymmetric keys), and to enforce access control.

This solution can be used to implement access control in complicated cases.
For example: only signed python binary and signed python script and access
special files/devices/ports.

Thanks,
Song

Song Liu (6):
  bpf: Add kfunc bpf_get_file_xattr
  bpf, fsverity: Add kfunc bpf_get_fsverity_digest
  Documentation/bpf: Add documentation for filesystem kfuncs
  selftests/bpf: Sort config in alphabetic order
  selftests/bpf: Add tests for filesystem kfuncs
  selftests/bpf: Add test that uses fsverity and xattr to sign a file

 Documentation/bpf/fs_kfuncs.rst               |  21 +++
 Documentation/bpf/index.rst                   |   1 +
 fs/verity/fsverity_private.h                  |  10 ++
 fs/verity/init.c                              |   1 +
 fs/verity/measure.c                           |  84 +++++++++
 kernel/trace/bpf_trace.c                      |  63 +++++++
 tools/testing/selftests/bpf/bpf_kfuncs.h      |  10 ++
 tools/testing/selftests/bpf/config            |   3 +-
 .../selftests/bpf/prog_tests/fs_kfuncs.c      | 132 ++++++++++++++
 .../bpf/prog_tests/verify_pkcs7_sig.c         | 163 +++++++++++++++++-
 .../selftests/bpf/progs/test_fsverity.c       |  46 +++++
 .../selftests/bpf/progs/test_get_xattr.c      |  37 ++++
 .../selftests/bpf/progs/test_sig_in_xattr.c   |  82 +++++++++
 .../bpf/progs/test_verify_pkcs7_sig.c         |   8 +-
 .../testing/selftests/bpf/verify_sig_setup.sh |  25 +++
 15 files changed, 677 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/bpf/fs_kfuncs.rst
 create mode 100644 tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_fsverity.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_get_xattr.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_sig_in_xattr.c

--
2.34.1

[PATCH v13 bpf-next 1/6] bpf: Add kfunc bpf_get_file_xattr

[Song Liu]

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 <song@kernel.org>
---
 kernel/trace/bpf_trace.c | 63 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index f0b8b7c29126..55758a6fbe90 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>
 
@@ -1431,6 +1432,68 @@ static int __init bpf_key_sig_kfuncs_init(void)
 late_initcall(bpf_key_sig_kfuncs_init);
 #endif /* CONFIG_KEYS */
 
+/* filesystem kfuncs */
+__bpf_kfunc_start_defs();
+
+/**
+ * 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);
+}
+
+__bpf_kfunc_end_defs();
+
+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)
 {
-- 
2.34.1

[PATCH v13 bpf-next 2/6] bpf, fsverity: Add kfunc bpf_get_fsverity_digest

[Song Liu]

fsverity provides fast and reliable hash of files, namely fsverity_digest.
The digest can be used by security solutions to verify file contents.

Add new kfunc bpf_get_fsverity_digest() so that we can access fsverity from
BPF LSM programs. This kfunc is added to fs/verity/measure.c because some
data structure used in the function is private to fsverity
(fs/verity/fsverity_private.h).

To avoid recursion, bpf_get_fsverity_digest is only allowed in BPF LSM
programs.

Signed-off-by: Song Liu <song@kernel.org>
Acked-by: Eric Biggers <ebiggers@google.com>
---
 fs/verity/fsverity_private.h | 10 +++++
 fs/verity/init.c             |  1 +
 fs/verity/measure.c          | 84 ++++++++++++++++++++++++++++++++++++
 3 files changed, 95 insertions(+)

diff --git a/fs/verity/fsverity_private.h b/fs/verity/fsverity_private.h
index d071a6e32581..a6a6b2749241 100644
--- a/fs/verity/fsverity_private.h
+++ b/fs/verity/fsverity_private.h
@@ -100,6 +100,16 @@ fsverity_msg(const struct inode *inode, const char *level,
 #define fsverity_err(inode, fmt, ...)		\
 	fsverity_msg((inode), KERN_ERR, fmt, ##__VA_ARGS__)
 
+/* measure.c */
+
+#ifdef CONFIG_BPF_SYSCALL
+void __init fsverity_init_bpf(void);
+#else
+static inline void fsverity_init_bpf(void)
+{
+}
+#endif
+
 /* open.c */
 
 int fsverity_init_merkle_tree_params(struct merkle_tree_params *params,
diff --git a/fs/verity/init.c b/fs/verity/init.c
index a29f062f6047..1e207c0f71de 100644
--- a/fs/verity/init.c
+++ b/fs/verity/init.c
@@ -69,6 +69,7 @@ static int __init fsverity_init(void)
 	fsverity_init_workqueue();
 	fsverity_init_sysctl();
 	fsverity_init_signature();
+	fsverity_init_bpf();
 	return 0;
 }
 late_initcall(fsverity_init)
diff --git a/fs/verity/measure.c b/fs/verity/measure.c
index eec5956141da..bf7a5f4cccaf 100644
--- a/fs/verity/measure.c
+++ b/fs/verity/measure.c
@@ -7,6 +7,8 @@
 
 #include "fsverity_private.h"
 
+#include <linux/bpf.h>
+#include <linux/btf.h>
 #include <linux/uaccess.h>
 
 /**
@@ -100,3 +102,85 @@ int fsverity_get_digest(struct inode *inode,
 	return hash_alg->digest_size;
 }
 EXPORT_SYMBOL_GPL(fsverity_get_digest);
+
+#ifdef CONFIG_BPF_SYSCALL
+
+/* bpf kfuncs */
+__bpf_kfunc_start_defs();
+
+/**
+ * bpf_get_fsverity_digest: read fsverity digest of file
+ * @file: file to get digest from
+ * @digest_ptr: (out) dynptr for struct fsverity_digest
+ *
+ * Read fsverity_digest of *file* into *digest_ptr*.
+ *
+ * Return: 0 on success, a negative value on error.
+ */
+__bpf_kfunc int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr_kern *digest_ptr)
+{
+	const struct inode *inode = file_inode(file);
+	u32 dynptr_sz = __bpf_dynptr_size(digest_ptr);
+	struct fsverity_digest *arg;
+	const struct fsverity_info *vi;
+	const struct fsverity_hash_alg *hash_alg;
+	int out_digest_sz;
+
+	if (dynptr_sz < sizeof(struct fsverity_digest))
+		return -EINVAL;
+
+	arg = __bpf_dynptr_data_rw(digest_ptr, dynptr_sz);
+	if (!arg)
+		return -EINVAL;
+
+	if (!IS_ALIGNED((uintptr_t)arg, __alignof__(*arg)))
+		return -EINVAL;
+
+	vi = fsverity_get_info(inode);
+	if (!vi)
+		return -ENODATA; /* not a verity file */
+
+	hash_alg = vi->tree_params.hash_alg;
+
+	arg->digest_algorithm = hash_alg - fsverity_hash_algs;
+	arg->digest_size = hash_alg->digest_size;
+
+	out_digest_sz = dynptr_sz - sizeof(struct fsverity_digest);
+
+	/* copy digest */
+	memcpy(arg->digest, vi->file_digest,  min_t(int, hash_alg->digest_size, out_digest_sz));
+
+	/* fill the extra buffer with zeros */
+	if (out_digest_sz > hash_alg->digest_size)
+		memset(arg->digest + arg->digest_size, 0, out_digest_sz - hash_alg->digest_size);
+
+	return 0;
+}
+
+__bpf_kfunc_end_defs();
+
+BTF_SET8_START(fsverity_set_ids)
+BTF_ID_FLAGS(func, bpf_get_fsverity_digest, KF_TRUSTED_ARGS)
+BTF_SET8_END(fsverity_set_ids)
+
+static int bpf_get_fsverity_digest_filter(const struct bpf_prog *prog, u32 kfunc_id)
+{
+	if (!btf_id_set8_contains(&fsverity_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;
+}
+
+static const struct btf_kfunc_id_set bpf_fsverity_set = {
+	.owner = THIS_MODULE,
+	.set = &fsverity_set_ids,
+	.filter = bpf_get_fsverity_digest_filter,
+};
+
+void __init fsverity_init_bpf(void)
+{
+	register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_fsverity_set);
+}
+
+#endif /* CONFIG_BPF_SYSCALL */
-- 
2.34.1

[PATCH v13 bpf-next 3/6] Documentation/bpf: Add documentation for filesystem kfuncs

[Song Liu]

Add a brief introduction for file system kfuncs:

  bpf_get_file_xattr()
  bpf_get_fsverity_digest()

The documentation highlights the strategy to avoid recursions of these
kfuncs.

Signed-off-by: Song Liu <song@kernel.org>
---
 Documentation/bpf/fs_kfuncs.rst | 21 +++++++++++++++++++++
 Documentation/bpf/index.rst     |  1 +
 2 files changed, 22 insertions(+)
 create mode 100644 Documentation/bpf/fs_kfuncs.rst

diff --git a/Documentation/bpf/fs_kfuncs.rst b/Documentation/bpf/fs_kfuncs.rst
new file mode 100644
index 000000000000..8762c3233a3d
--- /dev/null
+++ b/Documentation/bpf/fs_kfuncs.rst
@@ -0,0 +1,21 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+.. _fs_kfuncs-header-label:
+
+=====================
+BPF filesystem kfuncs
+=====================
+
+BPF LSM programs need to access filesystem data from LSM hooks. The following
+BPF kfuncs can be used to get these data.
+
+ * ``bpf_get_file_xattr()``
+
+ * ``bpf_get_fsverity_digest()``
+
+To avoid recursions, these kfuncs follow the following rules:
+
+1. These kfuncs are only permitted from BPF LSM function.
+2. These kfuncs should not call into other LSM hooks, i.e. security_*(). For
+   example, ``bpf_get_file_xattr()`` does not use ``vfs_getxattr()``, because
+   the latter calls LSM hook ``security_inode_getxattr``.
diff --git a/Documentation/bpf/index.rst b/Documentation/bpf/index.rst
index aeaeb35e6d4a..0bb5cb8157f1 100644
--- a/Documentation/bpf/index.rst
+++ b/Documentation/bpf/index.rst
@@ -21,6 +21,7 @@ that goes into great technical depth about the BPF Architecture.
    helpers
    kfuncs
    cpumasks
+   fs_kfuncs
    programs
    maps
    bpf_prog_run
-- 
2.34.1

[PATCH v13 bpf-next 4/6] selftests/bpf: Sort config in alphabetic order

[Song Liu]

Move CONFIG_VSOCKETS up, so the CONFIGs are in alphabetic order.

Signed-off-by: Song Liu <song@kernel.org>
---
 tools/testing/selftests/bpf/config | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
index 3ec5927ec3e5..782876452acf 100644
--- a/tools/testing/selftests/bpf/config
+++ b/tools/testing/selftests/bpf/config
@@ -82,7 +82,7 @@ CONFIG_SECURITY=y
 CONFIG_SECURITYFS=y
 CONFIG_TEST_BPF=m
 CONFIG_USERFAULTFD=y
+CONFIG_VSOCKETS=y
 CONFIG_VXLAN=y
 CONFIG_XDP_SOCKETS=y
 CONFIG_XFRM_INTERFACE=y
-CONFIG_VSOCKETS=y
-- 
2.34.1

[PATCH v13 bpf-next 5/6] selftests/bpf: Add tests for filesystem kfuncs

[Song Liu]

Add selftests for two new filesystem kfuncs:
  1. bpf_get_file_xattr
  2. bpf_get_fsverity_digest

These tests simply make sure the two kfuncs work. Another selftest will be
added to demonstrate how to use these kfuncs to verify file signature.

CONFIG_FS_VERITY is added to selftests config. However, this is not
sufficient to guarantee bpf_get_fsverity_digest works. This is because
fsverity need to be enabled at file system level (for example, with tune2fs
on ext4). If local file system doesn't have this feature enabled, just skip
the test.

Signed-off-by: Song Liu <song@kernel.org>
---
 tools/testing/selftests/bpf/bpf_kfuncs.h      |   3 +
 tools/testing/selftests/bpf/config            |   1 +
 .../selftests/bpf/prog_tests/fs_kfuncs.c      | 132 ++++++++++++++++++
 .../selftests/bpf/progs/test_fsverity.c       |  46 ++++++
 .../selftests/bpf/progs/test_get_xattr.c      |  37 +++++
 5 files changed, 219 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_fsverity.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_get_xattr.c

diff --git a/tools/testing/selftests/bpf/bpf_kfuncs.h b/tools/testing/selftests/bpf/bpf_kfuncs.h
index 5ca68ff0b59f..c2c084a44eae 100644
--- a/tools/testing/selftests/bpf/bpf_kfuncs.h
+++ b/tools/testing/selftests/bpf/bpf_kfuncs.h
@@ -55,4 +55,7 @@ void *bpf_cast_to_kern_ctx(void *) __ksym;
 
 void *bpf_rdonly_cast(void *obj, __u32 btf_id) __ksym;
 
+extern int bpf_get_file_xattr(struct file *file, const char *name,
+			      struct bpf_dynptr *value_ptr) __ksym;
+extern int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr *digest_ptr) __ksym;
 #endif
diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
index 782876452acf..c125c441abc7 100644
--- a/tools/testing/selftests/bpf/config
+++ b/tools/testing/selftests/bpf/config
@@ -23,6 +23,7 @@ CONFIG_FPROBE=y
 CONFIG_FTRACE_SYSCALLS=y
 CONFIG_FUNCTION_ERROR_INJECTION=y
 CONFIG_FUNCTION_TRACER=y
+CONFIG_FS_VERITY=y
 CONFIG_GENEVE=y
 CONFIG_IKCONFIG=y
 CONFIG_IKCONFIG_PROC=y
diff --git a/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c b/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
new file mode 100644
index 000000000000..3084872ad1f4
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+
+#include <stdlib.h>
+#include <sys/types.h>
+#include <sys/xattr.h>
+#include <linux/fsverity.h>
+#include <unistd.h>
+#include <test_progs.h>
+#include "test_get_xattr.skel.h"
+#include "test_fsverity.skel.h"
+
+static const char testfile[] = "/tmp/test_progs_fs_kfuncs";
+
+static void test_xattr(void)
+{
+	struct test_get_xattr *skel = NULL;
+	int fd = -1, err;
+
+	fd = open(testfile, O_CREAT | O_RDONLY, 0644);
+	if (!ASSERT_GE(fd, 0, "create_file"))
+		return;
+
+	close(fd);
+	fd = -1;
+
+	err = setxattr(testfile, "user.kfuncs", "hello", sizeof("hello"), 0);
+	if (!ASSERT_OK(err, "setxattr"))
+		goto out;
+
+	skel = test_get_xattr__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "test_get_xattr__open_and_load"))
+		goto out;
+
+	skel->bss->monitored_pid = getpid();
+	err = test_get_xattr__attach(skel);
+
+	if (!ASSERT_OK(err, "test_get_xattr__attach"))
+		goto out;
+
+	fd = open(testfile, O_RDONLY, 0644);
+	if (!ASSERT_GE(fd, 0, "open_file"))
+		goto out;
+
+	ASSERT_EQ(skel->bss->found_xattr, 1, "found_xattr");
+
+out:
+	close(fd);
+	test_get_xattr__destroy(skel);
+	remove(testfile);
+}
+
+#ifndef SHA256_DIGEST_SIZE
+#define SHA256_DIGEST_SIZE      32
+#endif
+
+static void test_fsverity(void)
+{
+	struct fsverity_enable_arg arg = {0};
+	struct test_fsverity *skel = NULL;
+	struct fsverity_digest *d;
+	int fd, err;
+	char buffer[4096];
+
+	fd = open(testfile, O_CREAT | O_RDWR, 0644);
+	if (!ASSERT_GE(fd, 0, "create_file"))
+		return;
+
+	/* Write random buffer, so the file is not empty */
+	err = write(fd, buffer, 4096);
+	if (!ASSERT_EQ(err, 4096, "write_file"))
+		goto out;
+	close(fd);
+
+	/* Reopen read-only, otherwise FS_IOC_ENABLE_VERITY will fail */
+	fd = open(testfile, O_RDONLY, 0644);
+	if (!ASSERT_GE(fd, 0, "open_file1"))
+		return;
+
+	/* Enable fsverity for the file.
+	 * If the file system doesn't support verity, this will fail. Skip
+	 * the test in such case.
+	 */
+	arg.version = 1;
+	arg.hash_algorithm = FS_VERITY_HASH_ALG_SHA256;
+	arg.block_size = 4096;
+	err = ioctl(fd, FS_IOC_ENABLE_VERITY, &arg);
+	if (err) {
+		printf("%s:SKIP:local fs doesn't support fsverity (%d)\n", __func__, errno);
+		test__skip();
+		goto out;
+	}
+
+	skel = test_fsverity__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "test_fsverity__open_and_load"))
+		goto out;
+
+	/* Get fsverity_digest from ioctl */
+	d = (struct fsverity_digest *)skel->bss->expected_digest;
+	d->digest_algorithm = FS_VERITY_HASH_ALG_SHA256;
+	d->digest_size = SHA256_DIGEST_SIZE;
+	err = ioctl(fd, FS_IOC_MEASURE_VERITY, skel->bss->expected_digest);
+	if (!ASSERT_OK(err, "ioctl_FS_IOC_MEASURE_VERITY"))
+		goto out;
+
+	skel->bss->monitored_pid = getpid();
+	err = test_fsverity__attach(skel);
+	if (!ASSERT_OK(err, "test_fsverity__attach"))
+		goto out;
+
+	/* Reopen the file to trigger the program */
+	close(fd);
+	fd = open(testfile, O_RDONLY);
+	if (!ASSERT_GE(fd, 0, "open_file2"))
+		goto out;
+
+	ASSERT_EQ(skel->bss->got_fsverity, 1, "got_fsverity");
+	ASSERT_EQ(skel->bss->digest_matches, 1, "digest_matches");
+out:
+	close(fd);
+	test_fsverity__destroy(skel);
+	remove(testfile);
+}
+
+void test_fs_kfuncs(void)
+{
+	if (test__start_subtest("xattr"))
+		test_xattr();
+
+	if (test__start_subtest("fsverity"))
+		test_fsverity();
+}
diff --git a/tools/testing/selftests/bpf/progs/test_fsverity.c b/tools/testing/selftests/bpf/progs/test_fsverity.c
new file mode 100644
index 000000000000..ddba2edc8e7a
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_fsverity.c
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_kfuncs.h"
+
+char _license[] SEC("license") = "GPL";
+
+#ifndef SHA256_DIGEST_SIZE
+#define SHA256_DIGEST_SIZE      32
+#endif
+
+__u32 monitored_pid;
+char expected_digest[sizeof(struct fsverity_digest) + SHA256_DIGEST_SIZE];
+char digest[sizeof(struct fsverity_digest) + SHA256_DIGEST_SIZE];
+__u32 got_fsverity;
+__u32 digest_matches;
+
+SEC("lsm.s/file_open")
+int BPF_PROG(test_file_open, struct file *f)
+{
+	struct bpf_dynptr digest_ptr;
+	__u32 pid;
+	int ret;
+	int i;
+
+	pid = bpf_get_current_pid_tgid() >> 32;
+	if (pid != monitored_pid)
+		return 0;
+
+	bpf_dynptr_from_mem(digest, sizeof(digest), 0, &digest_ptr);
+	ret = bpf_get_fsverity_digest(f, &digest_ptr);
+	if (ret < 0)
+		return 0;
+	got_fsverity = 1;
+
+	for (i = 0; i < sizeof(digest); i++) {
+		if (digest[i] != expected_digest[i])
+			return 0;
+	}
+
+	digest_matches = 1;
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/test_get_xattr.c b/tools/testing/selftests/bpf/progs/test_get_xattr.c
new file mode 100644
index 000000000000..7eb2a4e5a3e5
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_get_xattr.c
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_kfuncs.h"
+
+char _license[] SEC("license") = "GPL";
+
+__u32 monitored_pid;
+__u32 found_xattr;
+
+static const char expected_value[] = "hello";
+char value[32];
+
+SEC("lsm.s/file_open")
+int BPF_PROG(test_file_open, struct file *f)
+{
+	struct bpf_dynptr value_ptr;
+	__u32 pid;
+	int ret;
+
+	pid = bpf_get_current_pid_tgid() >> 32;
+	if (pid != monitored_pid)
+		return 0;
+
+	bpf_dynptr_from_mem(value, sizeof(value), 0, &value_ptr);
+
+	ret = bpf_get_file_xattr(f, "user.kfuncs", &value_ptr);
+	if (ret != sizeof(expected_value))
+		return 0;
+	if (bpf_strncmp(value, ret, expected_value))
+		return 0;
+	found_xattr = 1;
+	return 0;
+}
-- 
2.34.1

[PATCH v13 bpf-next 6/6] selftests/bpf: Add test that uses fsverity and xattr to sign a file

[Song Liu]

This selftests shows a proof of concept method to use BPF LSM to enforce
file signature. This test is added to verify_pkcs7_sig, so that some
existing logic can be reused.

This file signature method uses fsverity, which provides reliable and
efficient hash (known as digest) of the file. The file digest is signed
with asymmetic key, and the signature is stored in xattr. At the run time,
BPF LSM reads file digest and the signature, and then checks them against
the public key.

Note that this solution does NOT require FS_VERITY_BUILTIN_SIGNATURES.
fsverity is only used to provide file digest. The signature verification
and access control is all implemented in BPF LSM.

Signed-off-by: Song Liu <song@kernel.org>
---
 tools/testing/selftests/bpf/bpf_kfuncs.h      |   7 +
 .../bpf/prog_tests/verify_pkcs7_sig.c         | 163 +++++++++++++++++-
 .../selftests/bpf/progs/test_sig_in_xattr.c   |  82 +++++++++
 .../bpf/progs/test_verify_pkcs7_sig.c         |   8 +-
 .../testing/selftests/bpf/verify_sig_setup.sh |  25 +++
 5 files changed, 277 insertions(+), 8 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_sig_in_xattr.c

diff --git a/tools/testing/selftests/bpf/bpf_kfuncs.h b/tools/testing/selftests/bpf/bpf_kfuncs.h
index c2c084a44eae..b4e78c1eb37b 100644
--- a/tools/testing/selftests/bpf/bpf_kfuncs.h
+++ b/tools/testing/selftests/bpf/bpf_kfuncs.h
@@ -58,4 +58,11 @@ void *bpf_rdonly_cast(void *obj, __u32 btf_id) __ksym;
 extern int bpf_get_file_xattr(struct file *file, const char *name,
 			      struct bpf_dynptr *value_ptr) __ksym;
 extern int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr *digest_ptr) __ksym;
+
+extern struct bpf_key *bpf_lookup_user_key(__u32 serial, __u64 flags) __ksym;
+extern struct bpf_key *bpf_lookup_system_key(__u64 id) __ksym;
+extern void bpf_key_put(struct bpf_key *key) __ksym;
+extern int bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr,
+				      struct bpf_dynptr *sig_ptr,
+				      struct bpf_key *trusted_keyring) __ksym;
 #endif
diff --git a/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
index dd7f2bc70048..682b6af8d0a4 100644
--- a/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
+++ b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
@@ -16,9 +16,12 @@
 #include <sys/wait.h>
 #include <sys/mman.h>
 #include <linux/keyctl.h>
+#include <sys/xattr.h>
+#include <linux/fsverity.h>
 #include <test_progs.h>
 
 #include "test_verify_pkcs7_sig.skel.h"
+#include "test_sig_in_xattr.skel.h"
 
 #define MAX_DATA_SIZE (1024 * 1024)
 #define MAX_SIG_SIZE 1024
@@ -26,6 +29,10 @@
 #define VERIFY_USE_SECONDARY_KEYRING (1UL)
 #define VERIFY_USE_PLATFORM_KEYRING  (2UL)
 
+#ifndef SHA256_DIGEST_SIZE
+#define SHA256_DIGEST_SIZE      32
+#endif
+
 /* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
 #define MODULE_SIG_STRING "~Module signature appended~\n"
 
@@ -254,7 +261,7 @@ static int populate_data_item_mod(struct data *data_item)
 	return ret;
 }
 
-void test_verify_pkcs7_sig(void)
+static void test_verify_pkcs7_sig_from_map(void)
 {
 	libbpf_print_fn_t old_print_cb;
 	char tmp_dir_template[] = "/tmp/verify_sigXXXXXX";
@@ -400,3 +407,157 @@ void test_verify_pkcs7_sig(void)
 	skel->bss->monitored_pid = 0;
 	test_verify_pkcs7_sig__destroy(skel);
 }
+
+static int get_signature_size(const char *sig_path)
+{
+	struct stat st;
+
+	if (stat(sig_path, &st) == -1)
+		return -1;
+
+	return st.st_size;
+}
+
+static int add_signature_to_xattr(const char *data_path, const char *sig_path)
+{
+	char sig[MAX_SIG_SIZE] = {0};
+	int fd, size, ret;
+
+	if (sig_path) {
+		fd = open(sig_path, O_RDONLY);
+		if (fd < 0)
+			return -1;
+
+		size = read(fd, sig, MAX_SIG_SIZE);
+		close(fd);
+		if (size <= 0)
+			return -1;
+	} else {
+		/* no sig_path, just write 32 bytes of zeros */
+		size = 32;
+	}
+	ret = setxattr(data_path, "user.sig", sig, size, 0);
+	if (!ASSERT_OK(ret, "setxattr"))
+		return -1;
+
+	return 0;
+}
+
+static int test_open_file(struct test_sig_in_xattr *skel, char *data_path,
+			  pid_t pid, bool should_success, char *name)
+{
+	int ret;
+
+	skel->bss->monitored_pid = pid;
+	ret = open(data_path, O_RDONLY);
+	close(ret);
+	skel->bss->monitored_pid = 0;
+
+	if (should_success) {
+		if (!ASSERT_GE(ret, 0, name))
+			return -1;
+	} else {
+		if (!ASSERT_LT(ret, 0, name))
+			return -1;
+	}
+	return 0;
+}
+
+static void test_pkcs7_sig_fsverity(void)
+{
+	char data_path[PATH_MAX];
+	char sig_path[PATH_MAX];
+	char tmp_dir_template[] = "/tmp/verify_sigXXXXXX";
+	char *tmp_dir;
+	struct test_sig_in_xattr *skel = NULL;
+	pid_t pid;
+	int ret;
+
+	tmp_dir = mkdtemp(tmp_dir_template);
+	if (!ASSERT_OK_PTR(tmp_dir, "mkdtemp"))
+		return;
+
+	snprintf(data_path, PATH_MAX, "%s/data-file", tmp_dir);
+	snprintf(sig_path, PATH_MAX, "%s/sig-file", tmp_dir);
+
+	ret = _run_setup_process(tmp_dir, "setup");
+	if (!ASSERT_OK(ret, "_run_setup_process"))
+		goto out;
+
+	ret = _run_setup_process(tmp_dir, "fsverity-create-sign");
+
+	if (ret) {
+		printf("%s: SKIP: fsverity [sign|enable] doesn't work.\n", __func__);
+		test__skip();
+		goto out;
+	}
+
+	skel = test_sig_in_xattr__open();
+	if (!ASSERT_OK_PTR(skel, "test_sig_in_xattr__open"))
+		goto out;
+	ret = get_signature_size(sig_path);
+	if (!ASSERT_GT(ret, 0, "get_signaure_size"))
+		goto out;
+	skel->bss->sig_size = ret;
+	skel->bss->user_keyring_serial = syscall(__NR_request_key, "keyring",
+						 "ebpf_testing_keyring", NULL,
+						 KEY_SPEC_SESSION_KEYRING);
+	memcpy(skel->bss->digest, "FSVerity", 8);
+
+	ret = test_sig_in_xattr__load(skel);
+	if (!ASSERT_OK(ret, "test_sig_in_xattr__load"))
+		goto out;
+
+	ret = test_sig_in_xattr__attach(skel);
+	if (!ASSERT_OK(ret, "test_sig_in_xattr__attach"))
+		goto out;
+
+	pid = getpid();
+
+	/* Case 1: fsverity is not enabled, open should succeed */
+	if (test_open_file(skel, data_path, pid, true, "open_1"))
+		goto out;
+
+	/* Case 2: fsverity is enabled, xattr is missing, open should
+	 * fail
+	 */
+	ret = _run_setup_process(tmp_dir, "fsverity-enable");
+	if (!ASSERT_OK(ret, "fsverity-enable"))
+		goto out;
+	if (test_open_file(skel, data_path, pid, false, "open_2"))
+		goto out;
+
+	/* Case 3: fsverity is enabled, xattr has valid signature, open
+	 * should succeed
+	 */
+	ret = add_signature_to_xattr(data_path, sig_path);
+	if (!ASSERT_OK(ret, "add_signature_to_xattr_1"))
+		goto out;
+
+	if (test_open_file(skel, data_path, pid, true, "open_3"))
+		goto out;
+
+	/* Case 4: fsverity is enabled, xattr has invalid signature, open
+	 * should fail
+	 */
+	ret = add_signature_to_xattr(data_path, NULL);
+	if (!ASSERT_OK(ret, "add_signature_to_xattr_2"))
+		goto out;
+	test_open_file(skel, data_path, pid, false, "open_4");
+
+out:
+	_run_setup_process(tmp_dir, "cleanup");
+	if (!skel)
+		return;
+
+	skel->bss->monitored_pid = 0;
+	test_sig_in_xattr__destroy(skel);
+}
+
+void test_verify_pkcs7_sig(void)
+{
+	if (test__start_subtest("pkcs7_sig_from_map"))
+		test_verify_pkcs7_sig_from_map();
+	if (test__start_subtest("pkcs7_sig_fsverity"))
+		test_pkcs7_sig_fsverity();
+}
diff --git a/tools/testing/selftests/bpf/progs/test_sig_in_xattr.c b/tools/testing/selftests/bpf/progs/test_sig_in_xattr.c
new file mode 100644
index 000000000000..820b891171d8
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_sig_in_xattr.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+
+#include "vmlinux.h"
+#include <errno.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_kfuncs.h"
+
+char _license[] SEC("license") = "GPL";
+
+#ifndef SHA256_DIGEST_SIZE
+#define SHA256_DIGEST_SIZE      32
+#endif
+
+#define MAX_SIG_SIZE 1024
+
+/* By default, "fsverity sign" signs a file with fsverity_formatted_digest
+ * of the file. fsverity_formatted_digest on the kernel side is only used
+ * with CONFIG_FS_VERITY_BUILTIN_SIGNATURES. However, BPF LSM doesn't not
+ * require CONFIG_FS_VERITY_BUILTIN_SIGNATURES, so vmlinux.h may not have
+ * fsverity_formatted_digest. In this test, we intentionally avoid using
+ * fsverity_formatted_digest.
+ *
+ * Luckily, fsverity_formatted_digest is simply 8-byte magic followed by
+ * fsverity_digest. We use a char array of size fsverity_formatted_digest
+ * plus SHA256_DIGEST_SIZE. The magic part of it is filled by user space,
+ * and the rest of it is filled by bpf_get_fsverity_digest.
+ *
+ * Note that, generating signatures based on fsverity_formatted_digest is
+ * the design choice of this selftest (and "fsverity sign"). With BPF
+ * LSM, we have the flexibility to generate signature based on other data
+ * sets, for example, fsverity_digest or only the digest[] part of it.
+ */
+#define MAGIC_SIZE 8
+char digest[MAGIC_SIZE + sizeof(struct fsverity_digest) + SHA256_DIGEST_SIZE];
+
+__u32 monitored_pid;
+char sig[MAX_SIG_SIZE];
+__u32 sig_size;
+__u32 user_keyring_serial;
+
+SEC("lsm.s/file_open")
+int BPF_PROG(test_file_open, struct file *f)
+{
+	struct bpf_dynptr digest_ptr, sig_ptr;
+	struct bpf_key *trusted_keyring;
+	__u32 pid;
+	int ret;
+
+	pid = bpf_get_current_pid_tgid() >> 32;
+	if (pid != monitored_pid)
+		return 0;
+
+	/* digest_ptr points to fsverity_digest */
+	bpf_dynptr_from_mem(digest + MAGIC_SIZE, sizeof(digest) - MAGIC_SIZE, 0, &digest_ptr);
+
+	ret = bpf_get_fsverity_digest(f, &digest_ptr);
+	/* No verity, allow access */
+	if (ret < 0)
+		return 0;
+
+	/* Move digest_ptr to fsverity_formatted_digest */
+	bpf_dynptr_from_mem(digest, sizeof(digest), 0, &digest_ptr);
+
+	/* Read signature from xattr */
+	bpf_dynptr_from_mem(sig, sizeof(sig), 0, &sig_ptr);
+	ret = bpf_get_file_xattr(f, "user.sig", &sig_ptr);
+	/* No signature, reject access */
+	if (ret < 0)
+		return -EPERM;
+
+	trusted_keyring = bpf_lookup_user_key(user_keyring_serial, 0);
+	if (!trusted_keyring)
+		return -ENOENT;
+
+	/* Verify signature */
+	ret = bpf_verify_pkcs7_signature(&digest_ptr, &sig_ptr, trusted_keyring);
+
+	bpf_key_put(trusted_keyring);
+	return ret;
+}
diff --git a/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
index 7748cc23de8a..f42e9f3831a1 100644
--- a/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
+++ b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
@@ -10,17 +10,11 @@
 #include <errno.h>
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_tracing.h>
+#include "bpf_kfuncs.h"
 
 #define MAX_DATA_SIZE (1024 * 1024)
 #define MAX_SIG_SIZE 1024
 
-extern struct bpf_key *bpf_lookup_user_key(__u32 serial, __u64 flags) __ksym;
-extern struct bpf_key *bpf_lookup_system_key(__u64 id) __ksym;
-extern void bpf_key_put(struct bpf_key *key) __ksym;
-extern int bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr,
-				      struct bpf_dynptr *sig_ptr,
-				      struct bpf_key *trusted_keyring) __ksym;
-
 __u32 monitored_pid;
 __u32 user_keyring_serial;
 __u64 system_keyring_id;
diff --git a/tools/testing/selftests/bpf/verify_sig_setup.sh b/tools/testing/selftests/bpf/verify_sig_setup.sh
index ba08922b4a27..7e6caa134e1a 100755
--- a/tools/testing/selftests/bpf/verify_sig_setup.sh
+++ b/tools/testing/selftests/bpf/verify_sig_setup.sh
@@ -60,6 +60,27 @@ cleanup() {
 	rm -rf ${tmp_dir}
 }
 
+fsverity_create_sign_file() {
+	local tmp_dir="$1"
+
+	data_file=${tmp_dir}/data-file
+	sig_file=${tmp_dir}/sig-file
+	dd if=/dev/urandom of=$data_file bs=1 count=12345 2> /dev/null
+	fsverity sign --key ${tmp_dir}/signing_key.pem $data_file $sig_file
+
+	# We do not want to enable fsverity on $data_file yet. Try whether
+	# the file system support fsverity on a different file.
+	touch ${tmp_dir}/tmp-file
+	fsverity enable ${tmp_dir}/tmp-file
+}
+
+fsverity_enable_file() {
+    local tmp_dir="$1"
+
+	data_file=${tmp_dir}/data-file
+	fsverity enable $data_file
+}
+
 catch()
 {
 	local exit_code="$1"
@@ -86,6 +107,10 @@ main()
 		setup "${tmp_dir}"
 	elif [[ "${action}" == "cleanup" ]]; then
 		cleanup "${tmp_dir}"
+	elif [[ "${action}" == "fsverity-create-sign" ]]; then
+		fsverity_create_sign_file "${tmp_dir}"
+	elif [[ "${action}" == "fsverity-enable" ]]; then
+		fsverity_enable_file "${tmp_dir}"
 	else
 		echo "Unknown action: ${action}"
 		exit 1
-- 
2.34.1

Re: [PATCH v13 bpf-next 1/6] bpf: Add kfunc bpf_get_file_xattr

[Song Liu]

On Thu, Nov 23, 2023 at 3:40 PM Song Liu <song@kernel.org> 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.
>
> Signed-off-by: Song Liu <song@kernel.org>
> ---
>  kernel/trace/bpf_trace.c | 63 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index f0b8b7c29126..55758a6fbe90 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>
>
> @@ -1431,6 +1432,68 @@ static int __init bpf_key_sig_kfuncs_init(void)
>  late_initcall(bpf_key_sig_kfuncs_init);
>  #endif /* CONFIG_KEYS */
>
> +/* filesystem kfuncs */
> +__bpf_kfunc_start_defs();
> +
> +/**
> + * 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);
> +}
> +
> +__bpf_kfunc_end_defs();
> +
> +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 = {

Missed static here. If there will be v14, I will fix it here.

Thanks,
Song

> +       .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)
>  {
> --
> 2.34.1
>

Re: [PATCH v13 bpf-next 1/6] bpf: Add kfunc bpf_get_file_xattr

[Christian Brauner]

On Thu, Nov 23, 2023 at 03:39:31PM -0800, Song Liu 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.
> 
> Signed-off-by: Song Liu <song@kernel.org>
> ---

Looks ok to me. But see below for a question.

If you ever allow the retrieval of additional extended attributes
through bfs_get_file_xattr() or other bpf interfaces we would like to be
Cced, please. The xattr stuff is (/me looks for suitable words)...

Over the last months we've moved POSIX_ACL retrieval out of these
low-level functions. They now have a dedicated api. The same is going to
happen for fscaps as well.

But even with these out of the way we would want the bpf helpers to
always maintain an allowlist of retrievable attributes.

>  kernel/trace/bpf_trace.c | 63 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index f0b8b7c29126..55758a6fbe90 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>
>  
> @@ -1431,6 +1432,68 @@ static int __init bpf_key_sig_kfuncs_init(void)
>  late_initcall(bpf_key_sig_kfuncs_init);
>  #endif /* CONFIG_KEYS */
>  
> +/* filesystem kfuncs */
> +__bpf_kfunc_start_defs();
> +
> +/**
> + * 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);

By calling __vfs_getxattr() from bpf_get_file_xattr() you're skipping at
least inode_permission() from xattr_permission(). I'm probably just
missing or forgot the context. But why is that ok?

> +}
> +
> +__bpf_kfunc_end_defs();
> +
> +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)
>  {
> -- 
> 2.34.1
> 

Re: [PATCH v13 bpf-next 1/6] bpf: Add kfunc bpf_get_file_xattr

[Song Liu]

On Fri, Nov 24, 2023 at 12:44 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Thu, Nov 23, 2023 at 03:39:31PM -0800, Song Liu 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.
> >
> > Signed-off-by: Song Liu <song@kernel.org>
> > ---
>
> Looks ok to me. But see below for a question.
>
> If you ever allow the retrieval of additional extended attributes
> through bfs_get_file_xattr() or other bpf interfaces we would like to be
> Cced, please. The xattr stuff is (/me looks for suitable words)...
>
> Over the last months we've moved POSIX_ACL retrieval out of these
> low-level functions. They now have a dedicated api. The same is going to
> happen for fscaps as well.
>
> But even with these out of the way we would want the bpf helpers to
> always maintain an allowlist of retrievable attributes.

Agreed. We will be very specific which attributes are available to bpf
helpers/kfuncs.

>
> >  kernel/trace/bpf_trace.c | 63 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index f0b8b7c29126..55758a6fbe90 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>
> >
> > @@ -1431,6 +1432,68 @@ static int __init bpf_key_sig_kfuncs_init(void)
> >  late_initcall(bpf_key_sig_kfuncs_init);
> >  #endif /* CONFIG_KEYS */
> >
> > +/* filesystem kfuncs */
> > +__bpf_kfunc_start_defs();
> > +
> > +/**
> > + * 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);
>
> By calling __vfs_getxattr() from bpf_get_file_xattr() you're skipping at
> least inode_permission() from xattr_permission(). I'm probably just
> missing or forgot the context. But why is that ok?

AFAICT, the XATTR_USER_PREFIX above is equivalent to the prefix
check in xattr_permission().

For inode_permission(), I think it is not required because we already
have the "struct file" of  the target file. Did I misunderstand something
here?

Thanks,
Song

> > +}
> > +

[...]

Re: [PATCH v13 bpf-next 6/6] selftests/bpf: Add test that uses fsverity and xattr to sign a file

[kernel test robot]

Hi Song,

kernel test robot noticed the following build errors:

[auto build test ERROR on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Song-Liu/bpf-Add-kfunc-bpf_get_file_xattr/20231124-074239
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20231123233936.3079687-7-song%40kernel.org
patch subject: [PATCH v13 bpf-next 6/6] selftests/bpf: Add test that uses fsverity and xattr to sign a file
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231125/202311250314.KGxKh0fm-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311250314.KGxKh0fm-lkp@intel.com/

All errors (new ones prefixed by >>):

>> progs/test_sig_in_xattr.c:36:26: error: invalid application of 'sizeof' to an incomplete type 'struct fsverity_digest'
      36 | char digest[MAGIC_SIZE + sizeof(struct fsverity_digest) + SHA256_DIGEST_SIZE];
         |                          ^     ~~~~~~~~~~~~~~~~~~~~~~~~
   progs/test_sig_in_xattr.c:36:40: note: forward declaration of 'struct fsverity_digest'
      36 | char digest[MAGIC_SIZE + sizeof(struct fsverity_digest) + SHA256_DIGEST_SIZE];
         |                                        ^
   1 error generated.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v13 bpf-next 5/6] selftests/bpf: Add tests for filesystem kfuncs

[Alexei Starovoitov]

On Thu, Nov 23, 2023 at 3:40 PM Song Liu <song@kernel.org> wrote:
>
> +static const char expected_value[] = "hello";
> +char value[32];
> +
> +SEC("lsm.s/file_open")
> +int BPF_PROG(test_file_open, struct file *f)
> +{
> +       struct bpf_dynptr value_ptr;
> +       __u32 pid;
> +       int ret;
> +
> +       pid = bpf_get_current_pid_tgid() >> 32;
> +       if (pid != monitored_pid)
> +               return 0;
> +
> +       bpf_dynptr_from_mem(value, sizeof(value), 0, &value_ptr);
> +
> +       ret = bpf_get_file_xattr(f, "user.kfuncs", &value_ptr);
> +       if (ret != sizeof(expected_value))
> +               return 0;
> +       if (bpf_strncmp(value, ret, expected_value))

Hmm. It doesn't work like:
if (bpf_strncmp(value, ret, "hello"))

?

Re: [PATCH v13 bpf-next 1/6] bpf: Add kfunc bpf_get_file_xattr

[Christian Brauner]

On Fri, Nov 24, 2023 at 09:07:33AM -0800, Song Liu wrote:
> On Fri, Nov 24, 2023 at 12:44 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Thu, Nov 23, 2023 at 03:39:31PM -0800, Song Liu 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.
> > >
> > > Signed-off-by: Song Liu <song@kernel.org>
> > > ---
> >
> > Looks ok to me. But see below for a question.
> >
> > If you ever allow the retrieval of additional extended attributes
> > through bfs_get_file_xattr() or other bpf interfaces we would like to be
> > Cced, please. The xattr stuff is (/me looks for suitable words)...
> >
> > Over the last months we've moved POSIX_ACL retrieval out of these
> > low-level functions. They now have a dedicated api. The same is going to
> > happen for fscaps as well.
> >
> > But even with these out of the way we would want the bpf helpers to
> > always maintain an allowlist of retrievable attributes.
> 
> Agreed. We will be very specific which attributes are available to bpf
> helpers/kfuncs.
> 
> >
> > >  kernel/trace/bpf_trace.c | 63 ++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 63 insertions(+)
> > >
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index f0b8b7c29126..55758a6fbe90 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>
> > >
> > > @@ -1431,6 +1432,68 @@ static int __init bpf_key_sig_kfuncs_init(void)
> > >  late_initcall(bpf_key_sig_kfuncs_init);
> > >  #endif /* CONFIG_KEYS */
> > >
> > > +/* filesystem kfuncs */
> > > +__bpf_kfunc_start_defs();
> > > +
> > > +/**
> > > + * 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);
> >
> > By calling __vfs_getxattr() from bpf_get_file_xattr() you're skipping at
> > least inode_permission() from xattr_permission(). I'm probably just
> > missing or forgot the context. But why is that ok?
> 
> AFAICT, the XATTR_USER_PREFIX above is equivalent to the prefix
> check in xattr_permission().
> 
> For inode_permission(), I think it is not required because we already
> have the "struct file" of  the target file. Did I misunderstand something
> here?

I had overlooked that you don't allow writing xattrs. But there's still
some issues:

So if you look at the system call interface:

fgetxattr(fd)
-> getxattr()
   -> do_getxattr()
      -> vfs_getxattr()
         -> xattr_permission()
         -> __vfs_getxattr()

and io_uring:

do_getxattr()
-> vfs_getxattr()
   -> xattr_permission()
   -> __vfs_getxattr()

you can see that xattr_permission() is a _read/write-time check_, not an
open check. That's because the read/write permissions may depend on what
xattr is read/written. Since you don't know what xattr will be
read/written at open-time.

So there needs to be a good reason for bpf_get_file_xattr() to deviate
from the system call and io_uring interface. And I'd like to hear it,
please. :)

I think I might see the argument because you document the helper as "may
only be called from BPF LSM function" in which case you're trying to say
that bpf_get_file_xattr() is equivalent to a call to __vfs_getxattr()
from an LSM to get at it's own security xattr.

But if that's the case you really should have a way to verify that these
helpers are only callable from a specific BPF context. Because you
otherwise omit read/write-time permission checking when retrieving
xattrs which is a potentialy security issue and may be abused by a BPF
program to skip permission checks that are otherwise enforced.

Is there a way for BPF to enforce/verify that a function is only called
from a specific BPF program? It should be able to recognize that, no?
And then refuse to load that BPF program if a helper is called outside
it's intended context.

Re: [PATCH v13 bpf-next 5/6] selftests/bpf: Add tests for filesystem kfuncs

[Song Liu]

On Sun, Nov 26, 2023 at 6:09 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Nov 23, 2023 at 3:40 PM Song Liu <song@kernel.org> wrote:
> >
> > +static const char expected_value[] = "hello";
> > +char value[32];
> > +
> > +SEC("lsm.s/file_open")
> > +int BPF_PROG(test_file_open, struct file *f)
> > +{
> > +       struct bpf_dynptr value_ptr;
> > +       __u32 pid;
> > +       int ret;
> > +
> > +       pid = bpf_get_current_pid_tgid() >> 32;
> > +       if (pid != monitored_pid)
> > +               return 0;
> > +
> > +       bpf_dynptr_from_mem(value, sizeof(value), 0, &value_ptr);
> > +
> > +       ret = bpf_get_file_xattr(f, "user.kfuncs", &value_ptr);
> > +       if (ret != sizeof(expected_value))
> > +               return 0;
> > +       if (bpf_strncmp(value, ret, expected_value))
>
> Hmm. It doesn't work like:
> if (bpf_strncmp(value, ret, "hello"))

This also works. I used expected_value because there is a size
check above. We can also make do something like

if (ret != sizeof("hello"))
             return 0;
if (bpf_strncmp(value, ret, "hello"))
             return 0;

Both of the two work.

Thanks,
Song

Re: [PATCH v13 bpf-next 1/6] bpf: Add kfunc bpf_get_file_xattr

[Song Liu]

Hi Christian,

Thanks again for your comments.

On Mon, Nov 27, 2023 at 2:50 AM Christian Brauner <brauner@kernel.org> wrote:
>
[...]
> >
> > AFAICT, the XATTR_USER_PREFIX above is equivalent to the prefix
> > check in xattr_permission().
> >
> > For inode_permission(), I think it is not required because we already
> > have the "struct file" of  the target file. Did I misunderstand something
> > here?
>
> I had overlooked that you don't allow writing xattrs. But there's still
> some issues:
>
> So if you look at the system call interface:
>
> fgetxattr(fd)
> -> getxattr()
>    -> do_getxattr()
>       -> vfs_getxattr()
>          -> xattr_permission()
>          -> __vfs_getxattr()
>
> and io_uring:
>
> do_getxattr()
> -> vfs_getxattr()
>    -> xattr_permission()
>    -> __vfs_getxattr()
>
> you can see that xattr_permission() is a _read/write-time check_, not an
> open check. That's because the read/write permissions may depend on what
> xattr is read/written. Since you don't know what xattr will be
> read/written at open-time.
>
> So there needs to be a good reason for bpf_get_file_xattr() to deviate
> from the system call and io_uring interface. And I'd like to hear it,
> please. :)
>
> I think I might see the argument because you document the helper as "may
> only be called from BPF LSM function" in which case you're trying to say
> that bpf_get_file_xattr() is equivalent to a call to __vfs_getxattr()
> from an LSM to get at it's own security xattr.
>
> But if that's the case you really should have a way to verify that these
> helpers are only callable from a specific BPF context. Because you
> otherwise omit read/write-time permission checking when retrieving
> xattrs which is a potentialy security issue and may be abused by a BPF
> program to skip permission checks that are otherwise enforced.

What do you mean by "a specific BPF context"? Current implementation
makes sure the helper only works on LSM hooks with "struct file *" in the
argument list. Specifically, we can only use them from the following hooks:

    security_binder_transfer_file
    security_bprm_creds_from_file
    security_file_permission
    security_file_alloc_security
    security_file_free_security
    security_file_ioctl
    security_mmap_file
    security_file_lock
    security_file_fcntl
    security_file_set_fowner
    security_file_receive
    security_file_open
    security_file_truncate
    security_kernel_read_file
    security_kernel_post_read_file

Note that, we disallow pointer-walking with the kfunc, so the kfunc is not
allowed from hooks with indirect access to "struct file". For example, we
cannot use it with security_bprm_creds_for_exec(struct linux_binprm *bprm)
as this hook only has bprm, and calling bpf_get_file_xattr(bprm->file) is
not allowed.

> Is there a way for BPF to enforce/verify that a function is only called
> from a specific BPF program? It should be able to recognize that, no?
> And then refuse to load that BPF program if a helper is called outside
> it's intended context.

Similarly, I am not quite sure what you mean by "a specific BPF program".
My answer to this is probably the same as above.

Going back to xattr_permission itself. AFAICT, it does 3 checks:

1. MAY_WRITE check;
2. prefix check;
3. inode_permission().

We don't need MAY_WRITE check as bpf_get_file_xattr is read only.
We have the prefix check embedded in bpf_get_file_xattr():

       if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN))
               return -EPERM;

inode_permission() is a little trickier here, which checks against idmap.
However, I don't think the check makes sense in the context of LSM.
In this case, we have two processes: one security daemon, which
owns the BPF LSM program, and a process being monitored.
idmap here, from file_mnt_idmap(file), is the idmap from the being
monitored process. However, whether the BPF LSM program have the
permission to read the xattr should be determined by the security
daemon.

Overall, we can technically add xattr_permission() check here. But I
don't think that's the right check for the LSM use case.

Does this make sense? Did I miss or misunderstand something?

Thanks,
Song

Re: [PATCH v13 bpf-next 1/6] bpf: Add kfunc bpf_get_file_xattr

[Song Liu]

On Mon, Nov 27, 2023 at 10:05 AM Song Liu <song@kernel.org> wrote:
>
> Hi Christian,
>
> Thanks again for your comments.
>
> On Mon, Nov 27, 2023 at 2:50 AM Christian Brauner <brauner@kernel.org> wrote:
> >
[...]
> Going back to xattr_permission itself. AFAICT, it does 3 checks:
>
> 1. MAY_WRITE check;
> 2. prefix check;
> 3. inode_permission().
>
> We don't need MAY_WRITE check as bpf_get_file_xattr is read only.
> We have the prefix check embedded in bpf_get_file_xattr():
>
>        if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN))
>                return -EPERM;
>
> inode_permission() is a little trickier here, which checks against idmap.
> However, I don't think the check makes sense in the context of LSM.
> In this case, we have two processes: one security daemon, which
> owns the BPF LSM program, and a process being monitored.
> idmap here, from file_mnt_idmap(file), is the idmap from the being
> monitored process. However, whether the BPF LSM program have the
> permission to read the xattr should be determined by the security
> daemon.

Maybe we should check against nop_mnt_idmap? Would something like
the following make more sense?

Thanks,
Song

diff --git i/kernel/trace/bpf_trace.c w/kernel/trace/bpf_trace.c
index 0019e990408e..62fc51bc57af 100644
--- i/kernel/trace/bpf_trace.c
+++ w/kernel/trace/bpf_trace.c
@@ -1453,6 +1453,7 @@ __bpf_kfunc int bpf_get_file_xattr(struct file
*file, const char *name__str,
        struct dentry *dentry;
        u32 value_len;
        void *value;
+       int ret;

        if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN))
                return -EPERM;
@@ -1463,6 +1464,9 @@ __bpf_kfunc int bpf_get_file_xattr(struct file
*file, const char *name__str,
                return -EINVAL;

        dentry = file_dentry(file);
+       ret = inode_permission(&nop_mnt_idmap, dentry->d_inode, MAY_READ);
+       if (ret)
+               return ret;
        return __vfs_getxattr(dentry, dentry->d_inode, name__str,
value, value_len);
 }

Re: [PATCH v13 bpf-next 1/6] bpf: Add kfunc bpf_get_file_xattr

[Christian Brauner]

On Mon, Nov 27, 2023 at 10:05:23AM -0800, Song Liu wrote:
> Hi Christian,
> 
> Thanks again for your comments.
> 
> On Mon, Nov 27, 2023 at 2:50 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> [...]
> > >
> > > AFAICT, the XATTR_USER_PREFIX above is equivalent to the prefix
> > > check in xattr_permission().
> > >
> > > For inode_permission(), I think it is not required because we already
> > > have the "struct file" of  the target file. Did I misunderstand something
> > > here?
> >
> > I had overlooked that you don't allow writing xattrs. But there's still
> > some issues:
> >
> > So if you look at the system call interface:
> >
> > fgetxattr(fd)
> > -> getxattr()
> >    -> do_getxattr()
> >       -> vfs_getxattr()
> >          -> xattr_permission()
> >          -> __vfs_getxattr()
> >
> > and io_uring:
> >
> > do_getxattr()
> > -> vfs_getxattr()
> >    -> xattr_permission()
> >    -> __vfs_getxattr()
> >
> > you can see that xattr_permission() is a _read/write-time check_, not an
> > open check. That's because the read/write permissions may depend on what
> > xattr is read/written. Since you don't know what xattr will be
> > read/written at open-time.
> >
> > So there needs to be a good reason for bpf_get_file_xattr() to deviate
> > from the system call and io_uring interface. And I'd like to hear it,
> > please. :)
> >
> > I think I might see the argument because you document the helper as "may
> > only be called from BPF LSM function" in which case you're trying to say
> > that bpf_get_file_xattr() is equivalent to a call to __vfs_getxattr()
> > from an LSM to get at it's own security xattr.
> >
> > But if that's the case you really should have a way to verify that these
> > helpers are only callable from a specific BPF context. Because you
> > otherwise omit read/write-time permission checking when retrieving
> > xattrs which is a potentialy security issue and may be abused by a BPF
> > program to skip permission checks that are otherwise enforced.
> 
> What do you mean by "a specific BPF context"? Current implementation
> makes sure the helper only works on LSM hooks with "struct file *" in the
> argument list. Specifically, we can only use them from the following hooks:
> 
>     security_binder_transfer_file
>     security_bprm_creds_from_file
>     security_file_permission
>     security_file_alloc_security
>     security_file_free_security
>     security_file_ioctl
>     security_mmap_file
>     security_file_lock
>     security_file_fcntl
>     security_file_set_fowner
>     security_file_receive
>     security_file_open
>     security_file_truncate
>     security_kernel_read_file
>     security_kernel_post_read_file

Ok, good!

> Note that, we disallow pointer-walking with the kfunc, so the kfunc is not
> allowed from hooks with indirect access to "struct file". For example, we
> cannot use it with security_bprm_creds_for_exec(struct linux_binprm *bprm)
> as this hook only has bprm, and calling bpf_get_file_xattr(bprm->file) is
> not allowed.

Great.

> 
> > Is there a way for BPF to enforce/verify that a function is only called
> > from a specific BPF program? It should be able to recognize that, no?
> > And then refuse to load that BPF program if a helper is called outside
> > it's intended context.
> 
> Similarly, I am not quite sure what you mean by "a specific BPF program".
> My answer to this is probably the same as above.

Yes, this is exactly what I meant.

> 
> Going back to xattr_permission itself. AFAICT, it does 3 checks:
> 
> 1. MAY_WRITE check;
> 2. prefix check;
> 3. inode_permission().
> 
> We don't need MAY_WRITE check as bpf_get_file_xattr is read only.
> We have the prefix check embedded in bpf_get_file_xattr():
> 
>        if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN))
>                return -EPERM;
> 
> inode_permission() is a little trickier here, which checks against idmap.
> However, I don't think the check makes sense in the context of LSM.
> In this case, we have two processes: one security daemon, which
> owns the BPF LSM program, and a process being monitored.
> idmap here, from file_mnt_idmap(file), is the idmap from the being
> monitored process. However, whether the BPF LSM program have the
> permission to read the xattr should be determined by the security
> daemon.
> 
> Overall, we can technically add xattr_permission() check here. But I
> don't think that's the right check for the LSM use case.
> 
> Does this make sense? Did I miss or misunderstand something?

If the helper is only callable from an LSM context then this should be
fine.

Re: [PATCH v13 bpf-next 1/6] bpf: Add kfunc bpf_get_file_xattr

[Song Liu]

Hi Christian,

On Tue, Nov 28, 2023 at 1:13 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Mon, Nov 27, 2023 at 10:05:23AM -0800, Song Liu wrote:
[...]
> >
> > Overall, we can technically add xattr_permission() check here. But I
> > don't think that's the right check for the LSM use case.
> >
> > Does this make sense? Did I miss or misunderstand something?
>
> If the helper is only callable from an LSM context then this should be
> fine.

If everything looks good, would you please give an official Acked-by or
Reviewed-by?

Thanks,
Song

Re: [PATCH v13 bpf-next 1/6] bpf: Add kfunc bpf_get_file_xattr

[Christian Brauner]

On Tue, Nov 28, 2023 at 06:19:35AM -0800, Song Liu wrote:
> Hi Christian,
> 
> On Tue, Nov 28, 2023 at 1:13 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Mon, Nov 27, 2023 at 10:05:23AM -0800, Song Liu wrote:
> [...]
> > >
> > > Overall, we can technically add xattr_permission() check here. But I
> > > don't think that's the right check for the LSM use case.
> > >
> > > Does this make sense? Did I miss or misunderstand something?
> >
> > If the helper is only callable from an LSM context then this should be
> > fine.
> 
> If everything looks good, would you please give an official Acked-by or
> Reviewed-by?

Yeah looks ok to me,
Acked-by: Christian Brauner <brauner@kernel.org>