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

[PATCH v5 0/2] 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 (2):
  tpm: add sysfs exports for all banks of PCR registers
  ABI: add sysfs description for tpm exports of PCR registers

 Documentation/ABI/stable/sysfs-class-tpm |  14 ++
 drivers/char/tpm/tpm-sysfs.c             | 179 +++++++++++++++++++++++
 include/linux/tpm.h                      |   9 +-
 3 files changed, 201 insertions(+), 1 deletion(-)

-- 
2.26.2

[PATCH v5 1/2] 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

[PATCH v5 2/2] ABI: add sysfs description for tpm exports of PCR registers

[James Bottomley]

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

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

[Greg KH]

On Wed, Jan 13, 2021 at 03:26:33PM -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");

Please use sysfs_emit() and sysfs_emit_at() for new sysfs files.

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

Can you use __ATTR_RO()?  "open" coding the sysfs mode is frowned apon
these days.

thanks,

greg k-h

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

[James Bottomley]

On Thu, 2021-01-14 at 08:59 +0100, Greg KH wrote:
> On Wed, Jan 13, 2021 at 03:26:33PM -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");
> 
> Please use sysfs_emit() and sysfs_emit_at() for new sysfs files.

Hey these interfaces were added after this patch began life.  But
looking at sysfs_emit_at() I've got to say "aah ... don't you guys ever
read rusty's guide to interfaces?" an interface which takes in an
absolute page position but returns a relative offset to the position it
took in is asking for people to get it wrong.  You should always be
consistent about uses for inputs and outputs.  Basically the only way
you can ever use sysfs_emit_at in a show routine is as

offset += sysfs_emit_at(buf, offset, ...);

because you always need to track the absolute offset.

It looks like we already have a couple of bugs in the kernel introduced
by this confusion ...  return sysfs_emit() vs return sysfs_emit_at()
being the most tricky ...

