[v4,1/6] tpm: dynamically allocate active_banks array
diff mbox series

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

Commit Message

Roberto Sassu Nov. 6, 2018, 3:01 p.m. UTC
This patch removes the hard-coded limit of the active_banks array size.
It stores in the tpm_chip structure the number of active PCR banks,
determined in tpm2_get_pcr_allocation(), and replaces the static array
with a pointer to a dynamically allocated array.

As a consequence of the introduction of nr_active_banks, tpm_pcr_extend()
does not check anymore if the algorithm stored in tpm_chip is equal to
zero. The active_banks array always contains valid algorithms.

Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
PCR banks")

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

Comments

Nayna Jain Nov. 7, 2018, 6:14 a.m. UTC | #1
On 11/06/2018 08:31 PM, Roberto Sassu wrote:
> This patch removes the hard-coded limit of the active_banks array size.


The hard-coded limit in static array active_banks[] represents the 
maximum possible banks.
A TPM might have three banks, but only one bank may be active.

To confirm my understanding, is the idea for this patch is to 
dynamically identify the number of possible banks or the number of 
active banks ?


> It stores in the tpm_chip structure the number of active PCR banks,
> determined in tpm2_get_pcr_allocation(), and replaces the static array
> with a pointer to a dynamically allocated array.
>
> As a consequence of the introduction of nr_active_banks, tpm_pcr_extend()
> does not check anymore if the algorithm stored in tpm_chip is equal to
> zero. The active_banks array always contains valid algorithms.
>
> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
> PCR banks")
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>   drivers/char/tpm/tpm-chip.c      |  1 +
>   drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
>   drivers/char/tpm/tpm.h           |  3 ++-
>   drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
>   4 files changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 46caadca916a..2a9e8b744436 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>   	kfree(chip->log.bios_event_log);
>   	kfree(chip->work_space.context_buf);
>   	kfree(chip->work_space.session_buf);
> +	kfree(chip->active_banks);
>   	kfree(chip);
>   }
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 1a803b0cf980..ba7ca6b3e664 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -1039,8 +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)];
> -	u32 count = 0;
> +	struct tpm2_digest *digest_list;
>   	int i;
>
>   	chip = tpm_find_get_ops(chip);
> @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>   		return -ENODEV;
>
>   	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> -		memset(digest_list, 0, sizeof(digest_list));
> +		digest_list = kmalloc_array(chip->nr_active_banks,
> +					    sizeof(*digest_list), GFP_KERNEL);
> +		if (!digest_list)
> +			return -ENOMEM;
>
> -		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
> -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
> +		memset(digest_list, 0,
> +		       chip->nr_active_banks * sizeof(*digest_list));
> +
> +		for (i = 0; i < chip->nr_active_banks; i++) {
>   			digest_list[i].alg_id = chip->active_banks[i];
>   			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
> -			count++;
>   		}
>
> -		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
> +		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
> +				     digest_list);
> +		kfree(digest_list);
>   		tpm_put_ops(chip);
>   		return rc;
>   	}
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f3501d05264f..98368c3a6ff7 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -248,7 +248,8 @@ struct tpm_chip {
>   	const struct attribute_group *groups[3];
>   	unsigned int groups_cnt;
>
> -	u16 active_banks[7];
> +	u32 nr_active_banks;
> +	u16 *active_banks;
>   #ifdef CONFIG_ACPI
>   	acpi_handle acpi_dev_handle;
>   	char ppi_version[TPM_PPI_VERSION_LEN + 1];
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index c31b490bd41d..533089cede07 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
>   	int i;
>   	int j;
>
> -	if (count > ARRAY_SIZE(chip->active_banks))
> +	if (count > chip->nr_active_banks)
>   		return -EINVAL;
>
>   	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
> @@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>   	void *marker;
>   	void *end;
>   	void *pcr_select_offset;
> -	unsigned int count;
>   	u32 sizeof_pcr_selection;
>   	u32 rsp_len;
>   	int rc;
> @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>   	if (rc)
>   		goto out;
>
> -	count = be32_to_cpup(
> +	chip->nr_active_banks = be32_to_cpup(
>   		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);


As per my understanding, the count in the TPML_PCR_SELECTION represent 
the number of possible banks and not the number of active banks.
TCG Structures Spec for TPM 2.0 - Table 102 mentions this as explanation 
of #TPM_RC_SIZE.

Thanks & Regards,
     - Nayna


>
> -	if (count > ARRAY_SIZE(chip->active_banks)) {
> -		rc = -ENODEV;
> +	chip->active_banks = kmalloc_array(chip->nr_active_banks,
> +					   sizeof(*chip->active_banks),
> +					   GFP_KERNEL);
> +	if (!chip->active_banks) {
> +		rc = -ENOMEM;
>   		goto out;
>   	}
>
> @@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>   	rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>   	end = &buf.data[rsp_len];
>
> -	for (i = 0; i < count; i++) {
> +	for (i = 0; i < chip->nr_active_banks; i++) {
>   		pcr_select_offset = marker +
>   			offsetof(struct tpm2_pcr_selection, size_of_select);
>   		if (pcr_select_offset >= end) {
> @@ -908,9 +910,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;
Roberto Sassu Nov. 7, 2018, 9:41 a.m. UTC | #2
On 11/7/2018 7:14 AM, Nayna Jain wrote:
> 
> 
> On 11/06/2018 08:31 PM, Roberto Sassu wrote:
>> This patch removes the hard-coded limit of the active_banks array size.
> 
> 
> The hard-coded limit in static array active_banks[] represents the 
> maximum possible banks.
> A TPM might have three banks, but only one bank may be active.
> 
> To confirm my understanding, is the idea for this patch is to 
> dynamically identify the number of possible banks or the number of 
> active banks ?

The idea is to dynamically identify the number of active banks.

In the TPM Commands specification (section 30.2.1), I found:

TPM_CAP_PCRS – Returns the current allocation of PCR in a
TPML_PCR_SELECTION.

You mentioned:

#TPM_RC_SIZE response code when count is greater
than the possible number of banks

but TPML_PCR_SELECTION is provided by the TPM.

Roberto


>> It stores in the tpm_chip structure the number of active PCR banks,
>> determined in tpm2_get_pcr_allocation(), and replaces the static array
>> with a pointer to a dynamically allocated array.
>>
>> As a consequence of the introduction of nr_active_banks, tpm_pcr_extend()
>> does not check anymore if the algorithm stored in tpm_chip is equal to
>> zero. The active_banks array always contains valid algorithms.
>>
>> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
>> PCR banks")
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>>   drivers/char/tpm/tpm-chip.c      |  1 +
>>   drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
>>   drivers/char/tpm/tpm.h           |  3 ++-
>>   drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
>>   4 files changed, 23 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 46caadca916a..2a9e8b744436 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>>       kfree(chip->log.bios_event_log);
>>       kfree(chip->work_space.context_buf);
>>       kfree(chip->work_space.session_buf);
>> +    kfree(chip->active_banks);
>>       kfree(chip);
>>   }
>>
>> diff --git a/drivers/char/tpm/tpm-interface.c 
>> b/drivers/char/tpm/tpm-interface.c
>> index 1a803b0cf980..ba7ca6b3e664 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -1039,8 +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)];
>> -    u32 count = 0;
>> +    struct tpm2_digest *digest_list;
>>       int i;
>>
>>       chip = tpm_find_get_ops(chip);
>> @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, int 
>> pcr_idx, const u8 *hash)
>>           return -ENODEV;
>>
>>       if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>> -        memset(digest_list, 0, sizeof(digest_list));
>> +        digest_list = kmalloc_array(chip->nr_active_banks,
>> +                        sizeof(*digest_list), GFP_KERNEL);
>> +        if (!digest_list)
>> +            return -ENOMEM;
>>
>> -        for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>> -                chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>> +        memset(digest_list, 0,
>> +               chip->nr_active_banks * sizeof(*digest_list));
>> +
>> +        for (i = 0; i < chip->nr_active_banks; i++) {
>>               digest_list[i].alg_id = chip->active_banks[i];
>>               memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>> -            count++;
>>           }
>>
>> -        rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
>> +        rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
>> +                     digest_list);
>> +        kfree(digest_list);
>>           tpm_put_ops(chip);
>>           return rc;
>>       }
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index f3501d05264f..98368c3a6ff7 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -248,7 +248,8 @@ struct tpm_chip {
>>       const struct attribute_group *groups[3];
>>       unsigned int groups_cnt;
>>
>> -    u16 active_banks[7];
>> +    u32 nr_active_banks;
>> +    u16 *active_banks;
>>   #ifdef CONFIG_ACPI
>>       acpi_handle acpi_dev_handle;
>>       char ppi_version[TPM_PPI_VERSION_LEN + 1];
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index c31b490bd41d..533089cede07 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int 
>> pcr_idx, u32 count,
>>       int i;
>>       int j;
>>
>> -    if (count > ARRAY_SIZE(chip->active_banks))
>> +    if (count > chip->nr_active_banks)
>>           return -EINVAL;
>>
>>       rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>> @@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>> tpm_chip *chip)
>>       void *marker;
>>       void *end;
>>       void *pcr_select_offset;
>> -    unsigned int count;
>>       u32 sizeof_pcr_selection;
>>       u32 rsp_len;
>>       int rc;
>> @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>> tpm_chip *chip)
>>       if (rc)
>>           goto out;
>>
>> -    count = be32_to_cpup(
>> +    chip->nr_active_banks = be32_to_cpup(
>>           (__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
> 
> 
> As per my understanding, the count in the TPML_PCR_SELECTION represent 
> the number of possible banks and not the number of active banks.
> TCG Structures Spec for TPM 2.0 - Table 102 mentions this as explanation 
> of #TPM_RC_SIZE.
> 
> Thanks & Regards,
>      - Nayna
> 
> 
>>
>> -    if (count > ARRAY_SIZE(chip->active_banks)) {
>> -        rc = -ENODEV;
>> +    chip->active_banks = kmalloc_array(chip->nr_active_banks,
>> +                       sizeof(*chip->active_banks),
>> +                       GFP_KERNEL);
>> +    if (!chip->active_banks) {
>> +        rc = -ENOMEM;
>>           goto out;
>>       }
>>
>> @@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>> tpm_chip *chip)
>>       rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>>       end = &buf.data[rsp_len];
>>
>> -    for (i = 0; i < count; i++) {
>> +    for (i = 0; i < chip->nr_active_banks; i++) {
>>           pcr_select_offset = marker +
>>               offsetof(struct tpm2_pcr_selection, size_of_select);
>>           if (pcr_select_offset >= end) {
>> @@ -908,9 +910,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;
>
Mimi Zohar Nov. 7, 2018, 11:10 a.m. UTC | #3
On Wed, 2018-11-07 at 11:44 +0530, Nayna Jain wrote:
> On 11/06/2018 08:31 PM, Roberto Sassu wrote:


> > @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
> >   	if (rc)
> >   		goto out;
> >
> > -	count = be32_to_cpup(
> > +	chip->nr_active_banks = be32_to_cpup(
> >   		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
> 
> 
> As per my understanding, the count in the TPML_PCR_SELECTION represent 
> the number of possible banks and not the number of active banks.
> TCG Structures Spec for TPM 2.0 - Table 102 mentions this as explanation 
> of #TPM_RC_SIZE.

Instead of storing the result in a local variable, the only change
here is saving the result in the chip info (nr_active_banks).
 Everything else remains the same.


> >
> > -	if (count > ARRAY_SIZE(chip->active_banks)) {
> > -		rc = -ENODEV;
> > +	chip->active_banks = kmalloc_array(chip->nr_active_banks,
> > +					   sizeof(*chip->active_banks),
> > +					   GFP_KERNEL);

With this change, the exact number of banks can be allocated, as done
here.  Nice! 

Mimi

> > +	if (!chip->active_banks) {
> > +		rc = -ENOMEM;
> >   		goto out;
> >   	}
> >
>
Jarkko Sakkinen Nov. 8, 2018, 1:46 p.m. UTC | #4
Orrayn Tue, Nov 06, 2018 at 04:01:54PM +0100, Roberto Sassu wrote:
> This patch removes the hard-coded limit of the active_banks array size.
> It stores in the tpm_chip structure the number of active PCR banks,
> determined in tpm2_get_pcr_allocation(), and replaces the static array
> with a pointer to a dynamically allocated array.
> 
> As a consequence of the introduction of nr_active_banks, tpm_pcr_extend()
> does not check anymore if the algorithm stored in tpm_chip is equal to
> zero. The active_banks array always contains valid algorithms.
> 
> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
> PCR banks")
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  drivers/char/tpm/tpm-chip.c      |  1 +
>  drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
>  drivers/char/tpm/tpm.h           |  3 ++-
>  drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
>  4 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 46caadca916a..2a9e8b744436 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>  	kfree(chip->log.bios_event_log);
>  	kfree(chip->work_space.context_buf);
>  	kfree(chip->work_space.session_buf);
> +	kfree(chip->active_banks);
>  	kfree(chip);
>  }
>  
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 1a803b0cf980..ba7ca6b3e664 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -1039,8 +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)];
> -	u32 count = 0;
> +	struct tpm2_digest *digest_list;
>  	int i;
>  
>  	chip = tpm_find_get_ops(chip);
> @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>  		return -ENODEV;

Should take digest_list as input. Probably callers could re-use the
same digest_list array multiple times?

Move struct tpm_chip to include/linux/tpm.h so that the caller can query
nr_active_banks and active_banks and can create the digest array by
itself. Lets do this right at once now that this is being restructured.

>  
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> -		memset(digest_list, 0, sizeof(digest_list));
> +		digest_list = kmalloc_array(chip->nr_active_banks,
> +					    sizeof(*digest_list), GFP_KERNEL);
> +		if (!digest_list)
> +			return -ENOMEM;
>  
> -		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
> -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
> +		memset(digest_list, 0,
> +		       chip->nr_active_banks * sizeof(*digest_list));

You should use kcalloc() to allocate digest_list.

> +
> +		for (i = 0; i < chip->nr_active_banks; i++) {
>  			digest_list[i].alg_id = chip->active_banks[i];
>  			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
> -			count++;
>  		}
>  
> -		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
> +		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
> +				     digest_list);
> +		kfree(digest_list);
>  		tpm_put_ops(chip);
>  		return rc;
>  	}
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f3501d05264f..98368c3a6ff7 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -248,7 +248,8 @@ struct tpm_chip {
>  	const struct attribute_group *groups[3];
>  	unsigned int groups_cnt;
>  
> -	u16 active_banks[7];
> +	u32 nr_active_banks;
> +	u16 *active_banks;
>  #ifdef CONFIG_ACPI
>  	acpi_handle acpi_dev_handle;
>  	char ppi_version[TPM_PPI_VERSION_LEN + 1];
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index c31b490bd41d..533089cede07 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
>  	int i;
>  	int j;
>  
> -	if (count > ARRAY_SIZE(chip->active_banks))
> +	if (count > chip->nr_active_banks)
>  		return -EINVAL;
>  
>  	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
> @@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>  	void *marker;
>  	void *end;
>  	void *pcr_select_offset;
> -	unsigned int count;
>  	u32 sizeof_pcr_selection;
>  	u32 rsp_len;
>  	int rc;
> @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>  	if (rc)
>  		goto out;
>  
> -	count = be32_to_cpup(
> +	chip->nr_active_banks = be32_to_cpup(
>  		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
>  
> -	if (count > ARRAY_SIZE(chip->active_banks)) {
> -		rc = -ENODEV;
> +	chip->active_banks = kmalloc_array(chip->nr_active_banks,
> +					   sizeof(*chip->active_banks),
> +					   GFP_KERNEL);
> +	if (!chip->active_banks) {
> +		rc = -ENOMEM;
>  		goto out;
>  	}
>  
> @@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>  	rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>  	end = &buf.data[rsp_len];
>  
> -	for (i = 0; i < count; i++) {
> +	for (i = 0; i < chip->nr_active_banks; i++) {
>  		pcr_select_offset = marker +
>  			offsetof(struct tpm2_pcr_selection, size_of_select);
>  		if (pcr_select_offset >= end) {
> @@ -908,9 +910,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
> 

/Jarkko
Nayna Jain Nov. 8, 2018, 1:50 p.m. UTC | #5
On 11/07/2018 03:11 PM, Roberto Sassu wrote:
> On 11/7/2018 7:14 AM, Nayna Jain wrote:
>>
>>
>> On 11/06/2018 08:31 PM, Roberto Sassu wrote:
>>> This patch removes the hard-coded limit of the active_banks array size.
>>
>>
>> The hard-coded limit in static array active_banks[] represents the 
>> maximum possible banks.
>> A TPM might have three banks, but only one bank may be active.
>>
>> To confirm my understanding, is the idea for this patch is to 
>> dynamically identify the number of possible banks or the number of 
>> active banks ?
>
> The idea is to dynamically identify the number of active banks.
>
> In the TPM Commands specification (section 30.2.1), I found:
>
> TPM_CAP_PCRS – Returns the current allocation of PCR in a
> TPML_PCR_SELECTION.
>
> You mentioned:
>
> #TPM_RC_SIZE response code when count is greater
> than the possible number of banks
>
> but TPML_PCR_SELECTION is provided by the TPM.

Based on a discussion with Ken, the count in the TPML_PCR_SELECTION 
returns the number of possible algorithms supported. In the example 
below, two possible algorithms - SHA1 and SHA256 - are returned.

# /usr/local/bin/tssgetcapability -cap 5
2 PCR selections
     hash TPM_ALG_SHA1
     TPMS_PCR_SELECTION length 3
     ff ff ff
     hash TPM_ALG_SHA256
     TPMS_PCR_SELECTION length 3
     00 00 00

The pcr_select fields - "ff ff ff" and "00 00 00" - are bit masks for 
the enabled PCRs. The SHA1 bank is enabled for all PCRs (0-23), while 
the SHA256 bank is not enabled.

The current code works, but it unnecessarily extends some banks. Instead 
of basing the number of active banks on the number of algorithms 
returned, it should be based on the pcr_select field.

    - Mimi & Nayna


>
> Roberto
>
>
>>> It stores in the tpm_chip structure the number of active PCR banks,
>>> determined in tpm2_get_pcr_allocation(), and replaces the static array
>>> with a pointer to a dynamically allocated array.
>>>
>>> As a consequence of the introduction of nr_active_banks, 
>>> tpm_pcr_extend()
>>> does not check anymore if the algorithm stored in tpm_chip is equal to
>>> zero. The active_banks array always contains valid algorithms.
>>>
>>> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
>>> PCR banks")
>>>
>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>> ---
>>>   drivers/char/tpm/tpm-chip.c      |  1 +
>>>   drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
>>>   drivers/char/tpm/tpm.h           |  3 ++-
>>>   drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
>>>   4 files changed, 23 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>>> index 46caadca916a..2a9e8b744436 100644
>>> --- a/drivers/char/tpm/tpm-chip.c
>>> +++ b/drivers/char/tpm/tpm-chip.c
>>> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>>>       kfree(chip->log.bios_event_log);
>>>       kfree(chip->work_space.context_buf);
>>>       kfree(chip->work_space.session_buf);
>>> +    kfree(chip->active_banks);
>>>       kfree(chip);
>>>   }
>>>
>>> diff --git a/drivers/char/tpm/tpm-interface.c 
>>> b/drivers/char/tpm/tpm-interface.c
>>> index 1a803b0cf980..ba7ca6b3e664 100644
>>> --- a/drivers/char/tpm/tpm-interface.c
>>> +++ b/drivers/char/tpm/tpm-interface.c
>>> @@ -1039,8 +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)];
>>> -    u32 count = 0;
>>> +    struct tpm2_digest *digest_list;
>>>       int i;
>>>
>>>       chip = tpm_find_get_ops(chip);
>>> @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, 
>>> int pcr_idx, const u8 *hash)
>>>           return -ENODEV;
>>>
>>>       if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>>> -        memset(digest_list, 0, sizeof(digest_list));
>>> +        digest_list = kmalloc_array(chip->nr_active_banks,
>>> +                        sizeof(*digest_list), GFP_KERNEL);
>>> +        if (!digest_list)
>>> +            return -ENOMEM;
>>>
>>> -        for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>>> -                chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>>> +        memset(digest_list, 0,
>>> +               chip->nr_active_banks * sizeof(*digest_list));
>>> +
>>> +        for (i = 0; i < chip->nr_active_banks; i++) {
>>>               digest_list[i].alg_id = chip->active_banks[i];
>>>               memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>>> -            count++;
>>>           }
>>>
>>> -        rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
>>> +        rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
>>> +                     digest_list);
>>> +        kfree(digest_list);
>>>           tpm_put_ops(chip);
>>>           return rc;
>>>       }
>>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>>> index f3501d05264f..98368c3a6ff7 100644
>>> --- a/drivers/char/tpm/tpm.h
>>> +++ b/drivers/char/tpm/tpm.h
>>> @@ -248,7 +248,8 @@ struct tpm_chip {
>>>       const struct attribute_group *groups[3];
>>>       unsigned int groups_cnt;
>>>
>>> -    u16 active_banks[7];
>>> +    u32 nr_active_banks;
>>> +    u16 *active_banks;
>>>   #ifdef CONFIG_ACPI
>>>       acpi_handle acpi_dev_handle;
>>>       char ppi_version[TPM_PPI_VERSION_LEN + 1];
>>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>>> index c31b490bd41d..533089cede07 100644
>>> --- a/drivers/char/tpm/tpm2-cmd.c
>>> +++ b/drivers/char/tpm/tpm2-cmd.c
>>> @@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int 
>>> pcr_idx, u32 count,
>>>       int i;
>>>       int j;
>>>
>>> -    if (count > ARRAY_SIZE(chip->active_banks))
>>> +    if (count > chip->nr_active_banks)
>>>           return -EINVAL;
>>>
>>>       rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>>> @@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>>> tpm_chip *chip)
>>>       void *marker;
>>>       void *end;
>>>       void *pcr_select_offset;
>>> -    unsigned int count;
>>>       u32 sizeof_pcr_selection;
>>>       u32 rsp_len;
>>>       int rc;
>>> @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>>> tpm_chip *chip)
>>>       if (rc)
>>>           goto out;
>>>
>>> -    count = be32_to_cpup(
>>> +    chip->nr_active_banks = be32_to_cpup(
>>>           (__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
>>
>>
>> As per my understanding, the count in the TPML_PCR_SELECTION 
>> represent the number of possible banks and not the number of active 
>> banks.
>> TCG Structures Spec for TPM 2.0 - Table 102 mentions this as 
>> explanation of #TPM_RC_SIZE.
>>
>> Thanks & Regards,
>>      - Nayna
>>
>>
>>>
>>> -    if (count > ARRAY_SIZE(chip->active_banks)) {
>>> -        rc = -ENODEV;
>>> +    chip->active_banks = kmalloc_array(chip->nr_active_banks,
>>> +                       sizeof(*chip->active_banks),
>>> +                       GFP_KERNEL);
>>> +    if (!chip->active_banks) {
>>> +        rc = -ENOMEM;
>>>           goto out;
>>>       }
>>>
>>> @@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>>> tpm_chip *chip)
>>>       rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>>>       end = &buf.data[rsp_len];
>>>
>>> -    for (i = 0; i < count; i++) {
>>> +    for (i = 0; i < chip->nr_active_banks; i++) {
>>>           pcr_select_offset = marker +
>>>               offsetof(struct tpm2_pcr_selection, size_of_select);
>>>           if (pcr_select_offset >= end) {
>>> @@ -908,9 +910,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;
>>
>
Roberto Sassu Nov. 8, 2018, 2:24 p.m. UTC | #6
On 11/8/2018 2:46 PM, Jarkko Sakkinen wrote:
> Orrayn Tue, Nov 06, 2018 at 04:01:54PM +0100, Roberto Sassu wrote:
>> This patch removes the hard-coded limit of the active_banks array size.
>> It stores in the tpm_chip structure the number of active PCR banks,
>> determined in tpm2_get_pcr_allocation(), and replaces the static array
>> with a pointer to a dynamically allocated array.
>>
>> As a consequence of the introduction of nr_active_banks, tpm_pcr_extend()
>> does not check anymore if the algorithm stored in tpm_chip is equal to
>> zero. The active_banks array always contains valid algorithms.
>>
>> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
>> PCR banks")
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>>   drivers/char/tpm/tpm-chip.c      |  1 +
>>   drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
>>   drivers/char/tpm/tpm.h           |  3 ++-
>>   drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
>>   4 files changed, 23 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 46caadca916a..2a9e8b744436 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>>   	kfree(chip->log.bios_event_log);
>>   	kfree(chip->work_space.context_buf);
>>   	kfree(chip->work_space.session_buf);
>> +	kfree(chip->active_banks);
>>   	kfree(chip);
>>   }
>>   
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index 1a803b0cf980..ba7ca6b3e664 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -1039,8 +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)];
>> -	u32 count = 0;
>> +	struct tpm2_digest *digest_list;
>>   	int i;
>>   
>>   	chip = tpm_find_get_ops(chip);
>> @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>>   		return -ENODEV;
> 
> Should take digest_list as input. Probably callers could re-use the
> same digest_list array multiple times?

Should I include the patch for tpm_pcr_extend() in this patch set, even
if the set fixes the digest size issue?

Roberto


> Move struct tpm_chip to include/linux/tpm.h so that the caller can query
> nr_active_banks and active_banks and can create the digest array by
> itself. Lets do this right at once now that this is being restructured.
> 
>>   
>>   	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>> -		memset(digest_list, 0, sizeof(digest_list));
>> +		digest_list = kmalloc_array(chip->nr_active_banks,
>> +					    sizeof(*digest_list), GFP_KERNEL);
>> +		if (!digest_list)
>> +			return -ENOMEM;
>>   
>> -		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>> -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>> +		memset(digest_list, 0,
>> +		       chip->nr_active_banks * sizeof(*digest_list));
> 
> You should use kcalloc() to allocate digest_list.
> 
>> +
>> +		for (i = 0; i < chip->nr_active_banks; i++) {
>>   			digest_list[i].alg_id = chip->active_banks[i];
>>   			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>> -			count++;
>>   		}
>>   
>> -		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
>> +		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
>> +				     digest_list);
>> +		kfree(digest_list);
>>   		tpm_put_ops(chip);
>>   		return rc;
>>   	}
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index f3501d05264f..98368c3a6ff7 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -248,7 +248,8 @@ struct tpm_chip {
>>   	const struct attribute_group *groups[3];
>>   	unsigned int groups_cnt;
>>   
>> -	u16 active_banks[7];
>> +	u32 nr_active_banks;
>> +	u16 *active_banks;
>>   #ifdef CONFIG_ACPI
>>   	acpi_handle acpi_dev_handle;
>>   	char ppi_version[TPM_PPI_VERSION_LEN + 1];
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index c31b490bd41d..533089cede07 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
>>   	int i;
>>   	int j;
>>   
>> -	if (count > ARRAY_SIZE(chip->active_banks))
>> +	if (count > chip->nr_active_banks)
>>   		return -EINVAL;
>>   
>>   	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>> @@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	void *marker;
>>   	void *end;
>>   	void *pcr_select_offset;
>> -	unsigned int count;
>>   	u32 sizeof_pcr_selection;
>>   	u32 rsp_len;
>>   	int rc;
>> @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	if (rc)
>>   		goto out;
>>   
>> -	count = be32_to_cpup(
>> +	chip->nr_active_banks = be32_to_cpup(
>>   		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
>>   
>> -	if (count > ARRAY_SIZE(chip->active_banks)) {
>> -		rc = -ENODEV;
>> +	chip->active_banks = kmalloc_array(chip->nr_active_banks,
>> +					   sizeof(*chip->active_banks),
>> +					   GFP_KERNEL);
>> +	if (!chip->active_banks) {
>> +		rc = -ENOMEM;
>>   		goto out;
>>   	}
>>   
>> @@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>>   	end = &buf.data[rsp_len];
>>   
>> -	for (i = 0; i < count; i++) {
>> +	for (i = 0; i < chip->nr_active_banks; i++) {
>>   		pcr_select_offset = marker +
>>   			offsetof(struct tpm2_pcr_selection, size_of_select);
>>   		if (pcr_select_offset >= end) {
>> @@ -908,9 +910,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
>>
> 
> /Jarkko
>
Roberto Sassu Nov. 8, 2018, 2:40 p.m. UTC | #7
On 11/8/2018 2:50 PM, Nayna Jain wrote:
> 
> 
> On 11/07/2018 03:11 PM, Roberto Sassu wrote:
>> On 11/7/2018 7:14 AM, Nayna Jain wrote:
>>>
>>>
>>> On 11/06/2018 08:31 PM, Roberto Sassu wrote:
>>>> This patch removes the hard-coded limit of the active_banks array size.
>>>
>>>
>>> The hard-coded limit in static array active_banks[] represents the 
>>> maximum possible banks.
>>> A TPM might have three banks, but only one bank may be active.
>>>
>>> To confirm my understanding, is the idea for this patch is to 
>>> dynamically identify the number of possible banks or the number of 
>>> active banks ?
>>
>> The idea is to dynamically identify the number of active banks.
>>
>> In the TPM Commands specification (section 30.2.1), I found:
>>
>> TPM_CAP_PCRS – Returns the current allocation of PCR in a
>> TPML_PCR_SELECTION.
>>
>> You mentioned:
>>
>> #TPM_RC_SIZE response code when count is greater
>> than the possible number of banks
>>
>> but TPML_PCR_SELECTION is provided by the TPM.
> 
> Based on a discussion with Ken, the count in the TPML_PCR_SELECTION 
> returns the number of possible algorithms supported. In the example 
> below, two possible algorithms - SHA1 and SHA256 - are returned.
> 
> # /usr/local/bin/tssgetcapability -cap 5
> 2 PCR selections
>      hash TPM_ALG_SHA1
>      TPMS_PCR_SELECTION length 3
>      ff ff ff
>      hash TPM_ALG_SHA256
>      TPMS_PCR_SELECTION length 3
>      00 00 00
> 
> The pcr_select fields - "ff ff ff" and "00 00 00" - are bit masks for 
> the enabled PCRs. The SHA1 bank is enabled for all PCRs (0-23), while 
> the SHA256 bank is not enabled.
> 
> The current code works, but it unnecessarily extends some banks. Instead 
> of basing the number of active banks on the number of algorithms 
> returned, it should be based on the pcr_select field.

Thanks. I will add a bank if at least one bit is set in the pcr_select
mask.

Roberto


>     - Mimi & Nayna
> 
> 
>>
>> Roberto
>>
>>
>>>> It stores in the tpm_chip structure the number of active PCR banks,
>>>> determined in tpm2_get_pcr_allocation(), and replaces the static array
>>>> with a pointer to a dynamically allocated array.
>>>>
>>>> As a consequence of the introduction of nr_active_banks, 
>>>> tpm_pcr_extend()
>>>> does not check anymore if the algorithm stored in tpm_chip is equal to
>>>> zero. The active_banks array always contains valid algorithms.
>>>>
>>>> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
>>>> PCR banks")
>>>>
>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>> ---
>>>>   drivers/char/tpm/tpm-chip.c      |  1 +
>>>>   drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
>>>>   drivers/char/tpm/tpm.h           |  3 ++-
>>>>   drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
>>>>   4 files changed, 23 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>>>> index 46caadca916a..2a9e8b744436 100644
>>>> --- a/drivers/char/tpm/tpm-chip.c
>>>> +++ b/drivers/char/tpm/tpm-chip.c
>>>> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>>>>       kfree(chip->log.bios_event_log);
>>>>       kfree(chip->work_space.context_buf);
>>>>       kfree(chip->work_space.session_buf);
>>>> +    kfree(chip->active_banks);
>>>>       kfree(chip);
>>>>   }
>>>>
>>>> diff --git a/drivers/char/tpm/tpm-interface.c 
>>>> b/drivers/char/tpm/tpm-interface.c
>>>> index 1a803b0cf980..ba7ca6b3e664 100644
>>>> --- a/drivers/char/tpm/tpm-interface.c
>>>> +++ b/drivers/char/tpm/tpm-interface.c
>>>> @@ -1039,8 +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)];
>>>> -    u32 count = 0;
>>>> +    struct tpm2_digest *digest_list;
>>>>       int i;
>>>>
>>>>       chip = tpm_find_get_ops(chip);
>>>> @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, 
>>>> int pcr_idx, const u8 *hash)
>>>>           return -ENODEV;
>>>>
>>>>       if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>>>> -        memset(digest_list, 0, sizeof(digest_list));
>>>> +        digest_list = kmalloc_array(chip->nr_active_banks,
>>>> +                        sizeof(*digest_list), GFP_KERNEL);
>>>> +        if (!digest_list)
>>>> +            return -ENOMEM;
>>>>
>>>> -        for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>>>> -                chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>>>> +        memset(digest_list, 0,
>>>> +               chip->nr_active_banks * sizeof(*digest_list));
>>>> +
>>>> +        for (i = 0; i < chip->nr_active_banks; i++) {
>>>>               digest_list[i].alg_id = chip->active_banks[i];
>>>>               memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>>>> -            count++;
>>>>           }
>>>>
>>>> -        rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
>>>> +        rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
>>>> +                     digest_list);
>>>> +        kfree(digest_list);
>>>>           tpm_put_ops(chip);
>>>>           return rc;
>>>>       }
>>>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>>>> index f3501d05264f..98368c3a6ff7 100644
>>>> --- a/drivers/char/tpm/tpm.h
>>>> +++ b/drivers/char/tpm/tpm.h
>>>> @@ -248,7 +248,8 @@ struct tpm_chip {
>>>>       const struct attribute_group *groups[3];
>>>>       unsigned int groups_cnt;
>>>>
>>>> -    u16 active_banks[7];
>>>> +    u32 nr_active_banks;
>>>> +    u16 *active_banks;
>>>>   #ifdef CONFIG_ACPI
>>>>       acpi_handle acpi_dev_handle;
>>>>       char ppi_version[TPM_PPI_VERSION_LEN + 1];
>>>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>>>> index c31b490bd41d..533089cede07 100644
>>>> --- a/drivers/char/tpm/tpm2-cmd.c
>>>> +++ b/drivers/char/tpm/tpm2-cmd.c
>>>> @@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int 
>>>> pcr_idx, u32 count,
>>>>       int i;
>>>>       int j;
>>>>
>>>> -    if (count > ARRAY_SIZE(chip->active_banks))
>>>> +    if (count > chip->nr_active_banks)
>>>>           return -EINVAL;
>>>>
>>>>       rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>>>> @@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>>>> tpm_chip *chip)
>>>>       void *marker;
>>>>       void *end;
>>>>       void *pcr_select_offset;
>>>> -    unsigned int count;
>>>>       u32 sizeof_pcr_selection;
>>>>       u32 rsp_len;
>>>>       int rc;
>>>> @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>>>> tpm_chip *chip)
>>>>       if (rc)
>>>>           goto out;
>>>>
>>>> -    count = be32_to_cpup(
>>>> +    chip->nr_active_banks = be32_to_cpup(
>>>>           (__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
>>>
>>>
>>> As per my understanding, the count in the TPML_PCR_SELECTION 
>>> represent the number of possible banks and not the number of active 
>>> banks.
>>> TCG Structures Spec for TPM 2.0 - Table 102 mentions this as 
>>> explanation of #TPM_RC_SIZE.
>>>
>>> Thanks & Regards,
>>>      - Nayna
>>>
>>>
>>>>
>>>> -    if (count > ARRAY_SIZE(chip->active_banks)) {
>>>> -        rc = -ENODEV;
>>>> +    chip->active_banks = kmalloc_array(chip->nr_active_banks,
>>>> +                       sizeof(*chip->active_banks),
>>>> +                       GFP_KERNEL);
>>>> +    if (!chip->active_banks) {
>>>> +        rc = -ENOMEM;
>>>>           goto out;
>>>>       }
>>>>
>>>> @@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>>>> tpm_chip *chip)
>>>>       rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>>>>       end = &buf.data[rsp_len];
>>>>
>>>> -    for (i = 0; i < count; i++) {
>>>> +    for (i = 0; i < chip->nr_active_banks; i++) {
>>>>           pcr_select_offset = marker +
>>>>               offsetof(struct tpm2_pcr_selection, size_of_select);
>>>>           if (pcr_select_offset >= end) {
>>>> @@ -908,9 +910,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;
>>>
>>
>
Jarkko Sakkinen Nov. 8, 2018, 3:21 p.m. UTC | #8
On Thu, Nov 08, 2018 at 07:20:51PM +0530, Nayna Jain wrote:
> Based on a discussion with Ken, the count in the TPML_PCR_SELECTION returns
> the number of possible algorithms supported. In the example below, two
> possible algorithms - SHA1 and SHA256 - are returned.
> 
> # /usr/local/bin/tssgetcapability -cap 5
> 2 PCR selections
>     hash TPM_ALG_SHA1
>     TPMS_PCR_SELECTION length 3
>     ff ff ff
>     hash TPM_ALG_SHA256
>     TPMS_PCR_SELECTION length 3
>     00 00 00
> 
> The pcr_select fields - "ff ff ff" and "00 00 00" - are bit masks for the
> enabled PCRs. The SHA1 bank is enabled for all PCRs (0-23), while the SHA256
> bank is not enabled.
> 
> The current code works, but it unnecessarily extends some banks. Instead of
> basing the number of active banks on the number of algorithms returned, it
> should be based on the pcr_select field.
> 
>    - Mimi & Nayna

I would just allocate array of the size of possible banks and grow
nr_active_banks for active algorithms to keep the code simple because
we are talking about insignificant amount of wasted space (might be
even zero bytes given how kernel allocators works)>

/Jarkko
Jarkko Sakkinen Nov. 8, 2018, 3:22 p.m. UTC | #9
On Thu, Nov 08, 2018 at 03:24:46PM +0100, Roberto Sassu wrote:
> Should I include the patch for tpm_pcr_extend() in this patch set, even
> if the set fixes the digest size issue?

Just add some note to the cover letter. Makes sense here to have a
prequel patch for that because otherwise the implementation will be
a bit messy and half-baked.

/Jarkko
Mimi Zohar Nov. 8, 2018, 3:29 p.m. UTC | #10
On Thu, 2018-11-08 at 17:21 +0200, Jarkko Sakkinen wrote:
> On Thu, Nov 08, 2018 at 07:20:51PM +0530, Nayna Jain wrote:
> > Based on a discussion with Ken, the count in the TPML_PCR_SELECTION returns
> > the number of possible algorithms supported. In the example below, two
> > possible algorithms - SHA1 and SHA256 - are returned.
> > 
> > # /usr/local/bin/tssgetcapability -cap 5
> > 2 PCR selections
> >     hash TPM_ALG_SHA1
> >     TPMS_PCR_SELECTION length 3
> >     ff ff ff
> >     hash TPM_ALG_SHA256
> >     TPMS_PCR_SELECTION length 3
> >     00 00 00
> > 
> > The pcr_select fields - "ff ff ff" and "00 00 00" - are bit masks for the
> > enabled PCRs. The SHA1 bank is enabled for all PCRs (0-23), while the SHA256
> > bank is not enabled.
> > 
> > The current code works, but it unnecessarily extends some banks. Instead of
> > basing the number of active banks on the number of algorithms returned, it
> > should be based on the pcr_select field.
> > 
> >    - Mimi & Nayna
> 
> I would just allocate array of the size of possible banks and grow
> nr_active_banks for active algorithms to keep the code simple because
> we are talking about insignificant amount of wasted space (might be
> even zero bytes given how kernel allocators works)>

That's fine.  Remember the memory is just one concern, but the other
concerns are the performance of calculating the unneeded hash and the
TPM performance of including it in the PCR extend.

Mimi
Ken Goldman Nov. 8, 2018, 3:54 p.m. UTC | #11
On 11/8/2018 10:21 AM, Jarkko Sakkinen wrote:

> I would just allocate array of the size of possible banks and grow
> nr_active_banks for active algorithms to keep the code simple because
> we are talking about insignificant amount of wasted space (might be
> even zero bytes given how kernel allocators works)>

Just beware that "possible banks" is tricky.  While 2 is typical, 
getcapability will return a bitmap for each digest algorithm.  This is
also currently 2, but will be 3 in the next TPM, is 4 in the SW TPM,
and is potentially even higher.

Also, account for a TPM that is malicious and can return a count
as high as 0xffffffff.  Range check count.
Jarkko Sakkinen Nov. 8, 2018, 6:57 p.m. UTC | #12
On Thu, Nov 08, 2018 at 10:29:53AM -0500, Mimi Zohar wrote:
> On Thu, 2018-11-08 at 17:21 +0200, Jarkko Sakkinen wrote:
> > On Thu, Nov 08, 2018 at 07:20:51PM +0530, Nayna Jain wrote:
> > > Based on a discussion with Ken, the count in the TPML_PCR_SELECTION returns
> > > the number of possible algorithms supported. In the example below, two
> > > possible algorithms - SHA1 and SHA256 - are returned.
> > > 
> > > # /usr/local/bin/tssgetcapability -cap 5
> > > 2 PCR selections
> > >     hash TPM_ALG_SHA1
> > >     TPMS_PCR_SELECTION length 3
> > >     ff ff ff
> > >     hash TPM_ALG_SHA256
> > >     TPMS_PCR_SELECTION length 3
> > >     00 00 00
> > > 
> > > The pcr_select fields - "ff ff ff" and "00 00 00" - are bit masks for the
> > > enabled PCRs. The SHA1 bank is enabled for all PCRs (0-23), while the SHA256
> > > bank is not enabled.
> > > 
> > > The current code works, but it unnecessarily extends some banks. Instead of
> > > basing the number of active banks on the number of algorithms returned, it
> > > should be based on the pcr_select field.
> > > 
> > >    - Mimi & Nayna
> > 
> > I would just allocate array of the size of possible banks and grow
> > nr_active_banks for active algorithms to keep the code simple because
> > we are talking about insignificant amount of wasted space (might be
> > even zero bytes given how kernel allocators works)>
> 
> That's fine.  Remember the memory is just one concern, but the other
> concerns are the performance of calculating the unneeded hash and the
> TPM performance of including it in the PCR extend.

The driver would initialize only as many entries as are active array and
set nr_active_banks accordingly.

/Jarkko
Roberto Sassu Nov. 13, 2018, 1:34 p.m. UTC | #13
On 11/8/2018 2:46 PM, Jarkko Sakkinen wrote:
> Orrayn Tue, Nov 06, 2018 at 04:01:54PM +0100, Roberto Sassu wrote:
>> This patch removes the hard-coded limit of the active_banks array size.
>> It stores in the tpm_chip structure the number of active PCR banks,
>> determined in tpm2_get_pcr_allocation(), and replaces the static array
>> with a pointer to a dynamically allocated array.
>>
>> As a consequence of the introduction of nr_active_banks, tpm_pcr_extend()
>> does not check anymore if the algorithm stored in tpm_chip is equal to
>> zero. The active_banks array always contains valid algorithms.
>>
>> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
>> PCR banks")
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>>   drivers/char/tpm/tpm-chip.c      |  1 +
>>   drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
>>   drivers/char/tpm/tpm.h           |  3 ++-
>>   drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
>>   4 files changed, 23 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 46caadca916a..2a9e8b744436 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>>   	kfree(chip->log.bios_event_log);
>>   	kfree(chip->work_space.context_buf);
>>   	kfree(chip->work_space.session_buf);
>> +	kfree(chip->active_banks);
>>   	kfree(chip);
>>   }
>>   
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index 1a803b0cf980..ba7ca6b3e664 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -1039,8 +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)];
>> -	u32 count = 0;
>> +	struct tpm2_digest *digest_list;
>>   	int i;
>>   
>>   	chip = tpm_find_get_ops(chip);
>> @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>>   		return -ENODEV;
> 
> Should take digest_list as input. Probably callers could re-use the
> same digest_list array multiple times?
> 
> Move struct tpm_chip to include/linux/tpm.h so that the caller can query
> nr_active_banks and active_banks and can create the digest array by
> itself. Lets do this right at once now that this is being restructured.

I have to move also other structures and #define. Wouldn't be better to
introduce a new function to pass to the caller active_banks and
nr_active_banks?

Roberto


>>   
>>   	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>> -		memset(digest_list, 0, sizeof(digest_list));
>> +		digest_list = kmalloc_array(chip->nr_active_banks,
>> +					    sizeof(*digest_list), GFP_KERNEL);
>> +		if (!digest_list)
>> +			return -ENOMEM;
>>   
>> -		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>> -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>> +		memset(digest_list, 0,
>> +		       chip->nr_active_banks * sizeof(*digest_list));
> 
> You should use kcalloc() to allocate digest_list.
> 
>> +
>> +		for (i = 0; i < chip->nr_active_banks; i++) {
>>   			digest_list[i].alg_id = chip->active_banks[i];
>>   			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>> -			count++;
>>   		}
>>   
>> -		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
>> +		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
>> +				     digest_list);
>> +		kfree(digest_list);
>>   		tpm_put_ops(chip);
>>   		return rc;
>>   	}
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index f3501d05264f..98368c3a6ff7 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -248,7 +248,8 @@ struct tpm_chip {
>>   	const struct attribute_group *groups[3];
>>   	unsigned int groups_cnt;
>>   
>> -	u16 active_banks[7];
>> +	u32 nr_active_banks;
>> +	u16 *active_banks;
>>   #ifdef CONFIG_ACPI
>>   	acpi_handle acpi_dev_handle;
>>   	char ppi_version[TPM_PPI_VERSION_LEN + 1];
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index c31b490bd41d..533089cede07 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
>>   	int i;
>>   	int j;
>>   
>> -	if (count > ARRAY_SIZE(chip->active_banks))
>> +	if (count > chip->nr_active_banks)
>>   		return -EINVAL;
>>   
>>   	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>> @@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	void *marker;
>>   	void *end;
>>   	void *pcr_select_offset;
>> -	unsigned int count;
>>   	u32 sizeof_pcr_selection;
>>   	u32 rsp_len;
>>   	int rc;
>> @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	if (rc)
>>   		goto out;
>>   
>> -	count = be32_to_cpup(
>> +	chip->nr_active_banks = be32_to_cpup(
>>   		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
>>   
>> -	if (count > ARRAY_SIZE(chip->active_banks)) {
>> -		rc = -ENODEV;
>> +	chip->active_banks = kmalloc_array(chip->nr_active_banks,
>> +					   sizeof(*chip->active_banks),
>> +					   GFP_KERNEL);
>> +	if (!chip->active_banks) {
>> +		rc = -ENOMEM;
>>   		goto out;
>>   	}
>>   
>> @@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>>   	end = &buf.data[rsp_len];
>>   
>> -	for (i = 0; i < count; i++) {
>> +	for (i = 0; i < chip->nr_active_banks; i++) {
>>   		pcr_select_offset = marker +
>>   			offsetof(struct tpm2_pcr_selection, size_of_select);
>>   		if (pcr_select_offset >= end) {
>> @@ -908,9 +910,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
>>
> 
> /Jarkko
>
Roberto Sassu Nov. 13, 2018, 1:53 p.m. UTC | #14
Adding in CC Monty and Dave.


On 11/8/2018 2:46 PM, Jarkko Sakkinen wrote:
> Orrayn Tue, Nov 06, 2018 at 04:01:54PM +0100, Roberto Sassu wrote:
>> This patch removes the hard-coded limit of the active_banks array size.
>> It stores in the tpm_chip structure the number of active PCR banks,
>> determined in tpm2_get_pcr_allocation(), and replaces the static array
>> with a pointer to a dynamically allocated array.
>>
>> As a consequence of the introduction of nr_active_banks, tpm_pcr_extend()
>> does not check anymore if the algorithm stored in tpm_chip is equal to
>> zero. The active_banks array always contains valid algorithms.
>>
>> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
>> PCR banks")
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>>   drivers/char/tpm/tpm-chip.c      |  1 +
>>   drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
>>   drivers/char/tpm/tpm.h           |  3 ++-
>>   drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
>>   4 files changed, 23 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 46caadca916a..2a9e8b744436 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>>   	kfree(chip->log.bios_event_log);
>>   	kfree(chip->work_space.context_buf);
>>   	kfree(chip->work_space.session_buf);
>> +	kfree(chip->active_banks);
>>   	kfree(chip);
>>   }
>>   
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index 1a803b0cf980..ba7ca6b3e664 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -1039,8 +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)];
>> -	u32 count = 0;
>> +	struct tpm2_digest *digest_list;
>>   	int i;
>>   
>>   	chip = tpm_find_get_ops(chip);
>> @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>>   		return -ENODEV;
> 
> Should take digest_list as input. Probably callers could re-use the
> same digest_list array multiple times?

I plan to introduce a new structure with the digest size, as Monty
suggested at LSS. The name of the new structure is tpm_bank_list.

The benefit is that we don't have to rely on the TPM driver or the
crypto subsystem to find the digest size for the TPM algorithms
specified by the users of the TPM driver (it is not mandatory that the
users check the list of active banks).

It could happen that the TPM driver does not know the size of SHA256 if
the SHA256 bank was not allocated. Then, it has to search in the array
containing the mapping between TPM algorithm IDs and crypto IDs.

Since it is the user of the TPM driver that calculates the digest,
passing the digest size to tpm_pcr_extend() does not require too much
effort, and it also simplifies the TPM driver code.

Roberto


> Move struct tpm_chip to include/linux/tpm.h so that the caller can query
> nr_active_banks and active_banks and can create the digest array by
> itself. Lets do this right at once now that this is being restructured.
> 
>>   
>>   	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>> -		memset(digest_list, 0, sizeof(digest_list));
>> +		digest_list = kmalloc_array(chip->nr_active_banks,
>> +					    sizeof(*digest_list), GFP_KERNEL);
>> +		if (!digest_list)
>> +			return -ENOMEM;
>>   
>> -		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>> -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>> +		memset(digest_list, 0,
>> +		       chip->nr_active_banks * sizeof(*digest_list));
> 
> You should use kcalloc() to allocate digest_list.
> 
>> +
>> +		for (i = 0; i < chip->nr_active_banks; i++) {
>>   			digest_list[i].alg_id = chip->active_banks[i];
>>   			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>> -			count++;
>>   		}
>>   
>> -		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
>> +		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
>> +				     digest_list);
>> +		kfree(digest_list);
>>   		tpm_put_ops(chip);
>>   		return rc;
>>   	}
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index f3501d05264f..98368c3a6ff7 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -248,7 +248,8 @@ struct tpm_chip {
>>   	const struct attribute_group *groups[3];
>>   	unsigned int groups_cnt;
>>   
>> -	u16 active_banks[7];
>> +	u32 nr_active_banks;
>> +	u16 *active_banks;
>>   #ifdef CONFIG_ACPI
>>   	acpi_handle acpi_dev_handle;
>>   	char ppi_version[TPM_PPI_VERSION_LEN + 1];
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index c31b490bd41d..533089cede07 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
>>   	int i;
>>   	int j;
>>   
>> -	if (count > ARRAY_SIZE(chip->active_banks))
>> +	if (count > chip->nr_active_banks)
>>   		return -EINVAL;
>>   
>>   	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>> @@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	void *marker;
>>   	void *end;
>>   	void *pcr_select_offset;
>> -	unsigned int count;
>>   	u32 sizeof_pcr_selection;
>>   	u32 rsp_len;
>>   	int rc;
>> @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	if (rc)
>>   		goto out;
>>   
>> -	count = be32_to_cpup(
>> +	chip->nr_active_banks = be32_to_cpup(
>>   		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
>>   
>> -	if (count > ARRAY_SIZE(chip->active_banks)) {
>> -		rc = -ENODEV;
>> +	chip->active_banks = kmalloc_array(chip->nr_active_banks,
>> +					   sizeof(*chip->active_banks),
>> +					   GFP_KERNEL);
>> +	if (!chip->active_banks) {
>> +		rc = -ENOMEM;
>>   		goto out;
>>   	}
>>   
>> @@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>>   	end = &buf.data[rsp_len];
>>   
>> -	for (i = 0; i < count; i++) {
>> +	for (i = 0; i < chip->nr_active_banks; i++) {
>>   		pcr_select_offset = marker +
>>   			offsetof(struct tpm2_pcr_selection, size_of_select);
>>   		if (pcr_select_offset >= end) {
>> @@ -908,9 +910,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
>>
> 
> /Jarkko
>
Jarkko Sakkinen Nov. 13, 2018, 5:04 p.m. UTC | #15
On Tue, Nov 13, 2018 at 02:34:39PM +0100, Roberto Sassu wrote:
> On 11/8/2018 2:46 PM, Jarkko Sakkinen wrote:
> > Orrayn Tue, Nov 06, 2018 at 04:01:54PM +0100, Roberto Sassu wrote:
> > > This patch removes the hard-coded limit of the active_banks array size.
> > > It stores in the tpm_chip structure the number of active PCR banks,
> > > determined in tpm2_get_pcr_allocation(), and replaces the static array
> > > with a pointer to a dynamically allocated array.
> > > 
> > > As a consequence of the introduction of nr_active_banks, tpm_pcr_extend()
> > > does not check anymore if the algorithm stored in tpm_chip is equal to
> > > zero. The active_banks array always contains valid algorithms.
> > > 
> > > Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
> > > PCR banks")
> > > 
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >   drivers/char/tpm/tpm-chip.c      |  1 +
> > >   drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
> > >   drivers/char/tpm/tpm.h           |  3 ++-
> > >   drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
> > >   4 files changed, 23 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > > index 46caadca916a..2a9e8b744436 100644
> > > --- a/drivers/char/tpm/tpm-chip.c
> > > +++ b/drivers/char/tpm/tpm-chip.c
> > > @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
> > >   	kfree(chip->log.bios_event_log);
> > >   	kfree(chip->work_space.context_buf);
> > >   	kfree(chip->work_space.session_buf);
> > > +	kfree(chip->active_banks);
> > >   	kfree(chip);
> > >   }
> > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > index 1a803b0cf980..ba7ca6b3e664 100644
> > > --- a/drivers/char/tpm/tpm-interface.c
> > > +++ b/drivers/char/tpm/tpm-interface.c
> > > @@ -1039,8 +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)];
> > > -	u32 count = 0;
> > > +	struct tpm2_digest *digest_list;
> > >   	int i;
> > >   	chip = tpm_find_get_ops(chip);
> > > @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
> > >   		return -ENODEV;
> > 
> > Should take digest_list as input. Probably callers could re-use the
> > same digest_list array multiple times?
> > 
> > Move struct tpm_chip to include/linux/tpm.h so that the caller can query
> > nr_active_banks and active_banks and can create the digest array by
> > itself. Lets do this right at once now that this is being restructured.
> 
> I have to move also other structures and #define. Wouldn't be better to
> introduce a new function to pass to the caller active_banks and
> nr_active_banks?

Revisited. I think it is fine how it is for now and we reconsider
later. Only thing I want to remark is that use should use kcalloc()
instead of kalloc_array() + memset().

/Jarkko
Ken Goldman Dec. 13, 2018, 8:21 p.m. UTC | #16
On 11/7/2018 4:41 AM, Roberto Sassu wrote:
> On 11/7/2018 7:14 AM, Nayna Jain wrote:
> 
> In the TPM Commands specification (section 30.2.1), I found:
> 
> TPM_CAP_PCRS – Returns the current allocation of PCR in a
> TPML_PCR_SELECTION.
> 
> You mentioned:
> 
> #TPM_RC_SIZE response code when count is greater
> than the possible number of banks
> 
> but TPML_PCR_SELECTION is provided by the TPM.
> 
> Roberto
> 
> 
[snip]
>>
>>
>> As per my understanding, the count in the TPML_PCR_SELECTION represent 
>> the number of possible banks and not the number of active banks.
>> TCG Structures Spec for TPM 2.0 - Table 102 mentions this as 
>> explanation of #TPM_RC_SIZE.

FYI: This was clarified in the TCG's TPM work group today.  TPM_CAP_PCRS 
returns:

The TPML_PCR_SELECTION must include a TPMS_PCR_SELECTION for each PCR 
bank in which there is at least one allocated PCR. The 
TPML_PCR_SELECTION may return a TPMS_PCR_SELECTION for each implemented 
PCR bank.  The TPML_PCR_SELECTION may return a TPMS_PCR_SELECTION for 
each implemented hash algorithm.

Also:

The TPM doesn't use the term "active banks"

Allocated = a bank that has at least one PCR bit set in the selection 
bitmap.

Supported or implemented banks = the number of PCR banks that can be 
allocated, based on the TPM hardware.

Hash algorithms = The hash algorithms supported by the TPM

For example, the TPM may support 3 hash algorithms and 2 PCR banks, and 
have 1 bank allocated.

Patch
diff mbox series

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 46caadca916a..2a9e8b744436 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -160,6 +160,7 @@  static void tpm_dev_release(struct device *dev)
 	kfree(chip->log.bios_event_log);
 	kfree(chip->work_space.context_buf);
 	kfree(chip->work_space.session_buf);
+	kfree(chip->active_banks);
 	kfree(chip);
 }
 
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1a803b0cf980..ba7ca6b3e664 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1039,8 +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)];
-	u32 count = 0;
+	struct tpm2_digest *digest_list;
 	int i;
 
 	chip = tpm_find_get_ops(chip);
@@ -1048,16 +1047,22 @@  int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
 		return -ENODEV;
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-		memset(digest_list, 0, sizeof(digest_list));
+		digest_list = kmalloc_array(chip->nr_active_banks,
+					    sizeof(*digest_list), GFP_KERNEL);
+		if (!digest_list)
+			return -ENOMEM;
 
-		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
-			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
+		memset(digest_list, 0,
+		       chip->nr_active_banks * sizeof(*digest_list));
+
+		for (i = 0; i < chip->nr_active_banks; i++) {
 			digest_list[i].alg_id = chip->active_banks[i];
 			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
-			count++;
 		}
 
-		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
+		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
+				     digest_list);
+		kfree(digest_list);
 		tpm_put_ops(chip);
 		return rc;
 	}
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f3501d05264f..98368c3a6ff7 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -248,7 +248,8 @@  struct tpm_chip {
 	const struct attribute_group *groups[3];
 	unsigned int groups_cnt;
 
-	u16 active_banks[7];
+	u32 nr_active_banks;
+	u16 *active_banks;
 #ifdef CONFIG_ACPI
 	acpi_handle acpi_dev_handle;
 	char ppi_version[TPM_PPI_VERSION_LEN + 1];
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index c31b490bd41d..533089cede07 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -242,7 +242,7 @@  int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
 	int i;
 	int j;
 
-	if (count > ARRAY_SIZE(chip->active_banks))
+	if (count > chip->nr_active_banks)
 		return -EINVAL;
 
 	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
@@ -859,7 +859,6 @@  static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	void *marker;
 	void *end;
 	void *pcr_select_offset;
-	unsigned int count;
 	u32 sizeof_pcr_selection;
 	u32 rsp_len;
 	int rc;
@@ -878,11 +877,14 @@  static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	if (rc)
 		goto out;
 
-	count = be32_to_cpup(
+	chip->nr_active_banks = be32_to_cpup(
 		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
 
-	if (count > ARRAY_SIZE(chip->active_banks)) {
-		rc = -ENODEV;
+	chip->active_banks = kmalloc_array(chip->nr_active_banks,
+					   sizeof(*chip->active_banks),
+					   GFP_KERNEL);
+	if (!chip->active_banks) {
+		rc = -ENOMEM;
 		goto out;
 	}
 
@@ -891,7 +893,7 @@  static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
 	end = &buf.data[rsp_len];
 
-	for (i = 0; i < count; i++) {
+	for (i = 0; i < chip->nr_active_banks; i++) {
 		pcr_select_offset = marker +
 			offsetof(struct tpm2_pcr_selection, size_of_select);
 		if (pcr_select_offset >= end) {
@@ -908,9 +910,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;