[PATCH RESEND v4 0/1] add sysfs exports for TPM 2 PCR registers

[PATCH RESEND v4 0/1] add sysfs exports for TPM 2 PCR registers

[James Bottomley]

Cc to linux-api to get an opinion on two issues.  First the background:

We've had a fairly extensive discussion over on linux-integrity and
iterated to the conclusion that the kernel does need to export TPM 2.0
PCR values for use by a variety of userspace integrity programmes
including early boot.  The principle clinching argument seems to be
that these values are required by non-root systems, but in a default
Linux set up the packet marshalled communication device: /dev/tpmrm0,
is by default only usable by root.  Historically, TPM 1.2 exported
these values via sysfs in a single file containing all 24 values:

  /sys/class/tpm/tpm0/pcrs

with the format

  PCR-00: 7D 29 CB 08 0C 0F C4 16 7A 0E 9A F7 C6 D3 97 CD C1 21 A7 69 
  PCR-01: 9C B6 79 4C E4 4B 62 97 4C AB 55 13 1A 2F 7E AE 09 B3 30 BE 
  ...

TPM 2.0 adds more complexity: because of it's "agile" format, each TPM
2.0 is required to support a set of hashes (of which at least sha1 and
sha256 are required but quite a few TPM 2.0s have at least two or
three more) and maintain 24 PCR registers for each supported hash.
The current patch exports each PCR bank under the directory

  /sys/class/tpm/tpm0/pcr-<hash>/<bank>

So the sha256 bank value of PCR 7 can be obtained as

  cat /sys/class/tpm/tpm0/pcr-sha256/7
  2ED93F199692DC6788EFA6A1FE74514AB9760B2A6CEEAEF6C808C13E4ABB0D42

And the output is a single non-space separated ascii hex value of the
hash.

The issues we'd like input on are:

 1. Should this be in sysfs or securityfs?

  2. Should we export the values as one value per file (current patch)
     or as a binary blob of all 24?

I'm largely ambivalent about 1.  I can easily do securityfs output, it
is more work than sysfs largely because securityfs lacks most of the
features of sysfs, including the groups one that this patch uses
heavily, but that can all be open coded (as most other securityfs
consumers do).

I'm less ambivalent about the binary blob idea: pretty much every use
case we have requires a set of PCRs which are fewer than the 24 and a
lot only require a single PCR, so providing all 24 in a format that
has to be parsed seems to make life more difficult for the consuming
program.  The argument, at least, for providing the PCRs in binary
form is that most of the consuming programs, once they've selected
their set, tend to need the hash value of the set, which necessitates
converting from ascii to binary.  I do this by the simple script (for
PCRs say 1,6,7) as:

  cat /sys/class/tpm/tpm0/pcr-sha256/{1,6,7}|xxd -r -p|sha256sum

I've cc'd Jarkko, who's the main proponent of the binary blob use case
because he can make better arguments than I can.

Regards,

James

---

James Bottomley (1):
  tpm: add sysfs exports for all banks of PCR registers

 drivers/char/tpm/tpm-sysfs.c | 178 +++++++++++++++++++++++++++++++++++
 include/linux/tpm.h          |   9 +-
 2 files changed, 186 insertions(+), 1 deletion(-)

-- 
2.26.2

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

[James Bottomley]

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.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

---

v2: fix TPM 1.2 legacy links failure
v3: fix warn on and add note to tpm_algorithms
v4: reword commit and add tested-by
---
 drivers/char/tpm/tpm-sysfs.c | 178 +++++++++++++++++++++++++++++++++++
 include/linux/tpm.h          |   9 +-
 2 files changed, 186 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index d52bf4df0bca..81a02200b207 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -348,11 +348,189 @@ static const struct attribute_group tpm2_dev_group = {
 	.attrs = tpm2_dev_attrs,
 };
 
