[PATCH bpf-next 0/5] bpf: file verification with LSM and fsverity

[PATCH bpf-next 0/5] bpf: file verification with LSM and fsverity

[Song Liu]

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 (5):
  bpf: Add kfunc bpf_get_file_xattr
  bpf, fsverity: Add kfunc bpf_get_fsverity_digest
  selftests/bpf: Sort config in alphabetic order
  selftests/bpf: Add tests for filesystem kfuncs
  selftests/bpf: Add test that use fsverity and xattr to sign a file

 fs/verity/measure.c                           |  66 +++++++
 include/linux/bpf.h                           |  12 ++
 kernel/trace/bpf_trace.c                      |  44 +++++
 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      |  39 +++++
 .../selftests/bpf/progs/test_sig_in_xattr.c   |  84 +++++++++
 .../bpf/progs/test_verify_pkcs7_sig.c         |   8 +-
 .../testing/selftests/bpf/verify_sig_setup.sh |  25 +++
 12 files changed, 623 insertions(+), 9 deletions(-)
 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 bpf-next 1/5] bpf: Add kfunc bpf_get_file_xattr

[Song Liu]

This kfunc can be used to read xattr of a file.

Since vfs_getxattr() requires null-terminated string as input "name", a new
helper bpf_dynptr_is_string() is added to check the input before calling
vfs_getxattr().

Signed-off-by: Song Liu <song@kernel.org>
---
 include/linux/bpf.h      | 12 +++++++++++
 kernel/trace/bpf_trace.c | 44 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 61bde4520f5c..f14fae45e13d 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2472,6 +2472,13 @@ static inline bool has_current_bpf_ctx(void)
 	return !!current->bpf_ctx;
 }
 
+static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
+{
+	char *str = ptr->data;
+
+	return str[__bpf_dynptr_size(ptr) - 1] == '\0';
+}
+
 void notrace bpf_prog_inc_misses_counter(struct bpf_prog *prog);
 
 void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data,
@@ -2708,6 +2715,11 @@ static inline bool has_current_bpf_ctx(void)
 	return false;
 }
 
+static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
+{
+	return false;
+}
+
 static inline void bpf_prog_inc_misses_counter(struct bpf_prog *prog)
 {
 }
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index df697c74d519..946268574e05 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>
 
@@ -1429,6 +1430,49 @@ static int __init bpf_key_sig_kfuncs_init(void)
 late_initcall(bpf_key_sig_kfuncs_init);
 #endif /* CONFIG_KEYS */
 
+/* filesystem kfuncs */
+__diag_push();
+__diag_ignore_all("-Wmissing-prototypes",
+		  "kfuncs which will be used in BPF programs");
+
+/**
+ * bpf_get_file_xattr - get xattr of a file
+ * @name_ptr: name of the xattr
+ * @value_ptr: output buffer of the xattr value
+ *
+ * Get xattr *name_ptr* of *file* and store the output in *value_ptr*.
+ *
+ * Return: 0 on success, a negative value on error.
+ */
+__bpf_kfunc int bpf_get_file_xattr(struct file *file, struct bpf_dynptr_kern *name_ptr,
+				   struct bpf_dynptr_kern *value_ptr)
+{
+	if (!bpf_dynptr_is_string(name_ptr))
+		return -EINVAL;
+
+	return vfs_getxattr(mnt_idmap(file->f_path.mnt), file_dentry(file), name_ptr->data,
+			    value_ptr->data, __bpf_dynptr_size(value_ptr));
+}
+
+__diag_pop();
+
+BTF_SET8_START(fs_kfunc_set)
+BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE)
+BTF_SET8_END(fs_kfunc_set)
+
+const struct btf_kfunc_id_set bpf_fs_kfunc_set = {
+	.owner = THIS_MODULE,
+	.set = &fs_kfunc_set,
+};
+
+static int __init bpf_fs_kfuncs_init(void)
+{
+	return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING,
+					 &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 bpf-next 2/5] bpf, fsverity: Add kfunc bpf_get_fsverity_digest

[Song Liu]

The kfunc can be used to read fsverity_digest, so that we can verify
signature in BPF LSM.

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).

Signed-off-by: Song Liu <song@kernel.org>
---
 fs/verity/measure.c | 66 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/fs/verity/measure.c b/fs/verity/measure.c
index eec5956141da..2d4b2e6f5a5d 100644
--- a/fs/verity/measure.c
+++ b/fs/verity/measure.c
@@ -8,6 +8,8 @@
 #include "fsverity_private.h"
 
 #include <linux/uaccess.h>
+#include <linux/bpf.h>
+#include <linux/btf.h>
 
 /**
  * fsverity_ioctl_measure() - get a verity file's digest
@@ -100,3 +102,67 @@ int fsverity_get_digest(struct inode *inode,
 	return hash_alg->digest_size;
 }
 EXPORT_SYMBOL_GPL(fsverity_get_digest);
+
+/* bpf kfuncs */
+__diag_push();
+__diag_ignore_all("-Wmissing-prototypes",
+		  "kfuncs which will be used in BPF programs");
+
+/**
+ * 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);
+	struct fsverity_digest *arg = digest_ptr->data;
+	const struct fsverity_info *vi;
+	const struct fsverity_hash_alg *hash_alg;
+	int out_digest_sz;
+
+	if (__bpf_dynptr_size(digest_ptr) < sizeof(struct fsverity_digest))
+		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 = __bpf_dynptr_size(digest_ptr) - 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 */
+	memset(arg->digest + arg->digest_size, 0, out_digest_sz - hash_alg->digest_size);
+
+	return 0;
+}
+
+__diag_pop();
+
+BTF_SET8_START(fsverity_set)
+BTF_ID_FLAGS(func, bpf_get_fsverity_digest, KF_SLEEPABLE)
+BTF_SET8_END(fsverity_set)
+
+const struct btf_kfunc_id_set bpf_fsverity_set = {
+	.owner = THIS_MODULE,
+	.set = &fsverity_set,
+};
+
+static int __init bpf_fsverity_init(void)
+{
+	return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING,
+					 &bpf_fsverity_set);
+}
+
+late_initcall(bpf_fsverity_init);
-- 
2.34.1

[PATCH bpf-next 3/5] selftests/bpf: Sort config in alphabetic order

[Song Liu]

Move CONFIG_VSOCKETS up, so the CONFIGs are in sorted 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 02dd4409200e..09da30be8728 100644
--- a/tools/testing/selftests/bpf/config
+++ b/tools/testing/selftests/bpf/config
@@ -81,7 +81,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 bpf-next 4/5] 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.

CONFIG_FS_VERITY is added to selftests config. However, this is not
sufficient to garantee 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      |  39 ++++++
 5 files changed, 221 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..ce27b0f3ffc0 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, struct bpf_dynptr *name_ptr,
+			      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 09da30be8728..4534a913e46c 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..dc018877526f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_get_xattr.c
@@ -0,0 +1,39 @@
+// 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;
+
+char key[] = "user.kfuncs";
+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 key_ptr, value_ptr;
+	__u32 pid;
+	int ret;
+
+	pid = bpf_get_current_pid_tgid() >> 32;
+	if (pid != monitored_pid)
+		return 0;
+
+	bpf_dynptr_from_mem(key, sizeof(key), 0, &key_ptr);
+	bpf_dynptr_from_mem(value, sizeof(value), 0, &value_ptr);
+
+	ret = bpf_get_file_xattr(f, &key_ptr, &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 bpf-next 5/5] selftests/bpf: Add test that use 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   |  84 +++++++++
 .../bpf/progs/test_verify_pkcs7_sig.c         |   8 +-
 .../testing/selftests/bpf/verify_sig_setup.sh |  25 +++
 5 files changed, 279 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 ce27b0f3ffc0..fd17f1da42eb 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, struct bpf_dynptr *name_ptr,
 			      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..e7933091f52f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_sig_in_xattr.c
@@ -0,0 +1,84 @@
+// 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 xattr_name[] = "user.sig";
+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, name_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);
+	bpf_dynptr_from_mem(xattr_name, sizeof(xattr_name), 0, &name_ptr);
+	ret = bpf_get_file_xattr(f, &name_ptr, &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 bpf-next 2/5] bpf, fsverity: Add kfunc bpf_get_fsverity_digest

[Eric Biggers]

On Fri, Oct 13, 2023 at 11:26:41AM -0700, Song Liu wrote:
> The kfunc can be used to read fsverity_digest, so that we can verify
> signature in BPF LSM.
> 
> 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).
> 
> Signed-off-by: Song Liu <song@kernel.org>
> ---
>  fs/verity/measure.c | 66 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/fs/verity/measure.c b/fs/verity/measure.c
> index eec5956141da..2d4b2e6f5a5d 100644
> --- a/fs/verity/measure.c
> +++ b/fs/verity/measure.c
> @@ -8,6 +8,8 @@
>  #include "fsverity_private.h"
>  
>  #include <linux/uaccess.h>
> +#include <linux/bpf.h>
> +#include <linux/btf.h>
>  
>  /**
>   * fsverity_ioctl_measure() - get a verity file's digest
> @@ -100,3 +102,67 @@ int fsverity_get_digest(struct inode *inode,
>  	return hash_alg->digest_size;
>  }
>  EXPORT_SYMBOL_GPL(fsverity_get_digest);
> +
> +/* bpf kfuncs */
> +__diag_push();
> +__diag_ignore_all("-Wmissing-prototypes",
> +		  "kfuncs which will be used in BPF programs");
> +
> +/**
> + * 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);
> +	struct fsverity_digest *arg = digest_ptr->data;

What alignment is guaranteed here?

> +	const struct fsverity_info *vi;
> +	const struct fsverity_hash_alg *hash_alg;
> +	int out_digest_sz;
> +
> +	if (__bpf_dynptr_size(digest_ptr) < sizeof(struct fsverity_digest))
> +		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 = __bpf_dynptr_size(digest_ptr) - 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 */
> +	memset(arg->digest + arg->digest_size, 0, out_digest_sz - hash_alg->digest_size);

Can't 'out_digest_sz - hash_alg->digest_size' underflow?

> +
> +	return 0;
> +}
> +
> +__diag_pop();
> +
> +BTF_SET8_START(fsverity_set)
> +BTF_ID_FLAGS(func, bpf_get_fsverity_digest, KF_SLEEPABLE)

Should it be sleepable?  Nothing in it sleeps, as far as I can tell.

> +BTF_SET8_END(fsverity_set)
> +
> +const struct btf_kfunc_id_set bpf_fsverity_set = {
> +	.owner = THIS_MODULE,
> +	.set = &fsverity_set,
> +};

static const?

> +
> +static int __init bpf_fsverity_init(void)
> +{
> +	return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING,
> +					 &bpf_fsverity_set);
> +}
> +
> +late_initcall(bpf_fsverity_init);

Maybe this should be called by the existing fsverity_init() initcall instead of
having a brand new initcall just for this.

Also, doesn't this all need to be guarded by a kconfig such as CONFIG_BPF?

Also, it looks like I'm being signed up to maintain this.  This isn't a stable
UAPI, right?  No need to document this in Documentation/?

- Eric

Re: [PATCH bpf-next 2/5] bpf, fsverity: Add kfunc bpf_get_fsverity_digest

[Song Liu]

On Sun, Oct 15, 2023 at 12:07 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
[...]
> > + */
> > +__bpf_kfunc int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr_kern *digest_ptr)
> > +{
> > +     const struct inode *inode = file_inode(file);
> > +     struct fsverity_digest *arg = digest_ptr->data;
>
> What alignment is guaranteed here?

drnptr doesn't not provide alignment guarantee for digest_ptr->data.
If we need alignment guarantee, we need to add it here.

>
> > +     const struct fsverity_info *vi;
> > +     const struct fsverity_hash_alg *hash_alg;
> > +     int out_digest_sz;
> > +
> > +     if (__bpf_dynptr_size(digest_ptr) < sizeof(struct fsverity_digest))
> > +             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 = __bpf_dynptr_size(digest_ptr) - 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 */
> > +     memset(arg->digest + arg->digest_size, 0, out_digest_sz - hash_alg->digest_size);
>
> Can't 'out_digest_sz - hash_alg->digest_size' underflow?

Will fix this in the next version.

>
> > +
> > +     return 0;
> > +}
> > +
> > +__diag_pop();
> > +
> > +BTF_SET8_START(fsverity_set)
> > +BTF_ID_FLAGS(func, bpf_get_fsverity_digest, KF_SLEEPABLE)
>
> Should it be sleepable?  Nothing in it sleeps, as far as I can tell.

Indeed, we can remove sleepable requirement here.

>
> > +BTF_SET8_END(fsverity_set)
> > +
> > +const struct btf_kfunc_id_set bpf_fsverity_set = {
> > +     .owner = THIS_MODULE,
> > +     .set = &fsverity_set,
> > +};
>
> static const?

Will fix in v2.

>
> > +
> > +static int __init bpf_fsverity_init(void)
> > +{
> > +     return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING,
> > +                                      &bpf_fsverity_set);
> > +}
> > +
> > +late_initcall(bpf_fsverity_init);
>
> Maybe this should be called by the existing fsverity_init() initcall instead of
> having a brand new initcall just for this.

Yeah, that would also work.

>
> Also, doesn't this all need to be guarded by a kconfig such as CONFIG_BPF?

Will add this in v2.

>
> Also, it looks like I'm being signed up to maintain this.  This isn't a stable
> UAPI, right?  No need to document this in Documentation/?

BPF kfuncs are not UAPI. They are as stable as exported symbols.
We do have some documents for BPF kfuncs, for example in
Documentation/bpf/cpumasks.rst.

Do you have a recommendation or preference on where we should
document this? AFAICT, we can either add it to fsverity.rst or somewhere
in Documentation/bpf/.

Thanks,
Song

Re: [PATCH bpf-next 2/5] bpf, fsverity: Add kfunc bpf_get_fsverity_digest

[Eric Biggers]

