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

[PATCH v2] 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>
---
v2: It makes sense to actually compile the code.

 drivers/char/tpm/tpm-chip.c  |   4 --
 drivers/char/tpm/tpm-sysfs.c | 127 ++++++++++++++++++++++++++++++++-----------
 2 files changed, 94 insertions(+), 37 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..13405bdde725 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;
+	if (rc) {
+		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 v2] tpm: Add explicit chip->ops locking for sysfs attributes.

[Jason Gunthorpe]

On Thu, Nov 16, 2017 at 01:40:54PM -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>
> v2: It makes sense to actually compile the code.
> 
>  drivers/char/tpm/tpm-chip.c  |   4 --
>  drivers/char/tpm/tpm-sysfs.c | 127 ++++++++++++++++++++++++++++++++-----------
>  2 files changed, 94 insertions(+), 37 deletions(-)

I haven't exhaustively checked this, but as the originator of the
comment, this looks broadly like the right approach to me.

Cheers,
Jason