Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
To:     Roberto Sassu <roberto.sassu@huawei.com>,
        jarkko.sakkinen@linux.intel.com, zohar@linux.ibm.com
Cc:     linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-kernel@vger.kernel.org, silviu.vlasceanu@huawei.com
References: <20181106150159.1136-1-roberto.sassu@huawei.com>
 <20181106150159.1136-2-roberto.sassu@huawei.com>
From:   Nayna Jain <nayna@linux.ibm.com>
Date:   Wed, 7 Nov 2018 11:44:27 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.2
MIME-Version: 1.0
In-Reply-To: <20181106150159.1136-2-roberto.sassu@huawei.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Message-Id: <98482eee-6e91-1666-1ce2-cfa94a33efc2@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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;