[PATCH v3 0/5] tpm: retrieve digest size of unknown algorithms from TPM

[PATCH v3 0/5] tpm: retrieve digest size of unknown algorithms from TPM

[Roberto Sassu]

The TPM driver currently relies on the crypto subsystem to determine the
digest size of supported TPM algorithms. In the future, TPM vendors might
implement new algorithms in their chips, and those algorithms might not
be supported by the crypto subsystem.

Usually, vendors provide patches for the new hardware, and likely
the crypto subsystem will be updated before the new algorithm is
introduced. However, old kernels might be updated later, after patches
are included in the mainline kernel. This would leave the opportunity
for attackers to misuse PCRs, as PCR banks with an unknown algorithm
are not extended.

This patch set provides a long term solution for this issue. If a TPM
algorithm is not known by the crypto subsystem, the TPM driver retrieves
the digest size from the TPM with a PCR read. All the PCR banks are
extended, even if the algorithm is not yet supported by the crypto
subsystem.

PCR bank information (TPM algorithm ID, digest size, crypto subsystem ID)
is stored in the tpm_chip structure and available for users of the TPM
driver.

Changelog

v2:
- change the end marker of the active_banks array
- check digest size from output of PCR read command
- remove count parameter from tpm_pcr_read() and tpm2_pcr_read()

v1:
- modify definition of tpm_pcr_read()
- move hash algorithms and definition of tpm2_digest to include/linux/tpm.h

Roberto Sassu (5):
  tpm: change the end marker of the active_banks array to zero
  tpm: rename and export tpm2_digest and tpm2_algorithms
  tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm
  tpm: retrieve digest size of unknown algorithms with PCR read
  tpm: ensure that output of PCR read contains the correct digest size

 drivers/char/tpm/tpm-interface.c    | 19 ++++--
 drivers/char/tpm/tpm.h              | 18 ++----
 drivers/char/tpm/tpm2-cmd.c         | 99 +++++++++++++++++++++--------
 include/linux/tpm.h                 | 30 ++++++++-
 include/linux/tpm_eventlog.h        |  9 +--
 security/integrity/ima/ima_crypto.c | 10 +--
 6 files changed, 122 insertions(+), 63 deletions(-)

-- 
2.17.1

[PATCH v3 1/5] tpm: change the end marker of the active_banks array to zero

[Roberto Sassu]

This patch changes the end marker of the active_banks array from
TPM2_ALG_ERROR to zero.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 drivers/char/tpm/tpm-interface.c | 2 +-
 drivers/char/tpm/tpm2-cmd.c      | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1a803b0cf980..f7fc4b5ee239 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1051,7 +1051,7 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
 		memset(digest_list, 0, sizeof(digest_list));
 
 		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
-			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
+			    chip->active_banks[i]; i++) {
 			digest_list[i].alg_id = chip->active_banks[i];
 			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
 			count++;
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index c31b490bd41d..046c9d8f3c1e 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -908,9 +908,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	}
 
 out:
-	if (i < ARRAY_SIZE(chip->active_banks))
-		chip->active_banks[i] = TPM2_ALG_ERROR;
-
 	tpm_buf_destroy(&buf);
 
 	return rc;
-- 
2.17.1

Re: [PATCH v3 1/5] tpm: change the end marker of the active_banks array to zero

[Jarkko Sakkinen]

On Tue, 30 Oct 2018, Roberto Sassu wrote:
> This patch changes the end marker of the active_banks array from
> TPM2_ALG_ERROR to zero.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
> drivers/char/tpm/tpm-interface.c | 2 +-
> drivers/char/tpm/tpm2-cmd.c      | 3 ---
> 2 files changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 1a803b0cf980..f7fc4b5ee239 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -1051,7 +1051,7 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
> 		memset(digest_list, 0, sizeof(digest_list));
>
> 		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
> -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
> +			    chip->active_banks[i]; i++) {
> 			digest_list[i].alg_id = chip->active_banks[i];
> 			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
> 			count++;
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index c31b490bd41d..046c9d8f3c1e 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -908,9 +908,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
> 	}
>
> out:
> -	if (i < ARRAY_SIZE(chip->active_banks))
> -		chip->active_banks[i] = TPM2_ALG_ERROR;
> -
> 	tpm_buf_destroy(&buf);
>
> 	return rc;
> -- 
> 2.17.1
>
>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

Re: [PATCH v3 1/5] tpm: change the end marker of the active_banks array to zero

[Mimi Zohar]

On Tue, 2018-10-30 at 16:47 +0100, Roberto Sassu wrote:
> This patch changes the end marker of the active_banks array from
> TPM2_ALG_ERROR to zero.

The patch description is a bit off.

TPM2_ALG_ERROR is defined as zero.  Since tpm_chip_alloc() calls
kzalloc to allocate the structure, there is no need to explicitly set
the active_banks end marker to TPM2_ALG_ERROR, nor is there a need to
explicitly test for the end marker.

This patch removes explicitly setting the end marker and changes the
coding style.

> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  drivers/char/tpm/tpm-interface.c | 2 +-
>  drivers/char/tpm/tpm2-cmd.c      | 3 ---
>  2 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 1a803b0cf980..f7fc4b5ee239 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -1051,7 +1051,7 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>  		memset(digest_list, 0, sizeof(digest_list));
>  
>  		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
> -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
> +			    chip->active_banks[i]; i++) {
>  			digest_list[i].alg_id = chip->active_banks[i];
>  			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>  			count++;
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index c31b490bd41d..046c9d8f3c1e 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -908,9 +908,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>  	}
>  
>  out:
> -	if (i < ARRAY_SIZE(chip->active_banks))
> -		chip->active_banks[i] = TPM2_ALG_ERROR;
> -
>  	tpm_buf_destroy(&buf);
>  
>  	return rc;

Re: [PATCH v3 1/5] tpm: change the end marker of the active_banks array to zero

[Mimi Zohar]

On Wed, 2018-10-31 at 10:43 -0400, Mimi Zohar wrote:
> On Tue, 2018-10-30 at 16:47 +0100, Roberto Sassu wrote:
> > This patch changes the end marker of the active_banks array from
> > TPM2_ALG_ERROR to zero.
> 
> The patch description is a bit off.
> 
> TPM2_ALG_ERROR is defined as zero.  Since tpm_chip_alloc() calls
> kzalloc to allocate the structure, there is no need to explicitly set
> the active_banks end marker to TPM2_ALG_ERROR, nor is there a need to
> explicitly test for the end marker.
> 
> This patch removes explicitly setting the end marker and changes the
> coding style.
> 
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  drivers/char/tpm/tpm-interface.c | 2 +-
> >  drivers/char/tpm/tpm2-cmd.c      | 3 ---
> >  2 files changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > index 1a803b0cf980..f7fc4b5ee239 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -1051,7 +1051,7 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
> >  		memset(digest_list, 0, sizeof(digest_list));
> >  
> >  		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
> > -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
> > +			    chip->active_banks[i]; i++) {
> >  			digest_list[i].alg_id = chip->active_banks[i];
> >  			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
> >  			count++;

The fourth patch further updates this code.  Please move those changes
to this first patch.  No need to touch the same code in both places.

                for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
-                           chip->active_banks[i]; i++) {
-                       digest_list[i].alg_id = chip->active_banks[i];
+                           chip->active_banks[i].alg_id; i++) {
+                       digest_list[i].alg_id = chip->active_banks[i].alg_id;
                        memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
                        count++;
                }