On Mon, Oct 16, 2023 at 01:10:40PM -0700, Song Liu wrote:
> On Sun, Oct 15, 2023 at 12:07 AM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> [...]
> > > + */
> > > +__bpf_kfunc int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr_kern *digest_ptr)
> > > +{
> > > +     const struct inode *inode = file_inode(file);
> > > +     struct fsverity_digest *arg = digest_ptr->data;
> >
> > What alignment is guaranteed here?
> 
> drnptr doesn't not provide alignment guarantee for digest_ptr->data.
> If we need alignment guarantee, we need to add it here.

So technically it's wrong to cast it to struct fsverity_digest, then.

> >
> > Also, it looks like I'm being signed up to maintain this.  This isn't a stable
> > UAPI, right?  No need to document this in Documentation/?
> 
> BPF kfuncs are not UAPI. They are as stable as exported symbols.
> We do have some documents for BPF kfuncs, for example in
> Documentation/bpf/cpumasks.rst.
> 
> Do you have a recommendation or preference on where we should
> document this? AFAICT, we can either add it to fsverity.rst or somewhere
> in Documentation/bpf/.

The BPF documentation seems like the right place.

- Eric

Re: [PATCH bpf-next 2/5] bpf, fsverity: Add kfunc bpf_get_fsverity_digest

[Song Liu]

On Mon, Oct 16, 2023 at 8:12 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Mon, Oct 16, 2023 at 01:10:40PM -0700, Song Liu wrote:
> > On Sun, Oct 15, 2023 at 12:07 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > >
> > [...]
> > > > + */
> > > > +__bpf_kfunc int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr_kern *digest_ptr)
> > > > +{
> > > > +     const struct inode *inode = file_inode(file);
> > > > +     struct fsverity_digest *arg = digest_ptr->data;
> > >
> > > What alignment is guaranteed here?
> >
> > drnptr doesn't not provide alignment guarantee for digest_ptr->data.
> > If we need alignment guarantee, we need to add it here.
>
> So technically it's wrong to cast it to struct fsverity_digest, then.

We can enforce alignment here. Would __aligned(2) be sufficient?

Thanks,
Song

Re: [PATCH bpf-next 2/5] bpf, fsverity: Add kfunc bpf_get_fsverity_digest

[Eric Biggers]

On Mon, Oct 16, 2023 at 10:35:16PM -0700, Song Liu wrote:
> On Mon, Oct 16, 2023 at 8:12 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Mon, Oct 16, 2023 at 01:10:40PM -0700, Song Liu wrote:
> > > On Sun, Oct 15, 2023 at 12:07 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > > >
> > > [...]
> > > > > + */
> > > > > +__bpf_kfunc int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr_kern *digest_ptr)
> > > > > +{
> > > > > +     const struct inode *inode = file_inode(file);
> > > > > +     struct fsverity_digest *arg = digest_ptr->data;
> > > >
> > > > What alignment is guaranteed here?
> > >
> > > drnptr doesn't not provide alignment guarantee for digest_ptr->data.
> > > If we need alignment guarantee, we need to add it here.
> >
> > So technically it's wrong to cast it to struct fsverity_digest, then.
> 
> We can enforce alignment here. Would __aligned(2) be sufficient?
> 

Do you mean something like the following:

	if (!IS_ALIGNED((uintptr_t)digest_ptr->data, __alignof__(*arg)))
		return -EINVAL;

Re: [PATCH bpf-next 2/5] bpf, fsverity: Add kfunc bpf_get_fsverity_digest

[Song Liu]

> On Oct 16, 2023, at 10:46 PM, Eric Biggers <ebiggers@kernel.org> wrote:
> 
> On Mon, Oct 16, 2023 at 10:35:16PM -0700, Song Liu wrote:
>> On Mon, Oct 16, 2023 at 8:12 PM Eric Biggers <ebiggers@kernel.org> wrote:
>>> 
>>> On Mon, Oct 16, 2023 at 01:10:40PM -0700, Song Liu wrote:
>>>> On Sun, Oct 15, 2023 at 12:07 AM Eric Biggers <ebiggers@kernel.org> wrote:
>>>>> 
>>>> [...]
>>>>>> + */
>>>>>> +__bpf_kfunc int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr_kern *digest_ptr)
>>>>>> +{
>>>>>> +     const struct inode *inode = file_inode(file);
>>>>>> +     struct fsverity_digest *arg = digest_ptr->data;
>>>>> 
>>>>> What alignment is guaranteed here?
>>>> 
>>>> drnptr doesn't not provide alignment guarantee for digest_ptr->data.
>>>> If we need alignment guarantee, we need to add it here.
>>> 
>>> So technically it's wrong to cast it to struct fsverity_digest, then.
>> 
>> We can enforce alignment here. Would __aligned(2) be sufficient?
>> 
> 
> Do you mean something like the following:
> 
> if (!IS_ALIGNED((uintptr_t)digest_ptr->data, __alignof__(*arg)))
> return -EINVAL;

I was thinking about hard-coding the alignment requirement. 
__alignof__ is much better. Thanks for the suggestion!

Song

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

[Andrii Nakryiko]

On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> wrote:
>
> This kfunc can be used to read xattr of a file.
>
> Since vfs_getxattr() requires null-terminated string as input "name", a new
> helper bpf_dynptr_is_string() is added to check the input before calling
> vfs_getxattr().
>
> Signed-off-by: Song Liu <song@kernel.org>
> ---
>  include/linux/bpf.h      | 12 +++++++++++
>  kernel/trace/bpf_trace.c | 44 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 61bde4520f5c..f14fae45e13d 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2472,6 +2472,13 @@ static inline bool has_current_bpf_ctx(void)
>         return !!current->bpf_ctx;
>  }
>
> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)

is_zero_terminated would be more accurate? though there is nothing
really dynptr-specific here...

> +{
> +       char *str = ptr->data;
> +
> +       return str[__bpf_dynptr_size(ptr) - 1] == '\0';
> +}
> +
>  void notrace bpf_prog_inc_misses_counter(struct bpf_prog *prog);
>
>  void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data,
> @@ -2708,6 +2715,11 @@ static inline bool has_current_bpf_ctx(void)
>         return false;
>  }
>
> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
> +{
> +       return false;
> +}
> +
>  static inline void bpf_prog_inc_misses_counter(struct bpf_prog *prog)
>  {
>  }
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index df697c74d519..946268574e05 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>
>
> @@ -1429,6 +1430,49 @@ static int __init bpf_key_sig_kfuncs_init(void)
>  late_initcall(bpf_key_sig_kfuncs_init);
>  #endif /* CONFIG_KEYS */
>
> +/* filesystem kfuncs */
> +__diag_push();
> +__diag_ignore_all("-Wmissing-prototypes",
> +                 "kfuncs which will be used in BPF programs");
> +
> +/**
> + * bpf_get_file_xattr - get xattr of a file
> + * @name_ptr: name of the xattr
> + * @value_ptr: output buffer of the xattr value
> + *
> + * Get xattr *name_ptr* of *file* and store the output in *value_ptr*.
> + *
> + * Return: 0 on success, a negative value on error.
> + */
> +__bpf_kfunc int bpf_get_file_xattr(struct file *file, struct bpf_dynptr_kern *name_ptr,
> +                                  struct bpf_dynptr_kern *value_ptr)
> +{
> +       if (!bpf_dynptr_is_string(name_ptr))
> +               return -EINVAL;

so dynptr can be invalid and name_ptr->data will be NULL, you should
account for that

and there could also be special dynptrs that don't have contiguous
memory region, so somehow you'd need to take care of that as well

> +
> +       return vfs_getxattr(mnt_idmap(file->f_path.mnt), file_dentry(file), name_ptr->data,
> +                           value_ptr->data, __bpf_dynptr_size(value_ptr));
> +}
> +
> +__diag_pop();
> +
> +BTF_SET8_START(fs_kfunc_set)
> +BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE)
> +BTF_SET8_END(fs_kfunc_set)
> +
> +const struct btf_kfunc_id_set bpf_fs_kfunc_set = {
> +       .owner = THIS_MODULE,
> +       .set = &fs_kfunc_set,
> +};
> +
> +static int __init bpf_fs_kfuncs_init(void)
> +{
> +       return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING,
> +                                        &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 bpf-next 5/5] selftests/bpf: Add test that use fsverity and xattr to sign a file

