[PATCH] tpm: Add explicit chip->ops locking for sysfs attributes.

[PATCH] tpm: Add explicit chip->ops locking for sysfs attributes.

[Guenter Roeck]

Add explicit chip->ops locking for all sysfs attributes.
This lets us support those attributes on tpm2 devices.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/char/tpm/tpm-chip.c  |   4 --
 drivers/char/tpm/tpm-sysfs.c | 125 ++++++++++++++++++++++++++++++++-----------
 2 files changed, 93 insertions(+), 36 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 0eca20c5a80c..f0593ec1c11b 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -149,10 +149,6 @@ static void tpm_devs_release(struct device *dev)
  * Issues a TPM2_Shutdown command prior to loss of power, as required by the
  * TPM 2.0 spec.
  * Then, calls bus- and device- specific shutdown code.
- *
- * XXX: This codepath relies on the fact that sysfs is not enabled for
- * TPM2: sysfs uses an implicit lock on chip->ops, so this could race if TPM2
- * has sysfs support enabled before TPM sysfs's implicit locking is fixed.
  */
 static int tpm_class_shutdown(struct device *dev)
 {
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index 83a77a445538..292d5bbf79a8 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -47,18 +47,22 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
 
 	memset(&anti_replay, 0, sizeof(anti_replay));
 
-	rc = tpm_buf_init(&tpm_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK);
+	rc = tpm_try_get_ops(chip);
 	if (rc)
 		return rc;
 
+	rc = tpm_buf_init(&tpm_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK);
+	if (rc)
+		goto put_ops;
+
 	tpm_buf_append(&tpm_buf, anti_replay, sizeof(anti_replay));
 
 	rc = tpm_transmit_cmd(chip, NULL, tpm_buf.data, PAGE_SIZE,
 			      READ_PUBEK_RESULT_MIN_BODY_SIZE, 0,
 			      "attempting to read the PUBEK");
 	if (rc) {
-		tpm_buf_destroy(&tpm_buf);
-		return 0;
+		rc = 0;
+		goto buf_destroy;
 	}
 
 	out = (struct tpm_readpubek_out *)&tpm_buf.data[10];
@@ -91,7 +95,10 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
 	}
 
 	rc = str - buf;
+buf_destroy:
 	tpm_buf_destroy(&tpm_buf);
+put_ops:
+	tpm_put_ops(chip);
 	return rc;
 }
 static DEVICE_ATTR_RO(pubek);
@@ -106,11 +113,17 @@ static ssize_t pcrs_show(struct device *dev, struct device_attribute *attr,
 	char *str = buf;
 	struct tpm_chip *chip = to_tpm_chip(dev);
 
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
 	rc = tpm_getcap(chip, TPM_CAP_PROP_PCR, &cap,
 			"attempting to determine the number of PCRS",
 			sizeof(cap.num_pcrs));
 	if (rc)
-		return 0;
+		rc = 0;
+		goto put_ops;
+	}
 
 	num_pcrs = be32_to_cpu(cap.num_pcrs);
 	for (i = 0; i < num_pcrs; i++) {
@@ -122,23 +135,35 @@ static ssize_t pcrs_show(struct device *dev, struct device_attribute *attr,
 			str += sprintf(str, "%02X ", digest[j]);
 		str += sprintf(str, "\n");
 	}
-	return str - buf;
+	rc = str - buf;
+put_ops:
+	tpm_put_ops(chip);
+	return rc;
 }
 static DEVICE_ATTR_RO(pcrs);
 
 static ssize_t enabled_show(struct device *dev, struct device_attribute *attr,
 		     char *buf)
 {
+	struct tpm_chip *chip = to_tpm_chip(dev);
 	cap_t cap;
 	ssize_t rc;
 
-	rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_PERM, &cap,
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
+	rc = tpm_getcap(chip, TPM_CAP_FLAG_PERM, &cap,
 			"attempting to determine the permanent enabled state",
 			sizeof(cap.perm_flags));
-	if (rc)
-		return 0;
+	if (rc) {
+		rc = 0;
+		goto put_ops;
+	}
 
 	rc = sprintf(buf, "%d\n", !cap.perm_flags.disable);
+put_ops:
+	tpm_put_ops(chip);
 	return rc;
 }
 static DEVICE_ATTR_RO(enabled);
