| Message ID | 20211009130828.101396-2-tianjia.zhang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tpm: use SM3 instead of SM3_256 | expand |
On Sat, 2021-10-09 at 21:08 +0800, Tianjia Zhang wrote: [...] > diff --git a/include/uapi/linux/hash_info.h > b/include/uapi/linux/hash_info.h > index 74a8609fcb4d..1355525dd4aa 100644 > --- a/include/uapi/linux/hash_info.h > +++ b/include/uapi/linux/hash_info.h > @@ -32,7 +32,7 @@ enum hash_algo { > HASH_ALGO_TGR_128, > HASH_ALGO_TGR_160, > HASH_ALGO_TGR_192, > - HASH_ALGO_SM3_256, > + HASH_ALGO_SM3, > HASH_ALGO_STREEBOG_256, > HASH_ALGO_STREEBOG_512, > HASH_ALGO__LAST This is another one you can't do: all headers in UAPI are exports to userspace and the definitions constitute an ABI. If you simply do a rename, every userspace program that uses the current definition will immediately break on compile. You could add HASH_ALGO_SM3, but you can't remove HASH_ALGO_SM3_256 James
On Mon, 2021-10-18 at 09:05 -0400, James Bottomley wrote: > On Sat, 2021-10-09 at 21:08 +0800, Tianjia Zhang wrote: > [...] > > diff --git a/include/uapi/linux/hash_info.h > > b/include/uapi/linux/hash_info.h > > index 74a8609fcb4d..1355525dd4aa 100644 > > --- a/include/uapi/linux/hash_info.h > > +++ b/include/uapi/linux/hash_info.h > > @@ -32,7 +32,7 @@ enum hash_algo { > > HASH_ALGO_TGR_128, > > HASH_ALGO_TGR_160, > > HASH_ALGO_TGR_192, > > - HASH_ALGO_SM3_256, > > + HASH_ALGO_SM3, > > HASH_ALGO_STREEBOG_256, > > HASH_ALGO_STREEBOG_512, > > HASH_ALGO__LAST > > This is another one you can't do: all headers in UAPI are exports to > userspace and the definitions constitute an ABI. If you simply do a > rename, every userspace program that uses the current definition will > immediately break on compile. You could add HASH_ALGO_SM3, but you > can't remove HASH_ALGO_SM3_256 > > James So: shouldn't then also the old symbol continue to work also semantically? /Jarkko
On Mon, 2021-10-18 at 16:27 +0300, Jarkko Sakkinen wrote: > On Mon, 2021-10-18 at 09:05 -0400, James Bottomley wrote: > > On Sat, 2021-10-09 at 21:08 +0800, Tianjia Zhang wrote: > > [...] > > > diff --git a/include/uapi/linux/hash_info.h > > > b/include/uapi/linux/hash_info.h > > > index 74a8609fcb4d..1355525dd4aa 100644 > > > --- a/include/uapi/linux/hash_info.h > > > +++ b/include/uapi/linux/hash_info.h > > > @@ -32,7 +32,7 @@ enum hash_algo { > > > HASH_ALGO_TGR_128, > > > HASH_ALGO_TGR_160, > > > HASH_ALGO_TGR_192, > > > - HASH_ALGO_SM3_256, > > > + HASH_ALGO_SM3, > > > HASH_ALGO_STREEBOG_256, > > > HASH_ALGO_STREEBOG_512, > > > HASH_ALGO__LAST > > > > This is another one you can't do: all headers in UAPI are exports > > to userspace and the definitions constitute an ABI. If you simply > > do a rename, every userspace program that uses the current > > definition will immediately break on compile. You could add > > HASH_ALGO_SM3, but you can't remove HASH_ALGO_SM3_256 > > > > James > > So: shouldn't then also the old symbol continue to work also > semantically? Yes, that's the point: you can add a new definition ... in this case an alias for the old one, but you can't remove a definition that's been previously exported. James
On Mon, 2021-10-18 at 09:32 -0400, James Bottomley wrote: > On Mon, 2021-10-18 at 16:27 +0300, Jarkko Sakkinen wrote: > > On Mon, 2021-10-18 at 09:05 -0400, James Bottomley wrote: > > > On Sat, 2021-10-09 at 21:08 +0800, Tianjia Zhang wrote: > > > [...] > > > > diff --git a/include/uapi/linux/hash_info.h > > > > b/include/uapi/linux/hash_info.h > > > > index 74a8609fcb4d..1355525dd4aa 100644 > > > > --- a/include/uapi/linux/hash_info.h > > > > +++ b/include/uapi/linux/hash_info.h > > > > @@ -32,7 +32,7 @@ enum hash_algo { > > > > HASH_ALGO_TGR_128, > > > > HASH_ALGO_TGR_160, > > > > HASH_ALGO_TGR_192, > > > > - HASH_ALGO_SM3_256, > > > > + HASH_ALGO_SM3, > > > > HASH_ALGO_STREEBOG_256, > > > > HASH_ALGO_STREEBOG_512, > > > > HASH_ALGO__LAST > > > > > > This is another one you can't do: all headers in UAPI are exports > > > to userspace and the definitions constitute an ABI. If you simply > > > do a rename, every userspace program that uses the current > > > definition will immediately break on compile. You could add > > > HASH_ALGO_SM3, but you can't remove HASH_ALGO_SM3_256 > > > > > > James > > > > So: shouldn't then also the old symbol continue to work also > > semantically? > > Yes, that's the point: you can add a new definition ... in this case an > alias for the old one, but you can't remove a definition that's been > previously exported. Thanks, this of course obvious :-) I forgot temporarily that crypto has uapi interface. Tianjia, this patch set break production systems, so no chance we would ever merge it in this form. Why not just do this: ... HASH_ALGO_SM3_256, HASH_ALOG_SM3 = HASH_ALOG_SM_256, ... There is not good reason to mod the implementation because both symbols are kept. /Jarkko
Hi James, On 10/18/21 9:05 PM, James Bottomley wrote: > On Sat, 2021-10-09 at 21:08 +0800, Tianjia Zhang wrote: > [...] >> diff --git a/include/uapi/linux/hash_info.h >> b/include/uapi/linux/hash_info.h >> index 74a8609fcb4d..1355525dd4aa 100644 >> --- a/include/uapi/linux/hash_info.h >> +++ b/include/uapi/linux/hash_info.h >> @@ -32,7 +32,7 @@ enum hash_algo { >> HASH_ALGO_TGR_128, >> HASH_ALGO_TGR_160, >> HASH_ALGO_TGR_192, >> - HASH_ALGO_SM3_256, >> + HASH_ALGO_SM3, >> HASH_ALGO_STREEBOG_256, >> HASH_ALGO_STREEBOG_512, >> HASH_ALGO__LAST > > This is another one you can't do: all headers in UAPI are exports to > userspace and the definitions constitute an ABI. If you simply do a > rename, every userspace program that uses the current definition will > immediately break on compile. You could add HASH_ALGO_SM3, but you > can't remove HASH_ALGO_SM3_256 > > James > Correct, Thanks for pointing it out, redefining a macro is indeed a more appropriate method. Best regards, Tianjia
Hi Jarkko, On 10/18/21 9:41 PM, Jarkko Sakkinen wrote: > On Mon, 2021-10-18 at 09:32 -0400, James Bottomley wrote: >> On Mon, 2021-10-18 at 16:27 +0300, Jarkko Sakkinen wrote: >>> On Mon, 2021-10-18 at 09:05 -0400, James Bottomley wrote: >>>> On Sat, 2021-10-09 at 21:08 +0800, Tianjia Zhang wrote: >>>> [...] >>>>> diff --git a/include/uapi/linux/hash_info.h >>>>> b/include/uapi/linux/hash_info.h >>>>> index 74a8609fcb4d..1355525dd4aa 100644 >>>>> --- a/include/uapi/linux/hash_info.h >>>>> +++ b/include/uapi/linux/hash_info.h >>>>> @@ -32,7 +32,7 @@ enum hash_algo { >>>>> HASH_ALGO_TGR_128, >>>>> HASH_ALGO_TGR_160, >>>>> HASH_ALGO_TGR_192, >>>>> - HASH_ALGO_SM3_256, >>>>> + HASH_ALGO_SM3, >>>>> HASH_ALGO_STREEBOG_256, >>>>> HASH_ALGO_STREEBOG_512, >>>>> HASH_ALGO__LAST >>>> >>>> This is another one you can't do: all headers in UAPI are exports >>>> to userspace and the definitions constitute an ABI. If you simply >>>> do a rename, every userspace program that uses the current >>>> definition will immediately break on compile. You could add >>>> HASH_ALGO_SM3, but you can't remove HASH_ALGO_SM3_256 >>>> >>>> James >>> >>> So: shouldn't then also the old symbol continue to work also >>> semantically? >> >> Yes, that's the point: you can add a new definition ... in this case an >> alias for the old one, but you can't remove a definition that's been >> previously exported. > > Thanks, this of course obvious :-) I forgot temporarily that crypto > has uapi interface. Tianjia, this patch set break production systems, > so no chance we would ever merge it in this form. > > Why not just do this: > > ... > HASH_ALGO_SM3_256, > HASH_ALOG_SM3 = HASH_ALOG_SM_256, > ... > > There is not good reason to mod the implementation because both symbols > are kept. > > /Jarkko > Very good suggestion, I will do this in the next version patch. Maybe this is more appropriate: HASH_ALGO_SM3, HASH_ALGO_SM3_256 = HASH_ALGO_SM3, Best regards, Tianjia
diff --git a/Documentation/security/keys/trusted-encrypted.rst b/Documentation/security/keys/trusted-encrypted.rst index 80d5a5af62a1..3292461517f6 100644 --- a/Documentation/security/keys/trusted-encrypted.rst +++ b/Documentation/security/keys/trusted-encrypted.rst @@ -162,7 +162,7 @@ Usage:: default 1 (resealing allowed) hash= hash algorithm name as a string. For TPM 1.x the only allowed value is sha1. For TPM 2.x the allowed values - are sha1, sha256, sha384, sha512 and sm3-256. + are sha1, sha256, sha384, sha512 and sm3. policydigest= digest for the authorization policy. must be calculated with the same hash algorithm as specified by the 'hash=' option. diff --git a/crypto/hash_info.c b/crypto/hash_info.c index a49ff96bde77..fe0119407219 100644 --- a/crypto/hash_info.c +++ b/crypto/hash_info.c @@ -26,7 +26,7 @@ const char *const hash_algo_name[HASH_ALGO__LAST] = { [HASH_ALGO_TGR_128] = "tgr128", [HASH_ALGO_TGR_160] = "tgr160", [HASH_ALGO_TGR_192] = "tgr192", - [HASH_ALGO_SM3_256] = "sm3", + [HASH_ALGO_SM3] = "sm3", [HASH_ALGO_STREEBOG_256] = "streebog256", [HASH_ALGO_STREEBOG_512] = "streebog512", }; @@ -50,7 +50,7 @@ const int hash_digest_size[HASH_ALGO__LAST] = { [HASH_ALGO_TGR_128] = TGR128_DIGEST_SIZE, [HASH_ALGO_TGR_160] = TGR160_DIGEST_SIZE, [HASH_ALGO_TGR_192] = TGR192_DIGEST_SIZE, - [HASH_ALGO_SM3_256] = SM3256_DIGEST_SIZE, + [HASH_ALGO_SM3] = SM3_DIGEST_SIZE, [HASH_ALGO_STREEBOG_256] = STREEBOG256_DIGEST_SIZE, [HASH_ALGO_STREEBOG_512] = STREEBOG512_DIGEST_SIZE, }; diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index a25815a6f625..20f55de9d87b 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -19,7 +19,7 @@ static struct tpm2_hash tpm2_hash_map[] = { {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}, + {HASH_ALGO_SM3, TPM_ALG_SM3_256}, }; int tpm2_get_timeouts(struct tpm_chip *chip) diff --git a/include/crypto/hash_info.h b/include/crypto/hash_info.h index dd4f06785049..c1e6b2884732 100644 --- a/include/crypto/hash_info.h +++ b/include/crypto/hash_info.h @@ -32,7 +32,7 @@ #define TGR192_DIGEST_SIZE 24 /* not defined in include/crypto/ */ -#define SM3256_DIGEST_SIZE 32 +#define SM3_DIGEST_SIZE 32 extern const char *const hash_algo_name[HASH_ALGO__LAST]; extern const int hash_digest_size[HASH_ALGO__LAST]; diff --git a/include/uapi/linux/hash_info.h b/include/uapi/linux/hash_info.h index 74a8609fcb4d..1355525dd4aa 100644 --- a/include/uapi/linux/hash_info.h +++ b/include/uapi/linux/hash_info.h @@ -32,7 +32,7 @@ enum hash_algo { HASH_ALGO_TGR_128, HASH_ALGO_TGR_160, HASH_ALGO_TGR_192, - HASH_ALGO_SM3_256, + HASH_ALGO_SM3, HASH_ALGO_STREEBOG_256, HASH_ALGO_STREEBOG_512, HASH_ALGO__LAST diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c index 0165da386289..52a696035176 100644 --- a/security/keys/trusted-keys/trusted_tpm2.c +++ b/security/keys/trusted-keys/trusted_tpm2.c @@ -23,7 +23,7 @@ static struct tpm2_hash tpm2_hash_map[] = { {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}, + {HASH_ALGO_SM3, TPM_ALG_SM3_256}, }; static u32 tpm2key_oid[] = { 2, 23, 133, 10, 1, 5 };
According to https://tools.ietf.org/id/draft-oscca-cfrg-sm3-01.html, SM3 always produces a 256-bit hash value and there are no plans for other length development, so there is no ambiguity in the name of sm3. Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> --- Documentation/security/keys/trusted-encrypted.rst | 2 +- crypto/hash_info.c | 4 ++-- drivers/char/tpm/tpm2-cmd.c | 2 +- include/crypto/hash_info.h | 2 +- include/uapi/linux/hash_info.h | 2 +- security/keys/trusted-keys/trusted_tpm2.c | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-)