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

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

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 <redacted>
Tested-by: Thiago Jung Bauermann <redacted>

---

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

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

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.

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

thanks,

greg k-h

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:

quoted

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

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

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:

quoted

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!

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 <redacted>
Tested-by: Thiago Jung Bauermann <redacted>

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

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:

quoted

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 <redacted>
Tested-by: Thiago Jung Bauermann <redacted>

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 :)

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:

quoted

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.

quoted

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

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:

quoted

On Mon, Sep 07, 2020 at 07:38:24AM +0200, Greg KH wrote:

quoted

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

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:

quoted

On Sun, Sep 06, 2020 at 01:32:45PM -0700, James Bottomley wrote:

quoted

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 <redacted>
Tested-by: Thiago Jung Bauermann <redacted>

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

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:

quoted

On Mon, 2020-09-07 at 16:23 +0300, Jarkko Sakkinen wrote:

quoted

On Mon, Sep 07, 2020 at 07:38:24AM +0200, Greg KH wrote:

quoted

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.

quoted

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

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:

quoted

On Mon, Sep 07, 2020 at 02:52:08PM -0700, James Bottomley wrote:

[...]

quoted

quoted

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

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:

quoted

On Tue, Sep 08, 2020 at 07:45:52AM +0200, Greg KH wrote:

quoted

On Mon, Sep 07, 2020 at 02:52:08PM -0700, James Bottomley wrote:

[...]

quoted

quoted

quoted

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

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:

quoted

On Tue, Sep 08, 2020 at 07:45:52AM +0200, Greg KH wrote:

quoted

On Mon, Sep 07, 2020 at 02:52:08PM -0700, James Bottomley wrote:

[...]

quoted

quoted

quoted

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

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:

quoted

On Tue, 2020-09-08 at 21:05 +0300, Jarkko Sakkinen wrote:

quoted

On Tue, Sep 08, 2020 at 07:45:52AM +0200, Greg KH wrote:

quoted

On Mon, Sep 07, 2020 at 02:52:08PM -0700, James Bottomley wrote:

[...]

quoted

quoted

quoted

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