diff mbox series

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

Message ID 20181030154711.2782-5-roberto.sassu@huawei.com (mailing list archive)
State New, archived
Headers show
Series tpm: retrieve digest size of unknown algorithms from TPM | expand

Commit Message

Roberto Sassu Oct. 30, 2018, 3:47 p.m. UTC
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(-)

Comments

Mimi Zohar Nov. 1, 2018, 4:02 p.m. UTC | #1
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),
>  };
Mimi Zohar Nov. 1, 2018, 4:23 p.m. UTC | #2
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
Roberto Sassu Nov. 5, 2018, 9:47 a.m. UTC | #3
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),
>>   };
>
Jarkko Sakkinen Nov. 5, 2018, 12:01 p.m. UTC | #4
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
Roberto Sassu Nov. 5, 2018, 1:09 p.m. UTC | #5
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
>
Mimi Zohar Nov. 5, 2018, 2:48 p.m. UTC | #6
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
Jarkko Sakkinen Nov. 5, 2018, 5:10 p.m. UTC | #7
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
Jarkko Sakkinen Nov. 5, 2018, 5:13 p.m. UTC | #8
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
diff mbox series

Patch

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