Mimi

> > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> > index c31b490bd41d..046c9d8f3c1e 100644
> > --- a/drivers/char/tpm/tpm2-cmd.c
> > +++ b/drivers/char/tpm/tpm2-cmd.c
> > @@ -908,9 +908,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
> >  	}
> >  
> >  out:
> > -	if (i < ARRAY_SIZE(chip->active_banks))
> > -		chip->active_banks[i] = TPM2_ALG_ERROR;
> > -
> >  	tpm_buf_destroy(&buf);
> >  
> >  	return rc;
> 

Re: [PATCH v3 1/5] tpm: change the end marker of the active_banks array to zero

[Roberto Sassu]

On 11/1/2018 3:42 PM, Mimi Zohar wrote:
> On Wed, 2018-10-31 at 10:43 -0400, Mimi Zohar wrote:
>> On Tue, 2018-10-30 at 16:47 +0100, Roberto Sassu wrote:
>>> This patch changes the end marker of the active_banks array from
>>> TPM2_ALG_ERROR to zero.
>>
>> The patch description is a bit off.
>>
>> TPM2_ALG_ERROR is defined as zero.  Since tpm_chip_alloc() calls
>> kzalloc to allocate the structure, there is no need to explicitly set
>> the active_banks end marker to TPM2_ALG_ERROR, nor is there a need to
>> explicitly test for the end marker.
>>
>> This patch removes explicitly setting the end marker and changes the
>> coding style.
>>
>>>
>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>> ---
>>>   drivers/char/tpm/tpm-interface.c | 2 +-
>>>   drivers/char/tpm/tpm2-cmd.c      | 3 ---
>>>   2 files changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>>> index 1a803b0cf980..f7fc4b5ee239 100644
>>> --- a/drivers/char/tpm/tpm-interface.c
>>> +++ b/drivers/char/tpm/tpm-interface.c
>>> @@ -1051,7 +1051,7 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>>>   		memset(digest_list, 0, sizeof(digest_list));
>>>   
>>>   		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>>> -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>>> +			    chip->active_banks[i]; i++) {
>>>   			digest_list[i].alg_id = chip->active_banks[i];
>>>   			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>>>   			count++;
> 
> The fourth patch further updates this code.  Please move those changes
> to this first patch.  No need to touch the same code in both places.

alg_id is introduced in patch 4/5. It cannot be moved here.

Roberto


>                  for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
> -                           chip->active_banks[i]; i++) {
> -                       digest_list[i].alg_id = chip->active_banks[i];
> +                           chip->active_banks[i].alg_id; i++) {
> +                       digest_list[i].alg_id = chip->active_banks[i].alg_id;
>                          memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>                          count++;
>                  }
> Mimi
> 
>>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>>> index c31b490bd41d..046c9d8f3c1e 100644
>>> --- a/drivers/char/tpm/tpm2-cmd.c
>>> +++ b/drivers/char/tpm/tpm2-cmd.c
>>> @@ -908,9 +908,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>>   	}
>>>   
>>>   out:
>>> -	if (i < ARRAY_SIZE(chip->active_banks))
>>> -		chip->active_banks[i] = TPM2_ALG_ERROR;
>>> -
>>>   	tpm_buf_destroy(&buf);
>>>   
>>>   	return rc;
>>
> 

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

Re: [PATCH v3 1/5] tpm: change the end marker of the active_banks array to zero

[Mimi Zohar]

> >>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> >>> index 1a803b0cf980..f7fc4b5ee239 100644
> >>> --- a/drivers/char/tpm/tpm-interface.c
> >>> +++ b/drivers/char/tpm/tpm-interface.c
> >>> @@ -1051,7 +1051,7 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
> >>>   		memset(digest_list, 0, sizeof(digest_list));
> >>>   
> >>>   		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
> >>> -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
> >>> +			    chip->active_banks[i]; i++) {
> >>>   			digest_list[i].alg_id = chip->active_banks[i];
> >>>   			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
> >>>   			count++;
> > 
> > The fourth patch further updates this code.  Please move those changes
> > to this first patch.  No need to touch the same code in both places.
> 
> alg_id is introduced in patch 4/5. It cannot be moved here.

Thanks, I missed that.

Mimi 

Re: [PATCH v3 1/5] tpm: change the end marker of the active_banks array to zero

[Roberto Sassu]

On 10/31/2018 3:43 PM, Mimi Zohar wrote:
> On Tue, 2018-10-30 at 16:47 +0100, Roberto Sassu wrote:
>> This patch changes the end marker of the active_banks array from
>> TPM2_ALG_ERROR to zero.
> 
> The patch description is a bit off.
> 
> TPM2_ALG_ERROR is defined as zero.  Since tpm_chip_alloc() calls
> kzalloc to allocate the structure, there is no need to explicitly set
> the active_banks end marker to TPM2_ALG_ERROR, nor is there a need to
> explicitly test for the end marker.
> 
> This patch removes explicitly setting the end marker and changes the
> coding style.

Thanks, I will use the text above as the patch description.

Roberto


>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>>   drivers/char/tpm/tpm-interface.c | 2 +-
>>   drivers/char/tpm/tpm2-cmd.c      | 3 ---
>>   2 files changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index 1a803b0cf980..f7fc4b5ee239 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -1051,7 +1051,7 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>>   		memset(digest_list, 0, sizeof(digest_list));
>>   
>>   		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>> -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>> +			    chip->active_banks[i]; i++) {
>>   			digest_list[i].alg_id = chip->active_banks[i];
>>   			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>>   			count++;
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index c31b490bd41d..046c9d8f3c1e 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -908,9 +908,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	}
>>   
>>   out:
>> -	if (i < ARRAY_SIZE(chip->active_banks))
>> -		chip->active_banks[i] = TPM2_ALG_ERROR;
>> -
>>   	tpm_buf_destroy(&buf);
>>   
>>   	return rc;
> 

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

[PATCH v3 2/5] tpm: rename and export tpm2_digest and tpm2_algorithms

[Roberto Sassu]

