[PATCH v2 0/3] bpf: Add bpf_verify_pkcs7_signature() helper

[PATCH v2 0/3] bpf: Add bpf_verify_pkcs7_signature() helper

[Roberto Sassu]

One of the desirable features in security is the ability to restrict import
of data to a given system based on data authenticity. If data import can be
restricted, it would be possible to enforce a system-wide policy based on
the signing keys the system owner trusts.

This feature is widely used in the kernel. For example, if the restriction
is enabled, kernel modules can be plugged in only if they are signed with a
key whose public part is in the primary or secondary keyring.

For eBPF, it can be useful as well. For example, it might be useful to
authenticate data an eBPF program makes security decisions on.

After a discussion in the eBPF mailing list, it was decided that the stated
goal should be accomplished by introducing a new helper:
bpf_verify_pkcs7_signature(). It is simply a wrapper of
verify_pkcs7_signature(), and does the signature verification with a key in
the selected keyring (primary, secondary or platform).

Since verify_pkcs7_signature() is doing crypto operations, it must be
called by a sleepable program. This restricts the set of functions that can
call the associated helper (for example, lsm.s/bpf is suitable,
fexit/array_map_update_elem is not).

The added test check the ability of an eBPF program to verify module-style
appended signatures, as produced by the kernel tool sign-file, currently
used to sign kernel modules.

The patch set is organized as follows.

Patch 1 introduces the new helper. Patch 2 adds two new options to
test_progs (the eBPF selftest binary), to specify the path of sign-file and
the file containing the kernel private key and certificate. Finally,
patch 3 adds the test for the new helper.

Roberto Sassu (3):
  bpf: Add bpf_verify_pkcs7_signature() helper
  selftests/bpf: Add test_progs opts for sign-file and kernel priv key +
    cert
  selftests/bpf: Add test for bpf_verify_pkcs7_signature() helper

 include/uapi/linux/bpf.h                      |   8 +
 kernel/bpf/bpf_lsm.c                          |  32 ++++
 tools/include/uapi/linux/bpf.h                |   8 +
 tools/testing/selftests/bpf/config            |   2 +
 .../bpf/prog_tests/verify_pkcs7_sig.c         | 149 ++++++++++++++++++
 .../bpf/progs/test_verify_pkcs7_sig.c         | 127 +++++++++++++++
 tools/testing/selftests/bpf/test_progs.c      |  12 ++
 tools/testing/selftests/bpf/test_progs.h      |   3 +
 8 files changed, 341 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c

-- 
2.25.1

[PATCH v2 1/3] bpf: Add bpf_verify_pkcs7_signature() helper

[Roberto Sassu]

Add the bpf_verify_pkcs7_signature() helper, to give the ability to eBPF
security modules to check the validity of a PKCS#7 signature against
supplied data.