@@ -146,16 +171,25 @@ static DEVICE_ATTR_RO(enabled);
 static ssize_t active_show(struct device *dev, struct device_attribute *attr,
 		    char *buf)
 {
+	struct tpm_chip *chip = to_tpm_chip(dev);
 	cap_t cap;
 	ssize_t rc;
 
-	rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_PERM, &cap,
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
+	rc = tpm_getcap(chip, TPM_CAP_FLAG_PERM, &cap,
 			"attempting to determine the permanent active state",
 			sizeof(cap.perm_flags));
-	if (rc)
-		return 0;
+	if (rc) {
+		rc = 0;
+		goto put_ops;
+	}
 
 	rc = sprintf(buf, "%d\n", !cap.perm_flags.deactivated);
+put_ops:
+	tpm_put_ops(chip);
 	return rc;
 }
 static DEVICE_ATTR_RO(active);
@@ -163,16 +197,25 @@ static DEVICE_ATTR_RO(active);
 static ssize_t owned_show(struct device *dev, struct device_attribute *attr,
 			  char *buf)
 {
+	struct tpm_chip *chip = to_tpm_chip(dev);
 	cap_t cap;
 	ssize_t rc;
 
-	rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_PROP_OWNER, &cap,
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
+	rc = tpm_getcap(chip, TPM_CAP_PROP_OWNER, &cap,
 			"attempting to determine the owner state",
 			sizeof(cap.owned));
-	if (rc)
-		return 0;
+	if (rc) {
+		rc = 0;
+		goto put_ops;
+	}
 
 	rc = sprintf(buf, "%d\n", cap.owned);
+put_ops:
+	tpm_put_ops(chip);
 	return rc;
 }
 static DEVICE_ATTR_RO(owned);
@@ -180,16 +223,25 @@ static DEVICE_ATTR_RO(owned);
 static ssize_t temp_deactivated_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
+	struct tpm_chip *chip = to_tpm_chip(dev);
 	cap_t cap;
 	ssize_t rc;
 
-	rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_VOL, &cap,
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
+	rc = tpm_getcap(chip, TPM_CAP_FLAG_VOL, &cap,
 			"attempting to determine the temporary state",
 			sizeof(cap.stclear_flags));
-	if (rc)
-		return 0;
+	if (rc) {
+		rc = 0;
+		goto put_ops;
+	}
 
 	rc = sprintf(buf, "%d\n", cap.stclear_flags.deactivated);
+put_ops:
+	tpm_put_ops(chip);
 	return rc;
 }
 static DEVICE_ATTR_RO(temp_deactivated);
@@ -202,11 +254,18 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr,
 	ssize_t rc;
 	char *str = buf;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
 	rc = tpm_getcap(chip, TPM_CAP_PROP_MANUFACTURER, &cap,
 			"attempting to determine the manufacturer",
 			sizeof(cap.manufacturer_id));
-	if (rc)
-		return 0;
+	if (rc) {
+		rc = 0;
+		goto put_ops;
+	}
+
 	str += sprintf(str, "Manufacturer: 0x%x\n",
 		       be32_to_cpu(cap.manufacturer_id));
 
@@ -226,8 +285,10 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr,
 		rc = tpm_getcap(chip, TPM_CAP_VERSION_1_1, &cap,
 				"attempting to determine the 1.1 version",
 				sizeof(cap.tpm_version));
-		if (rc)
-			return 0;
+		if (rc) {
+			rc = 0;
+			goto put_ops;
+		}
 		str += sprintf(str,
 			       "TCG version: %d.%d\nFirmware version: %d.%d\n",
 			       cap.tpm_version.Major,
@@ -236,7 +297,10 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr,
 			       cap.tpm_version.revMinor);
 	}
 
-	return str - buf;
+	rc = str - buf;
+put_ops:
+	tpm_put_ops(chip);
+	return rc;
 }
 static DEVICE_ATTR_RO(caps);
 
@@ -244,10 +308,17 @@ static ssize_t cancel_store(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t count)
 {
 	struct tpm_chip *chip = to_tpm_chip(dev);
+	ssize_t rc;
+
 	if (chip == NULL)
 		return 0;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
 	chip->ops->cancel(chip);
+	tpm_put_ops(chip);
 	return count;
 }
 static DEVICE_ATTR_WO(cancel);