Rename tpm2_* to tpm_* and move the definitions to include/linux/tpm.h so
that these can be used by other kernel subsystems (e.g. IMA).

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Acked-by: Mimi Zohar <zohar@linux.ibm.com>
---
 drivers/char/tpm/tpm-interface.c |  2 +-
 drivers/char/tpm/tpm.h           | 13 +------------
 drivers/char/tpm/tpm2-cmd.c      | 18 +++++++++---------
 include/linux/tpm.h              | 18 ++++++++++++++++++
 include/linux/tpm_eventlog.h     |  9 ++-------
 5 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index f7fc4b5ee239..48c8d3ef5944 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1039,7 +1039,7 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
 int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
 {
 	int rc;
-	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
+	struct tpm_digest digest_list[ARRAY_SIZE(chip->active_banks)];
 	u32 count = 0;
 	int i;
 
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f3501d05264f..b928ba44d864 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -122,17 +122,6 @@ enum tpm2_return_codes {
 	TPM2_RC_RETRY		= 0x0922,
 };
 
-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_CREATE_PRIMARY  = 0x0131,
@@ -577,7 +566,7 @@ static inline u32 tpm2_rc_value(u32 rc)
 
 int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
 int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
-		    struct tpm2_digest *digests);
+		    struct tpm_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/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 046c9d8f3c1e..4744ab3a6df2 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -33,11 +33,11 @@ struct tpm2_hash {
 };
 
 static struct tpm2_hash tpm2_hash_map[] = {
-	{HASH_ALGO_SHA1, TPM2_ALG_SHA1},
-	{HASH_ALGO_SHA256, TPM2_ALG_SHA256},
-	{HASH_ALGO_SHA384, TPM2_ALG_SHA384},
-	{HASH_ALGO_SHA512, TPM2_ALG_SHA512},
-	{HASH_ALGO_SM3_256, TPM2_ALG_SM3_256},
+	{HASH_ALGO_SHA1, TPM_ALG_SHA1},
+	{HASH_ALGO_SHA256, TPM_ALG_SHA256},
+	{HASH_ALGO_SHA384, TPM_ALG_SHA384},
+	{HASH_ALGO_SHA512, TPM_ALG_SHA512},
+	{HASH_ALGO_SM3_256, TPM_ALG_SM3_256},
 };
 
 /*
@@ -200,7 +200,7 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
 	pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);
 
 	tpm_buf_append_u32(&buf, 1);
-	tpm_buf_append_u16(&buf, TPM2_ALG_SHA1);
+	tpm_buf_append_u16(&buf, TPM_ALG_SHA1);
 	tpm_buf_append_u8(&buf, TPM2_PCR_SELECT_MIN);
 	tpm_buf_append(&buf, (const unsigned char *)pcr_select,
 		       sizeof(pcr_select));
@@ -234,7 +234,7 @@ struct tpm2_null_auth_area {
  * Return: Same as with tpm_transmit_cmd.
  */
 int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
-		    struct tpm2_digest *digests)
+		    struct tpm_digest *digests)
 {
 	struct tpm_buf buf;
 	struct tpm2_null_auth_area auth_area;
@@ -457,7 +457,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 
 	/* public */
 	tpm_buf_append_u16(&buf, 14 + options->policydigest_len);
-	tpm_buf_append_u16(&buf, TPM2_ALG_KEYEDHASH);
+	tpm_buf_append_u16(&buf, TPM_ALG_KEYEDHASH);
 	tpm_buf_append_u16(&buf, hash);
 
 	/* policy */
@@ -472,7 +472,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 	}
 
 	/* public parameters */
-	tpm_buf_append_u16(&buf, TPM2_ALG_NULL);
+	tpm_buf_append_u16(&buf, TPM_ALG_NULL);
 	tpm_buf_append_u16(&buf, 0);
 
 	/* outside info */
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 4609b94142d4..71d7bbf5f690 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -22,12 +22,30 @@
 #ifndef __LINUX_TPM_H__
 #define __LINUX_TPM_H__
 
+#include <crypto/hash_info.h>
+
 #define TPM_DIGEST_SIZE 20	/* Max TPM v1.2 PCR size */
 
 struct tpm_chip;
 struct trusted_key_payload;
 struct trusted_key_options;
 
+enum tpm_algorithms {
+	TPM_ALG_ERROR		= 0x0000,
+	TPM_ALG_SHA1		= 0x0004,
+	TPM_ALG_KEYEDHASH	= 0x0008,
+	TPM_ALG_SHA256		= 0x000B,
+	TPM_ALG_SHA384		= 0x000C,
+	TPM_ALG_SHA512		= 0x000D,
+	TPM_ALG_NULL		= 0x0010,
+	TPM_ALG_SM3_256		= 0x0012,
+};
+
+struct tpm_digest {
+	u16 alg_id;
+	u8 digest[SHA512_DIGEST_SIZE];
+} __packed;
+
 enum TPM_OPS_FLAGS {
 	TPM_OPS_AUTO_STARTUP = BIT(0),
 };
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index 20d9da77fc11..1481186749f0 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -3,7 +3,7 @@
 #ifndef __LINUX_TPM_EVENTLOG_H__
 #define __LINUX_TPM_EVENTLOG_H__
 
-#include <crypto/hash_info.h>
+#include <linux/tpm.h>
 
 #define TCG_EVENT_NAME_LEN_MAX	255
 #define MAX_TEXT_EVENT		1000	/* Max event string length */
@@ -108,16 +108,11 @@ struct tcg_event_field {
 	u8 event[0];
 } __packed;
 
-struct tpm2_digest {
-	u16 alg_id;
-	u8 digest[SHA512_DIGEST_SIZE];
-} __packed;
-
 struct tcg_pcr_event2 {
 	u32 pcr_idx;
 	u32 event_type;
 	u32 count;
-	struct tpm2_digest digests[TPM2_ACTIVE_PCR_BANKS];
+	struct tpm_digest digests[TPM2_ACTIVE_PCR_BANKS];
 	struct tcg_event_field event;
 } __packed;
 
-- 
2.17.1

[PATCH v3 3/5] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm

[Roberto Sassu]

Currently the TPM driver allows other kernel subsystems to read only the
SHA1 PCR bank. This patch modifies the parameters of tpm_pcr_read() and
tpm2_pcr_read() to pass a tpm_digest structure, which contains the desired
hash algorithm. Also, since commit 125a22105410 ("tpm: React correctly to
RC_TESTING from TPM 2.0 self tests") removed the call to tpm2_pcr_read(),
the new parameter is expected to be always not NULL.

Due to the API change, IMA functions have been modified.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Acked-by: Mimi Zohar <zohar@linux.ibm.com>
---
 drivers/char/tpm/tpm-interface.c    |  9 +++++----
 drivers/char/tpm/tpm.h              |  3 ++-
 drivers/char/tpm/tpm2-cmd.c         | 23 ++++++++++++++++-------
 include/linux/tpm.h                 |  6 ++++--
 security/integrity/ima/ima_crypto.c | 10 +++++-----
 5 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 48c8d3ef5944..4d0f5da5bf98 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -976,11 +976,12 @@ EXPORT_SYMBOL_GPL(tpm_is_tpm2);
  * tpm_pcr_read - read a PCR value from SHA1 bank
  * @chip:	a &struct tpm_chip instance, %NULL for the default chip
  * @pcr_idx:	the PCR to be retrieved
- * @res_buf:	the value of the PCR
+ * @digest_struct:	pcr bank and buffer current PCR value is written to
  *
  * Return: same as with tpm_transmit_cmd()
  */
-int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
+int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
+		 struct tpm_digest *digest_struct)
 {
 	int rc;
 
@@ -988,9 +989,9 @@ int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
 	if (!chip)
 		return -ENODEV;
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		rc = tpm2_pcr_read(chip, pcr_idx, res_buf);
+		rc = tpm2_pcr_read(chip, pcr_idx, digest_struct);
 	else
-		rc = tpm_pcr_read_dev(chip, pcr_idx, res_buf);
+		rc = tpm_pcr_read_dev(chip, pcr_idx, digest_struct->digest);
 	tpm_put_ops(chip);
 	return rc;
 }
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index b928ba44d864..633e16f42107 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -564,7 +564,8 @@ static inline u32 tpm2_rc_value(u32 rc)
 	return (rc & BIT(7)) ? rc & 0xff : rc;
 }
 
