linux-kselftest.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification
@ 2022-08-09 13:45 Roberto Sassu
  2022-08-09 13:45 ` [PATCH v9 01/10] btf: Add a new kfunc flag which allows to mark a function to be sleepable Roberto Sassu
                   ` (11 more replies)
  0 siblings, 12 replies; 28+ messages in thread
From: Roberto Sassu @ 2022-08-09 13:45 UTC (permalink / raw)
  To: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, corbet, dhowells, jarkko, rostedt,
	mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel, 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 four new kfuncs:
bpf_lookup_user_key() and bpf_lookup_system_key(), for retrieving a keyring
with keys trusted for signature verification, respectively from its serial
and from a pre-determined ID; bpf_key_put(), to release the reference
obtained with the former two kfuncs, bpf_verify_pkcs7_signature(), for
verifying PKCS#7 signatures.

Other than the key serial, bpf_lookup_user_key() also accepts key lookup
flags, that influence the behavior of the lookup. bpf_lookup_system_key()
accepts pre-determined IDs defined in include/linux/verification.h.

bpf_key_put() accepts the new bpf_key structure, introduced to tell whether
the other structure member, a key pointer, is valid or not. The reason is
that verify_pkcs7_signature() also accepts invalid pointers, set with the
pre-determined ID, to select a system-defined keyring. key_put() must be
called only for valid key pointers.

Since the two key lookup functions allocate memory and one increments a key
reference count, they must be used in conjunction with bpf_key_put(). The
latter must be called only if the lookup functions returned a non-NULL
pointer. The verifier denies the execution of eBPF programs that don't
respect this rule.

The two key lookup functions should be used in alternative, depending on
the use case. While bpf_lookup_user_key() provides great flexibility, it
seems suboptimal in terms of security guarantees, as even if the eBPF
program is assumed to be trusted, the serial used to obtain the key pointer
might come from untrusted user space not choosing one that the system
administrator approves to enforce a mandatory policy.

bpf_lookup_system_key() instead provides much stronger guarantees,
especially if the pre-determined ID is not passed by user space but is
hardcoded in the eBPF program, and that program is signed. In this case,
bpf_verify_pkcs7_signature() will always perform signature verification
with a key that the system administrator approves, i.e. the primary,
secondary or platform keyring.

Nevertheless, key permission checks need to be done accurately. Since
bpf_lookup_user_key() cannot determine how a key will be used by other
kfuncs, it has to defer the permission check to the actual kfunc using the
key. It does it by calling lookup_user_key() with KEY_DEFER_PERM_CHECK as
needed permission. Later, bpf_verify_pkcs7_signature(), if called,
completes the permission check by calling key_validate(). It does not need
to call key_task_permission() with permission KEY_NEED_SEARCH, as it is
already done elsewhere by the key subsystem. Future kfuncs using the
bpf_key structure need to implement the proper checks as well.

Finally, the last kfunc, bpf_verify_pkcs7_signature(), accepts the data and
signature to verify as eBPF dynamic pointers, to minimize the number of
kfunc parameters, and the keyring with keys for signature verification as a
bpf_key structure, returned by one of the two key lookup functions.

All kfuncs except bpf_key_put() can be called only from sleepable programs,
because of memory allocation and crypto operations. For example, the
lsm.s/bpf attach point is suitable, fexit/array_map_update_elem is not.

The correctness of implementation of the new kfuncs and of their usage is
checked with the introduced tests.

The patch set includes patches from other authors (dependencies) for sake
of completeness. It is organized as follows.

Patch 1 from Benjamin Tissoires introduces the new KF_SLEEPABLE kfunc flag.
Patch 2 from KP Singh allows kfuncs to be used by LSM programs. Patch 3
allows dynamic pointers to be used as kfunc parameters. Patch 4 exports
bpf_dynptr_get_size(), to obtain the real size of data carried by a dynamic
pointer. Patch 5 makes available for new eBPF kfuncs some key-related
definitions. Patch 6 introduces the bpf_lookup_*_key() and bpf_key_put()
kfuncs. Patch 7 introduces the bpf_verify_pkcs7_signature() kfunc. Finally,
patches 8-10 introduce the tests.

Changelog

v8:
 - Define the new bpf_key structure to carry the key pointer and whether
   that pointer is valid or not (suggested by Daniel)
 - Drop patch to mark a kfunc parameter with the __maybe_null suffix
 - Improve documentation of kfuncs
 - Introduce bpf_lookup_system_key() to obtain a key pointer suitable for
   verify_pkcs7_signature() (suggested by Daniel)
 - Use the new kfunc registration API
 - Drop patch to test the __maybe_null suffix
 - Add tests for bpf_lookup_system_key()

v7:
 - Add support for using dynamic and NULL pointers in kfunc (suggested by
   Alexei)
 - Add new kfunc-related tests

v6:
 - Switch back to key lookup helpers + signature verification (until v5),
   and defer permission check from bpf_lookup_user_key() to
   bpf_verify_pkcs7_signature()
 - Add additional key lookup test to illustrate the usage of the
   KEY_LOOKUP_CREATE flag and validate the flags (suggested by Daniel)
 - Make description of flags of bpf_lookup_user_key() more user-friendly
   (suggested by Daniel)
 - Fix validation of flags parameter in bpf_lookup_user_key() (reported by
   Daniel)
 - Rename bpf_verify_pkcs7_signature() keyring-related parameters to
   user_keyring and system_keyring to make their purpose more clear
 - Accept keyring-related parameters of bpf_verify_pkcs7_signature() as
   alternatives (suggested by KP)
 - Replace unsigned long type with u64 in helper declaration (suggested by
   Daniel)
 - Extend the bpf_verify_pkcs7_signature() test by calling the helper
   without data, by ensuring that the helper enforces the keyring-related
   parameters as alternatives, by ensuring that the helper rejects
   inaccessible and expired keyrings, and by checking all system keyrings
 - Move bpf_lookup_user_key() and bpf_key_put() usage tests to
   ref_tracking.c (suggested by John)
 - Call bpf_lookup_user_key() and bpf_key_put() only in sleepable programs

v5:
 - Move KEY_LOOKUP_ to include/linux/key.h
   for validation of bpf_verify_pkcs7_signature() parameter
 - Remove bpf_lookup_user_key() and bpf_key_put() helpers, and the
   corresponding tests
 - Replace struct key parameter of bpf_verify_pkcs7_signature() with the
   keyring serial and lookup flags
 - Call lookup_user_key() and key_put() in bpf_verify_pkcs7_signature()
   code, to ensure that the retrieved key is used according to the
   permission requested at lookup time
 - Clarified keyring precedence in the description of
   bpf_verify_pkcs7_signature() (suggested by John)
 - Remove newline in the second argument of ASSERT_
 - Fix helper prototype regular expression in bpf_doc.py

v4:
 - Remove bpf_request_key_by_id(), don't return an invalid pointer that
   other helpers can use
 - Pass the keyring ID (without ULONG_MAX, suggested by Alexei) to
   bpf_verify_pkcs7_signature()
 - Introduce bpf_lookup_user_key() and bpf_key_put() helpers (suggested by
   Alexei)
 - Add lookup_key_norelease test, to ensure that the verifier blocks eBPF
   programs which don't decrement the key reference count
 - Parse raw PKCS#7 signature instead of module-style signature in the
   verify_pkcs7_signature test (suggested by Alexei)
 - Parse kernel module in user space and pass raw PKCS#7 signature to the
   eBPF program for signature verification

v3:
 - Rename bpf_verify_signature() back to bpf_verify_pkcs7_signature() to
   avoid managing different parameters for each signature verification
   function in one helper (suggested by Daniel)
 - Use dynamic pointers and export bpf_dynptr_get_size() (suggested by
   Alexei)
 - Introduce bpf_request_key_by_id() to give more flexibility to the caller
   of bpf_verify_pkcs7_signature() to retrieve the appropriate keyring
   (suggested by Alexei)
 - Fix test by reordering the gcc command line, always compile sign-file
 - Improve helper support check mechanism in the test

v2:
 - Rename bpf_verify_pkcs7_signature() to a more generic
   bpf_verify_signature() and pass the signature type (suggested by KP)
 - Move the helper and prototype declaration under #ifdef so that user
   space can probe for support for the helper (suggested by Daniel)
 - Describe better the keyring types (suggested by Daniel)
 - Include linux/bpf.h instead of vmlinux.h to avoid implicit or
   redeclaration
 - Make the test selfcontained (suggested by Alexei)

v1:
 - Don't define new map flag but introduce simple wrapper of
   verify_pkcs7_signature() (suggested by Alexei and KP)

Benjamin Tissoires (1):
  btf: Add a new kfunc flag which allows to mark a function to be
    sleepable

KP Singh (1):
  bpf: Allow kfuncs to be used in LSM programs

Roberto Sassu (8):
  btf: Handle dynamic pointer parameter in kfuncs
  bpf: Export bpf_dynptr_get_size()
  KEYS: Move KEY_LOOKUP_ to include/linux/key.h
  bpf: Add bpf_lookup_*_key() and bpf_key_put() kfuncs
  bpf: Add bpf_verify_pkcs7_signature() kfunc
  selftests/bpf: Add verifier tests for bpf_lookup_*_key() and
    bpf_key_put()
  selftests/bpf: Add additional tests for bpf_lookup_*_key()
  selftests/bpf: Add test for bpf_verify_pkcs7_signature() kfunc

 Documentation/bpf/kfuncs.rst                  |   6 +
 include/linux/bpf.h                           |   7 +
 include/linux/bpf_verifier.h                  |   3 +
 include/linux/btf.h                           |   1 +
 include/linux/key.h                           |   3 +
 kernel/bpf/btf.c                              |  27 ++
 kernel/bpf/helpers.c                          |   2 +-
 kernel/bpf/verifier.c                         |   4 +-
 kernel/trace/bpf_trace.c                      | 207 +++++++++
 security/keys/internal.h                      |   2 -
 tools/testing/selftests/bpf/Makefile          |  14 +-
 tools/testing/selftests/bpf/config            |   2 +
 .../selftests/bpf/prog_tests/lookup_key.c     | 112 +++++
 .../bpf/prog_tests/verify_pkcs7_sig.c         | 399 ++++++++++++++++++
 .../selftests/bpf/progs/test_lookup_key.c     |  46 ++
 .../bpf/progs/test_verify_pkcs7_sig.c         | 100 +++++
 tools/testing/selftests/bpf/test_verifier.c   |   3 +-
 .../selftests/bpf/verifier/ref_tracking.c     | 139 ++++++
 .../testing/selftests/bpf/verify_sig_setup.sh | 104 +++++
 19 files changed, 1172 insertions(+), 9 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/lookup_key.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_lookup_key.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
 create mode 100755 tools/testing/selftests/bpf/verify_sig_setup.sh

-- 
2.25.1

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v9 01/10] btf: Add a new kfunc flag which allows to mark a function to be sleepable
  2022-08-09 13:45 [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification Roberto Sassu
@ 2022-08-09 13:45 ` Roberto Sassu
  2022-08-09 16:54   ` Jarkko Sakkinen
  2022-08-09 13:45 ` [PATCH v9 02/10] bpf: Allow kfuncs to be used in LSM programs Roberto Sassu
                   ` (10 subsequent siblings)
  11 siblings, 1 reply; 28+ messages in thread
From: Roberto Sassu @ 2022-08-09 13:45 UTC (permalink / raw)
  To: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, corbet, dhowells, jarkko, rostedt,
	mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel, Benjamin Tissoires, Yosry Ahmed

From: Benjamin Tissoires <benjamin.tissoires@redhat.com>

From: Benjamin Tissoires <benjamin.tissoires@redhat.com>

This allows to declare a kfunc as sleepable and prevents its use in
a non sleepable program.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Co-developed-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Hao Luo <haoluo@google.com>
---
 Documentation/bpf/kfuncs.rst | 6 ++++++
 include/linux/btf.h          | 1 +
 kernel/bpf/btf.c             | 9 +++++++++
 3 files changed, 16 insertions(+)

diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
index c0b7dae6dbf5..c8b21de1c772 100644
--- a/Documentation/bpf/kfuncs.rst
+++ b/Documentation/bpf/kfuncs.rst
@@ -146,6 +146,12 @@ that operate (change some property, perform some operation) on an object that
 was obtained using an acquire kfunc. Such kfuncs need an unchanged pointer to
 ensure the integrity of the operation being performed on the expected object.
 
+2.4.6 KF_SLEEPABLE flag
+-----------------------
+
+The KF_SLEEPABLE flag is used for kfuncs that may sleep. Such kfuncs can only
+be called by sleepable BPF programs (BPF_F_SLEEPABLE).
+
 2.5 Registering the kfuncs
 --------------------------
 
diff --git a/include/linux/btf.h b/include/linux/btf.h
index cdb376d53238..976cbdd2981f 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -49,6 +49,7 @@
  * for this case.
  */
 #define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer arguments */
+#define KF_SLEEPABLE   (1 << 5) /* kfunc may sleep */
 
 struct btf;
 struct btf_member;
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 7e64447659f3..d3e4c86b8fcd 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6175,6 +6175,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 {
 	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
 	bool rel = false, kptr_get = false, trusted_arg = false;
+	bool sleepable = false;
 	struct bpf_verifier_log *log = &env->log;
 	u32 i, nargs, ref_id, ref_obj_id = 0;
 	bool is_kfunc = btf_is_kernel(btf);
@@ -6212,6 +6213,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		rel = kfunc_flags & KF_RELEASE;
 		kptr_get = kfunc_flags & KF_KPTR_GET;
 		trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
+		sleepable = kfunc_flags & KF_SLEEPABLE;
 	}
 
 	/* check that BTF function arguments match actual types that the
@@ -6419,6 +6421,13 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 			func_name);
 		return -EINVAL;
 	}
+
+	if (sleepable && !env->prog->aux->sleepable) {
+		bpf_log(log, "kernel function %s is sleepable but the program is not\n",
+			func_name);
+		return -EINVAL;
+	}
+
 	/* returns argument register number > 0 in case of reference release kfunc */
 	return rel ? ref_regno : 0;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v9 02/10] bpf: Allow kfuncs to be used in LSM programs
  2022-08-09 13:45 [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification Roberto Sassu
  2022-08-09 13:45 ` [PATCH v9 01/10] btf: Add a new kfunc flag which allows to mark a function to be sleepable Roberto Sassu
@ 2022-08-09 13:45 ` Roberto Sassu
  2022-08-09 21:53   ` Daniel Borkmann
  2022-08-09 13:45 ` [PATCH v9 03/10] btf: Handle dynamic pointer parameter in kfuncs Roberto Sassu
                   ` (9 subsequent siblings)
  11 siblings, 1 reply; 28+ messages in thread
From: Roberto Sassu @ 2022-08-09 13:45 UTC (permalink / raw)
  To: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, corbet, dhowells, jarkko, rostedt,
	mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel

From: KP Singh <kpsingh@kernel.org>

In preparation for the addition of bpf_getxattr kfunc.

Signed-off-by: KP Singh <kpsingh@kernel.org>
---
 kernel/bpf/btf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index d3e4c86b8fcd..67dfc728fbf8 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -7244,6 +7244,7 @@ static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type)
 	case BPF_PROG_TYPE_STRUCT_OPS:
 		return BTF_KFUNC_HOOK_STRUCT_OPS;
 	case BPF_PROG_TYPE_TRACING:
+	case BPF_PROG_TYPE_LSM:
 		return BTF_KFUNC_HOOK_TRACING;
 	case BPF_PROG_TYPE_SYSCALL:
 		return BTF_KFUNC_HOOK_SYSCALL;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v9 03/10] btf: Handle dynamic pointer parameter in kfuncs
  2022-08-09 13:45 [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification Roberto Sassu
  2022-08-09 13:45 ` [PATCH v9 01/10] btf: Add a new kfunc flag which allows to mark a function to be sleepable Roberto Sassu
  2022-08-09 13:45 ` [PATCH v9 02/10] bpf: Allow kfuncs to be used in LSM programs Roberto Sassu
@ 2022-08-09 13:45 ` Roberto Sassu
  2022-08-09 22:08   ` Daniel Borkmann
  2022-08-09 22:29   ` Daniel Borkmann
  2022-08-09 13:45 ` [PATCH v9 04/10] bpf: Export bpf_dynptr_get_size() Roberto Sassu
                   ` (8 subsequent siblings)
  11 siblings, 2 replies; 28+ messages in thread
From: Roberto Sassu @ 2022-08-09 13:45 UTC (permalink / raw)
  To: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, corbet, dhowells, jarkko, rostedt,
	mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel, Roberto Sassu, Joanne Koong

Allow the bpf_dynptr_kern parameter to be specified in kfuncs. Also, ensure
that the dynamic pointer is valid and initialized.

Cc: Joanne Koong <joannelkoong@gmail.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/bpf_verifier.h |  3 +++
 kernel/bpf/btf.c             | 17 +++++++++++++++++
 kernel/bpf/verifier.c        |  4 ++--
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 2e3bad8640dc..55876fbdbae2 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -560,6 +560,9 @@ int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state
 			     u32 regno);
 int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
 		   u32 regno, u32 mem_size);
+bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env,
+			      struct bpf_reg_state *reg,
+			      enum bpf_arg_type arg_type);
 
 /* this lives here instead of in bpf.h because it needs to dereference tgt_prog */
 static inline u64 bpf_trampoline_compute_key(const struct bpf_prog *tgt_prog,
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 67dfc728fbf8..17cca396c89f 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6363,6 +6363,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 
 			if (is_kfunc) {
 				bool arg_mem_size = i + 1 < nargs && is_kfunc_arg_mem_size(btf, &args[i + 1], &regs[regno + 1]);
+				bool arg_dynptr = btf_type_is_struct(ref_t) &&
+						  !strcmp(ref_tname, "bpf_dynptr_kern");
 
 				/* Permit pointer to mem, but only when argument
 				 * type is pointer to scalar, or struct composed
@@ -6372,6 +6374,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				 */
 				if (!btf_type_is_scalar(ref_t) &&
 				    !__btf_type_is_scalar_struct(log, btf, ref_t, 0) &&
+				    !arg_dynptr &&
 				    (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) {
 					bpf_log(log,
 						"arg#%d pointer type %s %s must point to %sscalar, or struct with scalar\n",
@@ -6379,6 +6382,20 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 					return -EINVAL;
 				}
 
+				/* Assume initialized dynptr. */
+				if (arg_dynptr) {
+					if (!is_dynptr_reg_valid_init(env, reg,
+							ARG_PTR_TO_DYNPTR)) {
+						bpf_log(log,
+							"arg#%d pointer type %s %s must be initialized\n",
+							i, btf_type_str(ref_t),
+							ref_tname);
+						return -EINVAL;
+					}
+
+					continue;
+				}
+
 				/* Check for mem, len pair */
 				if (arg_mem_size) {
 					if (check_kfunc_mem_size_reg(env, &regs[regno + 1], regno + 1)) {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 843a966cd02b..3c52246a1ceb 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -773,8 +773,8 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
 	return true;
 }
 
-static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
-				     enum bpf_arg_type arg_type)
+bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
+			      enum bpf_arg_type arg_type)
 {
 	struct bpf_func_state *state = func(env, reg);
 	int spi = get_spi(reg->off);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v9 04/10] bpf: Export bpf_dynptr_get_size()
  2022-08-09 13:45 [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification Roberto Sassu
                   ` (2 preceding siblings ...)
  2022-08-09 13:45 ` [PATCH v9 03/10] btf: Handle dynamic pointer parameter in kfuncs Roberto Sassu
@ 2022-08-09 13:45 ` Roberto Sassu
  2022-08-09 13:45 ` [PATCH v9 05/10] KEYS: Move KEY_LOOKUP_ to include/linux/key.h Roberto Sassu
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 28+ messages in thread
From: Roberto Sassu @ 2022-08-09 13:45 UTC (permalink / raw)
  To: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, corbet, dhowells, jarkko, rostedt,
	mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel, Roberto Sassu, Joanne Koong

Export bpf_dynptr_get_size(), so that kernel code dealing with eBPF dynamic
pointers can obtain the real size of data carried by this data structure.

Reviewed-by: Joanne Koong <joannelkoong@gmail.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/bpf.h  | 1 +
 kernel/bpf/helpers.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 20c26aed7896..0d56c23cc504 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2562,6 +2562,7 @@ void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data,
 		     enum bpf_dynptr_type type, u32 offset, u32 size);
 void bpf_dynptr_set_null(struct bpf_dynptr_kern *ptr);
 int bpf_dynptr_check_size(u32 size);
+u32 bpf_dynptr_get_size(struct bpf_dynptr_kern *ptr);
 
 #ifdef CONFIG_BPF_LSM
 void bpf_cgroup_atype_get(u32 attach_btf_id, int cgroup_atype);
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 1f961f9982d2..5dbb90ec34b0 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1430,7 +1430,7 @@ static void bpf_dynptr_set_type(struct bpf_dynptr_kern *ptr, enum bpf_dynptr_typ
 	ptr->size |= type << DYNPTR_TYPE_SHIFT;
 }
 
-static u32 bpf_dynptr_get_size(struct bpf_dynptr_kern *ptr)
+u32 bpf_dynptr_get_size(struct bpf_dynptr_kern *ptr)
 {
 	return ptr->size & DYNPTR_SIZE_MASK;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v9 05/10] KEYS: Move KEY_LOOKUP_ to include/linux/key.h
  2022-08-09 13:45 [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification Roberto Sassu
                   ` (3 preceding siblings ...)
  2022-08-09 13:45 ` [PATCH v9 04/10] bpf: Export bpf_dynptr_get_size() Roberto Sassu
@ 2022-08-09 13:45 ` Roberto Sassu
  2022-08-09 13:45 ` [PATCH v9 06/10] bpf: Add bpf_lookup_*_key() and bpf_key_put() kfuncs Roberto Sassu
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 28+ messages in thread
From: Roberto Sassu @ 2022-08-09 13:45 UTC (permalink / raw)
  To: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, corbet, dhowells, jarkko, rostedt,
	mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel, Roberto Sassu

In preparation for the patch that introduces the bpf_lookup_user_key() eBPF
kfunc, move KEY_LOOKUP_ definitions to include/linux/key.h, to be able to
validate the kfunc parameters.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/key.h      | 3 +++
 security/keys/internal.h | 2 --
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index 7febc4881363..a297e075038c 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -88,6 +88,9 @@ enum key_need_perm {
 	KEY_DEFER_PERM_CHECK,	/* Special: permission check is deferred */
 };
 
+#define KEY_LOOKUP_CREATE	0x01
+#define KEY_LOOKUP_PARTIAL	0x02
+
 struct seq_file;
 struct user_struct;
 struct signal_struct;
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 9b9cf3b6fcbb..3c1e7122076b 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -165,8 +165,6 @@ extern struct key *request_key_and_link(struct key_type *type,
 
 extern bool lookup_user_key_possessed(const struct key *key,
 				      const struct key_match_data *match_data);
-#define KEY_LOOKUP_CREATE	0x01
-#define KEY_LOOKUP_PARTIAL	0x02
 
 extern long join_session_keyring(const char *name);
 extern void key_change_session_keyring(struct callback_head *twork);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v9 06/10] bpf: Add bpf_lookup_*_key() and bpf_key_put() kfuncs
  2022-08-09 13:45 [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification Roberto Sassu
                   ` (4 preceding siblings ...)
  2022-08-09 13:45 ` [PATCH v9 05/10] KEYS: Move KEY_LOOKUP_ to include/linux/key.h Roberto Sassu
@ 2022-08-09 13:45 ` Roberto Sassu
  2022-08-09 22:53   ` Daniel Borkmann
  2022-08-09 13:46 ` [PATCH v9 07/10] bpf: Add bpf_verify_pkcs7_signature() kfunc Roberto Sassu
                   ` (5 subsequent siblings)
  11 siblings, 1 reply; 28+ messages in thread
From: Roberto Sassu @ 2022-08-09 13:45 UTC (permalink / raw)
  To: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, corbet, dhowells, jarkko, rostedt,
	mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel, Roberto Sassu

Add the bpf_lookup_user_key(), bpf_lookup_system_key() and bpf_key_put()
kfuncs, to respectively search a key with a given serial and flags, obtain
a key from a pre-determined ID defined in include/linux/verification.h, and
cleanup.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/bpf.h      |   6 ++
 kernel/trace/bpf_trace.c | 151 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 157 insertions(+)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 0d56c23cc504..564b9e0b8c16 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2572,4 +2572,10 @@ static inline void bpf_cgroup_atype_get(u32 attach_btf_id, int cgroup_atype) {}
 static inline void bpf_cgroup_atype_put(int cgroup_atype) {}
 #endif /* CONFIG_BPF_LSM */
 
+#ifdef CONFIG_KEYS
+struct bpf_key {
+	struct key *key;
+	bool valid_ptr;
+};
+#endif /* CONFIG_KEYS */
 #endif /* _LINUX_BPF_H */
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 68e5cdd24cef..33ca4cfe6e26 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -20,6 +20,7 @@
 #include <linux/fprobe.h>
 #include <linux/bsearch.h>
 #include <linux/sort.h>
+#include <linux/key.h>
 
 #include <net/bpf_sk_storage.h>
 
@@ -1181,6 +1182,156 @@ static const struct bpf_func_proto bpf_get_func_arg_cnt_proto = {
 	.arg1_type	= ARG_PTR_TO_CTX,
 };
 
+#ifdef CONFIG_KEYS
+__diag_push();
+__diag_ignore_all("-Wmissing-prototypes",
+		  "kfuncs which will be used in BPF programs");
+
+/**
+ * bpf_lookup_user_key - lookup a key by its serial
+ * @serial: key serial
+ * @flags: lookup-specific flags
+ *
+ * Search a key with a given *serial* and the provided *flags*. The
+ * returned key, if found, has the reference count incremented by
+ * one, and is stored in a bpf_key structure, returned to the caller.
+ * The bpf_key structure must be passed to bpf_key_put() when done
+ * with it, so that the key reference count is decremented and the
+ * bpf_key structure is freed.
+ *
+ * Permission checks are deferred to the time the key is used by
+ * one of the available key-specific kfuncs.
+ *
+ * Set *flags* with 1, to attempt creating a requested special
+ * keyring (e.g. session keyring), if it doesn't yet exist. Set
+ * *flags* with 2 to lookup a key without waiting for the key
+ * construction, and to retrieve uninstantiated keys (keys without
+ * data attached to them).
+ *
+ * Return: a bpf_key pointer with a valid key pointer if the key is found, a
+ *         NULL pointer otherwise.
+ */
+noinline __weak struct bpf_key *bpf_lookup_user_key(u32 serial, u64 flags)
+{
+	key_ref_t key_ref;
+	struct bpf_key *bkey;
+
+	/* Keep in sync with include/linux/key.h. */
+	if (flags > (KEY_LOOKUP_PARTIAL << 1) - 1)
+		return NULL;
+
+	/* Permission check is deferred until actual kfunc using the key. */
+	key_ref = lookup_user_key(serial, flags, KEY_DEFER_PERM_CHECK);
+	if (IS_ERR(key_ref))
+		return NULL;
+
+	bkey = kmalloc(sizeof(*bkey), GFP_KERNEL);
+	if (!bkey) {
+		key_put(key_ref_to_ptr(key_ref));
+		return bkey;
+	}
+
+	bkey->key = key_ref_to_ptr(key_ref);
+	bkey->valid_ptr = true;
+
+	return bkey;
+}
+
+/**
+ * bpf_lookup_system_key - lookup a key by a system-defined ID
+ * @id: key ID
+ *
+ * Obtain a bpf_key structure with a key pointer set to the passed key ID.
+ * The key pointer is marked as invalid, to prevent bpf_key_put() from
+ * attempting to decrement the key reference count on that pointer. The key
+ * pointer set in such way is currently understood only by
+ * verify_pkcs7_signature().
+ *
+ * Set *id* to one of the values defined in include/linux/verification.h:
+ * 0 for the primary keyring (immutable keyring of system keys); 1 for both
+ * the primary and secondary keyring (where keys can be added only if they
+ * are vouched for by existing keys in those keyrings); 2 for the platform
+ * keyring (primarily used by the integrity subsystem to verify a kexec'ed
+ * kerned image and, possibly, the initramfs signature).
+ *
+ * Return: a bpf_key pointer with an invalid key pointer set from the
+ *         pre-determined ID on success, a NULL pointer otherwise
+ */
+noinline __weak struct bpf_key *bpf_lookup_system_key(u64 id)
+{
+	struct bpf_key *bkey;
+
+	/* Keep in sync with defs in include/linux/verification.h. */
+	if (id > (unsigned long)VERIFY_USE_PLATFORM_KEYRING)
+		return NULL;
+
+	bkey = kmalloc(sizeof(*bkey), GFP_KERNEL);
+	if (!bkey)
+		return bkey;
+
+	bkey->key = (struct key *)(unsigned long)id;
+	bkey->valid_ptr = false;
+
+	return bkey;
+}
+
+/**
+ * bpf_key_put - decrement key reference count if key is valid and free bpf_key
+ * @bkey: bpf_key structure
+ *
+ * Decrement the reference count of the key inside *bkey*, if the pointer
+ * is valid, and free *bkey*.
+ */
+noinline __weak void bpf_key_put(struct bpf_key *bkey)
+{
+	if (bkey->valid_ptr)
+		key_put(bkey->key);
+
+	kfree(bkey);
+}
+
+__diag_pop();
+
+BTF_SET8_START(key_kfunc_set)
+BTF_ID_FLAGS(func, bpf_lookup_user_key, KF_ACQUIRE | KF_RET_NULL | KF_SLEEPABLE)
+BTF_ID_FLAGS(func, bpf_lookup_system_key,
+	     KF_ACQUIRE | KF_RET_NULL | KF_SLEEPABLE)
+BTF_ID_FLAGS(func, bpf_key_put, KF_RELEASE)
+BTF_SET8_END(key_kfunc_set)
+
+static const struct btf_kfunc_id_set bpf_key_kfunc_set = {
+	.owner = THIS_MODULE,
+	.set = &key_kfunc_set,
+};
+#endif /* CONFIG_KEYS */
+
+const struct btf_kfunc_id_set *kfunc_sets[] = {
+#ifdef CONFIG_KEYS
+	&bpf_key_kfunc_set,
+#endif /* CONFIG_KEYS */
+};
+
+static int __init bpf_kfuncs_init(void)
+{
+	int ret, i;
+
+	for (i = 0; i < ARRAY_SIZE(kfunc_sets); i++) {
+		ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING,
+						kfunc_sets[i]);
+		if (!ret)
+			continue;
+
+		ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM,
+						kfunc_sets[i]);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+late_initcall(bpf_kfuncs_init);
+
 static const struct bpf_func_proto *
 bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v9 07/10] bpf: Add bpf_verify_pkcs7_signature() kfunc
  2022-08-09 13:45 [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification Roberto Sassu
                   ` (5 preceding siblings ...)
  2022-08-09 13:45 ` [PATCH v9 06/10] bpf: Add bpf_lookup_*_key() and bpf_key_put() kfuncs Roberto Sassu
@ 2022-08-09 13:46 ` Roberto Sassu
  2022-08-09 23:09   ` Daniel Borkmann
  2022-08-10  2:41   ` Alexei Starovoitov
  2022-08-09 13:46 ` [PATCH v9 08/10] selftests/bpf: Add verifier tests for bpf_lookup_*_key() and bpf_key_put() Roberto Sassu
                   ` (4 subsequent siblings)
  11 siblings, 2 replies; 28+ messages in thread
From: Roberto Sassu @ 2022-08-09 13:46 UTC (permalink / raw)
  To: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, corbet, dhowells, jarkko, rostedt,
	mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel, Roberto Sassu

Add the bpf_verify_pkcs7_signature() kfunc, to give eBPF security modules
the ability to check the validity of a signature against supplied data, by
using user-provided or system-provided keys as trust anchor.

The new kfunc makes it possible to enforce mandatory policies, as eBPF
programs might be allowed to make security decisions only based on data
sources the system administrator approves.

The caller should provide the data to be verified and the signature as eBPF
dynamic pointers (to minimize the number of parameters) and a bpf_key
structure containing a reference to the keyring with keys trusted for
signature verification, obtained from bpf_lookup_user_key() or
bpf_lookup_system_key().

For bpf_key structures obtained from the former lookup function,
bpf_verify_pkcs7_signature() completes the permission check deferred by
that function by calling key_validate(). key_task_permission() is already
called by the PKCS#7 code.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 kernel/trace/bpf_trace.c | 56 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 33ca4cfe6e26..79ba8c96735a 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -21,6 +21,7 @@
 #include <linux/bsearch.h>
 #include <linux/sort.h>
 #include <linux/key.h>
+#include <linux/verification.h>
 
 #include <net/bpf_sk_storage.h>
 
@@ -1290,6 +1291,47 @@ noinline __weak void bpf_key_put(struct bpf_key *bkey)
 	kfree(bkey);
 }
 
+#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
+/**
+ * bpf_verify_pkcs7_signature - verify a PKCS#7 signature
+ * @data_ptr: data to verify
+ * @sig_ptr: signature of the data
+ * @trusted_keyring: keyring with keys trusted for signature verification
+ *
+ * Verify the PKCS#7 signature *sig_ptr* against the supplied *data_ptr*
+ * with keys in a keyring referenced by *trusted_keyring*.
+ *
+ * Return: 0 on success, a negative value on error.
+ */
+noinline __weak int bpf_verify_pkcs7_signature(struct bpf_dynptr_kern *data_ptr,
+					       struct bpf_dynptr_kern *sig_ptr,
+					       struct bpf_key *trusted_keyring)
+{
+	int ret;
+
+	if (trusted_keyring->valid_ptr) {
+		/*
+		 * Do the permission check deferred in bpf_lookup_user_key().
+		 *
+		 * A call to key_task_permission() here would be redundant, as
+		 * it is already done by keyring_search() called by
+		 * find_asymmetric_key().
+		 */
+		ret = key_validate(trusted_keyring->key);
+		if (ret < 0)
+			return ret;
+	}
+
+	return verify_pkcs7_signature(data_ptr->data,
+				      bpf_dynptr_get_size(data_ptr),
+				      sig_ptr->data,
+				      bpf_dynptr_get_size(sig_ptr),
+				      trusted_keyring->key,
+				      VERIFYING_UNSPECIFIED_SIGNATURE, NULL,
+				      NULL);
+}
+#endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
+
 __diag_pop();
 
 BTF_SET8_START(key_kfunc_set)
@@ -1303,11 +1345,25 @@ static const struct btf_kfunc_id_set bpf_key_kfunc_set = {
 	.owner = THIS_MODULE,
 	.set = &key_kfunc_set,
 };
+
+#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
+BTF_SET8_START(verify_sig_kfunc_set)
+BTF_ID_FLAGS(func, bpf_verify_pkcs7_signature, KF_SLEEPABLE)
+BTF_SET8_END(verify_sig_kfunc_set)
+
+static const struct btf_kfunc_id_set bpf_verify_sig_kfunc_set = {
+	.owner = THIS_MODULE,
+	.set = &verify_sig_kfunc_set,
+};
+#endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
 #endif /* CONFIG_KEYS */
 
 const struct btf_kfunc_id_set *kfunc_sets[] = {
 #ifdef CONFIG_KEYS
 	&bpf_key_kfunc_set,
+#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
+	&bpf_verify_sig_kfunc_set,
+#endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
 #endif /* CONFIG_KEYS */
 };
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v9 08/10] selftests/bpf: Add verifier tests for bpf_lookup_*_key() and bpf_key_put()
  2022-08-09 13:45 [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification Roberto Sassu
                   ` (6 preceding siblings ...)
  2022-08-09 13:46 ` [PATCH v9 07/10] bpf: Add bpf_verify_pkcs7_signature() kfunc Roberto Sassu
@ 2022-08-09 13:46 ` Roberto Sassu
  2022-08-09 13:46 ` [PATCH v9 09/10] selftests/bpf: Add additional tests for bpf_lookup_*_key() Roberto Sassu
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 28+ messages in thread
From: Roberto Sassu @ 2022-08-09 13:46 UTC (permalink / raw)
  To: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, corbet, dhowells, jarkko, rostedt,
	mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel, Roberto Sassu

Add verifier tests for bpf_lookup_*_key() and bpf_key_put(), to ensure that
acquired key references stored in the bpf_key structure are released, that
a non-NULL bpf_key pointer is passed to bpf_key_put(), and that key
references are not leaked.

Also, slightly modify test_verifier.c, to find the BTF ID of the attach
point for the LSM program type (currently, it is done only for TRACING).

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 tools/testing/selftests/bpf/test_verifier.c   |   3 +-
 .../selftests/bpf/verifier/ref_tracking.c     | 139 ++++++++++++++++++
 2 files changed, 141 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index f9d553fbf68a..2dbcbf363c18 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -1498,7 +1498,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
 		opts.log_level = DEFAULT_LIBBPF_LOG_LEVEL;
 	opts.prog_flags = pflags;
 
-	if (prog_type == BPF_PROG_TYPE_TRACING && test->kfunc) {
+	if ((prog_type == BPF_PROG_TYPE_TRACING ||
+	     prog_type == BPF_PROG_TYPE_LSM) && test->kfunc) {
 		int attach_btf_id;
 
 		attach_btf_id = libbpf_find_vmlinux_btf_id(test->kfunc,
diff --git a/tools/testing/selftests/bpf/verifier/ref_tracking.c b/tools/testing/selftests/bpf/verifier/ref_tracking.c
index 57a83d763ec1..f18ce867271f 100644
--- a/tools/testing/selftests/bpf/verifier/ref_tracking.c
+++ b/tools/testing/selftests/bpf/verifier/ref_tracking.c
@@ -84,6 +84,145 @@
 	.errstr = "Unreleased reference",
 	.result = REJECT,
 },
+{
+	"reference tracking: acquire/release user key reference",
+	.insns = {
+	BPF_MOV64_IMM(BPF_REG_1, -3),
+	BPF_MOV64_IMM(BPF_REG_2, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_LSM,
+	.kfunc = "bpf",
+	.expected_attach_type = BPF_LSM_MAC,
+	.flags = BPF_F_SLEEPABLE,
+	.fixup_kfunc_btf_id = {
+		{ "bpf_lookup_user_key", 2 },
+		{ "bpf_key_put", 5 },
+	},
+	.result = ACCEPT,
+},
+{
+	"reference tracking: acquire/release system key reference",
+	.insns = {
+	BPF_MOV64_IMM(BPF_REG_1, 1),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_LSM,
+	.kfunc = "bpf",
+	.expected_attach_type = BPF_LSM_MAC,
+	.flags = BPF_F_SLEEPABLE,
+	.fixup_kfunc_btf_id = {
+		{ "bpf_lookup_system_key", 1 },
+		{ "bpf_key_put", 4 },
+	},
+	.result = ACCEPT,
+},
+{
+	"reference tracking: release user key reference without check",
+	.insns = {
+	BPF_MOV64_IMM(BPF_REG_1, -3),
+	BPF_MOV64_IMM(BPF_REG_2, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_LSM,
+	.kfunc = "bpf",
+	.expected_attach_type = BPF_LSM_MAC,
+	.flags = BPF_F_SLEEPABLE,
+	.errstr = "arg#0 pointer type STRUCT bpf_key must point to scalar, or struct with scalar",
+	.fixup_kfunc_btf_id = {
+		{ "bpf_lookup_user_key", 2 },
+		{ "bpf_key_put", 4 },
+	},
+	.result = REJECT,
+},
+{
+	"reference tracking: release system key reference without check",
+	.insns = {
+	BPF_MOV64_IMM(BPF_REG_1, 1),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_LSM,
+	.kfunc = "bpf",
+	.expected_attach_type = BPF_LSM_MAC,
+	.flags = BPF_F_SLEEPABLE,
+	.errstr = "arg#0 pointer type STRUCT bpf_key must point to scalar, or struct with scalar",
+	.fixup_kfunc_btf_id = {
+		{ "bpf_lookup_system_key", 1 },
+		{ "bpf_key_put", 3 },
+	},
+	.result = REJECT,
+},
+{
+	"reference tracking: release with NULL key pointer",
+	.insns = {
+	BPF_MOV64_IMM(BPF_REG_1, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_LSM,
+	.kfunc = "bpf",
+	.expected_attach_type = BPF_LSM_MAC,
+	.flags = BPF_F_SLEEPABLE,
+	.errstr = "arg#0 pointer type STRUCT bpf_key must point to scalar, or struct with scalar",
+	.fixup_kfunc_btf_id = {
+		{ "bpf_key_put", 1 },
+	},
+	.result = REJECT,
+},
+{
+	"reference tracking: leak potential reference to user key",
+	.insns = {
+	BPF_MOV64_IMM(BPF_REG_1, -3),
+	BPF_MOV64_IMM(BPF_REG_2, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_LSM,
+	.kfunc = "bpf",
+	.expected_attach_type = BPF_LSM_MAC,
+	.flags = BPF_F_SLEEPABLE,
+	.errstr = "Unreleased reference",
+	.fixup_kfunc_btf_id = {
+		{ "bpf_lookup_user_key", 2 },
+	},
+	.result = REJECT,
+},
+{
+	"reference tracking: leak potential reference to system key",
+	.insns = {
+	BPF_MOV64_IMM(BPF_REG_1, 1),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_LSM,
+	.kfunc = "bpf",
+	.expected_attach_type = BPF_LSM_MAC,
+	.flags = BPF_F_SLEEPABLE,
+	.errstr = "Unreleased reference",
+	.fixup_kfunc_btf_id = {
+		{ "bpf_lookup_system_key", 1 },
+	},
+	.result = REJECT,
+},
 {
 	"reference tracking: release reference without check",
 	.insns = {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v9 09/10] selftests/bpf: Add additional tests for bpf_lookup_*_key()
  2022-08-09 13:45 [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification Roberto Sassu
                   ` (7 preceding siblings ...)
  2022-08-09 13:46 ` [PATCH v9 08/10] selftests/bpf: Add verifier tests for bpf_lookup_*_key() and bpf_key_put() Roberto Sassu
@ 2022-08-09 13:46 ` Roberto Sassu
  2022-08-09 13:46 ` [PATCH v9 10/10] selftests/bpf: Add test for bpf_verify_pkcs7_signature() kfunc Roberto Sassu
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 28+ messages in thread
From: Roberto Sassu @ 2022-08-09 13:46 UTC (permalink / raw)
  To: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, corbet, dhowells, jarkko, rostedt,
	mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel, Roberto Sassu

Add a test to ensure that bpf_lookup_user_key() creates a referenced
special keyring when the KEY_LOOKUP_CREATE flag is passed to this function.

Ensure that the kfunc rejects invalid flags.

Ensure that a keyring can be obtained from bpf_lookup_system_key() when one
of the pre-determined keyring IDs is provided.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 .../selftests/bpf/prog_tests/lookup_key.c     | 112 ++++++++++++++++++
 .../selftests/bpf/progs/test_lookup_key.c     |  46 +++++++
 2 files changed, 158 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/lookup_key.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_lookup_key.c

diff --git a/tools/testing/selftests/bpf/prog_tests/lookup_key.c b/tools/testing/selftests/bpf/prog_tests/lookup_key.c
new file mode 100644
index 000000000000..2e0cde729dc7
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/lookup_key.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH
+ *
+ * Author: Roberto Sassu <roberto.sassu@huawei.com>
+ */
+
+#include <linux/keyctl.h>
+#include <test_progs.h>
+
+#include "test_lookup_key.skel.h"
+
+#define KEY_LOOKUP_CREATE	0x01
+#define KEY_LOOKUP_PARTIAL	0x02
+
+static bool kfunc_not_supported;
+
+static int libbpf_print_cb(enum libbpf_print_level level, const char *fmt,
+			   va_list args)
+{
+	char *func;
+
+	if (strcmp(fmt, "libbpf: extern (func ksym) '%s': not found in kernel or module BTFs\n"))
+		return 0;
+
+	func = va_arg(args, char *);
+
+	if (strcmp(func, "bpf_lookup_user_key") && strcmp(func, "bpf_key_put") &&
+	    strcmp(func, "bpf_lookup_system_key"))
+		return 0;
+
+	kfunc_not_supported = true;
+	return 0;
+}
+
+void test_lookup_key(void)
+{
+	libbpf_print_fn_t old_print_cb;
+	struct test_lookup_key *skel;
+	u32 next_id;
+	int ret;
+
+	skel = test_lookup_key__open();
+	if (!ASSERT_OK_PTR(skel, "test_lookup_key__open"))
+		return;
+
+	old_print_cb = libbpf_set_print(libbpf_print_cb);
+	ret = test_lookup_key__load(skel);
+	libbpf_set_print(old_print_cb);
+
+	if (ret < 0 && kfunc_not_supported) {
+		printf("%s:SKIP:bpf_lookup_*_key(), bpf_key_put() kfuncs not supported\n",
+		       __func__);
+		test__skip();
+		goto close_prog;
+	}
+
+	if (!ASSERT_OK(ret, "test_lookup_key__load"))
+		goto close_prog;
+
+	ret = test_lookup_key__attach(skel);
+	if (!ASSERT_OK(ret, "test_lookup_key__attach"))
+		goto close_prog;
+
+	skel->bss->monitored_pid = getpid();
+	skel->bss->key_serial = KEY_SPEC_THREAD_KEYRING;
+
+	/* The thread-specific keyring does not exist, this test fails. */
+	skel->bss->flags = 0;
+
+	ret = bpf_prog_get_next_id(0, &next_id);
+	if (!ASSERT_LT(ret, 0, "bpf_prog_get_next_id"))
+		goto close_prog;
+
+	/* Force creation of the thread-specific keyring, this test succeeds. */
+	skel->bss->flags = KEY_LOOKUP_CREATE;
+
+	ret = bpf_prog_get_next_id(0, &next_id);
+	if (!ASSERT_OK(ret, "bpf_prog_get_next_id"))
+		goto close_prog;
+
+	/* Pass both lookup flags for parameter validation. */
+	skel->bss->flags = KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL;
+
+	ret = bpf_prog_get_next_id(0, &next_id);
+	if (!ASSERT_OK(ret, "bpf_prog_get_next_id"))
+		goto close_prog;
+
+	/* Pass invalid flags. */
+	skel->bss->flags = UINT64_MAX;
+
+	ret = bpf_prog_get_next_id(0, &next_id);
+	if (!ASSERT_LT(ret, 0, "bpf_prog_get_next_id"))
+		goto close_prog;
+
+	skel->bss->key_serial = 0;
+	skel->bss->key_id = 1;
+
+	ret = bpf_prog_get_next_id(0, &next_id);
+	if (!ASSERT_OK(ret, "bpf_prog_get_next_id"))
+		goto close_prog;
+
+	skel->bss->key_id = UINT32_MAX;
+
+	ret = bpf_prog_get_next_id(0, &next_id);
+	ASSERT_LT(ret, 0, "bpf_prog_get_next_id");
+
+close_prog:
+	skel->bss->monitored_pid = 0;
+	test_lookup_key__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_lookup_key.c b/tools/testing/selftests/bpf/progs/test_lookup_key.c
new file mode 100644
index 000000000000..c73776990ae3
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_lookup_key.c
@@ -0,0 +1,46 @@
+// 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>
+
+char _license[] SEC("license") = "GPL";
+
+__u32 monitored_pid;
+__u32 key_serial;
+__u32 key_id;
+__u64 flags;
+
+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;
+
+SEC("lsm.s/bpf")
+int BPF_PROG(bpf, int cmd, union bpf_attr *attr, unsigned int size)
+{
+	struct bpf_key *bkey;
+	__u32 pid;
+
+	pid = bpf_get_current_pid_tgid() >> 32;
+	if (pid != monitored_pid)
+		return 0;
+
+	if (key_serial)
+		bkey = bpf_lookup_user_key(key_serial, flags);
+	else
+		bkey = bpf_lookup_system_key(key_id);
+
+	if (!bkey)
+		return -ENOENT;
+
+	bpf_key_put(bkey);
+
+	return 0;
+}
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v9 10/10] selftests/bpf: Add test for bpf_verify_pkcs7_signature() kfunc
  2022-08-09 13:45 [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification Roberto Sassu
                   ` (8 preceding siblings ...)
  2022-08-09 13:46 ` [PATCH v9 09/10] selftests/bpf: Add additional tests for bpf_lookup_*_key() Roberto Sassu
@ 2022-08-09 13:46 ` Roberto Sassu
  2022-08-09 16:20 ` [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification patchwork-bot+netdevbpf
  2022-08-09 16:53 ` Jarkko Sakkinen
  11 siblings, 0 replies; 28+ messages in thread
From: Roberto Sassu @ 2022-08-09 13:46 UTC (permalink / raw)
  To: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, corbet, dhowells, jarkko, rostedt,
	mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel, Roberto Sassu

Perform several tests to ensure the correct implementation of the
bpf_verify_pkcs7_signature() kfunc.

Do the tests with data signed with a generated testing key (by using
sign-file from scripts/) and with the tcp_bic.ko kernel module if it is
found in the system. The test does not fail if tcp_bic.ko is not found.

First, perform an unsuccessful signature verification without data.

Second, perform a successful signature verification with the session
keyring and a new one created for testing.

Then, ensure that permission and validation checks are done properly on the
keyring provided to bpf_verify_pkcs7_signature(), despite those checks were
deferred at the time the keyring was retrieved with bpf_lookup_user_key().
The tests expect to encounter an error if the Search permission is removed
from the keyring, or the keyring is expired.

Finally, perform a successful and unsuccessful signature verification with
the keyrings with pre-determined IDs (the last test fails because the key
is not in the platform keyring).

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 tools/testing/selftests/bpf/Makefile          |  14 +-
 tools/testing/selftests/bpf/config            |   2 +
 .../bpf/prog_tests/verify_pkcs7_sig.c         | 399 ++++++++++++++++++
 .../bpf/progs/test_verify_pkcs7_sig.c         | 100 +++++
 .../testing/selftests/bpf/verify_sig_setup.sh | 104 +++++
 5 files changed, 616 insertions(+), 3 deletions(-)
 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
 create mode 100755 tools/testing/selftests/bpf/verify_sig_setup.sh

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 8d59ec7f4c2d..5ae079e276b3 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -14,6 +14,7 @@ BPFTOOLDIR := $(TOOLSDIR)/bpf/bpftool
 APIDIR := $(TOOLSINCDIR)/uapi
 GENDIR := $(abspath ../../../../include/generated)
 GENHDR := $(GENDIR)/autoconf.h
+HOSTPKG_CONFIG := pkg-config
 
 ifneq ($(wildcard $(GENHDR)),)
   GENFLAGS := -DHAVE_GENHDR
@@ -75,7 +76,7 @@ TEST_PROGS := test_kmod.sh \
 	test_xsk.sh
 
 TEST_PROGS_EXTENDED := with_addr.sh \
-	with_tunnels.sh ima_setup.sh \
+	with_tunnels.sh ima_setup.sh verify_sig_setup.sh \
 	test_xdp_vlan.sh test_bpftool.py
 
 # Compile but not part of 'make run_tests'
@@ -84,7 +85,7 @@ TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
 	test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \
 	xskxceiver xdp_redirect_multi xdp_synproxy
 
-TEST_CUSTOM_PROGS = $(OUTPUT)/urandom_read
+TEST_CUSTOM_PROGS = $(OUTPUT)/urandom_read $(OUTPUT)/sign-file
 
 # Emit succinct information message describing current building step
 # $1 - generic step name (e.g., CC, LINK, etc);
@@ -189,6 +190,12 @@ $(OUTPUT)/urandom_read: urandom_read.c urandom_read_aux.c $(OUTPUT)/liburandom_r
 		     -fuse-ld=$(LLD) -Wl,-znoseparate-code		       \
 		     -Wl,-rpath=. -Wl,--build-id=sha1 -o $@
 
+$(OUTPUT)/sign-file: ../../../../scripts/sign-file.c
+	$(call msg,SIGN-FILE,,$@)
+	$(Q)$(CC) $(shell $(HOSTPKG_CONFIG)--cflags libcrypto 2> /dev/null) \
+		  $< -o $@ \
+		  $(shell $(HOSTPKG_CONFIG) --libs libcrypto 2> /dev/null || echo -lcrypto)
+
 $(OUTPUT)/bpf_testmod.ko: $(VMLINUX_BTF) $(wildcard bpf_testmod/Makefile bpf_testmod/*.[ch])
 	$(call msg,MOD,,$@)
 	$(Q)$(RM) bpf_testmod/bpf_testmod.ko # force re-compilation
@@ -514,7 +521,8 @@ TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c trace_helpers.c	\
 TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko	\
 		       $(OUTPUT)/liburandom_read.so			\
 		       $(OUTPUT)/xdp_synproxy				\
-		       ima_setup.sh					\
+		       $(OUTPUT)/sign-file				\
+		       ima_setup.sh verify_sig_setup.sh			\
 		       $(wildcard progs/btf_dump_test_case_*.c)
 TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
 TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) -DENABLE_ATOMICS_TESTS
diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
index fabf0c014349..4d0e4ed20159 100644
--- a/tools/testing/selftests/bpf/config
+++ b/tools/testing/selftests/bpf/config
@@ -62,3 +62,5 @@ CONFIG_TEST_BPF=m
 CONFIG_USERFAULTFD=y
 CONFIG_VXLAN=y
 CONFIG_XDP_SOCKETS=y
+CONFIG_MODULE_SIG=y
+CONFIG_KEYS=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..20be68d4cce4
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
@@ -0,0 +1,399 @@
+// 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 <limits.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+#include <sys/mman.h>
+#include <linux/keyctl.h>
+#include <test_progs.h>
+
+#include "test_verify_pkcs7_sig.skel.h"
+
+#define MAX_DATA_SIZE (1024 * 1024)
+#define MAX_SIG_SIZE 1024
+
+#define VERIFY_USE_SECONDARY_KEYRING (1UL)
+#define VERIFY_USE_PLATFORM_KEYRING  (2UL)
+
+/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
+#define MODULE_SIG_STRING "~Module signature appended~\n"
+
+/*
+ * Module signature information block.
+ *
+ * The constituents of the signature section are, in order:
+ *
+ *	- Signer's name
+ *	- Key identifier
+ *	- Signature data
+ *	- Information block
+ */
+struct module_signature {
+	u8	algo;		/* Public-key crypto algorithm [0] */
+	u8	hash;		/* Digest algorithm [0] */
+	u8	id_type;	/* Key identifier type [PKEY_ID_PKCS7] */
+	u8	signer_len;	/* Length of signer's name [0] */
+	u8	key_id_len;	/* Length of key identifier [0] */
+	u8	__pad[3];
+	__be32	sig_len;	/* Length of signature data */
+};
+
+struct data {
+	u8 data[MAX_DATA_SIZE];
+	u32 data_len;
+	u8 sig[MAX_SIG_SIZE];
+	u32 sig_len;
+};
+
+static bool kfunc_not_supported;
+
+static int libbpf_print_cb(enum libbpf_print_level level, const char *fmt,
+			   va_list args)
+{
+	if (strcmp(fmt, "libbpf: extern (func ksym) '%s': not found in kernel or module BTFs\n"))
+		return 0;
+
+	if (strcmp(va_arg(args, char *), "bpf_verify_pkcs7_signature"))
+		return 0;
+
+	kfunc_not_supported = true;
+	return 0;
+}
+
+static int _run_setup_process(const char *setup_dir, const char *cmd)
+{
+	int child_pid, child_status;
+
+	child_pid = fork();
+	if (child_pid == 0) {
+		execlp("./verify_sig_setup.sh", "./verify_sig_setup.sh", cmd,
+		       setup_dir, NULL);
+		exit(errno);
+
+	} else if (child_pid > 0) {
+		waitpid(child_pid, &child_status, 0);
+		return WEXITSTATUS(child_status);
+	}
+
+	return -EINVAL;
+}
+
+static int populate_data_item_str(const char *tmp_dir, struct data *data_item)
+{
+	struct stat st;
+	char data_template[] = "/tmp/dataXXXXXX";
+	char path[PATH_MAX];
+	int ret, fd, child_status, child_pid;
+
+	data_item->data_len = 4;
+	memcpy(data_item->data, "test", data_item->data_len);
+
+	fd = mkstemp(data_template);
+	if (fd == -1)
+		return -errno;
+
+	ret = write(fd, data_item->data, data_item->data_len);
+
+	close(fd);
+
+	if (ret != data_item->data_len) {
+		ret = -EIO;
+		goto out;
+	}
+
+	child_pid = fork();
+
+	if (child_pid == -1) {
+		ret = -errno;
+		goto out;
+	}
+
+	if (child_pid == 0) {
+		snprintf(path, sizeof(path), "%s/signing_key.pem", tmp_dir);
+
+		return execlp("./sign-file", "./sign-file", "-d", "sha256",
+			      path, path, data_template, NULL);
+	}
+
+	waitpid(child_pid, &child_status, 0);
+
+	ret = WEXITSTATUS(child_status);
+	if (ret)
+		goto out;
+
+	snprintf(path, sizeof(path), "%s.p7s", data_template);
+
+	ret = stat(path, &st);
+	if (ret == -1) {
+		ret = -errno;
+		goto out;
+	}
+
+	if (st.st_size > sizeof(data_item->sig)) {
+		ret = -EINVAL;
+		goto out_sig;
+	}
+
+	data_item->sig_len = st.st_size;
+
+	fd = open(path, O_RDONLY);
+	if (fd == -1) {
+		ret = -errno;
+		goto out_sig;
+	}
+
+	ret = read(fd, data_item->sig, data_item->sig_len);
+
+	close(fd);
+
+	if (ret != data_item->sig_len) {
+		ret = -EIO;
+		goto out_sig;
+	}
+
+	ret = 0;
+out_sig:
+	unlink(path);
+out:
+	unlink(data_template);
+	return ret;
+}
+
+static int populate_data_item_mod(struct data *data_item)
+{
+	char mod_path[PATH_MAX], *mod_path_ptr;
+	struct stat st;
+	void *mod;
+	FILE *fp;
+	struct module_signature ms;
+	int ret, fd, modlen, marker_len, sig_len;
+
+	data_item->data_len = 0;
+
+	if (stat("/lib/modules", &st) == -1)
+		return 0;
+
+	/* Requires CONFIG_TCP_CONG_BIC=m. */
+	fp = popen("find /lib/modules/$(uname -r) -name tcp_bic.ko", "r");
+	if (!fp)
+		return 0;
+
+	mod_path_ptr = fgets(mod_path, sizeof(mod_path), fp);
+	pclose(fp);
+
+	if (!mod_path_ptr)
+		return 0;
+
+	mod_path_ptr = strchr(mod_path, '\n');
+	if (!mod_path_ptr)
+		return 0;
+
+	*mod_path_ptr = '\0';
+
+	if (stat(mod_path, &st) == -1)
+		return 0;
+
+	modlen = st.st_size;
+	marker_len = sizeof(MODULE_SIG_STRING) - 1;
+
+	fd = open(mod_path, O_RDONLY);
+	if (fd == -1)
+		return -errno;
+
+	mod = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+
+	close(fd);
+
+	if (mod == MAP_FAILED)
+		return -errno;
+
+	if (strncmp(mod + modlen - marker_len, MODULE_SIG_STRING, marker_len)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	modlen -= marker_len;
+
+	memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
+
+	sig_len = __be32_to_cpu(ms.sig_len);
+	modlen -= sig_len + sizeof(ms);
+
+	if (modlen > sizeof(data_item->data)) {
+		ret = -E2BIG;
+		goto out;
+	}
+
+	memcpy(data_item->data, mod, modlen);
+	data_item->data_len = modlen;
+
+	if (sig_len > sizeof(data_item->sig)) {
+		ret = -E2BIG;
+		goto out;
+	}
+
+	memcpy(data_item->sig, mod + modlen, sig_len);
+	data_item->sig_len = sig_len;
+	ret = 0;
+out:
+	munmap(mod, st.st_size);
+	return ret;
+}
+
+void test_verify_pkcs7_sig(void)
+{
+	libbpf_print_fn_t old_print_cb;
+	char tmp_dir_template[] = "/tmp/verify_sigXXXXXX";
+	char *tmp_dir;
+	struct test_verify_pkcs7_sig *skel = NULL;
+	struct bpf_map *map;
+	struct data data;
+	int ret, zero = 0;
+
+	/* Trigger creation of session keyring. */
+	syscall(__NR_request_key, "keyring", "_uid.0", NULL,
+		KEY_SPEC_SESSION_KEYRING);
+
+	tmp_dir = mkdtemp(tmp_dir_template);
+	if (!ASSERT_OK_PTR(tmp_dir, "mkdtemp"))
+		return;
+
+	ret = _run_setup_process(tmp_dir, "setup");
+	if (!ASSERT_OK(ret, "_run_setup_process"))
+		goto close_prog;
+
+	skel = test_verify_pkcs7_sig__open();
+	if (!ASSERT_OK_PTR(skel, "test_verify_pkcs7_sig__open"))
+		goto close_prog;
+
+	old_print_cb = libbpf_set_print(libbpf_print_cb);
+	ret = test_verify_pkcs7_sig__load(skel);
+	libbpf_set_print(old_print_cb);
+
+	if (ret < 0 && kfunc_not_supported) {
+		printf(
+		  "%s:SKIP:bpf_verify_pkcs7_signature() kfunc not supported\n",
+		  __func__);
+		test__skip();
+		goto close_prog;
+	}
+
+	if (!ASSERT_OK(ret, "test_verify_pkcs7_sig__load"))
+		goto close_prog;
+
+	ret = test_verify_pkcs7_sig__attach(skel);
+	if (!ASSERT_OK(ret, "test_verify_pkcs7_sig__attach"))
+		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;
+
+	skel->bss->monitored_pid = getpid();
+
+	/* Test without data and signature. */
+	skel->bss->user_keyring_serial = KEY_SPEC_SESSION_KEYRING;
+
+	ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &data, BPF_ANY);
+	if (!ASSERT_LT(ret, 0, "bpf_map_update_elem data_input"))
+		goto close_prog;
+
+	/* Test successful signature verification with session keyring. */
+	ret = populate_data_item_str(tmp_dir, &data);
+	if (!ASSERT_OK(ret, "populate_data_item_str"))
+		goto close_prog;
+
+	ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &data, BPF_ANY);
+	if (!ASSERT_OK(ret, "bpf_map_update_elem data_input"))
+		goto close_prog;
+
+	/* Test successful signature verification with testing keyring. */
+	skel->bss->user_keyring_serial = syscall(__NR_request_key, "keyring",
+						 "ebpf_testing_keyring", NULL,
+						 KEY_SPEC_SESSION_KEYRING);
+
+	ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &data, BPF_ANY);
+	if (!ASSERT_OK(ret, "bpf_map_update_elem data_input"))
+		goto close_prog;
+
+	/*
+	 * Ensure key_task_permission() is called and rejects the keyring
+	 * (no Search permission).
+	 */
+	syscall(__NR_keyctl, KEYCTL_SETPERM, skel->bss->user_keyring_serial,
+		0x37373737);
+
+	ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &data, BPF_ANY);
+	if (!ASSERT_LT(ret, 0, "bpf_map_update_elem data_input"))
+		goto close_prog;
+
+	syscall(__NR_keyctl, KEYCTL_SETPERM, skel->bss->user_keyring_serial,
+		0x3f3f3f3f);
+
+	/*
+	 * Ensure key_validate() is called and rejects the keyring (key expired)
+	 */
+	syscall(__NR_keyctl, KEYCTL_SET_TIMEOUT,
+		skel->bss->user_keyring_serial, 1);
+	sleep(1);
+
+	ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &data, BPF_ANY);
+	if (!ASSERT_LT(ret, 0, "bpf_map_update_elem data_input"))
+		goto close_prog;
+
+	skel->bss->user_keyring_serial = KEY_SPEC_SESSION_KEYRING;
+
+	/* Test with corrupted data (signature verification should fail). */
+	data.data[0] = 'a';
+	ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &data, BPF_ANY);
+	if (!ASSERT_LT(ret, 0, "bpf_map_update_elem data_input"))
+		goto close_prog;
+
+	ret = populate_data_item_mod(&data);
+	if (!ASSERT_OK(ret, "populate_data_item_mod"))
+		goto close_prog;
+
+	/* Test signature verification with system keyrings. */
+	if (data.data_len) {
+		skel->bss->user_keyring_serial = 0;
+		skel->bss->system_keyring_id = 0;
+
+		ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &data,
+					  BPF_ANY);
+		if (!ASSERT_OK(ret, "bpf_map_update_elem data_input"))
+			goto close_prog;
+
+		skel->bss->system_keyring_id = VERIFY_USE_SECONDARY_KEYRING;
+
+		ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &data,
+					  BPF_ANY);
+		if (!ASSERT_OK(ret, "bpf_map_update_elem data_input"))
+			goto close_prog;
+
+		skel->bss->system_keyring_id = VERIFY_USE_PLATFORM_KEYRING;
+
+		ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &data,
+					  BPF_ANY);
+		ASSERT_LT(ret, 0, "bpf_map_update_elem data_input");
+	}
+
+close_prog:
+	_run_setup_process(tmp_dir, "cleanup");
+
+	if (!skel)
+		return;
+
+	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..4ceab545d99a
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
@@ -0,0 +1,100 @@
+// 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>
+
+#define MAX_DATA_SIZE (1024 * 1024)
+#define MAX_SIG_SIZE 1024
+
+typedef __u8 u8;
+typedef __u16 u16;
+typedef __u32 u32;
+typedef __u64 u64;
+
+struct bpf_dynptr {
+	__u64 :64;
+	__u64 :64;
+} __attribute__((aligned(8)));
+
+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;
+
+struct data {
+	u8 data[MAX_DATA_SIZE];
+	u32 data_len;
+	u8 sig[MAX_SIG_SIZE];
+	u32 sig_len;
+};
+
+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";
+
+SEC("lsm.s/bpf")
+int BPF_PROG(bpf, int cmd, union bpf_attr *attr, unsigned int size)
+{
+	struct bpf_dynptr data_ptr, sig_ptr;
+	struct data *data_val;
+	struct bpf_key *trusted_keyring;
+	u32 pid;
+	u64 value;
+	int ret, zero = 0;
+
+	pid = bpf_get_current_pid_tgid() >> 32;
+	if (pid != monitored_pid)
+		return 0;
+
+	data_val = bpf_map_lookup_elem(&data_input, &zero);
+	if (!data_val)
+		return 0;
+
+	bpf_probe_read(&value, sizeof(value), &attr->value);
+
+	bpf_copy_from_user(data_val, sizeof(struct data),
+			   (void *)(unsigned long)value);
+
+	if (data_val->data_len > sizeof(data_val->data))
+		return -EINVAL;
+
+	bpf_dynptr_from_mem(data_val->data, data_val->data_len, 0, &data_ptr);
+
+	if (data_val->sig_len > sizeof(data_val->sig))
+		return -EINVAL;
+
+	bpf_dynptr_from_mem(data_val->sig, data_val->sig_len, 0, &sig_ptr);
+
+	if (user_keyring_serial)
+		trusted_keyring = bpf_lookup_user_key(user_keyring_serial, 0);
+	else
+		trusted_keyring = bpf_lookup_system_key(system_keyring_id);
+
+	if (!trusted_keyring)
+		return -ENOENT;
+
+	ret = bpf_verify_pkcs7_signature(&data_ptr, &sig_ptr, trusted_keyring);
+
+	bpf_key_put(trusted_keyring);
+
+	return ret;
+}
diff --git a/tools/testing/selftests/bpf/verify_sig_setup.sh b/tools/testing/selftests/bpf/verify_sig_setup.sh
new file mode 100755
index 000000000000..ba08922b4a27
--- /dev/null
+++ b/tools/testing/selftests/bpf/verify_sig_setup.sh
@@ -0,0 +1,104 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+set -e
+set -u
+set -o pipefail
+
+VERBOSE="${SELFTESTS_VERBOSE:=0}"
+LOG_FILE="$(mktemp /tmp/verify_sig_setup.log.XXXXXX)"
+
+x509_genkey_content="\
+[ req ]
+default_bits = 2048
+distinguished_name = req_distinguished_name
+prompt = no
+string_mask = utf8only
+x509_extensions = myexts
+
+[ req_distinguished_name ]
+CN = eBPF Signature Verification Testing Key
+
+[ myexts ]
+basicConstraints=critical,CA:FALSE
+keyUsage=digitalSignature
+subjectKeyIdentifier=hash
+authorityKeyIdentifier=keyid
+"
+
+usage()
+{
+	echo "Usage: $0 <setup|cleanup <existing_tmp_dir>"
+	exit 1
+}
+
+setup()
+{
+	local tmp_dir="$1"
+
+	echo "${x509_genkey_content}" > ${tmp_dir}/x509.genkey
+
+	openssl req -new -nodes -utf8 -sha256 -days 36500 \
+			-batch -x509 -config ${tmp_dir}/x509.genkey \
+			-outform PEM -out ${tmp_dir}/signing_key.pem \
+			-keyout ${tmp_dir}/signing_key.pem 2>&1
+
+	openssl x509 -in ${tmp_dir}/signing_key.pem -out \
+		${tmp_dir}/signing_key.der -outform der
+
+	key_id=$(cat ${tmp_dir}/signing_key.der | keyctl padd asymmetric ebpf_testing_key @s)
+
+	keyring_id=$(keyctl newring ebpf_testing_keyring @s)
+	keyctl link $key_id $keyring_id
+}
+
+cleanup() {
+	local tmp_dir="$1"
+
+	keyctl unlink $(keyctl search @s asymmetric ebpf_testing_key) @s
+	keyctl unlink $(keyctl search @s keyring ebpf_testing_keyring) @s
+	rm -rf ${tmp_dir}
+}
+
+catch()
+{
+	local exit_code="$1"
+	local log_file="$2"
+
+	if [[ "${exit_code}" -ne 0 ]]; then
+		cat "${log_file}" >&3
+	fi
+
+	rm -f "${log_file}"
+	exit ${exit_code}
+}
+
+main()
+{
+	[[ $# -ne 2 ]] && usage
+
+	local action="$1"
+	local tmp_dir="$2"
+
+	[[ ! -d "${tmp_dir}" ]] && echo "Directory ${tmp_dir} doesn't exist" && exit 1
+
+	if [[ "${action}" == "setup" ]]; then
+		setup "${tmp_dir}"
+	elif [[ "${action}" == "cleanup" ]]; then
+		cleanup "${tmp_dir}"
+	else
+		echo "Unknown action: ${action}"
+		exit 1
+	fi
+}
+
+trap 'catch "$?" "${LOG_FILE}"' EXIT
+
+if [[ "${VERBOSE}" -eq 0 ]]; then
+	# Save the stderr to 3 so that we can output back to
+	# it incase of an error.
+	exec 3>&2 1>"${LOG_FILE}" 2>&1
+fi
+
+main "$@"
+rm -f "${LOG_FILE}"
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification
  2022-08-09 13:45 [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification Roberto Sassu
                   ` (9 preceding siblings ...)
  2022-08-09 13:46 ` [PATCH v9 10/10] selftests/bpf: Add test for bpf_verify_pkcs7_signature() kfunc Roberto Sassu
@ 2022-08-09 16:20 ` patchwork-bot+netdevbpf
  2022-08-09 16:53 ` Jarkko Sakkinen
  11 siblings, 0 replies; 28+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-08-09 16:20 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, corbet, dhowells, jarkko, rostedt,
	mingo, paul, jmorris, serge, shuah, bpf, linux-doc, keyrings,
	linux-security-module, linux-kselftest, linux-kernel

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Tue, 9 Aug 2022 15:45:53 +0200 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [v9,01/10] btf: Add a new kfunc flag which allows to mark a function to be sleepable
    https://git.kernel.org/bpf/bpf-next/c/fa96b24204af
  - [v9,02/10] bpf: Allow kfuncs to be used in LSM programs
    (no matching commit)
  - [v9,03/10] btf: Handle dynamic pointer parameter in kfuncs
    (no matching commit)
  - [v9,04/10] bpf: Export bpf_dynptr_get_size()
    (no matching commit)
  - [v9,05/10] KEYS: Move KEY_LOOKUP_ to include/linux/key.h
    (no matching commit)
  - [v9,06/10] bpf: Add bpf_lookup_*_key() and bpf_key_put() kfuncs
    (no matching commit)
  - [v9,07/10] bpf: Add bpf_verify_pkcs7_signature() kfunc
    (no matching commit)
  - [v9,08/10] selftests/bpf: Add verifier tests for bpf_lookup_*_key() and bpf_key_put()
    (no matching commit)
  - [v9,09/10] selftests/bpf: Add additional tests for bpf_lookup_*_key()
    (no matching commit)
  - [v9,10/10] selftests/bpf: Add test for bpf_verify_pkcs7_signature() kfunc
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification
  2022-08-09 13:45 [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification Roberto Sassu
                   ` (10 preceding siblings ...)
  2022-08-09 16:20 ` [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification patchwork-bot+netdevbpf
@ 2022-08-09 16:53 ` Jarkko Sakkinen
  2022-08-10 10:47   ` Roberto Sassu
  11 siblings, 1 reply; 28+ messages in thread
From: Jarkko Sakkinen @ 2022-08-09 16:53 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, corbet, dhowells, rostedt, mingo,
	paul, jmorris, serge, shuah, bpf, linux-doc, keyrings,
	linux-security-module, linux-kselftest, linux-kernel

On Tue, Aug 09, 2022 at 03:45:53PM +0200, Roberto Sassu wrote:
> 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.

Security decision in LSM BPF?

> After a discussion in the eBPF mailing list, it was decided that the stated
> goal should be accomplished by introducing four new kfuncs:
> bpf_lookup_user_key() and bpf_lookup_system_key(), for retrieving a keyring
> with keys trusted for signature verification, respectively from its serial
> and from a pre-determined ID; bpf_key_put(), to release the reference
> obtained with the former two kfuncs, bpf_verify_pkcs7_signature(), for
> verifying PKCS#7 signatures.
> 
> Other than the key serial, bpf_lookup_user_key() also accepts key lookup
> flags, that influence the behavior of the lookup. bpf_lookup_system_key()
> accepts pre-determined IDs defined in include/linux/verification.h.
> 
> bpf_key_put() accepts the new bpf_key structure, introduced to tell whether
> the other structure member, a key pointer, is valid or not. The reason is
> that verify_pkcs7_signature() also accepts invalid pointers, set with the
> pre-determined ID, to select a system-defined keyring. key_put() must be
> called only for valid key pointers.
> 
> Since the two key lookup functions allocate memory and one increments a key
> reference count, they must be used in conjunction with bpf_key_put(). The
> latter must be called only if the lookup functions returned a non-NULL
> pointer. The verifier denies the execution of eBPF programs that don't
> respect this rule.
> 
> The two key lookup functions should be used in alternative, depending on
> the use case. While bpf_lookup_user_key() provides great flexibility, it
> seems suboptimal in terms of security guarantees, as even if the eBPF
> program is assumed to be trusted, the serial used to obtain the key pointer
> might come from untrusted user space not choosing one that the system
> administrator approves to enforce a mandatory policy.
> 
> bpf_lookup_system_key() instead provides much stronger guarantees,
> especially if the pre-determined ID is not passed by user space but is
> hardcoded in the eBPF program, and that program is signed. In this case,
> bpf_verify_pkcs7_signature() will always perform signature verification
> with a key that the system administrator approves, i.e. the primary,
> secondary or platform keyring.
> 
> Nevertheless, key permission checks need to be done accurately. Since
> bpf_lookup_user_key() cannot determine how a key will be used by other
> kfuncs, it has to defer the permission check to the actual kfunc using the
> key. It does it by calling lookup_user_key() with KEY_DEFER_PERM_CHECK as
> needed permission. Later, bpf_verify_pkcs7_signature(), if called,
> completes the permission check by calling key_validate(). It does not need
> to call key_task_permission() with permission KEY_NEED_SEARCH, as it is
> already done elsewhere by the key subsystem. Future kfuncs using the
> bpf_key structure need to implement the proper checks as well.
> 
> Finally, the last kfunc, bpf_verify_pkcs7_signature(), accepts the data and
> signature to verify as eBPF dynamic pointers, to minimize the number of
> kfunc parameters, and the keyring with keys for signature verification as a
> bpf_key structure, returned by one of the two key lookup functions.
> 
> All kfuncs except bpf_key_put() can be called only from sleepable programs,
> because of memory allocation and crypto operations. For example, the
> lsm.s/bpf attach point is suitable, fexit/array_map_update_elem is not.
> 
> The correctness of implementation of the new kfuncs and of their usage is
> checked with the introduced tests.
> 
> The patch set includes patches from other authors (dependencies) for sake
> of completeness. It is organized as follows.
> 
> Patch 1 from Benjamin Tissoires introduces the new KF_SLEEPABLE kfunc flag.
> Patch 2 from KP Singh allows kfuncs to be used by LSM programs. Patch 3
> allows dynamic pointers to be used as kfunc parameters. Patch 4 exports
> bpf_dynptr_get_size(), to obtain the real size of data carried by a dynamic
> pointer. Patch 5 makes available for new eBPF kfuncs some key-related
> definitions. Patch 6 introduces the bpf_lookup_*_key() and bpf_key_put()
> kfuncs. Patch 7 introduces the bpf_verify_pkcs7_signature() kfunc. Finally,
> patches 8-10 introduce the tests.
> 
> Changelog
> 
> v8:
>  - Define the new bpf_key structure to carry the key pointer and whether
>    that pointer is valid or not (suggested by Daniel)
>  - Drop patch to mark a kfunc parameter with the __maybe_null suffix
>  - Improve documentation of kfuncs
>  - Introduce bpf_lookup_system_key() to obtain a key pointer suitable for
>    verify_pkcs7_signature() (suggested by Daniel)
>  - Use the new kfunc registration API
>  - Drop patch to test the __maybe_null suffix
>  - Add tests for bpf_lookup_system_key()
> 
> v7:
>  - Add support for using dynamic and NULL pointers in kfunc (suggested by
>    Alexei)
>  - Add new kfunc-related tests
> 
> v6:
>  - Switch back to key lookup helpers + signature verification (until v5),
>    and defer permission check from bpf_lookup_user_key() to
>    bpf_verify_pkcs7_signature()
>  - Add additional key lookup test to illustrate the usage of the
>    KEY_LOOKUP_CREATE flag and validate the flags (suggested by Daniel)
>  - Make description of flags of bpf_lookup_user_key() more user-friendly
>    (suggested by Daniel)
>  - Fix validation of flags parameter in bpf_lookup_user_key() (reported by
>    Daniel)
>  - Rename bpf_verify_pkcs7_signature() keyring-related parameters to
>    user_keyring and system_keyring to make their purpose more clear
>  - Accept keyring-related parameters of bpf_verify_pkcs7_signature() as
>    alternatives (suggested by KP)
>  - Replace unsigned long type with u64 in helper declaration (suggested by
>    Daniel)
>  - Extend the bpf_verify_pkcs7_signature() test by calling the helper
>    without data, by ensuring that the helper enforces the keyring-related
>    parameters as alternatives, by ensuring that the helper rejects
>    inaccessible and expired keyrings, and by checking all system keyrings
>  - Move bpf_lookup_user_key() and bpf_key_put() usage tests to
>    ref_tracking.c (suggested by John)
>  - Call bpf_lookup_user_key() and bpf_key_put() only in sleepable programs
> 
> v5:
>  - Move KEY_LOOKUP_ to include/linux/key.h
>    for validation of bpf_verify_pkcs7_signature() parameter
>  - Remove bpf_lookup_user_key() and bpf_key_put() helpers, and the
>    corresponding tests
>  - Replace struct key parameter of bpf_verify_pkcs7_signature() with the
>    keyring serial and lookup flags
>  - Call lookup_user_key() and key_put() in bpf_verify_pkcs7_signature()
>    code, to ensure that the retrieved key is used according to the
>    permission requested at lookup time
>  - Clarified keyring precedence in the description of
>    bpf_verify_pkcs7_signature() (suggested by John)
>  - Remove newline in the second argument of ASSERT_
>  - Fix helper prototype regular expression in bpf_doc.py
> 
> v4:
>  - Remove bpf_request_key_by_id(), don't return an invalid pointer that
>    other helpers can use
>  - Pass the keyring ID (without ULONG_MAX, suggested by Alexei) to
>    bpf_verify_pkcs7_signature()
>  - Introduce bpf_lookup_user_key() and bpf_key_put() helpers (suggested by
>    Alexei)
>  - Add lookup_key_norelease test, to ensure that the verifier blocks eBPF
>    programs which don't decrement the key reference count
>  - Parse raw PKCS#7 signature instead of module-style signature in the
>    verify_pkcs7_signature test (suggested by Alexei)
>  - Parse kernel module in user space and pass raw PKCS#7 signature to the
>    eBPF program for signature verification
> 
> v3:
>  - Rename bpf_verify_signature() back to bpf_verify_pkcs7_signature() to
>    avoid managing different parameters for each signature verification
>    function in one helper (suggested by Daniel)
>  - Use dynamic pointers and export bpf_dynptr_get_size() (suggested by
>    Alexei)
>  - Introduce bpf_request_key_by_id() to give more flexibility to the caller
>    of bpf_verify_pkcs7_signature() to retrieve the appropriate keyring
>    (suggested by Alexei)
>  - Fix test by reordering the gcc command line, always compile sign-file
>  - Improve helper support check mechanism in the test
> 
> v2:
>  - Rename bpf_verify_pkcs7_signature() to a more generic
>    bpf_verify_signature() and pass the signature type (suggested by KP)
>  - Move the helper and prototype declaration under #ifdef so that user
>    space can probe for support for the helper (suggested by Daniel)
>  - Describe better the keyring types (suggested by Daniel)
>  - Include linux/bpf.h instead of vmlinux.h to avoid implicit or
>    redeclaration
>  - Make the test selfcontained (suggested by Alexei)
> 
> v1:
>  - Don't define new map flag but introduce simple wrapper of
>    verify_pkcs7_signature() (suggested by Alexei and KP)
> 
> Benjamin Tissoires (1):
>   btf: Add a new kfunc flag which allows to mark a function to be
>     sleepable
> 
> KP Singh (1):
>   bpf: Allow kfuncs to be used in LSM programs
> 
> Roberto Sassu (8):
>   btf: Handle dynamic pointer parameter in kfuncs
>   bpf: Export bpf_dynptr_get_size()
>   KEYS: Move KEY_LOOKUP_ to include/linux/key.h
>   bpf: Add bpf_lookup_*_key() and bpf_key_put() kfuncs
>   bpf: Add bpf_verify_pkcs7_signature() kfunc
>   selftests/bpf: Add verifier tests for bpf_lookup_*_key() and
>     bpf_key_put()
>   selftests/bpf: Add additional tests for bpf_lookup_*_key()
>   selftests/bpf: Add test for bpf_verify_pkcs7_signature() kfunc
> 
>  Documentation/bpf/kfuncs.rst                  |   6 +
>  include/linux/bpf.h                           |   7 +
>  include/linux/bpf_verifier.h                  |   3 +
>  include/linux/btf.h                           |   1 +
>  include/linux/key.h                           |   3 +
>  kernel/bpf/btf.c                              |  27 ++
>  kernel/bpf/helpers.c                          |   2 +-
>  kernel/bpf/verifier.c                         |   4 +-
>  kernel/trace/bpf_trace.c                      | 207 +++++++++
>  security/keys/internal.h                      |   2 -
>  tools/testing/selftests/bpf/Makefile          |  14 +-
>  tools/testing/selftests/bpf/config            |   2 +
>  .../selftests/bpf/prog_tests/lookup_key.c     | 112 +++++
>  .../bpf/prog_tests/verify_pkcs7_sig.c         | 399 ++++++++++++++++++
>  .../selftests/bpf/progs/test_lookup_key.c     |  46 ++
>  .../bpf/progs/test_verify_pkcs7_sig.c         | 100 +++++
>  tools/testing/selftests/bpf/test_verifier.c   |   3 +-
>  .../selftests/bpf/verifier/ref_tracking.c     | 139 ++++++
>  .../testing/selftests/bpf/verify_sig_setup.sh | 104 +++++
>  19 files changed, 1172 insertions(+), 9 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/lookup_key.c
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_lookup_key.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
>  create mode 100755 tools/testing/selftests/bpf/verify_sig_setup.sh
> 
> -- 
> 2.25.1

BR, Jarkko

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v9 01/10] btf: Add a new kfunc flag which allows to mark a function to be sleepable
  2022-08-09 13:45 ` [PATCH v9 01/10] btf: Add a new kfunc flag which allows to mark a function to be sleepable Roberto Sassu
@ 2022-08-09 16:54   ` Jarkko Sakkinen
  2022-08-10 13:44     ` Roberto Sassu
  0 siblings, 1 reply; 28+ messages in thread
From: Jarkko Sakkinen @ 2022-08-09 16:54 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, corbet, dhowells, rostedt, mingo,
	paul, jmorris, serge, shuah, bpf, linux-doc, keyrings,
	linux-security-module, linux-kselftest, linux-kernel,
	Benjamin Tissoires, Yosry Ahmed

On Tue, Aug 09, 2022 at 03:45:54PM +0200, Roberto Sassu wrote:
> From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> This allows to declare a kfunc as sleepable and prevents its use in
> a non sleepable program.

Nit: "Declare a kfunc as sleepable and prevent its use in a
non-sleepable program."

It's missing the part *how* the patch accomplishes its goals.

> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Co-developed-by: Yosry Ahmed <yosryahmed@google.com>
> Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
> Signed-off-by: Hao Luo <haoluo@google.com>
> ---
>  Documentation/bpf/kfuncs.rst | 6 ++++++
>  include/linux/btf.h          | 1 +
>  kernel/bpf/btf.c             | 9 +++++++++
>  3 files changed, 16 insertions(+)
> 
> diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> index c0b7dae6dbf5..c8b21de1c772 100644
> --- a/Documentation/bpf/kfuncs.rst
> +++ b/Documentation/bpf/kfuncs.rst
> @@ -146,6 +146,12 @@ that operate (change some property, perform some operation) on an object that
>  was obtained using an acquire kfunc. Such kfuncs need an unchanged pointer to
>  ensure the integrity of the operation being performed on the expected object.
>  
> +2.4.6 KF_SLEEPABLE flag
> +-----------------------
> +
> +The KF_SLEEPABLE flag is used for kfuncs that may sleep. Such kfuncs can only
> +be called by sleepable BPF programs (BPF_F_SLEEPABLE).
> +
>  2.5 Registering the kfuncs
>  --------------------------
>  
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index cdb376d53238..976cbdd2981f 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -49,6 +49,7 @@
>   * for this case.
>   */
>  #define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer arguments */
> +#define KF_SLEEPABLE   (1 << 5) /* kfunc may sleep */
>  
>  struct btf;
>  struct btf_member;
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 7e64447659f3..d3e4c86b8fcd 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6175,6 +6175,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  {
>  	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
>  	bool rel = false, kptr_get = false, trusted_arg = false;
> +	bool sleepable = false;
>  	struct bpf_verifier_log *log = &env->log;
>  	u32 i, nargs, ref_id, ref_obj_id = 0;
>  	bool is_kfunc = btf_is_kernel(btf);
> @@ -6212,6 +6213,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  		rel = kfunc_flags & KF_RELEASE;
>  		kptr_get = kfunc_flags & KF_KPTR_GET;
>  		trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
> +		sleepable = kfunc_flags & KF_SLEEPABLE;
>  	}
>  
>  	/* check that BTF function arguments match actual types that the
> @@ -6419,6 +6421,13 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  			func_name);
>  		return -EINVAL;
>  	}
> +
> +	if (sleepable && !env->prog->aux->sleepable) {
> +		bpf_log(log, "kernel function %s is sleepable but the program is not\n",
> +			func_name);
> +		return -EINVAL;
> +	}
> +
>  	/* returns argument register number > 0 in case of reference release kfunc */
>  	return rel ? ref_regno : 0;
>  }
> -- 
> 2.25.1
> 

BR, Jarkko

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v9 02/10] bpf: Allow kfuncs to be used in LSM programs
  2022-08-09 13:45 ` [PATCH v9 02/10] bpf: Allow kfuncs to be used in LSM programs Roberto Sassu
@ 2022-08-09 21:53   ` Daniel Borkmann
  0 siblings, 0 replies; 28+ messages in thread
From: Daniel Borkmann @ 2022-08-09 21:53 UTC (permalink / raw)
  To: Roberto Sassu, ast, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, corbet, dhowells,
	jarkko, rostedt, mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel

On 8/9/22 3:45 PM, Roberto Sassu wrote:
> From: KP Singh <kpsingh@kernel.org>
> 
> In preparation for the addition of bpf_getxattr kfunc.

Given this is taken out of context and the series is not about bpf_getxattr,
this patch should have a better commit description.

> Signed-off-by: KP Singh <kpsingh@kernel.org>

Please also append your SoB then [0].

   [0] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#developer-s-certificate-of-origin-1-1

> ---
>   kernel/bpf/btf.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index d3e4c86b8fcd..67dfc728fbf8 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -7244,6 +7244,7 @@ static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type)
>   	case BPF_PROG_TYPE_STRUCT_OPS:
>   		return BTF_KFUNC_HOOK_STRUCT_OPS;
>   	case BPF_PROG_TYPE_TRACING:
> +	case BPF_PROG_TYPE_LSM:
>   		return BTF_KFUNC_HOOK_TRACING;
>   	case BPF_PROG_TYPE_SYSCALL:
>   		return BTF_KFUNC_HOOK_SYSCALL;
> 


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v9 03/10] btf: Handle dynamic pointer parameter in kfuncs
  2022-08-09 13:45 ` [PATCH v9 03/10] btf: Handle dynamic pointer parameter in kfuncs Roberto Sassu
@ 2022-08-09 22:08   ` Daniel Borkmann
  2022-08-09 22:29   ` Daniel Borkmann
  1 sibling, 0 replies; 28+ messages in thread
From: Daniel Borkmann @ 2022-08-09 22:08 UTC (permalink / raw)
  To: Roberto Sassu, ast, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, corbet, dhowells,
	jarkko, rostedt, mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel, Joanne Koong

On 8/9/22 3:45 PM, Roberto Sassu wrote:
> Allow the bpf_dynptr_kern parameter to be specified in kfuncs. Also, ensure
> that the dynamic pointer is valid and initialized.
> 
> Cc: Joanne Koong <joannelkoong@gmail.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>   include/linux/bpf_verifier.h |  3 +++
>   kernel/bpf/btf.c             | 17 +++++++++++++++++
>   kernel/bpf/verifier.c        |  4 ++--
>   3 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 2e3bad8640dc..55876fbdbae2 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -560,6 +560,9 @@ int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state
>   			     u32 regno);
>   int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
>   		   u32 regno, u32 mem_size);
> +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env,
> +			      struct bpf_reg_state *reg,
> +			      enum bpf_arg_type arg_type);
>   
>   /* this lives here instead of in bpf.h because it needs to dereference tgt_prog */
>   static inline u64 bpf_trampoline_compute_key(const struct bpf_prog *tgt_prog,
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 67dfc728fbf8..17cca396c89f 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6363,6 +6363,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>   
>   			if (is_kfunc) {
>   				bool arg_mem_size = i + 1 < nargs && is_kfunc_arg_mem_size(btf, &args[i + 1], &regs[regno + 1]);
> +				bool arg_dynptr = btf_type_is_struct(ref_t) &&
> +						  !strcmp(ref_tname, "bpf_dynptr_kern");

For the strcmp(), could we add some BUILD_BUG_ON() if "bpf_dynptr_kern" ever
gets renamed? I played a bit with the below compile-tested toy example. If we
rename foo_bar into something else, we then get:

   [...]
   CC      net/core/dev.o
   In file included from <command-line>:
   net/core/dev.c: In function ‘net_dev_init’:
   net/core/dev.c:11376:25: error: invalid application of ‘sizeof’ to incomplete type ‘struct foo_bar’
   11376 |  ({ BUILD_BUG_ON(sizeof(struct x) < 0); \
         |                         ^~~~~~
   [...]

diff --git a/net/core/dev.c b/net/core/dev.c
index 716df64fcfa5..a2ed271f7ded 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -11368,16 +11368,30 @@ static struct pernet_operations __net_initdata default_device_ops = {
   *
   */

+struct foo_bar {
+       int a;
+};
+
+#define stringify_struct(x)                    \
+       ({ BUILD_BUG_ON(sizeof(struct x) < 0);  \
+       __stringify(x); })
+
  /*
   *       This is called single threaded during boot, so no need
   *       to take the rtnl semaphore.
   */
  static int __init net_dev_init(void)
  {
+       const char *ref_tname = "abc";
         int i, rc = -ENOMEM;

         BUG_ON(!dev_boot_phase);

+       printk("%s\n", stringify_struct(foo_bar));
+
+       if (!strcmp(ref_tname, stringify_struct(foo_bar)))
+               goto out;
+
         if (dev_proc_init())
                 goto out;


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v9 03/10] btf: Handle dynamic pointer parameter in kfuncs
  2022-08-09 13:45 ` [PATCH v9 03/10] btf: Handle dynamic pointer parameter in kfuncs Roberto Sassu
  2022-08-09 22:08   ` Daniel Borkmann
@ 2022-08-09 22:29   ` Daniel Borkmann
  1 sibling, 0 replies; 28+ messages in thread
From: Daniel Borkmann @ 2022-08-09 22:29 UTC (permalink / raw)
  To: Roberto Sassu, ast, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, corbet, dhowells,
	jarkko, rostedt, mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel, Joanne Koong

On 8/9/22 3:45 PM, Roberto Sassu wrote:
[...]
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 67dfc728fbf8..17cca396c89f 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6363,6 +6363,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>   
>   			if (is_kfunc) {
>   				bool arg_mem_size = i + 1 < nargs && is_kfunc_arg_mem_size(btf, &args[i + 1], &regs[regno + 1]);
> +				bool arg_dynptr = btf_type_is_struct(ref_t) &&
> +						  !strcmp(ref_tname, "bpf_dynptr_kern");
>   
>   				/* Permit pointer to mem, but only when argument
>   				 * type is pointer to scalar, or struct composed
> @@ -6372,6 +6374,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>   				 */
>   				if (!btf_type_is_scalar(ref_t) &&
>   				    !__btf_type_is_scalar_struct(log, btf, ref_t, 0) &&
> +				    !arg_dynptr &&
>   				    (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) {
>   					bpf_log(log,
>   						"arg#%d pointer type %s %s must point to %sscalar, or struct with scalar\n",
> @@ -6379,6 +6382,20 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>   					return -EINVAL;
>   				}
>   
> +				/* Assume initialized dynptr. */

This comment is a bit misleading, too, given we don't assume but enforce it. I'd probably
just fold this into above one where we permit pointer to mem given the test there gets
extended anyway, so the comment should be in line with the tests.

> +				if (arg_dynptr) {
> +					if (!is_dynptr_reg_valid_init(env, reg,
> +							ARG_PTR_TO_DYNPTR)) {
> +						bpf_log(log,
> +							"arg#%d pointer type %s %s must be initialized\n",
> +							i, btf_type_str(ref_t),
> +							ref_tname);
> +						return -EINVAL;
> +					}
> +
> +					continue;
> +				}
> +
>   				/* Check for mem, len pair */
>   				if (arg_mem_size) {
>   					if (check_kfunc_mem_size_reg(env, &regs[regno + 1], regno + 1)) {


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v9 06/10] bpf: Add bpf_lookup_*_key() and bpf_key_put() kfuncs
  2022-08-09 13:45 ` [PATCH v9 06/10] bpf: Add bpf_lookup_*_key() and bpf_key_put() kfuncs Roberto Sassu
@ 2022-08-09 22:53   ` Daniel Borkmann
  2022-08-10 14:17     ` Roberto Sassu
  0 siblings, 1 reply; 28+ messages in thread
From: Daniel Borkmann @ 2022-08-09 22:53 UTC (permalink / raw)
  To: Roberto Sassu, ast, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, corbet, dhowells,
	jarkko, rostedt, mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel

On 8/9/22 3:45 PM, Roberto Sassu wrote:
> Add the bpf_lookup_user_key(), bpf_lookup_system_key() and bpf_key_put()
> kfuncs, to respectively search a key with a given serial and flags, obtain
> a key from a pre-determined ID defined in include/linux/verification.h, and
> cleanup.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>   include/linux/bpf.h      |   6 ++
>   kernel/trace/bpf_trace.c | 151 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 157 insertions(+)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 0d56c23cc504..564b9e0b8c16 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2572,4 +2572,10 @@ static inline void bpf_cgroup_atype_get(u32 attach_btf_id, int cgroup_atype) {}
>   static inline void bpf_cgroup_atype_put(int cgroup_atype) {}
>   #endif /* CONFIG_BPF_LSM */
>   
> +#ifdef CONFIG_KEYS
> +struct bpf_key {
> +	struct key *key;
> +	bool valid_ptr;
> +};
> +#endif /* CONFIG_KEYS */
>   #endif /* _LINUX_BPF_H */
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 68e5cdd24cef..33ca4cfe6e26 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -20,6 +20,7 @@
>   #include <linux/fprobe.h>
>   #include <linux/bsearch.h>
>   #include <linux/sort.h>
> +#include <linux/key.h>
>   
>   #include <net/bpf_sk_storage.h>
>   
> @@ -1181,6 +1182,156 @@ static const struct bpf_func_proto bpf_get_func_arg_cnt_proto = {
>   	.arg1_type	= ARG_PTR_TO_CTX,
>   };
>   
> +#ifdef CONFIG_KEYS
> +__diag_push();
> +__diag_ignore_all("-Wmissing-prototypes",
> +		  "kfuncs which will be used in BPF programs");
> +
> +/**
> + * bpf_lookup_user_key - lookup a key by its serial
> + * @serial: key serial
> + * @flags: lookup-specific flags
> + *
> + * Search a key with a given *serial* and the provided *flags*. The
> + * returned key, if found, has the reference count incremented by
> + * one, and is stored in a bpf_key structure, returned to the caller.
> + * The bpf_key structure must be passed to bpf_key_put() when done
> + * with it, so that the key reference count is decremented and the
> + * bpf_key structure is freed.
> + *
> + * Permission checks are deferred to the time the key is used by
> + * one of the available key-specific kfuncs.
> + *
> + * Set *flags* with 1, to attempt creating a requested special
> + * keyring (e.g. session keyring), if it doesn't yet exist. Set
> + * *flags* with 2 to lookup a key without waiting for the key
> + * construction, and to retrieve uninstantiated keys (keys without
> + * data attached to them).
> + *
> + * Return: a bpf_key pointer with a valid key pointer if the key is found, a
> + *         NULL pointer otherwise.
> + */
> +noinline __weak struct bpf_key *bpf_lookup_user_key(u32 serial, u64 flags)

Why the need for noinline and the __weak here and below? (If indeed needed this
should probably be explained in the commit desc.)

> +{
> +	key_ref_t key_ref;
> +	struct bpf_key *bkey;
> +
> +	/* Keep in sync with include/linux/key.h. */
> +	if (flags > (KEY_LOOKUP_PARTIAL << 1) - 1)

Can't we just simplify and test flags & ~(KEY_LOOKUP_CREATE|KEY_LOOKUP_PARTIAL)?

> +		return NULL;
> +
> +	/* Permission check is deferred until actual kfunc using the key. */
> +	key_ref = lookup_user_key(serial, flags, KEY_DEFER_PERM_CHECK);
> +	if (IS_ERR(key_ref))
> +		return NULL;
> +
> +	bkey = kmalloc(sizeof(*bkey), GFP_KERNEL);
> +	if (!bkey) {
> +		key_put(key_ref_to_ptr(key_ref));
> +		return bkey;

nit: just return NULL probably cleaner

> +	}
> +
> +	bkey->key = key_ref_to_ptr(key_ref);
> +	bkey->valid_ptr = true;

nit: I'd probably rename s/valid_ptr/has_ref/.

> +	return bkey;
> +}
> +
> +/**
> + * bpf_lookup_system_key - lookup a key by a system-defined ID
> + * @id: key ID
> + *
> + * Obtain a bpf_key structure with a key pointer set to the passed key ID.
> + * The key pointer is marked as invalid, to prevent bpf_key_put() from
> + * attempting to decrement the key reference count on that pointer. The key
> + * pointer set in such way is currently understood only by
> + * verify_pkcs7_signature().
> + *
> + * Set *id* to one of the values defined in include/linux/verification.h:
> + * 0 for the primary keyring (immutable keyring of system keys); 1 for both
> + * the primary and secondary keyring (where keys can be added only if they
> + * are vouched for by existing keys in those keyrings); 2 for the platform
> + * keyring (primarily used by the integrity subsystem to verify a kexec'ed
> + * kerned image and, possibly, the initramfs signature).
> + *
> + * Return: a bpf_key pointer with an invalid key pointer set from the
> + *         pre-determined ID on success, a NULL pointer otherwise
> + */
> +noinline __weak struct bpf_key *bpf_lookup_system_key(u64 id)
> +{
> +	struct bpf_key *bkey;
> +
> +	/* Keep in sync with defs in include/linux/verification.h. */
> +	if (id > (unsigned long)VERIFY_USE_PLATFORM_KEYRING)
> +		return NULL;
> +
> +	bkey = kmalloc(sizeof(*bkey), GFP_KERNEL);

nit: Can't this be GFP_ATOMIC? Then bpf_lookup_system_key doesn't need KF_SLEEPABLE
attribute, fwiw. Overall, the bpf_lookup_{system,user}_key() look reasonable.

> +	if (!bkey)
> +		return bkey;
> +
> +	bkey->key = (struct key *)(unsigned long)id;
> +	bkey->valid_ptr = false;
> +
> +	return bkey;
> +}
> +
> +/**
> + * bpf_key_put - decrement key reference count if key is valid and free bpf_key
> + * @bkey: bpf_key structure
> + *
> + * Decrement the reference count of the key inside *bkey*, if the pointer
> + * is valid, and free *bkey*.
> + */
> +noinline __weak void bpf_key_put(struct bpf_key *bkey)
> +{
> +	if (bkey->valid_ptr)
> +		key_put(bkey->key);
> +
> +	kfree(bkey);
> +}
> +
> +__diag_pop();
> +
> +BTF_SET8_START(key_kfunc_set)
> +BTF_ID_FLAGS(func, bpf_lookup_user_key, KF_ACQUIRE | KF_RET_NULL | KF_SLEEPABLE)
> +BTF_ID_FLAGS(func, bpf_lookup_system_key,
> +	     KF_ACQUIRE | KF_RET_NULL | KF_SLEEPABLE)
> +BTF_ID_FLAGS(func, bpf_key_put, KF_RELEASE)
> +BTF_SET8_END(key_kfunc_set)
> +
> +static const struct btf_kfunc_id_set bpf_key_kfunc_set = {
> +	.owner = THIS_MODULE,
> +	.set = &key_kfunc_set,
> +};
> +#endif /* CONFIG_KEYS */
> +
> +const struct btf_kfunc_id_set *kfunc_sets[] = {
> +#ifdef CONFIG_KEYS
> +	&bpf_key_kfunc_set,
> +#endif /* CONFIG_KEYS */
> +};

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v9 07/10] bpf: Add bpf_verify_pkcs7_signature() kfunc
  2022-08-09 13:46 ` [PATCH v9 07/10] bpf: Add bpf_verify_pkcs7_signature() kfunc Roberto Sassu
@ 2022-08-09 23:09   ` Daniel Borkmann
  2022-08-10  2:41   ` Alexei Starovoitov
  1 sibling, 0 replies; 28+ messages in thread
From: Daniel Borkmann @ 2022-08-09 23:09 UTC (permalink / raw)
  To: Roberto Sassu, ast, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, corbet, dhowells,
	jarkko, rostedt, mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel

On 8/9/22 3:46 PM, Roberto Sassu wrote:
[...]
> For bpf_key structures obtained from the former lookup function,
> bpf_verify_pkcs7_signature() completes the permission check deferred by
> that function by calling key_validate(). key_task_permission() is already
> called by the PKCS#7 code.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>   kernel/trace/bpf_trace.c | 56 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 56 insertions(+)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 33ca4cfe6e26..79ba8c96735a 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -21,6 +21,7 @@
>   #include <linux/bsearch.h>
>   #include <linux/sort.h>
>   #include <linux/key.h>
> +#include <linux/verification.h>
>   
>   #include <net/bpf_sk_storage.h>
>   
> @@ -1290,6 +1291,47 @@ noinline __weak void bpf_key_put(struct bpf_key *bkey)
>   	kfree(bkey);
>   }
>   
> +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> +/**
> + * bpf_verify_pkcs7_signature - verify a PKCS#7 signature
> + * @data_ptr: data to verify
> + * @sig_ptr: signature of the data
> + * @trusted_keyring: keyring with keys trusted for signature verification
> + *
> + * Verify the PKCS#7 signature *sig_ptr* against the supplied *data_ptr*
> + * with keys in a keyring referenced by *trusted_keyring*.
> + *
> + * Return: 0 on success, a negative value on error.
> + */
> +noinline __weak int bpf_verify_pkcs7_signature(struct bpf_dynptr_kern *data_ptr,

nit: see comment on prev patch for noinline and __weak

> +					       struct bpf_dynptr_kern *sig_ptr,
> +					       struct bpf_key *trusted_keyring)
> +{
> +	int ret;
> +
> +	if (trusted_keyring->valid_ptr) {
> +		/*
> +		 * Do the permission check deferred in bpf_lookup_user_key().
> +		 *
> +		 * A call to key_task_permission() here would be redundant, as
> +		 * it is already done by keyring_search() called by
> +		 * find_asymmetric_key().
> +		 */

In this patch and previous one, you describe that calling key_validate() is
deferred, but you should also provide the actual rationale for readers on
"why" we need to do it.

> +		ret = key_validate(trusted_keyring->key);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return verify_pkcs7_signature(data_ptr->data,
> +				      bpf_dynptr_get_size(data_ptr),
> +				      sig_ptr->data,
> +				      bpf_dynptr_get_size(sig_ptr),
> +				      trusted_keyring->key,
> +				      VERIFYING_UNSPECIFIED_SIGNATURE, NULL,
> +				      NULL);
> +}
> +#endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
> +
>   __diag_pop();
>   
>   BTF_SET8_START(key_kfunc_set)
> @@ -1303,11 +1345,25 @@ static const struct btf_kfunc_id_set bpf_key_kfunc_set = {
>   	.owner = THIS_MODULE,
>   	.set = &key_kfunc_set,
>   };

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v9 07/10] bpf: Add bpf_verify_pkcs7_signature() kfunc
  2022-08-09 13:46 ` [PATCH v9 07/10] bpf: Add bpf_verify_pkcs7_signature() kfunc Roberto Sassu
  2022-08-09 23:09   ` Daniel Borkmann
@ 2022-08-10  2:41   ` Alexei Starovoitov
  1 sibling, 0 replies; 28+ messages in thread
From: Alexei Starovoitov @ 2022-08-10  2:41 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, corbet, dhowells, jarkko, rostedt,
	mingo, paul, jmorris, serge, shuah, bpf, linux-doc, keyrings,
	linux-security-module, linux-kselftest, linux-kernel

On Tue, Aug 09, 2022 at 03:46:00PM +0200, Roberto Sassu wrote:
> Add the bpf_verify_pkcs7_signature() kfunc, to give eBPF security modules
> the ability to check the validity of a signature against supplied data, by
> using user-provided or system-provided keys as trust anchor.
> 
> The new kfunc makes it possible to enforce mandatory policies, as eBPF
> programs might be allowed to make security decisions only based on data
> sources the system administrator approves.
> 
> The caller should provide the data to be verified and the signature as eBPF
> dynamic pointers (to minimize the number of parameters) and a bpf_key
> structure containing a reference to the keyring with keys trusted for
> signature verification, obtained from bpf_lookup_user_key() or
> bpf_lookup_system_key().
> 
> For bpf_key structures obtained from the former lookup function,
> bpf_verify_pkcs7_signature() completes the permission check deferred by
> that function by calling key_validate(). key_task_permission() is already
> called by the PKCS#7 code.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  kernel/trace/bpf_trace.c | 56 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 33ca4cfe6e26..79ba8c96735a 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -21,6 +21,7 @@
>  #include <linux/bsearch.h>
>  #include <linux/sort.h>
>  #include <linux/key.h>
> +#include <linux/verification.h>
>  
>  #include <net/bpf_sk_storage.h>
>  
> @@ -1290,6 +1291,47 @@ noinline __weak void bpf_key_put(struct bpf_key *bkey)
>  	kfree(bkey);
>  }
>  
> +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> +/**
> + * bpf_verify_pkcs7_signature - verify a PKCS#7 signature
> + * @data_ptr: data to verify
> + * @sig_ptr: signature of the data
> + * @trusted_keyring: keyring with keys trusted for signature verification
> + *
> + * Verify the PKCS#7 signature *sig_ptr* against the supplied *data_ptr*
> + * with keys in a keyring referenced by *trusted_keyring*.
> + *
> + * Return: 0 on success, a negative value on error.
> + */
> +noinline __weak int bpf_verify_pkcs7_signature(struct bpf_dynptr_kern *data_ptr,
> +					       struct bpf_dynptr_kern *sig_ptr,
> +					       struct bpf_key *trusted_keyring)
> +{
> +	int ret;
> +
> +	if (trusted_keyring->valid_ptr) {
> +		/*
> +		 * Do the permission check deferred in bpf_lookup_user_key().
> +		 *
> +		 * A call to key_task_permission() here would be redundant, as
> +		 * it is already done by keyring_search() called by
> +		 * find_asymmetric_key().
> +		 */
> +		ret = key_validate(trusted_keyring->key);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return verify_pkcs7_signature(data_ptr->data,
> +				      bpf_dynptr_get_size(data_ptr),
> +				      sig_ptr->data,
> +				      bpf_dynptr_get_size(sig_ptr),
> +				      trusted_keyring->key,
> +				      VERIFYING_UNSPECIFIED_SIGNATURE, NULL,
> +				      NULL);
> +}
> +#endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
> +
>  __diag_pop();
>  
>  BTF_SET8_START(key_kfunc_set)
> @@ -1303,11 +1345,25 @@ static const struct btf_kfunc_id_set bpf_key_kfunc_set = {
>  	.owner = THIS_MODULE,
>  	.set = &key_kfunc_set,
>  };
> +
> +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> +BTF_SET8_START(verify_sig_kfunc_set)
> +BTF_ID_FLAGS(func, bpf_verify_pkcs7_signature, KF_SLEEPABLE)
> +BTF_SET8_END(verify_sig_kfunc_set)
> +
> +static const struct btf_kfunc_id_set bpf_verify_sig_kfunc_set = {
> +	.owner = THIS_MODULE,
> +	.set = &verify_sig_kfunc_set,
> +};
> +#endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
>  #endif /* CONFIG_KEYS */
>  
>  const struct btf_kfunc_id_set *kfunc_sets[] = {
>  #ifdef CONFIG_KEYS
>  	&bpf_key_kfunc_set,
> +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> +	&bpf_verify_sig_kfunc_set,
> +#endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
>  #endif /* CONFIG_KEYS */
>  };

Why different sets?
The loop over the set from the previous patch can be removed if it's just one set.
Each kfunc can be ifdef-ed independently.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification
  2022-08-09 16:53 ` Jarkko Sakkinen
@ 2022-08-10 10:47   ` Roberto Sassu
  0 siblings, 0 replies; 28+ messages in thread
From: Roberto Sassu @ 2022-08-10 10:47 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, corbet, dhowells, rostedt, mingo,
	paul, jmorris, serge, shuah, bpf, linux-doc, keyrings,
	linux-security-module, linux-kselftest, linux-kernel

> From: Jarkko Sakkinen [mailto:jarkko@kernel.org]
> Sent: Tuesday, August 9, 2022 6:53 PM
> On Tue, Aug 09, 2022 at 03:45:53PM +0200, Roberto Sassu wrote:
> > 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.
> 
> Security decision in LSM BPF?

Yes, based on the eBPF programs attached to it.

Roberto

> > After a discussion in the eBPF mailing list, it was decided that the stated
> > goal should be accomplished by introducing four new kfuncs:
> > bpf_lookup_user_key() and bpf_lookup_system_key(), for retrieving a keyring
> > with keys trusted for signature verification, respectively from its serial
> > and from a pre-determined ID; bpf_key_put(), to release the reference
> > obtained with the former two kfuncs, bpf_verify_pkcs7_signature(), for
> > verifying PKCS#7 signatures.
> >
> > Other than the key serial, bpf_lookup_user_key() also accepts key lookup
> > flags, that influence the behavior of the lookup. bpf_lookup_system_key()
> > accepts pre-determined IDs defined in include/linux/verification.h.
> >
> > bpf_key_put() accepts the new bpf_key structure, introduced to tell whether
> > the other structure member, a key pointer, is valid or not. The reason is
> > that verify_pkcs7_signature() also accepts invalid pointers, set with the
> > pre-determined ID, to select a system-defined keyring. key_put() must be
> > called only for valid key pointers.
> >
> > Since the two key lookup functions allocate memory and one increments a key
> > reference count, they must be used in conjunction with bpf_key_put(). The
> > latter must be called only if the lookup functions returned a non-NULL
> > pointer. The verifier denies the execution of eBPF programs that don't
> > respect this rule.
> >
> > The two key lookup functions should be used in alternative, depending on
> > the use case. While bpf_lookup_user_key() provides great flexibility, it
> > seems suboptimal in terms of security guarantees, as even if the eBPF
> > program is assumed to be trusted, the serial used to obtain the key pointer
> > might come from untrusted user space not choosing one that the system
> > administrator approves to enforce a mandatory policy.
> >
> > bpf_lookup_system_key() instead provides much stronger guarantees,
> > especially if the pre-determined ID is not passed by user space but is
> > hardcoded in the eBPF program, and that program is signed. In this case,
> > bpf_verify_pkcs7_signature() will always perform signature verification
> > with a key that the system administrator approves, i.e. the primary,
> > secondary or platform keyring.
> >
> > Nevertheless, key permission checks need to be done accurately. Since
> > bpf_lookup_user_key() cannot determine how a key will be used by other
> > kfuncs, it has to defer the permission check to the actual kfunc using the
> > key. It does it by calling lookup_user_key() with KEY_DEFER_PERM_CHECK as
> > needed permission. Later, bpf_verify_pkcs7_signature(), if called,
> > completes the permission check by calling key_validate(). It does not need
> > to call key_task_permission() with permission KEY_NEED_SEARCH, as it is
> > already done elsewhere by the key subsystem. Future kfuncs using the
> > bpf_key structure need to implement the proper checks as well.
> >
> > Finally, the last kfunc, bpf_verify_pkcs7_signature(), accepts the data and
> > signature to verify as eBPF dynamic pointers, to minimize the number of
> > kfunc parameters, and the keyring with keys for signature verification as a
> > bpf_key structure, returned by one of the two key lookup functions.
> >
> > All kfuncs except bpf_key_put() can be called only from sleepable programs,
> > because of memory allocation and crypto operations. For example, the
> > lsm.s/bpf attach point is suitable, fexit/array_map_update_elem is not.
> >
> > The correctness of implementation of the new kfuncs and of their usage is
> > checked with the introduced tests.
> >
> > The patch set includes patches from other authors (dependencies) for sake
> > of completeness. It is organized as follows.
> >
> > Patch 1 from Benjamin Tissoires introduces the new KF_SLEEPABLE kfunc flag.
> > Patch 2 from KP Singh allows kfuncs to be used by LSM programs. Patch 3
> > allows dynamic pointers to be used as kfunc parameters. Patch 4 exports
> > bpf_dynptr_get_size(), to obtain the real size of data carried by a dynamic
> > pointer. Patch 5 makes available for new eBPF kfuncs some key-related
> > definitions. Patch 6 introduces the bpf_lookup_*_key() and bpf_key_put()
> > kfuncs. Patch 7 introduces the bpf_verify_pkcs7_signature() kfunc. Finally,
> > patches 8-10 introduce the tests.
> >
> > Changelog
> >
> > v8:
> >  - Define the new bpf_key structure to carry the key pointer and whether
> >    that pointer is valid or not (suggested by Daniel)
> >  - Drop patch to mark a kfunc parameter with the __maybe_null suffix
> >  - Improve documentation of kfuncs
> >  - Introduce bpf_lookup_system_key() to obtain a key pointer suitable for
> >    verify_pkcs7_signature() (suggested by Daniel)
> >  - Use the new kfunc registration API
> >  - Drop patch to test the __maybe_null suffix
> >  - Add tests for bpf_lookup_system_key()
> >
> > v7:
> >  - Add support for using dynamic and NULL pointers in kfunc (suggested by
> >    Alexei)
> >  - Add new kfunc-related tests
> >
> > v6:
> >  - Switch back to key lookup helpers + signature verification (until v5),
> >    and defer permission check from bpf_lookup_user_key() to
> >    bpf_verify_pkcs7_signature()
> >  - Add additional key lookup test to illustrate the usage of the
> >    KEY_LOOKUP_CREATE flag and validate the flags (suggested by Daniel)
> >  - Make description of flags of bpf_lookup_user_key() more user-friendly
> >    (suggested by Daniel)
> >  - Fix validation of flags parameter in bpf_lookup_user_key() (reported by
> >    Daniel)
> >  - Rename bpf_verify_pkcs7_signature() keyring-related parameters to
> >    user_keyring and system_keyring to make their purpose more clear
> >  - Accept keyring-related parameters of bpf_verify_pkcs7_signature() as
> >    alternatives (suggested by KP)
> >  - Replace unsigned long type with u64 in helper declaration (suggested by
> >    Daniel)
> >  - Extend the bpf_verify_pkcs7_signature() test by calling the helper
> >    without data, by ensuring that the helper enforces the keyring-related
> >    parameters as alternatives, by ensuring that the helper rejects
> >    inaccessible and expired keyrings, and by checking all system keyrings
> >  - Move bpf_lookup_user_key() and bpf_key_put() usage tests to
> >    ref_tracking.c (suggested by John)
> >  - Call bpf_lookup_user_key() and bpf_key_put() only in sleepable programs
> >
> > v5:
> >  - Move KEY_LOOKUP_ to include/linux/key.h
> >    for validation of bpf_verify_pkcs7_signature() parameter
> >  - Remove bpf_lookup_user_key() and bpf_key_put() helpers, and the
> >    corresponding tests
> >  - Replace struct key parameter of bpf_verify_pkcs7_signature() with the
> >    keyring serial and lookup flags
> >  - Call lookup_user_key() and key_put() in bpf_verify_pkcs7_signature()
> >    code, to ensure that the retrieved key is used according to the
> >    permission requested at lookup time
> >  - Clarified keyring precedence in the description of
> >    bpf_verify_pkcs7_signature() (suggested by John)
> >  - Remove newline in the second argument of ASSERT_
> >  - Fix helper prototype regular expression in bpf_doc.py
> >
> > v4:
> >  - Remove bpf_request_key_by_id(), don't return an invalid pointer that
> >    other helpers can use
> >  - Pass the keyring ID (without ULONG_MAX, suggested by Alexei) to
> >    bpf_verify_pkcs7_signature()
> >  - Introduce bpf_lookup_user_key() and bpf_key_put() helpers (suggested by
> >    Alexei)
> >  - Add lookup_key_norelease test, to ensure that the verifier blocks eBPF
> >    programs which don't decrement the key reference count
> >  - Parse raw PKCS#7 signature instead of module-style signature in the
> >    verify_pkcs7_signature test (suggested by Alexei)
> >  - Parse kernel module in user space and pass raw PKCS#7 signature to the
> >    eBPF program for signature verification
> >
> > v3:
> >  - Rename bpf_verify_signature() back to bpf_verify_pkcs7_signature() to
> >    avoid managing different parameters for each signature verification
> >    function in one helper (suggested by Daniel)
> >  - Use dynamic pointers and export bpf_dynptr_get_size() (suggested by
> >    Alexei)
> >  - Introduce bpf_request_key_by_id() to give more flexibility to the caller
> >    of bpf_verify_pkcs7_signature() to retrieve the appropriate keyring
> >    (suggested by Alexei)
> >  - Fix test by reordering the gcc command line, always compile sign-file
> >  - Improve helper support check mechanism in the test
> >
> > v2:
> >  - Rename bpf_verify_pkcs7_signature() to a more generic
> >    bpf_verify_signature() and pass the signature type (suggested by KP)
> >  - Move the helper and prototype declaration under #ifdef so that user
> >    space can probe for support for the helper (suggested by Daniel)
> >  - Describe better the keyring types (suggested by Daniel)
> >  - Include linux/bpf.h instead of vmlinux.h to avoid implicit or
> >    redeclaration
> >  - Make the test selfcontained (suggested by Alexei)
> >
> > v1:
> >  - Don't define new map flag but introduce simple wrapper of
> >    verify_pkcs7_signature() (suggested by Alexei and KP)
> >
> > Benjamin Tissoires (1):
> >   btf: Add a new kfunc flag which allows to mark a function to be
> >     sleepable
> >
> > KP Singh (1):
> >   bpf: Allow kfuncs to be used in LSM programs
> >
> > Roberto Sassu (8):
> >   btf: Handle dynamic pointer parameter in kfuncs
> >   bpf: Export bpf_dynptr_get_size()
> >   KEYS: Move KEY_LOOKUP_ to include/linux/key.h
> >   bpf: Add bpf_lookup_*_key() and bpf_key_put() kfuncs
> >   bpf: Add bpf_verify_pkcs7_signature() kfunc
> >   selftests/bpf: Add verifier tests for bpf_lookup_*_key() and
> >     bpf_key_put()
> >   selftests/bpf: Add additional tests for bpf_lookup_*_key()
> >   selftests/bpf: Add test for bpf_verify_pkcs7_signature() kfunc
> >
> >  Documentation/bpf/kfuncs.rst                  |   6 +
> >  include/linux/bpf.h                           |   7 +
> >  include/linux/bpf_verifier.h                  |   3 +
> >  include/linux/btf.h                           |   1 +
> >  include/linux/key.h                           |   3 +
> >  kernel/bpf/btf.c                              |  27 ++
> >  kernel/bpf/helpers.c                          |   2 +-
> >  kernel/bpf/verifier.c                         |   4 +-
> >  kernel/trace/bpf_trace.c                      | 207 +++++++++
> >  security/keys/internal.h                      |   2 -
> >  tools/testing/selftests/bpf/Makefile          |  14 +-
> >  tools/testing/selftests/bpf/config            |   2 +
> >  .../selftests/bpf/prog_tests/lookup_key.c     | 112 +++++
> >  .../bpf/prog_tests/verify_pkcs7_sig.c         | 399 ++++++++++++++++++
> >  .../selftests/bpf/progs/test_lookup_key.c     |  46 ++
> >  .../bpf/progs/test_verify_pkcs7_sig.c         | 100 +++++
> >  tools/testing/selftests/bpf/test_verifier.c   |   3 +-
> >  .../selftests/bpf/verifier/ref_tracking.c     | 139 ++++++
> >  .../testing/selftests/bpf/verify_sig_setup.sh | 104 +++++
> >  19 files changed, 1172 insertions(+), 9 deletions(-)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/lookup_key.c
> >  create mode 100644
> tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/test_lookup_key.c
> >  create mode 100644
> tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> >  create mode 100755 tools/testing/selftests/bpf/verify_sig_setup.sh
> >
> > --
> > 2.25.1
> 
> BR, Jarkko

^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v9 01/10] btf: Add a new kfunc flag which allows to mark a function to be sleepable
  2022-08-09 16:54   ` Jarkko Sakkinen
@ 2022-08-10 13:44     ` Roberto Sassu
  2022-08-10 14:25       ` Benjamin Tissoires
  0 siblings, 1 reply; 28+ messages in thread
From: Roberto Sassu @ 2022-08-10 13:44 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, corbet, dhowells, rostedt, mingo,
	paul, jmorris, serge, shuah, bpf, linux-doc, keyrings,
	linux-security-module, linux-kselftest, linux-kernel,
	Benjamin Tissoires, Yosry Ahmed

> From: Jarkko Sakkinen [mailto:jarkko@kernel.org]
> Sent: Tuesday, August 9, 2022 6:55 PM
> On Tue, Aug 09, 2022 at 03:45:54PM +0200, Roberto Sassu wrote:
> > From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >
> > From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >
> > This allows to declare a kfunc as sleepable and prevents its use in
> > a non sleepable program.
> 
> Nit: "Declare a kfunc as sleepable and prevent its use in a
> non-sleepable program."
> 
> It's missing the part *how* the patch accomplishes its goals.

I will add:

If an eBPF program is going to call a kfunc declared as sleepable,
eBPF will look at the eBPF program flags. If BPF_F_SLEEPABLE is
not set, execution of that program is denied.

Roberto

> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > Co-developed-by: Yosry Ahmed <yosryahmed@google.com>
> > Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
> > Signed-off-by: Hao Luo <haoluo@google.com>
> > ---
> >  Documentation/bpf/kfuncs.rst | 6 ++++++
> >  include/linux/btf.h          | 1 +
> >  kernel/bpf/btf.c             | 9 +++++++++
> >  3 files changed, 16 insertions(+)
> >
> > diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> > index c0b7dae6dbf5..c8b21de1c772 100644
> > --- a/Documentation/bpf/kfuncs.rst
> > +++ b/Documentation/bpf/kfuncs.rst
> > @@ -146,6 +146,12 @@ that operate (change some property, perform some
> operation) on an object that
> >  was obtained using an acquire kfunc. Such kfuncs need an unchanged pointer
> to
> >  ensure the integrity of the operation being performed on the expected object.
> >
> > +2.4.6 KF_SLEEPABLE flag
> > +-----------------------
> > +
> > +The KF_SLEEPABLE flag is used for kfuncs that may sleep. Such kfuncs can
> only
> > +be called by sleepable BPF programs (BPF_F_SLEEPABLE).
> > +
> >  2.5 Registering the kfuncs
> >  --------------------------
> >
> > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > index cdb376d53238..976cbdd2981f 100644
> > --- a/include/linux/btf.h
> > +++ b/include/linux/btf.h
> > @@ -49,6 +49,7 @@
> >   * for this case.
> >   */
> >  #define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer
> arguments */
> > +#define KF_SLEEPABLE   (1 << 5) /* kfunc may sleep */
> >
> >  struct btf;
> >  struct btf_member;
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 7e64447659f3..d3e4c86b8fcd 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6175,6 +6175,7 @@ static int btf_check_func_arg_match(struct
> bpf_verifier_env *env,
> >  {
> >  	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
> >  	bool rel = false, kptr_get = false, trusted_arg = false;
> > +	bool sleepable = false;
> >  	struct bpf_verifier_log *log = &env->log;
> >  	u32 i, nargs, ref_id, ref_obj_id = 0;
> >  	bool is_kfunc = btf_is_kernel(btf);
> > @@ -6212,6 +6213,7 @@ static int btf_check_func_arg_match(struct
> bpf_verifier_env *env,
> >  		rel = kfunc_flags & KF_RELEASE;
> >  		kptr_get = kfunc_flags & KF_KPTR_GET;
> >  		trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
> > +		sleepable = kfunc_flags & KF_SLEEPABLE;
> >  	}
> >
> >  	/* check that BTF function arguments match actual types that the
> > @@ -6419,6 +6421,13 @@ static int btf_check_func_arg_match(struct
> bpf_verifier_env *env,
> >  			func_name);
> >  		return -EINVAL;
> >  	}
> > +
> > +	if (sleepable && !env->prog->aux->sleepable) {
> > +		bpf_log(log, "kernel function %s is sleepable but the program is
> not\n",
> > +			func_name);
> > +		return -EINVAL;
> > +	}
> > +
> >  	/* returns argument register number > 0 in case of reference release
> kfunc */
> >  	return rel ? ref_regno : 0;
> >  }
> > --
> > 2.25.1
> >
> 
> BR, Jarkko

^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v9 06/10] bpf: Add bpf_lookup_*_key() and bpf_key_put() kfuncs
  2022-08-09 22:53   ` Daniel Borkmann
@ 2022-08-10 14:17     ` Roberto Sassu
  2022-08-10 14:29       ` Daniel Borkmann
  0 siblings, 1 reply; 28+ messages in thread
From: Roberto Sassu @ 2022-08-10 14:17 UTC (permalink / raw)
  To: Daniel Borkmann, ast, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, corbet, dhowells,
	jarkko, rostedt, mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel

> From: Daniel Borkmann [mailto:daniel@iogearbox.net]
> Sent: Wednesday, August 10, 2022 12:53 AM
> On 8/9/22 3:45 PM, Roberto Sassu wrote:
> > Add the bpf_lookup_user_key(), bpf_lookup_system_key() and bpf_key_put()
> > kfuncs, to respectively search a key with a given serial and flags, obtain
> > a key from a pre-determined ID defined in include/linux/verification.h, and
> > cleanup.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >   include/linux/bpf.h      |   6 ++
> >   kernel/trace/bpf_trace.c | 151
> +++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 157 insertions(+)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 0d56c23cc504..564b9e0b8c16 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2572,4 +2572,10 @@ static inline void bpf_cgroup_atype_get(u32
> attach_btf_id, int cgroup_atype) {}
> >   static inline void bpf_cgroup_atype_put(int cgroup_atype) {}
> >   #endif /* CONFIG_BPF_LSM */
> >
> > +#ifdef CONFIG_KEYS
> > +struct bpf_key {
> > +	struct key *key;
> > +	bool valid_ptr;
> > +};
> > +#endif /* CONFIG_KEYS */
> >   #endif /* _LINUX_BPF_H */
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 68e5cdd24cef..33ca4cfe6e26 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -20,6 +20,7 @@
> >   #include <linux/fprobe.h>
> >   #include <linux/bsearch.h>
> >   #include <linux/sort.h>
> > +#include <linux/key.h>
> >
> >   #include <net/bpf_sk_storage.h>
> >
> > @@ -1181,6 +1182,156 @@ static const struct bpf_func_proto
> bpf_get_func_arg_cnt_proto = {
> >   	.arg1_type	= ARG_PTR_TO_CTX,
> >   };
> >
> > +#ifdef CONFIG_KEYS
> > +__diag_push();
> > +__diag_ignore_all("-Wmissing-prototypes",
> > +		  "kfuncs which will be used in BPF programs");
> > +
> > +/**
> > + * bpf_lookup_user_key - lookup a key by its serial
> > + * @serial: key serial
> > + * @flags: lookup-specific flags
> > + *
> > + * Search a key with a given *serial* and the provided *flags*. The
> > + * returned key, if found, has the reference count incremented by
> > + * one, and is stored in a bpf_key structure, returned to the caller.
> > + * The bpf_key structure must be passed to bpf_key_put() when done
> > + * with it, so that the key reference count is decremented and the
> > + * bpf_key structure is freed.
> > + *
> > + * Permission checks are deferred to the time the key is used by
> > + * one of the available key-specific kfuncs.
> > + *
> > + * Set *flags* with 1, to attempt creating a requested special
> > + * keyring (e.g. session keyring), if it doesn't yet exist. Set
> > + * *flags* with 2 to lookup a key without waiting for the key
> > + * construction, and to retrieve uninstantiated keys (keys without
> > + * data attached to them).
> > + *
> > + * Return: a bpf_key pointer with a valid key pointer if the key is found, a
> > + *         NULL pointer otherwise.
> > + */
> > +noinline __weak struct bpf_key *bpf_lookup_user_key(u32 serial, u64 flags)
> 
> Why the need for noinline and the __weak here and below? (If indeed needed
> this
> should probably be explained in the commit desc.)

Oh, I took from v3 of KP's patch set. It is gone in v5. Will do
the same as well.

> > +{
> > +	key_ref_t key_ref;
> > +	struct bpf_key *bkey;
> > +
> > +	/* Keep in sync with include/linux/key.h. */
> > +	if (flags > (KEY_LOOKUP_PARTIAL << 1) - 1)
> 
> Can't we just simplify and test flags &
> ~(KEY_LOOKUP_CREATE|KEY_LOOKUP_PARTIAL)?

I thought as if we have many flags.

> > +		return NULL;
> > +
> > +	/* Permission check is deferred until actual kfunc using the key. */
> > +	key_ref = lookup_user_key(serial, flags, KEY_DEFER_PERM_CHECK);
> > +	if (IS_ERR(key_ref))
> > +		return NULL;
> > +
> > +	bkey = kmalloc(sizeof(*bkey), GFP_KERNEL);
> > +	if (!bkey) {
> > +		key_put(key_ref_to_ptr(key_ref));
> > +		return bkey;
> 
> nit: just return NULL probably cleaner

Ok.

> > +	}
> > +
> > +	bkey->key = key_ref_to_ptr(key_ref);
> > +	bkey->valid_ptr = true;
> 
> nit: I'd probably rename s/valid_ptr/has_ref/.

Ok.

> > +	return bkey;
> > +}
> > +
> > +/**
> > + * bpf_lookup_system_key - lookup a key by a system-defined ID
> > + * @id: key ID
> > + *
> > + * Obtain a bpf_key structure with a key pointer set to the passed key ID.
> > + * The key pointer is marked as invalid, to prevent bpf_key_put() from
> > + * attempting to decrement the key reference count on that pointer. The key
> > + * pointer set in such way is currently understood only by
> > + * verify_pkcs7_signature().
> > + *
> > + * Set *id* to one of the values defined in include/linux/verification.h:
> > + * 0 for the primary keyring (immutable keyring of system keys); 1 for both
> > + * the primary and secondary keyring (where keys can be added only if they
> > + * are vouched for by existing keys in those keyrings); 2 for the platform
> > + * keyring (primarily used by the integrity subsystem to verify a kexec'ed
> > + * kerned image and, possibly, the initramfs signature).
> > + *
> > + * Return: a bpf_key pointer with an invalid key pointer set from the
> > + *         pre-determined ID on success, a NULL pointer otherwise
> > + */
> > +noinline __weak struct bpf_key *bpf_lookup_system_key(u64 id)
> > +{
> > +	struct bpf_key *bkey;
> > +
> > +	/* Keep in sync with defs in include/linux/verification.h. */
> > +	if (id > (unsigned long)VERIFY_USE_PLATFORM_KEYRING)
> > +		return NULL;
> > +
> > +	bkey = kmalloc(sizeof(*bkey), GFP_KERNEL);
> 
> nit: Can't this be GFP_ATOMIC? Then bpf_lookup_system_key doesn't need
> KF_SLEEPABLE
> attribute, fwiw. Overall, the bpf_lookup_{system,user}_key() look reasonable.

Ok, will do.

Thanks

Roberto

> > +	if (!bkey)
> > +		return bkey;
> > +
> > +	bkey->key = (struct key *)(unsigned long)id;
> > +	bkey->valid_ptr = false;
> > +
> > +	return bkey;
> > +}
> > +
> > +/**
> > + * bpf_key_put - decrement key reference count if key is valid and free
> bpf_key
> > + * @bkey: bpf_key structure
> > + *
> > + * Decrement the reference count of the key inside *bkey*, if the pointer
> > + * is valid, and free *bkey*.
> > + */
> > +noinline __weak void bpf_key_put(struct bpf_key *bkey)
> > +{
> > +	if (bkey->valid_ptr)
> > +		key_put(bkey->key);
> > +
> > +	kfree(bkey);
> > +}
> > +
> > +__diag_pop();
> > +
> > +BTF_SET8_START(key_kfunc_set)
> > +BTF_ID_FLAGS(func, bpf_lookup_user_key, KF_ACQUIRE | KF_RET_NULL |
> KF_SLEEPABLE)
> > +BTF_ID_FLAGS(func, bpf_lookup_system_key,
> > +	     KF_ACQUIRE | KF_RET_NULL | KF_SLEEPABLE)
> > +BTF_ID_FLAGS(func, bpf_key_put, KF_RELEASE)
> > +BTF_SET8_END(key_kfunc_set)
> > +
> > +static const struct btf_kfunc_id_set bpf_key_kfunc_set = {
> > +	.owner = THIS_MODULE,
> > +	.set = &key_kfunc_set,
> > +};
> > +#endif /* CONFIG_KEYS */
> > +
> > +const struct btf_kfunc_id_set *kfunc_sets[] = {
> > +#ifdef CONFIG_KEYS
> > +	&bpf_key_kfunc_set,
> > +#endif /* CONFIG_KEYS */
> > +};

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v9 01/10] btf: Add a new kfunc flag which allows to mark a function to be sleepable
  2022-08-10 13:44     ` Roberto Sassu
@ 2022-08-10 14:25       ` Benjamin Tissoires
  2022-08-10 14:38         ` Daniel Borkmann
  2022-08-10 14:58         ` Yosry Ahmed
  0 siblings, 2 replies; 28+ messages in thread
From: Benjamin Tissoires @ 2022-08-10 14:25 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: Jarkko Sakkinen, ast, daniel, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, corbet, dhowells,
	rostedt, mingo, paul, jmorris, serge, shuah, bpf, linux-doc,
	keyrings, linux-security-module, linux-kselftest, linux-kernel,
	Yosry Ahmed

On Wed, Aug 10, 2022 at 3:44 PM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> > From: Jarkko Sakkinen [mailto:jarkko@kernel.org]
> > Sent: Tuesday, August 9, 2022 6:55 PM
> > On Tue, Aug 09, 2022 at 03:45:54PM +0200, Roberto Sassu wrote:
> > > From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > >
> > > From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > >
> > > This allows to declare a kfunc as sleepable and prevents its use in
> > > a non sleepable program.
> >
> > Nit: "Declare a kfunc as sleepable and prevent its use in a
> > non-sleepable program."
> >
> > It's missing the part *how* the patch accomplishes its goals.
>
> I will add:
>
> If an eBPF program is going to call a kfunc declared as sleepable,
> eBPF will look at the eBPF program flags. If BPF_F_SLEEPABLE is
> not set, execution of that program is denied.

All those changes are looking good to me.

Thanks a lot for keeping pushing on this patch :)

Cheers,
Benjamin

>
> Roberto
>
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > Co-developed-by: Yosry Ahmed <yosryahmed@google.com>
> > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
> > > Signed-off-by: Hao Luo <haoluo@google.com>
> > > ---
> > >  Documentation/bpf/kfuncs.rst | 6 ++++++
> > >  include/linux/btf.h          | 1 +
> > >  kernel/bpf/btf.c             | 9 +++++++++
> > >  3 files changed, 16 insertions(+)
> > >
> > > diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> > > index c0b7dae6dbf5..c8b21de1c772 100644
> > > --- a/Documentation/bpf/kfuncs.rst
> > > +++ b/Documentation/bpf/kfuncs.rst
> > > @@ -146,6 +146,12 @@ that operate (change some property, perform some
> > operation) on an object that
> > >  was obtained using an acquire kfunc. Such kfuncs need an unchanged pointer
> > to
> > >  ensure the integrity of the operation being performed on the expected object.
> > >
> > > +2.4.6 KF_SLEEPABLE flag
> > > +-----------------------
> > > +
> > > +The KF_SLEEPABLE flag is used for kfuncs that may sleep. Such kfuncs can
> > only
> > > +be called by sleepable BPF programs (BPF_F_SLEEPABLE).
> > > +
> > >  2.5 Registering the kfuncs
> > >  --------------------------
> > >
> > > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > > index cdb376d53238..976cbdd2981f 100644
> > > --- a/include/linux/btf.h
> > > +++ b/include/linux/btf.h
> > > @@ -49,6 +49,7 @@
> > >   * for this case.
> > >   */
> > >  #define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer
> > arguments */
> > > +#define KF_SLEEPABLE   (1 << 5) /* kfunc may sleep */
> > >
> > >  struct btf;
> > >  struct btf_member;
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index 7e64447659f3..d3e4c86b8fcd 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -6175,6 +6175,7 @@ static int btf_check_func_arg_match(struct
> > bpf_verifier_env *env,
> > >  {
> > >     enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
> > >     bool rel = false, kptr_get = false, trusted_arg = false;
> > > +   bool sleepable = false;
> > >     struct bpf_verifier_log *log = &env->log;
> > >     u32 i, nargs, ref_id, ref_obj_id = 0;
> > >     bool is_kfunc = btf_is_kernel(btf);
> > > @@ -6212,6 +6213,7 @@ static int btf_check_func_arg_match(struct
> > bpf_verifier_env *env,
> > >             rel = kfunc_flags & KF_RELEASE;
> > >             kptr_get = kfunc_flags & KF_KPTR_GET;
> > >             trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
> > > +           sleepable = kfunc_flags & KF_SLEEPABLE;
> > >     }
> > >
> > >     /* check that BTF function arguments match actual types that the
> > > @@ -6419,6 +6421,13 @@ static int btf_check_func_arg_match(struct
> > bpf_verifier_env *env,
> > >                     func_name);
> > >             return -EINVAL;
> > >     }
> > > +
> > > +   if (sleepable && !env->prog->aux->sleepable) {
> > > +           bpf_log(log, "kernel function %s is sleepable but the program is
> > not\n",
> > > +                   func_name);
> > > +           return -EINVAL;
> > > +   }
> > > +
> > >     /* returns argument register number > 0 in case of reference release
> > kfunc */
> > >     return rel ? ref_regno : 0;
> > >  }
> > > --
> > > 2.25.1
> > >
> >
> > BR, Jarkko
>


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v9 06/10] bpf: Add bpf_lookup_*_key() and bpf_key_put() kfuncs
  2022-08-10 14:17     ` Roberto Sassu
@ 2022-08-10 14:29       ` Daniel Borkmann
  0 siblings, 0 replies; 28+ messages in thread
From: Daniel Borkmann @ 2022-08-10 14:29 UTC (permalink / raw)
  To: Roberto Sassu, ast, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, corbet, dhowells,
	jarkko, rostedt, mingo, paul, jmorris, serge, shuah
  Cc: bpf, linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel

[...]
>>> +noinline __weak struct bpf_key *bpf_lookup_user_key(u32 serial, u64 flags)
>>
>> Why the need for noinline and the __weak here and below? (If indeed needed
>> this
>> should probably be explained in the commit desc.)
> 
> Oh, I took from v3 of KP's patch set. It is gone in v5. Will do
> the same as well.
> 
>>> +{
>>> +	key_ref_t key_ref;
>>> +	struct bpf_key *bkey;
>>> +
>>> +	/* Keep in sync with include/linux/key.h. */
>>> +	if (flags > (KEY_LOOKUP_PARTIAL << 1) - 1)
>>
>> Can't we just simplify and test flags &
>> ~(KEY_LOOKUP_CREATE|KEY_LOOKUP_PARTIAL)?
> 
> I thought as if we have many flags.

I'd keep it simple for now, and if the actual need comes this can still be changed.

>>> +		return NULL;
>>> +
>>> +	/* Permission check is deferred until actual kfunc using the key. */
>>> +	key_ref = lookup_user_key(serial, flags, KEY_DEFER_PERM_CHECK);
>>> +	if (IS_ERR(key_ref))
>>> +		return NULL;
>>> +
>>> +	bkey = kmalloc(sizeof(*bkey), GFP_KERNEL);
>>> +	if (!bkey) {
>>> +		key_put(key_ref_to_ptr(key_ref));
>>> +		return bkey;
>>
>> nit: just return NULL probably cleaner
> 
> Ok.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v9 01/10] btf: Add a new kfunc flag which allows to mark a function to be sleepable
  2022-08-10 14:25       ` Benjamin Tissoires
@ 2022-08-10 14:38         ` Daniel Borkmann
  2022-08-10 14:52           ` Roberto Sassu
  2022-08-10 14:58         ` Yosry Ahmed
  1 sibling, 1 reply; 28+ messages in thread
From: Daniel Borkmann @ 2022-08-10 14:38 UTC (permalink / raw)
  To: Benjamin Tissoires, Roberto Sassu
  Cc: Jarkko Sakkinen, ast, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, corbet, dhowells,
	rostedt, mingo, paul, jmorris, serge, shuah, bpf, linux-doc,
	keyrings, linux-security-module, linux-kselftest, linux-kernel,
	Yosry Ahmed

On 8/10/22 4:25 PM, Benjamin Tissoires wrote:
> On Wed, Aug 10, 2022 at 3:44 PM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>>> From: Jarkko Sakkinen [mailto:jarkko@kernel.org]
>>> Sent: Tuesday, August 9, 2022 6:55 PM
>>> On Tue, Aug 09, 2022 at 03:45:54PM +0200, Roberto Sassu wrote:
>>>> From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>>>
>>>> From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>>>
>>>> This allows to declare a kfunc as sleepable and prevents its use in
>>>> a non sleepable program.
>>>
>>> Nit: "Declare a kfunc as sleepable and prevent its use in a
>>> non-sleepable program."
>>>
>>> It's missing the part *how* the patch accomplishes its goals.
>>
>> I will add:
>>
>> If an eBPF program is going to call a kfunc declared as sleepable,
>> eBPF will look at the eBPF program flags. If BPF_F_SLEEPABLE is
>> not set, execution of that program is denied.
> 
> All those changes are looking good to me.
> 
> Thanks a lot for keeping pushing on this patch :)

This single one from the series got already applied here:

   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=fa96b24204af42274ec13dfb2f2e6990d7510e55

>>>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>>> Co-developed-by: Yosry Ahmed <yosryahmed@google.com>
>>>> Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
>>>> Signed-off-by: Hao Luo <haoluo@google.com>
>>>> ---
>>>>   Documentation/bpf/kfuncs.rst | 6 ++++++
>>>>   include/linux/btf.h          | 1 +
>>>>   kernel/bpf/btf.c             | 9 +++++++++
>>>>   3 files changed, 16 insertions(+)
>>>>
>>>> diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
>>>> index c0b7dae6dbf5..c8b21de1c772 100644
>>>> --- a/Documentation/bpf/kfuncs.rst
>>>> +++ b/Documentation/bpf/kfuncs.rst
>>>> @@ -146,6 +146,12 @@ that operate (change some property, perform some
>>> operation) on an object that
>>>>   was obtained using an acquire kfunc. Such kfuncs need an unchanged pointer
>>> to
>>>>   ensure the integrity of the operation being performed on the expected object.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v9 01/10] btf: Add a new kfunc flag which allows to mark a function to be sleepable
  2022-08-10 14:38         ` Daniel Borkmann
@ 2022-08-10 14:52           ` Roberto Sassu
  0 siblings, 0 replies; 28+ messages in thread
From: Roberto Sassu @ 2022-08-10 14:52 UTC (permalink / raw)
  To: Daniel Borkmann, Benjamin Tissoires
  Cc: Jarkko Sakkinen, ast, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, corbet, dhowells,
	rostedt, mingo, paul, jmorris, serge, shuah, bpf, linux-doc,
	keyrings, linux-security-module, linux-kselftest, linux-kernel,
	Yosry Ahmed

> From: Daniel Borkmann [mailto:daniel@iogearbox.net]
> Sent: Wednesday, August 10, 2022 4:39 PM
> On 8/10/22 4:25 PM, Benjamin Tissoires wrote:
> > On Wed, Aug 10, 2022 at 3:44 PM Roberto Sassu
> <roberto.sassu@huawei.com> wrote:
> >>> From: Jarkko Sakkinen [mailto:jarkko@kernel.org]
> >>> Sent: Tuesday, August 9, 2022 6:55 PM
> >>> On Tue, Aug 09, 2022 at 03:45:54PM +0200, Roberto Sassu wrote:
> >>>> From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >>>>
> >>>> From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >>>>
> >>>> This allows to declare a kfunc as sleepable and prevents its use in
> >>>> a non sleepable program.
> >>>
> >>> Nit: "Declare a kfunc as sleepable and prevent its use in a
> >>> non-sleepable program."
> >>>
> >>> It's missing the part *how* the patch accomplishes its goals.
> >>
> >> I will add:
> >>
> >> If an eBPF program is going to call a kfunc declared as sleepable,
> >> eBPF will look at the eBPF program flags. If BPF_F_SLEEPABLE is
> >> not set, execution of that program is denied.
> >
> > All those changes are looking good to me.
> >
> > Thanks a lot for keeping pushing on this patch :)
> 
> This single one from the series got already applied here:
> 
>    https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-
> next.git/commit/?id=fa96b24204af42274ec13dfb2f2e6990d7510e55

