Re: [PATCH v2 6/6] integrity: PowerVM support for loading third party code signing keys

["Jarkko Sakkinen" <jarkko@kernel.org>]

On Wed Aug 9, 2023 at 10:53 PM EEST, Nayna Jain wrote:
> On secure boot enabled PowerVM LPAR, third party code signing keys are
> needed during early boot to verify signed third party modules. These
> third party keys are stored in moduledb object in the Platform
> KeyStore(PKS).
          ^ space

>
> Load third party code signing keys onto .secondary_trusted_keys keyring.
>
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> ---
>  certs/system_keyring.c                        | 23 +++++++++++++++++++
>  include/keys/system_keyring.h                 |  7 ++++++
>  security/integrity/integrity.h                |  1 +
>  .../platform_certs/keyring_handler.c          |  8 +++++++
>  .../platform_certs/keyring_handler.h          |  5 ++++
>  .../integrity/platform_certs/load_powerpc.c   | 18 ++++++++++++++-
>  6 files changed, 61 insertions(+), 1 deletion(-)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index b348e0898d34..3435d4936fb2 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -396,3 +396,26 @@ void __init set_platform_trusted_keys(struct key *keyring)
>  	platform_trusted_keys = keyring;
>  }
>  #endif
> +

spurious newline character

> +void __init add_to_secondary_keyring(const char *source, const void *data,
> +				     size_t len)

Documentation is lacking, and should be in a single line, as it totals
less than 100 characters.

> +{
> +	key_ref_t key;
> +	key_perm_t perm; the following structure
> +	int rc = 0;

	int rc;

> +
> +	perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW;
> +
> +	key = key_create_or_update(make_key_ref(secondary_trusted_keys, 1),
> +				   "asymmetric",
> +				   NULL, data, len, perm,
> +				   KEY_ALLOC_NOT_IN_QUOTA);
> +	if (IS_ERR(key)) {
> +		rc = PTR_ERR(key);

This helper variable is not very useful.

> +		pr_err("Problem loading X.509 certificate %d\n", rc);

Why pr_err()? What kind of object is "a problem"?

Also X.509 certificates are everywhere. If these are printed to the
klog, how can e.g. an admin deduce the problem over here?

Even without having these log messages at all I could trace the called
function and be informed that some X.509 cert has an issues. Actually
then I could even deduce the location, thanks to call backtrace.

These have a potential to lead into wrong conclusions.

> +	} else {
> +		pr_notice("Loaded X.509 cert '%s'\n",
> +			  key_ref_to_ptr(key)->description);

single line

> +		key_ref_put(key);
> +	}

I'd suggest instead the following structure:

	if (IS_ERR(key)) {
		pr_err("Problem loading X.509 certificate %d\n", PTR_ERR(key));
		return;
	}

	pr_notice("Loaded X.509 cert '%s'\n", key_ref_to_ptr(key)->description);
	key_ref_put(key);
}

BR, Jarkko