linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/1] add sysfs exports for TPM 2 PCR registers
@ 2020-07-21 15:56 James Bottomley
  2020-07-21 15:56 ` [PATCH v2 1/1] tpm: add sysfs exports for all banks of " James Bottomley
  0 siblings, 1 reply; 9+ messages in thread
From: James Bottomley @ 2020-07-21 15:56 UTC (permalink / raw)
  To: linux-integrity; +Cc: Mimi Zohar, Jarkko Sakkinen

v2 to fix the failure on TPM 1.2

At last year's plumbers conference it was agreed in principle to
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 | 180 +++++++++++++++++++++++++++++++++++
 include/linux/tpm.h          |   8 +-
 2 files changed, 187 insertions(+), 1 deletion(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/1] tpm: add sysfs exports for all banks of PCR registers
  2020-07-21 15:56 [PATCH v2 0/1] add sysfs exports for TPM 2 PCR registers James Bottomley
@ 2020-07-21 15:56 ` James Bottomley
  2020-07-21 16:57   ` Mimi Zohar
  2020-07-21 23:16   ` Jerry Snitselaar
  0 siblings, 2 replies; 9+ messages in thread
From: James Bottomley @ 2020-07-21 15:56 UTC (permalink / raw)
  To: linux-integrity; +Cc: Mimi Zohar, Jarkko Sakkinen

use macro magic to create sysfs per hash groups with 24 PCR files in
them one for each possible agile hash of the TPM.  The files are
plugged in to a read function which is TPM version agnostic, so this
works also for TPM 1.2 although the hash is only sha1 in that case.
For every hash the TPM supports, a group named pcr-<hash> is created
and each of the PCR read files placed under it.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---

v2: fix TPM 1.2 legacy links failure
---
 drivers/char/tpm/tpm-sysfs.c | 180 +++++++++++++++++++++++++++++++++++
 include/linux/tpm.h          |   8 +-
 2 files changed, 187 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index d52bf4df0bca..6c4e984d6c8d 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -348,11 +348,191 @@ 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.  This has to be a BUG_ON because under this
+	 * condition, the chip->groups array will overflow corrupting
+	 * the chips structure.
+	 */
+	BUG_ON(chip->groups_cnt > TPM_MAX_HASHES);
 }
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 5026a06977e1..f73dc4f9fbf4 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -42,6 +42,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];
@@ -145,7 +151,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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/1] tpm: add sysfs exports for all banks of PCR registers
  2020-07-21 15:56 ` [PATCH v2 1/1] tpm: add sysfs exports for all banks of " James Bottomley
@ 2020-07-21 16:57   ` Mimi Zohar
  2020-07-21 23:16   ` Jerry Snitselaar
  1 sibling, 0 replies; 9+ messages in thread
From: Mimi Zohar @ 2020-07-21 16:57 UTC (permalink / raw)
  To: James Bottomley, linux-integrity; +Cc: Jarkko Sakkinen

On Tue, 2020-07-21 at 08:56 -0700, James Bottomley wrote:
> use macro magic to create sysfs per hash groups with 24 PCR files in
> them one for each possible agile hash of the TPM.  The files are
> plugged in to a read function which is TPM version agnostic, so this
> works also for TPM 1.2 although the hash is only sha1 in that case.
> For every hash the TPM supports, a group named pcr-<hash> is created
> and each of the PCR read files placed under it.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> ---
> 
> v2: fix TPM 1.2 legacy links failure

Thanks, this version works with TPM 1.2.

Mimi

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/1] tpm: add sysfs exports for all banks of PCR registers
  2020-07-21 15:56 ` [PATCH v2 1/1] tpm: add sysfs exports for all banks of " James Bottomley
  2020-07-21 16:57   ` Mimi Zohar
@ 2020-07-21 23:16   ` Jerry Snitselaar
  2020-07-21 23:37     ` James Bottomley
  1 sibling, 1 reply; 9+ messages in thread
From: Jerry Snitselaar @ 2020-07-21 23:16 UTC (permalink / raw)
  To: James Bottomley; +Cc: linux-integrity, Mimi Zohar, Jarkko Sakkinen


James Bottomley @ 2020-07-21 08:56 MST:

> use macro magic to create sysfs per hash groups with 24 PCR files in
> them one for each possible agile hash of the TPM.  The files are
> plugged in to a read function which is TPM version agnostic, so this
> works also for TPM 1.2 although the hash is only sha1 in that case.
> For every hash the TPM supports, a group named pcr-<hash> is created
> and each of the PCR read files placed under it.
>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>
> ---
>
> v2: fix TPM 1.2 legacy links failure
> ---
>  drivers/char/tpm/tpm-sysfs.c | 180 +++++++++++++++++++++++++++++++++++
>  include/linux/tpm.h          |   8 +-
>  2 files changed, 187 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
> index d52bf4df0bca..6c4e984d6c8d 100644
> --- a/drivers/char/tpm/tpm-sysfs.c
> +++ b/drivers/char/tpm/tpm-sysfs.c
> @@ -348,11 +348,191 @@ 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.  This has to be a BUG_ON because under this
> +	 * condition, the chip->groups array will overflow corrupting
> +	 * the chips structure.
> +	 */
> +	BUG_ON(chip->groups_cnt > TPM_MAX_HASHES);

Should this check be 3 + TPM_MAX_HASHES like below?

>  }
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 5026a06977e1..f73dc4f9fbf4 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -42,6 +42,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];
> @@ -145,7 +151,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;


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/1] tpm: add sysfs exports for all banks of PCR registers
  2020-07-21 23:16   ` Jerry Snitselaar
