[v16,00/12] bpf: Add kfuncs for PKCS#7 signature verification

Message ID	20220905143318.1592015-1-roberto.sassu@huaweicloud.com
Headers	show Return-Path: <linux-kselftest-owner@kernel.org> From: Roberto Sassu <roberto.sassu@huaweicloud.com> To: ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org, martin.lau@linux.dev, song@kernel.org, yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com, haoluo@google.com, jolsa@kernel.org, mykolal@fb.com, dhowells@redhat.com, jarkko@kernel.org, rostedt@goodmis.org, mingo@redhat.com, paul@paul-moore.com, jmorris@namei.org, serge@hallyn.com, shuah@kernel.org Cc: bpf@vger.kernel.org, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org, deso@posteo.net, memxor@gmail.com, Roberto Sassu <roberto.sassu@huawei.com> Subject: [PATCH v16 00/12] bpf: Add kfuncs for PKCS#7 signature verification Date: Mon, 5 Sep 2022 16:33:06 +0200 Message-Id: <20220905143318.1592015-1-roberto.sassu@huaweicloud.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	bpf: Add kfuncs for PKCS#7 signature verification \| expand [v16,00/12] bpf: Add kfuncs for PKCS#7 signature verification [v16,01/12] bpf: Allow kfuncs to be used in LSM programs [v16,02/12] bpf: Move dynptr type check to is_dynptr_type_expected() [v16,03/12] btf: Allow dynamic pointer parameters in kfuncs [v16,04/12] bpf: Export bpf_dynptr_get_size() [v16,05/12] KEYS: Move KEY_LOOKUP_ to include/linux/key.h and define KEY_LOOKUP_ALL [v16,06/12] bpf: Add bpf_lookup__key() and bpf_key_put() kfuncs [v16,07/12] bpf: Add bpf_verify_pkcs7_signature() kfunc [v16,08/12] selftests/bpf: Compile kernel with everything as built-in [v16,09/12] selftests/bpf: Add verifier tests for bpf_lookup__key() and bpf_key_put() [v16,10/12] selftests/bpf: Add additional tests for bpf_lookup_*_key() [v16,11/12] selftests/bpf: Add test for bpf_verify_pkcs7_signature() kfunc [v16,12/12] selftests/bpf: Add tests for dynamic pointers parameters in kfuncs

Roberto Sassu Sept. 5, 2022, 2:33 p.m. UTC

From: Roberto Sassu <roberto.sassu@huawei.com>

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.

bpf_lookup_user_key() and bpf_verify_pkcs7_signature() 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 a patch from another author (dependency) for sake of
completeness. It is organized as follows.

Patch 1 from KP Singh allows kfuncs to be used by LSM programs. Patch 2
splits is_dynptr_reg_valid_init() and introduces is_dynptr_type_expected(),
to know more precisely the cause of a negative result of a dynamic pointer
check. 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
and programs 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. Patch 8 changes the testing kernel
configuration to compile everything as built-in. Finally, patches 9-12
introduce the tests.

Changelog

v15:
 - Add kfunc_dynptr_param test to deny list for s390x

v14:
 - Explain that is_dynptr_type_expected() will be useful also for BTF
   (suggested by Joanne)
 - Rename KEY_LOOKUP_FLAGS_ALL to KEY_LOOKUP_ALL (suggested by Jarkko)
 - Swap declaration of spi and dynptr_type in is_dynptr_type_expected()
   (suggested by Joanne)
 - Reimplement kfunc dynptr tests with a regular eBPF program instead of
   executing them with test_verifier (suggested by Joanne)
 - Make key lookup flags as enum so that they are automatically exported
   through BTF (suggested by Alexei)
 
v13:
 - Split is_dynptr_reg_valid_init() and introduce is_dynptr_type_expected()
   to see if the dynamic pointer type passed as argument to a kfunc is
   supported (suggested by Kumar)
 - Add forward declaration of struct key in include/linux/bpf.h (suggested
   by Song)
 - Declare mask for key lookup flags, remove key_lookup_flags_check()
   (suggested by Jarkko and KP)
 - Allow only certain dynamic pointer types (currently, local) to be passed
   as argument to kfuncs (suggested by Kumar)
 - For each dynamic pointer parameter in kfunc, additionally check if the
   passed pointer is to the stack (suggested by Kumar)
 - Split the validity/initialization and dynamic pointer type check also in
   the verifier, and adjust the expected error message in the test (a test
   for an unexpected dynptr type passed to a helper cannot be added due to
   missing suitable helpers, but this case has been tested manually)
 - Add verifier tests to check the dynamic pointers passed as argument to
   kfuncs (suggested by Kumar)

v12:
 - Put lookup_key and verify_pkcs7_sig tests in deny list for s390x (JIT
   does not support calling kernel function)

v11:
 - Move stringify_struct() macro to include/linux/btf.h (suggested by
   Daniel)
 - Change kernel configuration options in
   tools/testing/selftests/bpf/config* from =m to =y

v10:
 - Introduce key_lookup_flags_check() and system_keyring_id_check() inline
   functions to check parameters (suggested by KP)
 - Fix descriptions and comment of key-related kfuncs (suggested by KP)
 - Register kfunc set only once (suggested by Alexei)
 - Move needed kernel options to the architecture-independent configuration
   for testing

v9:
 - Drop patch to introduce KF_SLEEPABLE kfunc flag (already merged)
 - Rename valid_ptr member of bpf_key to has_ref (suggested by Daniel)
 - Check dynamic pointers in kfunc definition with bpf_dynptr_kern struct
   definition instead of string, to detect structure renames (suggested by
   Daniel)
 - Explicitly say that we permit initialized dynamic pointers in kfunc
   definition (suggested by Daniel)
 - Remove noinline __weak from kfuncs definition (reported by Daniel)
 - Simplify key lookup flags check in bpf_lookup_user_key() (suggested by
   Daniel)
 - Explain the reason for deferring key permission check (suggested by
   Daniel)
 - Allocate memory with GFP_ATOMIC in bpf_lookup_system_key(), and remove
   KF_SLEEPABLE kfunc flag from kfunc declaration (suggested by Daniel)
 - Define only one kfunc set and remove the loop for registration
   (suggested by Alexei)

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)

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

Roberto Sassu (11):
  bpf: Move dynptr type check to is_dynptr_type_expected()
  btf: Allow dynamic pointer parameters in kfuncs
  bpf: Export bpf_dynptr_get_size()
  KEYS: Move KEY_LOOKUP_ to include/linux/key.h and define
    KEY_LOOKUP_ALL
  bpf: Add bpf_lookup_*_key() and bpf_key_put() kfuncs
  bpf: Add bpf_verify_pkcs7_signature() kfunc
  selftests/bpf: Compile kernel with everything as built-in
  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
  selftests/bpf: Add tests for dynamic pointers parameters in kfuncs

 include/linux/bpf.h                           |   9 +
 include/linux/bpf_verifier.h                  |   5 +
 include/linux/btf.h                           |   9 +
 include/linux/key.h                           |   6 +
 include/linux/verification.h                  |   8 +
 kernel/bpf/btf.c                              |  34 ++
 kernel/bpf/helpers.c                          |   2 +-
 kernel/bpf/verifier.c                         |  35 +-
 kernel/trace/bpf_trace.c                      | 180 ++++++++
 security/keys/internal.h                      |   2 -
 tools/testing/selftests/bpf/DENYLIST.s390x    |   3 +
 tools/testing/selftests/bpf/Makefile          |  14 +-
 tools/testing/selftests/bpf/config            |  32 +-
 tools/testing/selftests/bpf/config.x86_64     |   7 +-
 .../testing/selftests/bpf/prog_tests/dynptr.c |   2 +-
 .../bpf/prog_tests/kfunc_dynptr_param.c       | 103 +++++
 .../selftests/bpf/prog_tests/lookup_key.c     | 112 +++++
 .../bpf/prog_tests/verify_pkcs7_sig.c         | 399 ++++++++++++++++++
 .../bpf/progs/test_kfunc_dynptr_param.c       |  57 +++
 .../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 +++++
 24 files changed, 1376 insertions(+), 35 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
 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_kfunc_dynptr_param.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

