| Message ID | 20181213102945.30946-6-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm: retrieve digest size of unknown algorithms from TPM | expand |
On Thu, Dec 13, 2018 at 11:29:45AM +0100, Roberto Sassu wrote: > Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest. > > This patch modifies the definition of tpm_pcr_extend() to allow other > kernel subsystems to pass a digest for each algorithm supported by the TPM. > All digests are processed by the TPM in one operation. > > If a tpm_pcr_extend() caller provides a subset of the supported algorithms, > the TPM driver extends the remaining PCR banks with the first digest > passed as an argument to the function. > > The new tpm_extend digest structure has been preferred to the tpm_digest > structure, to let the caller specify the size of the digest (which may be > unknown to the TPM driver). > > Due to the API change, ima_pcr_extend() and pcrlock() have been modified. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > drivers/char/tpm/tpm-interface.c | 24 +++++--------------- > drivers/char/tpm/tpm.h | 5 +++-- > drivers/char/tpm/tpm1-cmd.c | 13 ++++++++--- > drivers/char/tpm/tpm2-cmd.c | 35 +++++++++++++++++++++--------- > include/linux/tpm.h | 13 ++++++++--- > security/integrity/ima/ima_queue.c | 5 ++++- > security/keys/trusted.c | 5 ++++- > 7 files changed, 62 insertions(+), 38 deletions(-) > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > index eb7c79ca8a94..911fea19e408 100644 > --- a/drivers/char/tpm/tpm-interface.c > +++ b/drivers/char/tpm/tpm-interface.c > @@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read); > * tpm_pcr_extend - extend a PCR value in SHA1 bank. > * @chip: a &struct tpm_chip instance, %NULL for the default chip > * @pcr_idx: the PCR to be retrieved > - * @hash: the hash value used to extend the PCR value > + * @count: number of tpm_extend_digest structures > + * @digests: array of tpm_extend_digest structures used to extend PCRs > * > * Note: with TPM 2.0 extends also those banks for which no digest was > * specified in order to prevent malicious use of those PCR banks. > * > * Return: same as with tpm_transmit_cmd() > */ > -int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash) > +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > + const struct tpm_extend_digest *digests) Remove const. Document how @digests is used like the special meaning of the first index. I faintly remember asking this last time. > { > int rc; > - struct tpm_digest *digest_list; > - int i; > > chip = tpm_find_get_ops(chip); > if (!chip) > return -ENODEV; > > if (chip->flags & TPM_CHIP_FLAG_TPM2) { > - digest_list = kcalloc(chip->nr_allocated_banks, > - sizeof(*digest_list), GFP_KERNEL); > - if (!digest_list) > - return -ENOMEM; > - > - for (i = 0; i < chip->nr_allocated_banks; i++) { > - digest_list[i].alg_id = chip->allocated_banks[i].alg_id; > - memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE); > - } > - > - rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks, > - digest_list); > - kfree(digest_list); > + rc = tpm2_pcr_extend(chip, pcr_idx, count, digests); > tpm_put_ops(chip); > return rc; > } > > - rc = tpm1_pcr_extend(chip, pcr_idx, hash, > + rc = tpm1_pcr_extend(chip, pcr_idx, count, digests, > "attempting extend a PCR value"); The validation is missing that the provided array has only one element and the algorithm is SHA1. Could be done also inside tpm1_pcr_extend() but what you are doing to that function does not make any sense so better to do it here. > tpm_put_ops(chip); > return rc; > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 64d93d26087f..6b446504d2fe 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -504,7 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip); > int tpm1_do_selftest(struct tpm_chip *chip); > int tpm1_get_timeouts(struct tpm_chip *chip); > unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal); > -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash, > +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > + const struct tpm_extend_digest *digests, > const char *log_msg); > int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf); > ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap, > @@ -551,7 +552,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip); > int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, > struct tpm_digest *digest, u16 *digest_size_ptr); > int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > - struct tpm_digest *digests); > + const struct tpm_extend_digest *digests); > int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max); > void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle, > unsigned int flags); > diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c > index 8b70a7f884a7..04ee10284b8c 100644 > --- a/drivers/char/tpm/tpm1-cmd.c > +++ b/drivers/char/tpm/tpm1-cmd.c > @@ -449,12 +449,20 @@ int tpm1_get_timeouts(struct tpm_chip *chip) > } > > #define TPM_ORD_PCR_EXTEND 20 > -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash, > +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > + const struct tpm_extend_digest *digests, > const char *log_msg) > { > struct tpm_buf buf; > + u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 }; > + const u8 *hash; > int rc; > > + hash = dummy_hash; > + if (count) > + memcpy(dummy_hash, digests[0].data, > + min(digests[0].size, (u16)sizeof(dummy_hash))); > + You copy memory from one place to another without any good reason to do so. My suggestion is just not to change tpm1_pcr_extend() at all. > rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND); > if (rc) > return rc; > @@ -743,7 +751,6 @@ int tpm1_auto_startup(struct tpm_chip *chip) > */ > int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr) > { > - u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 }; > struct tpm_buf buf; > unsigned int try; > int rc; > @@ -751,7 +758,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr) > > /* for buggy tpm, flush pcrs with extend to selected dummy */ > if (tpm_suspend_pcr) > - rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash, > + rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL, > "extending dummy pcr before suspend"); > > rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE); > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index 6ce5173cf0e5..77b5808270c6 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -247,21 +247,22 @@ struct tpm2_null_auth_area { > * > * @chip: TPM chip to use. > * @pcr_idx: index of the PCR. > - * @count: number of digests passed. > - * @digests: list of pcr banks and corresponding digest values to extend. > + * @count: number of tpm_extend_digest passed. > + * @digests: array of tpm_extend_digest with digest values to extend. > * > * Return: Same as with tpm_transmit_cmd. > */ The documentation about @digests. > int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > - struct tpm_digest *digests) > + const struct tpm_extend_digest *digests) > { > struct tpm_buf buf; > struct tpm2_null_auth_area auth_area; > + const struct tpm_extend_digest *digest; > + u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 }; > + const u8 *hash; > int rc; > int i; > - > - if (count > chip->nr_allocated_banks) > - return -EINVAL; > + int j; > > rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND); > if (rc) > @@ -277,11 +278,25 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area)); > tpm_buf_append(&buf, (const unsigned char *)&auth_area, > sizeof(auth_area)); > - tpm_buf_append_u32(&buf, count); > + tpm_buf_append_u32(&buf, chip->nr_allocated_banks); > + > + if (count) > + memcpy(dummy_hash, digests[0].data, digests[0].size); > + > + for (i = 0; i < chip->nr_allocated_banks; i++) { > + tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id); > + > + hash = dummy_hash; > + for (j = 0; j < count; j++) { > + digest = digests + j; > + > + if (digest->alg_id == chip->allocated_banks[i].alg_id) { > + hash = digest->data; > + break; I think the whole design is just wrong. I did re-read your response to v6 again and I'm very sorry, but I just don't get this. Caller has all the information (from struct tpm_chip) to give the correct data. This function should validate that data (check algorithm ID and that's it). Extending with the dummy hash should be done by the caller when preparing the array, not baked into this function. This kind of also makes obvious that we don't need this new struct. There should never be a local variable (whose size is BTW randomly chosen) called dummy_hash. /Jarkko
On 12/20/2018 4:21 PM, Jarkko Sakkinen wrote: > On Thu, Dec 13, 2018 at 11:29:45AM +0100, Roberto Sassu wrote: >> Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest. >> >> This patch modifies the definition of tpm_pcr_extend() to allow other >> kernel subsystems to pass a digest for each algorithm supported by the TPM. >> All digests are processed by the TPM in one operation. >> >> If a tpm_pcr_extend() caller provides a subset of the supported algorithms, >> the TPM driver extends the remaining PCR banks with the first digest >> passed as an argument to the function. >> >> The new tpm_extend digest structure has been preferred to the tpm_digest >> structure, to let the caller specify the size of the digest (which may be >> unknown to the TPM driver). >> >> Due to the API change, ima_pcr_extend() and pcrlock() have been modified. >> >> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> >> --- >> drivers/char/tpm/tpm-interface.c | 24 +++++--------------- >> drivers/char/tpm/tpm.h | 5 +++-- >> drivers/char/tpm/tpm1-cmd.c | 13 ++++++++--- >> drivers/char/tpm/tpm2-cmd.c | 35 +++++++++++++++++++++--------- >> include/linux/tpm.h | 13 ++++++++--- >> security/integrity/ima/ima_queue.c | 5 ++++- >> security/keys/trusted.c | 5 ++++- >> 7 files changed, 62 insertions(+), 38 deletions(-) >> >> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c >> index eb7c79ca8a94..911fea19e408 100644 >> --- a/drivers/char/tpm/tpm-interface.c >> +++ b/drivers/char/tpm/tpm-interface.c >> @@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read); >> * tpm_pcr_extend - extend a PCR value in SHA1 bank. >> * @chip: a &struct tpm_chip instance, %NULL for the default chip >> * @pcr_idx: the PCR to be retrieved >> - * @hash: the hash value used to extend the PCR value >> + * @count: number of tpm_extend_digest structures >> + * @digests: array of tpm_extend_digest structures used to extend PCRs >> * >> * Note: with TPM 2.0 extends also those banks for which no digest was >> * specified in order to prevent malicious use of those PCR banks. >> * >> * Return: same as with tpm_transmit_cmd() >> */ >> -int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash) >> +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >> + const struct tpm_extend_digest *digests) > > Remove const. Document how @digests is used like the special meaning > of the first index. I faintly remember asking this last time. > >> { >> int rc; >> - struct tpm_digest *digest_list; >> - int i; >> >> chip = tpm_find_get_ops(chip); >> if (!chip) >> return -ENODEV; >> >> if (chip->flags & TPM_CHIP_FLAG_TPM2) { >> - digest_list = kcalloc(chip->nr_allocated_banks, >> - sizeof(*digest_list), GFP_KERNEL); >> - if (!digest_list) >> - return -ENOMEM; >> - >> - for (i = 0; i < chip->nr_allocated_banks; i++) { >> - digest_list[i].alg_id = chip->allocated_banks[i].alg_id; >> - memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE); >> - } >> - >> - rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks, >> - digest_list); >> - kfree(digest_list); >> + rc = tpm2_pcr_extend(chip, pcr_idx, count, digests); >> tpm_put_ops(chip); >> return rc; >> } >> >> - rc = tpm1_pcr_extend(chip, pcr_idx, hash, >> + rc = tpm1_pcr_extend(chip, pcr_idx, count, digests, >> "attempting extend a PCR value"); > > The validation is missing that the provided array has only one element > and the algorithm is SHA1. Could be done also inside tpm1_pcr_extend() > but what you are doing to that function does not make any sense so > better to do it here. > >> tpm_put_ops(chip); >> return rc; >> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h >> index 64d93d26087f..6b446504d2fe 100644 >> --- a/drivers/char/tpm/tpm.h >> +++ b/drivers/char/tpm/tpm.h >> @@ -504,7 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip); >> int tpm1_do_selftest(struct tpm_chip *chip); >> int tpm1_get_timeouts(struct tpm_chip *chip); >> unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal); >> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash, >> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >> + const struct tpm_extend_digest *digests, >> const char *log_msg); >> int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf); >> ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap, >> @@ -551,7 +552,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip); >> int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, >> struct tpm_digest *digest, u16 *digest_size_ptr); >> int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >> - struct tpm_digest *digests); >> + const struct tpm_extend_digest *digests); >> int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max); >> void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle, >> unsigned int flags); >> diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c >> index 8b70a7f884a7..04ee10284b8c 100644 >> --- a/drivers/char/tpm/tpm1-cmd.c >> +++ b/drivers/char/tpm/tpm1-cmd.c >> @@ -449,12 +449,20 @@ int tpm1_get_timeouts(struct tpm_chip *chip) >> } >> >> #define TPM_ORD_PCR_EXTEND 20 >> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash, >> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >> + const struct tpm_extend_digest *digests, >> const char *log_msg) >> { >> struct tpm_buf buf; >> + u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 }; >> + const u8 *hash; >> int rc; >> >> + hash = dummy_hash; >> + if (count) >> + memcpy(dummy_hash, digests[0].data, >> + min(digests[0].size, (u16)sizeof(dummy_hash))); >> + > > You copy memory from one place to another without any good reason to do > so. My suggestion is just not to change tpm1_pcr_extend() at all. > >> rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND); >> if (rc) >> return rc; >> @@ -743,7 +751,6 @@ int tpm1_auto_startup(struct tpm_chip *chip) >> */ >> int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr) >> { >> - u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 }; >> struct tpm_buf buf; >> unsigned int try; >> int rc; >> @@ -751,7 +758,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr) >> >> /* for buggy tpm, flush pcrs with extend to selected dummy */ >> if (tpm_suspend_pcr) >> - rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash, >> + rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL, >> "extending dummy pcr before suspend"); >> >> rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE); >> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c >> index 6ce5173cf0e5..77b5808270c6 100644 >> --- a/drivers/char/tpm/tpm2-cmd.c >> +++ b/drivers/char/tpm/tpm2-cmd.c >> @@ -247,21 +247,22 @@ struct tpm2_null_auth_area { >> * >> * @chip: TPM chip to use. >> * @pcr_idx: index of the PCR. >> - * @count: number of digests passed. >> - * @digests: list of pcr banks and corresponding digest values to extend. >> + * @count: number of tpm_extend_digest passed. >> + * @digests: array of tpm_extend_digest with digest values to extend. >> * >> * Return: Same as with tpm_transmit_cmd. >> */ > > The documentation about @digests. > >> int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >> - struct tpm_digest *digests) >> + const struct tpm_extend_digest *digests) >> { >> struct tpm_buf buf; >> struct tpm2_null_auth_area auth_area; >> + const struct tpm_extend_digest *digest; >> + u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 }; >> + const u8 *hash; >> int rc; >> int i; >> - >> - if (count > chip->nr_allocated_banks) >> - return -EINVAL; >> + int j; >> >> rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND); >> if (rc) >> @@ -277,11 +278,25 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >> tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area)); >> tpm_buf_append(&buf, (const unsigned char *)&auth_area, >> sizeof(auth_area)); >> - tpm_buf_append_u32(&buf, count); >> + tpm_buf_append_u32(&buf, chip->nr_allocated_banks); >> + >> + if (count) >> + memcpy(dummy_hash, digests[0].data, digests[0].size); >> + >> + for (i = 0; i < chip->nr_allocated_banks; i++) { >> + tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id); >> + >> + hash = dummy_hash; >> + for (j = 0; j < count; j++) { >> + digest = digests + j; >> + >> + if (digest->alg_id == chip->allocated_banks[i].alg_id) { >> + hash = digest->data; >> + break; > > I think the whole design is just wrong. I did re-read your response to > v6 again and I'm very sorry, but I just don't get this. Caller has all > the information (from struct tpm_chip) to give the correct data. This > function should validate that data (check algorithm ID and that's it). The question is if checking tpm->allocated_banks is a strict requirement, or we can allow callers to use the algorithm they are currently using, without further modifications. > Extending with the dummy hash should be done by the caller when > preparing the array, not baked into this function. This kind of also > makes obvious that we don't need this new struct. There should never be > a local variable (whose size is BTW randomly chosen) called dummy_hash. This means duplicating the code for each caller. Currently, this work is done by the TPM driver. Roberto > /Jarkko >
On Thu, Jan 17, 2019 at 08:59:00AM +0100, Roberto Sassu wrote: > On 12/20/2018 4:21 PM, Jarkko Sakkinen wrote: > > On Thu, Dec 13, 2018 at 11:29:45AM +0100, Roberto Sassu wrote: > > > Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest. > > > > > > This patch modifies the definition of tpm_pcr_extend() to allow other > > > kernel subsystems to pass a digest for each algorithm supported by the TPM. > > > All digests are processed by the TPM in one operation. > > > > > > If a tpm_pcr_extend() caller provides a subset of the supported algorithms, > > > the TPM driver extends the remaining PCR banks with the first digest > > > passed as an argument to the function. > > > > > > The new tpm_extend digest structure has been preferred to the tpm_digest > > > structure, to let the caller specify the size of the digest (which may be > > > unknown to the TPM driver). > > > > > > Due to the API change, ima_pcr_extend() and pcrlock() have been modified. > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > --- > > > drivers/char/tpm/tpm-interface.c | 24 +++++--------------- > > > drivers/char/tpm/tpm.h | 5 +++-- > > > drivers/char/tpm/tpm1-cmd.c | 13 ++++++++--- > > > drivers/char/tpm/tpm2-cmd.c | 35 +++++++++++++++++++++--------- > > > include/linux/tpm.h | 13 ++++++++--- > > > security/integrity/ima/ima_queue.c | 5 ++++- > > > security/keys/trusted.c | 5 ++++- > > > 7 files changed, 62 insertions(+), 38 deletions(-) > > > > > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > > > index eb7c79ca8a94..911fea19e408 100644 > > > --- a/drivers/char/tpm/tpm-interface.c > > > +++ b/drivers/char/tpm/tpm-interface.c > > > @@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read); > > > * tpm_pcr_extend - extend a PCR value in SHA1 bank. > > > * @chip: a &struct tpm_chip instance, %NULL for the default chip > > > * @pcr_idx: the PCR to be retrieved > > > - * @hash: the hash value used to extend the PCR value > > > + * @count: number of tpm_extend_digest structures > > > + * @digests: array of tpm_extend_digest structures used to extend PCRs > > > * > > > * Note: with TPM 2.0 extends also those banks for which no digest was > > > * specified in order to prevent malicious use of those PCR banks. > > > * > > > * Return: same as with tpm_transmit_cmd() > > > */ > > > -int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash) > > > +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > > > + const struct tpm_extend_digest *digests) > > > > Remove const. Document how @digests is used like the special meaning > > of the first index. I faintly remember asking this last time. > > > > > { > > > int rc; > > > - struct tpm_digest *digest_list; > > > - int i; > > > chip = tpm_find_get_ops(chip); > > > if (!chip) > > > return -ENODEV; > > > if (chip->flags & TPM_CHIP_FLAG_TPM2) { > > > - digest_list = kcalloc(chip->nr_allocated_banks, > > > - sizeof(*digest_list), GFP_KERNEL); > > > - if (!digest_list) > > > - return -ENOMEM; > > > - > > > - for (i = 0; i < chip->nr_allocated_banks; i++) { > > > - digest_list[i].alg_id = chip->allocated_banks[i].alg_id; > > > - memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE); > > > - } > > > - > > > - rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks, > > > - digest_list); > > > - kfree(digest_list); > > > + rc = tpm2_pcr_extend(chip, pcr_idx, count, digests); > > > tpm_put_ops(chip); > > > return rc; > > > } > > > - rc = tpm1_pcr_extend(chip, pcr_idx, hash, > > > + rc = tpm1_pcr_extend(chip, pcr_idx, count, digests, > > > "attempting extend a PCR value"); > > > > The validation is missing that the provided array has only one element > > and the algorithm is SHA1. Could be done also inside tpm1_pcr_extend() > > but what you are doing to that function does not make any sense so > > better to do it here. > > > > > tpm_put_ops(chip); > > > return rc; > > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > > > index 64d93d26087f..6b446504d2fe 100644 > > > --- a/drivers/char/tpm/tpm.h > > > +++ b/drivers/char/tpm/tpm.h > > > @@ -504,7 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip); > > > int tpm1_do_selftest(struct tpm_chip *chip); > > > int tpm1_get_timeouts(struct tpm_chip *chip); > > > unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal); > > > -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash, > > > +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > > > + const struct tpm_extend_digest *digests, > > > const char *log_msg); > > > int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf); > > > ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap, > > > @@ -551,7 +552,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip); > > > int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, > > > struct tpm_digest *digest, u16 *digest_size_ptr); > > > int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > > > - struct tpm_digest *digests); > > > + const struct tpm_extend_digest *digests); > > > int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max); > > > void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle, > > > unsigned int flags); > > > diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c > > > index 8b70a7f884a7..04ee10284b8c 100644 > > > --- a/drivers/char/tpm/tpm1-cmd.c > > > +++ b/drivers/char/tpm/tpm1-cmd.c > > > @@ -449,12 +449,20 @@ int tpm1_get_timeouts(struct tpm_chip *chip) > > > } > > > #define TPM_ORD_PCR_EXTEND 20 > > > -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash, > > > +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > > > + const struct tpm_extend_digest *digests, > > > const char *log_msg) > > > { > > > struct tpm_buf buf; > > > + u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 }; > > > + const u8 *hash; > > > int rc; > > > + hash = dummy_hash; > > > + if (count) > > > + memcpy(dummy_hash, digests[0].data, > > > + min(digests[0].size, (u16)sizeof(dummy_hash))); > > > + > > > > You copy memory from one place to another without any good reason to do > > so. My suggestion is just not to change tpm1_pcr_extend() at all. > > > > > rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND); > > > if (rc) > > > return rc; > > > @@ -743,7 +751,6 @@ int tpm1_auto_startup(struct tpm_chip *chip) > > > */ > > > int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr) > > > { > > > - u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 }; > > > struct tpm_buf buf; > > > unsigned int try; > > > int rc; > > > @@ -751,7 +758,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr) > > > /* for buggy tpm, flush pcrs with extend to selected dummy */ > > > if (tpm_suspend_pcr) > > > - rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash, > > > + rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL, > > > "extending dummy pcr before suspend"); > > > rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE); > > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > > > index 6ce5173cf0e5..77b5808270c6 100644 > > > --- a/drivers/char/tpm/tpm2-cmd.c > > > +++ b/drivers/char/tpm/tpm2-cmd.c > > > @@ -247,21 +247,22 @@ struct tpm2_null_auth_area { > > > * > > > * @chip: TPM chip to use. > > > * @pcr_idx: index of the PCR. > > > - * @count: number of digests passed. > > > - * @digests: list of pcr banks and corresponding digest values to extend. > > > + * @count: number of tpm_extend_digest passed. > > > + * @digests: array of tpm_extend_digest with digest values to extend. > > > * > > > * Return: Same as with tpm_transmit_cmd. > > > */ > > > > The documentation about @digests. > > > > > int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > > > - struct tpm_digest *digests) > > > + const struct tpm_extend_digest *digests) > > > { > > > struct tpm_buf buf; > > > struct tpm2_null_auth_area auth_area; > > > + const struct tpm_extend_digest *digest; > > > + u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 }; > > > + const u8 *hash; > > > int rc; > > > int i; > > > - > > > - if (count > chip->nr_allocated_banks) > > > - return -EINVAL; > > > + int j; > > > rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND); > > > if (rc) > > > @@ -277,11 +278,25 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > > > tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area)); > > > tpm_buf_append(&buf, (const unsigned char *)&auth_area, > > > sizeof(auth_area)); > > > - tpm_buf_append_u32(&buf, count); > > > + tpm_buf_append_u32(&buf, chip->nr_allocated_banks); > > > + > > > + if (count) > > > + memcpy(dummy_hash, digests[0].data, digests[0].size); > > > + > > > + for (i = 0; i < chip->nr_allocated_banks; i++) { > > > + tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id); > > > + > > > + hash = dummy_hash; > > > + for (j = 0; j < count; j++) { > > > + digest = digests + j; > > > + > > > + if (digest->alg_id == chip->allocated_banks[i].alg_id) { > > > + hash = digest->data; > > > + break; > > > > I think the whole design is just wrong. I did re-read your response to > > v6 again and I'm very sorry, but I just don't get this. Caller has all > > the information (from struct tpm_chip) to give the correct data. This > > function should validate that data (check algorithm ID and that's it). > > The question is if checking tpm->allocated_banks is a strict > requirement, or we can allow callers to use the algorithm they are > currently using, without further modifications. If you want to know what is available, you can use that array. > > Extending with the dummy hash should be done by the caller when > > preparing the array, not baked into this function. This kind of also > > makes obvious that we don't need this new struct. There should never be > > a local variable (whose size is BTW randomly chosen) called dummy_hash. > > This means duplicating the code for each caller. Currently, this work is > done by the TPM driver. It is better than fix the usage pattern like this. Not only that, but this somewhat complicated behavior now completely undocumented in the function description. You are making strong assumptions how some other subsystem might use extend operation. I will rather accept redundancy and consider consolidation later when there are two clients. Rather keep the TPM provided function simple and stupid. /Jarkko
On 1/18/2019 4:12 PM, Jarkko Sakkinen wrote: > On Thu, Jan 17, 2019 at 08:59:00AM +0100, Roberto Sassu wrote: >> On 12/20/2018 4:21 PM, Jarkko Sakkinen wrote: >>> On Thu, Dec 13, 2018 at 11:29:45AM +0100, Roberto Sassu wrote: >>>> Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest. >>>> >>>> This patch modifies the definition of tpm_pcr_extend() to allow other >>>> kernel subsystems to pass a digest for each algorithm supported by the TPM. >>>> All digests are processed by the TPM in one operation. >>>> >>>> If a tpm_pcr_extend() caller provides a subset of the supported algorithms, >>>> the TPM driver extends the remaining PCR banks with the first digest >>>> passed as an argument to the function. >>>> >>>> The new tpm_extend digest structure has been preferred to the tpm_digest >>>> structure, to let the caller specify the size of the digest (which may be >>>> unknown to the TPM driver). >>>> >>>> Due to the API change, ima_pcr_extend() and pcrlock() have been modified. >>>> >>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> >>>> --- >>>> drivers/char/tpm/tpm-interface.c | 24 +++++--------------- >>>> drivers/char/tpm/tpm.h | 5 +++-- >>>> drivers/char/tpm/tpm1-cmd.c | 13 ++++++++--- >>>> drivers/char/tpm/tpm2-cmd.c | 35 +++++++++++++++++++++--------- >>>> include/linux/tpm.h | 13 ++++++++--- >>>> security/integrity/ima/ima_queue.c | 5 ++++- >>>> security/keys/trusted.c | 5 ++++- >>>> 7 files changed, 62 insertions(+), 38 deletions(-) >>>> >>>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c >>>> index eb7c79ca8a94..911fea19e408 100644 >>>> --- a/drivers/char/tpm/tpm-interface.c >>>> +++ b/drivers/char/tpm/tpm-interface.c >>>> @@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read); >>>> * tpm_pcr_extend - extend a PCR value in SHA1 bank. >>>> * @chip: a &struct tpm_chip instance, %NULL for the default chip >>>> * @pcr_idx: the PCR to be retrieved >>>> - * @hash: the hash value used to extend the PCR value >>>> + * @count: number of tpm_extend_digest structures >>>> + * @digests: array of tpm_extend_digest structures used to extend PCRs >>>> * >>>> * Note: with TPM 2.0 extends also those banks for which no digest was >>>> * specified in order to prevent malicious use of those PCR banks. >>>> * >>>> * Return: same as with tpm_transmit_cmd() >>>> */ >>>> -int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash) >>>> +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >>>> + const struct tpm_extend_digest *digests) >>> >>> Remove const. Document how @digests is used like the special meaning >>> of the first index. I faintly remember asking this last time. >>> >>>> { >>>> int rc; >>>> - struct tpm_digest *digest_list; >>>> - int i; >>>> chip = tpm_find_get_ops(chip); >>>> if (!chip) >>>> return -ENODEV; >>>> if (chip->flags & TPM_CHIP_FLAG_TPM2) { >>>> - digest_list = kcalloc(chip->nr_allocated_banks, >>>> - sizeof(*digest_list), GFP_KERNEL); >>>> - if (!digest_list) >>>> - return -ENOMEM; >>>> - >>>> - for (i = 0; i < chip->nr_allocated_banks; i++) { >>>> - digest_list[i].alg_id = chip->allocated_banks[i].alg_id; >>>> - memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE); >>>> - } >>>> - >>>> - rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks, >>>> - digest_list); >>>> - kfree(digest_list); >>>> + rc = tpm2_pcr_extend(chip, pcr_idx, count, digests); >>>> tpm_put_ops(chip); >>>> return rc; >>>> } >>>> - rc = tpm1_pcr_extend(chip, pcr_idx, hash, >>>> + rc = tpm1_pcr_extend(chip, pcr_idx, count, digests, >>>> "attempting extend a PCR value"); >>> >>> The validation is missing that the provided array has only one element >>> and the algorithm is SHA1. Could be done also inside tpm1_pcr_extend() >>> but what you are doing to that function does not make any sense so >>> better to do it here. >>> >>>> tpm_put_ops(chip); >>>> return rc; >>>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h >>>> index 64d93d26087f..6b446504d2fe 100644 >>>> --- a/drivers/char/tpm/tpm.h >>>> +++ b/drivers/char/tpm/tpm.h >>>> @@ -504,7 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip); >>>> int tpm1_do_selftest(struct tpm_chip *chip); >>>> int tpm1_get_timeouts(struct tpm_chip *chip); >>>> unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal); >>>> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash, >>>> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >>>> + const struct tpm_extend_digest *digests, >>>> const char *log_msg); >>>> int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf); >>>> ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap, >>>> @@ -551,7 +552,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip); >>>> int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, >>>> struct tpm_digest *digest, u16 *digest_size_ptr); >>>> int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >>>> - struct tpm_digest *digests); >>>> + const struct tpm_extend_digest *digests); >>>> int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max); >>>> void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle, >>>> unsigned int flags); >>>> diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c >>>> index 8b70a7f884a7..04ee10284b8c 100644 >>>> --- a/drivers/char/tpm/tpm1-cmd.c >>>> +++ b/drivers/char/tpm/tpm1-cmd.c >>>> @@ -449,12 +449,20 @@ int tpm1_get_timeouts(struct tpm_chip *chip) >>>> } >>>> #define TPM_ORD_PCR_EXTEND 20 >>>> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash, >>>> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >>>> + const struct tpm_extend_digest *digests, >>>> const char *log_msg) >>>> { >>>> struct tpm_buf buf; >>>> + u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 }; >>>> + const u8 *hash; >>>> int rc; >>>> + hash = dummy_hash; >>>> + if (count) >>>> + memcpy(dummy_hash, digests[0].data, >>>> + min(digests[0].size, (u16)sizeof(dummy_hash))); >>>> + >>> >>> You copy memory from one place to another without any good reason to do >>> so. My suggestion is just not to change tpm1_pcr_extend() at all. >>> >>>> rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND); >>>> if (rc) >>>> return rc; >>>> @@ -743,7 +751,6 @@ int tpm1_auto_startup(struct tpm_chip *chip) >>>> */ >>>> int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr) >>>> { >>>> - u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 }; >>>> struct tpm_buf buf; >>>> unsigned int try; >>>> int rc; >>>> @@ -751,7 +758,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr) >>>> /* for buggy tpm, flush pcrs with extend to selected dummy */ >>>> if (tpm_suspend_pcr) >>>> - rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash, >>>> + rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL, >>>> "extending dummy pcr before suspend"); >>>> rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE); >>>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c >>>> index 6ce5173cf0e5..77b5808270c6 100644 >>>> --- a/drivers/char/tpm/tpm2-cmd.c >>>> +++ b/drivers/char/tpm/tpm2-cmd.c >>>> @@ -247,21 +247,22 @@ struct tpm2_null_auth_area { >>>> * >>>> * @chip: TPM chip to use. >>>> * @pcr_idx: index of the PCR. >>>> - * @count: number of digests passed. >>>> - * @digests: list of pcr banks and corresponding digest values to extend. >>>> + * @count: number of tpm_extend_digest passed. >>>> + * @digests: array of tpm_extend_digest with digest values to extend. >>>> * >>>> * Return: Same as with tpm_transmit_cmd. >>>> */ >>> >>> The documentation about @digests. >>> >>>> int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >>>> - struct tpm_digest *digests) >>>> + const struct tpm_extend_digest *digests) >>>> { >>>> struct tpm_buf buf; >>>> struct tpm2_null_auth_area auth_area; >>>> + const struct tpm_extend_digest *digest; >>>> + u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 }; >>>> + const u8 *hash; >>>> int rc; >>>> int i; >>>> - >>>> - if (count > chip->nr_allocated_banks) >>>> - return -EINVAL; >>>> + int j; >>>> rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND); >>>> if (rc) >>>> @@ -277,11 +278,25 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >>>> tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area)); >>>> tpm_buf_append(&buf, (const unsigned char *)&auth_area, >>>> sizeof(auth_area)); >>>> - tpm_buf_append_u32(&buf, count); >>>> + tpm_buf_append_u32(&buf, chip->nr_allocated_banks); >>>> + >>>> + if (count) >>>> + memcpy(dummy_hash, digests[0].data, digests[0].size); >>>> + >>>> + for (i = 0; i < chip->nr_allocated_banks; i++) { >>>> + tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id); >>>> + >>>> + hash = dummy_hash; >>>> + for (j = 0; j < count; j++) { >>>> + digest = digests + j; >>>> + >>>> + if (digest->alg_id == chip->allocated_banks[i].alg_id) { >>>> + hash = digest->data; >>>> + break; >>> >>> I think the whole design is just wrong. I did re-read your response to >>> v6 again and I'm very sorry, but I just don't get this. Caller has all >>> the information (from struct tpm_chip) to give the correct data. This >>> function should validate that data (check algorithm ID and that's it). >> >> The question is if checking tpm->allocated_banks is a strict >> requirement, or we can allow callers to use the algorithm they are >> currently using, without further modifications. > > If you want to know what is available, you can use that array. > >>> Extending with the dummy hash should be done by the caller when >>> preparing the array, not baked into this function. This kind of also >>> makes obvious that we don't need this new struct. There should never be >>> a local variable (whose size is BTW randomly chosen) called dummy_hash. >> >> This means duplicating the code for each caller. Currently, this work is >> done by the TPM driver. > > It is better than fix the usage pattern like this. Not only that, but > this somewhat complicated behavior now completely undocumented in the > function description. > > You are making strong assumptions how some other subsystem might use > extend operation. I will rather accept redundancy and consider > consolidation later when there are two clients. Rather keep the TPM > provided function simple and stupid. Ok, if I understood it correctly, every caller of tpm_pcr_extend() will pass an array of tpm_digest structures. I will not introduce a new structure, as the caller has to use algorithms from chip->allocated_banks and passing the digest size is not necessary. tpm_pcr_extend() will reject incomplete arrays (one or more algorithms are missing). To simplify even more tpm_pcr_extend(), I assume and check that passed algorithms are in the same order as those in chip->allocated_banks. I will remove the 'count' parameter from tpm_pcr_extend(), as the number of tpm_digest structures will be always chip->nr_allocated_banks. Roberto > /Jarkko >
On 1/18/2019 4:12 PM, Jarkko Sakkinen wrote: > On Thu, Jan 17, 2019 at 08:59:00AM +0100, Roberto Sassu wrote: >> On 12/20/2018 4:21 PM, Jarkko Sakkinen wrote: >>> On Thu, Dec 13, 2018 at 11:29:45AM +0100, Roberto Sassu wrote: >>>> Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest. >>>> >>>> This patch modifies the definition of tpm_pcr_extend() to allow other >>>> kernel subsystems to pass a digest for each algorithm supported by the TPM. >>>> All digests are processed by the TPM in one operation. >>>> >>>> If a tpm_pcr_extend() caller provides a subset of the supported algorithms, >>>> the TPM driver extends the remaining PCR banks with the first digest >>>> passed as an argument to the function. >>>> >>>> The new tpm_extend digest structure has been preferred to the tpm_digest >>>> structure, to let the caller specify the size of the digest (which may be >>>> unknown to the TPM driver). >>>> >>>> Due to the API change, ima_pcr_extend() and pcrlock() have been modified. >>>> >>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> >>>> --- >>>> drivers/char/tpm/tpm-interface.c | 24 +++++--------------- >>>> drivers/char/tpm/tpm.h | 5 +++-- >>>> drivers/char/tpm/tpm1-cmd.c | 13 ++++++++--- >>>> drivers/char/tpm/tpm2-cmd.c | 35 +++++++++++++++++++++--------- >>>> include/linux/tpm.h | 13 ++++++++--- >>>> security/integrity/ima/ima_queue.c | 5 ++++- >>>> security/keys/trusted.c | 5 ++++- >>>> 7 files changed, 62 insertions(+), 38 deletions(-) >>>> >>>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c >>>> index eb7c79ca8a94..911fea19e408 100644 >>>> --- a/drivers/char/tpm/tpm-interface.c >>>> +++ b/drivers/char/tpm/tpm-interface.c >>>> @@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read); >>>> * tpm_pcr_extend - extend a PCR value in SHA1 bank. >>>> * @chip: a &struct tpm_chip instance, %NULL for the default chip >>>> * @pcr_idx: the PCR to be retrieved >>>> - * @hash: the hash value used to extend the PCR value >>>> + * @count: number of tpm_extend_digest structures >>>> + * @digests: array of tpm_extend_digest structures used to extend PCRs >>>> * >>>> * Note: with TPM 2.0 extends also those banks for which no digest was >>>> * specified in order to prevent malicious use of those PCR banks. >>>> * >>>> * Return: same as with tpm_transmit_cmd() >>>> */ >>>> -int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash) >>>> +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >>>> + const struct tpm_extend_digest *digests) >>> >>> Remove const. Document how @digests is used like the special meaning >>> of the first index. I faintly remember asking this last time. >>> >>>> { >>>> int rc; >>>> - struct tpm_digest *digest_list; >>>> - int i; >>>> chip = tpm_find_get_ops(chip); >>>> if (!chip) >>>> return -ENODEV; >>>> if (chip->flags & TPM_CHIP_FLAG_TPM2) { >>>> - digest_list = kcalloc(chip->nr_allocated_banks, >>>> - sizeof(*digest_list), GFP_KERNEL); >>>> - if (!digest_list) >>>> - return -ENOMEM; >>>> - >>>> - for (i = 0; i < chip->nr_allocated_banks; i++) { >>>> - digest_list[i].alg_id = chip->allocated_banks[i].alg_id; >>>> - memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE); >>>> - } >>>> - >>>> - rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks, >>>> - digest_list); >>>> - kfree(digest_list); >>>> + rc = tpm2_pcr_extend(chip, pcr_idx, count, digests); >>>> tpm_put_ops(chip); >>>> return rc; >>>> } >>>> - rc = tpm1_pcr_extend(chip, pcr_idx, hash, >>>> + rc = tpm1_pcr_extend(chip, pcr_idx, count, digests, >>>> "attempting extend a PCR value"); >>> >>> The validation is missing that the provided array has only one element >>> and the algorithm is SHA1. Could be done also inside tpm1_pcr_extend() >>> but what you are doing to that function does not make any sense so >>> better to do it here. >>> >>>> tpm_put_ops(chip); >>>> return rc; >>>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h >>>> index 64d93d26087f..6b446504d2fe 100644 >>>> --- a/drivers/char/tpm/tpm.h >>>> +++ b/drivers/char/tpm/tpm.h >>>> @@ -504,7 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip); >>>> int tpm1_do_selftest(struct tpm_chip *chip); >>>> int tpm1_get_timeouts(struct tpm_chip *chip); >>>> unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal); >>>> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash, >>>> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >>>> + const struct tpm_extend_digest *digests, >>>> const char *log_msg); >>>> int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf); >>>> ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap, >>>> @@ -551,7 +552,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip); >>>> int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, >>>> struct tpm_digest *digest, u16 *digest_size_ptr); >>>> int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >>>> - struct tpm_digest *digests); >>>> + const struct tpm_extend_digest *digests); >>>> int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max); >>>> void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle, >>>> unsigned int flags); >>>> diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c >>>> index 8b70a7f884a7..04ee10284b8c 100644 >>>> --- a/drivers/char/tpm/tpm1-cmd.c >>>> +++ b/drivers/char/tpm/tpm1-cmd.c >>>> @@ -449,12 +449,20 @@ int tpm1_get_timeouts(struct tpm_chip *chip) >>>> } >>>> #define TPM_ORD_PCR_EXTEND 20 >>>> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash, >>>> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >>>> + const struct tpm_extend_digest *digests, >>>> const char *log_msg) >>>> { >>>> struct tpm_buf buf; >>>> + u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 }; >>>> + const u8 *hash; >>>> int rc; >>>> + hash = dummy_hash; >>>> + if (count) >>>> + memcpy(dummy_hash, digests[0].data, >>>> + min(digests[0].size, (u16)sizeof(dummy_hash))); >>>> + >>> >>> You copy memory from one place to another without any good reason to do >>> so. My suggestion is just not to change tpm1_pcr_extend() at all. >>> >>>> rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND); >>>> if (rc) >>>> return rc; >>>> @@ -743,7 +751,6 @@ int tpm1_auto_startup(struct tpm_chip *chip) >>>> */ >>>> int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr) >>>> { >>>> - u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 }; >>>> struct tpm_buf buf; >>>> unsigned int try; >>>> int rc; >>>> @@ -751,7 +758,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr) >>>> /* for buggy tpm, flush pcrs with extend to selected dummy */ >>>> if (tpm_suspend_pcr) >>>> - rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash, >>>> + rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL, >>>> "extending dummy pcr before suspend"); >>>> rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE); >>>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c >>>> index 6ce5173cf0e5..77b5808270c6 100644 >>>> --- a/drivers/char/tpm/tpm2-cmd.c >>>> +++ b/drivers/char/tpm/tpm2-cmd.c >>>> @@ -247,21 +247,22 @@ struct tpm2_null_auth_area { >>>> * >>>> * @chip: TPM chip to use. >>>> * @pcr_idx: index of the PCR. >>>> - * @count: number of digests passed. >>>> - * @digests: list of pcr banks and corresponding digest values to extend. >>>> + * @count: number of tpm_extend_digest passed. >>>> + * @digests: array of tpm_extend_digest with digest values to extend. >>>> * >>>> * Return: Same as with tpm_transmit_cmd. >>>> */ >>> >>> The documentation about @digests. >>> >>>> int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >>>> - struct tpm_digest *digests) >>>> + const struct tpm_extend_digest *digests) >>>> { >>>> struct tpm_buf buf; >>>> struct tpm2_null_auth_area auth_area; >>>> + const struct tpm_extend_digest *digest; >>>> + u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 }; >>>> + const u8 *hash; >>>> int rc; >>>> int i; >>>> - >>>> - if (count > chip->nr_allocated_banks) >>>> - return -EINVAL; >>>> + int j; >>>> rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND); >>>> if (rc) >>>> @@ -277,11 +278,25 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, >>>> tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area)); >>>> tpm_buf_append(&buf, (const unsigned char *)&auth_area, >>>> sizeof(auth_area)); >>>> - tpm_buf_append_u32(&buf, count); >>>> + tpm_buf_append_u32(&buf, chip->nr_allocated_banks); >>>> + >>>> + if (count) >>>> + memcpy(dummy_hash, digests[0].data, digests[0].size); >>>> + >>>> + for (i = 0; i < chip->nr_allocated_banks; i++) { >>>> + tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id); >>>> + >>>> + hash = dummy_hash; >>>> + for (j = 0; j < count; j++) { >>>> + digest = digests + j; >>>> + >>>> + if (digest->alg_id == chip->allocated_banks[i].alg_id) { >>>> + hash = digest->data; >>>> + break; >>> >>> I think the whole design is just wrong. I did re-read your response to >>> v6 again and I'm very sorry, but I just don't get this. Caller has all >>> the information (from struct tpm_chip) to give the correct data. This >>> function should validate that data (check algorithm ID and that's it). >> >> The question is if checking tpm->allocated_banks is a strict >> requirement, or we can allow callers to use the algorithm they are >> currently using, without further modifications. > > If you want to know what is available, you can use that array. > >>> Extending with the dummy hash should be done by the caller when >>> preparing the array, not baked into this function. This kind of also >>> makes obvious that we don't need this new struct. There should never be >>> a local variable (whose size is BTW randomly chosen) called dummy_hash. >> >> This means duplicating the code for each caller. Currently, this work is >> done by the TPM driver. > > It is better than fix the usage pattern like this. Not only that, but > this somewhat complicated behavior now completely undocumented in the > function description. > > You are making strong assumptions how some other subsystem might use > extend operation. I will rather accept redundancy and consider > consolidation later when there are two clients. Rather keep the TPM > provided function simple and stupid. I just found that your requirement does not match Mimi's requirements: --- > The caller of tpm_pcr_extend() could pass a subset of the allocated > banks, but the TPM driver extends all banks as before. Agreed, there should be a clear division. 1) The caller shouldn't need to know anything about the chip->info. 2) The TPM driver should not rely on the caller to supply all the hashes, but verify that all allocated banks are being extended. --- See https://lkml.org/lkml/2018/11/19/603. Also, if callers should be able to retrieve the list of supported algorithms from the TPM, I should move the tpm_chip definition to include/linux/tpm.h or introduce a new function that returns them. However, you said that this shouldn't be done now: --- > > > @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash) > > > return -ENODEV; > > > > Should take digest_list as input. Probably callers could re-use the > > same digest_list array multiple times? > > > > Move struct tpm_chip to include/linux/tpm.h so that the caller can query > > nr_active_banks and active_banks and can create the digest array by > > itself. Lets do this right at once now that this is being restructured. > > I have to move also other structures and #define. Wouldn't be better to > introduce a new function to pass to the caller active_banks and > nr_active_banks? Revisited. I think it is fine how it is for now and we reconsider later. Only thing I want to remark is that use should use kcalloc() instead of kalloc_array() + memset(). --- See https://lkml.org/lkml/2018/11/13/1059. How we should proceed? Thanks Roberto > /Jarkko >
On Mon, Jan 21, 2019 at 09:11:21AM +0100, Roberto Sassu wrote: > On 1/18/2019 4:12 PM, Jarkko Sakkinen wrote: > > On Thu, Jan 17, 2019 at 08:59:00AM +0100, Roberto Sassu wrote: > > > On 12/20/2018 4:21 PM, Jarkko Sakkinen wrote: > > > > On Thu, Dec 13, 2018 at 11:29:45AM +0100, Roberto Sassu wrote: > > > > > Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest. > > > > > > > > > > This patch modifies the definition of tpm_pcr_extend() to allow other > > > > > kernel subsystems to pass a digest for each algorithm supported by the TPM. > > > > > All digests are processed by the TPM in one operation. > > > > > > > > > > If a tpm_pcr_extend() caller provides a subset of the supported algorithms, > > > > > the TPM driver extends the remaining PCR banks with the first digest > > > > > passed as an argument to the function. > > > > > > > > > > The new tpm_extend digest structure has been preferred to the tpm_digest > > > > > structure, to let the caller specify the size of the digest (which may be > > > > > unknown to the TPM driver). > > > > > > > > > > Due to the API change, ima_pcr_extend() and pcrlock() have been modified. > > > > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > > > --- > > > > > drivers/char/tpm/tpm-interface.c | 24 +++++--------------- > > > > > drivers/char/tpm/tpm.h | 5 +++-- > > > > > drivers/char/tpm/tpm1-cmd.c | 13 ++++++++--- > > > > > drivers/char/tpm/tpm2-cmd.c | 35 +++++++++++++++++++++--------- > > > > > include/linux/tpm.h | 13 ++++++++--- > > > > > security/integrity/ima/ima_queue.c | 5 ++++- > > > > > security/keys/trusted.c | 5 ++++- > > > > > 7 files changed, 62 insertions(+), 38 deletions(-) > > > > > > > > > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > > > > > index eb7c79ca8a94..911fea19e408 100644 > > > > > --- a/drivers/char/tpm/tpm-interface.c > > > > > +++ b/drivers/char/tpm/tpm-interface.c > > > > > @@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read); > > > > > * tpm_pcr_extend - extend a PCR value in SHA1 bank. > > > > > * @chip: a &struct tpm_chip instance, %NULL for the default chip > > > > > * @pcr_idx: the PCR to be retrieved > > > > > - * @hash: the hash value used to extend the PCR value > > > > > + * @count: number of tpm_extend_digest structures > > > > > + * @digests: array of tpm_extend_digest structures used to extend PCRs > > > > > * > > > > > * Note: with TPM 2.0 extends also those banks for which no digest was > > > > > * specified in order to prevent malicious use of those PCR banks. > > > > > * > > > > > * Return: same as with tpm_transmit_cmd() > > > > > */ > > > > > -int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash) > > > > > +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > > > > > + const struct tpm_extend_digest *digests) > > > > > > > > Remove const. Document how @digests is used like the special meaning > > > > of the first index. I faintly remember asking this last time. > > > > > > > > > { > > > > > int rc; > > > > > - struct tpm_digest *digest_list; > > > > > - int i; > > > > > chip = tpm_find_get_ops(chip); > > > > > if (!chip) > > > > > return -ENODEV; > > > > > if (chip->flags & TPM_CHIP_FLAG_TPM2) { > > > > > - digest_list = kcalloc(chip->nr_allocated_banks, > > > > > - sizeof(*digest_list), GFP_KERNEL); > > > > > - if (!digest_list) > > > > > - return -ENOMEM; > > > > > - > > > > > - for (i = 0; i < chip->nr_allocated_banks; i++) { > > > > > - digest_list[i].alg_id = chip->allocated_banks[i].alg_id; > > > > > - memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE); > > > > > - } > > > > > - > > > > > - rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks, > > > > > - digest_list); > > > > > - kfree(digest_list); > > > > > + rc = tpm2_pcr_extend(chip, pcr_idx, count, digests); > > > > > tpm_put_ops(chip); > > > > > return rc; > > > > > } > > > > > - rc = tpm1_pcr_extend(chip, pcr_idx, hash, > > > > > + rc = tpm1_pcr_extend(chip, pcr_idx, count, digests, > > > > > "attempting extend a PCR value"); > > > > > > > > The validation is missing that the provided array has only one element > > > > and the algorithm is SHA1. Could be done also inside tpm1_pcr_extend() > > > > but what you are doing to that function does not make any sense so > > > > better to do it here. > > > > > > > > > tpm_put_ops(chip); > > > > > return rc; > > > > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > > > > > index 64d93d26087f..6b446504d2fe 100644 > > > > > --- a/drivers/char/tpm/tpm.h > > > > > +++ b/drivers/char/tpm/tpm.h > > > > > @@ -504,7 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip); > > > > > int tpm1_do_selftest(struct tpm_chip *chip); > > > > > int tpm1_get_timeouts(struct tpm_chip *chip); > > > > > unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal); > > > > > -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash, > > > > > +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > > > > > + const struct tpm_extend_digest *digests, > > > > > const char *log_msg); > > > > > int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf); > > > > > ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap, > > > > > @@ -551,7 +552,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip); > > > > > int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, > > > > > struct tpm_digest *digest, u16 *digest_size_ptr); > > > > > int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > > > > > - struct tpm_digest *digests); > > > > > + const struct tpm_extend_digest *digests); > > > > > int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max); > > > > > void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle, > > > > > unsigned int flags); > > > > > diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c > > > > > index 8b70a7f884a7..04ee10284b8c 100644 > > > > > --- a/drivers/char/tpm/tpm1-cmd.c > > > > > +++ b/drivers/char/tpm/tpm1-cmd.c > > > > > @@ -449,12 +449,20 @@ int tpm1_get_timeouts(struct tpm_chip *chip) > > > > > } > > > > > #define TPM_ORD_PCR_EXTEND 20 > > > > > -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash, > > > > > +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > > > > > + const struct tpm_extend_digest *digests, > > > > > const char *log_msg) > > > > > { > > > > > struct tpm_buf buf; > > > > > + u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 }; > > > > > + const u8 *hash; > > > > > int rc; > > > > > + hash = dummy_hash; > > > > > + if (count) > > > > > + memcpy(dummy_hash, digests[0].data, > > > > > + min(digests[0].size, (u16)sizeof(dummy_hash))); > > > > > + > > > > > > > > You copy memory from one place to another without any good reason to do > > > > so. My suggestion is just not to change tpm1_pcr_extend() at all. > > > > > > > > > rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND); > > > > > if (rc) > > > > > return rc; > > > > > @@ -743,7 +751,6 @@ int tpm1_auto_startup(struct tpm_chip *chip) > > > > > */ > > > > > int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr) > > > > > { > > > > > - u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 }; > > > > > struct tpm_buf buf; > > > > > unsigned int try; > > > > > int rc; > > > > > @@ -751,7 +758,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr) > > > > > /* for buggy tpm, flush pcrs with extend to selected dummy */ > > > > > if (tpm_suspend_pcr) > > > > > - rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash, > > > > > + rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL, > > > > > "extending dummy pcr before suspend"); > > > > > rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE); > > > > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > > > > > index 6ce5173cf0e5..77b5808270c6 100644 > > > > > --- a/drivers/char/tpm/tpm2-cmd.c > > > > > +++ b/drivers/char/tpm/tpm2-cmd.c > > > > > @@ -247,21 +247,22 @@ struct tpm2_null_auth_area { > > > > > * > > > > > * @chip: TPM chip to use. > > > > > * @pcr_idx: index of the PCR. > > > > > - * @count: number of digests passed. > > > > > - * @digests: list of pcr banks and corresponding digest values to extend. > > > > > + * @count: number of tpm_extend_digest passed. > > > > > + * @digests: array of tpm_extend_digest with digest values to extend. > > > > > * > > > > > * Return: Same as with tpm_transmit_cmd. > > > > > */ > > > > > > > > The documentation about @digests. > > > > > > > > > int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > > > > > - struct tpm_digest *digests) > > > > > + const struct tpm_extend_digest *digests) > > > > > { > > > > > struct tpm_buf buf; > > > > > struct tpm2_null_auth_area auth_area; > > > > > + const struct tpm_extend_digest *digest; > > > > > + u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 }; > > > > > + const u8 *hash; > > > > > int rc; > > > > > int i; > > > > > - > > > > > - if (count > chip->nr_allocated_banks) > > > > > - return -EINVAL; > > > > > + int j; > > > > > rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND); > > > > > if (rc) > > > > > @@ -277,11 +278,25 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, > > > > > tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area)); > > > > > tpm_buf_append(&buf, (const unsigned char *)&auth_area, > > > > > sizeof(auth_area)); > > > > > - tpm_buf_append_u32(&buf, count); > > > > > + tpm_buf_append_u32(&buf, chip->nr_allocated_banks); > > > > > + > > > > > + if (count) > > > > > + memcpy(dummy_hash, digests[0].data, digests[0].size); > > > > > + > > > > > + for (i = 0; i < chip->nr_allocated_banks; i++) { > > > > > + tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id); > > > > > + > > > > > + hash = dummy_hash; > > > > > + for (j = 0; j < count; j++) { > > > > > + digest = digests + j; > > > > > + > > > > > + if (digest->alg_id == chip->allocated_banks[i].alg_id) { > > > > > + hash = digest->data; > > > > > + break; > > > > > > > > I think the whole design is just wrong. I did re-read your response to > > > > v6 again and I'm very sorry, but I just don't get this. Caller has all > > > > the information (from struct tpm_chip) to give the correct data. This > > > > function should validate that data (check algorithm ID and that's it). > > > > > > The question is if checking tpm->allocated_banks is a strict > > > requirement, or we can allow callers to use the algorithm they are > > > currently using, without further modifications. > > > > If you want to know what is available, you can use that array. > > > > > > Extending with the dummy hash should be done by the caller when > > > > preparing the array, not baked into this function. This kind of also > > > > makes obvious that we don't need this new struct. There should never be > > > > a local variable (whose size is BTW randomly chosen) called dummy_hash. > > > > > > This means duplicating the code for each caller. Currently, this work is > > > done by the TPM driver. > > > > It is better than fix the usage pattern like this. Not only that, but > > this somewhat complicated behavior now completely undocumented in the > > function description. > > > > You are making strong assumptions how some other subsystem might use > > extend operation. I will rather accept redundancy and consider > > consolidation later when there are two clients. Rather keep the TPM > > provided function simple and stupid. > > Ok, if I understood it correctly, every caller of tpm_pcr_extend() will pass > an array of tpm_digest structures. > > I will not introduce a new structure, as the caller has to use > algorithms from chip->allocated_banks and passing the digest size is not > necessary. > > tpm_pcr_extend() will reject incomplete arrays (one or more algorithms > are missing). To simplify even more tpm_pcr_extend(), I assume and check > that passed algorithms are in the same order as those in > chip->allocated_banks. > > I will remove the 'count' parameter from tpm_pcr_extend(), as the number > of tpm_digest structures will be always chip->nr_allocated_banks. Sounds like a plan. I think some redundancy is almost always better than hard to undestand API. /Jarkko
On Mon, Jan 21, 2019 at 10:58:22AM +0100, Roberto Sassu wrote: > Agreed, there should be a clear division. > > 1) The caller shouldn't need to know anything about the chip->info. > 2) The TPM driver should not rely on the caller to supply all the > hashes, but verify that all allocated banks are being extended. 1. It is just plain wrong if the basic extend function invents its own dummy hashes when it doesn't get one. In the end, this is what matters to me, and I'm not going to accept anything that tries to do that. That is something that caller should prepare. You even left undocumented this very awkward and unguessable behaviour. I think redundancy is better than doing guesswork what a function might possibly do other than its basic task, which is constructing the extend command. 2. The driver should return -EINVAL, if it doesn't get all the hashes. 3. The would be nothing wrong exposing struct tpm_chip in include/linux/tpm.h. I would be totally fine with that. /Jarkko
On 1/21/2019 1:37 PM, Jarkko Sakkinen wrote: > 3. The would be nothing wrong exposing struct tpm_chip in > include/linux/tpm.h. I would be totally fine with that. Should I do it in a separate patch (before 5/5)? Is it fine to call tpm_default_chip() only in pcrlock() for trusted keys? Thanks Roberto > /Jarkko >
On Mon, Jan 21, 2019 at 02:50:49PM +0100, Roberto Sassu wrote: > On 1/21/2019 1:37 PM, Jarkko Sakkinen wrote: > > 3. The would be nothing wrong exposing struct tpm_chip in > > include/linux/tpm.h. I would be totally fine with that. > > Should I do it in a separate patch (before 5/5)? Yes. > Is it fine to call tpm_default_chip() only in pcrlock() for trusted > keys? I think you should get the reference in init_trusted() (similar pattern as in IMA). /Jarkko
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index eb7c79ca8a94..911fea19e408 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read); * tpm_pcr_extend - extend a PCR value in SHA1 bank. * @chip: a &struct tpm_chip instance, %NULL for the default chip * @pcr_idx: the PCR to be retrieved - * @hash: the hash value used to extend the PCR value + * @count: number of tpm_extend_digest structures + * @digests: array of tpm_extend_digest structures used to extend PCRs * * Note: with TPM 2.0 extends also those banks for which no digest was * specified in order to prevent malicious use of those PCR banks. * * Return: same as with tpm_transmit_cmd() */ -int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash) +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, + const struct tpm_extend_digest *digests) { int rc; - struct tpm_digest *digest_list; - int i; chip = tpm_find_get_ops(chip); if (!chip) return -ENODEV; if (chip->flags & TPM_CHIP_FLAG_TPM2) { - digest_list = kcalloc(chip->nr_allocated_banks, - sizeof(*digest_list), GFP_KERNEL); - if (!digest_list) - return -ENOMEM; - - for (i = 0; i < chip->nr_allocated_banks; i++) { - digest_list[i].alg_id = chip->allocated_banks[i].alg_id; - memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE); - } - - rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks, - digest_list); - kfree(digest_list); + rc = tpm2_pcr_extend(chip, pcr_idx, count, digests); tpm_put_ops(chip); return rc; } - rc = tpm1_pcr_extend(chip, pcr_idx, hash, + rc = tpm1_pcr_extend(chip, pcr_idx, count, digests, "attempting extend a PCR value"); tpm_put_ops(chip); return rc; diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 64d93d26087f..6b446504d2fe 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -504,7 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip); int tpm1_do_selftest(struct tpm_chip *chip); int tpm1_get_timeouts(struct tpm_chip *chip); unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal); -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash, +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, + const struct tpm_extend_digest *digests, const char *log_msg); int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf); ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap, @@ -551,7 +552,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip); int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, struct tpm_digest *digest, u16 *digest_size_ptr); int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, - struct tpm_digest *digests); + const struct tpm_extend_digest *digests); int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max); void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle, unsigned int flags); diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c index 8b70a7f884a7..04ee10284b8c 100644 --- a/drivers/char/tpm/tpm1-cmd.c +++ b/drivers/char/tpm/tpm1-cmd.c @@ -449,12 +449,20 @@ int tpm1_get_timeouts(struct tpm_chip *chip) } #define TPM_ORD_PCR_EXTEND 20 -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash, +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, + const struct tpm_extend_digest *digests, const char *log_msg) { struct tpm_buf buf; + u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 }; + const u8 *hash; int rc; + hash = dummy_hash; + if (count) + memcpy(dummy_hash, digests[0].data, + min(digests[0].size, (u16)sizeof(dummy_hash))); + rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND); if (rc) return rc; @@ -743,7 +751,6 @@ int tpm1_auto_startup(struct tpm_chip *chip) */ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr) { - u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 }; struct tpm_buf buf; unsigned int try; int rc; @@ -751,7 +758,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr) /* for buggy tpm, flush pcrs with extend to selected dummy */ if (tpm_suspend_pcr) - rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash, + rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL, "extending dummy pcr before suspend"); rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE); diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index 6ce5173cf0e5..77b5808270c6 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -247,21 +247,22 @@ struct tpm2_null_auth_area { * * @chip: TPM chip to use. * @pcr_idx: index of the PCR. - * @count: number of digests passed. - * @digests: list of pcr banks and corresponding digest values to extend. + * @count: number of tpm_extend_digest passed. + * @digests: array of tpm_extend_digest with digest values to extend. * * Return: Same as with tpm_transmit_cmd. */ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, - struct tpm_digest *digests) + const struct tpm_extend_digest *digests) { struct tpm_buf buf; struct tpm2_null_auth_area auth_area; + const struct tpm_extend_digest *digest; + u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 }; + const u8 *hash; int rc; int i; - - if (count > chip->nr_allocated_banks) - return -EINVAL; + int j; rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND); if (rc) @@ -277,11 +278,25 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area)); tpm_buf_append(&buf, (const unsigned char *)&auth_area, sizeof(auth_area)); - tpm_buf_append_u32(&buf, count); + tpm_buf_append_u32(&buf, chip->nr_allocated_banks); + + if (count) + memcpy(dummy_hash, digests[0].data, digests[0].size); + + for (i = 0; i < chip->nr_allocated_banks; i++) { + tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id); + + hash = dummy_hash; + for (j = 0; j < count; j++) { + digest = digests + j; + + if (digest->alg_id == chip->allocated_banks[i].alg_id) { + hash = digest->data; + break; + } + } - for (i = 0; i < count; i++) { - tpm_buf_append_u16(&buf, digests[i].alg_id); - tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest, + tpm_buf_append(&buf, hash, chip->allocated_banks[i].digest_size); } diff --git a/include/linux/tpm.h b/include/linux/tpm.h index 72df8d4252ef..f865bfdc39dc 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -52,6 +52,12 @@ struct tpm_bank_info { u16 crypto_id; }; +struct tpm_extend_digest { + u16 alg_id; + u16 size; + const u8 *data; +}; + enum TPM_OPS_FLAGS { TPM_OPS_AUTO_STARTUP = BIT(0), }; @@ -79,7 +85,8 @@ struct tpm_class_ops { extern int tpm_is_tpm2(struct tpm_chip *chip); extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx, struct tpm_digest *digest); -extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash); +extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, + const struct tpm_extend_digest *digests); extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen); extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max); extern int tpm_seal_trusted(struct tpm_chip *chip, @@ -101,8 +108,8 @@ static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, return -ENODEV; } -static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, - const u8 *hash) +static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, + const struct tpm_extend_digest *digests) { return -ENODEV; } diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index b186819bd5aa..183733cfb5d2 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -140,12 +140,15 @@ unsigned long ima_get_binary_runtime_size(void) static int ima_pcr_extend(const u8 *hash, int pcr) { + struct tpm_extend_digest digest = { .alg_id = TPM_ALG_SHA1, + .size = TPM_DIGEST_SIZE, + .data = hash }; int result = 0; if (!ima_tpm_chip) return result; - result = tpm_pcr_extend(ima_tpm_chip, pcr, hash); + result = tpm_pcr_extend(ima_tpm_chip, pcr, 1, &digest); if (result != 0) pr_err("Error Communicating to TPM chip, result: %d\n", result); return result; diff --git a/security/keys/trusted.c b/security/keys/trusted.c index ff6789365a12..208a64a3fe1f 100644 --- a/security/keys/trusted.c +++ b/security/keys/trusted.c @@ -380,6 +380,9 @@ EXPORT_SYMBOL_GPL(trusted_tpm_send); static int pcrlock(const int pcrnum) { unsigned char hash[SHA1_DIGEST_SIZE]; + struct tpm_extend_digest digest = { .alg_id = TPM_ALG_SHA1, + .size = sizeof(hash), + .data = hash }; int ret; if (!capable(CAP_SYS_ADMIN)) @@ -387,7 +390,7 @@ static int pcrlock(const int pcrnum) ret = tpm_get_random(NULL, hash, SHA1_DIGEST_SIZE); if (ret != SHA1_DIGEST_SIZE) return ret; - return tpm_pcr_extend(NULL, pcrnum, hash) ? -EINVAL : 0; + return tpm_pcr_extend(NULL, pcrnum, 1, &digest) ? -EINVAL : 0; } /*
Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest. This patch modifies the definition of tpm_pcr_extend() to allow other kernel subsystems to pass a digest for each algorithm supported by the TPM. All digests are processed by the TPM in one operation. If a tpm_pcr_extend() caller provides a subset of the supported algorithms, the TPM driver extends the remaining PCR banks with the first digest passed as an argument to the function. The new tpm_extend digest structure has been preferred to the tpm_digest structure, to let the caller specify the size of the digest (which may be unknown to the TPM driver). Due to the API change, ima_pcr_extend() and pcrlock() have been modified. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- drivers/char/tpm/tpm-interface.c | 24 +++++--------------- drivers/char/tpm/tpm.h | 5 +++-- drivers/char/tpm/tpm1-cmd.c | 13 ++++++++--- drivers/char/tpm/tpm2-cmd.c | 35 +++++++++++++++++++++--------- include/linux/tpm.h | 13 ++++++++--- security/integrity/ima/ima_queue.c | 5 ++++- security/keys/trusted.c | 5 ++++- 7 files changed, 62 insertions(+), 38 deletions(-)