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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux Kernel]     [Linux Kernel Hardening]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux SCSI]

  Powered by Linux