[v3,5/5] tpm: ensure that output of PCR read contains the correct digest size
diff mbox series

Message ID 20181030154711.2782-6-roberto.sassu@huawei.com
State New
Headers show
Series
  • tpm: retrieve digest size of unknown algorithms from TPM
Related show

Commit Message

Roberto Sassu Oct. 30, 2018, 3:47 p.m. UTC
This patch ensures that the digest size returned by the TPM during a PCR
read matches the size of the algorithm passed as argument to
tpm2_pcr_read(). The check is performed after information about the PCR
banks has been retrieved.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 drivers/char/tpm/tpm2-cmd.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Jarkko Sakkinen Oct. 30, 2018, 7:52 p.m. UTC | #1
On Tue, 30 Oct 2018, Roberto Sassu wrote:
> This patch ensures that the digest size returned by the TPM during a PCR
> read matches the size of the algorithm passed as argument to
> tpm2_pcr_read(). The check is performed after information about the PCR
> banks has been retrieved.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

What is the scenarion when this can happen (should be explained in
the commit message)?

/Jarkko
Roberto Sassu Oct. 31, 2018, 8:16 a.m. UTC | #2
On 10/30/2018 8:52 PM, Jarkko Sakkinen wrote:
> On Tue, 30 Oct 2018, Roberto Sassu wrote:
>> This patch ensures that the digest size returned by the TPM during a PCR
>> read matches the size of the algorithm passed as argument to
>> tpm2_pcr_read(). The check is performed after information about the PCR
>> banks has been retrieved.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> 
> What is the scenarion when this can happen (should be explained in
> the commit message)?

Without an HMAC session, the request/response payload can be modified.
This patch ensures that the digest size in the payload is equal to the
size of the algorithm specified by the caller.

Patch 3/5 only ensures that there is no buffer overflow when data is
copied to the tpm_digest structure passed by the caller.

Patch 5/5 uses the PCR bank information introduced in patch 4/5 to
ensure that the exact amount of data is copied from the response
payload. However, the patch may not help because an attacker can modify
the algorithm in the request payload so that the TPM returns a shorter
digest.

For me it is ok to remove this patch from the set. It was requested by
Mimi.

Roberto
Jarkko Sakkinen Nov. 1, 2018, 2:32 p.m. UTC | #3
On Wed, 31 Oct 2018, Roberto Sassu wrote:
> On 10/30/2018 8:52 PM, Jarkko Sakkinen wrote:
>> On Tue, 30 Oct 2018, Roberto Sassu wrote:
>>> This patch ensures that the digest size returned by the TPM during a PCR
>>> read matches the size of the algorithm passed as argument to
>>> tpm2_pcr_read(). The check is performed after information about the PCR
>>> banks has been retrieved.
>>> 
>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> 
>> What is the scenarion when this can happen (should be explained in
>> the commit message)?
>
> Without an HMAC session, the request/response payload can be modified.
> This patch ensures that the digest size in the payload is equal to the
> size of the algorithm specified by the caller.

i.e. it protect against memory corruption that could happen in the bus?
Please state this.

> For me it is ok to remove this patch from the set. It was requested by
> Mimi.

For me it is not ok remove this patch :-) I just want that note to the
commit message in order to have it documented.

/Jarkko
Mimi Zohar Nov. 1, 2018, 4:52 p.m. UTC | #4
On Tue, 2018-10-30 at 16:47 +0100, Roberto Sassu wrote:
> This patch ensures that the digest size returned by the TPM during a PCR
> read matches the size of the algorithm passed as argument to
> tpm2_pcr_read(). The check is performed after information about the PCR
> banks has been retrieved.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  drivers/char/tpm/tpm2-cmd.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 8e821e7b4674..477dcc30fc53 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -187,15 +187,28 @@ struct tpm2_pcr_read_out {
>  int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
>  		  struct tpm_digest *digest_struct, u16 *digest_size_ptr)
>  {
> +	int i;
>  	int rc;
>  	struct tpm_buf buf;
>  	struct tpm2_pcr_read_out *out;
>  	u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
>  	u16 digest_size;
> +	u16 expected_digest_size = 0;
>  
>  	if (pcr_idx >= TPM2_PLATFORM_PCR)
>  		return -EINVAL;
>  
> +	if (!digest_size_ptr) {
> +		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
> +		     chip->active_banks[i].alg_id != digest_struct->alg_id; i++)
> +			;
> +
> +		if (i == ARRAY_SIZE(chip->active_banks))
> +			return -EINVAL;
> +
> +		expected_digest_size = chip->active_banks[i].digest_size;
> +	}
> +
>  	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
>  	if (rc)
>  		return rc;
> @@ -215,7 +228,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
>  
>  	out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
>  	digest_size = be16_to_cpu(out->digest_size);
> -	if (digest_size > sizeof(digest_struct->digest)) {
> +	if ((digest_size_ptr && digest_size > sizeof(digest_struct->digest)) ||

The returned digest size should never be larger than the structure
field.  The digest_size_ptr test is unnecessary.

Mimi

> +	    (!digest_size_ptr && digest_size != expected_digest_size)) {
>  		rc = -EINVAL;
>  		goto out;
>  	}

Patch
diff mbox series

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 8e821e7b4674..477dcc30fc53 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -187,15 +187,28 @@  struct tpm2_pcr_read_out {
 int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
 		  struct tpm_digest *digest_struct, u16 *digest_size_ptr)
 {
+	int i;
 	int rc;
 	struct tpm_buf buf;
 	struct tpm2_pcr_read_out *out;
 	u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
 	u16 digest_size;
+	u16 expected_digest_size = 0;
 
 	if (pcr_idx >= TPM2_PLATFORM_PCR)
 		return -EINVAL;
 
+	if (!digest_size_ptr) {
+		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
+		     chip->active_banks[i].alg_id != digest_struct->alg_id; i++)
+			;
+
+		if (i == ARRAY_SIZE(chip->active_banks))
+			return -EINVAL;
+
+		expected_digest_size = chip->active_banks[i].digest_size;
+	}
+
 	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
 	if (rc)
 		return rc;
@@ -215,7 +228,8 @@  int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
 
 	out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
 	digest_size = be16_to_cpu(out->digest_size);
-	if (digest_size > sizeof(digest_struct->digest)) {
+	if ((digest_size_ptr && digest_size > sizeof(digest_struct->digest)) ||
+	    (!digest_size_ptr && digest_size != expected_digest_size)) {
 		rc = -EINVAL;
 		goto out;
 	}