Ok, now I understood the merge message better.

Roberto

> >>>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >>>> Co-developed-by: Yosry Ahmed <yosryahmed@google.com>
> >>>> Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
> >>>> Signed-off-by: Hao Luo <haoluo@google.com>
> >>>> ---
> >>>>   Documentation/bpf/kfuncs.rst | 6 ++++++
> >>>>   include/linux/btf.h          | 1 +
> >>>>   kernel/bpf/btf.c             | 9 +++++++++
> >>>>   3 files changed, 16 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> >>>> index c0b7dae6dbf5..c8b21de1c772 100644
> >>>> --- a/Documentation/bpf/kfuncs.rst
> >>>> +++ b/Documentation/bpf/kfuncs.rst
> >>>> @@ -146,6 +146,12 @@ that operate (change some property, perform
> some
> >>> operation) on an object that
> >>>>   was obtained using an acquire kfunc. Such kfuncs need an unchanged
> pointer
> >>> to
> >>>>   ensure the integrity of the operation being performed on the expected
> object.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v9 01/10] btf: Add a new kfunc flag which allows to mark a function to be sleepable
  2022-08-10 14:25       ` Benjamin Tissoires
  2022-08-10 14:38         ` Daniel Borkmann
@ 2022-08-10 14:58         ` Yosry Ahmed
  1 sibling, 0 replies; 28+ messages in thread
From: Yosry Ahmed @ 2022-08-10 14:58 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Roberto Sassu, Jarkko Sakkinen, ast, daniel, andrii, martin.lau,
	song, yhs, john.fastabend, kpsingh, sdf, haoluo, jolsa, corbet,
	dhowells, rostedt, mingo, paul, jmorris, serge, shuah, bpf,
	linux-doc, keyrings, linux-security-module, linux-kselftest,
	linux-kernel

On Wed, Aug 10, 2022 at 7:26 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Aug 10, 2022 at 3:44 PM Roberto Sassu <roberto.sassu@huawei.com> wrote:
> >
> > > From: Jarkko Sakkinen [mailto:jarkko@kernel.org]
> > > Sent: Tuesday, August 9, 2022 6:55 PM
> > > On Tue, Aug 09, 2022 at 03:45:54PM +0200, Roberto Sassu wrote:
> > > > From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > >
> > > > From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > >
> > > > This allows to declare a kfunc as sleepable and prevents its use in
> > > > a non sleepable program.
> > >
> > > Nit: "Declare a kfunc as sleepable and prevent its use in a
> > > non-sleepable program."
> > >
> > > It's missing the part *how* the patch accomplishes its goals.
> >
> > I will add:
> >
> > If an eBPF program is going to call a kfunc declared as sleepable,
> > eBPF will look at the eBPF program flags. If BPF_F_SLEEPABLE is
> > not set, execution of that program is denied.
>
> All those changes are looking good to me.
>
> Thanks a lot for keeping pushing on this patch :)

Multiple series other than the HID one started resending your patch
once you dropped it, it's obviously needed :) Thanks for sending it in
the first place :)

>
> Cheers,
> Benjamin
>
> >
> > Roberto
> >
> > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > Co-developed-by: Yosry Ahmed <yosryahmed@google.com>
> > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
> > > > Signed-off-by: Hao Luo <haoluo@google.com>
> > > > ---
> > > >  Documentation/bpf/kfuncs.rst | 6 ++++++
> > > >  include/linux/btf.h          | 1 +
> > > >  kernel/bpf/btf.c             | 9 +++++++++
> > > >  3 files changed, 16 insertions(+)
> > > >
> > > > diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> > > > index c0b7dae6dbf5..c8b21de1c772 100644
> > > > --- a/Documentation/bpf/kfuncs.rst
> > > > +++ b/Documentation/bpf/kfuncs.rst
> > > > @@ -146,6 +146,12 @@ that operate (change some property, perform some
> > > operation) on an object that
> > > >  was obtained using an acquire kfunc. Such kfuncs need an unchanged pointer
> > > to
> > > >  ensure the integrity of the operation being performed on the expected object.
> > > >
> > > > +2.4.6 KF_SLEEPABLE flag
> > > > +-----------------------
> > > > +
> > > > +The KF_SLEEPABLE flag is used for kfuncs that may sleep. Such kfuncs can
> > > only
> > > > +be called by sleepable BPF programs (BPF_F_SLEEPABLE).
> > > > +
> > > >  2.5 Registering the kfuncs
> > > >  --------------------------
> > > >
> > > > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > > > index cdb376d53238..976cbdd2981f 100644
> > > > --- a/include/linux/btf.h
> > > > +++ b/include/linux/btf.h
> > > > @@ -49,6 +49,7 @@
> > > >   * for this case.
> > > >   */
> > > >  #define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer
> > > arguments */
> > > > +#define KF_SLEEPABLE   (1 << 5) /* kfunc may sleep */
> > > >
> > > >  struct btf;
> > > >  struct btf_member;
> > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > > index 7e64447659f3..d3e4c86b8fcd 100644
> > > > --- a/kernel/bpf/btf.c
> > > > +++ b/kernel/bpf/btf.c
> > > > @@ -6175,6 +6175,7 @@ static int btf_check_func_arg_match(struct
> > > bpf_verifier_env *env,
> > > >  {
> > > >     enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
> > > >     bool rel = false, kptr_get = false, trusted_arg = false;
> > > > +   bool sleepable = false;
> > > >     struct bpf_verifier_log *log = &env->log;
> > > >     u32 i, nargs, ref_id, ref_obj_id = 0;
> > > >     bool is_kfunc = btf_is_kernel(btf);
> > > > @@ -6212,6 +6213,7 @@ static int btf_check_func_arg_match(struct
> > > bpf_verifier_env *env,
> > > >             rel = kfunc_flags & KF_RELEASE;
> > > >             kptr_get = kfunc_flags & KF_KPTR_GET;
> > > >             trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
> > > > +           sleepable = kfunc_flags & KF_SLEEPABLE;
> > > >     }
> > > >
> > > >     /* check that BTF function arguments match actual types that the
> > > > @@ -6419,6 +6421,13 @@ static int btf_check_func_arg_match(struct
> > > bpf_verifier_env *env,
> > > >                     func_name);
> > > >             return -EINVAL;
> > > >     }
> > > > +
> > > > +   if (sleepable && !env->prog->aux->sleepable) {
> > > > +           bpf_log(log, "kernel function %s is sleepable but the program is
> > > not\n",
> > > > +                   func_name);
> > > > +           return -EINVAL;
> > > > +   }
> > > > +
> > > >     /* returns argument register number > 0 in case of reference release
> > > kfunc */
> > > >     return rel ? ref_regno : 0;
> > > >  }
> > > > --
> > > > 2.25.1
> > > >
> > >
> > > BR, Jarkko
> >
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2022-08-10 14:58 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-09 13:45 [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification Roberto Sassu
2022-08-09 13:45 ` [PATCH v9 01/10] btf: Add a new kfunc flag which allows to mark a function to be sleepable Roberto Sassu
2022-08-09 16:54   ` Jarkko Sakkinen
2022-08-10 13:44     ` Roberto Sassu
2022-08-10 14:25       ` Benjamin Tissoires
2022-08-10 14:38         ` Daniel Borkmann
2022-08-10 14:52           ` Roberto Sassu
2022-08-10 14:58         ` Yosry Ahmed
2022-08-09 13:45 ` [PATCH v9 02/10] bpf: Allow kfuncs to be used in LSM programs Roberto Sassu
2022-08-09 21:53   ` Daniel Borkmann
2022-08-09 13:45 ` [PATCH v9 03/10] btf: Handle dynamic pointer parameter in kfuncs Roberto Sassu
2022-08-09 22:08   ` Daniel Borkmann
2022-08-09 22:29   ` Daniel Borkmann
2022-08-09 13:45 ` [PATCH v9 04/10] bpf: Export bpf_dynptr_get_size() Roberto Sassu
2022-08-09 13:45 ` [PATCH v9 05/10] KEYS: Move KEY_LOOKUP_ to include/linux/key.h Roberto Sassu
2022-08-09 13:45 ` [PATCH v9 06/10] bpf: Add bpf_lookup_*_key() and bpf_key_put() kfuncs Roberto Sassu
2022-08-09 22:53   ` Daniel Borkmann
2022-08-10 14:17     ` Roberto Sassu
2022-08-10 14:29       ` Daniel Borkmann
2022-08-09 13:46 ` [PATCH v9 07/10] bpf: Add bpf_verify_pkcs7_signature() kfunc Roberto Sassu
2022-08-09 23:09   ` Daniel Borkmann
2022-08-10  2:41   ` Alexei Starovoitov
2022-08-09 13:46 ` [PATCH v9 08/10] selftests/bpf: Add verifier tests for bpf_lookup_*_key() and bpf_key_put() Roberto Sassu
2022-08-09 13:46 ` [PATCH v9 09/10] selftests/bpf: Add additional tests for bpf_lookup_*_key() Roberto Sassu
2022-08-09 13:46 ` [PATCH v9 10/10] selftests/bpf: Add test for bpf_verify_pkcs7_signature() kfunc Roberto Sassu
2022-08-09 16:20 ` [PATCH v9 00/10] bpf: Add kfuncs for PKCS#7 signature verification patchwork-bot+netdevbpf
2022-08-09 16:53 ` Jarkko Sakkinen
2022-08-10 10:47   ` Roberto Sassu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).