On Fri May 24, 2024 at 4:40 PM EEST, Jarkko Sakkinen wrote:
> 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@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> > 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.
Not conflicting in the sense that this would somehow fight against that
code. It is anyway for different subsystem. I think this is probably
legit work but does bunch of conflicting changes so not caring about
this before asymmetric keys have been landed.
I rarely implement anything and it is feature that I checked from
various parties that it is still relevant so it is best to land it
first. I did all the trouble and time and effort reviewing and
even rewriting some of the buffer code in HMAC to better form
exactly because I saw it more priority than landing asymmetric
keys.
So it is not like I prioritize my patch sets over others but
these mess in same areas so thus the decision.
BR, Jarkko