[Alexei Starovoitov]

On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> wrote:
>
> +#define MAGIC_SIZE 8
> +char digest[MAGIC_SIZE + sizeof(struct fsverity_digest) + SHA256_DIGEST_SIZE];
> +
> +__u32 monitored_pid;
> +char xattr_name[] = "user.sig";
> +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, name_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);
> +       bpf_dynptr_from_mem(xattr_name, sizeof(xattr_name), 0, &name_ptr);
> +       ret = bpf_get_file_xattr(f, &name_ptr, &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;
> +}

I think the UX is cumbersome.
Putting a NULL terminated string into dynptr not only a source
code ugliness, but it adds run-time overhead too.
We better add proper C style string support in the verifier,
so that bpf_get_file_xattr() can look like normal C.

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

[Alexei Starovoitov]

On Fri, Oct 13, 2023 at 11:30 AM Song Liu <song@kernel.org> wrote:
> +__bpf_kfunc int bpf_get_file_xattr(struct file *file, struct bpf_dynptr_kern *name_ptr,
> +                                  struct bpf_dynptr_kern *value_ptr)
> +{
> +       if (!bpf_dynptr_is_string(name_ptr))
> +               return -EINVAL;
> +
> +       return vfs_getxattr(mnt_idmap(file->f_path.mnt), file_dentry(file), name_ptr->data,
> +                           value_ptr->data, __bpf_dynptr_size(value_ptr));
> +}
> +
> +__diag_pop();
> +
> +BTF_SET8_START(fs_kfunc_set)
> +BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE)

I suspect it needs to be allowlisted too.
Sleepable might not be enough.

KP proposed such kfunc in the past and there were recursion issues.

KP,
do you remember the details?

Re: [PATCH bpf-next 2/5] bpf, fsverity: Add kfunc bpf_get_fsverity_digest

[Andrii Nakryiko]

On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> wrote:
>
> The kfunc can be used to read fsverity_digest, so that we can verify
> signature in BPF LSM.
>
> 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).
>
> Signed-off-by: Song Liu <song@kernel.org>
> ---
>  fs/verity/measure.c | 66 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>
> diff --git a/fs/verity/measure.c b/fs/verity/measure.c
> index eec5956141da..2d4b2e6f5a5d 100644
> --- a/fs/verity/measure.c
> +++ b/fs/verity/measure.c
> @@ -8,6 +8,8 @@
>  #include "fsverity_private.h"
>
>  #include <linux/uaccess.h>
> +#include <linux/bpf.h>
> +#include <linux/btf.h>
>
>  /**
>   * fsverity_ioctl_measure() - get a verity file's digest
> @@ -100,3 +102,67 @@ int fsverity_get_digest(struct inode *inode,
>         return hash_alg->digest_size;
>  }
>  EXPORT_SYMBOL_GPL(fsverity_get_digest);
> +
> +/* bpf kfuncs */
> +__diag_push();
> +__diag_ignore_all("-Wmissing-prototypes",
> +                 "kfuncs which will be used in BPF programs");
> +
> +/**
> + * 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);
> +       struct fsverity_digest *arg = digest_ptr->data;

this can be null

I think we need some internal helpers that are similar to
bpf_dynptr_slice() that would handle invalid dynptr cases, as well as
abstract away potentially non-contiguous memory dynptr points to.
WDYT?

> +       const struct fsverity_info *vi;
> +       const struct fsverity_hash_alg *hash_alg;
> +       int out_digest_sz;
> +
> +       if (__bpf_dynptr_size(digest_ptr) < sizeof(struct fsverity_digest))
> +               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 = __bpf_dynptr_size(digest_ptr) - 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 */
> +       memset(arg->digest + arg->digest_size, 0, out_digest_sz - hash_alg->digest_size);
> +
> +       return 0;
> +}
> +
> +__diag_pop();
> +
> +BTF_SET8_START(fsverity_set)
> +BTF_ID_FLAGS(func, bpf_get_fsverity_digest, KF_SLEEPABLE)
> +BTF_SET8_END(fsverity_set)
> +
> +const struct btf_kfunc_id_set bpf_fsverity_set = {
> +       .owner = THIS_MODULE,
> +       .set = &fsverity_set,
> +};
> +
> +static int __init bpf_fsverity_init(void)
> +{
> +       return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING,
> +                                        &bpf_fsverity_set);
> +}
> +
> +late_initcall(bpf_fsverity_init);
> --
> 2.34.1
>

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

[Song Liu]

> On Oct 17, 2023, at 11:58 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> wrote:
>> 
>> This kfunc can be used to read xattr of a file.
>> 
>> Since vfs_getxattr() requires null-terminated string as input "name", a new
>> helper bpf_dynptr_is_string() is added to check the input before calling
>> vfs_getxattr().
>> 
>> Signed-off-by: Song Liu <song@kernel.org>
>> ---
>> include/linux/bpf.h      | 12 +++++++++++
>> kernel/trace/bpf_trace.c | 44 ++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 56 insertions(+)
>> 
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 61bde4520f5c..f14fae45e13d 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -2472,6 +2472,13 @@ static inline bool has_current_bpf_ctx(void)
>>        return !!current->bpf_ctx;
>> }
>> 
>> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
> 
> is_zero_terminated would be more accurate? though there is nothing
> really dynptr-specific here...

is_zero_terminated sounds better. 

