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
