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

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

[James Bottomley]

This represents v5 which has the spelling mistake fixed and the WARN
on unrecognized TPM hash algorithm becoming a dev_err.

We've had a fairly extensive discussion and iterated to agreement on
the output format, which becomes our ABI being one single compact hex
representation of the hash value per file according to sysfs rules,
with the file hierarchy going under

  /sys/class/tpm/tmp<x>/pcr-<hash>/<pcr number>

So to get the value of PCR 7 in the sha256 bank of the default TPM I'd do

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

If you need the binary hash of a set of PCRs, as is required for TPM
policy statements that lock to PCRs, you'd use something like:

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

Which produces the binary hash of PCRs 1, 6 and 7 in that order.

Note that this patch also adds the sha1 bank for TPM 1.2 in the same
manner (one file per PCR) but does not remove the existing pcrs file
which has the space separated all PCRs in one file format of

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


James

---

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

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

-- 
2.26.2

[PATCH v5 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
v5: algorithm spelling fix WARN->dev_err
---
 drivers/char/tpm/tpm-sysfs.c | 179 +++++++++++++++++++++++++++++++++++
 include/linux/tpm.h          |   9 +-
 2 files changed, 187 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index e2ff0b273a0f..63f03cfb8e6a 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -337,11 +337,190 @@ 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 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.
+			 */
+			dev_err(&chip->dev,
+				"TPM with unsupported bank algorithm 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 df56d7fbe78f..c4ca52138e8b 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 v5 1/1] tpm: add sysfs exports for all banks of PCR registers

[Greg KH]

On Tue, Jan 12, 2021 at 05:59:58PM -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
> v5: algorithm spelling fix WARN->dev_err
> ---
>  drivers/char/tpm/tpm-sysfs.c | 179 +++++++++++++++++++++++++++++++++++
>  include/linux/tpm.h          |   9 +-
>  2 files changed, 187 insertions(+), 1 deletion(-)

You add new sysfs files, but do not add Documentation/ABI/ entries
showing how they are used and what they contain :(

Please do that for the next version of this patch.

thanks,

greg k-h

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

[Greg KH]

On Tue, Jan 12, 2021 at 05:59:58PM -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
> v5: algorithm spelling fix WARN->dev_err
> ---
>  drivers/char/tpm/tpm-sysfs.c | 179 +++++++++++++++++++++++++++++++++++
>  include/linux/tpm.h          |   9 +-
>  2 files changed, 187 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
> index e2ff0b273a0f..63f03cfb8e6a 100644
> --- a/drivers/char/tpm/tpm-sysfs.c
> +++ b/drivers/char/tpm/tpm-sysfs.c
> @@ -337,11 +337,190 @@ 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");

sysfs_emit()?




> +
> +	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			   \
> +			},					   \

__ATTR_RO()?

>  void tpm_sysfs_add_device(struct tpm_chip *chip)
>  {
> +	int i;
> +
>  	WARN_ON(chip->groups_cnt != 0);
> +

How can that WARN_ON happen?

thanks,

greg k-h

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

[James Bottomley]

On Wed, 2021-01-13 at 08:48 +0100, Greg KH wrote:
> On Tue, Jan 12, 2021 at 05:59:58PM -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
> > v5: algorithm spelling fix WARN->dev_err
> > ---
> >  drivers/char/tpm/tpm-sysfs.c | 179
> > +++++++++++++++++++++++++++++++++++
> >  include/linux/tpm.h          |   9 +-
> >  2 files changed, 187 insertions(+), 1 deletion(-)
> 
> You add new sysfs files, but do not add Documentation/ABI/ entries
> showing how they are used and what they contain :(
> 
> Please do that for the next version of this patch.

It's a bit of a chicken and egg problem since I've no idea when this
will go upstream and the entries require that information making the
ABI more of a post accept type thing.  I can make a guess about the
values if Jarkko is going to but this in for the next merge window.

James

---8>8>8><8<8<8---

From: James Bottomley <James.Bottomley@HansenPartnership.com>
Subject: [PATCH] ABI: add sysfs description for tpm exports of PCR registers

Adds the ABI entries for the new

/sys/class/tpm/tpm<n>/pcr-<hash>/<m>

files which are added to export the PCR hash values on a one value per
file basis.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 Documentation/ABI/stable/sysfs-class-tpm | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/ABI/stable/sysfs-class-tpm b/Documentation/ABI/stable/sysfs-class-tpm
index 91ca63ec7581..d897ecb9615f 100644
--- a/Documentation/ABI/stable/sysfs-class-tpm
+++ b/Documentation/ABI/stable/sysfs-class-tpm
@@ -194,3 +194,17 @@ Description:	The "tpm_version_major" property shows the TCG spec major version
 		Example output::
 
 		  2
+
+What:		/sys/class/tpm/tpmX/pcr-H/N
+Date:		March 2021
+KernelVersion:	5.12
+Contact:	linux-integrity@vger.kernel.org
+Description:	produces output in compact hex representation for PCR
+		number N from hash bank H.  N is the numeric value of
+		the PCR number and H is the crypto string
+		representation of the hash
+
+		Example output::
+
+		  cat /sys/class/tpm/tpm0/pcr-sha256/7
+		  2ED93F199692DC6788EFA6A1FE74514AB9760B2A6CEEAEF6C808C13E4ABB0D42

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

[Jarkko Sakkinen]

On Tue, Jan 12, 2021 at 05:59:57PM -0800, James Bottomley wrote:
> This represents v5 which has the spelling mistake fixed and the WARN
> on unrecognized TPM hash algorithm becoming a dev_err.
> 
> We've had a fairly extensive discussion and iterated to agreement on
> the output format, which becomes our ABI being one single compact hex
> representation of the hash value per file according to sysfs rules,
> with the file hierarchy going under
> 
>   /sys/class/tpm/tmp<x>/pcr-<hash>/<pcr number>
> 
> So to get the value of PCR 7 in the sha256 bank of the default TPM I'd do
> 
>    cat /sys/class/tpm/tpm0/pcr-sha256/7
>    2ED93F199692DC6788EFA6A1FE74514AB9760B2A6CEEAEF6C808C13E4ABB0D42
> 
> If you need the binary hash of a set of PCRs, as is required for TPM
> policy statements that lock to PCRs, you'd use something like:
> 
>   cat /sys/class/tpm/tpm0/pcr-sha256/{1,6,7}|xxd -r -p|sha256sum
> 
> Which produces the binary hash of PCRs 1, 6 and 7 in that order.
> 
> Note that this patch also adds the sha1 bank for TPM 1.2 in the same
> manner (one file per PCR) but does not remove the existing pcrs file
> which has the space separated all PCRs in one file format of
> 
>   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 
>   ...
> 
> 
> James
> 
> ---
> 
> James Bottomley (1):
>   tpm: add sysfs exports for all banks of PCR registers
> 
>  drivers/char/tpm/tpm-sysfs.c | 179 +++++++++++++++++++++++++++++++++++
>  include/linux/tpm.h          |   9 +-
>  2 files changed, 187 insertions(+), 1 deletion(-)
> 
> -- 
> 2.26.2
> 

After clearing out Greg's remarks, can you use jarkko@kernel.org
for the next version?

/Jarkko

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

[James Bottomley]

On Wed, 2021-01-13 at 22:48 +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 12, 2021 at 05:59:57PM -0800, James Bottomley wrote:
> > This represents v5 which has the spelling mistake fixed and the
> > WARN
> > on unrecognized TPM hash algorithm becoming a dev_err.
> > 
> > We've had a fairly extensive discussion and iterated to agreement
> > on
> > the output format, which becomes our ABI being one single compact
> > hex
> > representation of the hash value per file according to sysfs rules,
> > with the file hierarchy going under
> > 
> >   /sys/class/tpm/tmp<x>/pcr-<hash>/<pcr number>
> > 
> > So to get the value of PCR 7 in the sha256 bank of the default TPM
> > I'd do
> > 
> >    cat /sys/class/tpm/tpm0/pcr-sha256/7
> >    2ED93F199692DC6788EFA6A1FE74514AB9760B2A6CEEAEF6C808C13E4ABB0D42
> > 
> > If you need the binary hash of a set of PCRs, as is required for
> > TPM
> > policy statements that lock to PCRs, you'd use something like:
> > 
> >   cat /sys/class/tpm/tpm0/pcr-sha256/{1,6,7}|xxd -r -p|sha256sum
> > 
> > Which produces the binary hash of PCRs 1, 6 and 7 in that order.
> > 
> > Note that this patch also adds the sha1 bank for TPM 1.2 in the
> > same
> > manner (one file per PCR) but does not remove the existing pcrs
> > file
> > which has the space separated all PCRs in one file format of
> > 
> >   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 
> >   ...
> > 
> > 
> > James
> > 
> > ---
> > 
> > James Bottomley (1):
> >   tpm: add sysfs exports for all banks of PCR registers
> > 
> >  drivers/char/tpm/tpm-sysfs.c | 179
> > +++++++++++++++++++++++++++++++++++
> >  include/linux/tpm.h          |   9 +-
> >  2 files changed, 187 insertions(+), 1 deletion(-)
> > 
> > -- 
> > 2.26.2
> > 
> 
> After clearing out Greg's remarks, can you use jarkko@kernel.org
> for the next version?

Sorry, it's a force of habit process: i go back to version n-1 and copy
the to and cc list ... so the promise to sent to kernel.org didn't get
realised.

I'll just do a resend since there are no changes requested.

James

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

[Jarkko Sakkinen]

On Wed, Jan 13, 2021 at 09:31:44AM -0800, James Bottomley wrote:
> On Wed, 2021-01-13 at 08:48 +0100, Greg KH wrote:
> > On Tue, Jan 12, 2021 at 05:59:58PM -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
> > > v5: algorithm spelling fix WARN->dev_err
> > > ---
> > >  drivers/char/tpm/tpm-sysfs.c | 179
> > > +++++++++++++++++++++++++++++++++++
> > >  include/linux/tpm.h          |   9 +-
> > >  2 files changed, 187 insertions(+), 1 deletion(-)
> > 
> > You add new sysfs files, but do not add Documentation/ABI/ entries
> > showing how they are used and what they contain :(
> > 
> > Please do that for the next version of this patch.
> 
> It's a bit of a chicken and egg problem since I've no idea when this
> will go upstream and the entries require that information making the
> ABI more of a post accept type thing.  I can make a guess about the
> values if Jarkko is going to but this in for the next merge window.
> 
> James

I agree with the ABI side, so you can safely include this to the patch set. 
And yes, this looks like something I can include to the 5.12 PR.

Did you address Greg's remarks about warns?

Other than that, please send a version with ABI entries  so that
we can move forward with this.

/Jarkko

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

[James Bottomley]

On Thu, 2021-01-14 at 00:14 +0200, Jarkko Sakkinen wrote:
> On Wed, Jan 13, 2021 at 09:31:44AM -0800, James Bottomley wrote:
> > On Wed, 2021-01-13 at 08:48 +0100, Greg KH wrote:
> > > On Tue, Jan 12, 2021 at 05:59:58PM -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
> > > > v5: algorithm spelling fix WARN->dev_err
> > > > ---
> > > >  drivers/char/tpm/tpm-sysfs.c | 179
> > > > +++++++++++++++++++++++++++++++++++
> > > >  include/linux/tpm.h          |   9 +-
> > > >  2 files changed, 187 insertions(+), 1 deletion(-)
> > > 
> > > You add new sysfs files, but do not add Documentation/ABI/
> > > entries
> > > showing how they are used and what they contain :(
> > > 
> > > Please do that for the next version of this patch.
> > 
> > It's a bit of a chicken and egg problem since I've no idea when
> > this
> > will go upstream and the entries require that information making
> > the
> > ABI more of a post accept type thing.  I can make a guess about the
> > values if Jarkko is going to but this in for the next merge window.
> > 
> > James
> 
> I agree with the ABI side, so you can safely include this to the
> patch set. 
> And yes, this looks like something I can include to the 5.12 PR.
> 
> Did you address Greg's remarks about warns?

You mean Rob Elliott's?  Yes, it was in the change log

> Other than that, please send a version with ABI entries  so that
> we can move forward with this.

It's already sent ... although vger is being a bit slow at the moment.

James

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

[James Bottomley]

[separate reply because the asked about part isn't in my patch it's in
existing code]

On Wed, 2021-01-13 at 08:50 +0100, Greg KH wrote:
> On Tue, Jan 12, 2021 at 05:59:58PM -0800, James Bottomley wrote:
[...]
> >  void tpm_sysfs_add_device(struct tpm_chip *chip)
> >  {
> > +	int i;
> > +
> >  	WARN_ON(chip->groups_cnt != 0);
> > +
> 
> How can that WARN_ON happen?

If tpm_sysfs_add_device gets called more than once, say because reuse
of the chip structure that causes it to be initialized again without
properly being torn down.  I think it's a reasonable assert given that
we'll run off the end of the chip->groups array if it isn't true ...
which does really argue it should be a BUG_ON because the machine will
be compromised and likely unrecoverable if it triggers.

James