> 
>> +{
>> +       char *str = ptr->data;
>> +
>> +       return str[__bpf_dynptr_size(ptr) - 1] == '\0';
>> +}
>> +
>> void notrace bpf_prog_inc_misses_counter(struct bpf_prog *prog);
>> 
>> void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data,
>> @@ -2708,6 +2715,11 @@ static inline bool has_current_bpf_ctx(void)
>>        return false;
>> }
>> 
>> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
>> +{
>> +       return false;
>> +}
>> +
>> static inline void bpf_prog_inc_misses_counter(struct bpf_prog *prog)
>> {
>> }
>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>> index df697c74d519..946268574e05 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>
>> 
>> @@ -1429,6 +1430,49 @@ static int __init bpf_key_sig_kfuncs_init(void)
>> late_initcall(bpf_key_sig_kfuncs_init);
>> #endif /* CONFIG_KEYS */
>> 
>> +/* filesystem kfuncs */
>> +__diag_push();
>> +__diag_ignore_all("-Wmissing-prototypes",
>> +                 "kfuncs which will be used in BPF programs");
>> +
>> +/**
>> + * bpf_get_file_xattr - get xattr of a file
>> + * @name_ptr: name of the xattr
>> + * @value_ptr: output buffer of the xattr value
>> + *
>> + * Get xattr *name_ptr* of *file* and store the output in *value_ptr*.
>> + *
>> + * Return: 0 on success, a negative value on error.
>> + */
>> +__bpf_kfunc int bpf_get_file_xattr(struct file *file, struct bpf_dynptr_kern *name_ptr,
>> +                                  struct bpf_dynptr_kern *value_ptr)
>> +{
>> +       if (!bpf_dynptr_is_string(name_ptr))
>> +               return -EINVAL;
> 
> so dynptr can be invalid and name_ptr->data will be NULL, you should
> account for that

We can add a NULL check (or size check) here. 

> 
> and there could also be special dynptrs that don't have contiguous
> memory region, so somehow you'd need to take care of that as well

We can require the dynptr to be BPF_DYNPTR_TYPE_LOCAL. I don't think
we need this for dynptr of skb or xdp. Would this be sufficient?

Thanks,
Song

> 
>> +
>> +       return vfs_getxattr(mnt_idmap(file->f_path.mnt), file_dentry(file), name_ptr->data,
>> +                           value_ptr->data, __bpf_dynptr_size(value_ptr));
>> +}

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

[Song Liu]

> On Oct 17, 2023, at 12:08 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> wrote:
>> 
>> +#define MAGIC_SIZE 8
>> +char digest[MAGIC_SIZE + sizeof(struct fsverity_digest) + SHA256_DIGEST_SIZE];
>> +
>> +__u32 monitored_pid;
>> +char xattr_name[] = "user.sig";
>> +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, name_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);
>> +       bpf_dynptr_from_mem(xattr_name, sizeof(xattr_name), 0, &name_ptr);
>> +       ret = bpf_get_file_xattr(f, &name_ptr, &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;
>> +}
> 
> I think the UX is cumbersome.
> Putting a NULL terminated string into dynptr not only a source
> code ugliness, but it adds run-time overhead too.
> We better add proper C style string support in the verifier,
> so that bpf_get_file_xattr() can look like normal C.

That will indeed look much better. Let me check what do we need 
make this happen.

Thanks,
Song

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

[Andrii Nakryiko]

On Tue, Oct 17, 2023 at 1:31 PM Song Liu <songliubraving@meta.com> wrote:
>
>
>
> > On Oct 17, 2023, at 11:58 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> wrote:
> >>
> >> This kfunc can be used to read xattr of a file.
> >>
> >> Since vfs_getxattr() requires null-terminated string as input "name", a new
> >> helper bpf_dynptr_is_string() is added to check the input before calling
> >> vfs_getxattr().
> >>
> >> Signed-off-by: Song Liu <song@kernel.org>
> >> ---
> >> include/linux/bpf.h      | 12 +++++++++++
> >> kernel/trace/bpf_trace.c | 44 ++++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 56 insertions(+)
> >>
> >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> >> index 61bde4520f5c..f14fae45e13d 100644
> >> --- a/include/linux/bpf.h
> >> +++ b/include/linux/bpf.h
> >> @@ -2472,6 +2472,13 @@ static inline bool has_current_bpf_ctx(void)
> >>        return !!current->bpf_ctx;
> >> }
> >>
> >> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
> >
> > is_zero_terminated would be more accurate? though there is nothing
> > really dynptr-specific here...
>
> is_zero_terminated sounds better.
>
> >
> >> +{
> >> +       char *str = ptr->data;
> >> +
> >> +       return str[__bpf_dynptr_size(ptr) - 1] == '\0';
> >> +}
> >> +
> >> void notrace bpf_prog_inc_misses_counter(struct bpf_prog *prog);
> >>
> >> void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data,
> >> @@ -2708,6 +2715,11 @@ static inline bool has_current_bpf_ctx(void)
> >>        return false;
> >> }
> >>
> >> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
> >> +{
> >> +       return false;
> >> +}
> >> +
> >> static inline void bpf_prog_inc_misses_counter(struct bpf_prog *prog)
> >> {
> >> }
> >> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> >> index df697c74d519..946268574e05 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>
> >>
> >> @@ -1429,6 +1430,49 @@ static int __init bpf_key_sig_kfuncs_init(void)
> >> late_initcall(bpf_key_sig_kfuncs_init);
> >> #endif /* CONFIG_KEYS */
> >>
> >> +/* filesystem kfuncs */
> >> +__diag_push();
> >> +__diag_ignore_all("-Wmissing-prototypes",
> >> +                 "kfuncs which will be used in BPF programs");
> >> +
> >> +/**
> >> + * bpf_get_file_xattr - get xattr of a file
> >> + * @name_ptr: name of the xattr
> >> + * @value_ptr: output buffer of the xattr value
> >> + *
> >> + * Get xattr *name_ptr* of *file* and store the output in *value_ptr*.
> >> + *
> >> + * Return: 0 on success, a negative value on error.
> >> + */
> >> +__bpf_kfunc int bpf_get_file_xattr(struct file *file, struct bpf_dynptr_kern *name_ptr,
> >> +                                  struct bpf_dynptr_kern *value_ptr)
> >> +{
> >> +       if (!bpf_dynptr_is_string(name_ptr))
> >> +               return -EINVAL;
> >
> > so dynptr can be invalid and name_ptr->data will be NULL, you should
> > account for that
>
> We can add a NULL check (or size check) here.

there must be some helper to check if dynptr is valid, let's use that
instead of NULL checks

>
> >
> > and there could also be special dynptrs that don't have contiguous
> > memory region, so somehow you'd need to take care of that as well
>
> We can require the dynptr to be BPF_DYNPTR_TYPE_LOCAL. I don't think
> we need this for dynptr of skb or xdp. Would this be sufficient?

well, to keep thing simple we can have a simple internal helper API
that will tell if it's safe to assume that dynptr memory is contiguous
and it's ok to use dynptr memory. But still, you shouldn't access data
pointer directly, there must be some helper for that. Please check. It
has to take into account offset and stuff like that.


Also, and separately from that, we should think about providing a
bpf_dynptr_slice()-like helper that will accept a fixed-sized
temporary buffer and return pointer to either actual memory or copy
non-contiguous memory into that buffer. That will make sure you can
use any dynptr as a source of data, and only pay the price of memory
copy in rare cases where it's necessary

>
> Thanks,
> Song
>
> >
> >> +
> >> +       return vfs_getxattr(mnt_idmap(file->f_path.mnt), file_dentry(file), name_ptr->data,
> >> +                           value_ptr->data, __bpf_dynptr_size(value_ptr));
> >> +}
>

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

[Song Liu]

