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

[Tushar Sugandhi]

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