| Message ID | 20210615144115.2113484-4-stefanb@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tcgbios: Use the proper hashes for the TPM 2 PCR banks | expand |
On 16/06/2021 00:41, Stefan Berger wrote: > Instead of just using sha256 for all PCR banks (and truncating > the value or zero-padding it) use the proper hash function for > each one of the banks. For unimplemented hashes, fill the buffer > with 0xff. Was it a bug that sha256 was always used? > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > --- > lib/libtpm/tcgbios.c | 42 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 32 insertions(+), 10 deletions(-) > > diff --git a/lib/libtpm/tcgbios.c b/lib/libtpm/tcgbios.c > index 91f2f46..af96c52 100644 > --- a/lib/libtpm/tcgbios.c > +++ b/lib/libtpm/tcgbios.c > @@ -127,28 +127,32 @@ static const struct hash_parameters { > uint8_t hashalg_flag; > uint8_t hash_buffersize; > const char *name; > + void (*hashfunc)(const uint8_t *data, uint32_t length, uint8_t *hash); > } hash_parameters[] = { > { > .hashalg = TPM2_ALG_SHA1, > .hashalg_flag = TPM2_ALG_SHA1_FLAG, > .hash_buffersize = SHA1_BUFSIZE, > .name = "SHA1", > + .hashfunc = sha1, > }, { > .hashalg = TPM2_ALG_SHA256, > .hashalg_flag = TPM2_ALG_SHA256_FLAG, > .hash_buffersize = SHA256_BUFSIZE, > .name = "SHA256", > + .hashfunc = sha256, > }, { > .hashalg = TPM2_ALG_SHA384, > .hashalg_flag = TPM2_ALG_SHA384_FLAG, > .hash_buffersize = SHA384_BUFSIZE, > .name = "SHA384", > - > + .hashfunc = sha384, > }, { > .hashalg = TPM2_ALG_SHA512, > .hashalg_flag = TPM2_ALG_SHA512_FLAG, > .hash_buffersize = SHA512_BUFSIZE, > .name = "SHA512", > + .hashfunc = sha512, > }, { > .hashalg = TPM2_ALG_SM3_256, > .hashalg_flag = TPM2_ALG_SM3_256_FLAG, Do not you want sha256/... for this and other guys after this one? > @@ -233,6 +237,25 @@ static const char * tpm20_hashalg_flag_to_name(uint8_t hashalg_flag) > return NULL; > } > > +static void tpm2_hash_data(uint16_t hashAlg, > + const uint8_t *data, uint32_t data_len, > + uint8_t *hash) > +{ > + unsigned i; > + > + for (i = 0; i < ARRAY_SIZE(hash_parameters); i++) { > + if (hash_parameters[i].hashalg == hashAlg) { > + if (hash_parameters[i].hashfunc) { > + hash_parameters[i].hashfunc(data, data_len, > + hash); > + } else { > + memset(hash, 0xff, > + hash_parameters[i].hash_buffersize); > + } > + } > + } > +} > + > /* > * Build the TPM2 TPML_DIGEST_VALUES data structure from the given hash. > * Follow the PCR bank configuration of the TPM and write the same hash > @@ -242,13 +265,15 @@ static const char * tpm20_hashalg_flag_to_name(uint8_t hashalg_flag) > * hash when writing it in the area of the sha1 hash. > * > * le: the log entry to build the digest in > - * sha1: the sha1 hash value to use > + * hashdata: the data to hash > + * hashdata_len: the length of the hashdata > * bigEndian: whether to build in big endian format for the TPM or log > * little endian for the log (TPM 2.0) > * > * Returns the digest size; -1 on fatal error > */ > -static int tpm20_build_digest(struct tpm_log_entry *le, const uint8_t *sha256, > +static int tpm20_build_digest(struct tpm_log_entry *le, > + const uint8_t *hashdata, uint32_t hashdata_len, > bool bigEndian) > { > struct tpms_pcr_selection *sel; > @@ -297,9 +322,8 @@ static int tpm20_build_digest(struct tpm_log_entry *le, const uint8_t *sha256, > else > v->hashAlg = cpu_to_le16(be16_to_cpu(sel->hashAlg)); > > - memset(v->hash, 0, hsize); > - memcpy(v->hash, sha256, > - hsize < SHA256_BUFSIZE ? hsize : SHA256_BUFSIZE); > + tpm2_hash_data(be16_to_cpu(sel->hashAlg), hashdata, hashdata_len, > + v->hash); > > dest += sizeof(*v) + hsize; > sel = nsel; > @@ -865,7 +889,6 @@ static uint32_t tpm_add_measurement_to_log(uint32_t pcrindex, > const uint8_t *hashdata, > uint32_t hashdatalen) > { > - uint8_t hash[SHA256_BUFSIZE]; > struct tpm_log_entry le = { > .hdr.pcrindex = cpu_to_log32(pcrindex), > .hdr.eventtype = cpu_to_log32(eventtype), > @@ -873,8 +896,7 @@ static uint32_t tpm_add_measurement_to_log(uint32_t pcrindex, > int digest_len; > int ret; > > - sha256(hashdata, hashdatalen, hash); > - digest_len = tpm20_build_digest(&le, hash, true); > + digest_len = tpm20_build_digest(&le, hashdata, hashdatalen, true); > if (digest_len < 0) > return TCGBIOS_GENERAL_ERROR; > ret = tpm20_extend(&le, digest_len); > @@ -882,7 +904,7 @@ static uint32_t tpm_add_measurement_to_log(uint32_t pcrindex, > tpm_set_failure(); > return TCGBIOS_COMMAND_ERROR; > } > - tpm20_build_digest(&le, hash, false); > + tpm20_build_digest(&le, hashdata, hashdatalen, false); > return tpm_log_event_long(&le.hdr, digest_len, info, infolen); > } > >
On 6/29/21 10:56 PM, Alexey Kardashevskiy wrote: > > > On 16/06/2021 00:41, Stefan Berger wrote: >> Instead of just using sha256 for all PCR banks (and truncating >> the value or zero-padding it) use the proper hash function for >> each one of the banks. For unimplemented hashes, fill the buffer >> with 0xff. > > Was it a bug that sha256 was always used? > Yes, it was a bug. > >> >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >> --- >> lib/libtpm/tcgbios.c | 42 ++++++++++++++++++++++++++++++++---------- >> 1 file changed, 32 insertions(+), 10 deletions(-) >> >> diff --git a/lib/libtpm/tcgbios.c b/lib/libtpm/tcgbios.c >> index 91f2f46..af96c52 100644 >> --- a/lib/libtpm/tcgbios.c >> +++ b/lib/libtpm/tcgbios.c >> @@ -127,28 +127,32 @@ static const struct hash_parameters { >> uint8_t hashalg_flag; >> uint8_t hash_buffersize; >> const char *name; >> + void (*hashfunc)(const uint8_t *data, uint32_t length, uint8_t >> *hash); >> } hash_parameters[] = { >> { >> .hashalg = TPM2_ALG_SHA1, >> .hashalg_flag = TPM2_ALG_SHA1_FLAG, >> .hash_buffersize = SHA1_BUFSIZE, >> .name = "SHA1", >> + .hashfunc = sha1, >> }, { >> .hashalg = TPM2_ALG_SHA256, >> .hashalg_flag = TPM2_ALG_SHA256_FLAG, >> .hash_buffersize = SHA256_BUFSIZE, >> .name = "SHA256", >> + .hashfunc = sha256, >> }, { >> .hashalg = TPM2_ALG_SHA384, >> .hashalg_flag = TPM2_ALG_SHA384_FLAG, >> .hash_buffersize = SHA384_BUFSIZE, >> .name = "SHA384", >> - >> + .hashfunc = sha384, >> }, { >> .hashalg = TPM2_ALG_SHA512, >> .hashalg_flag = TPM2_ALG_SHA512_FLAG, >> .hash_buffersize = SHA512_BUFSIZE, >> .name = "SHA512", >> + .hashfunc = sha512, >> }, { >> .hashalg = TPM2_ALG_SM3_256, >> .hashalg_flag = TPM2_ALG_SM3_256_FLAG, > > > > Do not you want sha256/... for this and other guys after this one? These ones will return an array of 0xff for now. They will need their own hash implementations in the future but for now only SHA1,SHA256,SHA384 andSHA512 are enabled in the vTPM. Stefan > > > >> @@ -233,6 +237,25 @@ static const char * >> tpm20_hashalg_flag_to_name(uint8_t hashalg_flag) >> return NULL; >> } >> +static void tpm2_hash_data(uint16_t hashAlg, >> + const uint8_t *data, uint32_t data_len, >> + uint8_t *hash) >> +{ >> + unsigned i; >> + >> + for (i = 0; i < ARRAY_SIZE(hash_parameters); i++) { >> + if (hash_parameters[i].hashalg == hashAlg) { >> + if (hash_parameters[i].hashfunc) { >> + hash_parameters[i].hashfunc(data, data_len, >> + hash); >> + } else { >> + memset(hash, 0xff, >> + hash_parameters[i].hash_buffersize); >> + } >> + } >> + } >> +} >> + >> /* >> * Build the TPM2 TPML_DIGEST_VALUES data structure from the given >> hash. >> * Follow the PCR bank configuration of the TPM and write the same >> hash >> @@ -242,13 +265,15 @@ static const char * >> tpm20_hashalg_flag_to_name(uint8_t hashalg_flag) >> * hash when writing it in the area of the sha1 hash. >> * >> * le: the log entry to build the digest in >> - * sha1: the sha1 hash value to use >> + * hashdata: the data to hash >> + * hashdata_len: the length of the hashdata >> * bigEndian: whether to build in big endian format for the TPM or log >> * little endian for the log (TPM 2.0) >> * >> * Returns the digest size; -1 on fatal error >> */ >> -static int tpm20_build_digest(struct tpm_log_entry *le, const >> uint8_t *sha256, >> +static int tpm20_build_digest(struct tpm_log_entry *le, >> + const uint8_t *hashdata, uint32_t >> hashdata_len, >> bool bigEndian) >> { >> struct tpms_pcr_selection *sel; >> @@ -297,9 +322,8 @@ static int tpm20_build_digest(struct >> tpm_log_entry *le, const uint8_t *sha256, >> else >> v->hashAlg = cpu_to_le16(be16_to_cpu(sel->hashAlg)); >> - memset(v->hash, 0, hsize); >> - memcpy(v->hash, sha256, >> - hsize < SHA256_BUFSIZE ? hsize : SHA256_BUFSIZE); >> + tpm2_hash_data(be16_to_cpu(sel->hashAlg), hashdata, >> hashdata_len, >> + v->hash); >> dest += sizeof(*v) + hsize; >> sel = nsel; >> @@ -865,7 +889,6 @@ static uint32_t >> tpm_add_measurement_to_log(uint32_t pcrindex, >> const uint8_t *hashdata, >> uint32_t hashdatalen) >> { >> - uint8_t hash[SHA256_BUFSIZE]; >> struct tpm_log_entry le = { >> .hdr.pcrindex = cpu_to_log32(pcrindex), >> .hdr.eventtype = cpu_to_log32(eventtype), >> @@ -873,8 +896,7 @@ static uint32_t >> tpm_add_measurement_to_log(uint32_t pcrindex, >> int digest_len; >> int ret; >> - sha256(hashdata, hashdatalen, hash); >> - digest_len = tpm20_build_digest(&le, hash, true); >> + digest_len = tpm20_build_digest(&le, hashdata, hashdatalen, true); >> if (digest_len < 0) >> return TCGBIOS_GENERAL_ERROR; >> ret = tpm20_extend(&le, digest_len); >> @@ -882,7 +904,7 @@ static uint32_t >> tpm_add_measurement_to_log(uint32_t pcrindex, >> tpm_set_failure(); >> return TCGBIOS_COMMAND_ERROR; >> } >> - tpm20_build_digest(&le, hash, false); >> + tpm20_build_digest(&le, hashdata, hashdatalen, false); >> return tpm_log_event_long(&le.hdr, digest_len, info, infolen); >> } >> >
On 30/06/2021 22:23, Stefan Berger wrote: > > On 6/29/21 10:56 PM, Alexey Kardashevskiy wrote: >> >> >> On 16/06/2021 00:41, Stefan Berger wrote: >>> Instead of just using sha256 for all PCR banks (and truncating >>> the value or zero-padding it) use the proper hash function for >>> each one of the banks. For unimplemented hashes, fill the buffer >>> with 0xff. >> >> Was it a bug that sha256 was always used? >> > Yes, it was a bug. Put it in the commit log. > > >> >>> >>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >>> --- >>> lib/libtpm/tcgbios.c | 42 ++++++++++++++++++++++++++++++++---------- >>> 1 file changed, 32 insertions(+), 10 deletions(-) >>> >>> diff --git a/lib/libtpm/tcgbios.c b/lib/libtpm/tcgbios.c >>> index 91f2f46..af96c52 100644 >>> --- a/lib/libtpm/tcgbios.c >>> +++ b/lib/libtpm/tcgbios.c >>> @@ -127,28 +127,32 @@ static const struct hash_parameters { >>> uint8_t hashalg_flag; >>> uint8_t hash_buffersize; >>> const char *name; >>> + void (*hashfunc)(const uint8_t *data, uint32_t length, uint8_t >>> *hash); >>> } hash_parameters[] = { >>> { >>> .hashalg = TPM2_ALG_SHA1, >>> .hashalg_flag = TPM2_ALG_SHA1_FLAG, >>> .hash_buffersize = SHA1_BUFSIZE, >>> .name = "SHA1", >>> + .hashfunc = sha1, >>> }, { >>> .hashalg = TPM2_ALG_SHA256, >>> .hashalg_flag = TPM2_ALG_SHA256_FLAG, >>> .hash_buffersize = SHA256_BUFSIZE, >>> .name = "SHA256", >>> + .hashfunc = sha256, >>> }, { >>> .hashalg = TPM2_ALG_SHA384, >>> .hashalg_flag = TPM2_ALG_SHA384_FLAG, >>> .hash_buffersize = SHA384_BUFSIZE, >>> .name = "SHA384", >>> - >>> + .hashfunc = sha384, >>> }, { >>> .hashalg = TPM2_ALG_SHA512, >>> .hashalg_flag = TPM2_ALG_SHA512_FLAG, >>> .hash_buffersize = SHA512_BUFSIZE, >>> .name = "SHA512", >>> + .hashfunc = sha512, >>> }, { >>> .hashalg = TPM2_ALG_SM3_256, >>> .hashalg_flag = TPM2_ALG_SM3_256_FLAG, >> >> >> >> Do not you want sha256/... for this and other guys after this one? > > > These ones will return an array of 0xff for now. They will need their > own hash implementations in the future but for now only > SHA1,SHA256,SHA384 andSHA512 are enabled in the vTPM. Than having hash_parameters structs without defined hashfunc is a bug too and we should just remove those until the proper support is added. Thanks, > > Stefan > > > >> >> >> >>> @@ -233,6 +237,25 @@ static const char * >>> tpm20_hashalg_flag_to_name(uint8_t hashalg_flag) >>> return NULL; >>> } >>> +static void tpm2_hash_data(uint16_t hashAlg, >>> + const uint8_t *data, uint32_t data_len, >>> + uint8_t *hash) >>> +{ >>> + unsigned i; >>> + >>> + for (i = 0; i < ARRAY_SIZE(hash_parameters); i++) { >>> + if (hash_parameters[i].hashalg == hashAlg) { >>> + if (hash_parameters[i].hashfunc) { >>> + hash_parameters[i].hashfunc(data, data_len, >>> + hash); >>> + } else { >>> + memset(hash, 0xff, >>> + hash_parameters[i].hash_buffersize); >>> + } >>> + } >>> + } >>> +} >>> + >>> /* >>> * Build the TPM2 TPML_DIGEST_VALUES data structure from the given >>> hash. >>> * Follow the PCR bank configuration of the TPM and write the same >>> hash >>> @@ -242,13 +265,15 @@ static const char * >>> tpm20_hashalg_flag_to_name(uint8_t hashalg_flag) >>> * hash when writing it in the area of the sha1 hash. >>> * >>> * le: the log entry to build the digest in >>> - * sha1: the sha1 hash value to use >>> + * hashdata: the data to hash >>> + * hashdata_len: the length of the hashdata >>> * bigEndian: whether to build in big endian format for the TPM or log >>> * little endian for the log (TPM 2.0) >>> * >>> * Returns the digest size; -1 on fatal error >>> */ >>> -static int tpm20_build_digest(struct tpm_log_entry *le, const >>> uint8_t *sha256, >>> +static int tpm20_build_digest(struct tpm_log_entry *le, >>> + const uint8_t *hashdata, uint32_t >>> hashdata_len, >>> bool bigEndian) >>> { >>> struct tpms_pcr_selection *sel; >>> @@ -297,9 +322,8 @@ static int tpm20_build_digest(struct >>> tpm_log_entry *le, const uint8_t *sha256, >>> else >>> v->hashAlg = cpu_to_le16(be16_to_cpu(sel->hashAlg)); >>> - memset(v->hash, 0, hsize); >>> - memcpy(v->hash, sha256, >>> - hsize < SHA256_BUFSIZE ? hsize : SHA256_BUFSIZE); >>> + tpm2_hash_data(be16_to_cpu(sel->hashAlg), hashdata, >>> hashdata_len, >>> + v->hash); >>> dest += sizeof(*v) + hsize; >>> sel = nsel; >>> @@ -865,7 +889,6 @@ static uint32_t >>> tpm_add_measurement_to_log(uint32_t pcrindex, >>> const uint8_t *hashdata, >>> uint32_t hashdatalen) >>> { >>> - uint8_t hash[SHA256_BUFSIZE]; >>> struct tpm_log_entry le = { >>> .hdr.pcrindex = cpu_to_log32(pcrindex), >>> .hdr.eventtype = cpu_to_log32(eventtype), >>> @@ -873,8 +896,7 @@ static uint32_t >>> tpm_add_measurement_to_log(uint32_t pcrindex, >>> int digest_len; >>> int ret; >>> - sha256(hashdata, hashdatalen, hash); >>> - digest_len = tpm20_build_digest(&le, hash, true); >>> + digest_len = tpm20_build_digest(&le, hashdata, hashdatalen, true); >>> if (digest_len < 0) >>> return TCGBIOS_GENERAL_ERROR; >>> ret = tpm20_extend(&le, digest_len); >>> @@ -882,7 +904,7 @@ static uint32_t >>> tpm_add_measurement_to_log(uint32_t pcrindex, >>> tpm_set_failure(); >>> return TCGBIOS_COMMAND_ERROR; >>> } >>> - tpm20_build_digest(&le, hash, false); >>> + tpm20_build_digest(&le, hashdata, hashdatalen, false); >>> return tpm_log_event_long(&le.hdr, digest_len, info, infolen); >>> } >>> >>
diff --git a/lib/libtpm/tcgbios.c b/lib/libtpm/tcgbios.c index 91f2f46..af96c52 100644 --- a/lib/libtpm/tcgbios.c +++ b/lib/libtpm/tcgbios.c @@ -127,28 +127,32 @@ static const struct hash_parameters { uint8_t hashalg_flag; uint8_t hash_buffersize; const char *name; + void (*hashfunc)(const uint8_t *data, uint32_t length, uint8_t *hash); } hash_parameters[] = { { .hashalg = TPM2_ALG_SHA1, .hashalg_flag = TPM2_ALG_SHA1_FLAG, .hash_buffersize = SHA1_BUFSIZE, .name = "SHA1", + .hashfunc = sha1, }, { .hashalg = TPM2_ALG_SHA256, .hashalg_flag = TPM2_ALG_SHA256_FLAG, .hash_buffersize = SHA256_BUFSIZE, .name = "SHA256", + .hashfunc = sha256, }, { .hashalg = TPM2_ALG_SHA384, .hashalg_flag = TPM2_ALG_SHA384_FLAG, .hash_buffersize = SHA384_BUFSIZE, .name = "SHA384", - + .hashfunc = sha384, }, { .hashalg = TPM2_ALG_SHA512, .hashalg_flag = TPM2_ALG_SHA512_FLAG, .hash_buffersize = SHA512_BUFSIZE, .name = "SHA512", + .hashfunc = sha512, }, { .hashalg = TPM2_ALG_SM3_256, .hashalg_flag = TPM2_ALG_SM3_256_FLAG, @@ -233,6 +237,25 @@ static const char * tpm20_hashalg_flag_to_name(uint8_t hashalg_flag) return NULL; } +static void tpm2_hash_data(uint16_t hashAlg, + const uint8_t *data, uint32_t data_len, + uint8_t *hash) +{ + unsigned i; + + for (i = 0; i < ARRAY_SIZE(hash_parameters); i++) { + if (hash_parameters[i].hashalg == hashAlg) { + if (hash_parameters[i].hashfunc) { + hash_parameters[i].hashfunc(data, data_len, + hash); + } else { + memset(hash, 0xff, + hash_parameters[i].hash_buffersize); + } + } + } +} + /* * Build the TPM2 TPML_DIGEST_VALUES data structure from the given hash. * Follow the PCR bank configuration of the TPM and write the same hash @@ -242,13 +265,15 @@ static const char * tpm20_hashalg_flag_to_name(uint8_t hashalg_flag) * hash when writing it in the area of the sha1 hash. * * le: the log entry to build the digest in - * sha1: the sha1 hash value to use + * hashdata: the data to hash + * hashdata_len: the length of the hashdata * bigEndian: whether to build in big endian format for the TPM or log * little endian for the log (TPM 2.0) * * Returns the digest size; -1 on fatal error */ -static int tpm20_build_digest(struct tpm_log_entry *le, const uint8_t *sha256, +static int tpm20_build_digest(struct tpm_log_entry *le, + const uint8_t *hashdata, uint32_t hashdata_len, bool bigEndian) { struct tpms_pcr_selection *sel; @@ -297,9 +322,8 @@ static int tpm20_build_digest(struct tpm_log_entry *le, const uint8_t *sha256, else v->hashAlg = cpu_to_le16(be16_to_cpu(sel->hashAlg)); - memset(v->hash, 0, hsize); - memcpy(v->hash, sha256, - hsize < SHA256_BUFSIZE ? hsize : SHA256_BUFSIZE); + tpm2_hash_data(be16_to_cpu(sel->hashAlg), hashdata, hashdata_len, + v->hash); dest += sizeof(*v) + hsize; sel = nsel; @@ -865,7 +889,6 @@ static uint32_t tpm_add_measurement_to_log(uint32_t pcrindex, const uint8_t *hashdata, uint32_t hashdatalen) { - uint8_t hash[SHA256_BUFSIZE]; struct tpm_log_entry le = { .hdr.pcrindex = cpu_to_log32(pcrindex), .hdr.eventtype = cpu_to_log32(eventtype), @@ -873,8 +896,7 @@ static uint32_t tpm_add_measurement_to_log(uint32_t pcrindex, int digest_len; int ret; - sha256(hashdata, hashdatalen, hash); - digest_len = tpm20_build_digest(&le, hash, true); + digest_len = tpm20_build_digest(&le, hashdata, hashdatalen, true); if (digest_len < 0) return TCGBIOS_GENERAL_ERROR; ret = tpm20_extend(&le, digest_len); @@ -882,7 +904,7 @@ static uint32_t tpm_add_measurement_to_log(uint32_t pcrindex, tpm_set_failure(); return TCGBIOS_COMMAND_ERROR; } - tpm20_build_digest(&le, hash, false); + tpm20_build_digest(&le, hashdata, hashdatalen, false); return tpm_log_event_long(&le.hdr, digest_len, info, infolen); }
Instead of just using sha256 for all PCR banks (and truncating the value or zero-padding it) use the proper hash function for each one of the banks. For unimplemented hashes, fill the buffer with 0xff. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- lib/libtpm/tcgbios.c | 42 ++++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-)