-int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
+int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
+		  struct tpm_digest *digest_struct);
 int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
 		    struct tpm_digest *digests);
 int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 4744ab3a6df2..8babd826ad27 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -179,16 +179,18 @@ struct tpm2_pcr_read_out {
  * tpm2_pcr_read() - read a PCR value
  * @chip:	TPM chip to use.
  * @pcr_idx:	index of the PCR to read.
- * @res_buf:	buffer to store the resulting hash.
+ * @digest_struct:	pcr bank and buffer current PCR value is written to.
  *
  * Return: Same as with tpm_transmit_cmd.
  */
-int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
+int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
+		  struct tpm_digest *digest_struct)
 {
 	int rc;
 	struct tpm_buf buf;
 	struct tpm2_pcr_read_out *out;
 	u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
+	u16 digest_size;
 
 	if (pcr_idx >= TPM2_PLATFORM_PCR)
 		return -EINVAL;
@@ -200,18 +202,25 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
 	pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);
 
 	tpm_buf_append_u32(&buf, 1);
-	tpm_buf_append_u16(&buf, TPM_ALG_SHA1);
+	tpm_buf_append_u16(&buf, digest_struct->alg_id);
 	tpm_buf_append_u8(&buf, TPM2_PCR_SELECT_MIN);
 	tpm_buf_append(&buf, (const unsigned char *)pcr_select,
 		       sizeof(pcr_select));
 
 	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
-			res_buf ? "attempting to read a pcr value" : NULL);
-	if (rc == 0 && res_buf) {
-		out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
-		memcpy(res_buf, out->digest, SHA1_DIGEST_SIZE);
+			      "attempting to read a pcr value");
+	if (rc)
+		goto out;
+
+	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)) {
+		rc = -EINVAL;
+		goto out;
 	}
 
+	memcpy(digest_struct->digest, out->digest, digest_size);
+out:
 	tpm_buf_destroy(&buf);
 	return rc;
 }
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 71d7bbf5f690..4f00daf44dd2 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -71,7 +71,8 @@ struct tpm_class_ops {
 #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
 
 extern int tpm_is_tpm2(struct tpm_chip *chip);
-extern int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
+extern int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
+			struct tpm_digest *digest_struct);
 extern int tpm_pcr_extend(struct tpm_chip *chip, int 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);
@@ -87,7 +88,8 @@ static inline int tpm_is_tpm2(struct tpm_chip *chip)
 {
 	return -ENODEV;
 }
-static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
+static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
+			       struct tpm_digest *digest_struct)
 {
 	return -ENODEV;
 }
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 7e7e7e7c250a..8985e34eb3ac 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -629,12 +629,12 @@ int ima_calc_buffer_hash(const void *buf, loff_t len,
 	return calc_buffer_shash(buf, len, hash);
 }
 
-static void __init ima_pcrread(int idx, u8 *pcr)
+static void __init ima_pcrread(int idx, struct tpm_digest *d)
 {
 	if (!ima_tpm_chip)
 		return;
 
-	if (tpm_pcr_read(ima_tpm_chip, idx, pcr) != 0)
+	if (tpm_pcr_read(ima_tpm_chip, idx, d) != 0)
 		pr_err("Error Communicating to TPM chip\n");
 }
 
@@ -644,7 +644,7 @@ static void __init ima_pcrread(int idx, u8 *pcr)
 static int __init ima_calc_boot_aggregate_tfm(char *digest,
 					      struct crypto_shash *tfm)
 {
-	u8 pcr_i[TPM_DIGEST_SIZE];
+	struct tpm_digest d = { .alg_id = TPM_ALG_SHA1, .digest = {0} };
 	int rc, i;
 	SHASH_DESC_ON_STACK(shash, tfm);
 
@@ -657,9 +657,9 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest,
 
 	/* cumulative sha1 over tpm registers 0-7 */
 	for (i = TPM_PCR0; i < TPM_PCR8; i++) {
-		ima_pcrread(i, pcr_i);
+		ima_pcrread(i, &d);
 		/* now accumulate with current aggregate */
-		rc = crypto_shash_update(shash, pcr_i, TPM_DIGEST_SIZE);
+		rc = crypto_shash_update(shash, d.digest, TPM_DIGEST_SIZE);
 	}
 	if (!rc)
 		crypto_shash_final(shash, digest);
-- 
2.17.1

[PATCH v3 4/5] tpm: retrieve digest size of unknown algorithms with PCR read

[Roberto Sassu]

Currently, the TPM driver retrieves the digest size from a table mapping
TPM algorithms identifiers to identifiers defined by the crypto subsystem.
If the algorithm is not defined by the latter, the digest size can be
retrieved from the output of the PCR read command.

The patch retrieves at TPM startup the digest sizes for each PCR bank and
stores them in the new structure tpm_bank_info, member of tpm_chip, so that
the information can be passed to other kernel subsystems.

tpm_bank_info contains: the TPM algorithm identifier, necessary to generate
the event log as defined by Trusted Computing Group (TCG); the digest size,
to pad/truncate a digest calculated with a different algorithm; the crypto
subsystem identifier, to calculate the digest of event data.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Acked-by: Mimi Zohar <zohar@linux.ibm.com>
---
 drivers/char/tpm/tpm-interface.c | 10 ++++---
 drivers/char/tpm/tpm.h           |  4 +--
 drivers/char/tpm/tpm2-cmd.c      | 45 ++++++++++++++++++++++++--------
 include/linux/tpm.h              |  6 +++++
 4 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 4d0f5da5bf98..49761c88c21e 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -989,7 +989,7 @@ int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
 	if (!chip)
 		return -ENODEV;
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		rc = tpm2_pcr_read(chip, pcr_idx, digest_struct);
+		rc = tpm2_pcr_read(chip, pcr_idx, digest_struct, NULL);
 	else
 		rc = tpm_pcr_read_dev(chip, pcr_idx, digest_struct->digest);
 	tpm_put_ops(chip);
@@ -1052,8 +1052,8 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
 		memset(digest_list, 0, sizeof(digest_list));
 
 		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
-			    chip->active_banks[i]; i++) {
-			digest_list[i].alg_id = chip->active_banks[i];
+			    chip->active_banks[i].alg_id; i++) {
+			digest_list[i].alg_id = chip->active_banks[i].alg_id;
 			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
 			count++;
 		}
@@ -1154,6 +1154,10 @@ int tpm1_auto_startup(struct tpm_chip *chip)
 		goto out;
 	}
 
