Re: [PATCH 1/6] tpm: implement TPM2 function to get update counter

[Tushar Sugandhi <tusharsu@linux.microsoft.com>]

Thanks for the response Jarkko.

On 8/1/23 12:02, Jarkko Sakkinen wrote:
> The short summary is cryptic to say the least.
Do you mean the patch subject line, or the description below?
> "update counter" does not map it to have anything to do with PCRs.
Agreed.  I noticed that when I was testing the patches.
The update counter is same for all PCRs.  It was also the same for
the two hash algo's I tested it for (SHA1 and SHA256). But the spec
description and Kernel implementation requires to pass the
pcr_idx and hash algo to PCR_Read command to get the update counter.
> Why not "tpm: Read pcrUpdateCounter field from TPM2_PCR_Read"?
As I said in the patch description below, update counter is only
needed for IMA measurements.  None of the other code that calls
tpm2_pcr_read() use the update counter.

I was not sure if you were okay changing the function signature and
implementation of tpm2_pcr_read(). It felt disruptive.

But I can update tpm2_pcr_read() if you are ok with it.
Please let me know.

I also have a few more thoughts on this in the comment below.
> On Tue Aug 1, 2023 at 9:19 PM EEST, Tushar Sugandhi wrote:
>> The TPM2_PCR_Read command returns TPM2_PCR_Read Response struct[1].  It
>> contains pcrUpdateCounter member which contains the current value of TPM
>> PCR update counter.  The update counter provides the number of times the
>> PCRs are updated, which is essential for tracking changes and verifying
>> system integrity.  Thus, subsystems (like IMA) should measure
>> pcrUpdateCounter value.  Although tpm2_pcr_read_out struct is returned
>> by tpm2_pcr_read(), it is not used by it's caller function tpm_pcr_read().
>> Further, TPM2_PCR_Read Response struct and pcrUpdateCounter is not
>> available in tpm1_pcr_read().
>>
>> PcrUpdateCounter is only needed in a specific case (IMA for measurements).
>> Changing tpm_pcr_read() and tpm2_pcr_read() function signature to return
>> tpm2_pcr_read_out struct would be a more disruptive change, since these
>> functions are used elsewhere too.  Creating separate functions to get
>> pcrUpdateCounter when needed would be a cleaner approach.
>>
>> Add a function, 'tpm2_pcr_get_update_counter()' to retrieve
>> the update counter for a given PCR index and algorithm ID on a TPM2 chip.
>>
>> This function complements existing TPM functionalities such as reading
>> and extending PCRs, and enhances the ability to monitor PCR status
>> in the Linux Kernel.
>>
>> [1] https://trustedcomputinggroup.org/wp-content/uploads/TCG_TPM2_r1p59_Part3_Commands_pub.pdf
>> Section 22.4.2, Page 206.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>> ---
>>   drivers/char/tpm/tpm.h      |  3 +++
>>   drivers/char/tpm/tpm2-cmd.c | 48 +++++++++++++++++++++++++++++++++++++
>>   2 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index 830014a26609..60489f21d3bd 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -288,6 +288,9 @@ static inline void tpm_add_ppi(struct tpm_chip *chip)
>>   int tpm2_get_timeouts(struct tpm_chip *chip);
>>   int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>>   		  struct tpm_digest *digest, u16 *digest_size_ptr);
>> +int tpm2_pcr_get_update_counter(struct tpm_chip *chip,
>> +				u32 pcr_idx, u16 alg_id,
>> +				u32 *update_counter);
> tpm_pcr_read_update_cnt()
I can rename 'get' -> 'read'

About 'tpm2' -> 'tpm':
I already have tpm_pcr_get_update_counter() defined in patch 2.

I was following the existing pattern here in patch 1 and 2 i. e.
  - Implementing the tpm1/tpm2 specific functionality in
    drivers/char/tpm/tpm.h, drivers/char/tpm/tpm2-cmd.c,
    drivers/char/tpm/tpm1-cmd.c.

  - And combining that functionality in drivers/char/tpm/tpm-interface.c
    and exposing it to other subsystems (like IMA) through 
include/linux/tpm.h
    (patch 2 of this series)

BTW, if I understand correctly, the update counter is not available in 
TPM 1.2.

Please let me know if you want me to expose the functionality directly
from drivers/char/tpm/tpm2-cmd.c and getting rid of patch #2 of this series.

~Tushar

>>   int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>>   		    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 93545be190a5..55f4e102289a 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -216,6 +216,54 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>>   	return rc;
>>   }
>>   
>> +/**
>> + * tpm2_pcr_get_update_counter() - gets an update counter value for a PCR bank
>> + * @chip: TPM chip to use
>> + * @pcr_idx: PCR index used to retrieve the update counter
>> + * @alg_id:	alg id used to retrieve the update counter
>> + * @update_counter: output update counter value
>> + *
>> + * Return: Same as with tpm_transmit_cmd.
>> + */
>> +int tpm2_pcr_get_update_counter(struct tpm_chip *chip,
>> +				u32 pcr_idx, u16 alg_id, u32 *update_counter)
>> +{
>> +	int rc;
>> +	struct tpm_buf buf;
>> +	struct tpm2_pcr_read_out *read_out;
>> +	u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
>> +
>> +	if (pcr_idx >= TPM2_PLATFORM_PCR)
>> +		return -EINVAL;
>> +
>> +	if (!update_counter)
>> +		return -EINVAL;
>> +
>> +	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
>> +	if (rc)
>> +		return rc;
>> +
>> +	pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);
>> +
>> +	tpm_buf_append_u32(&buf, 1);
>> +	tpm_buf_append_u16(&buf, alg_id);
>> +	tpm_buf_append_u8(&buf, TPM2_PCR_SELECT_MIN);
>> +	tpm_buf_append(&buf, (const unsigned char *)pcr_select,
>> +		       sizeof(pcr_select));
>> +
>> +	rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to read a pcr value");
>> +	if (rc)
>> +		goto out;
>> +
>> +	read_out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
>> +
>> +	*update_counter = be32_to_cpu(read_out->update_cnt);
>> +
>> +out:
>> +	tpm_buf_destroy(&buf);
>> +	return rc;
>> +}
>> +
>>   struct tpm2_null_auth_area {
>>   	__be32  handle;
>>   	__be16  nonce_size;
>> -- 
>> 2.25.1
> BR, Jarkko

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec