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

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

[James Bottomley]

export TPM 2 PCRs via sysfs.  We also agreed we should conform to
sysfs rules of one value per file, which rules out the "pcrs" file
format of TPM 1.2 which has every PCR value in the same file.

I added these files using device groups, so one group per bank hash of
the TPM.  Using an emulator which supports a variety of hashes, you
can see the structure of the group files:

root@testdeb:~# ls -F /sys/class/tpm/tpm0/
dev      pcr-sha1/    pcr-sha384/  power/      tpm_version_major
device@  pcr-sha256/  pcr-sha512/  subsystem@  uevent

As a future enhancement, we could use the group is_visible function to
remove files corresponding to PCRs which don't exist.  The reason this
isn't present is that so far I've never seen a TPM with a missing PCR.

James

---

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

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

-- 
2.26.2

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

[James Bottomley]

Create sysfs per hash groups with 24 PCR files in them one group,
named pcr-<hash>, for each agile hash of the TPM.  The files are
plugged in to a PCR read function which is TPM version agnostic, so
this works also for TPM 1.2 but the hash is only sha1 in that case.

Note: the macros used to create the hashes emit spurious checkpatch
warnings.  Do not try to "fix" them as checkpatch recommends, otherwise
they'll break.

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

---

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

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

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

[Jarkko Sakkinen]

On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote:
> Create sysfs per hash groups with 24 PCR files in them one group,
> named pcr-<hash>, for each agile hash of the TPM.  The files are
> plugged in to a PCR read function which is TPM version agnostic, so
> this works also for TPM 1.2 but the hash is only sha1 in that case.
> 
> Note: the macros used to create the hashes emit spurious checkpatch
> warnings.  Do not try to "fix" them as checkpatch recommends, otherwise
> they'll break.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

I have hard time understanding why this is required.

You can grab the information through /dev/tpm0 just fine.

/Jarkko

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

[Jarkko Sakkinen]

On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote:
> On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote:
> > Create sysfs per hash groups with 24 PCR files in them one group,
> > named pcr-<hash>, for each agile hash of the TPM.  The files are
> > plugged in to a PCR read function which is TPM version agnostic, so
> > this works also for TPM 1.2 but the hash is only sha1 in that case.
> > 
> > Note: the macros used to create the hashes emit spurious checkpatch
> > warnings.  Do not try to "fix" them as checkpatch recommends, otherwise
> > they'll break.
> > 
> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> 
> I have hard time understanding why this is required.
> 
> You can grab the information through /dev/tpm0 just fine.

I just think it is principally wrong to add sysfs files if they don't
have any measurable value other than perhaps some convenience.

It is trival to write only a libc dependent program that outputs PCRs.

I think this is essentially an user space problem that is getting sorted
out with kernel code.

/Jarkko

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

[Jarkko Sakkinen]

On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote:
> > > Create sysfs per hash groups with 24 PCR files in them one group,
> > > named pcr-<hash>, for each agile hash of the TPM.  The files are
> > > plugged in to a PCR read function which is TPM version agnostic, so
> > > this works also for TPM 1.2 but the hash is only sha1 in that case.
> > > 
> > > Note: the macros used to create the hashes emit spurious checkpatch
> > > warnings.  Do not try to "fix" them as checkpatch recommends, otherwise
> > > they'll break.
> > > 
> > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > 
> > I have hard time understanding why this is required.
> > 
> > You can grab the information through /dev/tpm0 just fine.
> 
> I just think it is principally wrong to add sysfs files if they don't
> have any measurable value other than perhaps some convenience.
> 
> It is trival to write only a libc dependent program that outputs PCRs.
> 
> I think this is essentially an user space problem that is getting sorted
> out with kernel code.

Jason, what do you make of this? I recall that it was you who I
discussed with about this topic when TPM 2.0 support was first
upstreamed.

/Jarkko

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

[Jason Gunthorpe]

On Tue, Aug 18, 2020 at 07:26:30PM +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote:
> > > > Create sysfs per hash groups with 24 PCR files in them one group,
> > > > named pcr-<hash>, for each agile hash of the TPM.  The files are
> > > > plugged in to a PCR read function which is TPM version agnostic, so
> > > > this works also for TPM 1.2 but the hash is only sha1 in that case.
> > > > 
> > > > Note: the macros used to create the hashes emit spurious checkpatch
> > > > warnings.  Do not try to "fix" them as checkpatch recommends, otherwise
> > > > they'll break.
> > > > 
> > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > > 
> > > I have hard time understanding why this is required.
> > > 
> > > You can grab the information through /dev/tpm0 just fine.
> > 
> > I just think it is principally wrong to add sysfs files if they don't
> > have any measurable value other than perhaps some convenience.
> > 
> > It is trival to write only a libc dependent program that outputs PCRs.
> > 
> > I think this is essentially an user space problem that is getting sorted
> > out with kernel code.
> 
> Jason, what do you make of this? I recall that it was you who I
> discussed with about this topic when TPM 2.0 support was first
> upstreamed.

TPM 2.0 broke all the userspace so it made sense to get rid of the
non-conforming sysfs files from TPM v1.x time as part of the userspace
API. That was the main reason to not continue forward with PCR in
userspace.

As far as doing it properly as this patch does.. I agree with you that
sysfs files should have some reason to be added, espcially if it
causes quite big code cost as this does. eg to drive a udev rule
decision.

Why is PCRs so special it needs to be in sysfs? What is userspace
going to do with this information?

Jason

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

[Mimi Zohar]

On Tue, 2020-08-18 at 13:46 -0300, Jason Gunthorpe wrote:
> On Tue, Aug 18, 2020 at 07:26:30PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen wrote:
> > > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote:
> > > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote:
> > > > > Create sysfs per hash groups with 24 PCR files in them one group,
> > > > > named pcr-<hash>, for each agile hash of the TPM.  The files are
> > > > > plugged in to a PCR read function which is TPM version agnostic, so
> > > > > this works also for TPM 1.2 but the hash is only sha1 in that case.
> > > > > 
> > > > > Note: the macros used to create the hashes emit spurious checkpatch
> > > > > warnings.  Do not try to "fix" them as checkpatch recommends, otherwise
> > > > > they'll break.
> > > > > 
> > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > > > 
> > > > I have hard time understanding why this is required.
> > > > 
> > > > You can grab the information through /dev/tpm0 just fine.
> > > 
> > > I just think it is principally wrong to add sysfs files if they don't
> > > have any measurable value other than perhaps some convenience.
> > > 
> > > It is trival to write only a libc dependent program that outputs PCRs.
> > > 
> > > I think this is essentially an user space problem that is getting sorted
> > > out with kernel code.
> > 
> > Jason, what do you make of this? I recall that it was you who I
> > discussed with about this topic when TPM 2.0 support was first
> > upstreamed.
> 
> TPM 2.0 broke all the userspace so it made sense to get rid of the
> non-conforming sysfs files from TPM v1.x time as part of the userspace
> API. That was the main reason to not continue forward with PCR in
> userspace.
> 
> As far as doing it properly as this patch does.. I agree with you that
> sysfs files should have some reason to be added, espcially if it
> causes quite big code cost as this does. eg to drive a udev rule
> decision.
> 
> Why is PCRs so special it needs to be in sysfs? What is userspace
> going to do with this information?

The original IMA LTP "boot_aggregate" regression test is dependent on
the exported TPM event log and PCRs.  Similar support is needed for TPM
2.0.  There isn't just a single userspace application for reading
PCRs.  As soon as we add support for one userspace
application,  support for the other applications is requested.  So
instead of a having a simple regression test with a single method of
reading PCRs, we're now required to support multiple userspace
applications.

I'm definitely in favor of exporting the PCRs, just as the TPM 2.0
event log and version are now exported.

Mimi

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

[Jason Gunthorpe]

On Tue, Aug 18, 2020 at 02:26:04PM -0400, Mimi Zohar wrote:
> On Tue, 2020-08-18 at 13:46 -0300, Jason Gunthorpe wrote:
> > On Tue, Aug 18, 2020 at 07:26:30PM +0300, Jarkko Sakkinen wrote:
> > > On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen wrote:
> > > > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote:
> > > > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote:
> > > > > > Create sysfs per hash groups with 24 PCR files in them one group,
> > > > > > named pcr-<hash>, for each agile hash of the TPM.  The files are
> > > > > > plugged in to a PCR read function which is TPM version agnostic, so
> > > > > > this works also for TPM 1.2 but the hash is only sha1 in that case.
> > > > > > 
> > > > > > Note: the macros used to create the hashes emit spurious checkpatch
> > > > > > warnings.  Do not try to "fix" them as checkpatch recommends, otherwise
> > > > > > they'll break.
> > > > > > 
> > > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > > > > 
> > > > > I have hard time understanding why this is required.
> > > > > 
> > > > > You can grab the information through /dev/tpm0 just fine.
> > > > 
> > > > I just think it is principally wrong to add sysfs files if they don't
> > > > have any measurable value other than perhaps some convenience.
> > > > 
> > > > It is trival to write only a libc dependent program that outputs PCRs.
> > > > 
> > > > I think this is essentially an user space problem that is getting sorted
> > > > out with kernel code.
> > > 
> > > Jason, what do you make of this? I recall that it was you who I
> > > discussed with about this topic when TPM 2.0 support was first
> > > upstreamed.
> > 
> > TPM 2.0 broke all the userspace so it made sense to get rid of the
> > non-conforming sysfs files from TPM v1.x time as part of the userspace
> > API. That was the main reason to not continue forward with PCR in
> > userspace.
> > 
> > As far as doing it properly as this patch does.. I agree with you that
> > sysfs files should have some reason to be added, espcially if it
> > causes quite big code cost as this does. eg to drive a udev rule
> > decision.
> > 
> > Why is PCRs so special it needs to be in sysfs? What is userspace
> > going to do with this information?
> 
> The original IMA LTP "boot_aggregate" regression test is dependent on
> the exported TPM event log and PCRs.  Similar support is needed for TPM
> 2.0.  There isn't just a single userspace application for reading
> PCRs.  As soon as we add support for one userspace
> application,  support for the other applications is requested.  So
> instead of a having a simple regression test with a single method of
> reading PCRs, we're now required to support multiple userspace
> applications.

But this test already has a C program as part of the boot aggregate
test, why is it such a problem to use a C program to also read the
PCRs?

As Jarkko says it is not so hard

Jason

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

[Mimi Zohar]

On Tue, 2020-08-18 at 15:36 -0300, Jason Gunthorpe wrote:
> On Tue, Aug 18, 2020 at 02:26:04PM -0400, Mimi Zohar wrote:
> > On Tue, 2020-08-18 at 13:46 -0300, Jason Gunthorpe wrote:
> > > On Tue, Aug 18, 2020 at 07:26:30PM +0300, Jarkko Sakkinen wrote:
> > > > On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen wrote:
> > > > > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote:
> > > > > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote:
> > > > > > > Create sysfs per hash groups with 24 PCR files in them one group,
> > > > > > > named pcr-<hash>, for each agile hash of the TPM.  The files are
> > > > > > > plugged in to a PCR read function which is TPM version agnostic, so
> > > > > > > this works also for TPM 1.2 but the hash is only sha1 in that case.
> > > > > > > 
> > > > > > > Note: the macros used to create the hashes emit spurious checkpatch
> > > > > > > warnings.  Do not try to "fix" them as checkpatch recommends, otherwise
> > > > > > > they'll break.
> > > > > > > 
> > > > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > > > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > > > > > 
> > > > > > I have hard time understanding why this is required.
> > > > > > 
> > > > > > You can grab the information through /dev/tpm0 just fine.
> > > > > 
> > > > > I just think it is principally wrong to add sysfs files if they don't
> > > > > have any measurable value other than perhaps some convenience.
> > > > > 
> > > > > It is trival to write only a libc dependent program that outputs PCRs.
> > > > > 
> > > > > I think this is essentially an user space problem that is getting sorted
> > > > > out with kernel code.
> > > > 
> > > > Jason, what do you make of this? I recall that it was you who I
> > > > discussed with about this topic when TPM 2.0 support was first
> > > > upstreamed.
> > > 
> > > TPM 2.0 broke all the userspace so it made sense to get rid of the
> > > non-conforming sysfs files from TPM v1.x time as part of the userspace
> > > API. That was the main reason to not continue forward with PCR in
> > > userspace.
> > > 
> > > As far as doing it properly as this patch does.. I agree with you that
> > > sysfs files should have some reason to be added, espcially if it
> > > causes quite big code cost as this does. eg to drive a udev rule
> > > decision.
> > > 
> > > Why is PCRs so special it needs to be in sysfs? What is userspace
> > > going to do with this information?
> > 
> > The original IMA LTP "boot_aggregate" regression test is dependent on
> > the exported TPM event log and PCRs.  Similar support is needed for TPM
> > 2.0.  There isn't just a single userspace application for reading
> > PCRs.  As soon as we add support for one userspace
> > application,  support for the other applications is requested.  So
> > instead of a having a simple regression test with a single method of
> > reading PCRs, we're now required to support multiple userspace
> > applications.
> 
> But this test already has a C program as part of the boot aggregate
> test, why is it such a problem to use a C program to also read the
> PCRs?
> 
> As Jarkko says it is not so hard