> > +/* 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			   
> > \
> 
> Can you use __ATTR_RO()?  "open" coding the sysfs mode is frowned
> apon these days.

No because the .show function is the same for every attribute even
though the name is different.  Somewhere way back at the beginning of
this there was a thread about trying to use the ATTR macros, but the
problem is there are multiple hash banks that each want files called
"1" "2" and so on ... we just can't structure the show functions to be
one per the entire attribute set without either including the hash in
the name, which we don't want because it's in the directory, or
creating clashes in the .show file.

James

Re: [PATCH v5 2/2] ABI: add sysfs description for tpm exports of PCR registers

[Jarkko Sakkinen]

On Wed, Jan 13, 2021 at 03:26:34PM -0800, James Bottomley wrote:
> 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>

Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

/Jarkko

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

[Jarkko Sakkinen]

On Wed, Jan 13, 2021 at 03:26:33PM -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>
> 
> ---

Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

/Jarkko

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

[Jarkko Sakkinen]

On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley wrote:
> On Thu, 2021-01-14 at 08:59 +0100, Greg KH wrote:
> > On Wed, Jan 13, 2021 at 03:26:33PM -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");
> > 
> > Please use sysfs_emit() and sysfs_emit_at() for new sysfs files.
> 
> Hey these interfaces were added after this patch began life.  But
> looking at sysfs_emit_at() I've got to say "aah ... don't you guys ever
> read rusty's guide to interfaces?" an interface which takes in an
> absolute page position but returns a relative offset to the position it
> took in is asking for people to get it wrong.  You should always be
> consistent about uses for inputs and outputs.  Basically the only way
> you can ever use sysfs_emit_at in a show routine is as
> 
> offset += sysfs_emit_at(buf, offset, ...);
> 
> because you always need to track the absolute offset.
> 
> It looks like we already have a couple of bugs in the kernel introduced
> by this confusion ...  return sysfs_emit() vs return sysfs_emit_at()
> being the most tricky ...

How is using sysfs_emit() different from using snprintf() for the
caller, ignoring the added safety measures? I'm new to this API.

> > > +/* 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			   
> > > \
> > 
> > Can you use __ATTR_RO()?  "open" coding the sysfs mode is frowned
> > apon these days.
> 
> No because the .show function is the same for every attribute even
> though the name is different.  Somewhere way back at the beginning of
> this there was a thread about trying to use the ATTR macros, but the
> problem is there are multiple hash banks that each want files called
> "1" "2" and so on ... we just can't structure the show functions to be
> one per the entire attribute set without either including the hash in
> the name, which we don't want because it's in the directory, or
> creating clashes in the .show file.

One could introduce __ATTR_RO_SHOW(), like there is __ATTR_RO_MODE().
Not sure if this worth of trouble.

> James

/Jarkko

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

[Greg KH]

On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley wrote:
> On Thu, 2021-01-14 at 08:59 +0100, Greg KH wrote:
> > On Wed, Jan 13, 2021 at 03:26:33PM -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");
> > 
> > Please use sysfs_emit() and sysfs_emit_at() for new sysfs files.
> 
> Hey these interfaces were added after this patch began life.  But
> looking at sysfs_emit_at() I've got to say "aah ... don't you guys ever
> read rusty's guide to interfaces?" an interface which takes in an
> absolute page position but returns a relative offset to the position it
> took in is asking for people to get it wrong.  You should always be
> consistent about uses for inputs and outputs.  Basically the only way
> you can ever use sysfs_emit_at in a show routine is as
> 
> offset += sysfs_emit_at(buf, offset, ...);
> 
> because you always need to track the absolute offset.

Well, you shouldn't be doing anything other than a "normal" single value
write, so the _at() function should be rare.

> It looks like we already have a couple of bugs in the kernel introduced
> by this confusion ...  return sysfs_emit() vs return sysfs_emit_at()
> being the most tricky ...

Hm, Joe, you did the conversion to these functions (and wrote the api),
care to review this?

> > > +/* 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			   
> > > \
> > 
> > Can you use __ATTR_RO()?  "open" coding the sysfs mode is frowned
> > apon these days.
> 
> No because the .show function is the same for every attribute even
> though the name is different.  Somewhere way back at the beginning of
> this there was a thread about trying to use the ATTR macros, but the
> problem is there are multiple hash banks that each want files called
> "1" "2" and so on ... we just can't structure the show functions to be
> one per the entire attribute set without either including the hash in
> the name, which we don't want because it's in the directory, or
> creating clashes in the .show file.

Ah, missed that you were using the same show() function.  Ok, makes
sense, but it feels odd to have your own attribute type for something as
"simple" as a tiny driver like this.  But you are maintaining it, not me
:)

thanks,

greg k-h

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

[James Bottomley]

On Fri, 2021-01-15 at 08:55 +0200, Jarkko Sakkinen wrote:
> On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley wrote:
> > On Thu, 2021-01-14 at 08:59 +0100, Greg KH wrote:
[...]
> > > Please use sysfs_emit() and sysfs_emit_at() for new sysfs files.
> > 
> > Hey these interfaces were added after this patch began life.  But
> > looking at sysfs_emit_at() I've got to say "aah ... don't you guys
> > ever read rusty's guide to interfaces?" an interface which takes in
> > an absolute page position but returns a relative offset to the
> > position it took in is asking for people to get it wrong.  You
> > should always be consistent about uses for inputs and
> > outputs.  Basically the only way you can ever use sysfs_emit_at in
> > a show routine is as
> > 
> > offset += sysfs_emit_at(buf, offset, ...);
> > 
> > because you always need to track the absolute offset.
> > 
> > It looks like we already have a couple of bugs in the kernel
> > introduced by this confusion ...  return sysfs_emit() vs return
> > sysfs_emit_at() being the most tricky ...
> 
> How is using sysfs_emit() different from using snprintf() for the
> caller, ignoring the added safety measures? I'm new to this API.

Using the sprintX variants you maintain a cursor pointer, so they all
look like

char *cursor = buf;

...

cursor += sprintX(cursor, "...", ...

...

return cursor - buf;

So the input is a relative cursor and the output is the additional
offset.  I'm not claiming it's the best interface but it is hard to get
wrong, just that if we're going to force a new interface we should make
it much better.

with sysfs_emit_at you use an offset "cursor" but it's hard to know
without reading the function how to do it because the return is
relative rather than absolute.  To have an interface it would be hard
to misuse, I think the best way would be to take a pointer to the
offset and adjust it after use, so

sysfs_emit_at(buf, &offset, ...);

That way it returns void so you can't use it in place of 

return sysfs_emit()

And you don't have to worry about whether the return is absolute or
relative because it adjusts the pointer for you.

The whole point about Rusty and interfaces is that if you are going to
invent new interfaces you should make them easy to get right and hard
to misuse.  A function you can't figure out how to use until you read
the source is about 2/10 on the rusty scale:

https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html

James

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

[James Bottomley]

On Fri, 2021-01-15 at 14:54 +0100, Greg KH wrote:
> On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley wrote:
[...]
> > It looks like we already have a couple of bugs in the kernel
> > introduced by this confusion ...  return sysfs_emit() vs return
> > sysfs_emit_at() being the most tricky ...
> 
> Hm, Joe, you did the conversion to these functions (and wrote the
> api), care to review this?

A cursory glance tells me that summary_show in 
drivers/infiniband/hw/usnic/usnic_ib_sysfs.c has a problem, I think the
last = should be +=

James

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

[James Bottomley]

On Fri, 2021-01-15 at 09:26 -0800, James Bottomley wrote:
> On Fri, 2021-01-15 at 14:54 +0100, Greg KH wrote:
> > On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley wrote:
> [...]
> > > It looks like we already have a couple of bugs in the kernel
> > > introduced by this confusion ...  return sysfs_emit() vs return
> > > sysfs_emit_at() being the most tricky ...
> > 
> > Hm, Joe, you did the conversion to these functions (and wrote the
> > api), care to review this?
> 
> A cursory glance tells me that summary_show in 
> drivers/infiniband/hw/usnic/usnic_ib_sysfs.c has a problem, I think
> the last = should be +=

The use in drivers/base/node.c:node_read_meminfo() is highly
questionable.  While currently not emitting wrong code, it depends on
len being 0 when passed in to sysfs_emit_at().  That argues it should
either be using sysfs_emit() or it should have a len += just in case
something gets prepended that makes len non zero.

James

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

[Joe Perches]

On Fri, 2021-01-15 at 14:54 +0100, Greg KH wrote:
> On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley wrote:
[]
> > It looks like we already have a couple of bugs in the kernel introduced
> > by this confusion ...  return sysfs_emit() vs return sysfs_emit_at()
> > being the most tricky ...
> 
> Hm, Joe, you did the conversion to these functions (and wrote the api),
> care to review this?

Can you point me at the original submission?
I don't see it on lore/linux-api

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

[Joe Perches]

On Fri, 2021-01-15 at 10:07 -0800, James Bottomley wrote:
> On Fri, 2021-01-15 at 09:26 -0800, James Bottomley wrote:
> > On Fri, 2021-01-15 at 14:54 +0100, Greg KH wrote:
> > > On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley wrote:
> > [...]
> > > > It looks like we already have a couple of bugs in the kernel
> > > > introduced by this confusion ...  return sysfs_emit() vs return
> > > > sysfs_emit_at() being the most tricky ...
> > > 
> > > Hm, Joe, you did the conversion to these functions (and wrote the
> > > api), care to review this?
> > 
> > A cursory glance tells me that summary_show in 
> > drivers/infiniband/hw/usnic/usnic_ib_sysfs.c has a problem, I think
> > the last = should be +=

No, it's correct and overwriting what would otherwise be a trailing space.

> The use in drivers/base/node.c:node_read_meminfo() is highly
> questionable.  While currently not emitting wrong code, it depends on
> len being 0 when passed in to sysfs_emit_at().  That argues it should
> either be using sysfs_emit() or it should have a len += just in case
> something gets prepended that makes len non zero.

<shrug>, it's currently correct and would be OK to change to += as
len is initialized though I think it's extremely doubtful it would
ever be changed.

sysfs is ABI right so prepending would break things.

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

[James Bottomley]

On Fri, 2021-01-15 at 12:48 -0800, Joe Perches wrote:
> On Fri, 2021-01-15 at 10:07 -0800, James Bottomley wrote:
> > On Fri, 2021-01-15 at 09:26 -0800, James Bottomley wrote:
> > > On Fri, 2021-01-15 at 14:54 +0100, Greg KH wrote:
> > > > On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley
> > > > wrote:
> > > [...]
> > > > > It looks like we already have a couple of bugs in the kernel
> > > > > introduced by this confusion ...  return sysfs_emit() vs
> > > > > return
> > > > > sysfs_emit_at() being the most tricky ...
> > > > 
> > > > Hm, Joe, you did the conversion to these functions (and wrote
> > > > the
> > > > api), care to review this?
> > > 
> > > A cursory glance tells me that summary_show in 
> > > drivers/infiniband/hw/usnic/usnic_ib_sysfs.c has a problem, I
> > > think the last = should be +=
> 
> No, it's correct and overwriting what would otherwise be a trailing
> space.

The last two lines of summary_show() are

   len = sysfs_emit_at(buf, len, "\n");

   return len;

So that always returns 2: the length of "\n", rather than the length of
everything you just put into buf, which is what sysfs attributes are
supposed to return.

James

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

[Joe Perches]

On Fri, 2021-01-15 at 13:06 -0800, James Bottomley wrote:
> On Fri, 2021-01-15 at 12:48 -0800, Joe Perches wrote:
> > On Fri, 2021-01-15 at 10:07 -0800, James Bottomley wrote:
> > > On Fri, 2021-01-15 at 09:26 -0800, James Bottomley wrote:
> > > > On Fri, 2021-01-15 at 14:54 +0100, Greg KH wrote:
> > > > > On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley
> > > > > wrote:
> > > > [...]
> > > > > > It looks like we already have a couple of bugs in the kernel
> > > > > > introduced by this confusion ...  return sysfs_emit() vs
> > > > > > return
> > > > > > sysfs_emit_at() being the most tricky ...
> > > > > 
> > > > > Hm, Joe, you did the conversion to these functions (and wrote
> > > > > the
> > > > > api), care to review this?
> > > > 
> > > > A cursory glance tells me that summary_show in 
> > > > drivers/infiniband/hw/usnic/usnic_ib_sysfs.c has a problem, I
> > > > think the last = should be +=
> > 
> > No, it's correct and overwriting what would otherwise be a trailing
> > space.
> 
> The last two lines of summary_show() are
> 
>    len = sysfs_emit_at(buf, len, "\n");
> 
>    return len;
> 
> So that always returns 2, the length of "\n",

1 rather than 2, but you are otherwise correct.

> rather than the length of
> everything you just put into buf, which is what sysfs attributes are
> supposed to return.

Ah, right.  My braino mistake.
This should not use the sysfs_emit_at return value at all.

Patch upcoming...