@ 2020-07-21 23:37     ` James Bottomley
  2020-07-22  0:02       ` Jerry Snitselaar
  0 siblings, 1 reply; 9+ messages in thread
From: James Bottomley @ 2020-07-21 23:37 UTC (permalink / raw)
  To: Jerry Snitselaar; +Cc: linux-integrity, Mimi Zohar, Jarkko Sakkinen

On Tue, 2020-07-21 at 16:16 -0700, Jerry Snitselaar wrote:
> James Bottomley @ 2020-07-21 08:56 MST:
[...]
> > +	/*
> > +	 * This will only trigger if someone has added an
> > additional
> > +	 * hash to the tpm_algorithms enum without incrementing
> > +	 * TPM_MAX_HASHES.  This has to be a BUG_ON because under
> > this
> > +	 * condition, the chip->groups array will overflow
> > corrupting
> > +	 * the chips structure.
> > +	 */
> > +	BUG_ON(chip->groups_cnt > TPM_MAX_HASHES);
> 
> Should this check be 3 + TPM_MAX_HASHES like below?

No, because at this point only a single additional group has been
addedin addition to the hashes groups.  The first line of
tpm_sysfs_add_device is

	WARN_ON(chip->groups_cnt != 0);

And then we add the unnamed group.  This loop over the banks follows
it, so chip->groups_cnt should be nr_banks_allocated by the end (it's
the index, which is one fewer than the number of entries in chip-
>groups[]).  We have a problem if nr_banks_allocated > TPM_MAX_HASHES
which is what the BUG_ON checks.

James


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/1] tpm: add sysfs exports for all banks of PCR registers
  2020-07-21 23:37     ` James Bottomley
@ 2020-07-22  0:02       ` Jerry Snitselaar
  2020-07-22  0:39         ` James Bottomley
  0 siblings, 1 reply; 9+ messages in thread
From: Jerry Snitselaar @ 2020-07-22  0:02 UTC (permalink / raw)
  To: James Bottomley; +Cc: linux-integrity, Mimi Zohar, Jarkko Sakkinen


James Bottomley @ 2020-07-21 16:37 MST:

> On Tue, 2020-07-21 at 16:16 -0700, Jerry Snitselaar wrote:
>> James Bottomley @ 2020-07-21 08:56 MST:
> [...]
>> > +	/*
>> > +	 * This will only trigger if someone has added an
>> > additional
>> > +	 * hash to the tpm_algorithms enum without incrementing
>> > +	 * TPM_MAX_HASHES.  This has to be a BUG_ON because under
>> > this
>> > +	 * condition, the chip->groups array will overflow
>> > corrupting
>> > +	 * the chips structure.
>> > +	 */
>> > +	BUG_ON(chip->groups_cnt > TPM_MAX_HASHES);
>> 
>> Should this check be 3 + TPM_MAX_HASHES like below?
>
> No, because at this point only a single additional group has been
> addedin addition to the hashes groups.  The first line of
> tpm_sysfs_add_device is
>
> 	WARN_ON(chip->groups_cnt != 0);
>
> And then we add the unnamed group.  This loop over the banks follows
> it, so chip->groups_cnt should be nr_banks_allocated by the end (it's
> the index, which is one fewer than the number of entries in chip-
>>groups[]).  We have a problem if nr_banks_allocated > TPM_MAX_HASHES
> which is what the BUG_ON checks.
>
> James

If the chip supported all 5 listed cases wouldn't groups_cnt be 6 at this
point?


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/1] tpm: add sysfs exports for all banks of PCR registers
  2020-07-22  0:02       ` Jerry Snitselaar