@@ -304,16 +375,6 @@ static const struct attribute_group tpm_dev_group = {
 
 void tpm_sysfs_add_device(struct tpm_chip *chip)
 {
-	/* XXX: If you wish to remove this restriction, you must first update
-	 * tpm_sysfs to explicitly lock chip->ops.
-	 */
-	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		return;
-
-	/* The sysfs routines rely on an implicit tpm_try_get_ops, device_del
-	 * is called before ops is null'd and the sysfs core synchronizes this
-	 * removal so that no callbacks are running or can run again
-	 */
 	WARN_ON(chip->groups_cnt != 0);
 	chip->groups[chip->groups_cnt++] = &tpm_dev_group;
 }
-- 
2.7.4

Re: [PATCH] tpm: Add explicit chip->ops locking for sysfs attributes.

[Jarkko Sakkinen]

On Thu, Nov 16, 2017 at 01:25:01PM -0800, Guenter Roeck wrote:
> Add explicit chip->ops locking for all sysfs attributes.
> This lets us support those attributes on tpm2 devices.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/char/tpm/tpm-chip.c  |   4 --
>  drivers/char/tpm/tpm-sysfs.c | 125 ++++++++++++++++++++++++++++++++-----------
>  2 files changed, 93 insertions(+), 36 deletions(-)

I think the patch looks ok (with a quick skim) as code change. We need
it. It should have been already done. Thanks for doing this.

I don't digest the commit message.

You should just to explain why this change needs to be done in order to
support sysfs attributes with TPM 2.0 devices and not speculate how it
will be used in future commits.

/Jarkko

Re: [PATCH] tpm: Add explicit chip->ops locking for sysfs attributes.

[Guenter Roeck]

On Tue, Nov 21, 2017 at 12:13:23AM +0200, Jarkko Sakkinen wrote:
> On Thu, Nov 16, 2017 at 01:25:01PM -0800, Guenter Roeck wrote:
> > Add explicit chip->ops locking for all sysfs attributes.
> > This lets us support those attributes on tpm2 devices.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  drivers/char/tpm/tpm-chip.c  |   4 --
> >  drivers/char/tpm/tpm-sysfs.c | 125 ++++++++++++++++++++++++++++++++-----------
> >  2 files changed, 93 insertions(+), 36 deletions(-)
> 
> I think the patch looks ok (with a quick skim) as code change. We need
> it. It should have been already done. Thanks for doing this.
> 
> I don't digest the commit message.
> 
> You should just to explain why this change needs to be done in order to
> support sysfs attributes with TPM 2.0 devices and not speculate how it
> will be used in future commits.
> 

How about the following ?

"tpm: Enable sysfs support for TPM2 devices

Access to chip->ops on TPM2 devices requires an explicit lock,
since the pointer is set to NULL in tpm_class_shutdown().
Implement that lock for sysfs access functions and enable sysfs
support for TPM2 devices."

Thanks,
Guenter

Re: [PATCH] tpm: Add explicit chip->ops locking for sysfs attributes.

[Jason Gunthorpe]

On Mon, Nov 20, 2017 at 02:45:23PM -0800, Guenter Roeck wrote:

> "tpm: Enable sysfs support for TPM2 devices
> 
> Access to chip->ops on TPM2 devices requires an explicit lock,
> since the pointer is set to NULL in tpm_class_shutdown().
> Implement that lock for sysfs access functions and enable sysfs
> support for TPM2 devices."

Wait.. one of the reasons we let it go with no sysfs for so long was
because there was not many sysfs files that were compatible with tpm2?

For TPM2 we have sort of had an API break of sorts from TPM1 in a
couple places around sysfs, and I would like to not re-introduce any
badly designed sysfs files for TPM2..

So.. When you apply this patch, what changes actually happen in the
sysfs directory?

Jason

Re: [PATCH] tpm: Add explicit chip->ops locking for sysfs attributes.

[Jarkko Sakkinen]

On Mon, Nov 20, 2017 at 02:45:23PM -0800, Guenter Roeck wrote:
> On Tue, Nov 21, 2017 at 12:13:23AM +0200, Jarkko Sakkinen wrote:
> > On Thu, Nov 16, 2017 at 01:25:01PM -0800, Guenter Roeck wrote:
> > > Add explicit chip->ops locking for all sysfs attributes.
> > > This lets us support those attributes on tpm2 devices.
> > > 
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > >  drivers/char/tpm/tpm-chip.c  |   4 --
> > >  drivers/char/tpm/tpm-sysfs.c | 125 ++++++++++++++++++++++++++++++++-----------
> > >  2 files changed, 93 insertions(+), 36 deletions(-)
> > 
> > I think the patch looks ok (with a quick skim) as code change. We need
> > it. It should have been already done. Thanks for doing this.
> > 
> > I don't digest the commit message.
> > 
> > You should just to explain why this change needs to be done in order to
> > support sysfs attributes with TPM 2.0 devices and not speculate how it
> > will be used in future commits.
> > 
> 
> How about the following ?
> 
> "tpm: Enable sysfs support for TPM2 devices
> 
> Access to chip->ops on TPM2 devices requires an explicit lock,
> since the pointer is set to NULL in tpm_class_shutdown().
> Implement that lock for sysfs access functions and enable sysfs
> support for TPM2 devices."
> 
> Thanks,
> Guenter

I can go with that. No need to send a new commit just for this :-)