Use the 'keyring' parameter to select the keyring containing the
verification key: 0 for the primary keyring, 1 for the primary and
secondary keyrings, 2 for the platform keyring.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/uapi/linux/bpf.h       |  8 ++++++++
 kernel/bpf/bpf_lsm.c           | 32 ++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h |  8 ++++++++
 3 files changed, 48 insertions(+)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index f4009dbdf62d..40d0fc0d9493 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -5249,6 +5249,13 @@ union bpf_attr {
  *		Pointer to the underlying dynptr data, NULL if the dynptr is
  *		read-only, if the dynptr is invalid, or if the offset and length
  *		is out of bounds.
+ *
+ * long bpf_verify_pkcs7_signature(u8 *data, u32 datalen, u8 *sig, u32 siglen, u64 keyring)
+ *	Description
+ *		Verify the PKCS#7 *sig* with length *siglen*, on *data* with
+ *		length *datalen*, with key in *keyring*.
+ *	Return
+ *		0 on success, a negative value on error.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5455,6 +5462,7 @@ union bpf_attr {
 	FN(dynptr_read),		\
 	FN(dynptr_write),		\
 	FN(dynptr_data),		\
+	FN(verify_pkcs7_signature),	\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index c1351df9f7ee..1cda43cb541a 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -16,6 +16,7 @@
 #include <linux/bpf_local_storage.h>
 #include <linux/btf_ids.h>
 #include <linux/ima.h>
+#include <linux/verification.h>
 
 /* For every LSM hook that allows attachment of BPF programs, declare a nop
  * function where a BPF program can be attached.
@@ -132,6 +133,35 @@ static const struct bpf_func_proto bpf_get_attach_cookie_proto = {
 	.arg1_type	= ARG_PTR_TO_CTX,
 };
 
+BPF_CALL_5(bpf_verify_pkcs7_signature, u8 *, data, u32, datalen, u8 *, sig,
+	   u32, siglen, u64, keyring)
+{
+	int ret = -EOPNOTSUPP;
+
+#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
+	if (keyring > (unsigned long)VERIFY_USE_PLATFORM_KEYRING)
+		return -EINVAL;
+
+	ret = verify_pkcs7_signature(data, datalen, sig, siglen,
+				     (struct key *)keyring,
+				     VERIFYING_UNSPECIFIED_SIGNATURE, NULL,
+				     NULL);
+#endif
+	return ret;
+}
+
+static const struct bpf_func_proto bpf_verify_pkcs7_signature_proto = {
+	.func		= bpf_verify_pkcs7_signature,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_MEM,
+	.arg2_type	= ARG_CONST_SIZE_OR_ZERO,
+	.arg3_type	= ARG_PTR_TO_MEM,
+	.arg4_type	= ARG_CONST_SIZE_OR_ZERO,
+	.arg5_type	= ARG_ANYTHING,
+	.allowed	= bpf_ima_inode_hash_allowed,
+};
+
 static const struct bpf_func_proto *
 bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
@@ -158,6 +188,8 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return prog->aux->sleepable ? &bpf_ima_file_hash_proto : NULL;
 	case BPF_FUNC_get_attach_cookie:
 		return bpf_prog_has_trampoline(prog) ? &bpf_get_attach_cookie_proto : NULL;
+	case BPF_FUNC_verify_pkcs7_signature:
+		return prog->aux->sleepable ? &bpf_verify_pkcs7_signature_proto : NULL;
 	default:
 		return tracing_prog_func_proto(func_id, prog);
 	}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index f4009dbdf62d..40d0fc0d9493 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -5249,6 +5249,13 @@ union bpf_attr {
  *		Pointer to the underlying dynptr data, NULL if the dynptr is
  *		read-only, if the dynptr is invalid, or if the offset and length
  *		is out of bounds.
+ *
+ * long bpf_verify_pkcs7_signature(u8 *data, u32 datalen, u8 *sig, u32 siglen, u64 keyring)
+ *	Description
+ *		Verify the PKCS#7 *sig* with length *siglen*, on *data* with
+ *		length *datalen*, with key in *keyring*.
+ *	Return
+ *		0 on success, a negative value on error.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5455,6 +5462,7 @@ union bpf_attr {
 	FN(dynptr_read),		\
 	FN(dynptr_write),		\
 	FN(dynptr_data),		\
+	FN(verify_pkcs7_signature),	\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
-- 
2.25.1

[PATCH v2 2/3] selftests/bpf: Add test_progs opts for sign-file and kernel priv key + cert

[Roberto Sassu]

According to the logs of the eBPF CI, built kernel and tests are copied to
a virtual machine to run there.

Since a test for a new helper to verify PKCS#7 signatures requires to sign
data to be verified, extend test_progs to store in the test_env data
structure (accessible by individual tests) the path of sign-file and of the
kernel private key and cert.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 tools/testing/selftests/bpf/test_progs.c | 12 ++++++++++++
 tools/testing/selftests/bpf/test_progs.h |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index c639f2e56fc5..90ce2c06a15e 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -707,6 +707,8 @@ enum ARG_KEYS {
 	ARG_TEST_NAME_GLOB_DENYLIST = 'd',
 	ARG_NUM_WORKERS = 'j',
 	ARG_DEBUG = -1,
+	ARG_SIGN_FILE = 'S',
+	ARG_KERNEL_PRIV_CERT = 'C',
 };
 
 static const struct argp_option opts[] = {
@@ -732,6 +734,10 @@ static const struct argp_option opts[] = {
 	  "Number of workers to run in parallel, default to number of cpus." },
 	{ "debug", ARG_DEBUG, NULL, 0,
 	  "print extra debug information for test_progs." },
+	{ "sign-file", ARG_SIGN_FILE, "PATH", 0,
+	  "sign-file path " },
+	{ "kernel-priv-cert", ARG_KERNEL_PRIV_CERT, "PATH", 0,
+	  "kernel private key and cert path " },
 	{},
 };
 
@@ -862,6 +868,12 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
 	case ARG_DEBUG:
 		env->debug = true;
 		break;
+	case ARG_SIGN_FILE:
+		env->sign_file_path = arg;
+		break;
+	case ARG_KERNEL_PRIV_CERT:
+		env->kernel_priv_cert_path = arg;
+		break;
 	case ARGP_KEY_ARG:
 		argp_usage(state);
 		break;
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 5fe1365c2bb1..9a860a59f06a 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -123,6 +123,9 @@ struct test_env {
 	pid_t *worker_pids; /* array of worker pids */
 	int *worker_socks; /* array of worker socks */
 	int *worker_current_test; /* array of current running test for each worker */
+
+	const char *sign_file_path; /* sign-file path */
+	const char *kernel_priv_cert_path; /* kernel priv key and cert path */
 };
 
 #define MAX_LOG_TRUNK_SIZE 8192
-- 
2.25.1

[PATCH v2 3/3] selftests/bpf: Add test for bpf_verify_pkcs7_signature() helper

[Roberto Sassu]

Ensure that signature verification is performed successfully from an eBPF
program, with the new bpf_verify_pkcs7_signature() helper.

The test requires access to the kernel modules signing key and the
execution of the sign-file tool with the signing key path passed as
argument.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 tools/testing/selftests/bpf/config            |   2 +
 .../bpf/prog_tests/verify_pkcs7_sig.c         | 149 ++++++++++++++++++
 .../bpf/progs/test_verify_pkcs7_sig.c         | 127 +++++++++++++++
 3 files changed, 278 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c

diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
index 3b3edc0fc8a6..43f92ce5f3f3 100644
--- a/tools/testing/selftests/bpf/config
+++ b/tools/testing/selftests/bpf/config
@@ -57,3 +57,5 @@ CONFIG_FPROBE=y
 CONFIG_IKCONFIG=y
 CONFIG_IKCONFIG_PROC=y
 CONFIG_MPTCP=y
+CONFIG_MODULE_SIG_FORMAT=y
+CONFIG_SECONDARY_TRUSTED_KEYRING=y
diff --git a/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
new file mode 100644
index 000000000000..3c85b8cd13d4
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH
+ *
+ * Author: Roberto Sassu <roberto.sassu@huawei.com>
+ */
+
+#include <stdio.h>
+#include <errno.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <endian.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+#include <test_progs.h>
+
+#include "test_verify_pkcs7_sig.skel.h"
+
+#define MAX_DATA_SIZE 4096
+
+struct data {
+	u8 payload[MAX_DATA_SIZE];
+};
+
+static int populate_data_item(struct data *data_item)
+{
+	struct stat st;
+	char signed_file_template[] = "/tmp/signed_fileXXXXXX";
+	int ret, fd, child_status, child_pid;
+
+	fd = mkstemp(signed_file_template);
+	if (fd == -1)
+		return -errno;
+
+	ret = write(fd, "test", 4);
+
+	close(fd);
+
+	if (ret != 4) {
+		ret = -EIO;
+		goto out;
+	}
+
+	child_pid = fork();
+
+	if (child_pid == -1) {
+		ret = -errno;
+		goto out;
+	}
+
+	if (child_pid == 0)
+		return execlp(env.sign_file_path, env.sign_file_path, "sha256",
+			      env.kernel_priv_cert_path,
+			      env.kernel_priv_cert_path,
+			      signed_file_template, NULL);
+
+	waitpid(child_pid, &child_status, 0);
+
+	ret = WEXITSTATUS(child_status);
+	if (ret)
+		goto out;
+
+	ret = stat(signed_file_template, &st);
+	if (ret == -1) {
+		ret = -errno;
+		goto out;
+	}
+
+	if (st.st_size > sizeof(data_item->payload) - sizeof(u32)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	*(u32 *)data_item->payload = __cpu_to_be32(st.st_size);
+
+	fd = open(signed_file_template, O_RDONLY);
+	if (fd == -1) {
+		ret = -errno;
+		goto out;
+	}
+
+	ret = read(fd, data_item->payload + sizeof(u32), st.st_size);
+
+	close(fd);
+
+	if (ret != st.st_size) {
+		ret = -EIO;
+		goto out;
+	}
+
+	ret = 0;
+out:
+	unlink(signed_file_template);
+	return ret;
+}
+
+void test_verify_pkcs7_sig(void)
+{
+	struct test_verify_pkcs7_sig *skel = NULL;
+	struct bpf_map *map;
+	struct data data;
+	u32 saved_len;
+	int ret, zero = 0;
+
+	if (!env.sign_file_path || !env.kernel_priv_cert_path) {
+		printf(
+		  "%s:SKIP:sign-file and kernel priv key cert paths missing\n",
+		  __func__);
+		test__skip();
+		return;
+	}
+
+	skel = test_verify_pkcs7_sig__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "test_verify_pkcs7_sig__open_and_load"))
+		goto close_prog;
+
+	ret = test_verify_pkcs7_sig__attach(skel);
+	if (!ASSERT_OK(ret, "test_verify_pkcs7_sig__attach\n"))
+		goto close_prog;
+
+	map = bpf_object__find_map_by_name(skel->obj, "data_input");
+	if (!ASSERT_OK_PTR(map, "data_input not found"))
+		goto close_prog;
+
+	ret = populate_data_item(&data);
+	if (!ASSERT_OK(ret, "populate_data_item\n"))
+		goto close_prog;
+
+	skel->bss->monitored_pid = getpid();
+
+	ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &data, BPF_ANY);
+	if (!ASSERT_OK(ret, "bpf_map_update_elem\n"))
+		goto close_prog;
+
+	saved_len = *(__u32 *)data.payload;
+	*(__u32 *)data.payload = sizeof(data.payload);
+	ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &data, BPF_ANY);
+	if (!ASSERT_LT(ret, 0, "bpf_map_update_elem data_input\n"))
+		goto close_prog;
+
+	*(__u32 *)data.payload = saved_len;
+	data.payload[sizeof(__u32)] = 'a';
+	ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &data, BPF_ANY);
+	ASSERT_LT(ret, 0, "bpf_map_update_elem data_input\n");
+close_prog:
+	skel->bss->monitored_pid = 0;
+	test_verify_pkcs7_sig__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
new file mode 100644
index 000000000000..e72bcb7fb7a9
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH
+ *
+ * Author: Roberto Sassu <roberto.sassu@huawei.com>
+ */
+
+#include "vmlinux.h"
+#include <errno.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_endian.h>
+
+#define MAX_DATA_SIZE 4096
+
+#ifdef __BIG_ENDIAN__
+#define be32_to_cpu(x) (x)
+#else
+#define be32_to_cpu(x) ___bpf_swab32(x)
+#endif
+
+#define VERIFY_USE_SECONDARY_KEYRING (1UL)
+
+/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
+#define MODULE_SIG_STRING "~Module signature appended~\n"
+
+u32 monitored_pid;
+
+struct data {
+	u8 payload[MAX_DATA_SIZE];
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, __u32);
+	__type(value, struct data);
+} data_input SEC(".maps");
+
+char _license[] SEC("license") = "GPL";
+
+static int mod_check_sig(const struct module_signature *ms, size_t file_len)
+{
+	if (!ms)
+		return -ENOENT;
+
+	if (be32_to_cpu(ms->sig_len) >= file_len - sizeof(*ms))
+		return -EBADMSG;
+
+	if (ms->id_type != PKEY_ID_PKCS7)
+		return -ENOPKG;
+
+	if (ms->algo != 0 ||
+	    ms->hash != 0 ||
+	    ms->signer_len != 0 ||
+	    ms->key_id_len != 0 ||
+	    ms->__pad[0] != 0 ||
+	    ms->__pad[1] != 0 ||
+	    ms->__pad[2] != 0)
+		return -EBADMSG;
+
+	return 0;
+}
+
+SEC("lsm.s/bpf")
+int BPF_PROG(bpf, int cmd, union bpf_attr *attr, unsigned int size)
+{
+	const size_t marker_len = sizeof(MODULE_SIG_STRING) - 1;
+	char marker[sizeof(MODULE_SIG_STRING) - 1];
+	struct module_signature ms;
+	struct data *data_ptr;
+	u32 modlen;
+	u32 sig_len;
+	u64 value;
+	u8 *mod;
+	u32 pid;
+	int ret, zero = 0;
+
+	pid = bpf_get_current_pid_tgid() >> 32;
+	if (pid != monitored_pid)
+		return 0;
+
+	data_ptr = bpf_map_lookup_elem(&data_input, &zero);
+	if (!data_ptr)
+		return 0;
+
+	bpf_probe_read(&value, sizeof(value), &attr->value);
+
+	bpf_copy_from_user(data_ptr, sizeof(struct data),
+			   (void *)(unsigned long)value);
+
+	modlen = be32_to_cpu(*(u32 *)data_ptr->payload);
+	mod = data_ptr->payload + sizeof(u32);
+
+	if (modlen > sizeof(struct data) - sizeof(u32))
+		return -EINVAL;
+
+	if (modlen <= marker_len)
+		return -ENOENT;
+
+	modlen &= sizeof(struct data) - 1;
+	bpf_probe_read(marker, marker_len, (char *)mod + modlen - marker_len);
+
+	if (bpf_strncmp(marker, marker_len, MODULE_SIG_STRING))
+		return -ENOENT;
+
+	modlen -= marker_len;
+
+	if (modlen <= sizeof(ms))
+		return -EBADMSG;
+
+	bpf_probe_read(&ms, sizeof(ms), (char *)mod + (modlen - sizeof(ms)));
+
+	ret = mod_check_sig(&ms, modlen);
+	if (ret)
+		return ret;
+
+	sig_len = be32_to_cpu(ms.sig_len);
+	modlen -= sig_len + sizeof(ms);
+
+	modlen &= 0x3ff;
+	sig_len &= 0x3ff;
+
+	return bpf_verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
+					  VERIFY_USE_SECONDARY_KEYRING);
+}
-- 
2.25.1