@ 2020-07-22  0:39         ` James Bottomley
  2020-07-22  0:51           ` Jerry Snitselaar
  0 siblings, 1 reply; 9+ messages in thread
From: James Bottomley @ 2020-07-22  0:39 UTC (permalink / raw)
  To: Jerry Snitselaar; +Cc: linux-integrity, Mimi Zohar, Jarkko Sakkinen

On Tue, 2020-07-21 at 17:02 -0700, Jerry Snitselaar wrote:
> James Bottomley @ 2020-07-21 16:37 MST:
> 
> > On Tue, 2020-07-21 at 16:16 -0700, Jerry Snitselaar wrote:
> > > James Bottomley @ 2020-07-21 08:56 MST:
> > 
> > [...]
> > > > +	/*
> > > > +	 * This will only trigger if someone has added an
> > > > additional
> > > > +	 * hash to the tpm_algorithms enum without
> > > > incrementing
> > > > +	 * TPM_MAX_HASHES.  This has to be a BUG_ON because
> > > > under
> > > > this
> > > > +	 * condition, the chip->groups array will overflow
> > > > corrupting
> > > > +	 * the chips structure.
> > > > +	 */
> > > > +	BUG_ON(chip->groups_cnt > TPM_MAX_HASHES);
> > > 
> > > Should this check be 3 + TPM_MAX_HASHES like below?
> > 
> > No, because at this point only a single additional group has been
> > addedin addition to the hashes groups.  The first line of
> > tpm_sysfs_add_device is
> > 
> > 	WARN_ON(chip->groups_cnt != 0);
> > 
> > And then we add the unnamed group.  This loop over the banks
> > follows it, so chip->groups_cnt should be nr_banks_allocated by the
> > end (it's the index, which is one fewer than the number of entries
> > in chip->groups[]).  We have a problem if nr_banks_allocated >
> > TPM_MAX_HASHES
> > 
> > which is what the BUG_ON checks.
> > 
> > James
> 
> If the chip supported all 5 listed cases wouldn't groups_cnt be 6 at
> this point?

Actually, yes, I think it would be because it's pointing at the next
free index not the current one.  So it should be BUG_ON (chip-
>groups_cnt > TPM_MAX_HASHES + 1)

James


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/1] tpm: add sysfs exports for all banks of PCR registers
  2020-07-22  0:39         ` James Bottomley
@ 2020-07-22  0:51           ` Jerry Snitselaar
  2020-07-22 15:29             ` James Bottomley
  0 siblings, 1 reply; 9+ messages in thread
From: Jerry Snitselaar @ 2020-07-22  0:51 UTC (permalink / raw)
  To: James Bottomley; +Cc: linux-integrity, Mimi Zohar, Jarkko Sakkinen


James Bottomley @ 2020-07-21 17:39 MST:

> On Tue, 2020-07-21 at 17:02 -0700, Jerry Snitselaar wrote:
>> James Bottomley @ 2020-07-21 16:37 MST:
>> 
>> > On Tue, 2020-07-21 at 16:16 -0700, Jerry Snitselaar wrote:
>> > > James Bottomley @ 2020-07-21 08:56 MST:
>> > 
>> > [...]
>> > > > +	/*
>> > > > +	 * This will only trigger if someone has added an
>> > > > additional
>> > > > +	 * hash to the tpm_algorithms enum without
>> > > > incrementing
>> > > > +	 * TPM_MAX_HASHES.  This has to be a BUG_ON because
>> > > > under
>> > > > this
>> > > > +	 * condition, the chip->groups array will overflow
>> > > > corrupting
>> > > > +	 * the chips structure.
>> > > > +	 */
>> > > > +	BUG_ON(chip->groups_cnt > TPM_MAX_HASHES);
>> > > 
>> > > Should this check be 3 + TPM_MAX_HASHES like below?
>> > 
>> > No, because at this point only a single additional group has been
>> > addedin addition to the hashes groups.  The first line of
>> > tpm_sysfs_add_device is
>> > 
>> > 	WARN_ON(chip->groups_cnt != 0);
>> > 
>> > And then we add the unnamed group.  This loop over the banks
>> > follows it, so chip->groups_cnt should be nr_banks_allocated by the
>> > end (it's the index, which is one fewer than the number of entries
>> > in chip->groups[]).  We have a problem if nr_banks_allocated >
>> > TPM_MAX_HASHES
>> > 
>> > which is what the BUG_ON checks.
>> > 
>> > James
>> 
>> If the chip supported all 5 listed cases wouldn't groups_cnt be 6 at
>> this point?
>
> Actually, yes, I think it would be because it's pointing at the next
> free index not the current one.  So it should be BUG_ON (chip-
>>groups_cnt > TPM_MAX_HASHES + 1)
>
> James

One other thought, should a note be added above tpm_algorithms to note
that when that is changed TPM_MAX_HASHES should be changed as well?

With the above change to the BUG_ON you can add to v3:

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/1] tpm: add sysfs exports for all banks of PCR registers
  2020-07-22  0:51           ` Jerry Snitselaar