I'll do some testing and give my final thoughts about the code
change and if everything is good I can change the commit message.
If not, submit the next version with that commit message.

/Jarkko

Re: [PATCH] tpm: Add explicit chip->ops locking for sysfs attributes.

[Jarkko Sakkinen]

On Mon, Nov 20, 2017 at 04:17:28PM -0700, Jason Gunthorpe wrote:
> On Mon, Nov 20, 2017 at 02:45:23PM -0800, Guenter Roeck wrote:
> 
> > "tpm: Enable sysfs support for TPM2 devices
> > 
> > Access to chip->ops on TPM2 devices requires an explicit lock,
> > since the pointer is set to NULL in tpm_class_shutdown().
> > Implement that lock for sysfs access functions and enable sysfs
> > support for TPM2 devices."
> 
> Wait.. one of the reasons we let it go with no sysfs for so long was
> because there was not many sysfs files that were compatible with tpm2?
> 
> For TPM2 we have sort of had an API break of sorts from TPM1 in a
> couple places around sysfs, and I would like to not re-introduce any
> badly designed sysfs files for TPM2..
> 
> So.. When you apply this patch, what changes actually happen in the
> sysfs directory?
> 
> Jason

Oops. I was too quick. This will cause all the TPM 1.x attributes
added also for TPM 2.0. That's not a great idea. The tpm_dev_group
should be only assigned for TPM 1.x devices. This commit should only
enable addition of sysfs attributes for TPM 2.0 devices.

/Jarkko

Re: [PATCH] tpm: Add explicit chip->ops locking for sysfs attributes.

[Guenter Roeck]

On Tue, Nov 21, 2017 at 01:49:47AM +0200, Jarkko Sakkinen wrote:
> On Mon, Nov 20, 2017 at 04:17:28PM -0700, Jason Gunthorpe wrote:
> > On Mon, Nov 20, 2017 at 02:45:23PM -0800, Guenter Roeck wrote:
> > 
> > > "tpm: Enable sysfs support for TPM2 devices
> > > 
> > > Access to chip->ops on TPM2 devices requires an explicit lock,
> > > since the pointer is set to NULL in tpm_class_shutdown().
> > > Implement that lock for sysfs access functions and enable sysfs
> > > support for TPM2 devices."
> > 
> > Wait.. one of the reasons we let it go with no sysfs for so long was
> > because there was not many sysfs files that were compatible with tpm2?
> > 
> > For TPM2 we have sort of had an API break of sorts from TPM1 in a
> > couple places around sysfs, and I would like to not re-introduce any
> > badly designed sysfs files for TPM2..
> > 
> > So.. When you apply this patch, what changes actually happen in the
> > sysfs directory?
> > 
> > Jason
> 
> Oops. I was too quick. This will cause all the TPM 1.x attributes
> added also for TPM 2.0. That's not a great idea. The tpm_dev_group
> should be only assigned for TPM 1.x devices. This commit should only
> enable addition of sysfs attributes for TPM 2.0 devices.
> 

After having a closer look, I agree. Sorry for my naivite.

I'll split the patch into two parts, and only add (hopefully)
non-controversial tpm2 attributes for now (which I think is durations
and timeouts). Or, in other words, I'll split the attributes into
two groups - one generic and one for tpm1.

Thanks,
Guenter

Re: [PATCH] tpm: Add explicit chip->ops locking for sysfs attributes.

[Jason Gunthorpe]

On Tue, Nov 21, 2017 at 10:28:58AM -0800, Guenter Roeck wrote:

> I'll split the patch into two parts, and only add (hopefully)
> non-controversial tpm2 attributes for now (which I think is durations
> and timeouts). Or, in other words, I'll split the attributes into
> two groups - one generic and one for tpm1.