+	chip->active_banks[0].alg_id = TPM_ALG_SHA1;
+	chip->active_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
+	chip->active_banks[0].crypto_id = HASH_ALGO_SHA1;
+
 	return rc;
 out:
 	if (rc > 0)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 633e16f42107..8662e8587dce 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -237,7 +237,7 @@ struct tpm_chip {
 	const struct attribute_group *groups[3];
 	unsigned int groups_cnt;
 
-	u16 active_banks[7];
+	struct tpm_bank_info active_banks[7];
 #ifdef CONFIG_ACPI
 	acpi_handle acpi_dev_handle;
 	char ppi_version[TPM_PPI_VERSION_LEN + 1];
@@ -565,7 +565,7 @@ static inline u32 tpm2_rc_value(u32 rc)
 }
 
 int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
-		  struct tpm_digest *digest_struct);
+		  struct tpm_digest *digest_struct, u16 *digest_size_ptr);
 int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
 		    struct tpm_digest *digests);
 int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 8babd826ad27..8e821e7b4674 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -180,11 +180,12 @@ struct tpm2_pcr_read_out {
  * @chip:	TPM chip to use.
  * @pcr_idx:	index of the PCR to read.
  * @digest_struct:	pcr bank and buffer current PCR value is written to.
+ * @digest_size_ptr:	pointer to variable that stores the digest size.
  *
  * Return: Same as with tpm_transmit_cmd.
  */
 int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
-		  struct tpm_digest *digest_struct)
+		  struct tpm_digest *digest_struct, u16 *digest_size_ptr)
 {
 	int rc;
 	struct tpm_buf buf;
@@ -219,6 +220,9 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
 		goto out;
 	}
 
+	if (digest_size_ptr)
+		*digest_size_ptr = digest_size;
+
 	memcpy(digest_struct->digest, out->digest, digest_size);
 out:
 	tpm_buf_destroy(&buf);
@@ -249,7 +253,6 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
 	struct tpm2_null_auth_area auth_area;
 	int rc;
 	int i;
-	int j;
 
 	if (count > ARRAY_SIZE(chip->active_banks))
 		return -EINVAL;
@@ -271,14 +274,9 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
 	tpm_buf_append_u32(&buf, count);
 
 	for (i = 0; i < count; i++) {
-		for (j = 0; j < ARRAY_SIZE(tpm2_hash_map); j++) {
-			if (digests[i].alg_id != tpm2_hash_map[j].tpm_id)
-				continue;
-			tpm_buf_append_u16(&buf, digests[i].alg_id);
-			tpm_buf_append(&buf, (const unsigned char
-					      *)&digests[i].digest,
-			       hash_digest_size[tpm2_hash_map[j].crypto_id]);
-		}
+		tpm_buf_append_u16(&buf, digests[i].alg_id);
+		tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
+			       chip->active_banks[i].digest_size);
 	}
 
 	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
@@ -855,6 +853,26 @@ int tpm2_probe(struct tpm_chip *chip)
 }
 EXPORT_SYMBOL_GPL(tpm2_probe);
 
+static int tpm2_init_bank_info(struct tpm_chip *chip,
+			       struct tpm_bank_info *bank)
+{
+	struct tpm_digest digest = {.alg_id = bank->alg_id};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
+		enum hash_algo crypto_algo = tpm2_hash_map[i].crypto_id;
+
+		if (bank->alg_id != tpm2_hash_map[i].tpm_id)
+			continue;
+
+		bank->digest_size = hash_digest_size[crypto_algo];
+		bank->crypto_id = crypto_algo;
+		return 0;
+	}
+
+	return tpm2_pcr_read(chip, 0, &digest, &bank->digest_size);
+}
+
 struct tpm2_pcr_selection {
 	__be16  hash_alg;
 	u8  size_of_select;
@@ -909,7 +927,12 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 		}
 
 		memcpy(&pcr_selection, marker, sizeof(pcr_selection));
-		chip->active_banks[i] = be16_to_cpu(pcr_selection.hash_alg);
+		chip->active_banks[i].alg_id =
+			be16_to_cpu(pcr_selection.hash_alg);
+		rc =  tpm2_init_bank_info(chip, &chip->active_banks[i]);
+		if (rc)
+			break;
+
 		sizeof_pcr_selection = sizeof(pcr_selection.hash_alg) +
 			sizeof(pcr_selection.size_of_select) +
 			pcr_selection.size_of_select;
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 4f00daf44dd2..3f91124837cf 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -46,6 +46,12 @@ struct tpm_digest {
 	u8 digest[SHA512_DIGEST_SIZE];
 } __packed;
 
+struct tpm_bank_info {
+	u16 alg_id;
+	u16 digest_size;
+	u16 crypto_id;
+};
+
 enum TPM_OPS_FLAGS {
 	TPM_OPS_AUTO_STARTUP = BIT(0),
 };
-- 
2.17.1

Re: [PATCH v3 4/5] tpm: retrieve digest size of unknown algorithms with PCR read

[Mimi Zohar]