The problem is that there isn't just one single userspace library or
application for reading PCRs.  So now not only is there the kernel
"boot_aggregate" regression testing, but regression testing of the tool
itself to support multiple methods of reading the PCRs.

Mimi

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

[Jason Gunthorpe]

On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote:

> The problem is that there isn't just one single userspace library or
> application for reading PCRs.  So now not only is there the kernel
> "boot_aggregate" regression testing, but regression testing of the tool
> itself to support multiple methods of reading the PCRs.

I was thinking just open code 
  open("/dev/tpm")
  write(read_pcrs_cmd)
  read(read_pcrs_cmd)
 
It isn't particularly hard to retrive the PCRs, don't really need to
depend on a library.

Jason

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

[Mimi Zohar]

On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote:
> On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote:
> 
> > The problem is that there isn't just one single userspace library or
> > application for reading PCRs.  So now not only is there the kernel
> > "boot_aggregate" regression testing, but regression testing of the tool
> > itself to support multiple methods of reading the PCRs.
> 
> I was thinking just open code 
>   open("/dev/tpm")
>   write(read_pcrs_cmd)
>   read(read_pcrs_cmd)
>  
> It isn't particularly hard to retrive the PCRs, don't really need to
> depend on a library.

Ok, do you want to contribute it to ima-evm-utils?  While you're at it,
do you also have code to parse the TPM 2.0 event log that you could
contribute?

Seriously, we shouldn't be (re-)writing code to do this.

Mimi

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

[Jason Gunthorpe]

On Wed, Aug 19, 2020 at 09:27:33AM -0400, Mimi Zohar wrote:
> On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote:
> > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote:
> > 
> > > The problem is that there isn't just one single userspace library or
> > > application for reading PCRs.  So now not only is there the kernel
> > > "boot_aggregate" regression testing, but regression testing of the tool
> > > itself to support multiple methods of reading the PCRs.
> > 
> > I was thinking just open code 
> >   open("/dev/tpm")
> >   write(read_pcrs_cmd)
> >   read(read_pcrs_cmd)
> >  
> > It isn't particularly hard to retrive the PCRs, don't really need to
> > depend on a library.
> 
> Ok, do you want to contribute it to ima-evm-utils?  While you're at it,
> do you also have code to parse the TPM 2.0 event log that you could
> contribute?
> 
> Seriously, we shouldn't be (re-)writing code to do this.