@ 2020-07-22 15:29             ` James Bottomley
  0 siblings, 0 replies; 9+ messages in thread
From: James Bottomley @ 2020-07-22 15:29 UTC (permalink / raw)
  To: Jerry Snitselaar; +Cc: linux-integrity, Mimi Zohar, Jarkko Sakkinen

On Tue, 2020-07-21 at 17:51 -0700, Jerry Snitselaar wrote:
> James Bottomley @ 2020-07-21 17:39 MST:
> 
> > On Tue, 2020-07-21 at 17:02 -0700, Jerry Snitselaar wrote:
> > > James Bottomley @ 2020-07-21 16:37 MST:
> > > 
> > > > On Tue, 2020-07-21 at 16:16 -0700, Jerry Snitselaar wrote:
> > > > > James Bottomley @ 2020-07-21 08:56 MST:
> > > > 
> > > > [...]
> > > > > > +	/*
> > > > > > +	 * This will only trigger if someone has added an
> > > > > > additional
> > > > > > +	 * hash to the tpm_algorithms enum without
> > > > > > incrementing
> > > > > > +	 * TPM_MAX_HASHES.  This has to be a BUG_ON
> > > > > > because
> > > > > > under
> > > > > > this
> > > > > > +	 * condition, the chip->groups array will overflow
> > > > > > corrupting
> > > > > > +	 * the chips structure.
> > > > > > +	 */
> > > > > > +	BUG_ON(chip->groups_cnt > TPM_MAX_HASHES);
> > > > > 
> > > > > Should this check be 3 + TPM_MAX_HASHES like below?
> > > > 
> > > > No, because at this point only a single additional group has
> > > > been addedin addition to the hashes groups.  The first line of
> > > > tpm_sysfs_add_device is
> > > > 
> > > > 	WARN_ON(chip->groups_cnt != 0);
> > > > 
> > > > And then we add the unnamed group.  This loop over the banks
> > > > follows it, so chip->groups_cnt should be nr_banks_allocated by
> > > > the end (it's the index, which is one fewer than the number of
> > > > entries in chip->groups[]).  We have a problem if
> > > > nr_banks_allocated > TPM_MAX_HASHES
> > > > 
> > > > which is what the BUG_ON checks.
> > > > 
> > > > James
> > > 
> > > If the chip supported all 5 listed cases wouldn't groups_cnt be 6
> > > at this point?
> > 
> > Actually, yes, I think it would be because it's pointing at the
> > next free index not the current one.  So it should be BUG_ON (chip-
> > > groups_cnt > TPM_MAX_HASHES + 1)
> > 
> > James
> 
> One other thought, should a note be added above tpm_algorithms to
> note that when that is changed TPM_MAX_HASHES should be changed as
> well?

I certainly can ... it's free.

> With the above change to the BUG_ON you can add to v3:

OK, I also changed the BUG_ON back to a WARN_ON to match the initial
one (if that one ever tripped, we'd get an overflow in the chip-
>groups[] as well, so it seems reasonable to keep them matching).

James


> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-07-22 15:29 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-21 15:56 [PATCH v2 0/1] add sysfs exports for TPM 2 PCR registers James Bottomley
2020-07-21 15:56 ` [PATCH v2 1/1] tpm: add sysfs exports for all banks of " James Bottomley
2020-07-21 16:57   ` Mimi Zohar
2020-07-21 23:16   ` Jerry Snitselaar
2020-07-21 23:37     ` James Bottomley
2020-07-22  0:02       ` Jerry Snitselaar
2020-07-22  0:39         ` James Bottomley
2020-07-22  0:51           ` Jerry Snitselaar
2020-07-22 15:29             ` James Bottomley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).