On Mon, 2020-11-30 at 22:50 +0000, Elliott, Robert (Servers) wrote:
> > -----Original Message-----
> > From: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
> > Sent: Monday, November 30, 2020 1:54 PM
> > To: Elliott, Robert (Servers) <elliott@xxxxxxx>; linux-
> > integrity@xxxxxxxxxxxxxxx
> > Cc: Mimi Zohar <zohar@xxxxxxxxxxxxx>; Jarkko Sakkinen
> > <jarkko.sakkinen@xxxxxxxxxxxxxxx>; linux-api@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH RESEND v4 1/1] tpm: add sysfs exports for all
> > banks of PCR registers
> >
> > On Mon, 2020-11-30 at 19:00 +0000, Elliott, Robert (Servers) wrote:
> > > ...
> > > > + * The first argument is the TPM algorithm id and the second
> > > > is
> > > > the
> > > > + * hash used as both the suffix and the group name. Note: the
> > > > group
> > > > + * name is a directory in the top level tpm class with the
> > > > name
> > > > + * pcr-<hash>, so it must not clash with any other names
> > > > already
> > > > + * in the sysfs directory.
> > > > + */
> > > > +PCR_ATTR_BUILD(TPM_ALG_SHA1, sha1);
> > > > +PCR_ATTR_BUILD(TPM_ALG_SHA256, sha256);
> > > > +PCR_ATTR_BUILD(TPM_ALG_SHA384, sha384);
> > > > +PCR_ATTR_BUILD(TPM_ALG_SHA512, sha512);
> > > > +PCR_ATTR_BUILD(TPM_ALG_SM3_256, sm3);
> > >
> > > The latest PC Client Platform TPM Profile and TPM 2.0 Part 2
> > > Structures specs also define codes for three SHA-3 hash
> > > algorithms:
> > > TPM_ALG_SHA3_256
> > > TPM_ALG_SHA3_384
> > > TPM_ALG_SHA3_512
> >
> > this is PTP 1.05 which was published this September? The basic
> > reason
> > is it wasn't there when this patch was first published, but they
> > can
> > always be added ... the whole idea is to be extensible.
>
> Yes, they are in
> * TCG PC Client Platform TPM Profile Specification for TPM 2.0
> Version 1.05, Revision 14, 4 September 2020
> * Trusted Platform Module Library Part 2: Structures
> Family 2.0, Level 00 Revision 1.59, 8 November 2019
>
> I don't know if anyone has started implementing SHA-3 for PCRs.
We at least need the algorithms to get added to our tpm.h, which really
has to be a separate patch, so I'd rather not conflate it with this
one. I'll make sure that they're added to this patch (provided it's
upstream) at the same time they get added to our tpm.h.
> > > ...
> > > > +
> > > > + /* add one group for each bank hash */
> > > > + for (i = 0; i < chip->nr_allocated_banks; i++) {
> > > > + switch (chip->allocated_banks[i].alg_id) {
> > > > + case TPM_ALG_SHA1:
> > > > + chip->groups[chip->groups_cnt++] =
> > > > &pcr_group_sha1;
> > > > + break;
> > > > + case TPM_ALG_SHA256:
> > > > + chip->groups[chip->groups_cnt++] =
> > > > &pcr_group_sha256;
> > > > + break;
> > > > + case TPM_ALG_SHA384:
> > > > + chip->groups[chip->groups_cnt++] =
> > > > &pcr_group_sha384;
> > > > + break;
> > > > + case TPM_ALG_SHA512:
> > > > + chip->groups[chip->groups_cnt++] =
> > > > &pcr_group_sha512;
> > > > + break;
> > > > + case TPM_ALG_SM3_256:
> > > > + chip->groups[chip->groups_cnt++] =
> > > > &pcr_group_sm3;
> > > > + break;
> > > > + default:
> > > > + /*
> > > > + * If this warning triggers, send a
> > > > patch to
> > > > + * add both a PCR_ATTR_BUILD() macro
> > > > above for
> > > > + * the missing algorithm as well as an
> > > > + * additional case in this switch
> > > > statement.
> > > > + */
> > > > + WARN(1, "TPM with unsupported bank
> > > > algorthm
> > > > 0x%04x",
> > > > + chip->allocated_banks[i].alg_id);
> > >
> > > algorithm is missing the letter i.
> >
> > Yes, I'll fix that.
> >
> > > It might help to print the bank id (variable i) as well.
> >
> > I'm not sure how it helps the user. We deliberately hide the bank
> > numbers because all banks in sysfs are referred to by hash ... how
> > would exposing the bank number here help?
>
> I just noticed the WARN inside the for loop and worried about
> spewing a lot of similar messages, each with long stack traces.
>
> nr_allocated_banks shouldn't be very large, though, so there's
> probably nothing to be concerned about.
>
> Maybe consider dev_warn instead of WARN? That would indicate which
> TPM has the unsupported algorithm, which could be useful, but avoid
> the stack dump, which might not be useful (except that it is more
> likely to be reported back to kernel developers).
The reason for a WARN is that we want it to trip a big bad error for
the developer. It means we came across a TPM whose algorithm is
unsupported, so it really needs adding ASAP. People take reporting
back WARN stack traces much more seriously than a stray dev_warn()
which often gets ignored.
James