The kernel should not be used a dumping ground to work around a
dysfunctional userspace either. :(

You've basicaly said you can't rely on a sane userspace library
because *reasons* so we need to dump stuff in the kernel instead.

It is not a good justification to add new uAPI.

James seems to have the same basic conclusion too, unfortunately.

Jason

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

[Mimi Zohar]

On Wed, 2020-08-19 at 11:09 -0300, Jason Gunthorpe wrote:
> On Wed, Aug 19, 2020 at 09:27:33AM -0400, Mimi Zohar wrote:
> > On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote:
> > > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote:
> > > 
> > > > The problem is that there isn't just one single userspace library or
> > > > application for reading PCRs.  So now not only is there the kernel
> > > > "boot_aggregate" regression testing, but regression testing of the tool
> > > > itself to support multiple methods of reading the PCRs.
> > > 
> > > I was thinking just open code 
> > >   open("/dev/tpm")
> > >   write(read_pcrs_cmd)
> > >   read(read_pcrs_cmd)
> > >  
> > > It isn't particularly hard to retrive the PCRs, don't really need to
> > > depend on a library.
> > 
> > Ok, do you want to contribute it to ima-evm-utils?  While you're at it,
> > do you also have code to parse the TPM 2.0 event log that you could
> > contribute?
> > 
> > Seriously, we shouldn't be (re-)writing code to do this.
> 
> The kernel should not be used a dumping ground to work around a
> dysfunctional userspace either. :(
> 
> You've basicaly said you can't rely on a sane userspace library
> because *reasons* so we need to dump stuff in the kernel instead.
> 
> It is not a good justification to add new uAPI.
> 
> James seems to have the same basic conclusion too, unfortunately.

"dysfunctional" is dropping existing TPM 1.2 sysfs support, which was
done without consideration about existing applications/tools (e.g. ima-
evm-utils, ltp) and without community input.  It's not only James that
is advocating for exporting the TPM PCRs, but Jerry Snitselaar, who
reviewed this patch and exported the TPM version, and Nayna Jain, who
exported the TPM 2.0 event log.  I'm pretty sure there are a number of
other people who would agree.

Mimi

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

[Mimi Zohar]

On Wed, 2020-08-19 at 10:53 -0400, Mimi Zohar wrote:
> On Wed, 2020-08-19 at 11:09 -0300, Jason Gunthorpe wrote:
> > On Wed, Aug 19, 2020 at 09:27:33AM -0400, Mimi Zohar wrote:
> > > On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote:
> > > > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote:
> > > > 
> > > > > The problem is that there isn't just one single userspace library or
> > > > > application for reading PCRs.  So now not only is there the kernel
> > > > > "boot_aggregate" regression testing, but regression testing of the tool
> > > > > itself to support multiple methods of reading the PCRs.
> > > > 
> > > > I was thinking just open code 
> > > >   open("/dev/tpm")
> > > >   write(read_pcrs_cmd)
> > > >   read(read_pcrs_cmd)
> > > >  
> > > > It isn't particularly hard to retrive the PCRs, don't really need to
> > > > depend on a library.
> > > 
> > > Ok, do you want to contribute it to ima-evm-utils?  While you're at it,
> > > do you also have code to parse the TPM 2.0 event log that you could
> > > contribute?
> > > 
> > > Seriously, we shouldn't be (re-)writing code to do this.
> > 
> > The kernel should not be used a dumping ground to work around a
> > dysfunctional userspace either. :(
> > 
> > You've basicaly said you can't rely on a sane userspace library
> > because *reasons* so we need to dump stuff in the kernel instead.
> > 
> > It is not a good justification to add new uAPI.
> > 
> > James seems to have the same basic conclusion too, unfortunately.
> 
> "dysfunctional" is dropping existing TPM 1.2 sysfs support, which was

^not supporting existing TPM 1.2 sysfs for TPM 2.0

> done without consideration about existing applications/tools (e.g. ima-
> evm-utils, ltp) and without community input.  It's not only James that
> is advocating for exporting the TPM PCRs, but Jerry Snitselaar, who
> reviewed this patch and exported the TPM version, and Nayna Jain, who
> exported the TPM 2.0 event log.  I'm pretty sure there are a number of
> other people who would agree.
> 
> Mimi
> 

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

[Jarkko Sakkinen]

On Wed, Aug 19, 2020 at 10:53:38AM -0400, Mimi Zohar wrote:
> On Wed, 2020-08-19 at 11:09 -0300, Jason Gunthorpe wrote:
> > On Wed, Aug 19, 2020 at 09:27:33AM -0400, Mimi Zohar wrote:
> > > On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote:
> > > > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote:
> > > > 
> > > > > The problem is that there isn't just one single userspace library or
> > > > > application for reading PCRs.  So now not only is there the kernel
> > > > > "boot_aggregate" regression testing, but regression testing of the tool
> > > > > itself to support multiple methods of reading the PCRs.
> > > > 
> > > > I was thinking just open code 
> > > >   open("/dev/tpm")
> > > >   write(read_pcrs_cmd)
> > > >   read(read_pcrs_cmd)
> > > >  
> > > > It isn't particularly hard to retrive the PCRs, don't really need to
> > > > depend on a library.
> > > 
> > > Ok, do you want to contribute it to ima-evm-utils?  While you're at it,
> > > do you also have code to parse the TPM 2.0 event log that you could
> > > contribute?
> > > 
> > > Seriously, we shouldn't be (re-)writing code to do this.
> > 
> > The kernel should not be used a dumping ground to work around a
> > dysfunctional userspace either. :(
> > 
> > You've basicaly said you can't rely on a sane userspace library
> > because *reasons* so we need to dump stuff in the kernel instead.
> > 
> > It is not a good justification to add new uAPI.
> > 
> > James seems to have the same basic conclusion too, unfortunately.
> 
> "dysfunctional" is dropping existing TPM 1.2 sysfs support, which was
> done without consideration about existing applications/tools (e.g. ima-
> evm-utils, ltp) and without community input.  It's not only James that
> is advocating for exporting the TPM PCRs, but Jerry Snitselaar, who
> reviewed this patch and exported the TPM version, and Nayna Jain, who
> exported the TPM 2.0 event log.  I'm pretty sure there are a number of
> other people who would agree.
> 
> Mimi

This is not true. TPM 1.2 sysfs was not dropped.

Not adding something does not mean technically dropping something.

/Jarkko

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

[Jerry Snitselaar]

Jarkko Sakkinen @ 2020-08-19 15:16 MST:

> On Wed, Aug 19, 2020 at 10:53:38AM -0400, Mimi Zohar wrote:
>> On Wed, 2020-08-19 at 11:09 -0300, Jason Gunthorpe wrote:
>> > On Wed, Aug 19, 2020 at 09:27:33AM -0400, Mimi Zohar wrote:
>> > > On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote:
>> > > > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote:
>> > > > 
>> > > > > The problem is that there isn't just one single userspace library or
>> > > > > application for reading PCRs.  So now not only is there the kernel
>> > > > > "boot_aggregate" regression testing, but regression testing of the tool
>> > > > > itself to support multiple methods of reading the PCRs.
>> > > > 
>> > > > I was thinking just open code 
>> > > >   open("/dev/tpm")
>> > > >   write(read_pcrs_cmd)
>> > > >   read(read_pcrs_cmd)
>> > > >  
>> > > > It isn't particularly hard to retrive the PCRs, don't really need to
>> > > > depend on a library.
>> > > 
>> > > Ok, do you want to contribute it to ima-evm-utils?  While you're at it,
>> > > do you also have code to parse the TPM 2.0 event log that you could
>> > > contribute?
>> > > 
>> > > Seriously, we shouldn't be (re-)writing code to do this.
>> > 
>> > The kernel should not be used a dumping ground to work around a
>> > dysfunctional userspace either. :(
>> > 
>> > You've basicaly said you can't rely on a sane userspace library
>> > because *reasons* so we need to dump stuff in the kernel instead.
>> > 
>> > It is not a good justification to add new uAPI.
>> > 
>> > James seems to have the same basic conclusion too, unfortunately.
>> 
>> "dysfunctional" is dropping existing TPM 1.2 sysfs support, which was
>> done without consideration about existing applications/tools (e.g. ima-
>> evm-utils, ltp) and without community input.  It's not only James that
>> is advocating for exporting the TPM PCRs, but Jerry Snitselaar, who
>> reviewed this patch and exported the TPM version, and Nayna Jain, who
>> exported the TPM 2.0 event log.  I'm pretty sure there are a number of
>> other people who would agree.
>> 
>> Mimi
>
> This is not true. TPM 1.2 sysfs was not dropped.
>
> Not adding something does not mean technically dropping something.
>
> /Jarkko

When reviewing it I honestly didn't give much(any?) thought to whether
it should be there. My thought was it adhered to the 1 value per file
rule unlike the 1.2 pcrs file and that was about it.

IIRC when 2.0 was added there was the issue of things like the 1.2 pcrs
not conforming to standards, possible issues of races, and a question of
what exactly should be exported.  1.2 has a number of files for doing
things that I think were also handled by ppi and userspace.

I guess the question is where does the line get drawn. My exporting the
major version of the tpm probably could've been handled instead with a
pr_info spitting it out for people to grab out of dmesg.


Jerry

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

[Jason Gunthorpe]

On Wed, Aug 19, 2020 at 03:48:11PM -0700, Jerry Snitselaar wrote:

> I guess the question is where does the line get drawn. My exporting the
> major version of the tpm probably could've been handled instead with a
> pr_info spitting it out for people to grab out of dmesg.

FWIW, I did feel the tpm_version could be detected from the /dev/tpm
as well. However the small size of kernel code, the general early boot
argument, udev (eg only start the TPM2 daemons on a TPM2 system), and
something like how INQUIRY is used by reporting tools, made it seem
OK.

Jason

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

[Jarkko Sakkinen]

On Wed, Aug 19, 2020 at 03:48:11PM -0700, Jerry Snitselaar wrote:
> 
> Jarkko Sakkinen @ 2020-08-19 15:16 MST:
> 
> > On Wed, Aug 19, 2020 at 10:53:38AM -0400, Mimi Zohar wrote:
> >> On Wed, 2020-08-19 at 11:09 -0300, Jason Gunthorpe wrote:
> >> > On Wed, Aug 19, 2020 at 09:27:33AM -0400, Mimi Zohar wrote:
> >> > > On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote:
> >> > > > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote:
> >> > > > 
> >> > > > > The problem is that there isn't just one single userspace library or
> >> > > > > application for reading PCRs.  So now not only is there the kernel
> >> > > > > "boot_aggregate" regression testing, but regression testing of the tool
> >> > > > > itself to support multiple methods of reading the PCRs.
> >> > > > 
> >> > > > I was thinking just open code 
> >> > > >   open("/dev/tpm")
> >> > > >   write(read_pcrs_cmd)
> >> > > >   read(read_pcrs_cmd)
> >> > > >  
> >> > > > It isn't particularly hard to retrive the PCRs, don't really need to
> >> > > > depend on a library.
> >> > > 
> >> > > Ok, do you want to contribute it to ima-evm-utils?  While you're at it,
> >> > > do you also have code to parse the TPM 2.0 event log that you could
> >> > > contribute?
> >> > > 
> >> > > Seriously, we shouldn't be (re-)writing code to do this.
> >> > 
> >> > The kernel should not be used a dumping ground to work around a
> >> > dysfunctional userspace either. :(
> >> > 
> >> > You've basicaly said you can't rely on a sane userspace library
> >> > because *reasons* so we need to dump stuff in the kernel instead.
> >> > 
> >> > It is not a good justification to add new uAPI.
> >> > 
> >> > James seems to have the same basic conclusion too, unfortunately.
> >> 
> >> "dysfunctional" is dropping existing TPM 1.2 sysfs support, which was
> >> done without consideration about existing applications/tools (e.g. ima-
> >> evm-utils, ltp) and without community input.  It's not only James that
> >> is advocating for exporting the TPM PCRs, but Jerry Snitselaar, who
> >> reviewed this patch and exported the TPM version, and Nayna Jain, who
> >> exported the TPM 2.0 event log.  I'm pretty sure there are a number of
> >> other people who would agree.
> >> 
> >> Mimi
> >
> > This is not true. TPM 1.2 sysfs was not dropped.
> >
> > Not adding something does not mean technically dropping something.
> >
> > /Jarkko
> 
> When reviewing it I honestly didn't give much(any?) thought to whether
> it should be there. My thought was it adhered to the 1 value per file
> rule unlike the 1.2 pcrs file and that was about it.
> 
> IIRC when 2.0 was added there was the issue of things like the 1.2 pcrs
> not conforming to standards, possible issues of races, and a question of
> what exactly should be exported.  1.2 has a number of files for doing
> things that I think were also handled by ppi and userspace.
> 
> I guess the question is where does the line get drawn. My exporting the
> major version of the tpm probably could've been handled instead with a
> pr_info spitting it out for people to grab out of dmesg.
> 
> 
> Jerry

TPM protocol version is a different case tha dumping all the PCRs as
ASCII. It fairly unintrusive feature for the kernel, kernel has this
knowledge stored already and it is constant for a boot cycle.

/Jarkko

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

[Serge E. Hallyn]

On Wed, Aug 19, 2020 at 11:09:43AM -0300, Jason Gunthorpe wrote:
> On Wed, Aug 19, 2020 at 09:27:33AM -0400, Mimi Zohar wrote:
> > On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote:
> > > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote:
> > > 
> > > > The problem is that there isn't just one single userspace library or
> > > > application for reading PCRs.  So now not only is there the kernel
> > > > "boot_aggregate" regression testing, but regression testing of the tool
> > > > itself to support multiple methods of reading the PCRs.
> > > 
> > > I was thinking just open code 
> > >   open("/dev/tpm")
> > >   write(read_pcrs_cmd)
> > >   read(read_pcrs_cmd)
> > >  
> > > It isn't particularly hard to retrive the PCRs, don't really need to
> > > depend on a library.
> > 
> > Ok, do you want to contribute it to ima-evm-utils?  While you're at it,
> > do you also have code to parse the TPM 2.0 event log that you could
> > contribute?
> > 
> > Seriously, we shouldn't be (re-)writing code to do this.
> 
> The kernel should not be used a dumping ground to work around a
> dysfunctional userspace either. :(
> 
> You've basicaly said you can't rely on a sane userspace library
> because *reasons* so we need to dump stuff in the kernel instead.
> 
> It is not a good justification to add new uAPI.
> 
> James seems to have the same basic conclusion too, unfortunately.

It seems to me the point is that we often want to do these things in
very early and/or stripped-down userspaces.

I'm definately also in favor of having sysfs provide these.

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

[Jarkko Sakkinen]

On Wed, Aug 19, 2020 at 09:27:33AM -0400, Mimi Zohar wrote:
> On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote:
> > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote:
> > 
> > > The problem is that there isn't just one single userspace library or
> > > application for reading PCRs.  So now not only is there the kernel
> > > "boot_aggregate" regression testing, but regression testing of the tool
> > > itself to support multiple methods of reading the PCRs.
> > 
> > I was thinking just open code 
> >   open("/dev/tpm")
> >   write(read_pcrs_cmd)
> >   read(read_pcrs_cmd)
> >  
> > It isn't particularly hard to retrive the PCRs, don't really need to
> > depend on a library.
> 
> Ok, do you want to contribute it to ima-evm-utils?  While you're at it,
> do you also have code to parse the TPM 2.0 event log that you could
> contribute?
> 
> Seriously, we shouldn't be (re-)writing code to do this.
> 
> Mimi
> 

I'm puzzled why go the trouble of submitting this to LKML and not use
that time to do the whatever changes ima-evm-utils requires?

/Jarkko

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

[James Bottomley]

On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote:
> On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote:
> 
> > The problem is that there isn't just one single userspace library
> > or application for reading PCRs.  So now not only is there the
> > kernel "boot_aggregate" regression testing, but regression testing
> > of the tool itself to support multiple methods of reading the PCRs.
> 
> I was thinking just open code 
>   open("/dev/tpm")
>   write(read_pcrs_cmd)

That's rather an over simplification.  The command takes a
TPML_PCR_SELECTION structure which is, in turn, a packed array of
TPMS_PCR_SELECTION which is a hash type and big endian packed bitmap.

>   read(read_pcrs_cmd)

And the return is a TPML_PCR_SELECTION, in case the system couldn't
provide something you asked for followed by TPML_DIGEST structure which
is a counted array of TPM2B packed digests.

This is a marshal/unmarshal nightmare for the uninitiated.  It is *not*
simple or even straightforward.

> It isn't particularly hard to retrive the PCRs, don't really need to
> depend on a library.

Well, having a PhD in Theoretical Physics, I find quantum field theory
remarkably easy to understand.  My friends tell me this is just me and
they'd rather I not talk about it at parties ...

James

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

[Jason Gunthorpe]

On Wed, Aug 19, 2020 at 08:17:11AM -0700, James Bottomley wrote:
> On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote:
> > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote:
> > 
> > > The problem is that there isn't just one single userspace library
> > > or application for reading PCRs.  So now not only is there the
> > > kernel "boot_aggregate" regression testing, but regression testing
> > > of the tool itself to support multiple methods of reading the PCRs.
> > 
> > I was thinking just open code 
> >   open("/dev/tpm")
> >   write(read_pcrs_cmd)
> 
> That's rather an over simplification.  The command takes a
> TPML_PCR_SELECTION structure which is, in turn, a packed array of
> TPMS_PCR_SELECTION which is a hash type and big endian packed bitmap.

Which is a fixed value, so at least the write is not a complete over
simplification, and this is a LTP where it is reasonable to directly
talk to the kernel.

Remember, the only reason we got here is because apparently there is
no sane single userspace library the LTP could use. Which seems crazy at
this point..

Your other reasons are more compelling, but they also sound like a
tool will be needed to do the other parts - and if we need a tool we
can't the tool read the PCRS progmatically too?

Especially if the tool has to parse log, call hashing functions and
unpack TPM keys already.

And to Mimi's other jab:

> "dysfunctional" is dropping existing TPM 1.2 sysfs support, which was
> done without consideration about existing applications/tools (e.g. ima-
> evm-utils, ltp) and without community input. 

Yes - it was dropped because TPM 2 was a *complete ABI break* for
everything. The kernel was reset to a uABI that matches current
uABI standards starting TPM 2.

The whole userspace needed to be redone anyhow, and certainly nobody
objected at the time.

At least my expecation was that a sensible userspace for TPM (for
administrator user) would be built, like we see in other subsystems eg
'ip' for netdev.

Jason

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

[Mimi Zohar]

On Wed, 2020-08-19 at 13:18 -0300, Jason Gunthorpe wrote:
> Yes - it was dropped because TPM 2 was a *complete ABI break* for
> everything. The kernel was reset to a uABI that matches current
> uABI standards starting TPM 2.
> 
> The whole userspace needed to be redone anyhow, and certainly nobody
> objected at the time.
> 
> At least my expecation was that a sensible userspace for TPM (for
> administrator user) would be built, like we see in other subsystems eg
> 'ip' for netdev.

"Because TPM 2 was a complete ABI break for everything" could be reason
for upstreaming a minimal subset of functionality initially, which
could be expanded over time.  I don't recall a discussion about limting
features in the future.

Mimi

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

[Jason Gunthorpe]

On Wed, Aug 19, 2020 at 12:57:42PM -0400, Mimi Zohar wrote:
> On Wed, 2020-08-19 at 13:18 -0300, Jason Gunthorpe wrote:
> > Yes - it was dropped because TPM 2 was a *complete ABI break* for
> > everything. The kernel was reset to a uABI that matches current
> > uABI standards starting TPM 2.
> > 
> > The whole userspace needed to be redone anyhow, and certainly nobody
> > objected at the time.
> > 
> > At least my expecation was that a sensible userspace for TPM (for
> > administrator user) would be built, like we see in other subsystems eg
> > 'ip' for netdev.
> 
> "Because TPM 2 was a complete ABI break for everything" could be reason
> for upstreaming a minimal subset of functionality initially, which
> could be expanded over time.  I don't recall a discussion about limting
> features in the future.

All new uAPI additions need to pass the usual uAPI hurdles.

As James outlined, justify why the kernel must present a duplicated
uAPI between sysfs and /dev/tpm. 

There have been good reasons in the past, eg SCSI inquiry.

But there are also bad reasons like "our userpsace is dysfunctional
and can't make a library or tool".

Jason

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

[James Bottomley]

On Wed, 2020-08-19 at 14:17 -0300, Jason Gunthorpe wrote:
> On Wed, Aug 19, 2020 at 12:57:42PM -0400, Mimi Zohar wrote:
> > On Wed, 2020-08-19 at 13:18 -0300, Jason Gunthorpe wrote:
> > > Yes - it was dropped because TPM 2 was a *complete ABI break* for
> > > everything. The kernel was reset to a uABI that matches current
> > > uABI standards starting TPM 2.
> > > 
> > > The whole userspace needed to be redone anyhow, and certainly
> > > nobody objected at the time.
> > > 
> > > At least my expecation was that a sensible userspace for TPM (for
> > > administrator user) would be built, like we see in other
> > > subsystems eg 'ip' for netdev.
> > 
> > "Because TPM 2 was a complete ABI break for everything" could be
> > reason for upstreaming a minimal subset of functionality initially,
> > which could be expanded over time.  I don't recall a discussion
> > about limting features in the future.
> 
> All new uAPI additions need to pass the usual uAPI hurdles.
> 
> As James outlined, justify why the kernel must present a duplicated
> uAPI between sysfs and /dev/tpm. 
> 
> There have been good reasons in the past, eg SCSI inquiry.

First, can we please agree /dev/tpm does not substitute as a "duplicate
API".  I can now clarify the objection into "it's a binary marshalled
interface and Linus doesn't think we should force users to use them":

https://lore.kernel.org/linux-api/CAHk-=wh5YifP7hzKSbwJj94+DZ2czjrZsczy6GBimiogZws=rg@mail.gmail.com/

So the question boils down to whether we're providing a simple
interface that allows preboot and other use cases or should we require
the complexity of a TSS installation to get the necessary tools. 
Perhaps we should also simply copy linux-api and accept the judgment of
the experts on whether we should expose PCRs via sysfs.

> But there are also bad reasons like "our userpsace is dysfunctional
> and can't make a library or tool".

The reason we provide a kernel interface instead of a library or tool
is that libraries and tools tend to be domain specific and the
information needs to be provided across domains.  So: both the current
TPM 2.0 TSSs are written in C.  This means they can just about be
plugged into python but not easily into Go because of its abhorrence of
ffis.  Providing the PCRs from sysfs allows Go attestation easy access
that the TSS tools don't because of the language domain problem.

James

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

[Jason Gunthorpe]

On Wed, Aug 19, 2020 at 01:09:16PM -0700, James Bottomley wrote:
> On Wed, 2020-08-19 at 14:17 -0300, Jason Gunthorpe wrote:
> > On Wed, Aug 19, 2020 at 12:57:42PM -0400, Mimi Zohar wrote:
> > > On Wed, 2020-08-19 at 13:18 -0300, Jason Gunthorpe wrote:
> > > > Yes - it was dropped because TPM 2 was a *complete ABI break* for
> > > > everything. The kernel was reset to a uABI that matches current
> > > > uABI standards starting TPM 2.
> > > > 
> > > > The whole userspace needed to be redone anyhow, and certainly
> > > > nobody objected at the time.
> > > > 
> > > > At least my expecation was that a sensible userspace for TPM (for
> > > > administrator user) would be built, like we see in other
> > > > subsystems eg 'ip' for netdev.
> > > 
> > > "Because TPM 2 was a complete ABI break for everything" could be
> > > reason for upstreaming a minimal subset of functionality initially,
> > > which could be expanded over time.  I don't recall a discussion
> > > about limting features in the future.
> > 
> > All new uAPI additions need to pass the usual uAPI hurdles.
> > 
> > As James outlined, justify why the kernel must present a duplicated
> > uAPI between sysfs and /dev/tpm. 
> > 
> > There have been good reasons in the past, eg SCSI inquiry.
> 
> First, can we please agree /dev/tpm does not substitute as a "duplicate
> API". 

Er? Huh? How so?

> I can now clarify the objection into "it's a binary marshalled
> interface and Linus doesn't think we should force users to use them":
> 
> https://lore.kernel.org/linux-api/CAHk-=wh5YifP7hzKSbwJj94+DZ2czjrZsczy6GBimiogZws=rg@mail.gmail.com/

I'm not sure which part of that you want to quote?

"It's great for well-specified wire protocols." which is describing
/dev/tpm - it has a multivendor standards body.

Bit puzzled about the rest of this message? Do you think Linus belives
netlink should have been implemented as ASCII? JSON parser in the
kernel maybe? Confusing.

> Perhaps we should also simply copy linux-api and accept the judgment of
> the experts on whether we should expose PCRs via sysfs.

Well, AFAIK, for a long time now the mantra has been "if it can be
done in userspace then it should not be in the kernel" ..

I would really like to see a better reason for this - one that doesn't
boil down to it being 'too hard' to write a bit of code in userspace.

eg we can't do it because we can't access /dev/tpm for permissions or
something.

> The reason we provide a kernel interface instead of a library or tool
> is that libraries and tools tend to be domain specific and the
> information needs to be provided across domains.  So: both the current
> TPM 2.0 TSSs are written in C.  This means they can just about be
> plugged into python but not easily into Go because of its abhorrence of
> ffis.  Providing the PCRs from sysfs allows Go attestation easy access
> that the TSS tools don't because of the language domain problem.

I went to try to make a python implementation.. After about 10mins I
came up with this approximate thing:

 select = struct.pack(">BBB", 1, 0, 0) # PCR 1
 pcrread_in = struct.pack(">IHB", 1, TPM2_ALG_SHA1, len(select)) + select
 msg = struct.pack(">HII", TPM2_ST_NO_SESSIONS, 10 + len(pcrread_in), TPM2_CC_PCR_READ) + pcrread_in

 with open("/dev/tpm","wb") as tpm:
    tpm.write(msg)
    resp = tpm.read(msg)

 tag, length, return_code = struct.unpack(">HII",res[:10])
 if not return_code:
    raise Error()

 return res[10+20:] # digest

Which is hopefully quite close to being something working - at least
it looks fairly close to what the kernel implementation does.

Fortunately no Phd was required! I think Go would be about similar,
right?

Jason

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

[James Bottomley]

On Wed, 2020-08-19 at 20:21 -0300, Jason Gunthorpe wrote:
> On Wed, Aug 19, 2020 at 01:09:16PM -0700, James Bottomley wrote:
> > On Wed, 2020-08-19 at 14:17 -0300, Jason Gunthorpe wrote:
> > > On Wed, Aug 19, 2020 at 12:57:42PM -0400, Mimi Zohar wrote:
> > > > On Wed, 2020-08-19 at 13:18 -0300, Jason Gunthorpe wrote:
> > > > > Yes - it was dropped because TPM 2 was a *complete ABI break*
> > > > > for everything. The kernel was reset to a uABI that matches
> > > > > current uABI standards starting TPM 2.
> > > > > 
> > > > > The whole userspace needed to be redone anyhow, and certainly
> > > > > nobody objected at the time.
> > > > > 
> > > > > At least my expecation was that a sensible userspace for TPM
> > > > > (for administrator user) would be built, like we see in other
> > > > > subsystems eg 'ip' for netdev.
> > > > 
> > > > "Because TPM 2 was a complete ABI break for everything" could
> > > > be reason for upstreaming a minimal subset of functionality
> > > > initially, which could be expanded over time.  I don't recall a
> > > > discussion about limting features in the future.
> > > 
> > > All new uAPI additions need to pass the usual uAPI hurdles.
> > > 
> > > As James outlined, justify why the kernel must present a
> > > duplicated uAPI between sysfs and /dev/tpm. 
> > > 
> > > There have been good reasons in the past, eg SCSI inquiry.
> > 
> > First, can we please agree /dev/tpm does not substitute as a
> > "duplicate API". 
> 
> Er? Huh? How so?

Because like the SCSI command interface it's a binary marshalled
protocol we want to abstract for users.  We can still argue whether the
kernel or a toolkit should do the abstraction but it's not one we want
to dump on users and say "this is it, what do you mean you don't like
it?"

> > I can now clarify the objection into "it's a binary marshalled
> > interface and Linus doesn't think we should force users to use
> > them":
> > 
> > https://lore.kernel.org/linux-api/CAHk-=wh5YifP7hzKSbwJj94+DZ2czjrZ
> > sczy6GBimiogZws=rg@mail.gmail.com/
> 
> I'm not sure which part of that you want to quote?
> 
> "It's great for well-specified wire protocols." which is describing
> /dev/tpm - it has a multivendor standards body.

Actually this bit "I think marshalling binary data is actively evil and
wrong. It's great for well-specified wire protocols. It's great for
internal communication in user space. It's *NOT* great for a kernel
system call interface."

/dev/tpm is a user to kernel interface.

> Bit puzzled about the rest of this message? Do you think Linus
> belives netlink should have been implemented as ASCII? JSON parser in
> the kernel maybe? Confusing.

Heh, well I remember ASN.1 parser over my dead body and now we have one
...

> > Perhaps we should also simply copy linux-api and accept the
> > judgment of the experts on whether we should expose PCRs via sysfs.
> 
> Well, AFAIK, for a long time now the mantra has been "if it can be
> done in userspace then it should not be in the kernel" ..

Really, no, we've never had that.  A filesystem can be done in
userspace but there's no move to throw them all out of the kernel.  We
bring stuff into the kernel when it's more efficient and useful and
makes the presentation layer easier.  The question isn't could the user
do it at any cost, the question is if we do it can we maintain it
easily and does it make life easier for the user (and would they
actually use the interface).

> I would really like to see a better reason for this - one that
> doesn't boil down to it being 'too hard' to write a bit of code in
> userspace.

I haven't said that.  I've said there are three (and got corrected to
4) reasons to have easy access to PCR hex values:

   1. early boot measurement checks
   2. log verification
   3. key sealing
   4. log stability checking

> eg we can't do it because we can't access /dev/tpm for permissions or
> something.

I already said that: we can't it's root.root 0600 currently.  All the
TSSs seem to change at least /dev/tpmrm to tpm.tpm 0660 but we can't do
that in the kernel because there's no fixed tpm uid/gid.

> > The reason we provide a kernel interface instead of a library or
> > tool is that libraries and tools tend to be domain specific and the
> > information needs to be provided across domains.  So: both the
> > current TPM 2.0 TSSs are written in C.  This means they can just
> > about be plugged into python but not easily into Go because of its
> > abhorrence of ffis.  Providing the PCRs from sysfs allows Go
> > attestation easy access that the TSS tools don't because of the
> > language domain problem.
> 
> I went to try to make a python implementation.. After about 10mins I
> came up with this approximate thing:
> 
>  select = struct.pack(">BBB", 1, 0, 0) # PCR 1
>  pcrread_in = struct.pack(">IHB", 1, TPM2_ALG_SHA1, len(select)) +
> select
>  msg = struct.pack(">HII", TPM2_ST_NO_SESSIONS, 10 + len(pcrread_in),
> TPM2_CC_PCR_READ) + pcrread_in
> 
>  with open("/dev/tpm","wb") as tpm:
>     tpm.write(msg)
>     resp = tpm.read(msg)
> 
>  tag, length, return_code = struct.unpack(">HII",res[:10])
>  if not return_code:
>     raise Error()
> 
>  return res[10+20:] # digest
> 
> Which is hopefully quite close to being something working - at least
> it looks fairly close to what the kernel implementation does.
> 
> Fortunately no Phd was required! I think Go would be about similar,
> right?

I could do the same with perl, but not bash.  In the same way I could
construct an anomalous SO(3) higgs model as a party trick.

the point is that when you ask users would they rather do the above or
cat /sys/class/tpm/tpm0/pcr-sha1/1 they'll universally opt for the
latter because it's way simpler.

Now perhaps if the mechanism that services this in the kernel were
thousands of lines long and unmaintainable you'd think twice, but it's
not, it's under 200 lines.  So the maintainability bar to us providing
this is low and the user convenience quite high ... that's what makes
it look like a good interface.

James

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

[Serge E. Hallyn]

On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley wrote:
> On Wed, 2020-08-19 at 20:21 -0300, Jason Gunthorpe wrote:
> > On Wed, Aug 19, 2020 at 01:09:16PM -0700, James Bottomley wrote:
> > I went to try to make a python implementation.. After about 10mins I
> > came up with this approximate thing:
> > 
> >  select = struct.pack(">BBB", 1, 0, 0) # PCR 1
> >  pcrread_in = struct.pack(">IHB", 1, TPM2_ALG_SHA1, len(select)) +
> > select
> >  msg = struct.pack(">HII", TPM2_ST_NO_SESSIONS, 10 + len(pcrread_in),
> > TPM2_CC_PCR_READ) + pcrread_in
> > 
> >  with open("/dev/tpm","wb") as tpm:
> >     tpm.write(msg)
> >     resp = tpm.read(msg)
> > 
> >  tag, length, return_code = struct.unpack(">HII",res[:10])
> >  if not return_code:
> >     raise Error()
> > 
> >  return res[10+20:] # digest
> > 
> > Which is hopefully quite close to being something working - at least
> > it looks fairly close to what the kernel implementation does.
> > 
> > Fortunately no Phd was required! I think Go would be about similar,
> > right?
> 
> I could do the same with perl, but not bash.  In the same way I could
> construct an anomalous SO(3) higgs model as a party trick.
> 
> the point is that when you ask users would they rather do the above or
> cat /sys/class/tpm/tpm0/pcr-sha1/1 they'll universally opt for the
> latter because it's way simpler.
> 
> Now perhaps if the mechanism that services this in the kernel were
> thousands of lines long and unmaintainable you'd think twice, but it's
> not, it's under 200 lines.  So the maintainability bar to us providing
> this is low and the user convenience quite high ... that's what makes
> it look like a good interface.
> 
> James

I'd also point out that this is the fundamental thing you do with the
pcrs.  There is no other way that some library would want to do it, and
everything builds on it.  We're exporting the core functionality as a
simpler file read/write.  I know that after taking filesystem interfaces
to an extreme, over the past 20 years we've turned back a bit, but in
this case it seems the right way to do it.

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

[Jarkko Sakkinen]

On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley wrote:
> On Wed, 2020-08-19 at 20:21 -0300, Jason Gunthorpe wrote:
> > On Wed, Aug 19, 2020 at 01:09:16PM -0700, James Bottomley wrote:
> > > On Wed, 2020-08-19 at 14:17 -0300, Jason Gunthorpe wrote:
> > > > On Wed, Aug 19, 2020 at 12:57:42PM -0400, Mimi Zohar wrote:
> > > > > On Wed, 2020-08-19 at 13:18 -0300, Jason Gunthorpe wrote:
> > > > > > Yes - it was dropped because TPM 2 was a *complete ABI break*
> > > > > > for everything. The kernel was reset to a uABI that matches
> > > > > > current uABI standards starting TPM 2.
> > > > > > 
> > > > > > The whole userspace needed to be redone anyhow, and certainly
> > > > > > nobody objected at the time.
> > > > > > 
> > > > > > At least my expecation was that a sensible userspace for TPM
> > > > > > (for administrator user) would be built, like we see in other
> > > > > > subsystems eg 'ip' for netdev.
> > > > > 
> > > > > "Because TPM 2 was a complete ABI break for everything" could
> > > > > be reason for upstreaming a minimal subset of functionality
> > > > > initially, which could be expanded over time.  I don't recall a
> > > > > discussion about limting features in the future.
> > > > 
> > > > All new uAPI additions need to pass the usual uAPI hurdles.
> > > > 
> > > > As James outlined, justify why the kernel must present a
> > > > duplicated uAPI between sysfs and /dev/tpm. 
> > > > 
> > > > There have been good reasons in the past, eg SCSI inquiry.
> > > 
> > > First, can we please agree /dev/tpm does not substitute as a
> > > "duplicate API". 
> > 
> > Er? Huh? How so?
> 
> Because like the SCSI command interface it's a binary marshalled
> protocol we want to abstract for users.  We can still argue whether the
> kernel or a toolkit should do the abstraction but it's not one we want
> to dump on users and say "this is it, what do you mean you don't like
> it?"

No we don't. On the contrary, we expose the protocol through /dev/tpm0.

/Jarkko

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

[Jason Gunthorpe]

On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley wrote:

> > eg we can't do it because we can't access /dev/tpm for permissions or
> > something.
> 
> I already said that: we can't it's root.root 0600 currently.  All the
> TSSs seem to change at least /dev/tpmrm to tpm.tpm 0660 but we can't do
> that in the kernel because there's no fixed tpm uid/gid.

Permissions is a pretty good reason to add a sysfs file.

Jason

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

[Jarkko Sakkinen]

On Fri, Aug 21, 2020 at 04:38:47PM -0300, Jason Gunthorpe wrote:
> On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley wrote:
> 
> > > eg we can't do it because we can't access /dev/tpm for permissions or
> > > something.
> > 
> > I already said that: we can't it's root.root 0600 currently.  All the
> > TSSs seem to change at least /dev/tpmrm to tpm.tpm 0660 but we can't do
> > that in the kernel because there's no fixed tpm uid/gid.
> 
> Permissions is a pretty good reason to add a sysfs file.
> 
> Jason

I'm not sure why suid/sgid utility to read pcrs would be worse.

/Jarkko

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

[James Bottomley]

On Mon, 2020-08-24 at 22:44 +0300, Jarkko Sakkinen wrote:
> On Fri, Aug 21, 2020 at 04:38:47PM -0300, Jason Gunthorpe wrote:
> > On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley wrote:
> > 
> > > > eg we can't do it because we can't access /dev/tpm for
> > > > permissions or
> > > > something.
> > > 
> > > I already said that: we can't it's root.root 0600 currently.  All
> > > the TSSs seem to change at least /dev/tpmrm to tpm.tpm 0660 but
> > > we can't do that in the kernel because there's no fixed tpm
> > > uid/gid.
> > 
> > Permissions is a pretty good reason to add a sysfs file.
> > 
> > Jason
> 
> I'm not sure why suid/sgid utility to read pcrs would be worse.

We don't do root running or suid/sgid binaries any more because they're
exceptional security risks.  That's why both TSSs for TPM 2.0 change
the device ownership.  For Trousers and TPM 1.2 we used to run the
daemon as root until we started getting CVEs about it.

James

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

[Jarkko Sakkinen]

On Mon, Aug 24, 2020 at 01:20:46PM -0700, James Bottomley wrote:
> On Mon, 2020-08-24 at 22:44 +0300, Jarkko Sakkinen wrote:
> > On Fri, Aug 21, 2020 at 04:38:47PM -0300, Jason Gunthorpe wrote:
> > > On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley wrote:
> > > 
> > > > > eg we can't do it because we can't access /dev/tpm for
> > > > > permissions or
> > > > > something.
> > > > 
> > > > I already said that: we can't it's root.root 0600 currently.  All
> > > > the TSSs seem to change at least /dev/tpmrm to tpm.tpm 0660 but
> > > > we can't do that in the kernel because there's no fixed tpm
> > > > uid/gid.
> > > 
> > > Permissions is a pretty good reason to add a sysfs file.
> > > 
> > > Jason
> > 
> > I'm not sure why suid/sgid utility to read pcrs would be worse.
> 
> We don't do root running or suid/sgid binaries any more because they're
> exceptional security risks.  That's why both TSSs for TPM 2.0 change
> the device ownership.  For Trousers and TPM 1.2 we used to run the
> daemon as root until we started getting CVEs about it.
> 
> James

OK, then a binary blob for pcrs would be sufficient.

/Jarkko

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

[James Bottomley]

On Tue, 2020-08-25 at 18:27 +0300, Jarkko Sakkinen wrote:
> On Mon, Aug 24, 2020 at 01:20:46PM -0700, James Bottomley wrote:
> > On Mon, 2020-08-24 at 22:44 +0300, Jarkko Sakkinen wrote:
> > > On Fri, Aug 21, 2020 at 04:38:47PM -0300, Jason Gunthorpe wrote:
> > > > On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley
> > > > wrote:
> > > > 
> > > > > > eg we can't do it because we can't access /dev/tpm for
> > > > > > permissions or something.
> > > > > 
> > > > > I already said that: we can't it's root.root 0600
> > > > > currently.  All the TSSs seem to change at least /dev/tpmrm
> > > > > to tpm.tpm 0660 but we can't do that in the kernel because
> > > > > there's no fixed tpm uid/gid.
> > > > 
> > > > Permissions is a pretty good reason to add a sysfs file.
> > > > 
> > > > Jason
> > > 
> > > I'm not sure why suid/sgid utility to read pcrs would be worse.
> > 
> > We don't do root running or suid/sgid binaries any more because
> > they're exceptional security risks.  That's why both TSSs for TPM
> > 2.0 change the device ownership.  For Trousers and TPM 1.2 we used
> > to run the daemon as root until we started getting CVEs about it.
> > 
> > James
> 
> OK, then a binary blob for pcrs would be sufficient.

From a sysfs perspective we only do one value per file and we don't
export binary if a valid and useful ascii representation exists.  On
both of those kernel principles, the current proposal is canonical.

James

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

[Jarkko Sakkinen]

On Tue, Aug 25, 2020 at 08:33:41AM -0700, James Bottomley wrote:
> On Tue, 2020-08-25 at 18:27 +0300, Jarkko Sakkinen wrote:
> > On Mon, Aug 24, 2020 at 01:20:46PM -0700, James Bottomley wrote:
> > > On Mon, 2020-08-24 at 22:44 +0300, Jarkko Sakkinen wrote:
> > > > On Fri, Aug 21, 2020 at 04:38:47PM -0300, Jason Gunthorpe wrote:
> > > > > On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley
> > > > > wrote:
> > > > > 
> > > > > > > eg we can't do it because we can't access /dev/tpm for
> > > > > > > permissions or something.
> > > > > > 
> > > > > > I already said that: we can't it's root.root 0600
> > > > > > currently.  All the TSSs seem to change at least /dev/tpmrm
> > > > > > to tpm.tpm 0660 but we can't do that in the kernel because
> > > > > > there's no fixed tpm uid/gid.
> > > > > 
> > > > > Permissions is a pretty good reason to add a sysfs file.
> > > > > 
> > > > > Jason
> > > > 
> > > > I'm not sure why suid/sgid utility to read pcrs would be worse.
> > > 
> > > We don't do root running or suid/sgid binaries any more because
> > > they're exceptional security risks.  That's why both TSSs for TPM
> > > 2.0 change the device ownership.  For Trousers and TPM 1.2 we used
> > > to run the daemon as root until we started getting CVEs about it.
> > > 
> > > James
> > 
> > OK, then a binary blob for pcrs would be sufficient.
> 
> From a sysfs perspective we only do one value per file and we don't
> export binary if a valid and useful ascii representation exists.  On
> both of those kernel principles, the current proposal is canonical.
> 
> James

The event log is also exported as a binary. This patch set pollutes the
sysfs and adds too much overhead for maintaining. Every single algorithm
will needs its own file and needs to be patched to the kernel.

A single 'pcrs' blob could with contents as <alg id, data> pairs would
remain static.

If you speak about principles, please add a reference and/or CC your
patch set also to sysfs maintainers. All I care if what is pragmatically
the best choice.

/Jarkko

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

[Jarkko Sakkinen]

On Wed, Aug 26, 2020 at 04:15:25PM +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 25, 2020 at 08:33:41AM -0700, James Bottomley wrote:
> > On Tue, 2020-08-25 at 18:27 +0300, Jarkko Sakkinen wrote:
> > > On Mon, Aug 24, 2020 at 01:20:46PM -0700, James Bottomley wrote:
> > > > On Mon, 2020-08-24 at 22:44 +0300, Jarkko Sakkinen wrote:
> > > > > On Fri, Aug 21, 2020 at 04:38:47PM -0300, Jason Gunthorpe wrote:
> > > > > > On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley
> > > > > > wrote:
> > > > > > 
> > > > > > > > eg we can't do it because we can't access /dev/tpm for
> > > > > > > > permissions or something.
> > > > > > > 
> > > > > > > I already said that: we can't it's root.root 0600
> > > > > > > currently.  All the TSSs seem to change at least /dev/tpmrm
> > > > > > > to tpm.tpm 0660 but we can't do that in the kernel because
> > > > > > > there's no fixed tpm uid/gid.
> > > > > > 
> > > > > > Permissions is a pretty good reason to add a sysfs file.
> > > > > > 
> > > > > > Jason
> > > > > 
> > > > > I'm not sure why suid/sgid utility to read pcrs would be worse.
> > > > 
> > > > We don't do root running or suid/sgid binaries any more because
> > > > they're exceptional security risks.  That's why both TSSs for TPM
> > > > 2.0 change the device ownership.  For Trousers and TPM 1.2 we used
> > > > to run the daemon as root until we started getting CVEs about it.
> > > > 
> > > > James
> > > 
> > > OK, then a binary blob for pcrs would be sufficient.
> > 
> > From a sysfs perspective we only do one value per file and we don't
> > export binary if a valid and useful ascii representation exists.  On
> > both of those kernel principles, the current proposal is canonical.
> > 
> > James
> 
> The event log is also exported as a binary. This patch set pollutes the
> sysfs and adds too much overhead for maintaining. Every single algorithm
> will needs its own file and needs to be patched to the kernel.
> 
> A single 'pcrs' blob could with contents as <alg id, data> pairs would
> remain static.
> 
> If you speak about principles, please add a reference and/or CC your
> patch set also to sysfs maintainers. All I care if what is pragmatically
> the best choice.

A correction: event log is exported through securityfs

I think pcrs should be exported also there instead of sysfs. Does not
feel very sound to have these files in different locations.

> /Jarkko

/Jarkko

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

[Jason Gunthorpe]

On Mon, Aug 24, 2020 at 10:44:57PM +0300, Jarkko Sakkinen wrote:
> On Fri, Aug 21, 2020 at 04:38:47PM -0300, Jason Gunthorpe wrote:
> > On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley wrote:
> > 
> > > > eg we can't do it because we can't access /dev/tpm for permissions or
> > > > something.
> > > 
> > > I already said that: we can't it's root.root 0600 currently.  All the
> > > TSSs seem to change at least /dev/tpmrm to tpm.tpm 0660 but we can't do
> > > that in the kernel because there's no fixed tpm uid/gid.
> > 
> > Permissions is a pretty good reason to add a sysfs file.
> > 
> > Jason
> 
> I'm not sure why suid/sgid utility to read pcrs would be worse.

suid would be more complex than this patch - most things ever made
suid seem to have security bugs :\

Jason

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

[Jarkko Sakkinen]

On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote:
> On Tue, 2020-08-18 at 15:36 -0300, Jason Gunthorpe wrote:
> > On Tue, Aug 18, 2020 at 02:26:04PM -0400, Mimi Zohar wrote:
> > > On Tue, 2020-08-18 at 13:46 -0300, Jason Gunthorpe wrote:
> > > > On Tue, Aug 18, 2020 at 07:26:30PM +0300, Jarkko Sakkinen wrote:
> > > > > On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen wrote:
> > > > > > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote:
> > > > > > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote:
> > > > > > > > Create sysfs per hash groups with 24 PCR files in them one group,
> > > > > > > > named pcr-<hash>, for each agile hash of the TPM.  The files are
> > > > > > > > plugged in to a PCR read function which is TPM version agnostic, so
> > > > > > > > this works also for TPM 1.2 but the hash is only sha1 in that case.
> > > > > > > > 
> > > > > > > > Note: the macros used to create the hashes emit spurious checkpatch
> > > > > > > > warnings.  Do not try to "fix" them as checkpatch recommends, otherwise
> > > > > > > > they'll break.
> > > > > > > > 
> > > > > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > > > > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > > > > > > 
> > > > > > > I have hard time understanding why this is required.
> > > > > > > 
> > > > > > > You can grab the information through /dev/tpm0 just fine.
> > > > > > 
> > > > > > I just think it is principally wrong to add sysfs files if they don't
> > > > > > have any measurable value other than perhaps some convenience.
> > > > > > 
> > > > > > It is trival to write only a libc dependent program that outputs PCRs.
> > > > > > 
> > > > > > I think this is essentially an user space problem that is getting sorted
> > > > > > out with kernel code.
> > > > > 
> > > > > Jason, what do you make of this? I recall that it was you who I
> > > > > discussed with about this topic when TPM 2.0 support was first
> > > > > upstreamed.
> > > > 
> > > > TPM 2.0 broke all the userspace so it made sense to get rid of the
> > > > non-conforming sysfs files from TPM v1.x time as part of the userspace
> > > > API. That was the main reason to not continue forward with PCR in
> > > > userspace.
> > > > 
> > > > As far as doing it properly as this patch does.. I agree with you that
> > > > sysfs files should have some reason to be added, espcially if it
> > > > causes quite big code cost as this does. eg to drive a udev rule
> > > > decision.
> > > > 
> > > > Why is PCRs so special it needs to be in sysfs? What is userspace
> > > > going to do with this information?
> > > 
> > > The original IMA LTP "boot_aggregate" regression test is dependent on
> > > the exported TPM event log and PCRs.  Similar support is needed for TPM
> > > 2.0.  There isn't just a single userspace application for reading
> > > PCRs.  As soon as we add support for one userspace
> > > application,  support for the other applications is requested.  So
> > > instead of a having a simple regression test with a single method of
> > > reading PCRs, we're now required to support multiple userspace
> > > applications.
> > 
> > But this test already has a C program as part of the boot aggregate
> > test, why is it such a problem to use a C program to also read the
> > PCRs?
> > 
> > As Jarkko says it is not so hard
> 
> The problem is that there isn't just one single userspace library or
> application for reading PCRs.  So now not only is there the kernel
> "boot_aggregate" regression testing, but regression testing of the tool
> itself to support multiple methods of reading the PCRs.
> 
> Mimi

It's not hard to write a program to read PCRs with just libc. Takes
about one afternoon.

/Jarkko

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

[James Bottomley]

On Tue, 2020-08-18 at 15:36 -0300, Jason Gunthorpe wrote:
> On Tue, Aug 18, 2020 at 02:26:04PM -0400, Mimi Zohar wrote:
> > On Tue, 2020-08-18 at 13:46 -0300, Jason Gunthorpe wrote:
> > > On Tue, Aug 18, 2020 at 07:26:30PM +0300, Jarkko Sakkinen wrote:
> > > > On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen
> > > > wrote:
> > > > > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen
> > > > > wrote:
> > > > > > On Mon, Aug 17, 2020 at 02:35:06PM -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@HansenPar
> > > > > > > tnership.com>
> > > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > > > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > > > > > 
> > > > > > I have hard time understanding why this is required.
> > > > > > 
> > > > > > You can grab the information through /dev/tpm0 just fine.
> > > > > 
> > > > > I just think it is principally wrong to add sysfs files if
> > > > > they don't have any measurable value other than perhaps some
> > > > > convenience.
> > > > > 
> > > > > It is trival to write only a libc dependent program that
> > > > > outputs PCRs.
> > > > > 
> > > > > I think this is essentially an user space problem that is
> > > > > getting sorted out with kernel code.
> > > > 
> > > > Jason, what do you make of this? I recall that it was you who I
> > > > discussed with about this topic when TPM 2.0 support was first
> > > > upstreamed.
> > > 
> > > TPM 2.0 broke all the userspace so it made sense to get rid of
> > > the non-conforming sysfs files from TPM v1.x time as part of the
> > > userspace API. That was the main reason to not continue forward
> > > with PCR in userspace.
> > > 
> > > As far as doing it properly as this patch does.. I agree with you
> > > that sysfs files should have some reason to be added, espcially
> > > if it causes quite big code cost as this does. eg to drive a udev
> > > rule decision.
> > > 
> > > Why is PCRs so special it needs to be in sysfs? What is userspace
> > > going to do with this information?
> > 
> > The original IMA LTP "boot_aggregate" regression test is dependent
> > on the exported TPM event log and PCRs.  Similar support is needed
> > for TPM 2.0.  There isn't just a single userspace application for
> > reading PCRs.  As soon as we add support for one userspace
> > application,  support for the other applications is requested.  So
> > instead of a having a simple regression test with a single method
> > of reading PCRs, we're now required to support multiple userspace
> > applications.
> 
> But this test already has a C program as part of the boot aggregate
> test, why is it such a problem to use a C program to also read the
> PCRs?
> 
> As Jarkko says it is not so hard

It's not so hard for system programmer who owns the root account, like
the people who hang out on this list.  For your average user, a root
only device you're expected to send and receive binary packets of big
endian data over is a textbook definition of an unusable interface.

I can potentially buy arguments that ordinary users can get this
information from TSS based toolkits (although both the Intel and the
IBM TSS seem to go out of their way to make it hard to extract). Or
even that the average user has no use for the information (although
I've given three examples).  However, it's not realistic to expect that
the average user start speaking binary packet protocols from shell
scripts.

James

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

[Jarkko Sakkinen]

On Tue, Aug 18, 2020 at 03:36:03PM -0300, Jason Gunthorpe wrote:
> On Tue, Aug 18, 2020 at 02:26:04PM -0400, Mimi Zohar wrote:
> > On Tue, 2020-08-18 at 13:46 -0300, Jason Gunthorpe wrote:
> > > On Tue, Aug 18, 2020 at 07:26:30PM +0300, Jarkko Sakkinen wrote:
> > > > On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen wrote:
> > > > > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote:
> > > > > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote:
> > > > > > > Create sysfs per hash groups with 24 PCR files in them one group,
> > > > > > > named pcr-<hash>, for each agile hash of the TPM.  The files are
> > > > > > > plugged in to a PCR read function which is TPM version agnostic, so
> > > > > > > this works also for TPM 1.2 but the hash is only sha1 in that case.
> > > > > > > 
> > > > > > > Note: the macros used to create the hashes emit spurious checkpatch
> > > > > > > warnings.  Do not try to "fix" them as checkpatch recommends, otherwise
> > > > > > > they'll break.
> > > > > > > 
> > > > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > > > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > > > > > 
> > > > > > I have hard time understanding why this is required.
> > > > > > 
> > > > > > You can grab the information through /dev/tpm0 just fine.
> > > > > 
> > > > > I just think it is principally wrong to add sysfs files if they don't
> > > > > have any measurable value other than perhaps some convenience.
> > > > > 
> > > > > It is trival to write only a libc dependent program that outputs PCRs.
> > > > > 
> > > > > I think this is essentially an user space problem that is getting sorted
> > > > > out with kernel code.
> > > > 
> > > > Jason, what do you make of this? I recall that it was you who I
> > > > discussed with about this topic when TPM 2.0 support was first
> > > > upstreamed.
> > > 
> > > TPM 2.0 broke all the userspace so it made sense to get rid of the
> > > non-conforming sysfs files from TPM v1.x time as part of the userspace
> > > API. That was the main reason to not continue forward with PCR in
> > > userspace.
> > > 
> > > As far as doing it properly as this patch does.. I agree with you that
> > > sysfs files should have some reason to be added, espcially if it
> > > causes quite big code cost as this does. eg to drive a udev rule
> > > decision.
> > > 
> > > Why is PCRs so special it needs to be in sysfs? What is userspace
> > > going to do with this information?
> > 
> > The original IMA LTP "boot_aggregate" regression test is dependent on
> > the exported TPM event log and PCRs.  Similar support is needed for TPM
> > 2.0.  There isn't just a single userspace application for reading
> > PCRs.  As soon as we add support for one userspace
> > application,  support for the other applications is requested.  So
> > instead of a having a simple regression test with a single method of
> > reading PCRs, we're now required to support multiple userspace
> > applications.
> 
> But this test already has a C program as part of the boot aggregate
> test, why is it such a problem to use a C program to also read the
> PCRs?
> 
> As Jarkko says it is not so hard
> 
> Jason

"A PCR blob" idea would have the (questionable, I'm not sure if it is
useful) benefit of atomic snapshot but it requires a more complex
implementation to be efficient. E.g. you would have to read PCRs once
in a boot and then intervene PCR manipulating operations, in order to
not cause too much run-time stress.

Convenience does not cut the deal here.

/Jarkko

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

[Jarkko Sakkinen]

On Tue, Aug 18, 2020 at 02:26:04PM -0400, Mimi Zohar wrote:
> On Tue, 2020-08-18 at 13:46 -0300, Jason Gunthorpe wrote:
> > On Tue, Aug 18, 2020 at 07:26:30PM +0300, Jarkko Sakkinen wrote:
> > > On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen wrote:
> > > > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote:
> > > > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote:
> > > > > > Create sysfs per hash groups with 24 PCR files in them one group,
> > > > > > named pcr-<hash>, for each agile hash of the TPM.  The files are
> > > > > > plugged in to a PCR read function which is TPM version agnostic, so
> > > > > > this works also for TPM 1.2 but the hash is only sha1 in that case.
> > > > > > 
> > > > > > Note: the macros used to create the hashes emit spurious checkpatch
> > > > > > warnings.  Do not try to "fix" them as checkpatch recommends, otherwise
> > > > > > they'll break.
> > > > > > 
> > > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > > > > 
> > > > > I have hard time understanding why this is required.
> > > > > 
> > > > > You can grab the information through /dev/tpm0 just fine.
> > > > 
> > > > I just think it is principally wrong to add sysfs files if they don't
> > > > have any measurable value other than perhaps some convenience.
> > > > 
> > > > It is trival to write only a libc dependent program that outputs PCRs.
> > > > 
> > > > I think this is essentially an user space problem that is getting sorted
> > > > out with kernel code.
> > > 
> > > Jason, what do you make of this? I recall that it was you who I
> > > discussed with about this topic when TPM 2.0 support was first
> > > upstreamed.
> > 
> > TPM 2.0 broke all the userspace so it made sense to get rid of the
> > non-conforming sysfs files from TPM v1.x time as part of the userspace
> > API. That was the main reason to not continue forward with PCR in
> > userspace.
> > 
> > As far as doing it properly as this patch does.. I agree with you that
> > sysfs files should have some reason to be added, espcially if it
> > causes quite big code cost as this does. eg to drive a udev rule
> > decision.
> > 
> > Why is PCRs so special it needs to be in sysfs? What is userspace
> > going to do with this information?
> 
> The original IMA LTP "boot_aggregate" regression test is dependent on
> the exported TPM event log and PCRs.  Similar support is needed for TPM
> 2.0.  There isn't just a single userspace application for reading
> PCRs.  As soon as we add support for one userspace
> application,  support for the other applications is requested.  So
> instead of a having a simple regression test with a single method of
> reading PCRs, we're now required to support multiple userspace
> applications.
> 
> I'm definitely in favor of exporting the PCRs, just as the TPM 2.0
> event log and version are now exported.
> 
> Mimi

They are very different use cases from this. In both kernel has
informatino that the user space does not possess. This is not the
case with the PCR values.

/Jarkko

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

[James Bottomley]

On Tue, 2020-08-18 at 19:19 +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote:
> > > Create sysfs per hash groups with 24 PCR files in them one group,
> > > named pcr-<hash>, for each agile hash of the TPM.  The files are
> > > plugged in to a PCR read function which is TPM version agnostic,
> > > so this works also for TPM 1.2 but the hash is only sha1 in that
> > > case.
> > > 
> > > Note: the macros used to create the hashes emit spurious
> > > checkpatch warnings.  Do not try to "fix" them as checkpatch
> > > recommends, otherwise they'll break.
> > > 
> > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership
> > > .com>
> > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > 
> > I have hard time understanding why this is required.
> > 
> > You can grab the information through /dev/tpm0 just fine.
> 
> I just think it is principally wrong to add sysfs files if they don't
> have any measurable value other than perhaps some convenience.

That's pretty much the whole point of sysfs (and procfs): to add
convenient extraction of information even if it could potentially be
obtained by other sources.  For instance, the whole reason we add a lot
of the broken out inquiry data in SCSI via sysfs is precisely so users
don't have to go prodding devices with direct SCSI commands, which are
pretty much analagous to TPM device commands.

The question you should be asking isn't whether the information *could*
be obtained by other means, but whether providing it in this form
facilitates current operations and whether the interface would have
users.

> It is trival to write only a libc dependent program that outputs
> PCRs.

Not for someone who understands only scripting.  There are two
problems: firstly the actual construction of TPM commands is somewhat
complex and pretty much impossible even for shells like bash to
construct and secondly without a TSS installed, the tpm and tpmrm
devices are root owned and 0600 permissioned, so a non-root user simply
can't use them.

> I think this is essentially an user space problem that is getting
> sorted out with kernel code.

So you'd argue that a kernel shouldn't provide a filesystem because
it's simply a seekable key/value retrieval system provided over storage
devices (and there are plenty of databases that do it for you on raw
devices from userspace)?

A big point of a Kernel is to provide a load of convenience interfaces
to users even if users could, in theory, do it all themselves.  A
filesystem is actually a classic example because the directory
structure and file API make data organization and retrieval relatively
easy for the average user, whereas just presenting them with a SCSI
command interface and telling them to use it would be an instant
blocker for most of them.

The question, again, is not whether a user *could* do this another way
but whether the interface provided makes a task (or set of tasks)
easier, whether the API provided is easy to use and finally, whether
the interface will actually attract any users.

James

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

[Jason Gunthorpe]

On Tue, Aug 18, 2020 at 09:44:30AM -0700, James Bottomley wrote:

> The question you should be asking isn't whether the information *could*
> be obtained by other means, but whether providing it in this form
> facilitates current operations and whether the interface would have
> users.

Sure. What are the use cases that need PCRs but no other TPM
operations?

The cover letter didn't say. As PCR is really only useful in the
context of the local TPM I'm not thinking of anything..

Jason

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

[James Bottomley]

On Tue, 2020-08-18 at 14:17 -0300, Jason Gunthorpe wrote:
> On Tue, Aug 18, 2020 at 09:44:30AM -0700, James Bottomley wrote:
> 
> > The question you should be asking isn't whether the information
> > *could*
> > be obtained by other means, but whether providing it in this form
> > facilitates current operations and whether the interface would have
> > users.
> 
> Sure. What are the use cases that need PCRs but no other TPM
> operations?
> 
> The cover letter didn't say. As PCR is really only useful in the
> context of the local TPM I'm not thinking of anything..

The three use cases I picked up at the Boot and Security MC were:

   1. local log verification: a script can run through the IMA ascii log
      and construct the PCR 10 hash which can then be verified
   2. Knowing what the PCR values actually are for sealed keys.  With the
      current trusted key infrastructure you have to calculate and supply
      the hash yourself.  With the new proposed infrastructure, the hash
      would be calculated by the seal operation, but you're still going to
      need the actual PCR values to debug unseal failures.
   3. As a stability check for log shipping: you read the PCR take the log
      then read the PCR: if the two reads are the same the PCR backing the
      log is stable for quoting.

James

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

[Jarkko Sakkinen]

> On Tue, 2020-08-18 at 14:17 -0300, Jason Gunthorpe wrote:
> > On Tue, Aug 18, 2020 at 09:44:30AM -0700, James Bottomley wrote:
> > 
> > > The question you should be asking isn't whether the information
> > > *could*
> > > be obtained by other means, but whether providing it in this form
> > > facilitates current operations and whether the interface would have
> > > users.
> > 
> > Sure. What are the use cases that need PCRs but no other TPM
> > operations?
> > 
> > The cover letter didn't say. As PCR is really only useful in the
> > context of the local TPM I'm not thinking of anything..
> 
> The three use cases I picked up at the Boot and Security MC were:
> 
>    1. local log verification: a script can run through the IMA ascii log
>       and construct the PCR 10 hash which can then be verified
>    2. Knowing what the PCR values actually are for sealed keys.  With the
>       current trusted key infrastructure you have to calculate and supply
>       the hash yourself.  With the new proposed infrastructure, the hash
>       would be calculated by the seal operation, but you're still going to
>       need the actual PCR values to debug unseal failures.
>    3. As a stability check for log shipping: you read the PCR take the log
>       then read the PCR: if the two reads are the same the PCR backing the
>       log is stable for quoting.
> 
> James

The proposed sysfs attributes are racy in the sense that PCRs could
change in-between reading different hashes.

A blob containing all the hashes would make more sense as it does not
have this issue.

If this is for scripts to further process, it is also more efficient
than printable ASCII text.

/Jarkko

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

[James Bottomley]

On Thu, 2020-08-20 at 00:53 +0300, Jarkko Sakkinen wrote:
> > On Tue, 2020-08-18 at 14:17 -0300, Jason Gunthorpe wrote:
> > > On Tue, Aug 18, 2020 at 09:44:30AM -0700, James Bottomley wrote:
> > > 
> > > > The question you should be asking isn't whether the information
> > > > *could* be obtained by other means, but whether providing it in
> > > > this form facilitates current operations and whether the
> > > > interface would have users.
> > > 
> > > Sure. What are the use cases that need PCRs but no other TPM
> > > operations?
> > > 
> > > The cover letter didn't say. As PCR is really only useful in the
> > > context of the local TPM I'm not thinking of anything..
> > 
> > The three use cases I picked up at the Boot and Security MC were:
> > 
> >    1. local log verification: a script can run through the IMA
> > ascii log
> >       and construct the PCR 10 hash which can then be verified
> >    2. Knowing what the PCR values actually are for sealed
> > keys.  With the
> >       current trusted key infrastructure you have to calculate and
> > supply
> >       the hash yourself.  With the new proposed infrastructure, the
> > hash
> >       would be calculated by the seal operation, but you're still
> > going to
> >       need the actual PCR values to debug unseal failures.
> >    3. As a stability check for log shipping: you read the PCR take
> > the log
> >       then read the PCR: if the two reads are the same the PCR
> > backing the
> >       log is stable for quoting.
> > 
> > James
> 
> The proposed sysfs attributes are racy in the sense that PCRs could
> change in-between reading different hashes.

That's not really a problem, is it?  For use case 2. we expect them to
be stable otherwise you're doing the wrong thing sealing to them. For
the IMA PCR you use the stability protocol in 3.

> A blob containing all the hashes would make more sense as it does not
> have this issue.

It doesn't really buy anything though.  If you're verifying the log you
always have the problem that the PCR and the log are at different
points, so you follow the protocol in 3. or read PCR then log and
unwind the log until it matches or you've gone too far.

> If this is for scripts to further process, it is also more efficient
> than printable ASCII text.

I'm not fundamentally opposed to binary attributes, but realistically
if I want the hash of PCRs 1 4 and 6 it's not fundamentally different
to me whether I do

cat /sys/class/tpm/tpm0/pcr-sha256/1 /sys/class/tpm/tpm0/pcr-sha256/4 /sys/class/tpm/tpm0/pcr-sha256/6|sha256sum

or

cat /sys/class/tpm/tpm0/pcr-sha256/1 /sys/class/tpm/tpm0/pcr-sha256/4 /sys/class/tpm/tpm0/pcr-sha256/6|xxd -r -p|sha256sum

The point being the tool to convert the hex output back to binary
already exists and is well known ... and binary attributes have nasty
console properties if you accidentally cat them directly.

James

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

[Jarkko Sakkinen]

On Wed, Aug 19, 2020 at 03:46:15PM -0700, James Bottomley wrote:
> On Thu, 2020-08-20 at 00:53 +0300, Jarkko Sakkinen wrote:
> > > On Tue, 2020-08-18 at 14:17 -0300, Jason Gunthorpe wrote:
> > > > On Tue, Aug 18, 2020 at 09:44:30AM -0700, James Bottomley wrote:
> > > > 
> > > > > The question you should be asking isn't whether the information
> > > > > *could* be obtained by other means, but whether providing it in
> > > > > this form facilitates current operations and whether the
> > > > > interface would have users.
> > > > 
> > > > Sure. What are the use cases that need PCRs but no other TPM
> > > > operations?
> > > > 
> > > > The cover letter didn't say. As PCR is really only useful in the
> > > > context of the local TPM I'm not thinking of anything..
> > > 
> > > The three use cases I picked up at the Boot and Security MC were:
> > > 
> > >    1. local log verification: a script can run through the IMA
> > > ascii log
> > >       and construct the PCR 10 hash which can then be verified
> > >    2. Knowing what the PCR values actually are for sealed
> > > keys.  With the
> > >       current trusted key infrastructure you have to calculate and
> > > supply
> > >       the hash yourself.  With the new proposed infrastructure, the
> > > hash
> > >       would be calculated by the seal operation, but you're still
> > > going to
> > >       need the actual PCR values to debug unseal failures.
> > >    3. As a stability check for log shipping: you read the PCR take
> > > the log
> > >       then read the PCR: if the two reads are the same the PCR
> > > backing the
> > >       log is stable for quoting.
> > > 
> > > James
> > 
> > The proposed sysfs attributes are racy in the sense that PCRs could
> > change in-between reading different hashes.
> 
> That's not really a problem, is it?  For use case 2. we expect them to
> be stable otherwise you're doing the wrong thing sealing to them. For
> the IMA PCR you use the stability protocol in 3.
> 
> > A blob containing all the hashes would make more sense as it does not
> > have this issue.
> 
> It doesn't really buy anything though.  If you're verifying the log you
> always have the problem that the PCR and the log are at different
> points, so you follow the protocol in 3. or read PCR then log and
> unwind the log until it matches or you've gone too far.
> 
> > If this is for scripts to further process, it is also more efficient
> > than printable ASCII text.
> 
> I'm not fundamentally opposed to binary attributes, but realistically
> if I want the hash of PCRs 1 4 and 6 it's not fundamentally different
> to me whether I do
> 
> cat /sys/class/tpm/tpm0/pcr-sha256/1 /sys/class/tpm/tpm0/pcr-sha256/4 /sys/class/tpm/tpm0/pcr-sha256/6|sha256sum
> 
> or
> 
> cat /sys/class/tpm/tpm0/pcr-sha256/1 /sys/class/tpm/tpm0/pcr-sha256/4 /sys/class/tpm/tpm0/pcr-sha256/6|xxd -r -p|sha256sum
> 
> The point being the tool to convert the hex output back to binary
> already exists and is well known ... and binary attributes have nasty
> console properties if you accidentally cat them directly.
> 
> James

This does not look like a kind of framework of things that we want
to maintain. Especially given that it is easy to get all the data
through /dev/tpm0 easily. It is an enormous addition to uapi with
a questionable value.

/Jarkko

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

[Jarkko Sakkinen]

On Tue, Aug 18, 2020 at 09:44:30AM -0700, James Bottomley wrote:
> On Tue, 2020-08-18 at 19:19 +0300, Jarkko Sakkinen wrote:
> > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote:
> > > > Create sysfs per hash groups with 24 PCR files in them one group,
> > > > named pcr-<hash>, for each agile hash of the TPM.  The files are
> > > > plugged in to a PCR read function which is TPM version agnostic,
> > > > so this works also for TPM 1.2 but the hash is only sha1 in that
> > > > case.
> > > > 
> > > > Note: the macros used to create the hashes emit spurious
> > > > checkpatch warnings.  Do not try to "fix" them as checkpatch
> > > > recommends, otherwise they'll break.
> > > > 
> > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership
> > > > .com>
> > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > > 
> > > I have hard time understanding why this is required.
> > > 
> > > You can grab the information through /dev/tpm0 just fine.
> > 
> > I just think it is principally wrong to add sysfs files if they don't
> > have any measurable value other than perhaps some convenience.
> 
> That's pretty much the whole point of sysfs (and procfs): to add
> convenient extraction of information even if it could potentially be
> obtained by other sources.  For instance, the whole reason we add a lot
> of the broken out inquiry data in SCSI via sysfs is precisely so users
> don't have to go prodding devices with direct SCSI commands, which are
> pretty much analagous to TPM device commands.
> 
> The question you should be asking isn't whether the information *could*
> be obtained by other means, but whether providing it in this form
> facilitates current operations and whether the interface would have
> users.

Usually users use some appropriate applications to do their work, not
talk directly to the kernel.

Grabbing PCRs is a trivial program to write and I don't get the logic.

My email program is useful for me but I definitely do not want it to be
part of the Linux kernel. One great reason for that is that it would
involve a tedious process to update it later on.

/Jarkko

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

[Jarkko Sakkinen]

On Mon, Aug 17, 2020 at 02:35:06PM -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.

"PCR access is required because IMA tools should be able to run without
any sort of TSS dependencies."

AFAIK, this is the only reason to merge this and it is missing from the
description. Perhaps you could either include that sentence, or
alternatively write something along the lines?

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

Please also cc this at least to Greg and Jason Gunthorpe next time.

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

This is an unnecessarly hard to maintain.

A better construction would be this.

enum tpm_alg_misc {
	TPM_ALG_ERROR		= 0x0000,
	TPM_ALG_KEYEDHASH	= 0x0008,
	TPM_ALG_NULL		= 0x0010,
}

enum tpm_alg_hash {
	TPM_ALG_SHA1		= 0x0004,
	TPM_ALG_SHA256		= 0x000B,
	TPM_ALG_SHA384		= 0x000C,
	TPM_ALG_SHA512		= 0x000D,
	TPM_ALG_SM3_256		= 0x0012,
	TPM_ALG_HASH_MAX,
};

> +
>  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;

/Jarkko

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

[James Bottomley]

On Mon, 2020-09-14 at 20:41 +0300, Jarkko Sakkinen wrote:
> On Mon, Aug 17, 2020 at 02:35:06PM -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.
> 
> "PCR access is required because IMA tools should be able to run
> without any sort of TSS dependencies."
> 
> AFAIK, this is the only reason to merge this and it is missing from
> the description. Perhaps you could either include that sentence, or
> alternatively write something along the lines?

Sure, I'll add all of them: it's IMA tools, early boot and key locking
to PCR policy.

> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.c
> > om>
> > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> 
> Please also cc this at least to Greg and Jason Gunthorpe next time.

OK

[...]

> > 
> enum tpm_alg_misc {
> 	TPM_ALG_ERROR		= 0x0000,
> 	TPM_ALG_KEYEDHASH	= 0x0008,
> 	TPM_ALG_NULL		= 0x0010,
> }
> 
> enum tpm_alg_hash {
> 	TPM_ALG_SHA1		= 0x0004,
> 	TPM_ALG_SHA256		= 0x000B,
> 	TPM_ALG_SHA384		= 0x000C,
> 	TPM_ALG_SHA512		= 0x000D,
> 	TPM_ALG_SM3_256		= 0x0012,
> 	TPM_ALG_HASH_MAX,
> };

I can separate them if you insist, but the latter construction won't
work.  TPM_ALG_HASH_MAX will get set to the previous value plus one.

You can see this with the test programme:

---
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

enum tpm_alg_hash {
        TPM_ALG_SHA1            = 0x0004,
        TPM_ALG_SHA256          = 0x000B,
        TPM_ALG_SHA384          = 0x000C,
        TPM_ALG_SHA512          = 0x000D,
        TPM_ALG_SM3_256         = 0x0012,
        TPM_ALG_HASH_MAX,
};

int main()
{
	printf("TPM_ALG_HASH_MAX = %d\n", TPM_ALG_HASH_MAX);
}
---

Which gives

jejb@jarvis> ./a.out
TPM_ALG_HASH_MAX = 19

Which is clearly the wrong value (it's 0x12 + 1).

That being so, is there any reason to separate up the algorithms enum?

James

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

[Jarkko Sakkinen]

On Mon, Sep 14, 2020 at 12:19:28PM -0700, James Bottomley wrote:
> On Mon, 2020-09-14 at 20:41 +0300, Jarkko Sakkinen wrote:
> > On Mon, Aug 17, 2020 at 02:35:06PM -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.
> > 
> > "PCR access is required because IMA tools should be able to run
> > without any sort of TSS dependencies."
> > 
> > AFAIK, this is the only reason to merge this and it is missing from
> > the description. Perhaps you could either include that sentence, or
> > alternatively write something along the lines?
> 
> Sure, I'll add all of them: it's IMA tools, early boot and key locking
> to PCR policy.

Great!

> > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.c
> > > om>
> > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> > 
> > Please also cc this at least to Greg and Jason Gunthorpe next time.
> 
> OK
> 
> [...]
> 
> > > 
> > enum tpm_alg_misc {
> > 	TPM_ALG_ERROR		= 0x0000,
> > 	TPM_ALG_KEYEDHASH	= 0x0008,
> > 	TPM_ALG_NULL		= 0x0010,
> > }
> > 
> > enum tpm_alg_hash {
> > 	TPM_ALG_SHA1		= 0x0004,
> > 	TPM_ALG_SHA256		= 0x000B,
> > 	TPM_ALG_SHA384		= 0x000C,
> > 	TPM_ALG_SHA512		= 0x000D,
> > 	TPM_ALG_SM3_256		= 0x0012,
> > 	TPM_ALG_HASH_MAX,
> > };
> 
> I can separate them if you insist, but the latter construction won't
> work.  TPM_ALG_HASH_MAX will get set to the previous value plus one.
> 
> You can see this with the test programme:
> 
> ---
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> 
> enum tpm_alg_hash {
>         TPM_ALG_SHA1            = 0x0004,
>         TPM_ALG_SHA256          = 0x000B,
>         TPM_ALG_SHA384          = 0x000C,
>         TPM_ALG_SHA512          = 0x000D,
>         TPM_ALG_SM3_256         = 0x0012,
>         TPM_ALG_HASH_MAX,
> };
> 
> int main()
> {
> 	printf("TPM_ALG_HASH_MAX = %d\n", TPM_ALG_HASH_MAX);
> }
> ---
> 
> Which gives
> 
> jejb@jarvis> ./a.out
> TPM_ALG_HASH_MAX = 19
> 
> Which is clearly the wrong value (it's 0x12 + 1).
> 
> That being so, is there any reason to separate up the algorithms enum?
> 
> James

No, my bad.

/Jarkko

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

[Petr Vorel]

Hi,

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

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Tested-by: Petr Vorel <pvorel@suse.cz>
On both TPM 1.2 and TPM 2.0.
/sys/class/tpm/tpm0/pcr-sha*/* is exporting well on both.
James, thanks for implementing nice API!

BTW are all of these just TPM 1.2 specific?
/sys/class/tpm/tpm0/device/enabled
/sys/class/tpm/tpm0/device/pcrs
/sys/kernel/security/tpm0/binary_bios_measurements

Kind regards,
Petr

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

[James Bottomley]

On Thu, 2020-10-08 at 13:45 +0200, Petr Vorel wrote:
> Hi,
> 
> > 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>
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> Tested-by: Petr Vorel <pvorel@suse.cz>
> On both TPM 1.2 and TPM 2.0.
> /sys/class/tpm/tpm0/pcr-sha*/* is exporting well on both.
> James, thanks for implementing nice API!

You're welcome

> BTW are all of these just TPM 1.2 specific?
> /sys/class/tpm/tpm0/device/enabled
> /sys/class/tpm/tpm0/device/pcrs

These two are

> /sys/kernel/security/tpm0/binary_bios_measurements

But this one isn't ... although the format is different from 1.2 to 2.0
and the 2.0 version didn't appear until around 4.16.

James

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

[Jarkko Sakkinen]

On Thu, Oct 08, 2020 at 01:45:18PM +0200, Petr Vorel wrote:
> Hi,
> 
> > 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>
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> Tested-by: Petr Vorel <pvorel@suse.cz>
> On both TPM 1.2 and TPM 2.0.
> /sys/class/tpm/tpm0/pcr-sha*/* is exporting well on both.
> James, thanks for implementing nice API!

OK, great thank you. Unfortunately too late for v5.10 but without doubt
going to v5.11.

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

> BTW are all of these just TPM 1.2 specific?
> /sys/class/tpm/tpm0/device/enabled
> /sys/class/tpm/tpm0/device/pcrs
> /sys/kernel/security/tpm0/binary_bios_measurements

Yes.

> Kind regards,
> Petr

Thanks a lot for your feedback, easier to judge given that you actually
consume this :-)

/Jarkko