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

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

Now I bandwidth to give this the first round.

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

Please use exact names i.e. conversion is between enum tpm2_algorithms
and enum hash_info. Much easier to lookup later on.

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

"the new policy session code" is not an artifact.

Instead, please say what needs the third instance and why. I don't
consume white paper text.

I'm fine if you just use merging two redundant copies together, with
no reference to the third copy. In this form this unconditional NAK
given that I have zero idea what the third copy is and where it is
located.

>
> 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);

Please use reverse christmas tree order.

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


BR, Jarkko