Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

On Mon, Sep 14, 2020 at 12:19:28PM -0700, James Bottomley wrote:
> On Mon, 2020-09-14 at 20:41 +0300, Jarkko Sakkinen wrote:
> > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote:
> > > Create sysfs per hash groups with 24 PCR files in them one group,
> > > named pcr-<hash>, for each agile hash of the TPM.  The files are
> > > plugged in to a PCR read function which is TPM version agnostic, so
> > > this works also for TPM 1.2 but the hash is only sha1 in that case.
> > > 
> > > Note: the macros used to create the hashes emit spurious checkpatch
> > > warnings.  Do not try to "fix" them as checkpatch recommends,
> > > otherwise
> > > they'll break.
> > 
> > "PCR access is required because IMA tools should be able to run
> > without any sort of TSS dependencies."
> > 
> > AFAIK, this is the only reason to merge this and it is missing from
> > the description. Perhaps you could either include that sentence, or
> > alternatively write something along the lines?
> 
> Sure, I'll add all of them: it's IMA tools, early boot and key locking
> to PCR policy.

Great!

> > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.c
> > > om>
> > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > 
> > Please also cc this at least to Greg and Jason Gunthorpe next time.
> 
> OK
> 
> [...]
> 
> > > 
> > enum tpm_alg_misc {
> > 	TPM_ALG_ERROR		= 0x0000,
> > 	TPM_ALG_KEYEDHASH	= 0x0008,
> > 	TPM_ALG_NULL		= 0x0010,
> > }
> > 
> > enum tpm_alg_hash {
> > 	TPM_ALG_SHA1		= 0x0004,
> > 	TPM_ALG_SHA256		= 0x000B,
> > 	TPM_ALG_SHA384		= 0x000C,
> > 	TPM_ALG_SHA512		= 0x000D,
> > 	TPM_ALG_SM3_256		= 0x0012,
> > 	TPM_ALG_HASH_MAX,
> > };
> 
> I can separate them if you insist, but the latter construction won't
> work.  TPM_ALG_HASH_MAX will get set to the previous value plus one.
> 
> You can see this with the test programme:
> 
> ---
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> 
> enum tpm_alg_hash {
>         TPM_ALG_SHA1            = 0x0004,
>         TPM_ALG_SHA256          = 0x000B,
>         TPM_ALG_SHA384          = 0x000C,
>         TPM_ALG_SHA512          = 0x000D,
>         TPM_ALG_SM3_256         = 0x0012,
>         TPM_ALG_HASH_MAX,
> };
> 
> int main()
> {
> 	printf("TPM_ALG_HASH_MAX = %d\n", TPM_ALG_HASH_MAX);
> }
> ---
> 
> Which gives
> 
> jejb@jarvis> ./a.out
> TPM_ALG_HASH_MAX = 19
> 
> Which is clearly the wrong value (it's 0x12 + 1).
> 
> That being so, is there any reason to separate up the algorithms enum?
> 
> James

No, my bad.

/Jarkko