Message ID | 20190701131505.17759-1-msuchanek@suse.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Revert "tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()" | expand |
On Mon, 2019-07-01 at 15:15 +0200, Michal Suchanek wrote: > This reverts commit 0b6cf6b97b7ef1fa3c7fefab0cac897a1c4a3400 to avoid > following crash: Thank you. I think this the right choice for the moment. I fixed a trivial checkpatch.pl error and added the mandatory tags. Can you check quickly v2 (just posted)? I already made it available in my master and next. /Jarkko
On 7/4/2019 12:03 PM, Jarkko Sakkinen wrote: > On Mon, 2019-07-01 at 15:15 +0200, Michal Suchanek wrote: >> This reverts commit 0b6cf6b97b7ef1fa3c7fefab0cac897a1c4a3400 to avoid >> following crash: > > Thank you. I think this the right choice for the moment. I fixed > a trivial checkpatch.pl error and added the mandatory tags. Can > you check quickly v2 (just posted)? > > I already made it available in my master and next. Could you please wait few days? I would prefer to fix this issue instead of reverting the whole patch. Thanks Roberto
On Thu, 2019-07-04 at 13:28 +0200, Roberto Sassu wrote: > On 7/4/2019 12:03 PM, Jarkko Sakkinen wrote: > > On Mon, 2019-07-01 at 15:15 +0200, Michal Suchanek wrote: > >> This reverts commit 0b6cf6b97b7ef1fa3c7fefab0cac897a1c4a3400 to avoid > >> following crash: > > > > Thank you. I think this the right choice for the moment. I fixed > > a trivial checkpatch.pl error and added the mandatory tags. Can > > you check quickly v2 (just posted)? > > > > I already made it available in my master and next. > > Could you please wait few days? I would prefer to fix this issue instead > of reverting the whole patch. Nayna posted a patch late yesterday titled "tpm: fixes uninitialized allocated banks for IBM vtpm driver", which addresses this bug. Mimi
Hi Jarkko, On Thu, 2019-07-04 at 07:48 -0400, Mimi Zohar wrote: > On Thu, 2019-07-04 at 13:28 +0200, Roberto Sassu wrote: > > On 7/4/2019 12:03 PM, Jarkko Sakkinen wrote: > > > On Mon, 2019-07-01 at 15:15 +0200, Michal Suchanek wrote: > > >> This reverts commit 0b6cf6b97b7ef1fa3c7fefab0cac897a1c4a3400 to avoid > > >> following crash: > > > > > > Thank you. I think this the right choice for the moment. I fixed > > > a trivial checkpatch.pl error and added the mandatory tags. Can > > > you check quickly v2 (just posted)? > > > > > > I already made it available in my master and next. > > > > Could you please wait few days? I would prefer to fix this issue instead > > of reverting the whole patch. > > Nayna posted a patch late yesterday titled "tpm: fixes uninitialized > allocated banks for IBM vtpm driver", which addresses this bug. Now with my review, and with Sachin Sant's and Michal Suchánek testing, instead of reverting this patch could you pick up Nayna's patch instead? thanks! Mimi
Hey Mimi! On 2019-07-04 11:46:41, Mimi Zohar wrote: > Hi Jarkko, > > On Thu, 2019-07-04 at 07:48 -0400, Mimi Zohar wrote: > > On Thu, 2019-07-04 at 13:28 +0200, Roberto Sassu wrote: > > > On 7/4/2019 12:03 PM, Jarkko Sakkinen wrote: > > > > On Mon, 2019-07-01 at 15:15 +0200, Michal Suchanek wrote: > > > >> This reverts commit 0b6cf6b97b7ef1fa3c7fefab0cac897a1c4a3400 to avoid > > > >> following crash: > > > > > > > > Thank you. I think this the right choice for the moment. I fixed > > > > a trivial checkpatch.pl error and added the mandatory tags. Can > > > > you check quickly v2 (just posted)? > > > > > > > > I already made it available in my master and next. > > > > > > Could you please wait few days? I would prefer to fix this issue instead > > > of reverting the whole patch. > > > > Nayna posted a patch late yesterday titled "tpm: fixes uninitialized > > allocated banks for IBM vtpm driver", which addresses this bug. > > Now with my review, and with Sachin Sant's and Michal Suchánek > testing, instead of reverting this patch could you pick up Nayna's > patch instead? It looks to me like the revert would also fix a bug that is keeping the eCryptfs module from loading when the TPM is in an "inactive" state: https://bugzilla.kernel.org/show_bug.cgi?id=203953 I just noticed that it was recently discussed here, too: https://lore.kernel.org/linux-integrity/1562244125.6165.95.camel@linux.ibm.com/T/#t I believe that the revert would fix it because the call to init_digests()/tpm_get_random() would no longer be in the path of loading ecryptfs.ko (which depends on encrypted-keys.ko, which depends on trusted.ko). If the revert isn't used, we'll need a different fix for bug 203953. It should be an easy fix but I don't want it to be forgotten. Tyler > > thanks! > > Mimi >
On Thu, 2019-07-04 at 13:28 +0200, Roberto Sassu wrote: > On 7/4/2019 12:03 PM, Jarkko Sakkinen wrote: > > On Mon, 2019-07-01 at 15:15 +0200, Michal Suchanek wrote: > > > This reverts commit 0b6cf6b97b7ef1fa3c7fefab0cac897a1c4a3400 to avoid > > > following crash: > > > > Thank you. I think this the right choice for the moment. I fixed > > a trivial checkpatch.pl error and added the mandatory tags. Can > > you check quickly v2 (just posted)? > > > > I already made it available in my master and next. > > Could you please wait few days? I would prefer to fix this issue instead > of reverting the whole patch. Nayna provided a fix should be ok. /Jarkko
On Thu, 2019-07-04 at 07:48 -0400, Mimi Zohar wrote: > On Thu, 2019-07-04 at 13:28 +0200, Roberto Sassu wrote: > > On 7/4/2019 12:03 PM, Jarkko Sakkinen wrote: > > > On Mon, 2019-07-01 at 15:15 +0200, Michal Suchanek wrote: > > > > This reverts commit 0b6cf6b97b7ef1fa3c7fefab0cac897a1c4a3400 to avoid > > > > following crash: > > > > > > Thank you. I think this the right choice for the moment. I fixed > > > a trivial checkpatch.pl error and added the mandatory tags. Can > > > you check quickly v2 (just posted)? > > > > > > I already made it available in my master and next. > > > > Could you please wait few days? I would prefer to fix this issue instead > > of reverting the whole patch. > > Nayna posted a patch late yesterday titled "tpm: fixes uninitialized > allocated banks for IBM vtpm driver", which addresses this bug. With some minor changes it should be fine. /Jarkko
Hi Tyler, On 07/04/2019 03:58 PM, Tyler Hicks wrote: > Hey Mimi! > > On 2019-07-04 11:46:41, Mimi Zohar wrote: >> Hi Jarkko, >> >> On Thu, 2019-07-04 at 07:48 -0400, Mimi Zohar wrote: >>> On Thu, 2019-07-04 at 13:28 +0200, Roberto Sassu wrote: >>>> On 7/4/2019 12:03 PM, Jarkko Sakkinen wrote: >>>>> On Mon, 2019-07-01 at 15:15 +0200, Michal Suchanek wrote: >>>>>> This reverts commit 0b6cf6b97b7ef1fa3c7fefab0cac897a1c4a3400 to avoid >>>>>> following crash: >>>>> Thank you. I think this the right choice for the moment. I fixed >>>>> a trivial checkpatch.pl error and added the mandatory tags. Can >>>>> you check quickly v2 (just posted)? >>>>> >>>>> I already made it available in my master and next. >>>> Could you please wait few days? I would prefer to fix this issue instead >>>> of reverting the whole patch. >>> Nayna posted a patch late yesterday titled "tpm: fixes uninitialized >>> allocated banks for IBM vtpm driver", which addresses this bug. >> Now with my review, and with Sachin Sant's and Michal Suchánek >> testing, instead of reverting this patch could you pick up Nayna's >> patch instead? > It looks to me like the revert would also fix a bug that is keeping the > eCryptfs module from loading when the TPM is in an "inactive" state: > > https://bugzilla.kernel.org/show_bug.cgi?id=203953 > > I just noticed that it was recently discussed here, too: > > https://lore.kernel.org/linux-integrity/1562244125.6165.95.camel@linux.ibm.com/T/#t > > I believe that the revert would fix it because the call to > init_digests()/tpm_get_random() would no longer be in the path of > loading ecryptfs.ko (which depends on encrypted-keys.ko, which depends > on trusted.ko). > > If the revert isn't used, we'll need a different fix for bug 203953. It > should be an easy fix but I don't want it to be forgotten. I think if TPM is inactive/disabled, it needs to be handled during tpm_chip_register() itself. However, probably that needs more analysis and discussion. For now, in context of the trusted.ko module, it seems init_trusted() should "put_device", but continue even if init_digests() fails, that will fix the issue. Thanks & Regards, - Nayna
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 1b4f95c13e00..dda742184ce2 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -303,34 +303,42 @@ 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 - * @digests: array of tpm_digest structures used to extend PCRs + * @hash: the hash value used to extend the PCR value * - * Note: callers must pass a digest for every allocated PCR bank, in the same - * order of the banks in chip->allocated_banks. + * 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, - struct tpm_digest *digests) +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash) { int rc; + struct tpm_digest *digest_list; int i; chip = tpm_find_get_ops(chip); if (!chip) return -ENODEV; - for (i = 0; i < chip->nr_allocated_banks; i++) - if (digests[i].alg_id != chip->allocated_banks[i].alg_id) - return -EINVAL; - if (chip->flags & TPM_CHIP_FLAG_TPM2) { - rc = tpm2_pcr_extend(chip, pcr_idx, digests); + 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); tpm_put_ops(chip); return rc; } - rc = tpm1_pcr_extend(chip, pcr_idx, digests[0].digest, + rc = tpm1_pcr_extend(chip, pcr_idx, hash, "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 e503ffc3aa39..3cf8ed290305 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -436,7 +436,7 @@ static inline u32 tpm2_rc_value(u32 rc) 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, +int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, struct tpm_digest *digests); int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max); void tpm2_flush_context(struct tpm_chip *chip, u32 handle); diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index 4de49924cfc4..231937d5f58c 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -242,11 +242,12 @@ 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. * * Return: Same as with tpm_transmit_cmd. */ -int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, +int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, struct tpm_digest *digests) { struct tpm_buf buf; @@ -254,6 +255,9 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, int rc; int i; + if (count > chip->nr_allocated_banks) + return -EINVAL; + rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND); if (rc) return rc; @@ -268,9 +272,9 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, 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, chip->nr_allocated_banks); + tpm_buf_append_u32(&buf, count); - for (i = 0; i < chip->nr_allocated_banks; i++) { + 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, chip->allocated_banks[i].digest_size); diff --git a/include/linux/tpm.h b/include/linux/tpm.h index 53c0ea9ec9df..d4516164dc9f 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -166,8 +166,7 @@ struct tpm_chip { 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, - struct tpm_digest *digests); +extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash); 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, @@ -190,7 +189,7 @@ static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, } static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, - struct tpm_digest *digests) + const u8 *hash) { return -ENODEV; } diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index ca10917b5f89..bf7178ef1619 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -149,7 +149,6 @@ int ima_measurements_show(struct seq_file *m, void *v); unsigned long ima_get_binary_runtime_size(void); int ima_init_template(void); void ima_init_template_list(void); -int __init ima_init_digests(void); /* * used to protect h_table and sha_table diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index 1e47c1026471..cf7a2f58077e 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -119,12 +119,8 @@ int __init ima_init(void) if (rc != 0) return rc; - /* It can be called before ima_init_digests(), it does not use TPM. */ ima_load_kexec_buffer(); - rc = ima_init_digests(); - if (rc != 0) - return rc; rc = ima_add_boot_aggregate(); /* boot aggregate must be first entry */ if (rc != 0) return rc; diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index 1ce8b1701566..03afa67c2e04 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -23,9 +23,6 @@ #define AUDIT_CAUSE_LEN_MAX 32 -/* pre-allocated array of tpm_digest structures to extend a PCR */ -static struct tpm_digest *digests; - LIST_HEAD(ima_measurements); /* list of all measurements */ #ifdef CONFIG_IMA_KEXEC static unsigned long binary_runtime_size; @@ -139,15 +136,11 @@ unsigned long ima_get_binary_runtime_size(void) static int ima_pcr_extend(const u8 *hash, int pcr) { int result = 0; - int i; if (!ima_tpm_chip) return result; - for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) - memcpy(digests[i].digest, hash, TPM_DIGEST_SIZE); - - result = tpm_pcr_extend(ima_tpm_chip, pcr, digests); + result = tpm_pcr_extend(ima_tpm_chip, pcr, hash); if (result != 0) pr_err("Error Communicating to TPM chip, result: %d\n", result); return result; @@ -214,21 +207,3 @@ int ima_restore_measurement_entry(struct ima_template_entry *entry) mutex_unlock(&ima_extend_list_mutex); return result; } - -int __init ima_init_digests(void) -{ - int i; - - if (!ima_tpm_chip) - return 0; - - digests = kcalloc(ima_tpm_chip->nr_allocated_banks, sizeof(*digests), - GFP_NOFS); - if (!digests) - return -ENOMEM; - - for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) - digests[i].alg_id = ima_tpm_chip->allocated_banks[i].alg_id; - - return 0; -} diff --git a/security/keys/trusted.c b/security/keys/trusted.c index 9a94672e7adc..287553039f0f 100644 --- a/security/keys/trusted.c +++ b/security/keys/trusted.c @@ -32,7 +32,6 @@ static const char hmac_alg[] = "hmac(sha1)"; static const char hash_alg[] = "sha1"; static struct tpm_chip *chip; -static struct tpm_digest *digests; struct sdesc { struct shash_desc shash; @@ -386,10 +385,15 @@ EXPORT_SYMBOL_GPL(trusted_tpm_send); */ static int pcrlock(const int pcrnum) { + unsigned char hash[SHA1_DIGEST_SIZE]; + int ret; + if (!capable(CAP_SYS_ADMIN)) return -EPERM; - - return tpm_pcr_extend(chip, pcrnum, digests) ? -EINVAL : 0; + ret = tpm_get_random(chip, hash, SHA1_DIGEST_SIZE); + if (ret != SHA1_DIGEST_SIZE) + return ret; + return tpm_pcr_extend(chip, pcrnum, hash) ? -EINVAL : 0; } /* @@ -1226,29 +1230,6 @@ static int __init trusted_shash_alloc(void) return ret; } -static int __init init_digests(void) -{ - u8 digest[TPM_MAX_DIGEST_SIZE]; - int ret; - int i; - - ret = tpm_get_random(chip, digest, TPM_MAX_DIGEST_SIZE); - if (ret < 0) - return ret; - if (ret < TPM_MAX_DIGEST_SIZE) - return -EFAULT; - - digests = kcalloc(chip->nr_allocated_banks, sizeof(*digests), - GFP_KERNEL); - if (!digests) - return -ENOMEM; - - for (i = 0; i < chip->nr_allocated_banks; i++) - memcpy(digests[i].digest, digest, TPM_MAX_DIGEST_SIZE); - - return 0; -} - static int __init init_trusted(void) { int ret; @@ -1259,21 +1240,15 @@ static int __init init_trusted(void) chip = tpm_default_chip(); if (!chip) return 0; - - ret = init_digests(); - if (ret < 0) - goto err_put; ret = trusted_shash_alloc(); if (ret < 0) - goto err_free; + goto err_put; ret = register_key_type(&key_type_trusted); if (ret < 0) goto err_release; return 0; err_release: trusted_shash_release(); -err_free: - kfree(digests); err_put: put_device(&chip->dev); return ret; @@ -1283,7 +1258,6 @@ static void __exit cleanup_trusted(void) { if (chip) { put_device(&chip->dev); - kfree(digests); trusted_shash_release(); unregister_key_type(&key_type_trusted); }