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

[Ben Boeckel <me@benboeckel.net>]

On Fri, May 24, 2024 at 09:04:54 -0400, James Bottomley wrote:
> 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

Should "alg id" be "algorithm id" everwhere in the docs?

> + *
> + * @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)

How about naming this `crypto_id`?

> +{
> +	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.

Copy pasta? Should be "crypto hash" I think.

> + */
> +static inline int tpm2_alg_to_crypto(int hash)

Maybe name this `alg_id`?

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

There seems to be some level of confusion with variable naming here. Are
these "hashes" or "ids" being passed around? The structure calls them
ids, the docs call them both, the variables call one a hash and the
other unadorned.

--Ben

> +
>  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)
> -- 
> 2.35.3
> 
>