> On Oct 17, 2023, at 2:52 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> On Tue, Oct 17, 2023 at 1:31 PM Song Liu <songliubraving@meta.com> wrote:
>> 
>> 
>> 
>>> On Oct 17, 2023, at 11:58 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>>> 
>>> On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> wrote:
>>>> 
>>>> This kfunc can be used to read xattr of a file.
>>>> 
>>>> Since vfs_getxattr() requires null-terminated string as input "name", a new
>>>> helper bpf_dynptr_is_string() is added to check the input before calling
>>>> vfs_getxattr().
>>>> 
>>>> Signed-off-by: Song Liu <song@kernel.org>
>>>> ---
>>>> include/linux/bpf.h      | 12 +++++++++++
>>>> kernel/trace/bpf_trace.c | 44 ++++++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 56 insertions(+)
>>>> 
>>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>>> index 61bde4520f5c..f14fae45e13d 100644
>>>> --- a/include/linux/bpf.h
>>>> +++ b/include/linux/bpf.h
>>>> @@ -2472,6 +2472,13 @@ static inline bool has_current_bpf_ctx(void)
>>>>       return !!current->bpf_ctx;
>>>> }
>>>> 
>>>> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
>>> 
>>> is_zero_terminated would be more accurate? though there is nothing
>>> really dynptr-specific here...
>> 
>> is_zero_terminated sounds better.
>> 
>>> 
>>>> +{
>>>> +       char *str = ptr->data;
>>>> +
>>>> +       return str[__bpf_dynptr_size(ptr) - 1] == '\0';
>>>> +}
>>>> +
>>>> void notrace bpf_prog_inc_misses_counter(struct bpf_prog *prog);
>>>> 
>>>> void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data,
>>>> @@ -2708,6 +2715,11 @@ static inline bool has_current_bpf_ctx(void)
>>>>       return false;
>>>> }
>>>> 
>>>> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
>>>> +{
>>>> +       return false;
>>>> +}
>>>> +
>>>> static inline void bpf_prog_inc_misses_counter(struct bpf_prog *prog)
>>>> {
>>>> }
>>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>>>> index df697c74d519..946268574e05 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>
>>>> 
>>>> @@ -1429,6 +1430,49 @@ static int __init bpf_key_sig_kfuncs_init(void)
>>>> late_initcall(bpf_key_sig_kfuncs_init);
>>>> #endif /* CONFIG_KEYS */
>>>> 
>>>> +/* filesystem kfuncs */
>>>> +__diag_push();
>>>> +__diag_ignore_all("-Wmissing-prototypes",
>>>> +                 "kfuncs which will be used in BPF programs");
>>>> +
>>>> +/**
>>>> + * bpf_get_file_xattr - get xattr of a file
>>>> + * @name_ptr: name of the xattr
>>>> + * @value_ptr: output buffer of the xattr value
>>>> + *
>>>> + * Get xattr *name_ptr* of *file* and store the output in *value_ptr*.
>>>> + *
>>>> + * Return: 0 on success, a negative value on error.
>>>> + */
>>>> +__bpf_kfunc int bpf_get_file_xattr(struct file *file, struct bpf_dynptr_kern *name_ptr,
>>>> +                                  struct bpf_dynptr_kern *value_ptr)
>>>> +{
>>>> +       if (!bpf_dynptr_is_string(name_ptr))
>>>> +               return -EINVAL;
>>> 
>>> so dynptr can be invalid and name_ptr->data will be NULL, you should
>>> account for that
>> 
>> We can add a NULL check (or size check) here.
> 
> there must be some helper to check if dynptr is valid, let's use that
> instead of NULL checks

Yeah, we can use bpf_dynptr_is_null(). 

> 
>> 
>>> 
>>> and there could also be special dynptrs that don't have contiguous
>>> memory region, so somehow you'd need to take care of that as well
>> 
>> We can require the dynptr to be BPF_DYNPTR_TYPE_LOCAL. I don't think
>> we need this for dynptr of skb or xdp. Would this be sufficient?
> 
> well, to keep thing simple we can have a simple internal helper API
> that will tell if it's safe to assume that dynptr memory is contiguous
> and it's ok to use dynptr memory. But still, you shouldn't access data
> pointer directly, there must be some helper for that. Please check. It
> has to take into account offset and stuff like that.

Yeah, we can use bpf_dynptr_write(), which is a helper (not kfunc). 

> 
> Also, and separately from that, we should think about providing a
> bpf_dynptr_slice()-like helper that will accept a fixed-sized
> temporary buffer and return pointer to either actual memory or copy
> non-contiguous memory into that buffer. That will make sure you can
> use any dynptr as a source of data, and only pay the price of memory
> copy in rare cases where it's necessary

I don't quite follow here. Currently, we have 

bpf_dynptr_data()
bpf_dynptr_slice()
bpf_dynptr_slice_rdwr()
bpf_dynptr_write()

AFAICT, they are sufficient to cover existing use cases (and the new 
use case we are adding in this set). What's the new kfunc are you
thinking about?

Thanks,
Song

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

[Andrii Nakryiko]

On Tue, Oct 17, 2023 at 3:16 PM Song Liu <songliubraving@meta.com> wrote:
>
>
>
> > On Oct 17, 2023, at 2:52 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Oct 17, 2023 at 1:31 PM Song Liu <songliubraving@meta.com> wrote:
> >>
> >>
> >>
> >>> On Oct 17, 2023, at 11:58 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >>>
> >>> On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> wrote:
> >>>>
> >>>> This kfunc can be used to read xattr of a file.
> >>>>
> >>>> Since vfs_getxattr() requires null-terminated string as input "name", a new
> >>>> helper bpf_dynptr_is_string() is added to check the input before calling
> >>>> vfs_getxattr().
> >>>>
> >>>> Signed-off-by: Song Liu <song@kernel.org>
> >>>> ---
> >>>> include/linux/bpf.h      | 12 +++++++++++
> >>>> kernel/trace/bpf_trace.c | 44 ++++++++++++++++++++++++++++++++++++++++
> >>>> 2 files changed, 56 insertions(+)
> >>>>
> >>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> >>>> index 61bde4520f5c..f14fae45e13d 100644
> >>>> --- a/include/linux/bpf.h
> >>>> +++ b/include/linux/bpf.h
> >>>> @@ -2472,6 +2472,13 @@ static inline bool has_current_bpf_ctx(void)
> >>>>       return !!current->bpf_ctx;
> >>>> }
> >>>>
> >>>> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
> >>>
> >>> is_zero_terminated would be more accurate? though there is nothing
> >>> really dynptr-specific here...
> >>
> >> is_zero_terminated sounds better.
> >>
> >>>
> >>>> +{
> >>>> +       char *str = ptr->data;
> >>>> +
> >>>> +       return str[__bpf_dynptr_size(ptr) - 1] == '\0';
> >>>> +}
> >>>> +
> >>>> void notrace bpf_prog_inc_misses_counter(struct bpf_prog *prog);
> >>>>
> >>>> void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data,
> >>>> @@ -2708,6 +2715,11 @@ static inline bool has_current_bpf_ctx(void)
> >>>>       return false;
> >>>> }
> >>>>
> >>>> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
> >>>> +{
> >>>> +       return false;
> >>>> +}
> >>>> +
> >>>> static inline void bpf_prog_inc_misses_counter(struct bpf_prog *prog)
> >>>> {
> >>>> }
> >>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> >>>> index df697c74d519..946268574e05 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>
> >>>>
> >>>> @@ -1429,6 +1430,49 @@ static int __init bpf_key_sig_kfuncs_init(void)
> >>>> late_initcall(bpf_key_sig_kfuncs_init);
> >>>> #endif /* CONFIG_KEYS */
> >>>>
> >>>> +/* filesystem kfuncs */
> >>>> +__diag_push();
> >>>> +__diag_ignore_all("-Wmissing-prototypes",
> >>>> +                 "kfuncs which will be used in BPF programs");
> >>>> +
> >>>> +/**
> >>>> + * bpf_get_file_xattr - get xattr of a file
> >>>> + * @name_ptr: name of the xattr
> >>>> + * @value_ptr: output buffer of the xattr value
> >>>> + *
> >>>> + * Get xattr *name_ptr* of *file* and store the output in *value_ptr*.
> >>>> + *
> >>>> + * Return: 0 on success, a negative value on error.
> >>>> + */
> >>>> +__bpf_kfunc int bpf_get_file_xattr(struct file *file, struct bpf_dynptr_kern *name_ptr,
> >>>> +                                  struct bpf_dynptr_kern *value_ptr)
> >>>> +{
> >>>> +       if (!bpf_dynptr_is_string(name_ptr))
> >>>> +               return -EINVAL;
> >>>
> >>> so dynptr can be invalid and name_ptr->data will be NULL, you should
> >>> account for that
> >>
> >> We can add a NULL check (or size check) here.
> >
> > there must be some helper to check if dynptr is valid, let's use that
> > instead of NULL checks
>
> Yeah, we can use bpf_dynptr_is_null().
>
> >
> >>
> >>>
> >>> and there could also be special dynptrs that don't have contiguous
> >>> memory region, so somehow you'd need to take care of that as well
> >>
> >> We can require the dynptr to be BPF_DYNPTR_TYPE_LOCAL. I don't think
> >> we need this for dynptr of skb or xdp. Would this be sufficient?
> >
> > well, to keep thing simple we can have a simple internal helper API
> > that will tell if it's safe to assume that dynptr memory is contiguous
> > and it's ok to use dynptr memory. But still, you shouldn't access data
> > pointer directly, there must be some helper for that. Please check. It
> > has to take into account offset and stuff like that.
>
> Yeah, we can use bpf_dynptr_write(), which is a helper (not kfunc).
>
> >
> > Also, and separately from that, we should think about providing a
> > bpf_dynptr_slice()-like helper that will accept a fixed-sized
> > temporary buffer and return pointer to either actual memory or copy
> > non-contiguous memory into that buffer. That will make sure you can
> > use any dynptr as a source of data, and only pay the price of memory
> > copy in rare cases where it's necessary
>
> I don't quite follow here. Currently, we have
>
> bpf_dynptr_data()
> bpf_dynptr_slice()
> bpf_dynptr_slice_rdwr()
> bpf_dynptr_write()
>
> AFAICT, they are sufficient to cover existing use cases (and the new
> use case we are adding in this set). What's the new kfunc are you
> thinking about?

I wasn't talking about kfuncs, but rather just internal helpers to be
used by other kfuncs when working with dynptrs as input arguments.

>
> Thanks,
> Song
>
>

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

[Song Liu]

> On Oct 17, 2023, at 3:40 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> On Tue, Oct 17, 2023 at 3:16 PM Song Liu <songliubraving@meta.com> wrote:
>> 
>> 
>> 
>>> On Oct 17, 2023, at 2:52 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>>> 
>>> On Tue, Oct 17, 2023 at 1:31 PM Song Liu <songliubraving@meta.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Oct 17, 2023, at 11:58 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>>>>> 
>>>>> On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> wrote:
>>>>>> 
>>>>>> This kfunc can be used to read xattr of a file.
>>>>>> 
>>>>>> Since vfs_getxattr() requires null-terminated string as input "name", a new
>>>>>> helper bpf_dynptr_is_string() is added to check the input before calling
>>>>>> vfs_getxattr().
>>>>>> 
>>>>>> Signed-off-by: Song Liu <song@kernel.org>
>>>>>> ---
>>>>>> include/linux/bpf.h      | 12 +++++++++++
>>>>>> kernel/trace/bpf_trace.c | 44 ++++++++++++++++++++++++++++++++++++++++
>>>>>> 2 files changed, 56 insertions(+)
>>>>>> 
>>>>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>>>>> index 61bde4520f5c..f14fae45e13d 100644
>>>>>> --- a/include/linux/bpf.h
>>>>>> +++ b/include/linux/bpf.h
>>>>>> @@ -2472,6 +2472,13 @@ static inline bool has_current_bpf_ctx(void)
>>>>>>      return !!current->bpf_ctx;
>>>>>> }
>>>>>> 
>>>>>> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
>>>>> 
>>>>> is_zero_terminated would be more accurate? though there is nothing
>>>>> really dynptr-specific here...
>>>> 
>>>> is_zero_terminated sounds better.
>>>> 
>>>>> 
>>>>>> +{
>>>>>> +       char *str = ptr->data;
>>>>>> +
>>>>>> +       return str[__bpf_dynptr_size(ptr) - 1] == '\0';
>>>>>> +}
>>>>>> +
>>>>>> void notrace bpf_prog_inc_misses_counter(struct bpf_prog *prog);
>>>>>> 
>>>>>> void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data,
>>>>>> @@ -2708,6 +2715,11 @@ static inline bool has_current_bpf_ctx(void)
>>>>>>      return false;
>>>>>> }
>>>>>> 
>>>>>> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
>>>>>> +{
>>>>>> +       return false;
>>>>>> +}
>>>>>> +
>>>>>> static inline void bpf_prog_inc_misses_counter(struct bpf_prog *prog)
>>>>>> {
>>>>>> }
>>>>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>>>>>> index df697c74d519..946268574e05 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>
>>>>>> 
>>>>>> @@ -1429,6 +1430,49 @@ static int __init bpf_key_sig_kfuncs_init(void)
>>>>>> late_initcall(bpf_key_sig_kfuncs_init);
>>>>>> #endif /* CONFIG_KEYS */
>>>>>> 
>>>>>> +/* filesystem kfuncs */
>>>>>> +__diag_push();
>>>>>> +__diag_ignore_all("-Wmissing-prototypes",
>>>>>> +                 "kfuncs which will be used in BPF programs");
>>>>>> +
>>>>>> +/**
>>>>>> + * bpf_get_file_xattr - get xattr of a file
>>>>>> + * @name_ptr: name of the xattr
>>>>>> + * @value_ptr: output buffer of the xattr value
>>>>>> + *
>>>>>> + * Get xattr *name_ptr* of *file* and store the output in *value_ptr*.
>>>>>> + *
>>>>>> + * Return: 0 on success, a negative value on error.
>>>>>> + */
>>>>>> +__bpf_kfunc int bpf_get_file_xattr(struct file *file, struct bpf_dynptr_kern *name_ptr,
>>>>>> +                                  struct bpf_dynptr_kern *value_ptr)
>>>>>> +{
>>>>>> +       if (!bpf_dynptr_is_string(name_ptr))
>>>>>> +               return -EINVAL;
>>>>> 
>>>>> so dynptr can be invalid and name_ptr->data will be NULL, you should
>>>>> account for that
>>>> 
>>>> We can add a NULL check (or size check) here.
>>> 
>>> there must be some helper to check if dynptr is valid, let's use that
>>> instead of NULL checks
>> 
>> Yeah, we can use bpf_dynptr_is_null().
>> 
>>> 
>>>> 
>>>>> 
>>>>> and there could also be special dynptrs that don't have contiguous
>>>>> memory region, so somehow you'd need to take care of that as well
>>>> 
>>>> We can require the dynptr to be BPF_DYNPTR_TYPE_LOCAL. I don't think
>>>> we need this for dynptr of skb or xdp. Would this be sufficient?
>>> 
>>> well, to keep thing simple we can have a simple internal helper API
>>> that will tell if it's safe to assume that dynptr memory is contiguous
>>> and it's ok to use dynptr memory. But still, you shouldn't access data
>>> pointer directly, there must be some helper for that. Please check. It
>>> has to take into account offset and stuff like that.
>> 
>> Yeah, we can use bpf_dynptr_write(), which is a helper (not kfunc).
>> 
>>> 
>>> Also, and separately from that, we should think about providing a
>>> bpf_dynptr_slice()-like helper that will accept a fixed-sized
>>> temporary buffer and return pointer to either actual memory or copy
>>> non-contiguous memory into that buffer. That will make sure you can
>>> use any dynptr as a source of data, and only pay the price of memory
>>> copy in rare cases where it's necessary
>> 
>> I don't quite follow here. Currently, we have
>> 
>> bpf_dynptr_data()
>> bpf_dynptr_slice()
>> bpf_dynptr_slice_rdwr()
>> bpf_dynptr_write()
>> 
>> AFAICT, they are sufficient to cover existing use cases (and the new
>> use case we are adding in this set). What's the new kfunc are you
>> thinking about?
> 
> I wasn't talking about kfuncs, but rather just internal helpers to be
> used by other kfuncs when working with dynptrs as input arguments.

AFAICT, kfuncs can call other kfuncs, for example bpf_dynptr_slice_rdwr()
calls bpf_dynptr_slice(). This is limited to the same file at the moment, 
since we do not expose kfuncs in headers. 


Thanks,
Song

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

[Hou Tao]

Hi,

On 10/18/2023 4:31 AM, Song Liu wrote:
>
>> On Oct 17, 2023, at 11:58 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>>
>> On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> wrote:
>>> This kfunc can be used to read xattr of a file.
>>>
>>> Since vfs_getxattr() requires null-terminated string as input "name", a new
>>> helper bpf_dynptr_is_string() is added to check the input before calling
>>> vfs_getxattr().
>>>
>>> Signed-off-by: Song Liu <song@kernel.org>
>>> ---
>>> include/linux/bpf.h      | 12 +++++++++++
>>> kernel/trace/bpf_trace.c | 44 ++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 56 insertions(+)
>>>
>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>> index 61bde4520f5c..f14fae45e13d 100644
>>> --- a/include/linux/bpf.h
>>> +++ b/include/linux/bpf.h
>>> @@ -2472,6 +2472,13 @@ static inline bool has_current_bpf_ctx(void)
>>>        return !!current->bpf_ctx;
>>> }
>>>
>>> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
>> is_zero_terminated would be more accurate? though there is nothing
>> really dynptr-specific here...
> is_zero_terminated sounds better. 
>
>>> +{
>>> +       char *str = ptr->data;
>>> +
>>> +       return str[__bpf_dynptr_size(ptr) - 1] == '\0';
>>> +}
>>> +
>>> void notrace bpf_prog_inc_misses_counter(struct bpf_prog *prog);
>>>
>>> void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data,
>>> @@ -2708,6 +2715,11 @@ static inline bool has_current_bpf_ctx(void)
>>>        return false;
>>> }
>>>
>>> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
>>> +{
>>> +       return false;
>>> +}
>>> +
>>> static inline void bpf_prog_inc_misses_counter(struct bpf_prog *prog)
>>> {
>>> }
>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>>> index df697c74d519..946268574e05 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>
>>>
>>> @@ -1429,6 +1430,49 @@ static int __init bpf_key_sig_kfuncs_init(void)
>>> late_initcall(bpf_key_sig_kfuncs_init);
>>> #endif /* CONFIG_KEYS */
>>>
>>> +/* filesystem kfuncs */
>>> +__diag_push();
>>> +__diag_ignore_all("-Wmissing-prototypes",
>>> +                 "kfuncs which will be used in BPF programs");
>>> +
>>> +/**
>>> + * bpf_get_file_xattr - get xattr of a file
>>> + * @name_ptr: name of the xattr
>>> + * @value_ptr: output buffer of the xattr value
>>> + *
>>> + * Get xattr *name_ptr* of *file* and store the output in *value_ptr*.
>>> + *
>>> + * Return: 0 on success, a negative value on error.
>>> + */
>>> +__bpf_kfunc int bpf_get_file_xattr(struct file *file, struct bpf_dynptr_kern *name_ptr,
>>> +                                  struct bpf_dynptr_kern *value_ptr)
>>> +{
>>> +       if (!bpf_dynptr_is_string(name_ptr))
>>> +               return -EINVAL;
>> so dynptr can be invalid and name_ptr->data will be NULL, you should
>> account for that
> We can add a NULL check (or size check) here. 
>
>> and there could also be special dynptrs that don't have contiguous
>> memory region, so somehow you'd need to take care of that as well
> We can require the dynptr to be BPF_DYNPTR_TYPE_LOCAL. I don't think
> we need this for dynptr of skb or xdp. Would this be sufficient?

I think bpf_dynptr_is_rdonly() is also needed. Because the content of
dynptr may be modified by other bpf program and the zero-terminated
condition will not true. As suggested by Alexei, add string support in
verifier is a better choice.
>
> Thanks,
> Song
>
>>> +
>>> +       return vfs_getxattr(mnt_idmap(file->f_path.mnt), file_dentry(file), name_ptr->data,
>>> +                           value_ptr->data, __bpf_dynptr_size(value_ptr));
>>> +}

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

[KP Singh]

On Tue, Oct 17, 2023 at 9:11 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Oct 13, 2023 at 11:30 AM Song Liu <song@kernel.org> wrote:
> > +__bpf_kfunc int bpf_get_file_xattr(struct file *file, struct bpf_dynptr_kern *name_ptr,
> > +                                  struct bpf_dynptr_kern *value_ptr)
> > +{
> > +       if (!bpf_dynptr_is_string(name_ptr))
> > +               return -EINVAL;
> > +
> > +       return vfs_getxattr(mnt_idmap(file->f_path.mnt), file_dentry(file), name_ptr->data,
> > +                           value_ptr->data, __bpf_dynptr_size(value_ptr));
> > +}
> > +
> > +__diag_pop();
> > +
> > +BTF_SET8_START(fs_kfunc_set)
> > +BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE)
>
> I suspect it needs to be allowlisted too.
> Sleepable might not be enough.
>
> KP proposed such kfunc in the past and there were recursion issues.
>
> KP,
> do you remember the details?

yeah, have a look at Al's reply:

https://lore.kernel.org/bpf/Yrs4+ThR4ACb5eD%2F@ZenIV/

it can create deadlocks and potentially UAFs (similar to the situation
Jann mentioned). This will need to be allowlisted.