+struct tpm_pcr_attr {
+	int alg_id;
+	int pcr;
+	struct device_attribute attr;
+};
+
+#define to_tpm_pcr_attr(a) container_of(a, struct tpm_pcr_attr, attr)
+
+static ssize_t pcr_value_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct tpm_pcr_attr *ha = to_tpm_pcr_attr(attr);
+	struct tpm_chip *chip = to_tpm_chip(dev);
+	struct tpm_digest digest;
+	int i;
+	int digest_size = 0;
+	int rc;
+	char *str = buf;
+
+	for (i = 0; i < chip->nr_allocated_banks; i++)
+		if (ha->alg_id == chip->allocated_banks[i].alg_id)
+			digest_size = chip->allocated_banks[i].digest_size;
+	/* should never happen */
+	if (!digest_size)
+		return -EINVAL;
+
+	digest.alg_id = ha->alg_id;
+	rc = tpm_pcr_read(chip, ha->pcr, &digest);
+	if (rc)
+		return rc;
+	for (i = 0; i < digest_size; i++)
+		str += sprintf(str, "%02X", digest.digest[i]);
+	str += sprintf(str, "\n");
+
+	return str - buf;
+}
+
+/*
+ * The following set of defines represents all the magic to build
+ * the per hash attribute groups for displaying each bank of PCRs.
+ * The only slight problem with this approach is that every PCR is
+ * hard coded to be present, so you don't know if an PCR is missing
+ * until a cat of the file returns -EINVAL
+ *
+ * Also note you must ignore checkpatch warnings in this macro
+ * code. This is deep macro magic that checkpatch.pl doesn't
+ * understand.
+ */
+
+/* Note, this must match TPM2_PLATFORM_PCR which is fixed at 24. */
+#define _TPM_HELPER(_alg, _hash, F) \
+	F(_alg, _hash, 0)	    \
+	F(_alg, _hash, 1)	    \
+	F(_alg, _hash, 2)	    \
+	F(_alg, _hash, 3)	    \
+	F(_alg, _hash, 4)	    \
+	F(_alg, _hash, 5)	    \
+	F(_alg, _hash, 6)	    \
+	F(_alg, _hash, 7)	    \
+	F(_alg, _hash, 8)	    \
+	F(_alg, _hash, 9)	    \
+	F(_alg, _hash, 10)	    \
+	F(_alg, _hash, 11)	    \
+	F(_alg, _hash, 12)	    \
+	F(_alg, _hash, 13)	    \
+	F(_alg, _hash, 14)	    \
+	F(_alg, _hash, 15)	    \
+	F(_alg, _hash, 16)	    \
+	F(_alg, _hash, 17)	    \
+	F(_alg, _hash, 18)	    \
+	F(_alg, _hash, 19)	    \
+	F(_alg, _hash, 20)	    \
+	F(_alg, _hash, 21)	    \
+	F(_alg, _hash, 22)	    \
+	F(_alg, _hash, 23)
+
+/* ignore checkpatch warning about trailing ; in macro. */
+#define PCR_ATTR(_alg, _hash, _pcr)				   \
+	static struct tpm_pcr_attr dev_attr_pcr_##_hash##_##_pcr = {	\
+		.alg_id = _alg,					   \
+		.pcr = _pcr,					   \
+		.attr = {					   \
+			.attr = {				   \
+				.name = __stringify(_pcr),	   \
+				.mode = 0444			   \
+			},					   \
+			.show = pcr_value_show			   \
+		}						   \
+	};
+
+#define PCR_ATTRS(_alg, _hash)			\
+	_TPM_HELPER(_alg, _hash, PCR_ATTR)
+
+/* ignore checkpatch warning about trailing , in macro. */
+#define PCR_ATTR_VAL(_alg, _hash, _pcr)		\
+	&dev_attr_pcr_##_hash##_##_pcr.attr.attr,
+
+#define PCR_ATTR_GROUP_ARRAY(_alg, _hash)		       \
+	static struct attribute *pcr_group_attrs_##_hash[] = { \
+		_TPM_HELPER(_alg, _hash, PCR_ATTR_VAL)	       \
+		NULL					       \
+	}
+
+#define PCR_ATTR_GROUP(_alg, _hash)			    \
+	static struct attribute_group pcr_group_##_hash = { \
+		.name = "pcr-" __stringify(_hash),	    \
+		.attrs = pcr_group_attrs_##_hash	    \
+	}
+
+#define PCR_ATTR_BUILD(_alg, _hash)	   \
+	PCR_ATTRS(_alg, _hash)		   \
+	PCR_ATTR_GROUP_ARRAY(_alg, _hash); \
+	PCR_ATTR_GROUP(_alg, _hash)
+/*
+ * End of macro structure to build an attribute group containing 24
+ * PCR value files for each supported hash algorithm
+ */
+
+/*
+ * The next set of macros implements the cleverness for each hash to
+ * build a static attribute group called pcr_group_<hash> which can be
+ * added to chip->groups[].
+ *
+ * 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);
+
+
 void tpm_sysfs_add_device(struct tpm_chip *chip)
 {
+	int i;
+
 	WARN_ON(chip->groups_cnt != 0);
+
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		chip->groups[chip->groups_cnt++] = &tpm2_dev_group;
 	else
 		chip->groups[chip->groups_cnt++] = &tpm1_dev_group;
+
+	/* 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);
+			break;
+		}
+	}
+
+	/*
+	 * This will only trigger if someone has added an additional
+	 * hash to the tpm_algorithms enum without incrementing
+	 * TPM_MAX_HASHES.
+	 */
+	WARN_ON(chip->groups_cnt > TPM_MAX_HASHES + 1);
 }
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 3b5d455501c5..cc0b94dcf21e 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -31,6 +31,7 @@ struct tpm_chip;
 struct trusted_key_payload;
 struct trusted_key_options;
 