On Tue, 2018-10-30 at 16:47 +0100, Roberto Sassu wrote:
> Currently, the TPM driver retrieves the digest size from a table mapping
> TPM algorithms identifiers to identifiers defined by the crypto subsystem.
> If the algorithm is not defined by the latter, the digest size can be
> retrieved from the output of the PCR read command.
> 
> The patch retrieves at TPM startup the digest sizes for each PCR bank and
> stores them in the new structure tpm_bank_info, member of tpm_chip, so that
> the information can be passed to other kernel subsystems.
> 
> tpm_bank_info contains: the TPM algorithm identifier, necessary to generate
> the event log as defined by Trusted Computing Group (TCG); the digest size,
> to pad/truncate a digest calculated with a different algorithm; the crypto
> subsystem identifier, to calculate the digest of event data.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Acked-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>  drivers/char/tpm/tpm-interface.c | 10 ++++---
>  drivers/char/tpm/tpm.h           |  4 +--
>  drivers/char/tpm/tpm2-cmd.c      | 45 ++++++++++++++++++++++++--------
>  include/linux/tpm.h              |  6 +++++
>  4 files changed, 49 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 4d0f5da5bf98..49761c88c21e 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -989,7 +989,7 @@ int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
>  	if (!chip)
>  		return -ENODEV;
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> -		rc = tpm2_pcr_read(chip, pcr_idx, digest_struct);
> +		rc = tpm2_pcr_read(chip, pcr_idx, digest_struct, NULL);
>  	else
>  		rc = tpm_pcr_read_dev(chip, pcr_idx, digest_struct->digest);
>  	tpm_put_ops(chip);
> @@ -1052,8 +1052,8 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>  		memset(digest_list, 0, sizeof(digest_list));
>  
>  		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
> -			    chip->active_banks[i]; i++) {
> -			digest_list[i].alg_id = chip->active_banks[i];
> +			    chip->active_banks[i].alg_id; i++) {
> +			digest_list[i].alg_id = chip->active_banks[i].alg_id;
>  			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>  			count++;
>  		}
> @@ -1154,6 +1154,10 @@ int tpm1_auto_startup(struct tpm_chip *chip)
>  		goto out;
>  	}
>  
> +	chip->active_banks[0].alg_id = TPM_ALG_SHA1;
> +	chip->active_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
> +	chip->active_banks[0].crypto_id = HASH_ALGO_SHA1;
> +
>  	return rc;
>  out:
>  	if (rc > 0)
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 633e16f42107..8662e8587dce 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -237,7 +237,7 @@ struct tpm_chip {
>  	const struct attribute_group *groups[3];
>  	unsigned int groups_cnt;
>  
> -	u16 active_banks[7];
> +	struct tpm_bank_info active_banks[7];

Commit 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
PCR banks") defined active_banks[7].  Subsequently, commit
4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware event
log") defined TPM2_PCR_ACTIVE_BANKS as 3.  I'm not sure which is the
correct value, but the number of active_banks should not be hard coded
here.


>  #ifdef CONFIG_ACPI
>  	acpi_handle acpi_dev_handle;
>  	char ppi_version[TPM_PPI_VERSION_LEN + 1];
> @@ -565,7 +565,7 @@ static inline u32 tpm2_rc_value(u32 rc)
>  }
>  
>  int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
> -		  struct tpm_digest *digest_struct);
> +		  struct tpm_digest *digest_struct, u16 *digest_size_ptr);
>  int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
>  		    struct tpm_digest *digests);
>  int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 8babd826ad27..8e821e7b4674 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -180,11 +180,12 @@ struct tpm2_pcr_read_out {
>   * @chip:	TPM chip to use.
>   * @pcr_idx:	index of the PCR to read.
>   * @digest_struct:	pcr bank and buffer current PCR value is written to.
> + * @digest_size_ptr:	pointer to variable that stores the digest size.
>   *
>   * Return: Same as with tpm_transmit_cmd.
>   */
>  int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
> -		  struct tpm_digest *digest_struct)
> +		  struct tpm_digest *digest_struct, u16 *digest_size_ptr)
>  {
>  	int rc;
>  	struct tpm_buf buf;
> @@ -219,6 +220,9 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
>  		goto out;
>  	}
>  
> +	if (digest_size_ptr)
> +		*digest_size_ptr = digest_size;
> +
>  	memcpy(digest_struct->digest, out->digest, digest_size);
>  out:
>  	tpm_buf_destroy(&buf);
> @@ -249,7 +253,6 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
>  	struct tpm2_null_auth_area auth_area;
>  	int rc;
>  	int i;
> -	int j;
>  
>  	if (count > ARRAY_SIZE(chip->active_banks))
>  		return -EINVAL;
> @@ -271,14 +274,9 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
>  	tpm_buf_append_u32(&buf, count);
>  
>  	for (i = 0; i < count; i++) {
> -		for (j = 0; j < ARRAY_SIZE(tpm2_hash_map); j++) {
> -			if (digests[i].alg_id != tpm2_hash_map[j].tpm_id)
> -				continue;
> -			tpm_buf_append_u16(&buf, digests[i].alg_id);
> -			tpm_buf_append(&buf, (const unsigned char
> -					      *)&digests[i].digest,
> -			       hash_digest_size[tpm2_hash_map[j].crypto_id]);
> -		}
> +		tpm_buf_append_u16(&buf, digests[i].alg_id);
> +		tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
> +			       chip->active_banks[i].digest_size);
>  	}
>  

These tpm2_pcr_extend changes don't belong here in this patch.  Please
move them to 1/5.

>  	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
> @@ -855,6 +853,26 @@ int tpm2_probe(struct tpm_chip *chip)
>  }
>  EXPORT_SYMBOL_GPL(tpm2_probe);
>  
> +static int tpm2_init_bank_info(struct tpm_chip *chip,
> +			       struct tpm_bank_info *bank)
> +{
> +	struct tpm_digest digest = {.alg_id = bank->alg_id};
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
> +		enum hash_algo crypto_algo = tpm2_hash_map[i].crypto_id;
> +
> +		if (bank->alg_id != tpm2_hash_map[i].tpm_id)
> +			continue;
> +
> +		bank->digest_size = hash_digest_size[crypto_algo];
> +		bank->crypto_id = crypto_algo;
> +		return 0;
> +	}
> +
> +	return tpm2_pcr_read(chip, 0, &digest, &bank->digest_size);
> +}
> +
>  struct tpm2_pcr_selection {
>  	__be16  hash_alg;
>  	u8  size_of_select;
> @@ -909,7 +927,12 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>  		}
>  
>  		memcpy(&pcr_selection, marker, sizeof(pcr_selection));
> -		chip->active_banks[i] = be16_to_cpu(pcr_selection.hash_alg);
> +		chip->active_banks[i].alg_id =
> +			be16_to_cpu(pcr_selection.hash_alg);
> +		rc =  tpm2_init_bank_info(chip, &chip->active_banks[i]);

Please fix the formatting in the above 2 lines.

There's been discussion in the past on removing scripts/Lindent from
the coding-style documentation, but that hasn't happened quite yet. I
do think section 3 "New drivers" in Documentation/hwmon/submitting-
patches has a good balance.

* Running your patch or driver file(s) through checkpatch does not mean its
  formatting is clean. If unsure about formatting in your new driver, run it
  through Lindent. Lindent is not perfect, and you may have to do some minor
  cleanup, but it is a good start.

thanks!

Mimi  

> +		if (rc)
> +			break;
> +
>  		sizeof_pcr_selection = sizeof(pcr_selection.hash_alg) +
>  			sizeof(pcr_selection.size_of_select) +
>  			pcr_selection.size_of_select;
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 4f00daf44dd2..3f91124837cf 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -46,6 +46,12 @@ struct tpm_digest {
>  	u8 digest[SHA512_DIGEST_SIZE];
>  } __packed;
>  
> +struct tpm_bank_info {
> +	u16 alg_id;
> +	u16 digest_size;
> +	u16 crypto_id;
> +};
> +
>  enum TPM_OPS_FLAGS {
>  	TPM_OPS_AUTO_STARTUP = BIT(0),
>  };

Re: [PATCH v3 4/5] tpm: retrieve digest size of unknown algorithms with PCR read

[Mimi Zohar]

On Thu, 2018-11-01 at 12:02 -0400, Mimi Zohar wrote:
> On Tue, 2018-10-30 at 16:47 +0100, Roberto Sassu wrote:

 
> > +static int tpm2_init_bank_info(struct tpm_chip *chip,
> > +			       struct tpm_bank_info *bank)
> > +{
> > +	struct tpm_digest digest = {.alg_id = bank->alg_id};
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
> > +		enum hash_algo crypto_algo = tpm2_hash_map[i].crypto_id;
> > +
> > +		if (bank->alg_id != tpm2_hash_map[i].tpm_id)
> > +			continue;
> > +
> > +		bank->digest_size = hash_digest_size[crypto_algo];
> > +		bank->crypto_id = crypto_algo;
> > +		return 0;
> > +	}
> > +
> > +	return tpm2_pcr_read(chip, 0, &digest, &bank->digest_size);

Somewhere there needs to be a check to verify that the returned digest
size is equal to or less than the maximum digest size defined in
tpm_digest structure.