Ok. Please look at new attributes you wish to add for tpm2 and see if
they meet the modern sysfs sensibility of one value per file, etc.

Jason

Re: [PATCH] tpm: Add explicit chip->ops locking for sysfs attributes.

[Jarkko Sakkinen]

On Tue, Nov 21, 2017 at 11:58:56AM -0700, Jason Gunthorpe wrote:
> On Tue, Nov 21, 2017 at 10:28:58AM -0800, Guenter Roeck wrote:
> 
> > I'll split the patch into two parts, and only add (hopefully)
> > non-controversial tpm2 attributes for now (which I think is durations
> > and timeouts). Or, in other words, I'll split the attributes into
> > two groups - one generic and one for tpm1.
> 
> Ok. Please look at new attributes you wish to add for tpm2 and see if
> they meet the modern sysfs sensibility of one value per file, etc.
> 
> Jason

In general: if something can be retrieved through /dev/tpm0, there is no
any sane reason to have a sysfs attribute for such.

/Jarkko

Re: [PATCH] tpm: Add explicit chip->ops locking for sysfs attributes.

[Guenter Roeck]

On Sun, Nov 26, 2017 at 03:56:41PM +0200, Jarkko Sakkinen wrote:
> On Tue, Nov 21, 2017 at 11:58:56AM -0700, Jason Gunthorpe wrote:
> > On Tue, Nov 21, 2017 at 10:28:58AM -0800, Guenter Roeck wrote:
> > 
> > > I'll split the patch into two parts, and only add (hopefully)
> > > non-controversial tpm2 attributes for now (which I think is durations
> > > and timeouts). Or, in other words, I'll split the attributes into
> > > two groups - one generic and one for tpm1.
> > 
> > Ok. Please look at new attributes you wish to add for tpm2 and see if
> > they meet the modern sysfs sensibility of one value per file, etc.
> > 
> > Jason
> 
> In general: if something can be retrieved through /dev/tpm0, there is no
> any sane reason to have a sysfs attribute for such.
> 

If I understand correctly, /dev/tpmX can be used to send any TPM command
to the chip. Given that, I translate your statement to mean that no sysfs
attribute will be accepted which sends a TPM command to the chip. This in
turn means that there is no neded to protect sysfs attributes with a lock
since any sysfs attribute requiring that lock will be rejected.

Thanks for the clarification. Please consider this patch abandoned.
It might be worthwhile mentioning that restriction in the code though -
the comment stating that TPM2 sysfs accesses are disabled due to lack
of locking is obvioulsy incorrect.

Guenter

Re: [PATCH] tpm: Add explicit chip->ops locking for sysfs attributes.

[Jarkko Sakkinen]

On Mon, Nov 27, 2017 at 08:02:55AM -0800, Guenter Roeck wrote:
> On Sun, Nov 26, 2017 at 03:56:41PM +0200, Jarkko Sakkinen wrote:
> > On Tue, Nov 21, 2017 at 11:58:56AM -0700, Jason Gunthorpe wrote:
> > > On Tue, Nov 21, 2017 at 10:28:58AM -0800, Guenter Roeck wrote:
> > > 
> > > > I'll split the patch into two parts, and only add (hopefully)
> > > > non-controversial tpm2 attributes for now (which I think is durations
> > > > and timeouts). Or, in other words, I'll split the attributes into
> > > > two groups - one generic and one for tpm1.
> > > 
> > > Ok. Please look at new attributes you wish to add for tpm2 and see if
> > > they meet the modern sysfs sensibility of one value per file, etc.
> > > 
> > > Jason
> > 
> > In general: if something can be retrieved through /dev/tpm0, there is no
> > any sane reason to have a sysfs attribute for such.
> > 
> 
> If I understand correctly, /dev/tpmX can be used to send any TPM command
> to the chip. Given that, I translate your statement to mean that no sysfs
> attribute will be accepted which sends a TPM command to the chip. This in
> turn means that there is no neded to protect sysfs attributes with a lock
> since any sysfs attribute requiring that lock will be rejected.
> 
> Thanks for the clarification. Please consider this patch abandoned.
> It might be worthwhile mentioning that restriction in the code though -
> the comment stating that TPM2 sysfs accesses are disabled due to lack
> of locking is obvioulsy incorrect.

Your statement about comment is correct. I guess we should rename the
file as tpm1_sysfs.c or tpm_legacy_sysfs.c.

It is not to say that new sysfs attributes would never make sense (like
in PPI case).

> Guenter

/Jarkko