[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, 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

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

[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

[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 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 0/1] add sysfs exports for TPM 2 PCR registers

[Greg KH]

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

Ick, that's not "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?
> 
>   2. Should we export the values as one value per file (current patch)
>      or as a binary blob of all 24?

Please just use a binary blob format.  Binary sysfs files are exactly
what this is for, you are just passing the data through the kernel from
the hardware to userspace.

You can have 24 binary files if that makes it easier, but the existing
format really is an abuse of sysfs.

Or use securityfs, that's fine too, but as you say, you have to write
more code for that.

thanks,

greg k-h

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

[Jarkko Sakkinen]

On Mon, Sep 07, 2020 at 07:38:24AM +0200, Greg KH wrote:
> Please just use a binary blob format.  Binary sysfs files are exactly
> what this is for, you are just passing the data through the kernel from
> the hardware to userspace.
> 
> You can have 24 binary files if that makes it easier, but the existing
> format really is an abuse of sysfs.
> 
> Or use securityfs, that's fine too, but as you say, you have to write
> more code for that.
> 
> thanks,
> 
> greg k-h

I suggested this in previous round: to have a single 'pcrs' binary file
with <TPM Alg ID, blob> pairs contained.

/Jarkko

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

[Greg KH]

On Mon, Sep 07, 2020 at 04:23:22PM +0300, Jarkko Sakkinen wrote:
> On Mon, Sep 07, 2020 at 07:38:24AM +0200, Greg KH wrote:
> > Please just use a binary blob format.  Binary sysfs files are exactly
> > what this is for, you are just passing the data through the kernel from
> > the hardware to userspace.
> > 
> > You can have 24 binary files if that makes it easier, but the existing
> > format really is an abuse of sysfs.
> > 
> > Or use securityfs, that's fine too, but as you say, you have to write
> > more code for that.
> > 
> > thanks,
> > 
> > greg k-h
> 
> I suggested this in previous round: to have a single 'pcrs' binary file
> with <TPM Alg ID, blob> pairs contained.

That's fine with me!

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

[James Bottomley]

On Mon, 2020-09-07 at 16:23 +0300, Jarkko Sakkinen wrote:
> On Mon, Sep 07, 2020 at 07:38:24AM +0200, Greg KH wrote:
> > Please just use a binary blob format.  Binary sysfs files are
> > exactly what this is for, you are just passing the data through the
> > kernel from the hardware to userspace.
> > 
> > You can have 24 binary files if that makes it easier, but the
> > existing format really is an abuse of sysfs.

There is no existing format for TPM 2.0 ... that's part of the problem
since we certainly didn't want to carry over the TPM 1.2 format.

I've got to say I think binary attributes are actively evil.  I can see
they're a necessity when there's no good way to represent the data they
contain, like the bios measurement log or firmware code or a raw
interface like we do for the SMP frame code in libsas.  But when
there's a well understood and easy to produce user friendly non-binary
representation, I think dumping binary is inimical to being a good API.


> > Or use securityfs, that's fine too, but as you say, you have to
> > write more code for that.
> > 
> > thanks,
> > 
> > greg k-h
> 
> I suggested this in previous round: to have a single 'pcrs' binary
> file with <TPM Alg ID, blob> pairs contained.

There's no current use case today that wants all values.  Every current
use case wants either a single PCR or a selection mostly from a single
bank, so forcing every current user to dig out the values they want 
from a binary blob rather than being able to gather them simply also
seems to be an API that makes users' lives harder than they need to be.

James

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

[Greg KH]

On Mon, Sep 07, 2020 at 02:52:08PM -0700, James Bottomley wrote:
> On Mon, 2020-09-07 at 16:23 +0300, Jarkko Sakkinen wrote:
> > On Mon, Sep 07, 2020 at 07:38:24AM +0200, Greg KH wrote:
> > > Please just use a binary blob format.  Binary sysfs files are
> > > exactly what this is for, you are just passing the data through the
> > > kernel from the hardware to userspace.
> > > 
> > > You can have 24 binary files if that makes it easier, but the
> > > existing format really is an abuse of sysfs.
> 
> There is no existing format for TPM 2.0 ... that's part of the problem
> since we certainly didn't want to carry over the TPM 1.2 format.

Ok, then no, if there is not already a binary format then you should not
use a binary sysfs file as you are then just sending a kernel structure
to userspace, not a hardware structure.

> I've got to say I think binary attributes are actively evil.  I can see
> they're a necessity when there's no good way to represent the data they
> contain, like the bios measurement log or firmware code or a raw
> interface like we do for the SMP frame code in libsas.  But when
> there's a well understood and easy to produce user friendly non-binary
> representation, I think dumping binary is inimical to being a good API.

Agreed.

thanks,

greg k-h

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

[Jarkko Sakkinen]

On Tue, Sep 08, 2020 at 07:45:52AM +0200, Greg KH wrote:
> On Mon, Sep 07, 2020 at 02:52:08PM -0700, James Bottomley wrote:
> > On Mon, 2020-09-07 at 16:23 +0300, Jarkko Sakkinen wrote:
> > > On Mon, Sep 07, 2020 at 07:38:24AM +0200, Greg KH wrote:
> > > > Please just use a binary blob format.  Binary sysfs files are
> > > > exactly what this is for, you are just passing the data through the
> > > > kernel from the hardware to userspace.
> > > > 
> > > > You can have 24 binary files if that makes it easier, but the
> > > > existing format really is an abuse of sysfs.
> > 
> > There is no existing format for TPM 2.0 ... that's part of the problem
> > since we certainly didn't want to carry over the TPM 1.2 format.
> 
> Ok, then no, if there is not already a binary format then you should not
> use a binary sysfs file as you are then just sending a kernel structure
> to userspace, not a hardware structure.
> 
> > I've got to say I think binary attributes are actively evil.  I can see
> > they're a necessity when there's no good way to represent the data they
> > contain, like the bios measurement log or firmware code or a raw
> > interface like we do for the SMP frame code in libsas.  But when
> > there's a well understood and easy to produce user friendly non-binary
> > representation, I think dumping binary is inimical to being a good API.
> 
> Agreed.
> 
> thanks,
> 
> greg k-h

Looking at the patch, something like <device>/pcrs/<hash>/<index> would
be a bit cleaner representation than the current <device>/pcrs-<hash>/<index>.

/Jarkko

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

[James Bottomley]

On Tue, 2020-09-08 at 21:05 +0300, Jarkko Sakkinen wrote:
> On Tue, Sep 08, 2020 at 07:45:52AM +0200, Greg KH wrote:
> > On Mon, Sep 07, 2020 at 02:52:08PM -0700, James Bottomley wrote:
[...]
> > > I've got to say I think binary attributes are actively evil.  I
> > > can see
> > > they're a necessity when there's no good way to represent the
> > > data they
> > > contain, like the bios measurement log or firmware code or a raw
> > > interface like we do for the SMP frame code in libsas.  But when
> > > there's a well understood and easy to produce user friendly non-
> > > binary
> > > representation, I think dumping binary is inimical to being a
> > > good API.
> > 
> > Agreed.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Looking at the patch, something like <device>/pcrs/<hash>/<index>
> would be a bit cleaner representation than the current <device>/pcrs-
> <hash>/<index>.

That's actually a technical limitation of using the current attribute
groups API: It's designed to support single level directories in sysfs
(or no directory at all).  That's not to say we can't do multi-level
ones, but if we do we have to roll our own machinery for managing the
files rather than relying on the groups API.

Given that the current groups API does all the nasty lifetime
management that I'd otherwise have to do in the patch, I have a strong
incentive for keeping it, which is why the single <device>/pcrs-
<hash>/<index> format.

James

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

[Greg KH]

On Tue, Sep 08, 2020 at 11:14:11AM -0700, James Bottomley wrote:
> On Tue, 2020-09-08 at 21:05 +0300, Jarkko Sakkinen wrote:
> > On Tue, Sep 08, 2020 at 07:45:52AM +0200, Greg KH wrote:
> > > On Mon, Sep 07, 2020 at 02:52:08PM -0700, James Bottomley wrote:
> [...]
> > > > I've got to say I think binary attributes are actively evil.  I
> > > > can see
> > > > they're a necessity when there's no good way to represent the
> > > > data they
> > > > contain, like the bios measurement log or firmware code or a raw
> > > > interface like we do for the SMP frame code in libsas.  But when
> > > > there's a well understood and easy to produce user friendly non-
> > > > binary
> > > > representation, I think dumping binary is inimical to being a
> > > > good API.
> > > 
> > > Agreed.
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > Looking at the patch, something like <device>/pcrs/<hash>/<index>
> > would be a bit cleaner representation than the current <device>/pcrs-
> > <hash>/<index>.
> 
> That's actually a technical limitation of using the current attribute
> groups API: It's designed to support single level directories in sysfs
> (or no directory at all).  That's not to say we can't do multi-level
> ones, but if we do we have to roll our own machinery for managing the
> files rather than relying on the groups API.

Agreed, do NOT do multi-level attribute groups please, userspace tools
will not handle them well, if at all.

> Given that the current groups API does all the nasty lifetime
> management that I'd otherwise have to do in the patch, I have a strong
> incentive for keeping it, which is why the single <device>/pcrs-
> <hash>/<index> format.

Agreed.

thanks,

greg k-h

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

[Jarkko Sakkinen]

On Wed, Sep 09, 2020 at 09:07:29AM +0200, Greg KH wrote:
> On Tue, Sep 08, 2020 at 11:14:11AM -0700, James Bottomley wrote:
> > On Tue, 2020-09-08 at 21:05 +0300, Jarkko Sakkinen wrote:
> > > On Tue, Sep 08, 2020 at 07:45:52AM +0200, Greg KH wrote:
> > > > On Mon, Sep 07, 2020 at 02:52:08PM -0700, James Bottomley wrote:
> > [...]
> > > > > I've got to say I think binary attributes are actively evil.  I
> > > > > can see
> > > > > they're a necessity when there's no good way to represent the
> > > > > data they
> > > > > contain, like the bios measurement log or firmware code or a raw
> > > > > interface like we do for the SMP frame code in libsas.  But when
> > > > > there's a well understood and easy to produce user friendly non-
> > > > > binary
> > > > > representation, I think dumping binary is inimical to being a
> > > > > good API.
> > > > 
> > > > Agreed.
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > 
> > > Looking at the patch, something like <device>/pcrs/<hash>/<index>
> > > would be a bit cleaner representation than the current <device>/pcrs-
> > > <hash>/<index>.
> > 
> > That's actually a technical limitation of using the current attribute
> > groups API: It's designed to support single level directories in sysfs
> > (or no directory at all).  That's not to say we can't do multi-level
> > ones, but if we do we have to roll our own machinery for managing the
> > files rather than relying on the groups API.
> 
> Agreed, do NOT do multi-level attribute groups please, userspace tools
> will not handle them well, if at all.

OK, thanks for confirming this.

/Jarkko

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

[Jarkko Sakkinen]

On Tue, Sep 08, 2020 at 11:14:11AM -0700, James Bottomley wrote:
> On Tue, 2020-09-08 at 21:05 +0300, Jarkko Sakkinen wrote:
> > On Tue, Sep 08, 2020 at 07:45:52AM +0200, Greg KH wrote:
> > > On Mon, Sep 07, 2020 at 02:52:08PM -0700, James Bottomley wrote:
> [...]
> > > > I've got to say I think binary attributes are actively evil.  I
> > > > can see
> > > > they're a necessity when there's no good way to represent the
> > > > data they
> > > > contain, like the bios measurement log or firmware code or a raw
> > > > interface like we do for the SMP frame code in libsas.  But when
> > > > there's a well understood and easy to produce user friendly non-
> > > > binary
> > > > representation, I think dumping binary is inimical to being a
> > > > good API.
> > > 
> > > Agreed.
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > Looking at the patch, something like <device>/pcrs/<hash>/<index>
> > would be a bit cleaner representation than the current <device>/pcrs-
> > <hash>/<index>.
> 
> That's actually a technical limitation of using the current attribute
> groups API: It's designed to support single level directories in sysfs
> (or no directory at all).  That's not to say we can't do multi-level
> ones, but if we do we have to roll our own machinery for managing the
> files rather than relying on the groups API.
> 
> Given that the current groups API does all the nasty lifetime
> management that I'd otherwise have to do in the patch, I have a strong
> incentive for keeping it, which is why the single <device>/pcrs-
> <hash>/<index> format.

OK, I do get that. I've tried to do something similar in past and it
turnd out to be a tremendous job.  I guess I'll have to re-review the
whole patch again. I'll prioritize that next week.

> James

/Jarkko

[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 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

[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 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