Kumar Kartikeya Dwivedi Sept. 5, 2022, 7:26 p.m. UTC | #1

On Mon, 5 Sept 2022 at 16:34, Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> 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.
>
> [...]

CI is crashing with NULL deref for test_progs-no_alu32 with llvm-16,
but I don't think the problem is in this series. This is most likely
unrelated to BPF, as the crash happens inside
kernel/time/tick-sched.c:tick_nohz_restart_sched_tick.

This was the same case in
https://lore.kernel.org/bpf/CAP01T74steDfP6O8QOshoto3e3RnHhKtAeTbnrPBZS3YJXjvbA@mail.gmail.com.

So, https://github.com/kernel-patches/bpf/runs/8194263557?check_suite_focus=true
and https://github.com/kernel-patches/bpf/runs/7982907380?check_suite_focus=true

look similar to me, and may not be related to BPF. They only trigger
during runs compiled using LLVM 16, so maybe some compiler
transformation is surfacing the problem?

Jarkko Sakkinen Sept. 5, 2022, 9:38 p.m. UTC | #2

On Mon, Sep 05, 2022 at 04:33:11PM +0200, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> 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. Add them to enum key_lookup_flag, so that
> all the current ones and the ones defined in the future are automatically
> exported through BTF and available to eBPF programs.
> 
> Also, add KEY_LOOKUP_ALL to the enum, to facilitate checking whether a
> variable contains only defined flags.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: KP Singh <kpsingh@kernel.org>
> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

You should remove ack if there is any substantial change.

> ---
>  include/linux/key.h      | 6 ++++++
>  security/keys/internal.h | 2 --
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/key.h b/include/linux/key.h
> index 7febc4881363..d84171f90cbd 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -88,6 +88,12 @@ enum key_need_perm {
>  	KEY_DEFER_PERM_CHECK,	/* Special: permission check is deferred */
>  };
>  
> +enum key_lookup_flag {
> +	KEY_LOOKUP_CREATE = 0x01,	/* Create special keyrings if they don't exist */
> +	KEY_LOOKUP_PARTIAL = 0x02,	/* Permit partially constructed keys to be found */
> +	KEY_LOOKUP_ALL = (KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL), /* OR of previous flags */

Drop the comments (should be reviewed separately + out of context).

> +};
> +
>  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
> 

BR, Jarkko

Kumar Kartikeya Dwivedi Sept. 6, 2022, 2:28 a.m. UTC | #3

On Mon, 5 Sept 2022 at 16:34, Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: KP Singh <kpsingh@kernel.org>
>
> In preparation for the addition of new kfuncs, allow kfuncs defined in the
> tracing subsystem to be used in LSM programs by mapping the LSM program
> type to the TRACING hook.
>
> Signed-off-by: KP Singh <kpsingh@kernel.org>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---

Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

>  kernel/bpf/btf.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 903719b89238..e49b3b6d48ad 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -7243,6 +7243,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
>

Kumar Kartikeya Dwivedi Sept. 6, 2022, 2:33 a.m. UTC | #4

On Mon, 5 Sept 2022 at 16:34, Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> Allow dynamic pointers (struct bpf_dynptr_kern *) to be specified as
> parameters in kfuncs. Also, ensure that dynamic pointers passed as argument
> are valid and initialized, are a pointer to the stack, and of the type
> local. More dynamic pointer types can be supported in the future.
>
> To properly detect whether a parameter is of the desired type, introduce
> the stringify_struct() macro to compare the returned structure name with
> the desired name. In addition, protect against structure renames, by
> halting the build with BUILD_BUG_ON(), so that developers have to revisit
> the code.
>
> To check if a dynamic pointer passed to the kfunc is valid and initialized,
> and if its type is local, export the existing functions
> is_dynptr_reg_valid_init() and is_dynptr_type_expected().
>
> Cc: Joanne Koong <joannelkoong@gmail.com>
> Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---

Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

