From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mimi Zohar Date: Tue, 27 Jun 2017 15:24:18 +0000 Subject: Re: [tpmdd-devel] [PATCH v3 3/6] tpm: introduce tpm_pcr_bank_info structure with digest_size from TP Message-Id: <1498577058.3387.103.camel@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit List-Id: References: <20170621142941.32674-1-roberto.sassu@huawei.com> <20170621142941.32674-4-roberto.sassu@huawei.com> In-Reply-To: <20170621142941.32674-4-roberto.sassu@huawei.com> To: linux-security-module@vger.kernel.org On Wed, 2017-06-21 at 16:29 +0200, Roberto Sassu wrote: > This patch introduces the new structure tpm_pcr_bank_info to store > information regarding PCR banks. The next patch will replace the array of > TPM algorithms IDs with an array of the new structure. > > tpm_pcr_bank_info contains the TPM algorithm ID, the digest size and, > optionally, the corresponding crypto ID, if a mapping exists. These > information will be used by IMA to calculate the digest of an event > and to provide measurements logs to userspace applications. The new > structure has been defined in include/linux/tpm.h, as it will be passed > to functions outside the TPM driver. > > The purpose of this patch is to fix a serious issue in tpm2_pcr_extend(): > if the mapping between a TPM algorithm and a crypto algorithm is not > defined, the PCR bank with the unknown algorithm is not extended. > This gives the opportunity to an attacker to reply to remote attestation > requests with a list of fake measurements. Instead, the digest size > is retrieved from the output buffer of a PCR read, without relying > on the crypto subsystem. > > Signed-off-by: Roberto Sassu To address some of Jarkko's comments, I would replace active_banks with a structure, named for example active_bank_info, containing the existing active_bank alg_id and just the digest size.  A subsequent patch would include the crypto_id, assuming it is needed. When a patch defines a new field/function, at least one usage of that field/function should be included in the patch.  In this case, you might want to verify the digest size returned by the TPM is as expected, comparing it with the crypto digest size. Mimi > --- > drivers/char/tpm/tpm.h | 11 ----------- > drivers/char/tpm/tpm2-cmd.c | 30 ++++++++++++++++++++++++++++++ > include/linux/tpm.h | 19 +++++++++++++++++++ > 3 files changed, 49 insertions(+), 11 deletions(-) > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 1df0521..62c600d 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -98,17 +98,6 @@ enum tpm2_return_codes { > TPM2_RC_REFERENCE_H0 = 0x0910, > }; > > -enum tpm2_algorithms { > - TPM2_ALG_ERROR = 0x0000, > - TPM2_ALG_SHA1 = 0x0004, > - TPM2_ALG_KEYEDHASH = 0x0008, > - TPM2_ALG_SHA256 = 0x000B, > - TPM2_ALG_SHA384 = 0x000C, > - TPM2_ALG_SHA512 = 0x000D, > - TPM2_ALG_NULL = 0x0010, > - TPM2_ALG_SM3_256 = 0x0012, > -}; > - > enum tpm2_command_codes { > TPM2_CC_FIRST = 0x011F, > TPM2_CC_SELF_TEST = 0x0143, > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index 6a9fe0d..74a68ea 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -992,6 +992,36 @@ int tpm2_probe(struct tpm_chip *chip) > } > EXPORT_SYMBOL_GPL(tpm2_probe); > > +static int tpm2_init_pcr_bank_info(struct tpm_chip *chip, u16 alg_id, > + struct tpm_pcr_bank_info *active_bank) > +{ > + struct tpm_buf buf; > + struct tpm2_pcr_read_out *pcrread_out; > + int rc = 0; > + int i; > + > + active_bank->alg_id = alg_id; > + > + rc = tpm2_pcr_read_tpm_buf(chip, 0, alg_id, &buf, NULL); > + if (rc) > + goto out; > + > + pcrread_out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE]; > + > + active_bank->digest_size = be16_to_cpu(pcrread_out->digest_size); > + active_bank->crypto_id = HASH_ALGO__LAST; > + > + for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) { > + if (active_bank->alg_id != tpm2_hash_map[i].tpm_id) > + continue; > + > + active_bank->crypto_id = tpm2_hash_map[i].crypto_id; > + } > +out: > + tpm_buf_destroy(&buf); > + return rc; > +} > + > struct tpm2_pcr_selection { > __be16 hash_alg; > u8 size_of_select; > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > index 5a090f5..ff06738 100644 > --- a/include/linux/tpm.h > +++ b/include/linux/tpm.h > @@ -22,6 +22,8 @@ > #ifndef __LINUX_TPM_H__ > #define __LINUX_TPM_H__ > > +#include > + > #define TPM_DIGEST_SIZE 20 /* Max TPM v1.2 PCR size */ > > /* > @@ -37,6 +39,17 @@ enum TPM_OPS_FLAGS { > TPM_OPS_AUTO_STARTUP = BIT(0), > }; > > +enum tpm2_algorithms { > + TPM2_ALG_ERROR = 0x0000, > + TPM2_ALG_SHA1 = 0x0004, > + TPM2_ALG_KEYEDHASH = 0x0008, > + TPM2_ALG_SHA256 = 0x000B, > + TPM2_ALG_SHA384 = 0x000C, > + TPM2_ALG_SHA512 = 0x000D, > + TPM2_ALG_NULL = 0x0010, > + TPM2_ALG_SM3_256 = 0x0012, > +}; > + > struct tpm_class_ops { > unsigned int flags; > const u8 req_complete_mask; > @@ -52,6 +65,12 @@ struct tpm_class_ops { > void (*relinquish_locality)(struct tpm_chip *chip, int loc); > }; > > +struct tpm_pcr_bank_info { > + enum tpm2_algorithms alg_id; > + enum hash_algo crypto_id; > + u32 digest_size; > +}; > + > #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE) > > extern int tpm_is_tpm2(u32 chip_num); From mboxrd@z Thu Jan 1 00:00:00 1970 From: zohar@linux.vnet.ibm.com (Mimi Zohar) Date: Tue, 27 Jun 2017 11:24:18 -0400 Subject: [tpmdd-devel] [PATCH v3 3/6] tpm: introduce tpm_pcr_bank_info structure with digest_size from TPM In-Reply-To: <20170621142941.32674-4-roberto.sassu@huawei.com> References: <20170621142941.32674-1-roberto.sassu@huawei.com> <20170621142941.32674-4-roberto.sassu@huawei.com> Message-ID: <1498577058.3387.103.camel@linux.vnet.ibm.com> To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On Wed, 2017-06-21 at 16:29 +0200, Roberto Sassu wrote: > This patch introduces the new structure tpm_pcr_bank_info to store > information regarding PCR banks. The next patch will replace the array of > TPM algorithms IDs with an array of the new structure. > > tpm_pcr_bank_info contains the TPM algorithm ID, the digest size and, > optionally, the corresponding crypto ID, if a mapping exists. These > information will be used by IMA to calculate the digest of an event > and to provide measurements logs to userspace applications. The new > structure has been defined in include/linux/tpm.h, as it will be passed > to functions outside the TPM driver. > > The purpose of this patch is to fix a serious issue in tpm2_pcr_extend(): > if the mapping between a TPM algorithm and a crypto algorithm is not > defined, the PCR bank with the unknown algorithm is not extended. > This gives the opportunity to an attacker to reply to remote attestation > requests with a list of fake measurements. Instead, the digest size > is retrieved from the output buffer of a PCR read, without relying > on the crypto subsystem. > > Signed-off-by: Roberto Sassu To address some of Jarkko's comments, I would replace active_banks with a structure, named for example active_bank_info, containing the existing active_bank alg_id and just the digest size. ?A subsequent patch would include the crypto_id, assuming it is needed. When a patch defines a new field/function, at least one usage of that field/function should be included in the patch. ?In this case, you might want to verify the digest size returned by the TPM is as expected, comparing it with the crypto digest size. Mimi > --- > drivers/char/tpm/tpm.h | 11 ----------- > drivers/char/tpm/tpm2-cmd.c | 30 ++++++++++++++++++++++++++++++ > include/linux/tpm.h | 19 +++++++++++++++++++ > 3 files changed, 49 insertions(+), 11 deletions(-) > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 1df0521..62c600d 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -98,17 +98,6 @@ enum tpm2_return_codes { > TPM2_RC_REFERENCE_H0 = 0x0910, > }; > > -enum tpm2_algorithms { > - TPM2_ALG_ERROR = 0x0000, > - TPM2_ALG_SHA1 = 0x0004, > - TPM2_ALG_KEYEDHASH = 0x0008, > - TPM2_ALG_SHA256 = 0x000B, > - TPM2_ALG_SHA384 = 0x000C, > - TPM2_ALG_SHA512 = 0x000D, > - TPM2_ALG_NULL = 0x0010, > - TPM2_ALG_SM3_256 = 0x0012, > -}; > - > enum tpm2_command_codes { > TPM2_CC_FIRST = 0x011F, > TPM2_CC_SELF_TEST = 0x0143, > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index 6a9fe0d..74a68ea 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -992,6 +992,36 @@ int tpm2_probe(struct tpm_chip *chip) > } > EXPORT_SYMBOL_GPL(tpm2_probe); > > +static int tpm2_init_pcr_bank_info(struct tpm_chip *chip, u16 alg_id, > + struct tpm_pcr_bank_info *active_bank) > +{ > + struct tpm_buf buf; > + struct tpm2_pcr_read_out *pcrread_out; > + int rc = 0; > + int i; > + > + active_bank->alg_id = alg_id; > + > + rc = tpm2_pcr_read_tpm_buf(chip, 0, alg_id, &buf, NULL); > + if (rc) > + goto out; > + > + pcrread_out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE]; > + > + active_bank->digest_size = be16_to_cpu(pcrread_out->digest_size); > + active_bank->crypto_id = HASH_ALGO__LAST; > + > + for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) { > + if (active_bank->alg_id != tpm2_hash_map[i].tpm_id) > + continue; > + > + active_bank->crypto_id = tpm2_hash_map[i].crypto_id; > + } > +out: > + tpm_buf_destroy(&buf); > + return rc; > +} > + > struct tpm2_pcr_selection { > __be16 hash_alg; > u8 size_of_select; > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > index 5a090f5..ff06738 100644 > --- a/include/linux/tpm.h > +++ b/include/linux/tpm.h > @@ -22,6 +22,8 @@ > #ifndef __LINUX_TPM_H__ > #define __LINUX_TPM_H__ > > +#include > + > #define TPM_DIGEST_SIZE 20 /* Max TPM v1.2 PCR size */ > > /* > @@ -37,6 +39,17 @@ enum TPM_OPS_FLAGS { > TPM_OPS_AUTO_STARTUP = BIT(0), > }; > > +enum tpm2_algorithms { > + TPM2_ALG_ERROR = 0x0000, > + TPM2_ALG_SHA1 = 0x0004, > + TPM2_ALG_KEYEDHASH = 0x0008, > + TPM2_ALG_SHA256 = 0x000B, > + TPM2_ALG_SHA384 = 0x000C, > + TPM2_ALG_SHA512 = 0x000D, > + TPM2_ALG_NULL = 0x0010, > + TPM2_ALG_SM3_256 = 0x0012, > +}; > + > struct tpm_class_ops { > unsigned int flags; > const u8 req_complete_mask; > @@ -52,6 +65,12 @@ struct tpm_class_ops { > void (*relinquish_locality)(struct tpm_chip *chip, int loc); > }; > > +struct tpm_pcr_bank_info { > + enum tpm2_algorithms alg_id; > + enum hash_algo crypto_id; > + u32 digest_size; > +}; > + > #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE) > > extern int tpm_is_tpm2(u32 chip_num); -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mimi Zohar Subject: Re: [PATCH v3 3/6] tpm: introduce tpm_pcr_bank_info structure with digest_size from TPM Date: Tue, 27 Jun 2017 11:24:18 -0400 Message-ID: <1498577058.3387.103.camel@linux.vnet.ibm.com> References: <20170621142941.32674-1-roberto.sassu@huawei.com> <20170621142941.32674-4-roberto.sassu@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20170621142941.32674-4-roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: tpmdd-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: Roberto Sassu , tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Cc: linux-ima-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, linux-security-module-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, keyrings-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: tpmdd-devel@lists.sourceforge.net T24gV2VkLCAyMDE3LTA2LTIxIGF0IDE2OjI5ICswMjAwLCBSb2JlcnRvIFNhc3N1IHdyb3RlOgo+ IFRoaXMgcGF0Y2ggaW50cm9kdWNlcyB0aGUgbmV3IHN0cnVjdHVyZSB0cG1fcGNyX2JhbmtfaW5m byB0byBzdG9yZQo+IGluZm9ybWF0aW9uIHJlZ2FyZGluZyBQQ1IgYmFua3MuIFRoZSBuZXh0IHBh dGNoIHdpbGwgcmVwbGFjZSB0aGUgYXJyYXkgb2YKPiBUUE0gYWxnb3JpdGhtcyBJRHMgd2l0aCBh biBhcnJheSBvZiB0aGUgbmV3IHN0cnVjdHVyZS4KPiAKPiB0cG1fcGNyX2JhbmtfaW5mbyBjb250 YWlucyB0aGUgVFBNIGFsZ29yaXRobSBJRCwgdGhlIGRpZ2VzdCBzaXplIGFuZCwKPiBvcHRpb25h bGx5LCB0aGUgY29ycmVzcG9uZGluZyBjcnlwdG8gSUQsIGlmIGEgbWFwcGluZyBleGlzdHMuIFRo ZXNlCj4gaW5mb3JtYXRpb24gd2lsbCBiZSB1c2VkIGJ5IElNQSB0byBjYWxjdWxhdGUgdGhlIGRp Z2VzdCBvZiBhbiBldmVudAo+IGFuZCB0byBwcm92aWRlIG1lYXN1cmVtZW50cyBsb2dzIHRvIHVz ZXJzcGFjZSBhcHBsaWNhdGlvbnMuIFRoZSBuZXcKPiBzdHJ1Y3R1cmUgaGFzIGJlZW4gZGVmaW5l ZCBpbiBpbmNsdWRlL2xpbnV4L3RwbS5oLCBhcyBpdCB3aWxsIGJlIHBhc3NlZAo+IHRvIGZ1bmN0 aW9ucyBvdXRzaWRlIHRoZSBUUE0gZHJpdmVyLgo+IAo+IFRoZSBwdXJwb3NlIG9mIHRoaXMgcGF0 Y2ggaXMgdG8gZml4IGEgc2VyaW91cyBpc3N1ZSBpbiB0cG0yX3Bjcl9leHRlbmQoKToKPiBpZiB0 aGUgbWFwcGluZyBiZXR3ZWVuIGEgVFBNIGFsZ29yaXRobSBhbmQgYSBjcnlwdG8gYWxnb3JpdGht IGlzIG5vdAo+IGRlZmluZWQsIHRoZSBQQ1IgYmFuayB3aXRoIHRoZSB1bmtub3duIGFsZ29yaXRo bSBpcyBub3QgZXh0ZW5kZWQuCj4gVGhpcyBnaXZlcyB0aGUgb3Bwb3J0dW5pdHkgdG8gYW4gYXR0 YWNrZXIgdG8gcmVwbHkgdG8gcmVtb3RlIGF0dGVzdGF0aW9uCj4gcmVxdWVzdHMgd2l0aCBhIGxp c3Qgb2YgZmFrZSBtZWFzdXJlbWVudHMuIEluc3RlYWQsIHRoZSBkaWdlc3Qgc2l6ZQo+IGlzIHJl dHJpZXZlZCBmcm9tIHRoZSBvdXRwdXQgYnVmZmVyIG9mIGEgUENSIHJlYWQsIHdpdGhvdXQgcmVs eWluZwo+IG9uIHRoZSBjcnlwdG8gc3Vic3lzdGVtLgo+IAo+IFNpZ25lZC1vZmYtYnk6IFJvYmVy dG8gU2Fzc3UgPHJvYmVydG8uc2Fzc3VAaHVhd2VpLmNvbT4KClRvIGFkZHJlc3Mgc29tZSBvZiBK YXJra28ncyBjb21tZW50cywgSSB3b3VsZCByZXBsYWNlIGFjdGl2ZV9iYW5rcwp3aXRoIGEgc3Ry dWN0dXJlLCBuYW1lZCBmb3IgZXhhbXBsZSBhY3RpdmVfYmFua19pbmZvLCBjb250YWluaW5nIHRo ZQpleGlzdGluZyBhY3RpdmVfYmFuayBhbGdfaWQgYW5kIGp1c3QgdGhlIGRpZ2VzdCBzaXplLiDC oEEgc3Vic2VxdWVudApwYXRjaCB3b3VsZCBpbmNsdWRlIHRoZSBjcnlwdG9faWQsIGFzc3VtaW5n IGl0IGlzIG5lZWRlZC4KCldoZW4gYSBwYXRjaCBkZWZpbmVzIGEgbmV3IGZpZWxkL2Z1bmN0aW9u LCBhdCBsZWFzdCBvbmUgdXNhZ2Ugb2YgdGhhdApmaWVsZC9mdW5jdGlvbiBzaG91bGQgYmUgaW5j bHVkZWQgaW4gdGhlIHBhdGNoLiDCoEluIHRoaXMgY2FzZSwgeW91Cm1pZ2h0IHdhbnQgdG8gdmVy aWZ5IHRoZSBkaWdlc3Qgc2l6ZSByZXR1cm5lZCBieSB0aGUgVFBNIGlzIGFzCmV4cGVjdGVkLCBj b21wYXJpbmcgaXQgd2l0aCB0aGUgY3J5cHRvIGRpZ2VzdCBzaXplLgoKTWltaQoKPiAtLS0KPiAg ZHJpdmVycy9jaGFyL3RwbS90cG0uaCAgICAgIHwgMTEgLS0tLS0tLS0tLS0KPiAgZHJpdmVycy9j aGFyL3RwbS90cG0yLWNtZC5jIHwgMzAgKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrCj4g IGluY2x1ZGUvbGludXgvdHBtLmggICAgICAgICB8IDE5ICsrKysrKysrKysrKysrKysrKysKPiAg MyBmaWxlcyBjaGFuZ2VkLCA0OSBpbnNlcnRpb25zKCspLCAxMSBkZWxldGlvbnMoLSkKPiAKPiBk aWZmIC0tZ2l0IGEvZHJpdmVycy9jaGFyL3RwbS90cG0uaCBiL2RyaXZlcnMvY2hhci90cG0vdHBt LmgKPiBpbmRleCAxZGYwNTIxLi42MmM2MDBkIDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMvY2hhci90 cG0vdHBtLmgKPiArKysgYi9kcml2ZXJzL2NoYXIvdHBtL3RwbS5oCj4gQEAgLTk4LDE3ICs5OCw2 IEBAIGVudW0gdHBtMl9yZXR1cm5fY29kZXMgewo+ICAJVFBNMl9SQ19SRUZFUkVOQ0VfSDAJPSAw eDA5MTAsCj4gIH07Cj4gCj4gLWVudW0gdHBtMl9hbGdvcml0aG1zIHsKPiAtCVRQTTJfQUxHX0VS Uk9SCQk9IDB4MDAwMCwKPiAtCVRQTTJfQUxHX1NIQTEJCT0gMHgwMDA0LAo+IC0JVFBNMl9BTEdf S0VZRURIQVNICT0gMHgwMDA4LAo+IC0JVFBNMl9BTEdfU0hBMjU2CQk9IDB4MDAwQiwKPiAtCVRQ TTJfQUxHX1NIQTM4NAkJPSAweDAwMEMsCj4gLQlUUE0yX0FMR19TSEE1MTIJCT0gMHgwMDBELAo+ IC0JVFBNMl9BTEdfTlVMTAkJPSAweDAwMTAsCj4gLQlUUE0yX0FMR19TTTNfMjU2CT0gMHgwMDEy LAo+IC19Owo+IC0KPiAgZW51bSB0cG0yX2NvbW1hbmRfY29kZXMgewo+ICAJVFBNMl9DQ19GSVJT VAkJPSAweDAxMUYsCj4gIAlUUE0yX0NDX1NFTEZfVEVTVAk9IDB4MDE0MywKPiBkaWZmIC0tZ2l0 IGEvZHJpdmVycy9jaGFyL3RwbS90cG0yLWNtZC5jIGIvZHJpdmVycy9jaGFyL3RwbS90cG0yLWNt ZC5jCj4gaW5kZXggNmE5ZmUwZC4uNzRhNjhlYSAxMDA2NDQKPiAtLS0gYS9kcml2ZXJzL2NoYXIv dHBtL3RwbTItY21kLmMKPiArKysgYi9kcml2ZXJzL2NoYXIvdHBtL3RwbTItY21kLmMKPiBAQCAt OTkyLDYgKzk5MiwzNiBAQCBpbnQgdHBtMl9wcm9iZShzdHJ1Y3QgdHBtX2NoaXAgKmNoaXApCj4g IH0KPiAgRVhQT1JUX1NZTUJPTF9HUEwodHBtMl9wcm9iZSk7Cj4gCj4gK3N0YXRpYyBpbnQgdHBt Ml9pbml0X3Bjcl9iYW5rX2luZm8oc3RydWN0IHRwbV9jaGlwICpjaGlwLCB1MTYgYWxnX2lkLAo+ ICsJCQkJICAgc3RydWN0IHRwbV9wY3JfYmFua19pbmZvICphY3RpdmVfYmFuaykKPiArewo+ICsJ c3RydWN0IHRwbV9idWYgYnVmOwo+ICsJc3RydWN0IHRwbTJfcGNyX3JlYWRfb3V0ICpwY3JyZWFk X291dDsKPiArCWludCByYyA9IDA7Cj4gKwlpbnQgaTsKPiArCj4gKwlhY3RpdmVfYmFuay0+YWxn X2lkID0gYWxnX2lkOwo+ICsKPiArCXJjID0gdHBtMl9wY3JfcmVhZF90cG1fYnVmKGNoaXAsIDAs IGFsZ19pZCwgJmJ1ZiwgTlVMTCk7Cj4gKwlpZiAocmMpCj4gKwkJZ290byBvdXQ7Cj4gKwo+ICsJ cGNycmVhZF9vdXQgPSAoc3RydWN0IHRwbTJfcGNyX3JlYWRfb3V0ICopJmJ1Zi5kYXRhW1RQTV9I RUFERVJfU0laRV07Cj4gKwo+ICsJYWN0aXZlX2JhbmstPmRpZ2VzdF9zaXplID0gYmUxNl90b19j cHUocGNycmVhZF9vdXQtPmRpZ2VzdF9zaXplKTsKPiArCWFjdGl2ZV9iYW5rLT5jcnlwdG9faWQg PSBIQVNIX0FMR09fX0xBU1Q7Cj4gKwo+ICsJZm9yIChpID0gMDsgaSA8IEFSUkFZX1NJWkUodHBt Ml9oYXNoX21hcCk7IGkrKykgewo+ICsJCWlmIChhY3RpdmVfYmFuay0+YWxnX2lkICE9IHRwbTJf aGFzaF9tYXBbaV0udHBtX2lkKQo+ICsJCQljb250aW51ZTsKPiArCj4gKwkJYWN0aXZlX2Jhbmst PmNyeXB0b19pZCA9IHRwbTJfaGFzaF9tYXBbaV0uY3J5cHRvX2lkOwo+ICsJfQo+ICtvdXQ6Cj4g Kwl0cG1fYnVmX2Rlc3Ryb3koJmJ1Zik7Cj4gKwlyZXR1cm4gcmM7Cj4gK30KPiArCj4gIHN0cnVj dCB0cG0yX3Bjcl9zZWxlY3Rpb24gewo+ICAJX19iZTE2ICBoYXNoX2FsZzsKPiAgCXU4ICBzaXpl X29mX3NlbGVjdDsKPiBkaWZmIC0tZ2l0IGEvaW5jbHVkZS9saW51eC90cG0uaCBiL2luY2x1ZGUv bGludXgvdHBtLmgKPiBpbmRleCA1YTA5MGY1Li5mZjA2NzM4IDEwMDY0NAo+IC0tLSBhL2luY2x1 ZGUvbGludXgvdHBtLmgKPiArKysgYi9pbmNsdWRlL2xpbnV4L3RwbS5oCj4gQEAgLTIyLDYgKzIy LDggQEAKPiAgI2lmbmRlZiBfX0xJTlVYX1RQTV9IX18KPiAgI2RlZmluZSBfX0xJTlVYX1RQTV9I X18KPiAKPiArI2luY2x1ZGUgPGNyeXB0by9oYXNoX2luZm8uaD4KPiArCj4gICNkZWZpbmUgVFBN X0RJR0VTVF9TSVpFIDIwCS8qIE1heCBUUE0gdjEuMiBQQ1Igc2l6ZSAqLwo+IAo+ICAvKgo+IEBA IC0zNyw2ICszOSwxNyBAQCBlbnVtIFRQTV9PUFNfRkxBR1Mgewo+ICAJVFBNX09QU19BVVRPX1NU QVJUVVAgPSBCSVQoMCksCj4gIH07Cj4gCj4gK2VudW0gdHBtMl9hbGdvcml0aG1zIHsKPiArCVRQ TTJfQUxHX0VSUk9SCQk9IDB4MDAwMCwKPiArCVRQTTJfQUxHX1NIQTEJCT0gMHgwMDA0LAo+ICsJ VFBNMl9BTEdfS0VZRURIQVNICT0gMHgwMDA4LAo+ICsJVFBNMl9BTEdfU0hBMjU2CQk9IDB4MDAw QiwKPiArCVRQTTJfQUxHX1NIQTM4NAkJPSAweDAwMEMsCj4gKwlUUE0yX0FMR19TSEE1MTIJCT0g MHgwMDBELAo+ICsJVFBNMl9BTEdfTlVMTAkJPSAweDAwMTAsCj4gKwlUUE0yX0FMR19TTTNfMjU2 CT0gMHgwMDEyLAo+ICt9Owo+ICsKPiAgc3RydWN0IHRwbV9jbGFzc19vcHMgewo+ICAJdW5zaWdu ZWQgaW50IGZsYWdzOwo+ICAJY29uc3QgdTggcmVxX2NvbXBsZXRlX21hc2s7Cj4gQEAgLTUyLDYg KzY1LDEyIEBAIHN0cnVjdCB0cG1fY2xhc3Nfb3BzIHsKPiAgCXZvaWQgKCpyZWxpbnF1aXNoX2xv Y2FsaXR5KShzdHJ1Y3QgdHBtX2NoaXAgKmNoaXAsIGludCBsb2MpOwo+ICB9Owo+IAo+ICtzdHJ1 Y3QgdHBtX3Bjcl9iYW5rX2luZm8gewo+ICsJZW51bSB0cG0yX2FsZ29yaXRobXMgYWxnX2lkOwo+ ICsJZW51bSBoYXNoX2FsZ28gY3J5cHRvX2lkOwo+ICsJdTMyIGRpZ2VzdF9zaXplOwo+ICt9Owo+ ICsKPiAgI2lmIGRlZmluZWQoQ09ORklHX1RDR19UUE0pIHx8IGRlZmluZWQoQ09ORklHX1RDR19U UE1fTU9EVUxFKQo+IAo+ICBleHRlcm4gaW50IHRwbV9pc190cG0yKHUzMiBjaGlwX251bSk7CgoK LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tCkNoZWNrIG91dCB0aGUgdmlicmFudCB0ZWNoIGNvbW11bml0 eSBvbiBvbmUgb2YgdGhlIHdvcmxkJ3MgbW9zdAplbmdhZ2luZyB0ZWNoIHNpdGVzLCBTbGFzaGRv dC5vcmchIGh0dHA6Ly9zZG0ubGluay9zbGFzaGRvdApfX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fXwp0cG1kZC1kZXZlbCBtYWlsaW5nIGxpc3QKdHBtZGQtZGV2 ZWxAbGlzdHMuc291cmNlZm9yZ2UubmV0Cmh0dHBzOi8vbGlzdHMuc291cmNlZm9yZ2UubmV0L2xp c3RzL2xpc3RpbmZvL3RwbWRkLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753264AbdF0PZB (ORCPT ); Tue, 27 Jun 2017 11:25:01 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:56768 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752111AbdF0PYh (ORCPT ); Tue, 27 Jun 2017 11:24:37 -0400 Subject: Re: [tpmdd-devel] [PATCH v3 3/6] tpm: introduce tpm_pcr_bank_info structure with digest_size from TPM From: Mimi Zohar To: Roberto Sassu , tpmdd-devel@lists.sourceforge.net Cc: linux-ima-devel@lists.sourceforge.net, linux-security-module@vger.kernel.org, keyrings@vger.kernel.org, linux-kernel@vger.kernel.org Date: Tue, 27 Jun 2017 11:24:18 -0400 In-Reply-To: <20170621142941.32674-4-roberto.sassu@huawei.com> References: <20170621142941.32674-1-roberto.sassu@huawei.com> <20170621142941.32674-4-roberto.sassu@huawei.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.5 (3.20.5-1.fc24) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-TM-AS-MML: disable x-cbid: 17062715-0004-0000-0000-0000021EE9FA X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17062715-0005-0000-0000-00005E02BDED Message-Id: <1498577058.3387.103.camel@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-06-27_09:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1706270249 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2017-06-21 at 16:29 +0200, Roberto Sassu wrote: > This patch introduces the new structure tpm_pcr_bank_info to store > information regarding PCR banks. The next patch will replace the array of > TPM algorithms IDs with an array of the new structure. > > tpm_pcr_bank_info contains the TPM algorithm ID, the digest size and, > optionally, the corresponding crypto ID, if a mapping exists. These > information will be used by IMA to calculate the digest of an event > and to provide measurements logs to userspace applications. The new > structure has been defined in include/linux/tpm.h, as it will be passed > to functions outside the TPM driver. > > The purpose of this patch is to fix a serious issue in tpm2_pcr_extend(): > if the mapping between a TPM algorithm and a crypto algorithm is not > defined, the PCR bank with the unknown algorithm is not extended. > This gives the opportunity to an attacker to reply to remote attestation > requests with a list of fake measurements. Instead, the digest size > is retrieved from the output buffer of a PCR read, without relying > on the crypto subsystem. > > Signed-off-by: Roberto Sassu To address some of Jarkko's comments, I would replace active_banks with a structure, named for example active_bank_info, containing the existing active_bank alg_id and just the digest size.  A subsequent patch would include the crypto_id, assuming it is needed. When a patch defines a new field/function, at least one usage of that field/function should be included in the patch.  In this case, you might want to verify the digest size returned by the TPM is as expected, comparing it with the crypto digest size. Mimi > --- > drivers/char/tpm/tpm.h | 11 ----------- > drivers/char/tpm/tpm2-cmd.c | 30 ++++++++++++++++++++++++++++++ > include/linux/tpm.h | 19 +++++++++++++++++++ > 3 files changed, 49 insertions(+), 11 deletions(-) > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 1df0521..62c600d 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -98,17 +98,6 @@ enum tpm2_return_codes { > TPM2_RC_REFERENCE_H0 = 0x0910, > }; > > -enum tpm2_algorithms { > - TPM2_ALG_ERROR = 0x0000, > - TPM2_ALG_SHA1 = 0x0004, > - TPM2_ALG_KEYEDHASH = 0x0008, > - TPM2_ALG_SHA256 = 0x000B, > - TPM2_ALG_SHA384 = 0x000C, > - TPM2_ALG_SHA512 = 0x000D, > - TPM2_ALG_NULL = 0x0010, > - TPM2_ALG_SM3_256 = 0x0012, > -}; > - > enum tpm2_command_codes { > TPM2_CC_FIRST = 0x011F, > TPM2_CC_SELF_TEST = 0x0143, > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index 6a9fe0d..74a68ea 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -992,6 +992,36 @@ int tpm2_probe(struct tpm_chip *chip) > } > EXPORT_SYMBOL_GPL(tpm2_probe); > > +static int tpm2_init_pcr_bank_info(struct tpm_chip *chip, u16 alg_id, > + struct tpm_pcr_bank_info *active_bank) > +{ > + struct tpm_buf buf; > + struct tpm2_pcr_read_out *pcrread_out; > + int rc = 0; > + int i; > + > + active_bank->alg_id = alg_id; > + > + rc = tpm2_pcr_read_tpm_buf(chip, 0, alg_id, &buf, NULL); > + if (rc) > + goto out; > + > + pcrread_out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE]; > + > + active_bank->digest_size = be16_to_cpu(pcrread_out->digest_size); > + active_bank->crypto_id = HASH_ALGO__LAST; > + > + for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) { > + if (active_bank->alg_id != tpm2_hash_map[i].tpm_id) > + continue; > + > + active_bank->crypto_id = tpm2_hash_map[i].crypto_id; > + } > +out: > + tpm_buf_destroy(&buf); > + return rc; > +} > + > struct tpm2_pcr_selection { > __be16 hash_alg; > u8 size_of_select; > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > index 5a090f5..ff06738 100644 > --- a/include/linux/tpm.h > +++ b/include/linux/tpm.h > @@ -22,6 +22,8 @@ > #ifndef __LINUX_TPM_H__ > #define __LINUX_TPM_H__ > > +#include > + > #define TPM_DIGEST_SIZE 20 /* Max TPM v1.2 PCR size */ > > /* > @@ -37,6 +39,17 @@ enum TPM_OPS_FLAGS { > TPM_OPS_AUTO_STARTUP = BIT(0), > }; > > +enum tpm2_algorithms { > + TPM2_ALG_ERROR = 0x0000, > + TPM2_ALG_SHA1 = 0x0004, > + TPM2_ALG_KEYEDHASH = 0x0008, > + TPM2_ALG_SHA256 = 0x000B, > + TPM2_ALG_SHA384 = 0x000C, > + TPM2_ALG_SHA512 = 0x000D, > + TPM2_ALG_NULL = 0x0010, > + TPM2_ALG_SM3_256 = 0x0012, > +}; > + > struct tpm_class_ops { > unsigned int flags; > const u8 req_complete_mask; > @@ -52,6 +65,12 @@ struct tpm_class_ops { > void (*relinquish_locality)(struct tpm_chip *chip, int loc); > }; > > +struct tpm_pcr_bank_info { > + enum tpm2_algorithms alg_id; > + enum hash_algo crypto_id; > + u32 digest_size; > +}; > + > #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE) > > extern int tpm_is_tpm2(u32 chip_num);