+/* if you add a new hash to this, increment TPM_MAX_HASHES below */
 enum tpm_algorithms {
 	TPM_ALG_ERROR		= 0x0000,
 	TPM_ALG_SHA1		= 0x0004,
@@ -42,6 +43,12 @@ enum tpm_algorithms {
 	TPM_ALG_SM3_256		= 0x0012,
 };
 
+/*
+ * maximum number of hashing algorithms a TPM can have.  This is
+ * basically a count of every hash in tpm_algorithms above
+ */
+#define TPM_MAX_HASHES	5
+
 struct tpm_digest {
 	u16 alg_id;
 	u8 digest[TPM_MAX_DIGEST_SIZE];
@@ -146,7 +153,7 @@ struct tpm_chip {
 
 	struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES];
 
-	const struct attribute_group *groups[3];
+	const struct attribute_group *groups[3 + TPM_MAX_HASHES];
 	unsigned int groups_cnt;
 
 	u32 nr_allocated_banks;
-- 
2.26.2

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

[Greg KH]

On Sun, Nov 29, 2020 at 02:30:22PM -0800, 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.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> 
> ---
> 
> v2: fix TPM 1.2 legacy links failure
> v3: fix warn on and add note to tpm_algorithms
> v4: reword commit and add tested-by
> ---
>  drivers/char/tpm/tpm-sysfs.c | 178 +++++++++++++++++++++++++++++++++++
>  include/linux/tpm.h          |   9 +-
>  2 files changed, 186 insertions(+), 1 deletion(-)

No Documentation/ABI/ entry for new sysfs files?

:(

Re: [PATCH RESEND v4 0/1] add sysfs exports for TPM 2 PCR registers

[Greg KH]

On Sun, Nov 29, 2020 at 02:30:21PM -0800, James Bottomley wrote:
> Cc to linux-api to get an opinion on two issues.  First the background:
> 
> We've had a fairly extensive discussion over on linux-integrity and
> iterated to the conclusion that the kernel does need to export TPM 2.0
> PCR values for use by a variety of userspace integrity programmes
> including early boot.  The principle clinching argument seems to be
> that these values are required by non-root systems, but in a default
> Linux set up the packet marshalled communication device: /dev/tpmrm0,
> is by default only usable by root.  Historically, TPM 1.2 exported
> these values via sysfs in a single file containing all 24 values:
> 
>   /sys/class/tpm/tpm0/pcrs
> 
> with the format
> 
>   PCR-00: 7D 29 CB 08 0C 0F C4 16 7A 0E 9A F7 C6 D3 97 CD C1 21 A7 69 
>   PCR-01: 9C B6 79 4C E4 4B 62 97 4C AB 55 13 1A 2F 7E AE 09 B3 30 BE 
>   ...

As you know, this breaks the "one value per file" for sysfs, so please,
do not add more files that do this.

> TPM 2.0 adds more complexity: because of it's "agile" format, each TPM
> 2.0 is required to support a set of hashes (of which at least sha1 and
> sha256 are required but quite a few TPM 2.0s have at least two or
> three more) and maintain 24 PCR registers for each supported hash.
> The current patch exports each PCR bank under the directory
> 
>   /sys/class/tpm/tpm0/pcr-<hash>/<bank>
> 
> So the sha256 bank value of PCR 7 can be obtained as
> 
>   cat /sys/class/tpm/tpm0/pcr-sha256/7
>   2ED93F199692DC6788EFA6A1FE74514AB9760B2A6CEEAEF6C808C13E4ABB0D42
> 
> And the output is a single non-space separated ascii hex value of the
> hash.
> 
> The issues we'd like input on are:
> 
>  1. Should this be in sysfs or securityfs?

If you want to use sysfs, use one value per file please.

>   2. Should we export the values as one value per file (current patch)
>      or as a binary blob of all 24?

Binary sysfs files are for "pass-through" mode where the kernel is not
parsing/manipulating the data at all.  Do these values come straight
from the hardware?  If so, sure, use a binary blob.  If not, then no, do
not use that in sysfs as sysfs is to be in text format.

thanks,

greg k-h

Re: [PATCH RESEND v4 0/1] add sysfs exports for TPM 2 PCR registers

[Mimi Zohar]

On Mon, 2020-11-30 at 09:18 +0100, Greg KH wrote:
> On Sun, Nov 29, 2020 at 02:30:21PM -0800, James Bottomley wrote:
> > Cc to linux-api to get an opinion on two issues.  First the background:
> > 
> > We've had a fairly extensive discussion over on linux-integrity and
> > iterated to the conclusion that the kernel does need to export TPM 2.0
> > PCR values for use by a variety of userspace integrity programmes
> > including early boot.  The principle clinching argument seems to be
> > that these values are required by non-root systems, but in a default
> > Linux set up the packet marshalled communication device: /dev/tpmrm0,
> > is by default only usable by root.  Historically, TPM 1.2 exported
> > these values via sysfs in a single file containing all 24 values:
> > 
> >   /sys/class/tpm/tpm0/pcrs
> > 
> > with the format
> > 
> >   PCR-00: 7D 29 CB 08 0C 0F C4 16 7A 0E 9A F7 C6 D3 97 CD C1 21 A7 69 
> >   PCR-01: 9C B6 79 4C E4 4B 62 97 4C AB 55 13 1A 2F 7E AE 09 B3 30 BE 
> >   ...
> 
> As you know, this breaks the "one value per file" for sysfs, so please,
> do not add more files that do this.
> 
> > TPM 2.0 adds more complexity: because of it's "agile" format, each TPM
> > 2.0 is required to support a set of hashes (of which at least sha1 and
> > sha256 are required but quite a few TPM 2.0s have at least two or
> > three more) and maintain 24 PCR registers for each supported hash.
> > The current patch exports each PCR bank under the directory
> > 
> >   /sys/class/tpm/tpm0/pcr-<hash>/<bank>
> > 
> > So the sha256 bank value of PCR 7 can be obtained as
> > 
> >   cat /sys/class/tpm/tpm0/pcr-sha256/7
> >   2ED93F199692DC6788EFA6A1FE74514AB9760B2A6CEEAEF6C808C13E4ABB0D42
> > 
> > And the output is a single non-space separated ascii hex value of the
> > hash.
> > 
> > The issues we'd like input on are:
> > 
> >  1. Should this be in sysfs or securityfs?
> 
> If you want to use sysfs, use one value per file please.
> 
> >   2. Should we export the values as one value per file (current patch)
> >      or as a binary blob of all 24?
> 
> Binary sysfs files are for "pass-through" mode where the kernel is not
> parsing/manipulating the data at all.  Do these values come straight
> from the hardware?  If so, sure, use a binary blob.  If not, then no, do
> not use that in sysfs as sysfs is to be in text format.

The data is coming from the hardware, but not the result of a single
TPM command.  Each TPM bank PCR has to be queried individually.  The
question is whether the result should be exported separately (via
sysfs) or aggregated as a single binary blob (via securityfs).

thanks,

Mimi

Re: [PATCH RESEND v4 0/1] add sysfs exports for TPM 2 PCR registers

[James Bottomley]

On Mon, 2020-11-30 at 09:18 +0100, Greg KH wrote:
> On Sun, Nov 29, 2020 at 02:30:21PM -0800, James Bottomley wrote:
> > Cc to linux-api to get an opinion on two issues.  First the
> > background:
> > 
> > We've had a fairly extensive discussion over on linux-integrity and
> > iterated to the conclusion that the kernel does need to export TPM
> > 2.0 PCR values for use by a variety of userspace integrity
> > programmes including early boot.  The principle clinching argument
> > seems to be that these values are required by non-root systems, but
> > in a default Linux set up the packet marshalled communication
> > device: /dev/tpmrm0, is by default only usable by
> > root.  Historically, TPM 1.2 exported these values via sysfs in a
> > single file containing all 24 values:
> > 
> >   /sys/class/tpm/tpm0/pcrs
> > 
> > with the format
> > 
> >   PCR-00: 7D 29 CB 08 0C 0F C4 16 7A 0E 9A F7 C6 D3 97 CD C1 21 A7
> > 69 
> >   PCR-01: 9C B6 79 4C E4 4B 62 97 4C AB 55 13 1A 2F 7E AE 09 B3 30
> > BE 
> >   ...
> 
> As you know, this breaks the "one value per file" for sysfs, so
> please, do not add more files that do this.

I haven't ... if you read the below you'll see it's now one value per
file.

> > TPM 2.0 adds more complexity: because of it's "agile" format, each
> > TPM 2.0 is required to support a set of hashes (of which at least
> > sha1 and sha256 are required but quite a few TPM 2.0s have at least
> > two or three more) and maintain 24 PCR registers for each supported
> > hash. The current patch exports each PCR bank under the directory
> > 
> >   /sys/class/tpm/tpm0/pcr-<hash>/<bank>
> > 
> > So the sha256 bank value of PCR 7 can be obtained as
> > 
> >   cat /sys/class/tpm/tpm0/pcr-sha256/7
> >   2ED93F199692DC6788EFA6A1FE74514AB9760B2A6CEEAEF6C808C13E4ABB0D42
> > 
> > And the output is a single non-space separated ascii hex value of
> > the hash.
> > 
> > The issues we'd like input on are:
> > 
> >  1. Should this be in sysfs or securityfs?
> 
> If you want to use sysfs, use one value per file please.

It is ... each PCR gives one hash value per PCR and bank.  That's what
the file above is showing.  The hash values are 20 bytes for sha1, 32
bytes for sha256 and so on.

> >   2. Should we export the values as one value per file (current
> > patch)
> >      or as a binary blob of all 24?
> 
> Binary sysfs files are for "pass-through" mode where the kernel is
> not parsing/manipulating the data at all.  Do these values come
> straight from the hardware?  If so, sure, use a binary blob.  If not,
> then no, do not use that in sysfs as sysfs is to be in text format.

There was a question over whether the hash should be ascii as above
(hex representation) so human readable, or the 20/32/whatever binary
bytes of the hash.  I think we've got that resolved that ascii works.

James

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

[Elliott, Robert (Servers)]

...
> + * 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

...
> +
> +	/* 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.

It might help to print the bank id (variable i) as well.

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

[James Bottomley]

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.

> ...
> > +
> > +	/* 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?

James

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

[Elliott, Robert (Servers)]

> -----Original Message-----
> From: James Bottomley <James.Bottomley@HansenPartnership.com>
> Sent: Monday, November 30, 2020 1:54 PM
> To: Elliott, Robert (Servers) <elliott@hpe.com>; linux-
> integrity@vger.kernel.org
> Cc: Mimi Zohar <zohar@linux.ibm.com>; Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com>; linux-api@vger.kernel.org
> 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.

> 
> > ...
> > > +
> > > +	/* 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).

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

[James Bottomley]

On Mon, 2020-11-30 at 22:50 +0000, Elliott, Robert (Servers) wrote:
> > -----Original Message-----
> > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Sent: Monday, November 30, 2020 1:54 PM
> > To: Elliott, Robert (Servers) <elliott@hpe.com>; linux-
> > integrity@vger.kernel.org
> > Cc: Mimi Zohar <zohar@linux.ibm.com>; Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com>; linux-api@vger.kernel.org
> > 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

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

[Jarkko Sakkinen]

On Sun, Nov 29, 2020 at 02:30:22PM -0800, James Bottomley wrote:
> +		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);

Please use pr_info() here instead. We don't want to dump the stack
because of this. Nothing is working as far as kernel goes in an
unexpected manner.

/Jarkko

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

[Ken Goldman]

On 11/30/2020 5:50 PM, Elliott, Robert (Servers) wrote:
> 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.

AFIAK, no for SHA-3

Generally, TPM vendors do not implement optional features.  Since
applications cannot use them if they want interoperability, it
increases the TPM cost with no perceived benefit.

SHA-384, ECC NIST-P384, and RSA-3072 recently became mandatory.

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

[Jarkko Sakkinen]

On Mon, Sep 07, 2020 at 07:37:42PM +0200, Greg KH wrote:
> On Mon, Sep 07, 2020 at 04:21:21PM +0300, Jarkko Sakkinen wrote:
> > On Sun, Sep 06, 2020 at 01:32:45PM -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.
> > > 
> > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > 
> > I'm not sure why this should be in sysfs when event log is in
> > securityfs.
> > 
> > Also, securityfs does not have to follow sysfs requirements,
> > which gives ability to dump all PCRs in a single binary file.
> 
> You can dump all registers in a single binary file in sysfs as well :)

Thanks for confirming this.

I think sysfs makes more sense. In a production system you have a single
TPM, and that's why there is a single global event log coming from the
BIOS. That's why securityfs makes sense for the event log.

However, PCRs themselves are device local, Linux supports multiple TPM
devices, and sometimes for development and testing purposes you might
want to do this.

/Jarkko

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

[Greg KH]

On Mon, Sep 07, 2020 at 04:21:21PM +0300, Jarkko Sakkinen wrote:
> On Sun, Sep 06, 2020 at 01:32:45PM -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.
> > 
> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> 
> I'm not sure why this should be in sysfs when event log is in
> securityfs.
> 
> Also, securityfs does not have to follow sysfs requirements,
> which gives ability to dump all PCRs in a single binary file.

You can dump all registers in a single binary file in sysfs as well :)

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

[Jarkko Sakkinen]

On Sun, Sep 06, 2020 at 01:32:45PM -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.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

I'm not sure why this should be in sysfs when event log is in
securityfs.

Also, securityfs does not have to follow sysfs requirements,
which gives ability to dump all PCRs in a single binary file.

Using ASCII for this is inefficient.

/Jarkko

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

[James Bottomley]

On Mon, 2020-09-07 at 07:39 +0200, Greg KH wrote:
> On Sun, Sep 06, 2020 at 01:32:45PM -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.
> 
> Oh you are just ensuring yourself a world of hurt for drive-by
> patches that everyone submits.  Don't do this if you can help it at
> all.

Well, what do you suggest: macros that generate macros can't use
brackets around the arguments because the macro generation becomes
wrong.  This is the thing that checkpatch is insisting on because the
usual fault people commit in macros that generate C code is forgetting
that unless you bracket the arguments in the macro they can have
unintended side effects.

James

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

[Greg KH]

On Sun, Sep 06, 2020 at 01:32:45PM -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.

Oh you are just ensuring yourself a world of hurt for drive-by patches
that everyone submits.  Don't do this if you can help it at all.


> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> 
> ---
> 
> v2: fix TPM 1.2 legacy links failure
> v3: fix warn on and add note to tpm_algorithms
> v4: reword commit and add tested-by
> ---
>  drivers/char/tpm/tpm-sysfs.c | 178 +++++++++++++++++++++++++++++++++++
>  include/linux/tpm.h          |   9 +-
>  2 files changed, 186 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
> index d52bf4df0bca..81a02200b207 100644
> --- a/drivers/char/tpm/tpm-sysfs.c
> +++ b/drivers/char/tpm/tpm-sysfs.c
> @@ -348,11 +348,189 @@ static const struct attribute_group tpm2_dev_group = {
>  	.attrs = tpm2_dev_attrs,
>  };
>  
> +struct tpm_pcr_attr {
> +	int alg_id;
> +	int pcr;
> +	struct device_attribute attr;
> +};
> +
> +#define to_tpm_pcr_attr(a) container_of(a, struct tpm_pcr_attr, attr)
> +
> +static ssize_t pcr_value_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{

Like I said before, just use a binary sysfs file, that should make this
function simpler.

thanks,

greg k-h

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

[James Bottomley]

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.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

---

v2: fix TPM 1.2 legacy links failure
v3: fix warn on and add note to tpm_algorithms
v4: reword commit and add tested-by
---
 drivers/char/tpm/tpm-sysfs.c | 178 +++++++++++++++++++++++++++++++++++
 include/linux/tpm.h          |   9 +-
 2 files changed, 186 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index d52bf4df0bca..81a02200b207 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -348,11 +348,189 @@ static const struct attribute_group tpm2_dev_group = {
 	.attrs = tpm2_dev_attrs,
 };
 
+struct tpm_pcr_attr {
+	int alg_id;
+	int pcr;
+	struct device_attribute attr;
+};
+
+#define to_tpm_pcr_attr(a) container_of(a, struct tpm_pcr_attr, attr)
+
+static ssize_t pcr_value_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct tpm_pcr_attr *ha = to_tpm_pcr_attr(attr);
+	struct tpm_chip *chip = to_tpm_chip(dev);
+	struct tpm_digest digest;
+	int i;
+	int digest_size = 0;
+	int rc;
+	char *str = buf;
+
+	for (i = 0; i < chip->nr_allocated_banks; i++)
+		if (ha->alg_id == chip->allocated_banks[i].alg_id)
+			digest_size = chip->allocated_banks[i].digest_size;
+	/* should never happen */
+	if (!digest_size)
+		return -EINVAL;
+
+	digest.alg_id = ha->alg_id;
+	rc = tpm_pcr_read(chip, ha->pcr, &digest);
+	if (rc)
+		return rc;
+	for (i = 0; i < digest_size; i++)
+		str += sprintf(str, "%02X", digest.digest[i]);
+	str += sprintf(str, "\n");
+
+	return str - buf;
+}
+
+/*
+ * The following set of defines represents all the magic to build
+ * the per hash attribute groups for displaying each bank of PCRs.
+ * The only slight problem with this approach is that every PCR is
+ * hard coded to be present, so you don't know if an PCR is missing
+ * until a cat of the file returns -EINVAL
+ *
+ * Also note you must ignore checkpatch warnings in this macro
+ * code. This is deep macro magic that checkpatch.pl doesn't
+ * understand.
+ */
+
+/* Note, this must match TPM2_PLATFORM_PCR which is fixed at 24. */
+#define _TPM_HELPER(_alg, _hash, F) \
+	F(_alg, _hash, 0)	    \
+	F(_alg, _hash, 1)	    \
+	F(_alg, _hash, 2)	    \
+	F(_alg, _hash, 3)	    \
+	F(_alg, _hash, 4)	    \
+	F(_alg, _hash, 5)	    \
+	F(_alg, _hash, 6)	    \
+	F(_alg, _hash, 7)	    \
+	F(_alg, _hash, 8)	    \
+	F(_alg, _hash, 9)	    \
+	F(_alg, _hash, 10)	    \
+	F(_alg, _hash, 11)	    \
+	F(_alg, _hash, 12)	    \
+	F(_alg, _hash, 13)	    \
+	F(_alg, _hash, 14)	    \
+	F(_alg, _hash, 15)	    \
+	F(_alg, _hash, 16)	    \
+	F(_alg, _hash, 17)	    \
+	F(_alg, _hash, 18)	    \
+	F(_alg, _hash, 19)	    \
+	F(_alg, _hash, 20)	    \
+	F(_alg, _hash, 21)	    \
+	F(_alg, _hash, 22)	    \
+	F(_alg, _hash, 23)
+
+/* ignore checkpatch warning about trailing ; in macro. */
+#define PCR_ATTR(_alg, _hash, _pcr)				   \
+	static struct tpm_pcr_attr dev_attr_pcr_##_hash##_##_pcr = {	\
+		.alg_id = _alg,					   \
+		.pcr = _pcr,					   \
+		.attr = {					   \
+			.attr = {				   \
+				.name = __stringify(_pcr),	   \
+				.mode = 0444			   \
+			},					   \
+			.show = pcr_value_show			   \
+		}						   \
+	};
+
+#define PCR_ATTRS(_alg, _hash)			\
+	_TPM_HELPER(_alg, _hash, PCR_ATTR)
+
+/* ignore checkpatch warning about trailing , in macro. */
+#define PCR_ATTR_VAL(_alg, _hash, _pcr)		\
+	&dev_attr_pcr_##_hash##_##_pcr.attr.attr,
+
+#define PCR_ATTR_GROUP_ARRAY(_alg, _hash)		       \
+	static struct attribute *pcr_group_attrs_##_hash[] = { \
+		_TPM_HELPER(_alg, _hash, PCR_ATTR_VAL)	       \
+		NULL					       \
+	}
+
+#define PCR_ATTR_GROUP(_alg, _hash)			    \
+	static struct attribute_group pcr_group_##_hash = { \
+		.name = "pcr-" __stringify(_hash),	    \
+		.attrs = pcr_group_attrs_##_hash	    \
+	}
+
+#define PCR_ATTR_BUILD(_alg, _hash)	   \
+	PCR_ATTRS(_alg, _hash)		   \
+	PCR_ATTR_GROUP_ARRAY(_alg, _hash); \
+	PCR_ATTR_GROUP(_alg, _hash)
+/*
+ * End of macro structure to build an attribute group containing 24
+ * PCR value files for each supported hash algorithm
+ */
+
+/*
+ * The next set of macros implements the cleverness for each hash to
+ * build a static attribute group called pcr_group_<hash> which can be
+ * added to chip->groups[].
+ *
+ * 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);
+
+
 void tpm_sysfs_add_device(struct tpm_chip *chip)
 {
+	int i;
+
 	WARN_ON(chip->groups_cnt != 0);
+
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		chip->groups[chip->groups_cnt++] = &tpm2_dev_group;
 	else
 		chip->groups[chip->groups_cnt++] = &tpm1_dev_group;
+
+	/* 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);
+			break;
+		}
+	}
+
+	/*
+	 * This will only trigger if someone has added an additional
+	 * hash to the tpm_algorithms enum without incrementing
+	 * TPM_MAX_HASHES.
+	 */
+	WARN_ON(chip->groups_cnt > TPM_MAX_HASHES + 1);
 }
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 3b5d455501c5..cc0b94dcf21e 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -31,6 +31,7 @@ struct tpm_chip;
 struct trusted_key_payload;
 struct trusted_key_options;
 
+/* if you add a new hash to this, increment TPM_MAX_HASHES below */
 enum tpm_algorithms {
 	TPM_ALG_ERROR		= 0x0000,
 	TPM_ALG_SHA1		= 0x0004,
@@ -42,6 +43,12 @@ enum tpm_algorithms {
 	TPM_ALG_SM3_256		= 0x0012,
 };
 
+/*
+ * maximum number of hashing algorithms a TPM can have.  This is
+ * basically a count of every hash in tpm_algorithms above
+ */
+#define TPM_MAX_HASHES	5
+
 struct tpm_digest {
 	u16 alg_id;
 	u8 digest[TPM_MAX_DIGEST_SIZE];
@@ -146,7 +153,7 @@ struct tpm_chip {
 
 	struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES];
 
-	const struct attribute_group *groups[3];
+	const struct attribute_group *groups[3 + TPM_MAX_HASHES];
 	unsigned int groups_cnt;
 
 	u32 nr_allocated_banks;
-- 
2.26.2