Re: [PATCH 1/6] tpm: consolidate TPM to crypto hash algorithm conversion

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

On Fri May 24, 2024 at 4:04 PM EEST, James Bottomley wrote:
> linux crypto and the TPM use different numeric algorithm identifiers
> for hash (and other) functions.  The conversion array for this already
> exists in two separate places.  The new policy sessions code would
> have to add a third copy, so instead of increasing the duplication,
> move the definition to a single consolidated place in tpm.h so the
> policy code can use it as is.
>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  drivers/char/tpm/tpm2-cmd.c               |  8 ----
>  include/linux/tpm.h                       | 52 ++++++++++++++++++++++-
>  security/keys/trusted-keys/trusted_tpm2.c | 20 +--------
>  3 files changed, 53 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 0cdf892ec2a7..f4428e715dd8 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -14,14 +14,6 @@
>  #include "tpm.h"
>  #include <crypto/hash_info.h>
>  
> -static struct tpm2_hash tpm2_hash_map[] = {
> -	{HASH_ALGO_SHA1, TPM_ALG_SHA1},
> -	{HASH_ALGO_SHA256, TPM_ALG_SHA256},
> -	{HASH_ALGO_SHA384, TPM_ALG_SHA384},
> -	{HASH_ALGO_SHA512, TPM_ALG_SHA512},
> -	{HASH_ALGO_SM3_256, TPM_ALG_SM3_256},
> -};
> -
>  int tpm2_get_timeouts(struct tpm_chip *chip)
>  {
>  	/* Fixed timeouts for TPM2 */
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index c17e4efbb2e5..07f532456a0c 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -418,11 +418,61 @@ enum tpm2_session_attributes {
>  	TPM2_SA_AUDIT			= BIT(7),
>  };
>  
> -struct tpm2_hash {
> +static const struct {
>  	unsigned int crypto_id;
>  	unsigned int tpm_id;
> +} tpm2_hash_map[] = {
> +	{HASH_ALGO_SHA1, TPM_ALG_SHA1},
> +	{HASH_ALGO_SHA256, TPM_ALG_SHA256},
> +	{HASH_ALGO_SHA384, TPM_ALG_SHA384},
> +	{HASH_ALGO_SHA512, TPM_ALG_SHA512},
> +	{HASH_ALGO_SM3_256, TPM_ALG_SM3_256},
>  };
>  
> +/**
> + * tpm2_crypto_to_alg() - convert a crypto hash to a TPM alg id
> + *
> + * @hash: the crypto subsystem view of the hash
> + *
> + * Return: TPM algorithm id or -1 if no mapping was found.
> + */
> +static inline int tpm2_crypto_to_alg(int hash)
> +{
> +	int i;
> +	int tpm_alg = -1;
> +
> +	for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
> +		if (hash == tpm2_hash_map[i].crypto_id) {
> +			tpm_alg = tpm2_hash_map[i].tpm_id;
> +			break;
> +		}
> +	}
> +
> +	return tpm_alg;
> +}
> +
> +/**
> + * tpm2_alg_to_crypto() - convert a TPM alg id to a crypto hash
> + *
> + * @hash: the TPM alg id view of the hash
> + *
> + * Return: TPM algorithm id or -1 if no mapping was found.
> + */
> +static inline int tpm2_alg_to_crypto(int hash)
> +{
> +	int i;
> +	int crypto_hash = -1;
> +
> +	for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
> +		if (hash == tpm2_hash_map[i].tpm_id) {
> +			crypto_hash = tpm2_hash_map[i].crypto_id;
> +			break;
> +		}
> +	}
> +
> +	return crypto_hash;
> +}
> +
>  int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal);
>  void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal);
>  int tpm_buf_init_sized(struct tpm_buf *buf);
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index dfeec06301ce..94ff9ccae66e 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -18,14 +18,6 @@
>  
>  #include "tpm2key.asn1.h"
>  
> -static struct tpm2_hash tpm2_hash_map[] = {
> -	{HASH_ALGO_SHA1, TPM_ALG_SHA1},
> -	{HASH_ALGO_SHA256, TPM_ALG_SHA256},
> -	{HASH_ALGO_SHA384, TPM_ALG_SHA384},
> -	{HASH_ALGO_SHA512, TPM_ALG_SHA512},
> -	{HASH_ALGO_SM3_256, TPM_ALG_SM3_256},
> -};
> -
>  static u32 tpm2key_oid[] = { 2, 23, 133, 10, 1, 5 };
>  
>  static int tpm2_key_encode(struct trusted_key_payload *payload,
> @@ -231,19 +223,11 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
>  	off_t offset = TPM_HEADER_SIZE;
>  	struct tpm_buf buf, sized;
>  	int blob_len = 0;
> -	u32 hash;
> +	int hash = tpm2_crypto_to_alg(options->hash);
>  	u32 flags;
> -	int i;
>  	int rc;
>  
> -	for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
> -		if (options->hash == tpm2_hash_map[i].crypto_id) {
> -			hash = tpm2_hash_map[i].tpm_id;
> -			break;
> -		}
> -	}
> -
> -	if (i == ARRAY_SIZE(tpm2_hash_map))
> +	if (hash < 0)
>  		return -EINVAL;
>  
>  	if (!options->keyhandle)

I want a patch set that renders out the WARN's before any other
modification to this code. I've spent fixing one myself plus
fixing totally trivial memory leak. That happens everyone but
still focus in now all wrong. I.e. adding new stuff without
polishing old first and let others take care cleaning up the
mess...

Also, HMAC still needs attention.

And this patch set is totally conflicting getting asymmetric
keys landed, which came first and if already maturing quite
well.

No issues reviewing after so this is not like rejecting the
idea but doing right things right and in right order.

BR, Jarkko