>  include/linux/bpf_verifier.h |  5 +++++
>  include/linux/btf.h          |  9 +++++++++
>  kernel/bpf/btf.c             | 33 +++++++++++++++++++++++++++++++++
>  kernel/bpf/verifier.c        | 10 +++++-----
>  4 files changed, 52 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 1fdddbf3546b..dd58dfccd025 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -571,6 +571,11 @@ 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);
> +bool is_dynptr_type_expected(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/include/linux/btf.h b/include/linux/btf.h
> index ad93c2d9cc1c..f546d368ac5d 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -52,6 +52,15 @@
>  #define KF_SLEEPABLE    (1 << 5) /* kfunc may sleep */
>  #define KF_DESTRUCTIVE  (1 << 6) /* kfunc performs destructive actions */
>
> +/*
> + * Return the name of the passed struct, if exists, or halt the build if for
> + * example the structure gets renamed. In this way, developers have to revisit
> + * the code using that structure name, and update it accordingly.
> + */
> +#define stringify_struct(x)                    \
> +       ({ BUILD_BUG_ON(sizeof(struct x) < 0);  \
> +          __stringify(x); })
> +
>  struct btf;
>  struct btf_member;
>  struct btf_type;
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index e49b3b6d48ad..4266bff5fada 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6362,15 +6362,20 @@ 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,
> +                                                         stringify_struct(bpf_dynptr_kern));
>
>                                 /* Permit pointer to mem, but only when argument
>                                  * type is pointer to scalar, or struct composed
>                                  * (recursively) of scalars.
>                                  * When arg_mem_size is true, the pointer can be
>                                  * void *.
> +                                * Also permit initialized local dynamic pointers.
>                                  */
>                                 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",
> @@ -6378,6 +6383,34 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>                                         return -EINVAL;
>                                 }
>
> +                               if (arg_dynptr) {
> +                                       if (reg->type != PTR_TO_STACK) {
> +                                               bpf_log(log, "arg#%d pointer type %s %s not to stack\n",
> +                                                       i, btf_type_str(ref_t),
> +                                                       ref_tname);
> +                                               return -EINVAL;
> +                                       }
> +
> +                                       if (!is_dynptr_reg_valid_init(env, reg)) {
> +                                               bpf_log(log,
> +                                                       "arg#%d pointer type %s %s must be valid and initialized\n",
> +                                                       i, btf_type_str(ref_t),
> +                                                       ref_tname);
> +                                               return -EINVAL;
> +                                       }
> +
> +                                       if (!is_dynptr_type_expected(env, reg,
> +                                                       ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_LOCAL)) {
> +                                               bpf_log(log,
> +                                                       "arg#%d pointer type %s %s points to unsupported dynamic pointer type\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 10b3c0a81d09..8f02729074c6 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -779,8 +779,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)
> +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env,
> +                             struct bpf_reg_state *reg)
>  {
>         struct bpf_func_state *state = func(env, reg);
>         int spi = get_spi(reg->off);
> @@ -799,9 +799,9 @@ static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env,
>         return true;
>  }
>
> -static bool is_dynptr_type_expected(struct bpf_verifier_env *env,
> -                                   struct bpf_reg_state *reg,
> -                                   enum bpf_arg_type arg_type)
> +bool is_dynptr_type_expected(struct bpf_verifier_env *env,
> +                            struct bpf_reg_state *reg,
> +                            enum bpf_arg_type arg_type)
>  {
>         struct bpf_func_state *state = func(env, reg);
>         enum bpf_dynptr_type dynptr_type;
> --
> 2.25.1
>

Kumar Kartikeya Dwivedi Sept. 6, 2022, 2:57 a.m. UTC | #5

On Mon, 5 Sept 2022 at 16:35, Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> 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>
> Acked-by: KP Singh <kpsingh@kernel.org>
> ---
>  kernel/trace/bpf_trace.c | 45 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 7a7023704ac2..8e2c026b0a58 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1294,12 +1294,57 @@ 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.
> + */
> +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->has_ref) {
> +               /*
> +                * Do the permission check deferred in bpf_lookup_user_key().
> +                * See bpf_lookup_user_key() for more details.
> +                *
> +                * 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),

MIssing check for data_ptr->data == NULL before making this call? Same
for sig_ptr.

> +                                     trusted_keyring->key,
> +                                     VERIFYING_UNSPECIFIED_SIGNATURE, NULL,
> +                                     NULL);
> +}
> +#endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
> +
>  __diag_pop();
>
>  BTF_SET8_START(key_sig_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)
>  BTF_ID_FLAGS(func, bpf_key_put, KF_RELEASE)
> +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> +BTF_ID_FLAGS(func, bpf_verify_pkcs7_signature, KF_SLEEPABLE)
> +#endif
>  BTF_SET8_END(key_sig_kfunc_set)
>
>  static const struct btf_kfunc_id_set bpf_key_sig_kfunc_set = {
> --
> 2.25.1
>

Kumar Kartikeya Dwivedi Sept. 6, 2022, 3:03 a.m. UTC | #6

On Mon, 5 Sept 2022 at 16:36, Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> 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>
> ---

Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

>  tools/testing/selftests/bpf/config            |   1 +
>  tools/testing/selftests/bpf/test_verifier.c   |   3 +-
>  .../selftests/bpf/verifier/ref_tracking.c     | 139 ++++++++++++++++++
>  3 files changed, 142 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
> index 0fdd11e6b742..add5a5a919b4 100644
> --- a/tools/testing/selftests/bpf/config
> +++ b/tools/testing/selftests/bpf/config
> @@ -30,6 +30,7 @@ CONFIG_IPV6_GRE=y
>  CONFIG_IPV6_SEG6_BPF=y
>  CONFIG_IPV6_SIT=y
>  CONFIG_IPV6_TUNNEL=y
> +CONFIG_KEYS=y
>  CONFIG_LIRC=y
>  CONFIG_LWTUNNEL=y
>  CONFIG_MPLS=y
> 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
>

Kumar Kartikeya Dwivedi Sept. 6, 2022, 3:15 a.m. UTC | #7

On Mon, 5 Sept 2022 at 16:36, Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> Add tests to ensure that only supported dynamic pointer types are accepted,
> that the passed argument is actually a dynamic pointer, and that the passed
> argument is a pointer to the stack.
>
> The tests are currently in the deny list for s390x (JIT does not support
> calling kernel function).
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  tools/testing/selftests/bpf/DENYLIST.s390x    |   1 +
>  .../bpf/prog_tests/kfunc_dynptr_param.c       | 103 ++++++++++++++++++
>  .../bpf/progs/test_kfunc_dynptr_param.c       |  57 ++++++++++
>  3 files changed, 161 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c
>
> diff --git a/tools/testing/selftests/bpf/DENYLIST.s390x b/tools/testing/selftests/bpf/DENYLIST.s390x
> index 4e305baa5277..9a6dc3671c65 100644
> --- a/tools/testing/selftests/bpf/DENYLIST.s390x
> +++ b/tools/testing/selftests/bpf/DENYLIST.s390x
> @@ -71,3 +71,4 @@ cgroup_hierarchical_stats                # JIT does not support calling kernel f
>  htab_update                              # failed to attach: ERROR: strerror_r(-524)=22                                (trampoline)
>  lookup_key                               # JIT does not support calling kernel function                                (kfunc)
>  verify_pkcs7_sig                         # JIT does not support calling kernel function                                (kfunc)
> +kfunc_dynptr_param                       # JIT does not support calling kernel function                                (kfunc)
> diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
> new file mode 100644
> index 000000000000..ea655a5c9d8b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
> @@ -0,0 +1,103 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright (c) 2022 Facebook
> + * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH
> + *
> + * Author: Roberto Sassu <roberto.sassu@huawei.com>
> + */
> +
> +#include <test_progs.h>
> +#include "test_kfunc_dynptr_param.skel.h"
> +
> +static size_t log_buf_sz = 1048576; /* 1 MB */
> +static char obj_log_buf[1048576];
> +
> +static struct {
> +       const char *prog_name;
> +       const char *expected_err_msg;
> +} kfunc_dynptr_tests[] = {
> +       {"dynptr_type_not_supp",
> +        "arg#0 pointer type STRUCT bpf_dynptr_kern points to unsupported dynamic pointer type"},
> +       {"not_valid_dynptr",
> +        "arg#0 pointer type STRUCT bpf_dynptr_kern must be valid and initialized"},
> +       {"not_ptr_to_stack", "arg#0 pointer type STRUCT bpf_dynptr_kern not to stack"},
> +};
> +
> +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 void verify_fail(const char *prog_name, const char *expected_err_msg)
> +{
> +       struct test_kfunc_dynptr_param *skel;
> +       LIBBPF_OPTS(bpf_object_open_opts, opts);
> +       libbpf_print_fn_t old_print_cb;
> +       struct bpf_program *prog;
> +       int err;
> +
> +       opts.kernel_log_buf = obj_log_buf;
> +       opts.kernel_log_size = log_buf_sz;
> +       opts.kernel_log_level = 1;
> +
> +       skel = test_kfunc_dynptr_param__open_opts(&opts);
> +       if (!ASSERT_OK_PTR(skel, "test_kfunc_dynptr_param__open_opts"))
> +               goto cleanup;
> +
> +       prog = bpf_object__find_program_by_name(skel->obj, prog_name);
> +       if (!ASSERT_OK_PTR(prog, "bpf_object__find_program_by_name"))
> +               goto cleanup;
> +
> +       bpf_program__set_autoload(prog, true);
> +
> +       bpf_map__set_max_entries(skel->maps.ringbuf, getpagesize());
> +
> +       kfunc_not_supported = false;
> +
> +       old_print_cb = libbpf_set_print(libbpf_print_cb);
> +       err = test_kfunc_dynptr_param__load(skel);
> +       libbpf_set_print(old_print_cb);
> +
> +       if (err < 0 && kfunc_not_supported) {
> +               fprintf(stderr,
> +                 "%s:SKIP:bpf_verify_pkcs7_signature() kfunc not supported\n",
> +                 __func__);
> +               test__skip();
> +               goto cleanup;
> +       }
> +
> +       if (!ASSERT_ERR(err, "unexpected load success"))
> +               goto cleanup;
> +
> +       if (!ASSERT_OK_PTR(strstr(obj_log_buf, expected_err_msg), "expected_err_msg")) {
> +               fprintf(stderr, "Expected err_msg: %s\n", expected_err_msg);
> +               fprintf(stderr, "Verifier output: %s\n", obj_log_buf);
> +       }
> +
> +cleanup:
> +       test_kfunc_dynptr_param__destroy(skel);
> +}
> +
> +void test_kfunc_dynptr_param(void)
> +{
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(kfunc_dynptr_tests); i++) {
> +               if (!test__start_subtest(kfunc_dynptr_tests[i].prog_name))
> +                       continue;
> +
> +               verify_fail(kfunc_dynptr_tests[i].prog_name,
> +                           kfunc_dynptr_tests[i].expected_err_msg);
> +       }
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c b/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c
> new file mode 100644
> index 000000000000..2f09f91a1576
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c
> @@ -0,0 +1,57 @@
> +// 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>
> +
> +struct bpf_dynptr {
> +       __u64 :64;
> +       __u64 :64;
> +} __attribute__((aligned(8)));
> +
> +extern int bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr,
> +                                     struct bpf_dynptr *sig_ptr,
> +                                     struct bpf_key *trusted_keyring) __ksym;
> +
> +struct {
> +       __uint(type, BPF_MAP_TYPE_RINGBUF);
> +} ringbuf SEC(".maps");
> +
> +char _license[] SEC("license") = "GPL";
> +
> +SEC("?lsm.s/bpf")
> +int BPF_PROG(dynptr_type_not_supp, int cmd, union bpf_attr *attr,
> +            unsigned int size)
> +{
> +       char write_data[64] = "hello there, world!!";
> +       struct bpf_dynptr ptr;
> +
> +       bpf_ringbuf_reserve_dynptr(&ringbuf, sizeof(write_data), 0, &ptr);
> +
> +       return bpf_verify_pkcs7_signature(&ptr, &ptr, NULL);
> +}
> +
> +SEC("?lsm.s/bpf")
> +int BPF_PROG(not_valid_dynptr, int cmd, union bpf_attr *attr, unsigned int size)
> +{
> +       unsigned long val;
> +
> +       return bpf_verify_pkcs7_signature((struct bpf_dynptr *)&val,
> +                                         (struct bpf_dynptr *)&val, NULL);
> +}
> +
> +SEC("?lsm.s/bpf")
> +int BPF_PROG(not_ptr_to_stack, int cmd, union bpf_attr *attr, unsigned int size)
> +{
> +       unsigned long val;
> +
> +       return bpf_verify_pkcs7_signature((struct bpf_dynptr *)val,
> +                                         (struct bpf_dynptr *)val, NULL);

Please also include a test where you cause the dynptr to be set to
NULL, e.g. by passing invalid stuff to ringbuf_reserve_dynptr, and
then try to pass it to bpf_verify_pkc7_signature.

> +}
> --
> 2.25.1
>

Roberto Sassu Sept. 6, 2022, 7:08 a.m. UTC | #8

On Tue, 2022-09-06 at 00:38 +0300, Jarkko Sakkinen wrote:
> On Mon, Sep 05, 2022 at 04:33:11PM +0200, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > 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. Add them to enum key_lookup_flag, so
> > that
> > all the current ones and the ones defined in the future are
> > automatically
> > exported through BTF and available to eBPF programs.
> > 
> > Also, add KEY_LOOKUP_ALL to the enum, to facilitate checking
> > whether a
> > variable contains only defined flags.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Reviewed-by: KP Singh <kpsingh@kernel.org>
> > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> You should remove ack if there is any substantial change.

Yes, sorry. I thought you were fine with the change due to:

https://lore.kernel.org/bpf/YxF4H9MTDj+PnJ+V@kernel.org/

> 
> > ---
> >  include/linux/key.h      | 6 ++++++
> >  security/keys/internal.h | 2 --
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/key.h b/include/linux/key.h
> > index 7febc4881363..d84171f90cbd 100644
> > --- a/include/linux/key.h
> > +++ b/include/linux/key.h
> > @@ -88,6 +88,12 @@ enum key_need_perm {
> >  	KEY_DEFER_PERM_CHECK,	/* Special: permission check is
> > deferred */
> >  };
> >  
> > +enum key_lookup_flag {
> > +	KEY_LOOKUP_CREATE = 0x01,	/* Create special keyrings if they
> > don't exist */
> > +	KEY_LOOKUP_PARTIAL = 0x02,	/* Permit partially constructed
> > keys to be found */
> > +	KEY_LOOKUP_ALL = (KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL), /*
> > OR of previous flags */
> 
> Drop the comments (should be reviewed separately + out of context).

The same style is used for many definitions in include/linux/key.h

No problem to remove them, please just let me know where they should
be. Often, eBPF maintainers asked me to add a description to the code
to explain how new definitions should be used.

Thanks

Roberto

Roberto Sassu Sept. 6, 2022, 7:35 a.m. UTC | #9

On Mon, 2022-09-05 at 21:26 +0200, Kumar Kartikeya Dwivedi wrote:
> On Mon, 5 Sept 2022 at 16:34, Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > 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.
> > 
> > [...]
> 
> CI is crashing with NULL deref for test_progs-no_alu32 with llvm-16,
> but I don't think the problem is in this series. This is most likely
> unrelated to BPF, as the crash happens inside
> kernel/time/tick-sched.c:tick_nohz_restart_sched_tick.
> 
> This was the same case in
> https://lore.kernel.org/bpf/CAP01T74steDfP6O8QOshoto3e3RnHhKtAeTbnrPBZS3YJXjvbA@mail.gmail.com.
> 
> So, 
> https://github.com/kernel-patches/bpf/runs/8194263557?check_suite_focus=true
> and 
> https://github.com/kernel-patches/bpf/runs/7982907380?check_suite_focus=true
> 
> look similar to me, and may not be related to BPF. They only trigger
> during runs compiled using LLVM 16, so maybe some compiler
> transformation is surfacing the problem?

Yes, I saw that too. Not sure what the cause could be.

Thanks

Roberto

Roberto Sassu Sept. 6, 2022, 8:07 a.m. UTC | #10

On Tue, 2022-09-06 at 04:57 +0200, Kumar Kartikeya Dwivedi wrote:
> On Mon, 5 Sept 2022 at 16:35, Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > 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>
> > Acked-by: KP Singh <kpsingh@kernel.org>
> > ---
> >  kernel/trace/bpf_trace.c | 45
> > ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> > 
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 7a7023704ac2..8e2c026b0a58 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -1294,12 +1294,57 @@ 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.
> > + */
> > +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->has_ref) {
> > +               /*
> > +                * Do the permission check deferred in
> > bpf_lookup_user_key().
> > +                * See bpf_lookup_user_key() for more details.
> > +                *
> > +                * 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),
> 
> MIssing check for data_ptr->data == NULL before making this call?
> Same
> for sig_ptr.

Patch 3 requires the dynptrs to be initialized. Isn't enough?

Thanks

Roberto

Roberto Sassu Sept. 6, 2022, 8:30 a.m. UTC | #11

On Tue, 2022-09-06 at 05:15 +0200, Kumar Kartikeya Dwivedi wrote:
> On Mon, 5 Sept 2022 at 16:36, Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Add tests to ensure that only supported dynamic pointer types are
> > accepted,
> > that the passed argument is actually a dynamic pointer, and that
> > the passed
> > argument is a pointer to the stack.
> > 
> > The tests are currently in the deny list for s390x (JIT does not
> > support
> > calling kernel function).
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  tools/testing/selftests/bpf/DENYLIST.s390x    |   1 +
> >  .../bpf/prog_tests/kfunc_dynptr_param.c       | 103
> > ++++++++++++++++++
> >  .../bpf/progs/test_kfunc_dynptr_param.c       |  57 ++++++++++
> >  3 files changed, 161 insertions(+)
> >  create mode 100644
> > tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
> >  create mode 100644
> > tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c
> > 
> > diff --git a/tools/testing/selftests/bpf/DENYLIST.s390x
> > b/tools/testing/selftests/bpf/DENYLIST.s390x
> > index 4e305baa5277..9a6dc3671c65 100644
> > --- a/tools/testing/selftests/bpf/DENYLIST.s390x
> > +++ b/tools/testing/selftests/bpf/DENYLIST.s390x
> > @@ -71,3 +71,4 @@ cgroup_hierarchical_stats                # JIT
> > does not support calling kernel f
> >  htab_update                              # failed to attach:
> > ERROR: strerror_r(-
> > 524)=22                                (trampoline)
> >  lookup_key                               # JIT does not support
> > calling kernel function                                (kfunc)
> >  verify_pkcs7_sig                         # JIT does not support
> > calling kernel function                                (kfunc)
> > +kfunc_dynptr_param                       # JIT does not support
> > calling kernel function                                (kfunc)
> > diff --git
> > a/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
> > b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
> > new file mode 100644
> > index 000000000000..ea655a5c9d8b
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
> > @@ -0,0 +1,103 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Copyright (c) 2022 Facebook
> > + * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Author: Roberto Sassu <roberto.sassu@huawei.com>
> > + */
> > +
> > +#include <test_progs.h>
> > +#include "test_kfunc_dynptr_param.skel.h"
> > +
> > +static size_t log_buf_sz = 1048576; /* 1 MB */
> > +static char obj_log_buf[1048576];
> > +
> > +static struct {
> > +       const char *prog_name;
> > +       const char *expected_err_msg;
> > +} kfunc_dynptr_tests[] = {
> > +       {"dynptr_type_not_supp",
> > +        "arg#0 pointer type STRUCT bpf_dynptr_kern points to
> > unsupported dynamic pointer type"},
> > +       {"not_valid_dynptr",
> > +        "arg#0 pointer type STRUCT bpf_dynptr_kern must be valid
> > and initialized"},
> > +       {"not_ptr_to_stack", "arg#0 pointer type STRUCT
> > bpf_dynptr_kern not to stack"},
> > +};
> > +
> > +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 void verify_fail(const char *prog_name, const char
> > *expected_err_msg)
> > +{
> > +       struct test_kfunc_dynptr_param *skel;
> > +       LIBBPF_OPTS(bpf_object_open_opts, opts);
> > +       libbpf_print_fn_t old_print_cb;
> > +       struct bpf_program *prog;
> > +       int err;
> > +
> > +       opts.kernel_log_buf = obj_log_buf;
> > +       opts.kernel_log_size = log_buf_sz;
> > +       opts.kernel_log_level = 1;
> > +
> > +       skel = test_kfunc_dynptr_param__open_opts(&opts);
> > +       if (!ASSERT_OK_PTR(skel,
> > "test_kfunc_dynptr_param__open_opts"))
> > +               goto cleanup;
> > +
> > +       prog = bpf_object__find_program_by_name(skel->obj,
> > prog_name);
> > +       if (!ASSERT_OK_PTR(prog,
> > "bpf_object__find_program_by_name"))
> > +               goto cleanup;
> > +
> > +       bpf_program__set_autoload(prog, true);
> > +
> > +       bpf_map__set_max_entries(skel->maps.ringbuf,
> > getpagesize());
> > +
> > +       kfunc_not_supported = false;
> > +
> > +       old_print_cb = libbpf_set_print(libbpf_print_cb);
> > +       err = test_kfunc_dynptr_param__load(skel);
> > +       libbpf_set_print(old_print_cb);
> > +
> > +       if (err < 0 && kfunc_not_supported) {
> > +               fprintf(stderr,
> > +                 "%s:SKIP:bpf_verify_pkcs7_signature() kfunc not
> > supported\n",
> > +                 __func__);
> > +               test__skip();
> > +               goto cleanup;
> > +       }
> > +
> > +       if (!ASSERT_ERR(err, "unexpected load success"))
> > +               goto cleanup;
> > +
> > +       if (!ASSERT_OK_PTR(strstr(obj_log_buf, expected_err_msg),
> > "expected_err_msg")) {
> > +               fprintf(stderr, "Expected err_msg: %s\n",
> > expected_err_msg);
> > +               fprintf(stderr, "Verifier output: %s\n",
> > obj_log_buf);
> > +       }
> > +
> > +cleanup:
> > +       test_kfunc_dynptr_param__destroy(skel);
> > +}
> > +
> > +void test_kfunc_dynptr_param(void)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(kfunc_dynptr_tests); i++) {
> > +               if
> > (!test__start_subtest(kfunc_dynptr_tests[i].prog_name))
> > +                       continue;
> > +
> > +               verify_fail(kfunc_dynptr_tests[i].prog_name,
> > +                           kfunc_dynptr_tests[i].expected_err_msg)
> > ;
> > +       }
> > +}
> > diff --git
> > a/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c
> > b/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c
> > new file mode 100644
> > index 000000000000..2f09f91a1576
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c
> > @@ -0,0 +1,57 @@
> > +// 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>
> > +
> > +struct bpf_dynptr {
> > +       __u64 :64;
> > +       __u64 :64;
> > +} __attribute__((aligned(8)));
> > +
> > +extern int bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr,
> > +                                     struct bpf_dynptr *sig_ptr,
> > +                                     struct bpf_key
> > *trusted_keyring) __ksym;
> > +
> > +struct {
> > +       __uint(type, BPF_MAP_TYPE_RINGBUF);
> > +} ringbuf SEC(".maps");
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +SEC("?lsm.s/bpf")
> > +int BPF_PROG(dynptr_type_not_supp, int cmd, union bpf_attr *attr,
> > +            unsigned int size)
> > +{
> > +       char write_data[64] = "hello there, world!!";
> > +       struct bpf_dynptr ptr;
> > +
> > +       bpf_ringbuf_reserve_dynptr(&ringbuf, sizeof(write_data), 0,
> > &ptr);
> > +
> > +       return bpf_verify_pkcs7_signature(&ptr, &ptr, NULL);
> > +}
> > +
> > +SEC("?lsm.s/bpf")
> > +int BPF_PROG(not_valid_dynptr, int cmd, union bpf_attr *attr,
> > unsigned int size)
> > +{
> > +       unsigned long val;
> > +
> > +       return bpf_verify_pkcs7_signature((struct bpf_dynptr
> > *)&val,
> > +                                         (struct bpf_dynptr
> > *)&val, NULL);
> > +}
> > +
> > +SEC("?lsm.s/bpf")
> > +int BPF_PROG(not_ptr_to_stack, int cmd, union bpf_attr *attr,
> > unsigned int size)
> > +{
> > +       unsigned long val;
> > +
> > +       return bpf_verify_pkcs7_signature((struct bpf_dynptr *)val,
> > +                                         (struct bpf_dynptr *)val,
> > NULL);
> 
> Please also include a test where you cause the dynptr to be set to
> NULL, e.g. by passing invalid stuff to ringbuf_reserve_dynptr, and
> then try to pass it to bpf_verify_pkc7_signature.

Uhm, bpf_ringbuf_reserve_dynptr() is expecting a valid map. How else I
can achieve it?

Thanks

Roberto

Jarkko Sakkinen Sept. 6, 2022, 10:37 a.m. UTC | #12

On Tue, Sep 06, 2022 at 09:08:23AM +0200, Roberto Sassu wrote:
> On Tue, 2022-09-06 at 00:38 +0300, Jarkko Sakkinen wrote:
> > On Mon, Sep 05, 2022 at 04:33:11PM +0200, Roberto Sassu wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > 
> > > 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. Add them to enum key_lookup_flag, so
> > > that
> > > all the current ones and the ones defined in the future are
> > > automatically
> > > exported through BTF and available to eBPF programs.
> > > 
> > > Also, add KEY_LOOKUP_ALL to the enum, to facilitate checking
> > > whether a
> > > variable contains only defined flags.
> > > 
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Reviewed-by: KP Singh <kpsingh@kernel.org>
> > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > You should remove ack if there is any substantial change.
> 
> Yes, sorry. I thought you were fine with the change due to:
> 
> https://lore.kernel.org/bpf/YxF4H9MTDj+PnJ+V@kernel.org/

It was the documentation part, not really the enum change.

BR, Jarkko

Roberto Sassu Sept. 6, 2022, 11:04 a.m. UTC | #13

On Tue, 2022-09-06 at 13:37 +0300, Jarkko Sakkinen wrote:
> On Tue, Sep 06, 2022 at 09:08:23AM +0200, Roberto Sassu wrote:
> > On Tue, 2022-09-06 at 00:38 +0300, Jarkko Sakkinen wrote:
> > > On Mon, Sep 05, 2022 at 04:33:11PM +0200, Roberto Sassu wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > 
> > > > 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. Add them to enum
> > > > key_lookup_flag, so
> > > > that
> > > > all the current ones and the ones defined in the future are
> > > > automatically
> > > > exported through BTF and available to eBPF programs.
> > > > 
> > > > Also, add KEY_LOOKUP_ALL to the enum, to facilitate checking
> > > > whether a
> > > > variable contains only defined flags.
> > > > 
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > Reviewed-by: KP Singh <kpsingh@kernel.org>
> > > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > 
> > > You should remove ack if there is any substantial change.
> > 
> > Yes, sorry. I thought you were fine with the change due to:
> > 
> > https://lore.kernel.org/bpf/YxF4H9MTDj+PnJ+V@kernel.org/
> 
> It was the documentation part, not really the enum change.
> 

Ok, so if I remove the documentation I can keep your ack?

Thanks

Roberto

Jarkko Sakkinen Sept. 6, 2022, 11:43 a.m. UTC | #14

On Tue, Sep 06, 2022 at 01:04:35PM +0200, Roberto Sassu wrote:
> On Tue, 2022-09-06 at 13:37 +0300, Jarkko Sakkinen wrote:
> > On Tue, Sep 06, 2022 at 09:08:23AM +0200, Roberto Sassu wrote:
> > > On Tue, 2022-09-06 at 00:38 +0300, Jarkko Sakkinen wrote:
> > > > On Mon, Sep 05, 2022 at 04:33:11PM +0200, Roberto Sassu wrote:
> > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > 
> > > > > 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. Add them to enum
> > > > > key_lookup_flag, so
> > > > > that
> > > > > all the current ones and the ones defined in the future are
> > > > > automatically
> > > > > exported through BTF and available to eBPF programs.
> > > > > 
> > > > > Also, add KEY_LOOKUP_ALL to the enum, to facilitate checking
> > > > > whether a
> > > > > variable contains only defined flags.
> > > > > 
> > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > Reviewed-by: KP Singh <kpsingh@kernel.org>
> > > > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > 
> > > > You should remove ack if there is any substantial change.
> > > 
> > > Yes, sorry. I thought you were fine with the change due to:
> > > 
> > > https://lore.kernel.org/bpf/YxF4H9MTDj+PnJ+V@kernel.org/
> > 
> > It was the documentation part, not really the enum change.
> > 
> 
> Ok, so if I remove the documentation I can keep your ack?

Can you show the updated patch (e.g. via using attachment)?

BR, Jarkko

Kumar Kartikeya Dwivedi Sept. 7, 2022, 2:28 a.m. UTC | #15

On Tue, 6 Sept 2022 at 10:08, Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> On Tue, 2022-09-06 at 04:57 +0200, Kumar Kartikeya Dwivedi wrote:
> > On Mon, 5 Sept 2022 at 16:35, Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > >
> > > 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>
> > > Acked-by: KP Singh <kpsingh@kernel.org>
> > > ---
> > >  kernel/trace/bpf_trace.c | 45
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 45 insertions(+)
> > >
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index 7a7023704ac2..8e2c026b0a58 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -1294,12 +1294,57 @@ 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.
> > > + */
> > > +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->has_ref) {
> > > +               /*
> > > +                * Do the permission check deferred in
> > > bpf_lookup_user_key().
> > > +                * See bpf_lookup_user_key() for more details.
> > > +                *
> > > +                * 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),
> >
> > MIssing check for data_ptr->data == NULL before making this call?
> > Same
> > for sig_ptr.
>
> Patch 3 requires the dynptrs to be initialized. Isn't enough?
>

No, it seems even initialized dynptr can be NULL at runtime. Look at
both ringbuf_submit_dynptr and ringbuf_discard_dynptr.
The verifier won't know after ringbuf_reserve_dynptr whether it set it
to NULL or some valid pointer.

dynptr_init is basically that stack slot is now STACK_DYNPTR, it says
nothing more about the dynptr.

As far as testing this goes, you can pass invalid parameters to
ringbuf_reserve_dynptr to have it set to NULL, then make sure your
helper returns an error at runtime for it.

> Thanks
>
> Roberto
>

Kumar Kartikeya Dwivedi Sept. 7, 2022, 2:34 a.m. UTC | #16

On Tue, 6 Sept 2022 at 10:31, Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> On Tue, 2022-09-06 at 05:15 +0200, Kumar Kartikeya Dwivedi wrote:
> > On Mon, 5 Sept 2022 at 16:36, Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > >
> > > Add tests to ensure that only supported dynamic pointer types are
> > > accepted,
> > > that the passed argument is actually a dynamic pointer, and that
> > > the passed
> > > argument is a pointer to the stack.
> > >
> > > The tests are currently in the deny list for s390x (JIT does not
> > > support
> > > calling kernel function).
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >  tools/testing/selftests/bpf/DENYLIST.s390x    |   1 +
> > >  .../bpf/prog_tests/kfunc_dynptr_param.c       | 103
> > > ++++++++++++++++++
> > >  .../bpf/progs/test_kfunc_dynptr_param.c       |  57 ++++++++++
> > >  3 files changed, 161 insertions(+)
> > >  create mode 100644
> > > tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
> > >  create mode 100644
> > > tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/DENYLIST.s390x
> > > b/tools/testing/selftests/bpf/DENYLIST.s390x
> > > index 4e305baa5277..9a6dc3671c65 100644
> > > --- a/tools/testing/selftests/bpf/DENYLIST.s390x
> > > +++ b/tools/testing/selftests/bpf/DENYLIST.s390x
> > > @@ -71,3 +71,4 @@ cgroup_hierarchical_stats                # JIT
> > > does not support calling kernel f
> > >  htab_update                              # failed to attach:
> > > ERROR: strerror_r(-
> > > 524)=22                                (trampoline)
> > >  lookup_key                               # JIT does not support
> > > calling kernel function                                (kfunc)
> > >  verify_pkcs7_sig                         # JIT does not support
> > > calling kernel function                                (kfunc)
> > > +kfunc_dynptr_param                       # JIT does not support
> > > calling kernel function                                (kfunc)
> > > diff --git
> > > a/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
> > > b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
> > > new file mode 100644
> > > index 000000000000..ea655a5c9d8b
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c
> > > @@ -0,0 +1,103 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +/*
> > > + * Copyright (c) 2022 Facebook
> > > + * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH
> > > + *
> > > + * Author: Roberto Sassu <roberto.sassu@huawei.com>
> > > + */
> > > +
> > > +#include <test_progs.h>
> > > +#include "test_kfunc_dynptr_param.skel.h"
> > > +
> > > +static size_t log_buf_sz = 1048576; /* 1 MB */
> > > +static char obj_log_buf[1048576];
> > > +
> > > +static struct {
> > > +       const char *prog_name;
> > > +       const char *expected_err_msg;
> > > +} kfunc_dynptr_tests[] = {
> > > +       {"dynptr_type_not_supp",
> > > +        "arg#0 pointer type STRUCT bpf_dynptr_kern points to
> > > unsupported dynamic pointer type"},
> > > +       {"not_valid_dynptr",
> > > +        "arg#0 pointer type STRUCT bpf_dynptr_kern must be valid
> > > and initialized"},
> > > +       {"not_ptr_to_stack", "arg#0 pointer type STRUCT
> > > bpf_dynptr_kern not to stack"},
> > > +};
> > > +
> > > +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 void verify_fail(const char *prog_name, const char
> > > *expected_err_msg)
> > > +{
> > > +       struct test_kfunc_dynptr_param *skel;
> > > +       LIBBPF_OPTS(bpf_object_open_opts, opts);
> > > +       libbpf_print_fn_t old_print_cb;
> > > +       struct bpf_program *prog;
> > > +       int err;
> > > +
> > > +       opts.kernel_log_buf = obj_log_buf;
> > > +       opts.kernel_log_size = log_buf_sz;
> > > +       opts.kernel_log_level = 1;
> > > +
> > > +       skel = test_kfunc_dynptr_param__open_opts(&opts);
> > > +       if (!ASSERT_OK_PTR(skel,
> > > "test_kfunc_dynptr_param__open_opts"))
> > > +               goto cleanup;
> > > +
> > > +       prog = bpf_object__find_program_by_name(skel->obj,
> > > prog_name);
> > > +       if (!ASSERT_OK_PTR(prog,
> > > "bpf_object__find_program_by_name"))
> > > +               goto cleanup;
> > > +
> > > +       bpf_program__set_autoload(prog, true);
> > > +
> > > +       bpf_map__set_max_entries(skel->maps.ringbuf,
> > > getpagesize());
> > > +
> > > +       kfunc_not_supported = false;
> > > +
> > > +       old_print_cb = libbpf_set_print(libbpf_print_cb);
> > > +       err = test_kfunc_dynptr_param__load(skel);
> > > +       libbpf_set_print(old_print_cb);
> > > +
> > > +       if (err < 0 && kfunc_not_supported) {
> > > +               fprintf(stderr,
> > > +                 "%s:SKIP:bpf_verify_pkcs7_signature() kfunc not
> > > supported\n",
> > > +                 __func__);
> > > +               test__skip();
> > > +               goto cleanup;
> > > +       }
> > > +
> > > +       if (!ASSERT_ERR(err, "unexpected load success"))
> > > +               goto cleanup;
> > > +
> > > +       if (!ASSERT_OK_PTR(strstr(obj_log_buf, expected_err_msg),
> > > "expected_err_msg")) {
> > > +               fprintf(stderr, "Expected err_msg: %s\n",
> > > expected_err_msg);
> > > +               fprintf(stderr, "Verifier output: %s\n",
> > > obj_log_buf);
> > > +       }
> > > +
> > > +cleanup:
> > > +       test_kfunc_dynptr_param__destroy(skel);
> > > +}
> > > +
> > > +void test_kfunc_dynptr_param(void)
> > > +{
> > > +       int i;
> > > +
> > > +       for (i = 0; i < ARRAY_SIZE(kfunc_dynptr_tests); i++) {
> > > +               if
> > > (!test__start_subtest(kfunc_dynptr_tests[i].prog_name))
> > > +                       continue;
> > > +
> > > +               verify_fail(kfunc_dynptr_tests[i].prog_name,
> > > +                           kfunc_dynptr_tests[i].expected_err_msg)
> > > ;
> > > +       }
> > > +}
> > > diff --git
> > > a/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c
> > > b/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c
> > > new file mode 100644
> > > index 000000000000..2f09f91a1576
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c
> > > @@ -0,0 +1,57 @@
> > > +// 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>
> > > +
> > > +struct bpf_dynptr {
> > > +       __u64 :64;
> > > +       __u64 :64;
> > > +} __attribute__((aligned(8)));
> > > +
> > > +extern int bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr,
> > > +                                     struct bpf_dynptr *sig_ptr,
> > > +                                     struct bpf_key
> > > *trusted_keyring) __ksym;
> > > +
> > > +struct {
> > > +       __uint(type, BPF_MAP_TYPE_RINGBUF);
> > > +} ringbuf SEC(".maps");
> > > +
> > > +char _license[] SEC("license") = "GPL";
> > > +
> > > +SEC("?lsm.s/bpf")
> > > +int BPF_PROG(dynptr_type_not_supp, int cmd, union bpf_attr *attr,
> > > +            unsigned int size)
> > > +{
> > > +       char write_data[64] = "hello there, world!!";
> > > +       struct bpf_dynptr ptr;
> > > +
> > > +       bpf_ringbuf_reserve_dynptr(&ringbuf, sizeof(write_data), 0,
> > > &ptr);
> > > +
> > > +       return bpf_verify_pkcs7_signature(&ptr, &ptr, NULL);
> > > +}
> > > +
> > > +SEC("?lsm.s/bpf")
> > > +int BPF_PROG(not_valid_dynptr, int cmd, union bpf_attr *attr,
> > > unsigned int size)
> > > +{
> > > +       unsigned long val;
> > > +
> > > +       return bpf_verify_pkcs7_signature((struct bpf_dynptr
> > > *)&val,
> > > +                                         (struct bpf_dynptr
> > > *)&val, NULL);
> > > +}
> > > +
> > > +SEC("?lsm.s/bpf")
> > > +int BPF_PROG(not_ptr_to_stack, int cmd, union bpf_attr *attr,
> > > unsigned int size)
> > > +{
> > > +       unsigned long val;
> > > +
> > > +       return bpf_verify_pkcs7_signature((struct bpf_dynptr *)val,
> > > +                                         (struct bpf_dynptr *)val,
> > > NULL);
> >
> > Please also include a test where you cause the dynptr to be set to
> > NULL, e.g. by passing invalid stuff to ringbuf_reserve_dynptr, and
> > then try to pass it to bpf_verify_pkc7_signature.
>
> Uhm, bpf_ringbuf_reserve_dynptr() is expecting a valid map. How else I
> can achieve it?

So? Just define a ringbuf map and pass to it? I'm missing why that is
undesirable.
It also needs to be a runtime test, not verifier test, I probably
replied to the wrong patch.

Roberto Sassu Sept. 7, 2022, 12:19 p.m. UTC | #17

On Wed, 2022-09-07 at 04:28 +0200, Kumar Kartikeya Dwivedi wrote:
> On Tue, 6 Sept 2022 at 10:08, Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > On Tue, 2022-09-06 at 04:57 +0200, Kumar Kartikeya Dwivedi wrote:
> > > On Mon, 5 Sept 2022 at 16:35, Roberto Sassu
> > > <roberto.sassu@huaweicloud.com> wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > 
> > > > 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>
> > > > Acked-by: KP Singh <kpsingh@kernel.org>
> > > > ---
> > > >  kernel/trace/bpf_trace.c | 45
> > > > ++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 45 insertions(+)
> > > > 
> > > > diff --git a/kernel/trace/bpf_trace.c
> > > > b/kernel/trace/bpf_trace.c
> > > > index 7a7023704ac2..8e2c026b0a58 100644
> > > > --- a/kernel/trace/bpf_trace.c
> > > > +++ b/kernel/trace/bpf_trace.c
> > > > @@ -1294,12 +1294,57 @@ 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.
> > > > + */
> > > > +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->has_ref) {
> > > > +               /*
> > > > +                * Do the permission check deferred in
> > > > bpf_lookup_user_key().
> > > > +                * See bpf_lookup_user_key() for more details.
> > > > +                *
> > > > +                * 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_p
> > > > tr),
> > > 
> > > MIssing check for data_ptr->data == NULL before making this call?
> > > Same
> > > for sig_ptr.
> > 
> > Patch 3 requires the dynptrs to be initialized. Isn't enough?
> > 
> 
> No, it seems even initialized dynptr can be NULL at runtime. Look at
> both ringbuf_submit_dynptr and ringbuf_discard_dynptr.
> The verifier won't know after ringbuf_reserve_dynptr whether it set
> it
> to NULL or some valid pointer.
> 
> dynptr_init is basically that stack slot is now STACK_DYNPTR, it says
> nothing more about the dynptr.
> 
> As far as testing this goes, you can pass invalid parameters to
> ringbuf_reserve_dynptr to have it set to NULL, then make sure your
> helper returns an error at runtime for it.

I see, thanks.

I did a quick test. Pass 1 as flags argument to bpf_dynptr_from_mem()
(not supported), and see how bpf_verify_pkcs7_signature() handles it.

Everything seems good, the ASN1 parser called by pkcs7_parse_message()
correctly handles zero length.

So, I will add just this test, right?

Thanks

Roberto

Kumar Kartikeya Dwivedi Sept. 7, 2022, 1:55 p.m. UTC | #18

On Wed, 7 Sept 2022 at 14:20, Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> On Wed, 2022-09-07 at 04:28 +0200, Kumar Kartikeya Dwivedi wrote:
> > On Tue, 6 Sept 2022 at 10:08, Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > > On Tue, 2022-09-06 at 04:57 +0200, Kumar Kartikeya Dwivedi wrote:
> > > > On Mon, 5 Sept 2022 at 16:35, Roberto Sassu
> > > > <roberto.sassu@huaweicloud.com> wrote:
> > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > >
> > > > > 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>
> > > > > Acked-by: KP Singh <kpsingh@kernel.org>
> > > > > ---
> > > > >  kernel/trace/bpf_trace.c | 45
> > > > > ++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 45 insertions(+)
> > > > >
> > > > > diff --git a/kernel/trace/bpf_trace.c
> > > > > b/kernel/trace/bpf_trace.c
> > > > > index 7a7023704ac2..8e2c026b0a58 100644
> > > > > --- a/kernel/trace/bpf_trace.c
> > > > > +++ b/kernel/trace/bpf_trace.c
> > > > > @@ -1294,12 +1294,57 @@ 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.
> > > > > + */
> > > > > +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->has_ref) {
> > > > > +               /*
> > > > > +                * Do the permission check deferred in
> > > > > bpf_lookup_user_key().
> > > > > +                * See bpf_lookup_user_key() for more details.
> > > > > +                *
> > > > > +                * 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_p
> > > > > tr),
> > > >
> > > > MIssing check for data_ptr->data == NULL before making this call?
> > > > Same
> > > > for sig_ptr.
> > >
> > > Patch 3 requires the dynptrs to be initialized. Isn't enough?
> > >
> >
> > No, it seems even initialized dynptr can be NULL at runtime. Look at
> > both ringbuf_submit_dynptr and ringbuf_discard_dynptr.
> > The verifier won't know after ringbuf_reserve_dynptr whether it set
> > it
> > to NULL or some valid pointer.
> >
> > dynptr_init is basically that stack slot is now STACK_DYNPTR, it says
> > nothing more about the dynptr.
> >
> > As far as testing this goes, you can pass invalid parameters to
> > ringbuf_reserve_dynptr to have it set to NULL, then make sure your
> > helper returns an error at runtime for it.
>
> I see, thanks.
>
> I did a quick test. Pass 1 as flags argument to bpf_dynptr_from_mem()
> (not supported), and see how bpf_verify_pkcs7_signature() handles it.
>
> Everything seems good, the ASN1 parser called by pkcs7_parse_message()
> correctly handles zero length.
>
> So, I will add just this test, right?
>

Yeah, if it handles it correctly, just adding a test to make sure it
stays that way in the future would be fine.

Roberto Sassu Sept. 7, 2022, 2:49 p.m. UTC | #19

On Tue, 2022-09-06 at 09:35 +0200, Roberto Sassu wrote:
> On Mon, 2022-09-05 at 21:26 +0200, Kumar Kartikeya Dwivedi wrote:
> > On Mon, 5 Sept 2022 at 16:34, Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > 
> > > 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.
> > > 
> > > [...]
> > 
> > CI is crashing with NULL deref for test_progs-no_alu32 with llvm-
> > 16,
> > but I don't think the problem is in this series. This is most
> > likely
> > unrelated to BPF, as the crash happens inside
> > kernel/time/tick-sched.c:tick_nohz_restart_sched_tick.
> > 
> > This was the same case in
> > https://lore.kernel.org/bpf/CAP01T74steDfP6O8QOshoto3e3RnHhKtAeTbnrPBZS3YJXjvbA@mail.gmail.com.
> > 
> > So, 
> > https://github.com/kernel-patches/bpf/runs/8194263557?check_suite_focus=true
> > and 
> > https://github.com/kernel-patches/bpf/runs/7982907380?check_suite_focus=true
> > 
> > look similar to me, and may not be related to BPF. They only
> > trigger
> > during runs compiled using LLVM 16, so maybe some compiler
> > transformation is surfacing the problem?
> 
> Yes, I saw that too. Not sure what the cause could be.
> 

Another occurrence, this time with gcc:

https://github.com/robertosassu/vmtest/runs/8230071814?check_suite_focus=true

Roberto

Kumar Kartikeya Dwivedi Sept. 7, 2022, 2:57 p.m. UTC | #20

On Wed, 7 Sept 2022 at 16:49, Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> On Tue, 2022-09-06 at 09:35 +0200, Roberto Sassu wrote:
> > On Mon, 2022-09-05 at 21:26 +0200, Kumar Kartikeya Dwivedi wrote:
> > > On Mon, 5 Sept 2022 at 16:34, Roberto Sassu
> > > <roberto.sassu@huaweicloud.com> wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > >
> > > > 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.
> > > >
> > > > [...]
> > >
> > > CI is crashing with NULL deref for test_progs-no_alu32 with llvm-
> > > 16,
> > > but I don't think the problem is in this series. This is most
> > > likely
> > > unrelated to BPF, as the crash happens inside
> > > kernel/time/tick-sched.c:tick_nohz_restart_sched_tick.
> > >
> > > This was the same case in
> > > https://lore.kernel.org/bpf/CAP01T74steDfP6O8QOshoto3e3RnHhKtAeTbnrPBZS3YJXjvbA@mail.gmail.com.
> > >
> > > So,
> > > https://github.com/kernel-patches/bpf/runs/8194263557?check_suite_focus=true
> > > and
> > > https://github.com/kernel-patches/bpf/runs/7982907380?check_suite_focus=true
> > >
> > > look similar to me, and may not be related to BPF. They only
> > > trigger
> > > during runs compiled using LLVM 16, so maybe some compiler
> > > transformation is surfacing the problem?
> >
> > Yes, I saw that too. Not sure what the cause could be.
> >
>
> Another occurrence, this time with gcc:
>
> https://github.com/robertosassu/vmtest/runs/8230071814?check_suite_focus=true
>

... and it seems like this run does not even have your patches, right?

> Roberto
>

Roberto Sassu Sept. 7, 2022, 3:09 p.m. UTC | #21

On Wed, 2022-09-07 at 16:57 +0200, Kumar Kartikeya Dwivedi wrote:
> On Wed, 7 Sept 2022 at 16:49, Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > On Tue, 2022-09-06 at 09:35 +0200, Roberto Sassu wrote:
> > > On Mon, 2022-09-05 at 21:26 +0200, Kumar Kartikeya Dwivedi wrote:
> > > > On Mon, 5 Sept 2022 at 16:34, Roberto Sassu
> > > > <roberto.sassu@huaweicloud.com> wrote:
> > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > 
> > > > > 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.
> > > > > 
> > > > > [...]
> > > > 
> > > > CI is crashing with NULL deref for test_progs-no_alu32 with
> > > > llvm-
> > > > 16,
> > > > but I don't think the problem is in this series. This is most
> > > > likely
> > > > unrelated to BPF, as the crash happens inside
> > > > kernel/time/tick-sched.c:tick_nohz_restart_sched_tick.
> > > > 
> > > > This was the same case in
> > > > https://lore.kernel.org/bpf/CAP01T74steDfP6O8QOshoto3e3RnHhKtAeTbnrPBZS3YJXjvbA@mail.gmail.com.
> > > > 
> > > > So,
> > > > https://github.com/kernel-patches/bpf/runs/8194263557?check_suite_focus=true
> > > > and
> > > > https://github.com/kernel-patches/bpf/runs/7982907380?check_suite_focus=true
> > > > 
> > > > look similar to me, and may not be related to BPF. They only
> > > > trigger
> > > > during runs compiled using LLVM 16, so maybe some compiler
> > > > transformation is surfacing the problem?
> > > 
> > > Yes, I saw that too. Not sure what the cause could be.
> > > 
> > 
> > Another occurrence, this time with gcc:
> > 
> > https://github.com/robertosassu/vmtest/runs/8230071814?check_suite_focus=true
> > 
> 
> ... and it seems like this run does not even have your patches,
> right?
> 

Uhm, the kernel patches are there. The tests except the verifier ones
weren't successfuly applied, probably due to the deny list.

One thing in common with the failures seems when the panic happens,
when test_progs reaches verif_twfw. I will try to execute this and
earlier tests to reproduce the panic locally.

Roberto

[v16,00/12] bpf: Add kfuncs for PKCS#7 signature verification

Message

Comments