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

From: Tushar Sugandhi <tusharsu@linux.microsoft.com>
To: Jarkko Sakkinen <jarkko@kernel.org>,
	zohar@linux.ibm.com, noodles@fb.com, bauermann@kolabnow.com,
	ebiederm@xmission.com, bhe@redhat.com, vgoyal@redhat.com,
	dyoung@redhat.com, peterhuewe@gmx.de, jgg@ziepe.ca,
	kexec@lists.infradead.org, linux-integrity@vger.kernel.org
Cc: code@tyhicks.com, nramas@linux.microsoft.com, paul@paul-moore.com
Subject: Re: [PATCH 1/6] tpm: implement TPM2 function to get update counter
Date: Wed, 2 Aug 2023 14:04:57 -0700	[thread overview]
Message-ID: <dd126081-3216-c15e-124e-ef137b376cdb@linux.microsoft.com> (raw)
In-Reply-To: <CUHRAC79EW66.1VQRXS97GB0UO@suppilovahvero>

On 8/1/23 20:58, Jarkko Sakkinen wrote:
> On Wed Aug 2, 2023 at 12:01 AM EEST, Tushar Sugandhi wrote:
>> 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?
> It is in the process documentation:
>
> https://www.kernel.org/doc/html/v6.3/process/submitting-patches.html#the-canonical-patch-format
Sounds good.  I will cleanup both the summary phrase and the patch 
description.
>>> "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.
> I was referring to the fact that TPM2_PCR_Read does not have a field
> called "update counter" in its response but it has a field called
> "pcrUpdateCounter". Please refer to thigs that actually exist.
>
> In the long description you are in some occasions referring to the same
> object as:
>
> 1. "update counter"
> 2. "pcrUpdateCounter"
> 3. "PcrUpdateCounter"
>
> This is ambiguous and wrong.
Thanks. I will consistently use pcrUpdateCounter going forward.
> >From long description I see zero motivation to ack this change, except
> some heresay about IMA requiring it. Why does IMA need update_cnt and
> why this is not documented to the long description?
Since patch 2 of this series exposes the functionality to IMA,
it is described in the long description of patch 2.

But I can add the description here as well for completeness.
>> But I can update tpm2_pcr_read() if you are ok with it.
>> Please let me know.
> You can add "u32 *update_cnt".
Sounds good.  Will do.

Btw, the function tpm2_pcr_read is not exposed directly to the other
subsystems (like IMA).  It is exposed via tpm_pcr_read.

Do you want to expose tpm2_pcr_read directly,
or do you want me to update the function signature of tpm_pcr_read as well?

Updating the function signature of tpm_pcr_read as well -
to return "u32 *update_cnt" seems like the right approach.
In that case, I can set *update_cnt to say 0 or -1 for TPM1
(because pcrUpdateCounter is not available for TPM1).

Please let me know what do you think.

I will make the changes accordingly.

I will also wait for IMA/Kexec maintainers to take a look at the 
remaining patches
in this series, incorporate their feedback, and send the V2 of this series.

Thanks again for your feedback. Really appreciate it.

~Tushar
>
> BR, Jarkko

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