Mimi

Re: [PATCH v3 4/5] tpm: retrieve digest size of unknown algorithms with PCR read

[Roberto Sassu]

On 11/1/2018 5:02 PM, Mimi Zohar wrote:
> On Tue, 2018-10-30 at 16:47 +0100, Roberto Sassu wrote:

[...]

>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index 633e16f42107..8662e8587dce 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -237,7 +237,7 @@ struct tpm_chip {
>>   	const struct attribute_group *groups[3];
>>   	unsigned int groups_cnt;
>>   
>> -	u16 active_banks[7];
>> +	struct tpm_bank_info active_banks[7];
> 
> Commit 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
> PCR banks") defined active_banks[7].  Subsequently, commit
> 4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware event
> log") defined TPM2_PCR_ACTIVE_BANKS as 3.  I'm not sure which is the
> correct value, but the number of active_banks should not be hard coded
> here.

Jarkko, should I change the value of TPM2_PCR_ACTIVE_BANKS, or set the
size of the active_banks array to TPM2_PCR_ACTIVE_BANKS?


>>   #ifdef CONFIG_ACPI
>>   	acpi_handle acpi_dev_handle;
>>   	char ppi_version[TPM_PPI_VERSION_LEN + 1];
>> @@ -565,7 +565,7 @@ static inline u32 tpm2_rc_value(u32 rc)
>>   }
>>   
>>   int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
>> -		  struct tpm_digest *digest_struct);
>> +		  struct tpm_digest *digest_struct, u16 *digest_size_ptr);
>>   int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
>>   		    struct tpm_digest *digests);
>>   int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index 8babd826ad27..8e821e7b4674 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -180,11 +180,12 @@ struct tpm2_pcr_read_out {
>>    * @chip:	TPM chip to use.
>>    * @pcr_idx:	index of the PCR to read.
>>    * @digest_struct:	pcr bank and buffer current PCR value is written to.
>> + * @digest_size_ptr:	pointer to variable that stores the digest size.
>>    *
>>    * Return: Same as with tpm_transmit_cmd.
>>    */
>>   int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
>> -		  struct tpm_digest *digest_struct)
>> +		  struct tpm_digest *digest_struct, u16 *digest_size_ptr)
>>   {
>>   	int rc;
>>   	struct tpm_buf buf;
>> @@ -219,6 +220,9 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
>>   		goto out;
>>   	}
>>   
>> +	if (digest_size_ptr)
>> +		*digest_size_ptr = digest_size;
>> +
>>   	memcpy(digest_struct->digest, out->digest, digest_size);
>>   out:
>>   	tpm_buf_destroy(&buf);
>> @@ -249,7 +253,6 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
>>   	struct tpm2_null_auth_area auth_area;
>>   	int rc;
>>   	int i;
>> -	int j;
>>   
>>   	if (count > ARRAY_SIZE(chip->active_banks))
>>   		return -EINVAL;
>> @@ -271,14 +274,9 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
>>   	tpm_buf_append_u32(&buf, count);
>>   
>>   	for (i = 0; i < count; i++) {
>> -		for (j = 0; j < ARRAY_SIZE(tpm2_hash_map); j++) {
>> -			if (digests[i].alg_id != tpm2_hash_map[j].tpm_id)
>> -				continue;
>> -			tpm_buf_append_u16(&buf, digests[i].alg_id);
>> -			tpm_buf_append(&buf, (const unsigned char
>> -					      *)&digests[i].digest,
>> -			       hash_digest_size[tpm2_hash_map[j].crypto_id]);
>> -		}
>> +		tpm_buf_append_u16(&buf, digests[i].alg_id);
>> +		tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
>> +			       chip->active_banks[i].digest_size);
>>   	}
>>   
> 
> These tpm2_pcr_extend changes don't belong here in this patch.  Please
> move them to 1/5.

Also in this case, alg_id and digest_size are defined in patch 4/5.


>>   	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
>> @@ -855,6 +853,26 @@ int tpm2_probe(struct tpm_chip *chip)
>>   }
>>   EXPORT_SYMBOL_GPL(tpm2_probe);
>>   
>> +static int tpm2_init_bank_info(struct tpm_chip *chip,
>> +			       struct tpm_bank_info *bank)
>> +{
>> +	struct tpm_digest digest = {.alg_id = bank->alg_id};
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
>> +		enum hash_algo crypto_algo = tpm2_hash_map[i].crypto_id;
>> +
>> +		if (bank->alg_id != tpm2_hash_map[i].tpm_id)
>> +			continue;
>> +
>> +		bank->digest_size = hash_digest_size[crypto_algo];
>> +		bank->crypto_id = crypto_algo;
>> +		return 0;
>> +	}
>> +
>> +	return tpm2_pcr_read(chip, 0, &digest, &bank->digest_size);
>> +}
>> +
>>   struct tpm2_pcr_selection {
>>   	__be16  hash_alg;
>>   	u8  size_of_select;
>> @@ -909,7 +927,12 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   		}
>>   
>>   		memcpy(&pcr_selection, marker, sizeof(pcr_selection));
>> -		chip->active_banks[i] = be16_to_cpu(pcr_selection.hash_alg);
>> +		chip->active_banks[i].alg_id =
>> +			be16_to_cpu(pcr_selection.hash_alg);
>> +		rc =  tpm2_init_bank_info(chip, &chip->active_banks[i]);
> 
> Please fix the formatting in the above 2 lines.
> 
> There's been discussion in the past on removing scripts/Lindent from
> the coding-style documentation, but that hasn't happened quite yet. I
> do think section 3 "New drivers" in Documentation/hwmon/submitting-
> patches has a good balance.
> 
> * Running your patch or driver file(s) through checkpatch does not mean its
>    formatting is clean. If unsure about formatting in your new driver, run it
>    through Lindent. Lindent is not perfect, and you may have to do some minor
>    cleanup, but it is a good start.

Ok.

Thanks

Roberto


> thanks!
> 
> Mimi
> 
>> +		if (rc)
>> +			break;
>> +
>>   		sizeof_pcr_selection = sizeof(pcr_selection.hash_alg) +
>>   			sizeof(pcr_selection.size_of_select) +
>>   			pcr_selection.size_of_select;
>> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>> index 4f00daf44dd2..3f91124837cf 100644
>> --- a/include/linux/tpm.h
>> +++ b/include/linux/tpm.h
>> @@ -46,6 +46,12 @@ struct tpm_digest {
>>   	u8 digest[SHA512_DIGEST_SIZE];
>>   } __packed;
>>   
>> +struct tpm_bank_info {
>> +	u16 alg_id;
>> +	u16 digest_size;
>> +	u16 crypto_id;
>> +};
>> +
>>   enum TPM_OPS_FLAGS {
>>   	TPM_OPS_AUTO_STARTUP = BIT(0),
>>   };
> 

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

Re: [PATCH v3 4/5] tpm: retrieve digest size of unknown algorithms with PCR read

[Jarkko Sakkinen]

On Mon, Nov 05, 2018 at 10:47:19AM +0100, Roberto Sassu wrote:
> > Commit 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
> > PCR banks") defined active_banks[7].  Subsequently, commit
> > 4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware event
> > log") defined TPM2_PCR_ACTIVE_BANKS as 3.  I'm not sure which is the
> > correct value, but the number of active_banks should not be hard coded
> > here.
> 
> Jarkko, should I change the value of TPM2_PCR_ACTIVE_BANKS, or set the
> size of the active_banks array to TPM2_PCR_ACTIVE_BANKS?

Hi, sorry I missed your patch set. Please add me either to 'To' or 'Cc'
field of the email if you want a quick response.

I think the implementation is flakky in both places and should be fixed
before doing any other changes. Thanks James for pointing out these
commits.

What you need to do is to create a prequel commit that reads the number
of banks to a variable e.g.

  unsigned int nr_active_banks;

and allocate 'active_banks' dynamically and change the places that
James pointed out. I guess it is OK to have a commit with two 'Fixes'
tags.

/Jarkko

Re: [PATCH v3 4/5] tpm: retrieve digest size of unknown algorithms with PCR read

[Roberto Sassu]

On 11/5/2018 1:01 PM, Jarkko Sakkinen wrote:
> On Mon, Nov 05, 2018 at 10:47:19AM +0100, Roberto Sassu wrote:
>>> Commit 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
>>> PCR banks") defined active_banks[7].  Subsequently, commit
>>> 4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware event
>>> log") defined TPM2_PCR_ACTIVE_BANKS as 3.  I'm not sure which is the
>>> correct value, but the number of active_banks should not be hard coded
>>> here.
>>
>> Jarkko, should I change the value of TPM2_PCR_ACTIVE_BANKS, or set the
>> size of the active_banks array to TPM2_PCR_ACTIVE_BANKS?
> 
> Hi, sorry I missed your patch set. Please add me either to 'To' or 'Cc'
> field of the email if you want a quick response.
> 
> I think the implementation is flakky in both places and should be fixed
> before doing any other changes. Thanks James for pointing out these
> commits.
> 
> What you need to do is to create a prequel commit that reads the number
> of banks to a variable e.g.
> 
>    unsigned int nr_active_banks;
> 
> and allocate 'active_banks' dynamically and change the places that
> James pointed out. I guess it is OK to have a commit with two 'Fixes'
> tags.

Ok, then I can remove patch 1/5 if nr_active_banks is included in the
tpm_chip structure.

Roberto


> /Jarkko
> 

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

Re: [PATCH v3 4/5] tpm: retrieve digest size of unknown algorithms with PCR read

[Mimi Zohar]

On Mon, 2018-11-05 at 14:09 +0100, Roberto Sassu wrote:
> On 11/5/2018 1:01 PM, Jarkko Sakkinen wrote:

> Ok, then I can remove patch 1/5 if nr_active_banks is included in the
> tpm_chip structure.

Right, 1/5 would be replaced with the nr_active_banks usage.

Mimi

Re: [PATCH v3 4/5] tpm: retrieve digest size of unknown algorithms with PCR read

[Jarkko Sakkinen]

On Mon, Nov 05, 2018 at 02:09:12PM +0100, Roberto Sassu wrote:
> On 11/5/2018 1:01 PM, Jarkko Sakkinen wrote:
> > On Mon, Nov 05, 2018 at 10:47:19AM +0100, Roberto Sassu wrote:
> > > > Commit 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
> > > > PCR banks") defined active_banks[7].  Subsequently, commit
> > > > 4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware event
> > > > log") defined TPM2_PCR_ACTIVE_BANKS as 3.  I'm not sure which is the
> > > > correct value, but the number of active_banks should not be hard coded
> > > > here.
> > > 
> > > Jarkko, should I change the value of TPM2_PCR_ACTIVE_BANKS, or set the
> > > size of the active_banks array to TPM2_PCR_ACTIVE_BANKS?
> > 
> > Hi, sorry I missed your patch set. Please add me either to 'To' or 'Cc'
> > field of the email if you want a quick response.
> > 
> > I think the implementation is flakky in both places and should be fixed
> > before doing any other changes. Thanks James for pointing out these
> > commits.
> > 
> > What you need to do is to create a prequel commit that reads the number
> > of banks to a variable e.g.
> > 
> >    unsigned int nr_active_banks;
> > 
> > and allocate 'active_banks' dynamically and change the places that
> > James pointed out. I guess it is OK to have a commit with two 'Fixes'
> > tags.
> 
> Ok, then I can remove patch 1/5 if nr_active_banks is included in the
> tpm_chip structure.
> 
> Roberto

Yeah, I think it would be appropriate to have two fixes tags albeit it
is arguable whether those are regressions (probably not, I guess
inconsistency would be a better word) but I don't think they need to be
cc'd to stable@vger.kernel.org.

/Jarkko

Re: [PATCH v3 4/5] tpm: retrieve digest size of unknown algorithms with PCR read

[Jarkko Sakkinen]

On Mon, Nov 05, 2018 at 02:01:47PM +0200, Jarkko Sakkinen wrote:
> On Mon, Nov 05, 2018 at 10:47:19AM +0100, Roberto Sassu wrote:
> > > Commit 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
> > > PCR banks") defined active_banks[7].  Subsequently, commit
> > > 4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware event
> > > log") defined TPM2_PCR_ACTIVE_BANKS as 3.  I'm not sure which is the
> > > correct value, but the number of active_banks should not be hard coded
> > > here.
> > 
> > Jarkko, should I change the value of TPM2_PCR_ACTIVE_BANKS, or set the
> > size of the active_banks array to TPM2_PCR_ACTIVE_BANKS?
> 
> Hi, sorry I missed your patch set. Please add me either to 'To' or 'Cc'
> field of the email if you want a quick response.
> 
> I think the implementation is flakky in both places and should be fixed
> before doing any other changes. Thanks James for pointing out these
> commits.
> 
> What you need to do is to create a prequel commit that reads the number
> of banks to a variable e.g.
> 
>   unsigned int nr_active_banks;
> 
> and allocate 'active_banks' dynamically and change the places that
> James pointed out. I guess it is OK to have a commit with two 'Fixes'
> tags.

Oops sorry Roberto! I had already reviewed this version :-) Apologies.

/Jarkko

[PATCH v3 5/5] tpm: ensure that output of PCR read contains the correct digest size

[Roberto Sassu]

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)) ||
+	    (!digest_size_ptr && digest_size != expected_digest_size)) {
 		rc = -EINVAL;
 		goto out;
 	}
-- 
2.17.1

Re: [PATCH v3 5/5] tpm: ensure that output of PCR read contains the correct digest size

[Jarkko Sakkinen]

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

Re: [PATCH v3 5/5] tpm: ensure that output of PCR read contains the correct digest size

[Roberto Sassu]

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

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

Re: [PATCH v3 5/5] tpm: ensure that output of PCR read contains the correct digest size

[Jarkko Sakkinen]

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

Re: [PATCH v3 5/5] tpm: ensure that output of PCR read contains the correct digest size

[Mimi Zohar]

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;
>  	}