Re: [PATCH v2 1/3] bpf: Add bpf_verify_pkcs7_signature() helper

[Daniel Borkmann]

On 6/8/22 1:12 PM, Roberto Sassu wrote:
> Add the bpf_verify_pkcs7_signature() helper, to give the ability to eBPF
> security modules to check the validity of a PKCS#7 signature against
> supplied data.
> 
> Use the 'keyring' parameter to select the keyring containing the
> verification key: 0 for the primary keyring, 1 for the primary and
> secondary keyrings, 2 for the platform keyring.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>   include/uapi/linux/bpf.h       |  8 ++++++++
>   kernel/bpf/bpf_lsm.c           | 32 ++++++++++++++++++++++++++++++++
>   tools/include/uapi/linux/bpf.h |  8 ++++++++
>   3 files changed, 48 insertions(+)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index f4009dbdf62d..40d0fc0d9493 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5249,6 +5249,13 @@ union bpf_attr {
>    *		Pointer to the underlying dynptr data, NULL if the dynptr is
>    *		read-only, if the dynptr is invalid, or if the offset and length
>    *		is out of bounds.
> + *
> + * long bpf_verify_pkcs7_signature(u8 *data, u32 datalen, u8 *sig, u32 siglen, u64 keyring)
> + *	Description
> + *		Verify the PKCS#7 *sig* with length *siglen*, on *data* with
> + *		length *datalen*, with key in *keyring*.

Could you also add a description for users about the keyring argument and guidance on when
they should use which in their programs? Above is a bit too terse, imho.

> + *	Return
> + *		0 on success, a negative value on error.
>    */
>   #define __BPF_FUNC_MAPPER(FN)		\
>   	FN(unspec),			\
> @@ -5455,6 +5462,7 @@ union bpf_attr {
>   	FN(dynptr_read),		\
>   	FN(dynptr_write),		\
>   	FN(dynptr_data),		\
> +	FN(verify_pkcs7_signature),	\
>   	/* */
>   
>   /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index c1351df9f7ee..1cda43cb541a 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -16,6 +16,7 @@
>   #include <linux/bpf_local_storage.h>
>   #include <linux/btf_ids.h>
>   #include <linux/ima.h>
> +#include <linux/verification.h>
>   
>   /* For every LSM hook that allows attachment of BPF programs, declare a nop
>    * function where a BPF program can be attached.
> @@ -132,6 +133,35 @@ static const struct bpf_func_proto bpf_get_attach_cookie_proto = {
>   	.arg1_type	= ARG_PTR_TO_CTX,
>   };
>   
> +BPF_CALL_5(bpf_verify_pkcs7_signature, u8 *, data, u32, datalen, u8 *, sig,
> +	   u32, siglen, u64, keyring)
> +{
> +	int ret = -EOPNOTSUPP;
> +
> +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> +	if (keyring > (unsigned long)VERIFY_USE_PLATFORM_KEYRING)
> +		return -EINVAL;
> +
> +	ret = verify_pkcs7_signature(data, datalen, sig, siglen,
> +				     (struct key *)keyring,
> +				     VERIFYING_UNSPECIFIED_SIGNATURE, NULL,
> +				     NULL);
> +#endif
> +	return ret;
> +}

Looks great! One small nit, I would move all of the BPF_CALL and _proto under the
#ifdef CONFIG_SYSTEM_DATA_VERIFICATION ...

> +static const struct bpf_func_proto bpf_verify_pkcs7_signature_proto = {
> +	.func		= bpf_verify_pkcs7_signature,
> +	.gpl_only	= false,
> +	.ret_type	= RET_INTEGER,
> +	.arg1_type	= ARG_PTR_TO_MEM,
> +	.arg2_type	= ARG_CONST_SIZE_OR_ZERO,
> +	.arg3_type	= ARG_PTR_TO_MEM,
> +	.arg4_type	= ARG_CONST_SIZE_OR_ZERO,
> +	.arg5_type	= ARG_ANYTHING,
> +	.allowed	= bpf_ima_inode_hash_allowed,
> +};
> +
>   static const struct bpf_func_proto *
>   bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>   {
> @@ -158,6 +188,8 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>   		return prog->aux->sleepable ? &bpf_ima_file_hash_proto : NULL;
>   	case BPF_FUNC_get_attach_cookie:
>   		return bpf_prog_has_trampoline(prog) ? &bpf_get_attach_cookie_proto : NULL;
> +	case BPF_FUNC_verify_pkcs7_signature:
> +		return prog->aux->sleepable ? &bpf_verify_pkcs7_signature_proto : NULL;

... same here:

#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
	case BPF_FUNC_verify_pkcs7_signature:
		return prog->aux->sleepable ? &bpf_verify_pkcs7_signature_proto : NULL;
#endif

So that bpftool or other feature probes can check for its availability. Otherwise, apps have
a hard time checking whether bpf_verify_pkcs7_signature() helper is available for use or not.

>   	default:
>   		return tracing_prog_func_proto(func_id, prog);
>   	}
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index f4009dbdf62d..40d0fc0d9493 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -5249,6 +5249,13 @@ union bpf_attr {
>    *		Pointer to the underlying dynptr data, NULL if the dynptr is
>    *		read-only, if the dynptr is invalid, or if the offset and length
>    *		is out of bounds.
> + *
> + * long bpf_verify_pkcs7_signature(u8 *data, u32 datalen, u8 *sig, u32 siglen, u64 keyring)
> + *	Description
> + *		Verify the PKCS#7 *sig* with length *siglen*, on *data* with
> + *		length *datalen*, with key in *keyring*.
> + *	Return
> + *		0 on success, a negative value on error.
>    */
>   #define __BPF_FUNC_MAPPER(FN)		\
>   	FN(unspec),			\
> @@ -5455,6 +5462,7 @@ union bpf_attr {
>   	FN(dynptr_read),		\
>   	FN(dynptr_write),		\
>   	FN(dynptr_data),		\
> +	FN(verify_pkcs7_signature),	\
>   	/* */
>   
>   /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> 

Re: [PATCH v2 1/3] bpf: Add bpf_verify_pkcs7_signature() helper

[KP Singh]

On Wed, Jun 8, 2022 at 4:43 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 6/8/22 1:12 PM, Roberto Sassu wrote:
> > Add the bpf_verify_pkcs7_signature() helper, to give the ability to eBPF
> > security modules to check the validity of a PKCS#7 signature against
> > supplied data.

Can we keep the helper generic so that it can be extended to more types of
signatures and pass the signature type as an enum?

bpf_verify_signature and a type SIG_PKCS7 or something.

> >
> > Use the 'keyring' parameter to select the keyring containing the
> > verification key: 0 for the primary keyring, 1 for the primary and
> > secondary keyrings, 2 for the platform keyring.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >   include/uapi/linux/bpf.h       |  8 ++++++++
> >   kernel/bpf/bpf_lsm.c           | 32 ++++++++++++++++++++++++++++++++
> >   tools/include/uapi/linux/bpf.h |  8 ++++++++
> >   3 files changed, 48 insertions(+)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index f4009dbdf62d..40d0fc0d9493 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -5249,6 +5249,13 @@ union bpf_attr {
> >    *          Pointer to the underlying dynptr data, NULL if the dynptr is
> >    *          read-only, if the dynptr is invalid, or if the offset and length
> >    *          is out of bounds.
> > + *
> > + * long bpf_verify_pkcs7_signature(u8 *data, u32 datalen, u8 *sig, u32 siglen, u64 keyring)
> > + *   Description
> > + *           Verify the PKCS#7 *sig* with length *siglen*, on *data* with
> > + *           length *datalen*, with key in *keyring*.
>
> Could you also add a description for users about the keyring argument and guidance on when
> they should use which in their programs? Above is a bit too terse, imho.
>
> > + *   Return
> > + *           0 on success, a negative value on error.
> >    */
> >   #define __BPF_FUNC_MAPPER(FN)               \
> >       FN(unspec),                     \
> > @@ -5455,6 +5462,7 @@ union bpf_attr {
> >       FN(dynptr_read),                \
> >       FN(dynptr_write),               \
> >       FN(dynptr_data),                \
> > +     FN(verify_pkcs7_signature),     \
> >       /* */
> >
> >   /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > index c1351df9f7ee..1cda43cb541a 100644
> > --- a/kernel/bpf/bpf_lsm.c
> > +++ b/kernel/bpf/bpf_lsm.c
> > @@ -16,6 +16,7 @@
> >   #include <linux/bpf_local_storage.h>
> >   #include <linux/btf_ids.h>
> >   #include <linux/ima.h>
> > +#include <linux/verification.h>
> >
> >   /* For every LSM hook that allows attachment of BPF programs, declare a nop
> >    * function where a BPF program can be attached.
> > @@ -132,6 +133,35 @@ static const struct bpf_func_proto bpf_get_attach_cookie_proto = {
> >       .arg1_type      = ARG_PTR_TO_CTX,
> >   };
> >
> > +BPF_CALL_5(bpf_verify_pkcs7_signature, u8 *, data, u32, datalen, u8 *, sig,
> > +        u32, siglen, u64, keyring)
> > +{
> > +     int ret = -EOPNOTSUPP;
> > +
> > +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> > +     if (keyring > (unsigned long)VERIFY_USE_PLATFORM_KEYRING)
> > +             return -EINVAL;
> > +
> > +     ret = verify_pkcs7_signature(data, datalen, sig, siglen,
> > +                                  (struct key *)keyring,
> > +                                  VERIFYING_UNSPECIFIED_SIGNATURE, NULL,
> > +                                  NULL);
> > +#endif
> > +     return ret;
> > +}
>
> Looks great! One small nit, I would move all of the BPF_CALL and _proto under the
> #ifdef CONFIG_SYSTEM_DATA_VERIFICATION ...
>
> > +static const struct bpf_func_proto bpf_verify_pkcs7_signature_proto = {
> > +     .func           = bpf_verify_pkcs7_signature,
> > +     .gpl_only       = false,
> > +     .ret_type       = RET_INTEGER,
> > +     .arg1_type      = ARG_PTR_TO_MEM,
> > +     .arg2_type      = ARG_CONST_SIZE_OR_ZERO,
> > +     .arg3_type      = ARG_PTR_TO_MEM,
> > +     .arg4_type      = ARG_CONST_SIZE_OR_ZERO,
> > +     .arg5_type      = ARG_ANYTHING,
> > +     .allowed        = bpf_ima_inode_hash_allowed,
> > +};
> > +
> >   static const struct bpf_func_proto *
> >   bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >   {
> > @@ -158,6 +188,8 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >               return prog->aux->sleepable ? &bpf_ima_file_hash_proto : NULL;
> >       case BPF_FUNC_get_attach_cookie:
> >               return bpf_prog_has_trampoline(prog) ? &bpf_get_attach_cookie_proto : NULL;
> > +     case BPF_FUNC_verify_pkcs7_signature:
> > +             return prog->aux->sleepable ? &bpf_verify_pkcs7_signature_proto : NULL;
>
> ... same here:
>
> #ifdef CONFIG_SYSTEM_DATA_VERIFICATION
>         case BPF_FUNC_verify_pkcs7_signature:
>                 return prog->aux->sleepable ? &bpf_verify_pkcs7_signature_proto : NULL;
> #endif
>
> So that bpftool or other feature probes can check for its availability. Otherwise, apps have
> a hard time checking whether bpf_verify_pkcs7_signature() helper is available for use or not.
>
> >       default:
> >               return tracing_prog_func_proto(func_id, prog);
> >       }
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index f4009dbdf62d..40d0fc0d9493 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -5249,6 +5249,13 @@ union bpf_attr {
> >    *          Pointer to the underlying dynptr data, NULL if the dynptr is
> >    *          read-only, if the dynptr is invalid, or if the offset and length
> >    *          is out of bounds.
> > + *
> > + * long bpf_verify_pkcs7_signature(u8 *data, u32 datalen, u8 *sig, u32 siglen, u64 keyring)
> > + *   Description
> > + *           Verify the PKCS#7 *sig* with length *siglen*, on *data* with
> > + *           length *datalen*, with key in *keyring*.
> > + *   Return
> > + *           0 on success, a negative value on error.
> >    */
> >   #define __BPF_FUNC_MAPPER(FN)               \
> >       FN(unspec),                     \
> > @@ -5455,6 +5462,7 @@ union bpf_attr {
> >       FN(dynptr_read),                \
> >       FN(dynptr_write),               \
> >       FN(dynptr_data),                \
> > +     FN(verify_pkcs7_signature),     \
> >       /* */
> >
> >   /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> >
>

Re: [PATCH v2 1/3] bpf: Add bpf_verify_pkcs7_signature() helper

[kernel test robot]

Hi Roberto,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]
[also build test WARNING on bpf/master]
[cannot apply to horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Roberto-Sassu/bpf-Add-bpf_verify_pkcs7_signature-helper/20220608-192110
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20220608/202206082219.09oAvCwe-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/223914a2278b692d4120315d2fc7a29e3b89512a
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Roberto-Sassu/bpf-Add-bpf_verify_pkcs7_signature-helper/20220608-192110
        git checkout 223914a2278b692d4120315d2fc7a29e3b89512a
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash kernel/bpf/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   kernel/bpf/bpf_lsm.c: In function '____bpf_verify_pkcs7_signature':
>> kernel/bpf/bpf_lsm.c:146:38: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
     146 |                                      (struct key *)keyring,
         |                                      ^


vim +146 kernel/bpf/bpf_lsm.c

   135	
   136	BPF_CALL_5(bpf_verify_pkcs7_signature, u8 *, data, u32, datalen, u8 *, sig,
   137		   u32, siglen, u64, keyring)
   138	{
   139		int ret = -EOPNOTSUPP;
   140	
   141	#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
   142		if (keyring > (unsigned long)VERIFY_USE_PLATFORM_KEYRING)
   143			return -EINVAL;
   144	
   145		ret = verify_pkcs7_signature(data, datalen, sig, siglen,
 > 146					     (struct key *)keyring,
   147					     VERIFYING_UNSPECIFIED_SIGNATURE, NULL,
   148					     NULL);
   149	#endif
   150		return ret;
   151	}
   152	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

RE: [PATCH v2 1/3] bpf: Add bpf_verify_pkcs7_signature() helper

[Roberto Sassu]

> From: Daniel Borkmann [mailto:daniel@iogearbox.net]
> Sent: Wednesday, June 8, 2022 4:43 PM
> On 6/8/22 1:12 PM, Roberto Sassu wrote:
> > Add the bpf_verify_pkcs7_signature() helper, to give the ability to eBPF
> > security modules to check the validity of a PKCS#7 signature against
> > supplied data.
> >
> > Use the 'keyring' parameter to select the keyring containing the
> > verification key: 0 for the primary keyring, 1 for the primary and
> > secondary keyrings, 2 for the platform keyring.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >   include/uapi/linux/bpf.h       |  8 ++++++++
> >   kernel/bpf/bpf_lsm.c           | 32 ++++++++++++++++++++++++++++++++
> >   tools/include/uapi/linux/bpf.h |  8 ++++++++
> >   3 files changed, 48 insertions(+)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index f4009dbdf62d..40d0fc0d9493 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -5249,6 +5249,13 @@ union bpf_attr {
> >    *		Pointer to the underlying dynptr data, NULL if the dynptr is
> >    *		read-only, if the dynptr is invalid, or if the offset and length
> >    *		is out of bounds.
> > + *
> > + * long bpf_verify_pkcs7_signature(u8 *data, u32 datalen, u8 *sig, u32 siglen,
> u64 keyring)
> > + *	Description
> > + *		Verify the PKCS#7 *sig* with length *siglen*, on *data* with
> > + *		length *datalen*, with key in *keyring*.
> 
> Could you also add a description for users about the keyring argument and
> guidance on when
> they should use which in their programs? Above is a bit too terse, imho.	

Hi Daniel

sure, will do.

> > + *	Return
> > + *		0 on success, a negative value on error.
> >    */
> >   #define __BPF_FUNC_MAPPER(FN)		\
> >   	FN(unspec),			\
> > @@ -5455,6 +5462,7 @@ union bpf_attr {
> >   	FN(dynptr_read),		\
> >   	FN(dynptr_write),		\
> >   	FN(dynptr_data),		\
> > +	FN(verify_pkcs7_signature),	\
> >   	/* */
> >
> >   /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > index c1351df9f7ee..1cda43cb541a 100644
> > --- a/kernel/bpf/bpf_lsm.c
> > +++ b/kernel/bpf/bpf_lsm.c
> > @@ -16,6 +16,7 @@
> >   #include <linux/bpf_local_storage.h>
> >   #include <linux/btf_ids.h>
> >   #include <linux/ima.h>
> > +#include <linux/verification.h>
> >
> >   /* For every LSM hook that allows attachment of BPF programs, declare a
> nop
> >    * function where a BPF program can be attached.
> > @@ -132,6 +133,35 @@ static const struct bpf_func_proto
> bpf_get_attach_cookie_proto = {
> >   	.arg1_type	= ARG_PTR_TO_CTX,
> >   };
> >
> > +BPF_CALL_5(bpf_verify_pkcs7_signature, u8 *, data, u32, datalen, u8 *, sig,
> > +	   u32, siglen, u64, keyring)
> > +{
> > +	int ret = -EOPNOTSUPP;
> > +
> > +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> > +	if (keyring > (unsigned long)VERIFY_USE_PLATFORM_KEYRING)
> > +		return -EINVAL;
> > +
> > +	ret = verify_pkcs7_signature(data, datalen, sig, siglen,
> > +				     (struct key *)keyring,
> > +				     VERIFYING_UNSPECIFIED_SIGNATURE,
> NULL,
> > +				     NULL);
> > +#endif
> > +	return ret;
> > +}
> 
> Looks great! One small nit, I would move all of the BPF_CALL and _proto under
> the
> #ifdef CONFIG_SYSTEM_DATA_VERIFICATION ...

Ok.

> > +static const struct bpf_func_proto bpf_verify_pkcs7_signature_proto = {
> > +	.func		= bpf_verify_pkcs7_signature,
> > +	.gpl_only	= false,
> > +	.ret_type	= RET_INTEGER,
> > +	.arg1_type	= ARG_PTR_TO_MEM,
> > +	.arg2_type	= ARG_CONST_SIZE_OR_ZERO,
> > +	.arg3_type	= ARG_PTR_TO_MEM,
> > +	.arg4_type	= ARG_CONST_SIZE_OR_ZERO,
> > +	.arg5_type	= ARG_ANYTHING,
> > +	.allowed	= bpf_ima_inode_hash_allowed,
> > +};
> > +
> >   static const struct bpf_func_proto *
> >   bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >   {
> > @@ -158,6 +188,8 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const
> struct bpf_prog *prog)
> >   		return prog->aux->sleepable ? &bpf_ima_file_hash_proto :
> NULL;
> >   	case BPF_FUNC_get_attach_cookie:
> >   		return bpf_prog_has_trampoline(prog) ?
> &bpf_get_attach_cookie_proto : NULL;
> > +	case BPF_FUNC_verify_pkcs7_signature:
> > +		return prog->aux->sleepable ?
> &bpf_verify_pkcs7_signature_proto : NULL;
> 
> ... same here:
> 
> #ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> 	case BPF_FUNC_verify_pkcs7_signature:
> 		return prog->aux->sleepable ?
> &bpf_verify_pkcs7_signature_proto : NULL;
> #endif
> 
> So that bpftool or other feature probes can check for its availability. Otherwise,
> apps have
> a hard time checking whether bpf_verify_pkcs7_signature() helper is available
> for use or not.

Ok.

I'm currently fixing the test. The challenge is that the module_signature
structure might not be defined. The way I found to fix it is to include
stdlib.h and linux/bpf.h instead of vmlinux.h, and always define the
structure.

Will also skip the test if CONFIG_MODULE_SIG is not defined.

About the CI, I noticed that the kernel configuration file is in the travis
directory. I modified tools/selftests/bpf/config to update the dependencies
for the new helper.

Maybe we should merge both configs at the time the kernel is built?

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Yang Xi, Li He

> >   	default:
> >   		return tracing_prog_func_proto(func_id, prog);
> >   	}
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index f4009dbdf62d..40d0fc0d9493 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -5249,6 +5249,13 @@ union bpf_attr {
> >    *		Pointer to the underlying dynptr data, NULL if the dynptr is
> >    *		read-only, if the dynptr is invalid, or if the offset and length
> >    *		is out of bounds.
> > + *
> > + * long bpf_verify_pkcs7_signature(u8 *data, u32 datalen, u8 *sig, u32 siglen,
> u64 keyring)
> > + *	Description
> > + *		Verify the PKCS#7 *sig* with length *siglen*, on *data* with
> > + *		length *datalen*, with key in *keyring*.
> > + *	Return
> > + *		0 on success, a negative value on error.
> >    */
> >   #define __BPF_FUNC_MAPPER(FN)		\
> >   	FN(unspec),			\
> > @@ -5455,6 +5462,7 @@ union bpf_attr {
> >   	FN(dynptr_read),		\
> >   	FN(dynptr_write),		\
> >   	FN(dynptr_data),		\
> > +	FN(verify_pkcs7_signature),	\
> >   	/* */
> >
> >   /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> >

RE: [PATCH v2 1/3] bpf: Add bpf_verify_pkcs7_signature() helper

[Roberto Sassu]

> From: KP Singh [mailto:kpsingh@kernel.org]
> Sent: Wednesday, June 8, 2022 4:45 PM
> On Wed, Jun 8, 2022 at 4:43 PM Daniel Borkmann <daniel@iogearbox.net>
> wrote:
> >
> > On 6/8/22 1:12 PM, Roberto Sassu wrote:
> > > Add the bpf_verify_pkcs7_signature() helper, to give the ability to eBPF
> > > security modules to check the validity of a PKCS#7 signature against
> > > supplied data.
> 
> Can we keep the helper generic so that it can be extended to more types of
> signatures and pass the signature type as an enum?
> 
> bpf_verify_signature and a type SIG_PKCS7 or something.

Hi KP

makes sense. Otherwise, we have to add a new helper every time
a new signature verification function is introduced (for example
one would be needed for PGP).

I will reuse enum pkey_id_type in module_signature.h

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Yang Xi, Li He

> > > Use the 'keyring' parameter to select the keyring containing the
> > > verification key: 0 for the primary keyring, 1 for the primary and
> > > secondary keyrings, 2 for the platform keyring.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >   include/uapi/linux/bpf.h       |  8 ++++++++
> > >   kernel/bpf/bpf_lsm.c           | 32 ++++++++++++++++++++++++++++++++
> > >   tools/include/uapi/linux/bpf.h |  8 ++++++++
> > >   3 files changed, 48 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index f4009dbdf62d..40d0fc0d9493 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -5249,6 +5249,13 @@ union bpf_attr {
> > >    *          Pointer to the underlying dynptr data, NULL if the dynptr is
> > >    *          read-only, if the dynptr is invalid, or if the offset and length
> > >    *          is out of bounds.
> > > + *
> > > + * long bpf_verify_pkcs7_signature(u8 *data, u32 datalen, u8 *sig, u32
> siglen, u64 keyring)
> > > + *   Description
> > > + *           Verify the PKCS#7 *sig* with length *siglen*, on *data* with
> > > + *           length *datalen*, with key in *keyring*.
> >
> > Could you also add a description for users about the keyring argument and
> guidance on when
> > they should use which in their programs? Above is a bit too terse, imho.
> >
> > > + *   Return
> > > + *           0 on success, a negative value on error.
> > >    */
> > >   #define __BPF_FUNC_MAPPER(FN)               \
> > >       FN(unspec),                     \
> > > @@ -5455,6 +5462,7 @@ union bpf_attr {
> > >       FN(dynptr_read),                \
> > >       FN(dynptr_write),               \
> > >       FN(dynptr_data),                \
> > > +     FN(verify_pkcs7_signature),     \
> > >       /* */
> > >
> > >   /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > > diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > > index c1351df9f7ee..1cda43cb541a 100644
> > > --- a/kernel/bpf/bpf_lsm.c
> > > +++ b/kernel/bpf/bpf_lsm.c
> > > @@ -16,6 +16,7 @@
> > >   #include <linux/bpf_local_storage.h>
> > >   #include <linux/btf_ids.h>
> > >   #include <linux/ima.h>
> > > +#include <linux/verification.h>
> > >
> > >   /* For every LSM hook that allows attachment of BPF programs, declare a
> nop
> > >    * function where a BPF program can be attached.
> > > @@ -132,6 +133,35 @@ static const struct bpf_func_proto
> bpf_get_attach_cookie_proto = {
> > >       .arg1_type      = ARG_PTR_TO_CTX,
> > >   };
> > >
> > > +BPF_CALL_5(bpf_verify_pkcs7_signature, u8 *, data, u32, datalen, u8 *, sig,
> > > +        u32, siglen, u64, keyring)
> > > +{
> > > +     int ret = -EOPNOTSUPP;
> > > +
> > > +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> > > +     if (keyring > (unsigned long)VERIFY_USE_PLATFORM_KEYRING)
> > > +             return -EINVAL;
> > > +
> > > +     ret = verify_pkcs7_signature(data, datalen, sig, siglen,
> > > +                                  (struct key *)keyring,
> > > +                                  VERIFYING_UNSPECIFIED_SIGNATURE, NULL,
> > > +                                  NULL);
> > > +#endif
> > > +     return ret;
> > > +}
> >
> > Looks great! One small nit, I would move all of the BPF_CALL and _proto under
> the
> > #ifdef CONFIG_SYSTEM_DATA_VERIFICATION ...
> >
> > > +static const struct bpf_func_proto bpf_verify_pkcs7_signature_proto = {
> > > +     .func           = bpf_verify_pkcs7_signature,
> > > +     .gpl_only       = false,
> > > +     .ret_type       = RET_INTEGER,
> > > +     .arg1_type      = ARG_PTR_TO_MEM,
> > > +     .arg2_type      = ARG_CONST_SIZE_OR_ZERO,
> > > +     .arg3_type      = ARG_PTR_TO_MEM,
> > > +     .arg4_type      = ARG_CONST_SIZE_OR_ZERO,
> > > +     .arg5_type      = ARG_ANYTHING,
> > > +     .allowed        = bpf_ima_inode_hash_allowed,
> > > +};
> > > +
> > >   static const struct bpf_func_proto *
> > >   bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog
> *prog)
> > >   {
> > > @@ -158,6 +188,8 @@ bpf_lsm_func_proto(enum bpf_func_id func_id,
> const struct bpf_prog *prog)
> > >               return prog->aux->sleepable ? &bpf_ima_file_hash_proto : NULL;
> > >       case BPF_FUNC_get_attach_cookie:
> > >               return bpf_prog_has_trampoline(prog) ?
> &bpf_get_attach_cookie_proto : NULL;
> > > +     case BPF_FUNC_verify_pkcs7_signature:
> > > +             return prog->aux->sleepable ? &bpf_verify_pkcs7_signature_proto :
> NULL;
> >
> > ... same here:
> >
> > #ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> >         case BPF_FUNC_verify_pkcs7_signature:
> >                 return prog->aux->sleepable ? &bpf_verify_pkcs7_signature_proto :
> NULL;
> > #endif
> >
> > So that bpftool or other feature probes can check for its availability.
> Otherwise, apps have
> > a hard time checking whether bpf_verify_pkcs7_signature() helper is available
> for use or not.
> >
> > >       default:
> > >               return tracing_prog_func_proto(func_id, prog);
> > >       }
> > > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > > index f4009dbdf62d..40d0fc0d9493 100644
> > > --- a/tools/include/uapi/linux/bpf.h
> > > +++ b/tools/include/uapi/linux/bpf.h
> > > @@ -5249,6 +5249,13 @@ union bpf_attr {
> > >    *          Pointer to the underlying dynptr data, NULL if the dynptr is
> > >    *          read-only, if the dynptr is invalid, or if the offset and length
> > >    *          is out of bounds.
> > > + *
> > > + * long bpf_verify_pkcs7_signature(u8 *data, u32 datalen, u8 *sig, u32
> siglen, u64 keyring)
> > > + *   Description
> > > + *           Verify the PKCS#7 *sig* with length *siglen*, on *data* with
> > > + *           length *datalen*, with key in *keyring*.
> > > + *   Return
> > > + *           0 on success, a negative value on error.
> > >    */
> > >   #define __BPF_FUNC_MAPPER(FN)               \
> > >       FN(unspec),                     \
> > > @@ -5455,6 +5462,7 @@ union bpf_attr {
> > >       FN(dynptr_read),                \
> > >       FN(dynptr_write),               \
> > >       FN(dynptr_data),                \
> > > +     FN(verify_pkcs7_signature),     \
> > >       /* */
> > >
> > >   /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > >
> >

Re: [PATCH v2 2/3] selftests/bpf: Add test_progs opts for sign-file and kernel priv key + cert

[Alexei Starovoitov]

On Wed, Jun 8, 2022 at 4:15 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> According to the logs of the eBPF CI, built kernel and tests are copied to
> a virtual machine to run there.
>
> Since a test for a new helper to verify PKCS#7 signatures requires to sign
> data to be verified, extend test_progs to store in the test_env data
> structure (accessible by individual tests) the path of sign-file and of the
> kernel private key and cert.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  tools/testing/selftests/bpf/test_progs.c | 12 ++++++++++++
>  tools/testing/selftests/bpf/test_progs.h |  3 +++
>  2 files changed, 15 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index c639f2e56fc5..90ce2c06a15e 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -707,6 +707,8 @@ enum ARG_KEYS {
>         ARG_TEST_NAME_GLOB_DENYLIST = 'd',
>         ARG_NUM_WORKERS = 'j',
>         ARG_DEBUG = -1,
> +       ARG_SIGN_FILE = 'S',
> +       ARG_KERNEL_PRIV_CERT = 'C',
>  };
>
>  static const struct argp_option opts[] = {
> @@ -732,6 +734,10 @@ static const struct argp_option opts[] = {
>           "Number of workers to run in parallel, default to number of cpus." },
>         { "debug", ARG_DEBUG, NULL, 0,
>           "print extra debug information for test_progs." },
> +       { "sign-file", ARG_SIGN_FILE, "PATH", 0,
> +         "sign-file path " },
> +       { "kernel-priv-cert", ARG_KERNEL_PRIV_CERT, "PATH", 0,
> +         "kernel private key and cert path " },
>         {},
>  };
>
> @@ -862,6 +868,12 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
>         case ARG_DEBUG:
>                 env->debug = true;
>                 break;
> +       case ARG_SIGN_FILE:
> +               env->sign_file_path = arg;
> +               break;
> +       case ARG_KERNEL_PRIV_CERT:
> +               env->kernel_priv_cert_path = arg;
> +               break;

That's cumbersome approach to use to force CI and
users to pass these args on command line.
The test has to be self contained.
test_progs should execute it without any additional input.
For example by having test-only private/public key
that is used to sign and verify the signature.

RE: [PATCH v2 2/3] selftests/bpf: Add test_progs opts for sign-file and kernel priv key + cert

[Roberto Sassu]

> From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> Sent: Thursday, June 9, 2022 2:13 AM
> On Wed, Jun 8, 2022 at 4:15 AM Roberto Sassu <roberto.sassu@huawei.com>
> wrote:
> >
> > According to the logs of the eBPF CI, built kernel and tests are copied to
> > a virtual machine to run there.
> >
> > Since a test for a new helper to verify PKCS#7 signatures requires to sign
> > data to be verified, extend test_progs to store in the test_env data
> > structure (accessible by individual tests) the path of sign-file and of the
> > kernel private key and cert.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  tools/testing/selftests/bpf/test_progs.c | 12 ++++++++++++
> >  tools/testing/selftests/bpf/test_progs.h |  3 +++
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/test_progs.c
> b/tools/testing/selftests/bpf/test_progs.c
> > index c639f2e56fc5..90ce2c06a15e 100644
> > --- a/tools/testing/selftests/bpf/test_progs.c
> > +++ b/tools/testing/selftests/bpf/test_progs.c
> > @@ -707,6 +707,8 @@ enum ARG_KEYS {
> >         ARG_TEST_NAME_GLOB_DENYLIST = 'd',
> >         ARG_NUM_WORKERS = 'j',
> >         ARG_DEBUG = -1,
> > +       ARG_SIGN_FILE = 'S',
> > +       ARG_KERNEL_PRIV_CERT = 'C',
> >  };
> >
> >  static const struct argp_option opts[] = {
> > @@ -732,6 +734,10 @@ static const struct argp_option opts[] = {
> >           "Number of workers to run in parallel, default to number of cpus." },
> >         { "debug", ARG_DEBUG, NULL, 0,
> >           "print extra debug information for test_progs." },
> > +       { "sign-file", ARG_SIGN_FILE, "PATH", 0,
> > +         "sign-file path " },
> > +       { "kernel-priv-cert", ARG_KERNEL_PRIV_CERT, "PATH", 0,
> > +         "kernel private key and cert path " },
> >         {},
> >  };
> >
> > @@ -862,6 +868,12 @@ static error_t parse_arg(int key, char *arg, struct
> argp_state *state)
> >         case ARG_DEBUG:
> >                 env->debug = true;
> >                 break;
> > +       case ARG_SIGN_FILE:
> > +               env->sign_file_path = arg;
> > +               break;
> > +       case ARG_KERNEL_PRIV_CERT:
> > +               env->kernel_priv_cert_path = arg;
> > +               break;
> 
> That's cumbersome approach to use to force CI and
> users to pass these args on command line.
> The test has to be self contained.
> test_progs should execute it without any additional input.
> For example by having test-only private/public key
> that is used to sign and verify the signature.

I thought a bit about this. Just generating a test key does not work,
as it must be signed by the kernel signing key (otherwise, loading
in the secondary keyring will be rejected). Having the test key around
is as dangerous as having the kernel signing key around copied
somewhere.

Allowing users to specify a test keyring in the helper is possible.
But it would introduce unnecessary code, plus the keyring identifier
will be understood by eBPF only and not by verify_pkcs7_signature(),
as it happens for other keyring identifiers.

We may have environment variables directly in the eBPF test, to
specify the location of the signing key, but there is a risk of
duplication, as other tests wanting the same information might
not be aware of them.

I would not introduce any code that handles the kernel signing
key (in the Makefile, or in a separate script). This information is
so sensible, that it must be responsibility of an external party
to do the work of making that key available and tell where it is.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Yang Xi, Li He

Re: [PATCH v2 2/3] selftests/bpf: Add test_progs opts for sign-file and kernel priv key + cert

[Alexei Starovoitov]

On Thu, Jun 9, 2022 at 2:00 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> > Sent: Thursday, June 9, 2022 2:13 AM
> > On Wed, Jun 8, 2022 at 4:15 AM Roberto Sassu <roberto.sassu@huawei.com>
> > wrote:
> > >
> > > According to the logs of the eBPF CI, built kernel and tests are copied to
> > > a virtual machine to run there.
> > >
> > > Since a test for a new helper to verify PKCS#7 signatures requires to sign
> > > data to be verified, extend test_progs to store in the test_env data
> > > structure (accessible by individual tests) the path of sign-file and of the
> > > kernel private key and cert.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >  tools/testing/selftests/bpf/test_progs.c | 12 ++++++++++++
> > >  tools/testing/selftests/bpf/test_progs.h |  3 +++
> > >  2 files changed, 15 insertions(+)
> > >
> > > diff --git a/tools/testing/selftests/bpf/test_progs.c
> > b/tools/testing/selftests/bpf/test_progs.c
> > > index c639f2e56fc5..90ce2c06a15e 100644
> > > --- a/tools/testing/selftests/bpf/test_progs.c
> > > +++ b/tools/testing/selftests/bpf/test_progs.c
> > > @@ -707,6 +707,8 @@ enum ARG_KEYS {
> > >         ARG_TEST_NAME_GLOB_DENYLIST = 'd',
> > >         ARG_NUM_WORKERS = 'j',
> > >         ARG_DEBUG = -1,
> > > +       ARG_SIGN_FILE = 'S',
> > > +       ARG_KERNEL_PRIV_CERT = 'C',
> > >  };
> > >
> > >  static const struct argp_option opts[] = {
> > > @@ -732,6 +734,10 @@ static const struct argp_option opts[] = {
> > >           "Number of workers to run in parallel, default to number of cpus." },
> > >         { "debug", ARG_DEBUG, NULL, 0,
> > >           "print extra debug information for test_progs." },
> > > +       { "sign-file", ARG_SIGN_FILE, "PATH", 0,
> > > +         "sign-file path " },
> > > +       { "kernel-priv-cert", ARG_KERNEL_PRIV_CERT, "PATH", 0,
> > > +         "kernel private key and cert path " },
> > >         {},
> > >  };
> > >
> > > @@ -862,6 +868,12 @@ static error_t parse_arg(int key, char *arg, struct
> > argp_state *state)
> > >         case ARG_DEBUG:
> > >                 env->debug = true;
> > >                 break;
> > > +       case ARG_SIGN_FILE:
> > > +               env->sign_file_path = arg;
> > > +               break;
> > > +       case ARG_KERNEL_PRIV_CERT:
> > > +               env->kernel_priv_cert_path = arg;
> > > +               break;
> >
> > That's cumbersome approach to use to force CI and
> > users to pass these args on command line.
> > The test has to be self contained.
> > test_progs should execute it without any additional input.
> > For example by having test-only private/public key
> > that is used to sign and verify the signature.
>
> I thought a bit about this. Just generating a test key does not work,
> as it must be signed by the kernel signing key (otherwise, loading
> in the secondary keyring will be rejected). Having the test key around
> is as dangerous as having the kernel signing key around copied
> somewhere.
>
> Allowing users to specify a test keyring in the helper is possible.

We shouldn't need to load into the secondary keyring.
The helper needs to support an arbitrary key ring.
The kernel shouldn't interfere with loading that test key into
a test ring.

> But it would introduce unnecessary code, plus the keyring identifier

What kind of 'unnecessary code' ?

> will be understood by eBPF only and not by verify_pkcs7_signature(),
> as it happens for other keyring identifiers.

Maybe wrapping verify_pkcs7_signature as a helper is too high level?

> We may have environment variables directly in the eBPF test, to
> specify the location of the signing key, but there is a risk of
> duplication, as other tests wanting the same information might
> not be aware of them.

That's no go.

> I would not introduce any code that handles the kernel signing
> key (in the Makefile, or in a separate script). This information is
> so sensible, that it must be responsibility of an external party
> to do the work of making that key available and tell where it is.
>
> Roberto
>
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Yang Xi, Li He

RE: [PATCH v2 2/3] selftests/bpf: Add test_progs opts for sign-file and kernel priv key + cert

[Roberto Sassu]

> From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> Sent: Thursday, June 9, 2022 5:38 PM
> On Thu, Jun 9, 2022 at 2:00 AM Roberto Sassu <roberto.sassu@huawei.com>
> wrote:
> >
> > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> > > Sent: Thursday, June 9, 2022 2:13 AM
> > > On Wed, Jun 8, 2022 at 4:15 AM Roberto Sassu
> <roberto.sassu@huawei.com>
> > > wrote:
> > > >
> > > > According to the logs of the eBPF CI, built kernel and tests are copied to
> > > > a virtual machine to run there.
> > > >
> > > > Since a test for a new helper to verify PKCS#7 signatures requires to sign
> > > > data to be verified, extend test_progs to store in the test_env data
> > > > structure (accessible by individual tests) the path of sign-file and of the
> > > > kernel private key and cert.
> > > >
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > ---
> > > >  tools/testing/selftests/bpf/test_progs.c | 12 ++++++++++++
> > > >  tools/testing/selftests/bpf/test_progs.h |  3 +++
> > > >  2 files changed, 15 insertions(+)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/test_progs.c
> > > b/tools/testing/selftests/bpf/test_progs.c
> > > > index c639f2e56fc5..90ce2c06a15e 100644
> > > > --- a/tools/testing/selftests/bpf/test_progs.c
> > > > +++ b/tools/testing/selftests/bpf/test_progs.c
> > > > @@ -707,6 +707,8 @@ enum ARG_KEYS {
> > > >         ARG_TEST_NAME_GLOB_DENYLIST = 'd',
> > > >         ARG_NUM_WORKERS = 'j',
> > > >         ARG_DEBUG = -1,
> > > > +       ARG_SIGN_FILE = 'S',
> > > > +       ARG_KERNEL_PRIV_CERT = 'C',
> > > >  };
> > > >
> > > >  static const struct argp_option opts[] = {
> > > > @@ -732,6 +734,10 @@ static const struct argp_option opts[] = {
> > > >           "Number of workers to run in parallel, default to number of cpus." },
> > > >         { "debug", ARG_DEBUG, NULL, 0,
> > > >           "print extra debug information for test_progs." },
> > > > +       { "sign-file", ARG_SIGN_FILE, "PATH", 0,
> > > > +         "sign-file path " },
> > > > +       { "kernel-priv-cert", ARG_KERNEL_PRIV_CERT, "PATH", 0,
> > > > +         "kernel private key and cert path " },
> > > >         {},
> > > >  };
> > > >
> > > > @@ -862,6 +868,12 @@ static error_t parse_arg(int key, char *arg, struct
> > > argp_state *state)
> > > >         case ARG_DEBUG:
> > > >                 env->debug = true;
> > > >                 break;
> > > > +       case ARG_SIGN_FILE:
> > > > +               env->sign_file_path = arg;
> > > > +               break;
> > > > +       case ARG_KERNEL_PRIV_CERT:
> > > > +               env->kernel_priv_cert_path = arg;
> > > > +               break;
> > >
> > > That's cumbersome approach to use to force CI and
> > > users to pass these args on command line.
> > > The test has to be self contained.
> > > test_progs should execute it without any additional input.
> > > For example by having test-only private/public key
> > > that is used to sign and verify the signature.
> >
> > I thought a bit about this. Just generating a test key does not work,
> > as it must be signed by the kernel signing key (otherwise, loading
> > in the secondary keyring will be rejected). Having the test key around
> > is as dangerous as having the kernel signing key around copied
> > somewhere.
> >
> > Allowing users to specify a test keyring in the helper is possible.
> 
> We shouldn't need to load into the secondary keyring.
> The helper needs to support an arbitrary key ring.
> The kernel shouldn't interfere with loading that test key into
> a test ring.
> 
> > But it would introduce unnecessary code, plus the keyring identifier
> 
> What kind of 'unnecessary code' ?

The code for handling eBPF-specific keyring identifiers.

But at the end, it is not that much.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Yang Xi, Li He

> > will be understood by eBPF only and not by verify_pkcs7_signature(),
> > as it happens for other keyring identifiers.
> 
> Maybe wrapping verify_pkcs7_signature as a helper is too high level?
> 
> > We may have environment variables directly in the eBPF test, to
> > specify the location of the signing key, but there is a risk of
> > duplication, as other tests wanting the same information might
> > not be aware of them.
> 
> That's no go.
> 
> > I would not introduce any code that handles the kernel signing
> > key (in the Makefile, or in a separate script). This information is
> > so sensible, that it must be responsibility of an external party
> > to do the work of making that key available and tell where it is.
> >
> > Roberto
> >
> > HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> > Managing Director: Li Peng, Yang Xi, Li He