linux-security-module.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v11 00/16] Remove nested TPM operations
@ 2019-02-05 22:47 Jarkko Sakkinen
  2019-02-05 22:47 ` [PATCH v11 01/16] tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter Jarkko Sakkinen
                   ` (16 more replies)
  0 siblings, 17 replies; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-05 22:47 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain, Jarkko Sakkinen

Make the changes necessary to detach TPM space code and TPM activation
code out of the tpm_transmit() flow because of both of these can cause
nested tpm_transmit() calls. The nesteds calls make the whole flow hard
to maintain, and thus, it is better to just fix things now before this
turns into a bigger mess.

v11:
* Drop the patch that tries to flush TPM space on system. Not a proper
  fallback + out of scope for this patch set.

v10:
* Use void pointers to avoid unnecessary casts in functions paramaters
  where it makes sense.

v9:
* Fixed again tpm_try_get_ops().
* Added missing reviewed-by's.

v8:
* Re-add the check for ret < 0 after calling tpm_try_transmit() that
  was dropped by mistake while moving code.
* Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
  fails.

v7:
*  Reorganize series so that more trivial and self-contained changes are
   in the head.

v6:
* When tpm_validate_commmand() was moved to tpm2-space.c, the struct for
  the TPM header was incorrectly declared as struct tpm_input_header.
* Fix return value in tpm_validate_command().

v5:
* Add the missing rev's from Stefan Berger.

v4:
* Return 0 from pcrs_show() when tpm1_pcr_read() fails.
* Fix error handling flow in tpm_try_transmit().
* Replace struct tpm_input_header and struct tpm_output_header with
  struct tpm_header.

v3:
* Encapsulate power gating code to tpm_chip_start() and tpm_chip_stop().
* Move TPM power gating code and locking to tpm_try_get_ops() and
  tpm_put_ops().
* Call power gating code directly in tpm_chip_register() and
  tpm2_del_space().

v2:
* Print tpm2_commit_space() error inside tpm2_commit_space()
* Error code was not printed when recv() callback failed. It is
  fixed in this version.
* Added a patch that removes @space from tpm_transmit().
* Fixed a regression in earlier series. Forgot to amend the change
  from the staging area that renames NESTED to UNLOCKED in tpm2-space.c.

Jarkko Sakkinen (16):
  tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
  tpm: fix invalid return value in pubek_show()
  tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
  tpm: print tpm2_commit_space() error inside tpm2_commit_space()
  tpm: declare struct tpm_header
  tpm: access command header through struct in tpm_try_transmit()
  tpm: encapsulate tpm_dev_transmit()
  tpm: clean up tpm_try_transmit() error handling flow
  tpm: move tpm_validate_commmand() to tpm2-space.c
  tpm: move TPM space code out of tpm_transmit()
  tpm: remove @space from tpm_transmit()
  tpm: use tpm_try_get_ops() in tpm-sysfs.c.
  tpm: remove TPM_TRANSMIT_UNLOCKED flag
  tpm: introduce tpm_chip_start() and tpm_chip_stop()
  tpm: take TPM chip power gating out of tpm_transmit()
  tpm: remove @flags from tpm_transmit()

 drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
 drivers/char/tpm/tpm-dev-common.c |  44 ++++-
 drivers/char/tpm/tpm-interface.c  | 264 ++++++------------------------
 drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
 drivers/char/tpm/tpm.h            |  64 +++-----
 drivers/char/tpm/tpm1-cmd.c       |  28 +---
 drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
 drivers/char/tpm/tpm2-space.c     |  91 +++++++---
 drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
 drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
 drivers/char/tpm/xen-tpmfront.c   |   2 +-
 11 files changed, 408 insertions(+), 421 deletions(-)

-- 
2.19.1


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

* [PATCH v11 01/16] tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
  2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
@ 2019-02-05 22:47 ` Jarkko Sakkinen
  2019-02-05 22:47 ` [PATCH v11 02/16] tpm: fix invalid return value in pubek_show() Jarkko Sakkinen
                   ` (15 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-05 22:47 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain, Jarkko Sakkinen

Since we pass an initialized struct tpm_buf instance in every call site
now, it is cleaner to pass that directly to the tpm_transmit_cmd() as
the TPM command/response buffer.

Fine-tune a little bit tpm_transmit() and tpm_transmit_cmd() comments
while doing this.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-interface.c  | 67 +++++++++++++++++--------------
 drivers/char/tpm/tpm-sysfs.c      |  2 +-
 drivers/char/tpm/tpm.h            |  5 +--
 drivers/char/tpm/tpm1-cmd.c       | 26 ++++--------
 drivers/char/tpm/tpm2-cmd.c       | 37 +++++++----------
 drivers/char/tpm/tpm2-space.c     |  4 +-
 drivers/char/tpm/tpm_vtpm_proxy.c |  3 +-
 7 files changed, 64 insertions(+), 80 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 6339a2e289ae..5ef5c1e6947d 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -298,23 +298,22 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
 
 /**
  * tpm_transmit - Internal kernel interface to transmit TPM commands.
+ * @chip:	a TPM chip to use
+ * @space:	a TPM space
+ * @buf:	a TPM command buffer
+ * @bufsiz:	length of the TPM command buffer
+ * @flags:	TPM transmit flags
  *
- * @chip: TPM chip to use
- * @space: tpm space
- * @buf: TPM command buffer
- * @bufsiz: length of the TPM command buffer
- * @flags: tpm transmit flags - bitmap
+ * A wrapper around tpm_try_transmit() that handles TPM2_RC_RETRY returns from
+ * the TPM and retransmits the command after a delay up to a maximum wait of
+ * TPM2_DURATION_LONG.
  *
- * A wrapper around tpm_try_transmit that handles TPM2_RC_RETRY
- * returns from the TPM and retransmits the command after a delay up
- * to a maximum wait of TPM2_DURATION_LONG.
- *
- * Note: TPM1 never returns TPM2_RC_RETRY so the retry logic is TPM2
- * only
+ * Note that TPM 1.x never returns TPM2_RC_RETRY so the retry logic is TPM 2.0
+ * only.
  *
  * Return:
- *     the length of the return when the operation is successful.
- *     A negative number for system errors (errno).
+ * * The response length	- OK
+ * * -errno			- A system error
  */
 ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		     u8 *buf, size_t bufsiz, unsigned int flags)
@@ -365,33 +364,31 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	}
 	return ret;
 }
+
 /**
  * tpm_transmit_cmd - send a tpm command to the device
- *    The function extracts tpm out header return code
- *
- * @chip: TPM chip to use
- * @space: tpm space
- * @buf: TPM command buffer
- * @bufsiz: length of the buffer
- * @min_rsp_body_length: minimum expected length of response body
- * @flags: tpm transmit flags - bitmap
- * @desc: command description used in the error message
+ * @chip:			a TPM chip to use
+ * @space:			a TPM space
+ * @buf:			a TPM command buffer
+ * @min_rsp_body_length:	minimum expected length of response body
+ * @flags:			TPM transmit flags
+ * @desc:			command description used in the error message
  *
  * Return:
- *     0 when the operation is successful.
- *     A negative number for system errors (errno).
- *     A positive number for a TPM error.
+ * * 0		- OK
+ * * -errno	- A system error
+ * * TPM_RC	- A TPM error
  */
 ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
-			 void *buf, size_t bufsiz,
-			 size_t min_rsp_body_length, unsigned int flags,
-			 const char *desc)
+			 struct tpm_buf *buf, size_t min_rsp_body_length,
+			 unsigned int flags, const char *desc)
 {
-	const struct tpm_output_header *header = buf;
+	const struct tpm_output_header *header =
+		(struct tpm_output_header *)buf->data;
 	int err;
 	ssize_t len;
 
-	len = tpm_transmit(chip, space, buf, bufsiz, flags);
+	len = tpm_transmit(chip, space, buf->data, PAGE_SIZE, flags);
 	if (len <  0)
 		return len;
 
@@ -528,14 +525,22 @@ EXPORT_SYMBOL_GPL(tpm_pcr_extend);
  */
 int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen)
 {
+	struct tpm_buf buf;
 	int rc;
 
 	chip = tpm_find_get_ops(chip);
 	if (!chip)
 		return -ENODEV;
 
-	rc = tpm_transmit_cmd(chip, NULL, cmd, buflen, 0, 0,
+	rc = tpm_buf_init(&buf, 0, 0);
+	if (rc)
+		goto out;
+
+	memcpy(buf.data, cmd, buflen);
+	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0,
 			      "attempting to a send a command");
+	tpm_buf_destroy(&buf);
+out:
 	tpm_put_ops(chip);
 	return rc;
 }
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index b88e08ec2c59..c2769e55cb6c 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -53,7 +53,7 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
 
 	tpm_buf_append(&tpm_buf, anti_replay, sizeof(anti_replay));
 
-	rc = tpm_transmit_cmd(chip, NULL, tpm_buf.data, PAGE_SIZE,
+	rc = tpm_transmit_cmd(chip, NULL, &tpm_buf,
 			      READ_PUBEK_RESULT_MIN_BODY_SIZE, 0,
 			      "attempting to read the PUBEK");
 	if (rc) {
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f27d1f38a93d..49bca4d1e786 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -503,9 +503,8 @@ enum tpm_transmit_flags {
 ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		     u8 *buf, size_t bufsiz, unsigned int flags);
 ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
-			 void *buf, size_t bufsiz,
-			 size_t min_rsp_body_length, unsigned int flags,
-			 const char *desc);
+			 struct tpm_buf *buf, size_t min_rsp_body_length,
+			 unsigned int flags, const char *desc);
 int tpm_get_timeouts(struct tpm_chip *);
 int tpm_auto_startup(struct tpm_chip *chip);
 
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index bda9a16b44f6..10a0b7683b4b 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -334,11 +334,9 @@ static int tpm1_startup(struct tpm_chip *chip)
 
 	tpm_buf_append_u16(&buf, TPM_ST_CLEAR);
 
-	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
+	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0,
 			      "attempting to start the TPM");
-
 	tpm_buf_destroy(&buf);
-
 	return rc;
 }
 
@@ -461,9 +459,7 @@ int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
 	tpm_buf_append_u32(&buf, pcr_idx);
 	tpm_buf_append(&buf, hash, TPM_DIGEST_SIZE);
 
-	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
-			      TPM_DIGEST_SIZE, 0, log_msg);
-
+	rc = tpm_transmit_cmd(chip, NULL, &buf, TPM_DIGEST_SIZE, 0, log_msg);
 	tpm_buf_destroy(&buf);
 	return rc;
 }
@@ -493,11 +489,9 @@ ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
 		tpm_buf_append_u32(&buf, 4);
 		tpm_buf_append_u32(&buf, subcap_id);
 	}
-	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
-			      min_cap_length, 0, desc);
+	rc = tpm_transmit_cmd(chip, NULL, &buf, min_cap_length, 0, desc);
 	if (!rc)
 		*cap = *(cap_t *)&buf.data[TPM_HEADER_SIZE + 4];
-
 	tpm_buf_destroy(&buf);
 	return rc;
 }
@@ -536,7 +530,7 @@ int tpm1_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
 	do {
 		tpm_buf_append_u32(&buf, num_bytes);
 
-		rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
+		rc = tpm_transmit_cmd(chip, NULL, &buf,
 				      sizeof(out->rng_data_len), 0,
 				      "attempting get random");
 		if (rc)
@@ -582,8 +576,7 @@ int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf)
 
 	tpm_buf_append_u32(&buf, pcr_idx);
 
-	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
-			      TPM_DIGEST_SIZE, 0,
+	rc = tpm_transmit_cmd(chip, NULL, &buf, TPM_DIGEST_SIZE, 0,
 			      "attempting to read a pcr value");
 	if (rc)
 		goto out;
@@ -617,11 +610,8 @@ static int tpm1_continue_selftest(struct tpm_chip *chip)
 	if (rc)
 		return rc;
 
-	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
-			      0, 0, "continue selftest");
-
+	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0, "continue selftest");
 	tpm_buf_destroy(&buf);
-
 	return rc;
 }
 
@@ -746,9 +736,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
 		return rc;
 	/* now do the actual savestate */
 	for (try = 0; try < TPM_RETRY; try++) {
-		rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
-				      0, 0, NULL);
-
+		rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0, NULL);
 		/*
 		 * If the TPM indicates that it is too busy to respond to
 		 * this command then retry before giving up.  It can take
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index a6bec13afa69..2bcf470c8e5d 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -197,8 +197,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf)
 	tpm_buf_append(&buf, (const unsigned char *)pcr_select,
 		       sizeof(pcr_select));
 
-	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
-			res_buf ? "attempting to read a pcr value" : NULL);
+	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0, res_buf ?
+			      "attempting to read a pcr value" : NULL);
 	if (rc == 0 && res_buf) {
 		out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
 		memcpy(res_buf, out->digest, SHA1_DIGEST_SIZE);
@@ -264,7 +264,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
 		}
 	}
 
-	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
+	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0,
 			      "attempting extend a PCR value");
 
 	tpm_buf_destroy(&buf);
@@ -309,7 +309,7 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
 	do {
 		tpm_buf_reset(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_RANDOM);
 		tpm_buf_append_u16(&buf, num_bytes);
-		err = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
+		err = tpm_transmit_cmd(chip, NULL, &buf,
 				       offsetof(struct tpm2_get_random_out,
 						buffer),
 				       0, "attempting get random");
@@ -362,9 +362,7 @@ void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
 
 	tpm_buf_append_u32(&buf, handle);
 
-	(void) tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, flags,
-				"flushing context");
-
+	tpm_transmit_cmd(chip, NULL, &buf, 0, flags, "flushing context");
 	tpm_buf_destroy(&buf);
 }
 
@@ -478,8 +476,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 		goto out;
 	}
 
-	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 4, 0,
-			      "sealing data");
+	rc = tpm_transmit_cmd(chip, NULL, &buf, 4, 0, "sealing data");
 	if (rc)
 		goto out;
 
@@ -561,8 +558,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 		goto out;
 	}
 
-	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 4, flags,
-			      "loading blob");
+	rc = tpm_transmit_cmd(chip, NULL, &buf, 4, flags, "loading blob");
 	if (!rc)
 		*blob_handle = be32_to_cpup(
 			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
@@ -612,8 +608,7 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
 			     options->blobauth /* hmac */,
 			     TPM_DIGEST_SIZE);
 
-	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 6, flags,
-			      "unsealing");
+	rc = tpm_transmit_cmd(chip, NULL, &buf, 6, flags, "unsealing");
 	if (rc > 0)
 		rc = -EPERM;
 
@@ -703,7 +698,7 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,  u32 *value,
 	tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
 	tpm_buf_append_u32(&buf, property_id);
 	tpm_buf_append_u32(&buf, 1);
-	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0, NULL);
+	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0, NULL);
 	if (!rc) {
 		out = (struct tpm2_get_cap_out *)
 			&buf.data[TPM_HEADER_SIZE];
@@ -733,8 +728,7 @@ void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type)
 	if (rc)
 		return;
 	tpm_buf_append_u16(&buf, shutdown_type);
-	tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
-			 "stopping the TPM");
+	tpm_transmit_cmd(chip, NULL, &buf, 0, 0, "stopping the TPM");
 	tpm_buf_destroy(&buf);
 }
 
@@ -763,7 +757,7 @@ static int tpm2_do_selftest(struct tpm_chip *chip)
 			return rc;
 
 		tpm_buf_append_u8(&buf, full);
-		rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
+		rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0,
 				      "attempting the self test");
 		tpm_buf_destroy(&buf);
 
@@ -800,7 +794,7 @@ int tpm2_probe(struct tpm_chip *chip)
 	tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
 	tpm_buf_append_u32(&buf, TPM_PT_TOTAL_COMMANDS);
 	tpm_buf_append_u32(&buf, 1);
-	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0, NULL);
+	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0, NULL);
 	/* We ignore TPM return codes on purpose. */
 	if (rc >=  0) {
 		out = (struct tpm_output_header *)buf.data;
@@ -839,7 +833,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	tpm_buf_append_u32(&buf, 0);
 	tpm_buf_append_u32(&buf, 1);
 
-	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 9, 0,
+	rc = tpm_transmit_cmd(chip, NULL, &buf, 9, 0,
 			      "get tpm pcr allocation");
 	if (rc)
 		goto out;
@@ -911,8 +905,7 @@ static int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip)
 	tpm_buf_append_u32(&buf, TPM2_CC_FIRST);
 	tpm_buf_append_u32(&buf, nr_commands);
 
-	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
-			      9 + 4 * nr_commands, 0, NULL);
+	rc = tpm_transmit_cmd(chip, NULL, &buf, 9 + 4 * nr_commands, 0, NULL);
 	if (rc) {
 		tpm_buf_destroy(&buf);
 		goto out;
@@ -969,7 +962,7 @@ static int tpm2_startup(struct tpm_chip *chip)
 		return rc;
 
 	tpm_buf_append_u16(&buf, TPM2_SU_CLEAR);
-	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
+	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0,
 			      "attempting to start the TPM");
 	tpm_buf_destroy(&buf);
 
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index dcdfde3c253e..1131a8e7b79b 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -83,7 +83,7 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
 	body_size = sizeof(*ctx) + be16_to_cpu(ctx->blob_size);
 	tpm_buf_append(&tbuf, &buf[*offset], body_size);
 
-	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
+	rc = tpm_transmit_cmd(chip, NULL, &tbuf, 4,
 			      TPM_TRANSMIT_NESTED, NULL);
 	if (rc < 0) {
 		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
@@ -132,7 +132,7 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
 
 	tpm_buf_append_u32(&tbuf, handle);
 
-	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 0,
+	rc = tpm_transmit_cmd(chip, NULL, &tbuf, 0,
 			      TPM_TRANSMIT_NESTED, NULL);
 	if (rc < 0) {
 		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index 87a0ce47f201..5f95fbfb7f6b 100644
--- a/drivers/char/tpm/tpm_vtpm_proxy.c
+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
@@ -417,8 +417,7 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
 
 	proxy_dev->state |= STATE_DRIVER_COMMAND;
 
-	rc = tpm_transmit_cmd(chip, NULL, buf.data, tpm_buf_length(&buf), 0,
-			      TPM_TRANSMIT_NESTED,
+	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, TPM_TRANSMIT_NESTED,
 			      "attempting to set locality");
 
 	proxy_dev->state &= ~STATE_DRIVER_COMMAND;
-- 
2.19.1


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

* [PATCH v11 02/16] tpm: fix invalid return value in pubek_show()
  2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
  2019-02-05 22:47 ` [PATCH v11 01/16] tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter Jarkko Sakkinen
@ 2019-02-05 22:47 ` Jarkko Sakkinen
  2019-02-05 22:47 ` [PATCH v11 03/16] tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails Jarkko Sakkinen
                   ` (14 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-05 22:47 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain, Jarkko Sakkinen

Return zero when tpm_buf_init() fails as we do for other functions in
tpm-sysfs.c.

Fixes: da379f3c1db0c ("tpm: migrate pubek_show to struct tpm_buf")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-sysfs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index c2769e55cb6c..7ed7eb6f906a 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -47,9 +47,8 @@ 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);
-	if (rc)
-		return rc;
+	if (tpm_buf_init(&tpm_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK))
+		return 0;
 
 	tpm_buf_append(&tpm_buf, anti_replay, sizeof(anti_replay));
 
-- 
2.19.1


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

* [PATCH v11 03/16] tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
  2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
  2019-02-05 22:47 ` [PATCH v11 01/16] tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter Jarkko Sakkinen
  2019-02-05 22:47 ` [PATCH v11 02/16] tpm: fix invalid return value in pubek_show() Jarkko Sakkinen
@ 2019-02-05 22:47 ` Jarkko Sakkinen
  2019-02-05 22:47 ` [PATCH v11 04/16] tpm: print tpm2_commit_space() error inside tpm2_commit_space() Jarkko Sakkinen
                   ` (13 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-05 22:47 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain, Jarkko Sakkinen

Do not print partial list of PCRs when tpm1_pcr_read() fails but instead
return 0 from pcrs_show(). This is consistent behavior with other sysfs
functions.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-sysfs.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index 7ed7eb6f906a..928d4e839bb7 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -100,22 +100,21 @@ static ssize_t pcrs_show(struct device *dev, struct device_attribute *attr,
 {
 	cap_t cap;
 	u8 digest[TPM_DIGEST_SIZE];
-	ssize_t rc;
 	u32 i, j, num_pcrs;
 	char *str = buf;
 	struct tpm_chip *chip = to_tpm_chip(dev);
 
-	rc = tpm1_getcap(chip, TPM_CAP_PROP_PCR, &cap,
-			 "attempting to determine the number of PCRS",
-			 sizeof(cap.num_pcrs));
-	if (rc)
+	if (tpm1_getcap(chip, TPM_CAP_PROP_PCR, &cap,
+			"attempting to determine the number of PCRS",
+			sizeof(cap.num_pcrs)))
 		return 0;
 
 	num_pcrs = be32_to_cpu(cap.num_pcrs);
 	for (i = 0; i < num_pcrs; i++) {
-		rc = tpm1_pcr_read(chip, i, digest);
-		if (rc)
+		if (tpm1_pcr_read(chip, i, digest)) {
+			str = buf;
 			break;
+		}
 		str += sprintf(str, "PCR-%02d: ", i);
 		for (j = 0; j < TPM_DIGEST_SIZE; j++)
 			str += sprintf(str, "%02X ", digest[j]);
-- 
2.19.1


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

* [PATCH v11 04/16] tpm: print tpm2_commit_space() error inside tpm2_commit_space()
  2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
                   ` (2 preceding siblings ...)
  2019-02-05 22:47 ` [PATCH v11 03/16] tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails Jarkko Sakkinen
@ 2019-02-05 22:47 ` Jarkko Sakkinen
  2019-02-05 22:47 ` [PATCH v11 05/16] tpm: declare struct tpm_header Jarkko Sakkinen
                   ` (12 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-05 22:47 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain, Jarkko Sakkinen, James Bottomley

The error logging for tpm2_commit_space() is in a wrong place. This
commit moves it inside that function.

Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-interface.c | 2 --
 drivers/char/tpm/tpm2-space.c    | 9 ++++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 5ef5c1e6947d..d594ea27810c 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -277,8 +277,6 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
 	}
 
 	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
-	if (rc)
-		dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc);
 
 out:
 	/* may fail but do not override previous error value in rc */
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 1131a8e7b79b..5ecc73988f7c 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -501,19 +501,19 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 	rc = tpm2_map_response_header(chip, cc, buf, *bufsiz);
 	if (rc) {
 		tpm2_flush_space(chip);
-		return rc;
+		goto out;
 	}
 
 	rc = tpm2_map_response_body(chip, cc, buf, *bufsiz);
 	if (rc) {
 		tpm2_flush_space(chip);
-		return rc;
+		goto out;
 	}
 
 	rc = tpm2_save_space(chip);
 	if (rc) {
 		tpm2_flush_space(chip);
-		return rc;
+		goto out;
 	}
 
 	*bufsiz = be32_to_cpu(header->length);
@@ -526,4 +526,7 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 	memcpy(space->session_buf, chip->work_space.session_buf, PAGE_SIZE);
 
 	return 0;
+out:
+	dev_err(&chip->dev, "%s: error %d\n", __func__, rc);
+	return rc;
 }
-- 
2.19.1


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

* [PATCH v11 05/16] tpm: declare struct tpm_header
  2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
                   ` (3 preceding siblings ...)
  2019-02-05 22:47 ` [PATCH v11 04/16] tpm: print tpm2_commit_space() error inside tpm2_commit_space() Jarkko Sakkinen
@ 2019-02-05 22:47 ` Jarkko Sakkinen
  2019-02-05 22:47 ` [PATCH v11 06/16] tpm: access command header through struct in tpm_try_transmit() Jarkko Sakkinen
                   ` (11 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-05 22:47 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain, Jarkko Sakkinen

Declare struct tpm_header that replaces struct tpm_input_header and
struct tpm_output_header.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-interface.c  | 21 ++++++++-------------
 drivers/char/tpm/tpm.h            | 29 +++++++++++++----------------
 drivers/char/tpm/tpm2-cmd.c       |  4 ++--
 drivers/char/tpm/tpm2-space.c     |  8 ++++----
 drivers/char/tpm/tpm_i2c_atmel.c  |  5 ++---
 drivers/char/tpm/tpm_vtpm_proxy.c |  8 ++++----
 drivers/char/tpm/xen-tpmfront.c   |  2 +-
 7 files changed, 34 insertions(+), 43 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index d594ea27810c..b5b6a7c3f995 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -62,12 +62,10 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
 }
 EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
 
-static int tpm_validate_command(struct tpm_chip *chip,
-				 struct tpm_space *space,
-				 const u8 *cmd,
-				 size_t len)
+static int tpm_validate_command(struct tpm_chip *chip, struct tpm_space *space,
+				const void *cmd, size_t len)
 {
-	const struct tpm_input_header *header = (const void *)cmd;
+	const struct tpm_header *header = cmd;
 	int i;
 	u32 cc;
 	u32 attrs;
@@ -161,12 +159,10 @@ static int tpm_go_idle(struct tpm_chip *chip, unsigned int flags)
 	return chip->ops->go_idle(chip);
 }
 
-static ssize_t tpm_try_transmit(struct tpm_chip *chip,
-				struct tpm_space *space,
-				u8 *buf, size_t bufsiz,
-				unsigned int flags)
+static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
+				void *buf, size_t bufsiz, unsigned int flags)
 {
-	struct tpm_output_header *header = (void *)buf;
+	struct tpm_header *header = buf;
 	int rc;
 	ssize_t len = 0;
 	u32 count, ordinal;
@@ -316,7 +312,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
 ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		     u8 *buf, size_t bufsiz, unsigned int flags)
 {
-	struct tpm_output_header *header = (struct tpm_output_header *)buf;
+	struct tpm_header *header = (struct tpm_header *)buf;
 	/* space for header and handles */
 	u8 save[TPM_HEADER_SIZE + 3*sizeof(u32)];
 	unsigned int delay_msec = TPM2_DURATION_SHORT;
@@ -381,8 +377,7 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
 			 struct tpm_buf *buf, size_t min_rsp_body_length,
 			 unsigned int flags, const char *desc)
 {
-	const struct tpm_output_header *header =
-		(struct tpm_output_header *)buf->data;
+	const struct tpm_header *header = (struct tpm_header *)buf->data;
 	int err;
 	ssize_t len;
 
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 49bca4d1e786..1454ef19d2f4 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -273,16 +273,13 @@ struct tpm_chip {
 
 #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
 
-struct tpm_input_header {
-	__be16	tag;
-	__be32	length;
-	__be32	ordinal;
-} __packed;
-
-struct tpm_output_header {
-	__be16	tag;
-	__be32	length;
-	__be32	return_code;
+struct tpm_header {
+	__be16 tag;
+	__be32 length;
+	union {
+		__be32 ordinal;
+		__be32 return_code;
+	};
 } __packed;
 
 #define TPM_TAG_RQU_COMMAND 193
@@ -401,8 +398,8 @@ struct tpm_buf {
 
 static inline void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
 {
-	struct tpm_input_header *head;
-	head = (struct tpm_input_header *)buf->data;
+	struct tpm_header *head = (struct tpm_header *)buf->data;
+
 	head->tag = cpu_to_be16(tag);
 	head->length = cpu_to_be32(sizeof(*head));
 	head->ordinal = cpu_to_be32(ordinal);
@@ -428,14 +425,14 @@ static inline void tpm_buf_destroy(struct tpm_buf *buf)
 
 static inline u32 tpm_buf_length(struct tpm_buf *buf)
 {
-	struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
+	struct tpm_header *head = (struct tpm_header *)buf->data;
 
 	return be32_to_cpu(head->length);
 }
 
 static inline u16 tpm_buf_tag(struct tpm_buf *buf)
 {
-	struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
+	struct tpm_header *head = (struct tpm_header *)buf->data;
 
 	return be16_to_cpu(head->tag);
 }
@@ -444,7 +441,7 @@ static inline void tpm_buf_append(struct tpm_buf *buf,
 				  const unsigned char *new_data,
 				  unsigned int new_len)
 {
-	struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
+	struct tpm_header *head = (struct tpm_header *)buf->data;
 	u32 len = tpm_buf_length(buf);
 
 	/* Return silently if overflow has already happened. */
@@ -582,7 +579,7 @@ void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space);
 int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
 		       u8 *cmd);
 int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
-		      u32 cc, u8 *buf, size_t *bufsiz);
+		      u32 cc, void *buf, size_t *bufsiz);
 
 int tpm_bios_log_setup(struct tpm_chip *chip);
 void tpm_bios_log_teardown(struct tpm_chip *chip);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 2bcf470c8e5d..ab03f8600f89 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -784,7 +784,7 @@ static int tpm2_do_selftest(struct tpm_chip *chip)
  */
 int tpm2_probe(struct tpm_chip *chip)
 {
-	struct tpm_output_header *out;
+	struct tpm_header *out;
 	struct tpm_buf buf;
 	int rc;
 
@@ -797,7 +797,7 @@ int tpm2_probe(struct tpm_chip *chip)
 	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0, NULL);
 	/* We ignore TPM return codes on purpose. */
 	if (rc >=  0) {
-		out = (struct tpm_output_header *)buf.data;
+		out = (struct tpm_header *)buf.data;
 		if (be16_to_cpu(out->tag) == TPM2_ST_NO_SESSIONS)
 			chip->flags |= TPM_CHIP_FLAG_TPM2;
 	}
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 5ecc73988f7c..39cb3915771e 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -334,7 +334,7 @@ static int tpm2_map_response_header(struct tpm_chip *chip, u32 cc, u8 *rsp,
 				    size_t len)
 {
 	struct tpm_space *space = &chip->work_space;
-	struct tpm_output_header *header = (void *)rsp;
+	struct tpm_header *header = (struct tpm_header *)rsp;
 	u32 phandle;
 	u32 phandle_type;
 	u32 vhandle;
@@ -394,7 +394,7 @@ static int tpm2_map_response_body(struct tpm_chip *chip, u32 cc, u8 *rsp,
 				  size_t len)
 {
 	struct tpm_space *space = &chip->work_space;
-	struct tpm_output_header *header = (void *)rsp;
+	struct tpm_header *header = (struct tpm_header *)rsp;
 	struct tpm2_cap_handles *data;
 	u32 phandle;
 	u32 phandle_type;
@@ -490,9 +490,9 @@ static int tpm2_save_space(struct tpm_chip *chip)
 }
 
 int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
-		      u32 cc, u8 *buf, size_t *bufsiz)
+		      u32 cc, void *buf, size_t *bufsiz)
 {
-	struct tpm_output_header *header = (void *)buf;
+	struct tpm_header *header = buf;
 	int rc;
 
 	if (!space)
diff --git a/drivers/char/tpm/tpm_i2c_atmel.c b/drivers/char/tpm/tpm_i2c_atmel.c
index 95ce2e9ccdc6..4720b2442ffe 100644
--- a/drivers/char/tpm/tpm_i2c_atmel.c
+++ b/drivers/char/tpm/tpm_i2c_atmel.c
@@ -46,7 +46,7 @@ struct priv_data {
 	/* This is the amount we read on the first try. 25 was chosen to fit a
 	 * fair number of read responses in the buffer so a 2nd retry can be
 	 * avoided in small message cases. */
-	u8 buffer[sizeof(struct tpm_output_header) + 25];
+	u8 buffer[sizeof(struct tpm_header) + 25];
 };
 
 static int i2c_atmel_send(struct tpm_chip *chip, u8 *buf, size_t len)
@@ -72,8 +72,7 @@ static int i2c_atmel_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 {
 	struct priv_data *priv = dev_get_drvdata(&chip->dev);
 	struct i2c_client *client = to_i2c_client(chip->dev.parent);
-	struct tpm_output_header *hdr =
-		(struct tpm_output_header *)priv->buffer;
+	struct tpm_header *hdr = (struct tpm_header *)priv->buffer;
 	u32 expected_len;
 	int rc;
 
diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index 5f95fbfb7f6b..28eb5ab15b99 100644
--- a/drivers/char/tpm/tpm_vtpm_proxy.c
+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
@@ -303,9 +303,9 @@ static int vtpm_proxy_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 static int vtpm_proxy_is_driver_command(struct tpm_chip *chip,
 					u8 *buf, size_t count)
 {
-	struct tpm_input_header *hdr = (struct tpm_input_header *)buf;
+	struct tpm_header *hdr = (struct tpm_header *)buf;
 
-	if (count < sizeof(struct tpm_input_header))
+	if (count < sizeof(struct tpm_header))
 		return 0;
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
@@ -402,7 +402,7 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
 {
 	struct tpm_buf buf;
 	int rc;
-	const struct tpm_output_header *header;
+	const struct tpm_header *header;
 	struct proxy_dev *proxy_dev = dev_get_drvdata(&chip->dev);
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
@@ -427,7 +427,7 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
 		goto out;
 	}
 
-	header = (const struct tpm_output_header *)buf.data;
+	header = (const struct tpm_header *)buf.data;
 	rc = be32_to_cpu(header->return_code);
 	if (rc)
 		locality = -1;
diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
index b150f87f38f5..1259e935fd58 100644
--- a/drivers/char/tpm/xen-tpmfront.c
+++ b/drivers/char/tpm/xen-tpmfront.c
@@ -163,7 +163,7 @@ static int vtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
 	wmb();
 	notify_remote_via_evtchn(priv->evtchn);
 
-	ordinal = be32_to_cpu(((struct tpm_input_header*)buf)->ordinal);
+	ordinal = be32_to_cpu(((struct tpm_header *)buf)->ordinal);
 	duration = tpm_calc_ordinal_duration(chip, ordinal);
 
 	if (wait_for_tpm_stat(chip, VTPM_STATUS_IDLE, duration,
-- 
2.19.1


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

* [PATCH v11 06/16] tpm: access command header through struct in tpm_try_transmit()
  2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
                   ` (4 preceding siblings ...)
  2019-02-05 22:47 ` [PATCH v11 05/16] tpm: declare struct tpm_header Jarkko Sakkinen
@ 2019-02-05 22:47 ` Jarkko Sakkinen
  2019-02-05 22:47 ` [PATCH v11 07/16] tpm: encapsulate tpm_dev_transmit() Jarkko Sakkinen
                   ` (10 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-05 22:47 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain, Jarkko Sakkinen

Instead of accessing fields of the command header through offsets to
the raw buffer, it is a better idea to use the header struct pointer
that is already used elsewhere in the function.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-interface.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index b5b6a7c3f995..c28ffef92f1a 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -187,8 +187,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	if (bufsiz > TPM_BUFSIZE)
 		bufsiz = TPM_BUFSIZE;
 
-	count = be32_to_cpu(*((__be32 *) (buf + 2)));
-	ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
+	count = be32_to_cpu(header->length);
+	ordinal = be32_to_cpu(header->ordinal);
 	if (count == 0)
 		return -ENODATA;
 	if (count > bufsiz) {
-- 
2.19.1


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

* [PATCH v11 07/16] tpm: encapsulate tpm_dev_transmit()
  2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
                   ` (5 preceding siblings ...)
  2019-02-05 22:47 ` [PATCH v11 06/16] tpm: access command header through struct in tpm_try_transmit() Jarkko Sakkinen
@ 2019-02-05 22:47 ` Jarkko Sakkinen
  2019-02-05 22:47 ` [PATCH v11 08/16] tpm: clean up tpm_try_transmit() error handling flow Jarkko Sakkinen
                   ` (9 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-05 22:47 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain, Jarkko Sakkinen

Encapsulate tpm_transmit() call pattern to tpm_dev_transmit() because it
is identically used from two places. Use unlocked version of
tpm_transmit() so that we are able to move the calls to
tpm2_prepare_space() and tpm2_commit_space() later on to this new
function.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-dev-common.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 5eecad233ea1..759796953d84 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -27,7 +27,19 @@
 static struct workqueue_struct *tpm_dev_wq;
 static DEFINE_MUTEX(tpm_dev_wq_lock);
 
-static void tpm_async_work(struct work_struct *work)
+static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,
+				u8 *buf, size_t bufsiz)
+{
+	ssize_t ret;
+
+	mutex_lock(&chip->tpm_mutex);
+	ret = tpm_transmit(chip, space, buf, bufsiz, TPM_TRANSMIT_UNLOCKED);
+	mutex_unlock(&chip->tpm_mutex);
+
+	return ret;
+}
+
+static void tpm_dev_async_work(struct work_struct *work)
 {
 	struct file_priv *priv =
 			container_of(work, struct file_priv, async_work);
@@ -35,9 +47,8 @@ static void tpm_async_work(struct work_struct *work)
 
 	mutex_lock(&priv->buffer_mutex);
 	priv->command_enqueued = false;
-	ret = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
-			   sizeof(priv->data_buffer), 0);
-
+	ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
+			       sizeof(priv->data_buffer));
 	tpm_put_ops(priv->chip);
 	if (ret > 0) {
 		priv->response_length = ret;
@@ -80,7 +91,7 @@ void tpm_common_open(struct file *file, struct tpm_chip *chip,
 	mutex_init(&priv->buffer_mutex);
 	timer_setup(&priv->user_read_timer, user_reader_timeout, 0);
 	INIT_WORK(&priv->timeout_work, tpm_timeout_work);
-	INIT_WORK(&priv->async_work, tpm_async_work);
+	INIT_WORK(&priv->async_work, tpm_dev_async_work);
 	init_waitqueue_head(&priv->async_wait);
 	file->private_data = priv;
 }
@@ -183,8 +194,8 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
 		return size;
 	}
 
-	ret = tpm_transmit(priv->chip, priv->space, priv->data_buffer,
-			   sizeof(priv->data_buffer), 0);
+	ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
+			       sizeof(priv->data_buffer));
 	tpm_put_ops(priv->chip);
 
 	if (ret > 0) {
-- 
2.19.1


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

* [PATCH v11 08/16] tpm: clean up tpm_try_transmit() error handling flow
  2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
                   ` (6 preceding siblings ...)
  2019-02-05 22:47 ` [PATCH v11 07/16] tpm: encapsulate tpm_dev_transmit() Jarkko Sakkinen
@ 2019-02-05 22:47 ` Jarkko Sakkinen
  2019-02-07 23:36   ` Stefan Berger
  2019-02-05 22:47 ` [PATCH v11 09/16] tpm: move tpm_validate_commmand() to tpm2-space.c Jarkko Sakkinen
                   ` (8 subsequent siblings)
  16 siblings, 1 reply; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-05 22:47 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain, Jarkko Sakkinen

Move locking, locality handling and power management to tpm_transmit()
in order to simplify the flow.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-interface.c | 94 +++++++++++++++-----------------
 drivers/char/tpm/tpm.h           |  1 +
 drivers/char/tpm/tpm2-space.c    |  2 +-
 3 files changed, 45 insertions(+), 52 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index c28ffef92f1a..f5f5224f68b0 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -167,7 +167,6 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	ssize_t len = 0;
 	u32 count, ordinal;
 	unsigned long stop;
-	bool need_locality;
 
 	rc = tpm_validate_command(chip, space, buf, bufsiz);
 	if (rc == -EINVAL)
@@ -197,37 +196,16 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		return -E2BIG;
 	}
 
-	if (!(flags & TPM_TRANSMIT_UNLOCKED) && !(flags & TPM_TRANSMIT_NESTED))
-		mutex_lock(&chip->tpm_mutex);
-
-	if (chip->ops->clk_enable != NULL)
-		chip->ops->clk_enable(chip, true);
-
-	/* Store the decision as chip->locality will be changed. */
-	need_locality = chip->locality == -1;
-
-	if (need_locality) {
-		rc = tpm_request_locality(chip, flags);
-		if (rc < 0) {
-			need_locality = false;
-			goto out_locality;
-		}
-	}
-
-	rc = tpm_cmd_ready(chip, flags);
-	if (rc)
-		goto out_locality;
-
 	rc = tpm2_prepare_space(chip, space, ordinal, buf);
 	if (rc)
-		goto out;
+		return rc;
 
 	rc = chip->ops->send(chip, buf, count);
 	if (rc < 0) {
 		if (rc != -EPIPE)
 			dev_err(&chip->dev,
 				"%s: tpm_send: error %d\n", __func__, rc);
-		goto out;
+		goto out_rc;
 	}
 
 	if (chip->flags & TPM_CHIP_FLAG_IRQ)
@@ -243,7 +221,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		if (chip->ops->req_canceled(chip, status)) {
 			dev_err(&chip->dev, "Operation Canceled\n");
 			rc = -ECANCELED;
-			goto out;
+			goto out_rc;
 		}
 
 		tpm_msleep(TPM_TIMEOUT_POLL);
@@ -253,40 +231,20 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	chip->ops->cancel(chip);
 	dev_err(&chip->dev, "Operation Timed out\n");
 	rc = -ETIME;
-	goto out;
+	goto out_rc;
 
 out_recv:
 	len = chip->ops->recv(chip, buf, bufsiz);
 	if (len < 0) {
 		rc = len;
-		dev_err(&chip->dev,
-			"tpm_transmit: tpm_recv: error %d\n", rc);
-		goto out;
-	} else if (len < TPM_HEADER_SIZE) {
+		dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
+	} else if (len < TPM_HEADER_SIZE || len != be32_to_cpu(header->length))
 		rc = -EFAULT;
-		goto out;
-	}
 
-	if (len != be32_to_cpu(header->length)) {
-		rc = -EFAULT;
-		goto out;
-	}
-
-	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
+out_rc:
+	if (!rc)
+		rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
 
-out:
-	/* may fail but do not override previous error value in rc */
-	tpm_go_idle(chip, flags);
-
-out_locality:
-	if (need_locality)
-		tpm_relinquish_locality(chip, flags);
-
-	if (chip->ops->clk_enable != NULL)
-		chip->ops->clk_enable(chip, false);
-
-	if (!(flags & TPM_TRANSMIT_UNLOCKED) && !(flags & TPM_TRANSMIT_NESTED))
-		mutex_unlock(&chip->tpm_mutex);
 	return rc ? rc : len;
 }
 
@@ -316,6 +274,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	/* space for header and handles */
 	u8 save[TPM_HEADER_SIZE + 3*sizeof(u32)];
 	unsigned int delay_msec = TPM2_DURATION_SHORT;
+	bool has_locality = false;
 	u32 rc = 0;
 	ssize_t ret;
 	const size_t save_size = min(space ? sizeof(save) : TPM_HEADER_SIZE,
@@ -331,7 +290,40 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	memcpy(save, buf, save_size);
 
 	for (;;) {
+		if (!(flags & TPM_TRANSMIT_UNLOCKED) &&
+		    !(flags & TPM_TRANSMIT_NESTED))
+			mutex_lock(&chip->tpm_mutex);
+
+		if (chip->ops->clk_enable != NULL)
+			chip->ops->clk_enable(chip, true);
+
+		if (chip->locality == -1) {
+			ret = tpm_request_locality(chip, flags);
+			if (ret)
+				goto out_locality;
+			has_locality = true;
+		}
+
+		ret = tpm_cmd_ready(chip, flags);
+		if (ret)
+			goto out_locality;
+
 		ret = tpm_try_transmit(chip, space, buf, bufsiz, flags);
+
+		/* This may fail but do not override ret. */
+		tpm_go_idle(chip, flags);
+
+out_locality:
+		if (has_locality)
+			tpm_relinquish_locality(chip, flags);
+
+		if (chip->ops->clk_enable != NULL)
+			chip->ops->clk_enable(chip, false);
+
+		if (!(flags & TPM_TRANSMIT_UNLOCKED) &&
+		    !(flags & TPM_TRANSMIT_NESTED))
+			mutex_unlock(&chip->tpm_mutex);
+
 		if (ret < 0)
 			break;
 		rc = be32_to_cpu(header->return_code);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 1454ef19d2f4..6eb67ccad2a3 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -576,6 +576,7 @@ int tpm2_probe(struct tpm_chip *chip);
 int tpm2_find_cc(struct tpm_chip *chip, u32 cc);
 int tpm2_init_space(struct tpm_space *space);
 void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space);
+void tpm2_flush_space(struct tpm_chip *chip);
 int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
 		       u8 *cmd);
 int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 39cb3915771e..5d6487575074 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -162,7 +162,7 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
 	return 0;
 }
 
-static void tpm2_flush_space(struct tpm_chip *chip)
+void tpm2_flush_space(struct tpm_chip *chip)
 {
 	struct tpm_space *space = &chip->work_space;
 	int i;
-- 
2.19.1


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

* [PATCH v11 09/16] tpm: move tpm_validate_commmand() to tpm2-space.c
  2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
                   ` (7 preceding siblings ...)
  2019-02-05 22:47 ` [PATCH v11 08/16] tpm: clean up tpm_try_transmit() error handling flow Jarkko Sakkinen
@ 2019-02-05 22:47 ` Jarkko Sakkinen
  2019-02-05 22:47 ` [PATCH v11 10/16] tpm: move TPM space code out of tpm_transmit() Jarkko Sakkinen
                   ` (7 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-05 22:47 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain, Jarkko Sakkinen, James Bottomley

Move tpm_validate_command() to tpm2-space.c and make it part of the
tpm2_prepare_space() flow. Make cc resolution as part of the TPM space
functionality in order to detach it from rest of the tpm_transmit()
flow.

Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-interface.c | 70 +++++++-------------------------
 drivers/char/tpm/tpm.h           |  9 ++--
 drivers/char/tpm/tpm2-space.c    | 52 +++++++++++++++++++++---
 3 files changed, 67 insertions(+), 64 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index f5f5224f68b0..669e702540e1 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -62,45 +62,6 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
 }
 EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
 
-static int tpm_validate_command(struct tpm_chip *chip, struct tpm_space *space,
-				const void *cmd, size_t len)
-{
-	const struct tpm_header *header = cmd;
-	int i;
-	u32 cc;
-	u32 attrs;
-	unsigned int nr_handles;
-
-	if (len < TPM_HEADER_SIZE)
-		return -EINVAL;
-
-	if (!space)
-		return 0;
-
-	if (chip->flags & TPM_CHIP_FLAG_TPM2 && chip->nr_commands) {
-		cc = be32_to_cpu(header->ordinal);
-
-		i = tpm2_find_cc(chip, cc);
-		if (i < 0) {
-			dev_dbg(&chip->dev, "0x%04X is an invalid command\n",
-				cc);
-			return -EOPNOTSUPP;
-		}
-
-		attrs = chip->cc_attrs_tbl[i];
-		nr_handles =
-			4 * ((attrs >> TPM2_CC_ATTR_CHANDLES) & GENMASK(2, 0));
-		if (len < TPM_HEADER_SIZE + 4 * nr_handles)
-			goto err_len;
-	}
-
-	return 0;
-err_len:
-	dev_dbg(&chip->dev,
-		"%s: insufficient command length %zu", __func__, len);
-	return -EINVAL;
-}
-
 static int tpm_request_locality(struct tpm_chip *chip, unsigned int flags)
 {
 	int rc;
@@ -168,20 +129,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	u32 count, ordinal;
 	unsigned long stop;
 
-	rc = tpm_validate_command(chip, space, buf, bufsiz);
-	if (rc == -EINVAL)
-		return rc;
-	/*
-	 * If the command is not implemented by the TPM, synthesize a
-	 * response with a TPM2_RC_COMMAND_CODE return for user-space.
-	 */
-	if (rc == -EOPNOTSUPP) {
-		header->length = cpu_to_be32(sizeof(*header));
-		header->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
-		header->return_code = cpu_to_be32(TPM2_RC_COMMAND_CODE |
-						  TSS2_RESMGR_TPM_RC_LAYER);
-		return sizeof(*header);
-	}
+	if (bufsiz < TPM_HEADER_SIZE)
+		return -EINVAL;
 
 	if (bufsiz > TPM_BUFSIZE)
 		bufsiz = TPM_BUFSIZE;
@@ -196,7 +145,18 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		return -E2BIG;
 	}
 
-	rc = tpm2_prepare_space(chip, space, ordinal, buf);
+	rc = tpm2_prepare_space(chip, space, buf, bufsiz);
+	/*
+	 * If the command is not implemented by the TPM, synthesize a
+	 * response with a TPM2_RC_COMMAND_CODE return for user-space.
+	 */
+	if (rc == -EOPNOTSUPP) {
+		header->length = cpu_to_be32(sizeof(*header));
+		header->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
+		header->return_code = cpu_to_be32(TPM2_RC_COMMAND_CODE |
+						  TSS2_RESMGR_TPM_RC_LAYER);
+		return sizeof(*header);
+	}
 	if (rc)
 		return rc;
 
@@ -243,7 +203,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
 
 out_rc:
 	if (!rc)
-		rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
+		rc = tpm2_commit_space(chip, space, buf, &len);
 
 	return rc ? rc : len;
 }
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 6eb67ccad2a3..e84333259e28 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -264,6 +264,7 @@ struct tpm_chip {
 #endif /* CONFIG_ACPI */
 
 	struct tpm_space work_space;
+	u32 last_cc;
 	u32 nr_commands;
 	u32 *cc_attrs_tbl;
 
@@ -577,10 +578,10 @@ int tpm2_find_cc(struct tpm_chip *chip, u32 cc);
 int tpm2_init_space(struct tpm_space *space);
 void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space);
 void tpm2_flush_space(struct tpm_chip *chip);
-int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
-		       u8 *cmd);
-int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
-		      u32 cc, void *buf, size_t *bufsiz);
+int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u8 *cmd,
+		       size_t cmdsiz);
+int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space, void *buf,
+		      size_t *bufsiz);
 
 int tpm_bios_log_setup(struct tpm_chip *chip);
 void tpm_bios_log_teardown(struct tpm_chip *chip);
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 5d6487575074..88f6d0df5c3d 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -264,14 +264,55 @@ static int tpm2_map_command(struct tpm_chip *chip, u32 cc, u8 *cmd)
 	return 0;
 }
 
-int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
-		       u8 *cmd)
+static int tpm_validate_command(struct tpm_chip *chip, struct tpm_space *space,
+				const void *cmd, size_t len)
+{
+	const struct tpm_header *header = (const void *)cmd;
+	int i;
+	u32 cc;
+	u32 attrs;
+	unsigned int nr_handles;
+
+	if (len < TPM_HEADER_SIZE)
+		return -EINVAL;
+
+	if (chip->nr_commands) {
+		cc = be32_to_cpu(header->ordinal);
+
+		i = tpm2_find_cc(chip, cc);
+		if (i < 0) {
+			dev_dbg(&chip->dev, "0x%04X is an invalid command\n",
+				cc);
+			return -EOPNOTSUPP;
+		}
+
+		attrs = chip->cc_attrs_tbl[i];
+		nr_handles =
+			4 * ((attrs >> TPM2_CC_ATTR_CHANDLES) & GENMASK(2, 0));
+		if (len < TPM_HEADER_SIZE + 4 * nr_handles)
+			goto err_len;
+	}
+
+	return cc;
+err_len:
+	dev_dbg(&chip->dev, "%s: insufficient command length %zu", __func__,
+		len);
+	return -EINVAL;
+}
+
+int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u8 *cmd,
+		       size_t cmdsiz)
 {
 	int rc;
+	int cc;
 
 	if (!space)
 		return 0;
 
+	cc = tpm_validate_command(chip, space, cmd, cmdsiz);
+	if (cc < 0)
+		return cc;
+
 	memcpy(&chip->work_space.context_tbl, &space->context_tbl,
 	       sizeof(space->context_tbl));
 	memcpy(&chip->work_space.session_tbl, &space->session_tbl,
@@ -291,6 +332,7 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
 		return rc;
 	}
 
+	chip->last_cc = cc;
 	return 0;
 }
 
@@ -490,7 +532,7 @@ static int tpm2_save_space(struct tpm_chip *chip)
 }
 
 int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
-		      u32 cc, void *buf, size_t *bufsiz)
+		      void *buf, size_t *bufsiz)
 {
 	struct tpm_header *header = buf;
 	int rc;
@@ -498,13 +540,13 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 	if (!space)
 		return 0;
 
-	rc = tpm2_map_response_header(chip, cc, buf, *bufsiz);
+	rc = tpm2_map_response_header(chip, chip->last_cc, buf, *bufsiz);
 	if (rc) {
 		tpm2_flush_space(chip);
 		goto out;
 	}
 
-	rc = tpm2_map_response_body(chip, cc, buf, *bufsiz);
+	rc = tpm2_map_response_body(chip, chip->last_cc, buf, *bufsiz);
 	if (rc) {
 		tpm2_flush_space(chip);
 		goto out;
-- 
2.19.1


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

* [PATCH v11 10/16] tpm: move TPM space code out of tpm_transmit()
  2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
                   ` (8 preceding siblings ...)
  2019-02-05 22:47 ` [PATCH v11 09/16] tpm: move tpm_validate_commmand() to tpm2-space.c Jarkko Sakkinen
@ 2019-02-05 22:47 ` Jarkko Sakkinen
  2019-02-05 22:47 ` [PATCH v11 11/16] tpm: remove @space from tpm_transmit() Jarkko Sakkinen
                   ` (6 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-05 22:47 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain, Jarkko Sakkinen, James Bottomley

Prepare and commit TPM space before and after calling tpm_transmit()
instead of doing that inside tpm_transmit(). After this change we can
remove TPM_TRANSMIT_NESTED flag from tpm2_prepare_space() and
tpm2_commit_space() and replace it with TPM_TRANSMIT_UNLOCKED.

Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-dev-common.c | 28 +++++++++++++++++++++++++---
 drivers/char/tpm/tpm-interface.c  | 27 +++------------------------
 drivers/char/tpm/tpm2-space.c     | 12 ++++++------
 3 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 759796953d84..327d1dca92c8 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -30,13 +30,35 @@ static DEFINE_MUTEX(tpm_dev_wq_lock);
 static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,
 				u8 *buf, size_t bufsiz)
 {
-	ssize_t ret;
+	struct tpm_header *header = (void *)buf;
+	ssize_t ret, len;
 
 	mutex_lock(&chip->tpm_mutex);
-	ret = tpm_transmit(chip, space, buf, bufsiz, TPM_TRANSMIT_UNLOCKED);
+	ret = tpm2_prepare_space(chip, space, buf, bufsiz);
+	/* If the command is not implemented by the TPM, synthesize a
+	 * response with a TPM2_RC_COMMAND_CODE return for user-space.
+	 */
+	if (ret == -EOPNOTSUPP) {
+		header->length = cpu_to_be32(sizeof(*header));
+		header->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
+		header->return_code = cpu_to_be32(TPM2_RC_COMMAND_CODE |
+						  TSS2_RESMGR_TPM_RC_LAYER);
+		ret = sizeof(*header);
+	}
+	if (ret)
+		goto out_lock;
+
+	len = tpm_transmit(chip, space, buf, bufsiz, TPM_TRANSMIT_UNLOCKED);
+	if (len < 0)
+		ret = len;
+
+	if (!ret)
+		ret = tpm2_commit_space(chip, space, buf, &len);
+
+out_lock:
 	mutex_unlock(&chip->tpm_mutex);
 
-	return ret;
+	return ret ? ret : len;
 }
 
 static void tpm_dev_async_work(struct work_struct *work)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 669e702540e1..cca0ee47d12d 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -145,27 +145,12 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		return -E2BIG;
 	}
 
-	rc = tpm2_prepare_space(chip, space, buf, bufsiz);
-	/*
-	 * If the command is not implemented by the TPM, synthesize a
-	 * response with a TPM2_RC_COMMAND_CODE return for user-space.
-	 */
-	if (rc == -EOPNOTSUPP) {
-		header->length = cpu_to_be32(sizeof(*header));
-		header->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
-		header->return_code = cpu_to_be32(TPM2_RC_COMMAND_CODE |
-						  TSS2_RESMGR_TPM_RC_LAYER);
-		return sizeof(*header);
-	}
-	if (rc)
-		return rc;
-
 	rc = chip->ops->send(chip, buf, count);
 	if (rc < 0) {
 		if (rc != -EPIPE)
 			dev_err(&chip->dev,
 				"%s: tpm_send: error %d\n", __func__, rc);
-		goto out_rc;
+		return rc;
 	}
 
 	if (chip->flags & TPM_CHIP_FLAG_IRQ)
@@ -180,8 +165,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
 
 		if (chip->ops->req_canceled(chip, status)) {
 			dev_err(&chip->dev, "Operation Canceled\n");
-			rc = -ECANCELED;
-			goto out_rc;
+			return -ECANCELED;
 		}
 
 		tpm_msleep(TPM_TIMEOUT_POLL);
@@ -190,8 +174,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
 
 	chip->ops->cancel(chip);
 	dev_err(&chip->dev, "Operation Timed out\n");
-	rc = -ETIME;
-	goto out_rc;
+	return -ETIME;
 
 out_recv:
 	len = chip->ops->recv(chip, buf, bufsiz);
@@ -201,10 +184,6 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	} else if (len < TPM_HEADER_SIZE || len != be32_to_cpu(header->length))
 		rc = -EFAULT;
 
-out_rc:
-	if (!rc)
-		rc = tpm2_commit_space(chip, space, buf, &len);
-
 	return rc ? rc : len;
 }
 
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 88f6d0df5c3d..8bddda2882ea 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -39,7 +39,7 @@ static void tpm2_flush_sessions(struct tpm_chip *chip, struct tpm_space *space)
 	for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
 		if (space->session_tbl[i])
 			tpm2_flush_context_cmd(chip, space->session_tbl[i],
-					       TPM_TRANSMIT_NESTED);
+					       TPM_TRANSMIT_UNLOCKED);
 	}
 }
 
@@ -84,7 +84,7 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
 	tpm_buf_append(&tbuf, &buf[*offset], body_size);
 
 	rc = tpm_transmit_cmd(chip, NULL, &tbuf, 4,
-			      TPM_TRANSMIT_NESTED, NULL);
+			      TPM_TRANSMIT_UNLOCKED, NULL);
 	if (rc < 0) {
 		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
 			 __func__, rc);
@@ -133,7 +133,7 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
 	tpm_buf_append_u32(&tbuf, handle);
 
 	rc = tpm_transmit_cmd(chip, NULL, &tbuf, 0,
-			      TPM_TRANSMIT_NESTED, NULL);
+			      TPM_TRANSMIT_UNLOCKED, NULL);
 	if (rc < 0) {
 		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
 			 __func__, rc);
@@ -170,7 +170,7 @@ void tpm2_flush_space(struct tpm_chip *chip)
 	for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
 		if (space->context_tbl[i] && ~space->context_tbl[i])
 			tpm2_flush_context_cmd(chip, space->context_tbl[i],
-					       TPM_TRANSMIT_NESTED);
+					       TPM_TRANSMIT_UNLOCKED);
 
 	tpm2_flush_sessions(chip, space);
 }
@@ -419,7 +419,7 @@ static int tpm2_map_response_header(struct tpm_chip *chip, u32 cc, u8 *rsp,
 
 	return 0;
 out_no_slots:
-	tpm2_flush_context_cmd(chip, phandle, TPM_TRANSMIT_NESTED);
+	tpm2_flush_context_cmd(chip, phandle, TPM_TRANSMIT_UNLOCKED);
 	dev_warn(&chip->dev, "%s: out of slots for 0x%08X\n", __func__,
 		 phandle);
 	return -ENOMEM;
@@ -507,7 +507,7 @@ static int tpm2_save_space(struct tpm_chip *chip)
 			return rc;
 
 		tpm2_flush_context_cmd(chip, space->context_tbl[i],
-				       TPM_TRANSMIT_NESTED);
+				       TPM_TRANSMIT_UNLOCKED);
 		space->context_tbl[i] = ~0;
 	}
 
-- 
2.19.1


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

* [PATCH v11 11/16] tpm: remove @space from tpm_transmit()
  2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
                   ` (9 preceding siblings ...)
  2019-02-05 22:47 ` [PATCH v11 10/16] tpm: move TPM space code out of tpm_transmit() Jarkko Sakkinen
@ 2019-02-05 22:47 ` Jarkko Sakkinen
  2019-02-05 22:47 ` [PATCH v11 12/16] tpm: use tpm_try_get_ops() in tpm-sysfs.c Jarkko Sakkinen
                   ` (5 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-05 22:47 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain, Jarkko Sakkinen, James Bottomley

Remove @space from tpm_transmit() API` in order to completely remove the
bound between low-level transmission functionality and TPM spaces. The
only real dependency existing is the amount of data saved before trying
to send a command to the TPM.

It doesn't really matter if we save always a bit more than needed so
this commit changes the amount saved always to be the size of the TPM
header and three handles.

Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-dev-common.c |  2 +-
 drivers/char/tpm/tpm-interface.c  | 25 +++++++++++--------------
 drivers/char/tpm/tpm-sysfs.c      |  5 ++---
 drivers/char/tpm/tpm.h            | 10 +++++-----
 drivers/char/tpm/tpm1-cmd.c       | 16 +++++++---------
 drivers/char/tpm/tpm2-cmd.c       | 30 ++++++++++++++----------------
 drivers/char/tpm/tpm2-space.c     |  6 ++----
 drivers/char/tpm/tpm_vtpm_proxy.c |  2 +-
 8 files changed, 43 insertions(+), 53 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 327d1dca92c8..95fe652b34ff 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -48,7 +48,7 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	if (ret)
 		goto out_lock;
 
-	len = tpm_transmit(chip, space, buf, bufsiz, TPM_TRANSMIT_UNLOCKED);
+	len = tpm_transmit(chip, buf, bufsiz, TPM_TRANSMIT_UNLOCKED);
 	if (len < 0)
 		ret = len;
 
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index cca0ee47d12d..b4c44b94e832 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -120,8 +120,8 @@ static int tpm_go_idle(struct tpm_chip *chip, unsigned int flags)
 	return chip->ops->go_idle(chip);
 }
 
-static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
-				void *buf, size_t bufsiz, unsigned int flags)
+static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz,
+				unsigned int flags)
 {
 	struct tpm_header *header = buf;
 	int rc;
@@ -190,7 +190,6 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
 /**
  * tpm_transmit - Internal kernel interface to transmit TPM commands.
  * @chip:	a TPM chip to use
- * @space:	a TPM space
  * @buf:	a TPM command buffer
  * @bufsiz:	length of the TPM command buffer
  * @flags:	TPM transmit flags
@@ -206,8 +205,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
  * * The response length	- OK
  * * -errno			- A system error
  */
-ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
-		     u8 *buf, size_t bufsiz, unsigned int flags)
+ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
+		     unsigned int flags)
 {
 	struct tpm_header *header = (struct tpm_header *)buf;
 	/* space for header and handles */
@@ -216,8 +215,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	bool has_locality = false;
 	u32 rc = 0;
 	ssize_t ret;
-	const size_t save_size = min(space ? sizeof(save) : TPM_HEADER_SIZE,
-				     bufsiz);
+	const size_t save_size = min(sizeof(save), bufsiz);
 	/* the command code is where the return code will be */
 	u32 cc = be32_to_cpu(header->return_code);
 
@@ -247,7 +245,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		if (ret)
 			goto out_locality;
 
-		ret = tpm_try_transmit(chip, space, buf, bufsiz, flags);
+		ret = tpm_try_transmit(chip, buf, bufsiz, flags);
 
 		/* This may fail but do not override ret. */
 		tpm_go_idle(chip, flags);
@@ -293,7 +291,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
 /**
  * tpm_transmit_cmd - send a tpm command to the device
  * @chip:			a TPM chip to use
- * @space:			a TPM space
  * @buf:			a TPM command buffer
  * @min_rsp_body_length:	minimum expected length of response body
  * @flags:			TPM transmit flags
@@ -304,15 +301,15 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
  * * -errno	- A system error
  * * TPM_RC	- A TPM error
  */
-ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
-			 struct tpm_buf *buf, size_t min_rsp_body_length,
-			 unsigned int flags, const char *desc)
+ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf,
+			 size_t min_rsp_body_length, unsigned int flags,
+			 const char *desc)
 {
 	const struct tpm_header *header = (struct tpm_header *)buf->data;
 	int err;
 	ssize_t len;
 
-	len = tpm_transmit(chip, space, buf->data, PAGE_SIZE, flags);
+	len = tpm_transmit(chip, buf->data, PAGE_SIZE, flags);
 	if (len <  0)
 		return len;
 
@@ -461,7 +458,7 @@ int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen)
 		goto out;
 
 	memcpy(buf.data, cmd, buflen);
-	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0,
+	rc = tpm_transmit_cmd(chip, &buf, 0, 0,
 			      "attempting to a send a command");
 	tpm_buf_destroy(&buf);
 out:
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index 928d4e839bb7..03e704f99ed6 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -52,9 +52,8 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
 
 	tpm_buf_append(&tpm_buf, anti_replay, sizeof(anti_replay));
 
-	rc = tpm_transmit_cmd(chip, NULL, &tpm_buf,
-			      READ_PUBEK_RESULT_MIN_BODY_SIZE, 0,
-			      "attempting to read the PUBEK");
+	rc = tpm_transmit_cmd(chip, &tpm_buf, READ_PUBEK_RESULT_MIN_BODY_SIZE,
+			      0, "attempting to read the PUBEK");
 	if (rc) {
 		tpm_buf_destroy(&tpm_buf);
 		return 0;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index e84333259e28..644f1a5c4fdd 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -498,11 +498,11 @@ enum tpm_transmit_flags {
 	TPM_TRANSMIT_NESTED      = BIT(1),
 };
 
-ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
-		     u8 *buf, size_t bufsiz, unsigned int flags);
-ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
-			 struct tpm_buf *buf, size_t min_rsp_body_length,
-			 unsigned int flags, const char *desc);
+ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
+		     unsigned int flags);
+ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf,
+			 size_t min_rsp_body_length, unsigned int flags,
+			 const char *desc);
 int tpm_get_timeouts(struct tpm_chip *);
 int tpm_auto_startup(struct tpm_chip *chip);
 
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 10a0b7683b4b..5b5f8bcc6210 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -334,8 +334,7 @@ static int tpm1_startup(struct tpm_chip *chip)
 
 	tpm_buf_append_u16(&buf, TPM_ST_CLEAR);
 
-	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0,
-			      "attempting to start the TPM");
+	rc = tpm_transmit_cmd(chip, &buf, 0, 0, "attempting to start the TPM");
 	tpm_buf_destroy(&buf);
 	return rc;
 }
@@ -459,7 +458,7 @@ int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
 	tpm_buf_append_u32(&buf, pcr_idx);
 	tpm_buf_append(&buf, hash, TPM_DIGEST_SIZE);
 
-	rc = tpm_transmit_cmd(chip, NULL, &buf, TPM_DIGEST_SIZE, 0, log_msg);
+	rc = tpm_transmit_cmd(chip, &buf, TPM_DIGEST_SIZE, 0, log_msg);
 	tpm_buf_destroy(&buf);
 	return rc;
 }
@@ -489,7 +488,7 @@ ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
 		tpm_buf_append_u32(&buf, 4);
 		tpm_buf_append_u32(&buf, subcap_id);
 	}
-	rc = tpm_transmit_cmd(chip, NULL, &buf, min_cap_length, 0, desc);
+	rc = tpm_transmit_cmd(chip, &buf, min_cap_length, 0, desc);
 	if (!rc)
 		*cap = *(cap_t *)&buf.data[TPM_HEADER_SIZE + 4];
 	tpm_buf_destroy(&buf);
@@ -530,8 +529,7 @@ int tpm1_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
 	do {
 		tpm_buf_append_u32(&buf, num_bytes);
 
-		rc = tpm_transmit_cmd(chip, NULL, &buf,
-				      sizeof(out->rng_data_len), 0,
+		rc = tpm_transmit_cmd(chip, &buf, sizeof(out->rng_data_len), 0,
 				      "attempting get random");
 		if (rc)
 			goto out;
@@ -576,7 +574,7 @@ int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf)
 
 	tpm_buf_append_u32(&buf, pcr_idx);
 
-	rc = tpm_transmit_cmd(chip, NULL, &buf, TPM_DIGEST_SIZE, 0,
+	rc = tpm_transmit_cmd(chip, &buf, TPM_DIGEST_SIZE, 0,
 			      "attempting to read a pcr value");
 	if (rc)
 		goto out;
@@ -610,7 +608,7 @@ static int tpm1_continue_selftest(struct tpm_chip *chip)
 	if (rc)
 		return rc;
 
-	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0, "continue selftest");
+	rc = tpm_transmit_cmd(chip, &buf, 0, 0, "continue selftest");
 	tpm_buf_destroy(&buf);
 	return rc;
 }
@@ -736,7 +734,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
 		return rc;
 	/* now do the actual savestate */
 	for (try = 0; try < TPM_RETRY; try++) {
-		rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0, NULL);
+		rc = tpm_transmit_cmd(chip, &buf, 0, 0, NULL);
 		/*
 		 * If the TPM indicates that it is too busy to respond to
 		 * this command then retry before giving up.  It can take
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index ab03f8600f89..f2b0e5c52a57 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -197,7 +197,7 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf)
 	tpm_buf_append(&buf, (const unsigned char *)pcr_select,
 		       sizeof(pcr_select));
 
-	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0, res_buf ?
+	rc = tpm_transmit_cmd(chip, &buf, 0, 0, res_buf ?
 			      "attempting to read a pcr value" : NULL);
 	if (rc == 0 && res_buf) {
 		out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
@@ -264,7 +264,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
 		}
 	}
 
-	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0,
+	rc = tpm_transmit_cmd(chip, &buf, 0, 0,
 			      "attempting extend a PCR value");
 
 	tpm_buf_destroy(&buf);
@@ -309,7 +309,7 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
 	do {
 		tpm_buf_reset(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_RANDOM);
 		tpm_buf_append_u16(&buf, num_bytes);
-		err = tpm_transmit_cmd(chip, NULL, &buf,
+		err = tpm_transmit_cmd(chip, &buf,
 				       offsetof(struct tpm2_get_random_out,
 						buffer),
 				       0, "attempting get random");
@@ -362,7 +362,7 @@ void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
 
 	tpm_buf_append_u32(&buf, handle);
 
-	tpm_transmit_cmd(chip, NULL, &buf, 0, flags, "flushing context");
+	tpm_transmit_cmd(chip, &buf, 0, flags, "flushing context");
 	tpm_buf_destroy(&buf);
 }
 
@@ -476,7 +476,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 		goto out;
 	}
 
-	rc = tpm_transmit_cmd(chip, NULL, &buf, 4, 0, "sealing data");
+	rc = tpm_transmit_cmd(chip, &buf, 4, 0, "sealing data");
 	if (rc)
 		goto out;
 
@@ -558,7 +558,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 		goto out;
 	}
 
-	rc = tpm_transmit_cmd(chip, NULL, &buf, 4, flags, "loading blob");
+	rc = tpm_transmit_cmd(chip, &buf, 4, flags, "loading blob");
 	if (!rc)
 		*blob_handle = be32_to_cpup(
 			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
@@ -608,7 +608,7 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
 			     options->blobauth /* hmac */,
 			     TPM_DIGEST_SIZE);
 
-	rc = tpm_transmit_cmd(chip, NULL, &buf, 6, flags, "unsealing");
+	rc = tpm_transmit_cmd(chip, &buf, 6, flags, "unsealing");
 	if (rc > 0)
 		rc = -EPERM;
 
@@ -698,7 +698,7 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,  u32 *value,
 	tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
 	tpm_buf_append_u32(&buf, property_id);
 	tpm_buf_append_u32(&buf, 1);
-	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0, NULL);
+	rc = tpm_transmit_cmd(chip, &buf, 0, 0, NULL);
 	if (!rc) {
 		out = (struct tpm2_get_cap_out *)
 			&buf.data[TPM_HEADER_SIZE];
@@ -728,7 +728,7 @@ void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type)
 	if (rc)
 		return;
 	tpm_buf_append_u16(&buf, shutdown_type);
-	tpm_transmit_cmd(chip, NULL, &buf, 0, 0, "stopping the TPM");
+	tpm_transmit_cmd(chip, &buf, 0, 0, "stopping the TPM");
 	tpm_buf_destroy(&buf);
 }
 
@@ -757,7 +757,7 @@ static int tpm2_do_selftest(struct tpm_chip *chip)
 			return rc;
 
 		tpm_buf_append_u8(&buf, full);
-		rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0,
+		rc = tpm_transmit_cmd(chip, &buf, 0, 0,
 				      "attempting the self test");
 		tpm_buf_destroy(&buf);
 
@@ -794,7 +794,7 @@ int tpm2_probe(struct tpm_chip *chip)
 	tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
 	tpm_buf_append_u32(&buf, TPM_PT_TOTAL_COMMANDS);
 	tpm_buf_append_u32(&buf, 1);
-	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0, NULL);
+	rc = tpm_transmit_cmd(chip, &buf, 0, 0, NULL);
 	/* We ignore TPM return codes on purpose. */
 	if (rc >=  0) {
 		out = (struct tpm_header *)buf.data;
@@ -833,8 +833,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	tpm_buf_append_u32(&buf, 0);
 	tpm_buf_append_u32(&buf, 1);
 
-	rc = tpm_transmit_cmd(chip, NULL, &buf, 9, 0,
-			      "get tpm pcr allocation");
+	rc = tpm_transmit_cmd(chip, &buf, 9, 0, "get tpm pcr allocation");
 	if (rc)
 		goto out;
 
@@ -905,7 +904,7 @@ static int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip)
 	tpm_buf_append_u32(&buf, TPM2_CC_FIRST);
 	tpm_buf_append_u32(&buf, nr_commands);
 
-	rc = tpm_transmit_cmd(chip, NULL, &buf, 9 + 4 * nr_commands, 0, NULL);
+	rc = tpm_transmit_cmd(chip, &buf, 9 + 4 * nr_commands, 0, NULL);
 	if (rc) {
 		tpm_buf_destroy(&buf);
 		goto out;
@@ -962,8 +961,7 @@ static int tpm2_startup(struct tpm_chip *chip)
 		return rc;
 
 	tpm_buf_append_u16(&buf, TPM2_SU_CLEAR);
-	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, 0,
-			      "attempting to start the TPM");
+	rc = tpm_transmit_cmd(chip, &buf, 0, 0, "attempting to start the TPM");
 	tpm_buf_destroy(&buf);
 
 	return rc;
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 8bddda2882ea..b0810a63df12 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -83,8 +83,7 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
 	body_size = sizeof(*ctx) + be16_to_cpu(ctx->blob_size);
 	tpm_buf_append(&tbuf, &buf[*offset], body_size);
 
-	rc = tpm_transmit_cmd(chip, NULL, &tbuf, 4,
-			      TPM_TRANSMIT_UNLOCKED, NULL);
+	rc = tpm_transmit_cmd(chip, &tbuf, 4, TPM_TRANSMIT_UNLOCKED, NULL);
 	if (rc < 0) {
 		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
 			 __func__, rc);
@@ -132,8 +131,7 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
 
 	tpm_buf_append_u32(&tbuf, handle);
 
-	rc = tpm_transmit_cmd(chip, NULL, &tbuf, 0,
-			      TPM_TRANSMIT_UNLOCKED, NULL);
+	rc = tpm_transmit_cmd(chip, &tbuf, 0, TPM_TRANSMIT_UNLOCKED, NULL);
 	if (rc < 0) {
 		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
 			 __func__, rc);
diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index 28eb5ab15b99..e8a1da2810a9 100644
--- a/drivers/char/tpm/tpm_vtpm_proxy.c
+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
@@ -417,7 +417,7 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
 
 	proxy_dev->state |= STATE_DRIVER_COMMAND;
 
-	rc = tpm_transmit_cmd(chip, NULL, &buf, 0, TPM_TRANSMIT_NESTED,
+	rc = tpm_transmit_cmd(chip, &buf, 0, TPM_TRANSMIT_NESTED,
 			      "attempting to set locality");
 
 	proxy_dev->state &= ~STATE_DRIVER_COMMAND;
-- 
2.19.1


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

* [PATCH v11 12/16] tpm: use tpm_try_get_ops() in tpm-sysfs.c.
  2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
                   ` (10 preceding siblings ...)
  2019-02-05 22:47 ` [PATCH v11 11/16] tpm: remove @space from tpm_transmit() Jarkko Sakkinen
@ 2019-02-05 22:47 ` Jarkko Sakkinen
  2019-02-05 22:47 ` [PATCH v11 13/16] tpm: remove TPM_TRANSMIT_UNLOCKED flag Jarkko Sakkinen
                   ` (4 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-05 22:47 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain, Jarkko Sakkinen

Use tpm_try_get_ops() in tpm-sysfs.c so that we can consider moving
other decorations (locking, localities, power management for example)
inside it. This direction can be of course taken only after other call
sites for tpm_transmit() have been treated in the same way.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-sysfs.c | 123 ++++++++++++++++++++++-------------
 1 file changed, 78 insertions(+), 45 deletions(-)

diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index 03e704f99ed6..3733491671ca 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -39,7 +39,6 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
 {
 	struct tpm_buf tpm_buf;
 	struct tpm_readpubek_out *out;
-	ssize_t rc;
 	int i;
 	char *str = buf;
 	struct tpm_chip *chip = to_tpm_chip(dev);
@@ -47,17 +46,17 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
 
 	memset(&anti_replay, 0, sizeof(anti_replay));
 
-	if (tpm_buf_init(&tpm_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK))
+	if (tpm_try_get_ops(chip))
 		return 0;
 
+	if (tpm_buf_init(&tpm_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK))
+		goto out_ops;
+
 	tpm_buf_append(&tpm_buf, anti_replay, sizeof(anti_replay));
 
-	rc = tpm_transmit_cmd(chip, &tpm_buf, READ_PUBEK_RESULT_MIN_BODY_SIZE,
-			      0, "attempting to read the PUBEK");
-	if (rc) {
-		tpm_buf_destroy(&tpm_buf);
-		return 0;
-	}
+	if (tpm_transmit_cmd(chip, &tpm_buf, READ_PUBEK_RESULT_MIN_BODY_SIZE,
+			      0, "attempting to read the PUBEK"))
+		goto out_buf;
 
 	out = (struct tpm_readpubek_out *)&tpm_buf.data[10];
 	str +=
@@ -88,9 +87,11 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
 			str += sprintf(str, "\n");
 	}
 
-	rc = str - buf;
+out_buf:
 	tpm_buf_destroy(&tpm_buf);
-	return rc;
+out_ops:
+	tpm_put_ops(chip);
+	return str - buf;
 }
 static DEVICE_ATTR_RO(pubek);
 
@@ -103,10 +104,15 @@ static ssize_t pcrs_show(struct device *dev, struct device_attribute *attr,
 	char *str = buf;
 	struct tpm_chip *chip = to_tpm_chip(dev);
 
+	if (tpm_try_get_ops(chip))
+		return 0;
+
 	if (tpm1_getcap(chip, TPM_CAP_PROP_PCR, &cap,
 			"attempting to determine the number of PCRS",
-			sizeof(cap.num_pcrs)))
+			sizeof(cap.num_pcrs))) {
+		tpm_put_ops(chip);
 		return 0;
+	}
 
 	num_pcrs = be32_to_cpu(cap.num_pcrs);
 	for (i = 0; i < num_pcrs; i++) {
@@ -119,6 +125,7 @@ static ssize_t pcrs_show(struct device *dev, struct device_attribute *attr,
 			str += sprintf(str, "%02X ", digest[j]);
 		str += sprintf(str, "\n");
 	}
+	tpm_put_ops(chip);
 	return str - buf;
 }
 static DEVICE_ATTR_RO(pcrs);
@@ -126,16 +133,21 @@ 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);
+	ssize_t rc = 0;
 	cap_t cap;
-	ssize_t rc;
 
-	rc = tpm1_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_PERM, &cap,
-			 "attempting to determine the permanent enabled state",
-			 sizeof(cap.perm_flags));
-	if (rc)
+	if (tpm_try_get_ops(chip))
 		return 0;
 
+	if (tpm1_getcap(chip, TPM_CAP_FLAG_PERM, &cap,
+			"attempting to determine the permanent enabled state",
+			sizeof(cap.perm_flags)))
+		goto out_ops;
+
 	rc = sprintf(buf, "%d\n", !cap.perm_flags.disable);
+out_ops:
+	tpm_put_ops(chip);
 	return rc;
 }
 static DEVICE_ATTR_RO(enabled);
@@ -143,16 +155,21 @@ 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);
+	ssize_t rc = 0;
 	cap_t cap;
-	ssize_t rc;
 
-	rc = tpm1_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_PERM, &cap,
-			 "attempting to determine the permanent active state",
-			 sizeof(cap.perm_flags));
-	if (rc)
+	if (tpm_try_get_ops(chip))
 		return 0;
 
+	if (tpm1_getcap(chip, TPM_CAP_FLAG_PERM, &cap,
+			"attempting to determine the permanent active state",
+			sizeof(cap.perm_flags)))
+		goto out_ops;
+
 	rc = sprintf(buf, "%d\n", !cap.perm_flags.deactivated);
+out_ops:
+	tpm_put_ops(chip);
 	return rc;
 }
 static DEVICE_ATTR_RO(active);
@@ -160,16 +177,21 @@ 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);
+	ssize_t rc = 0;
 	cap_t cap;
-	ssize_t rc;
 
-	rc = tpm1_getcap(to_tpm_chip(dev), TPM_CAP_PROP_OWNER, &cap,
-			 "attempting to determine the owner state",
-			 sizeof(cap.owned));
-	if (rc)
+	if (tpm_try_get_ops(chip))
 		return 0;
 
+	if (tpm1_getcap(to_tpm_chip(dev), TPM_CAP_PROP_OWNER, &cap,
+			"attempting to determine the owner state",
+			sizeof(cap.owned)))
+		goto out_ops;
+
 	rc = sprintf(buf, "%d\n", cap.owned);
+out_ops:
+	tpm_put_ops(chip);
 	return rc;
 }
 static DEVICE_ATTR_RO(owned);
@@ -177,16 +199,21 @@ 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);
+	ssize_t rc = 0;
 	cap_t cap;
-	ssize_t rc;
 
-	rc = tpm1_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_VOL, &cap,
-			 "attempting to determine the temporary state",
-			 sizeof(cap.stclear_flags));
-	if (rc)
+	if (tpm_try_get_ops(chip))
 		return 0;
 
+	if (tpm1_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_VOL, &cap,
+			"attempting to determine the temporary state",
+			sizeof(cap.stclear_flags)))
+		goto out_ops;
+
 	rc = sprintf(buf, "%d\n", cap.stclear_flags.deactivated);
+out_ops:
+	tpm_put_ops(chip);
 	return rc;
 }
 static DEVICE_ATTR_RO(temp_deactivated);
@@ -195,15 +222,18 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
 	struct tpm_chip *chip = to_tpm_chip(dev);
-	cap_t cap;
-	ssize_t rc;
+	ssize_t rc = 0;
 	char *str = buf;
+	cap_t cap;
 
-	rc = tpm1_getcap(chip, TPM_CAP_PROP_MANUFACTURER, &cap,
-			 "attempting to determine the manufacturer",
-			 sizeof(cap.manufacturer_id));
-	if (rc)
+	if (tpm_try_get_ops(chip))
 		return 0;
+
+	if (tpm1_getcap(chip, TPM_CAP_PROP_MANUFACTURER, &cap,
+			"attempting to determine the manufacturer",
+			sizeof(cap.manufacturer_id)))
+		goto out_ops;
+
 	str += sprintf(str, "Manufacturer: 0x%x\n",
 		       be32_to_cpu(cap.manufacturer_id));
 
@@ -220,11 +250,10 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr,
 			       cap.tpm_version_1_2.revMinor);
 	} else {
 		/* Otherwise just use TPM_STRUCT_VER */
-		rc = tpm1_getcap(chip, TPM_CAP_VERSION_1_1, &cap,
-				 "attempting to determine the 1.1 version",
-				 sizeof(cap.tpm_version));
-		if (rc)
-			return 0;
+		if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, &cap,
+				"attempting to determine the 1.1 version",
+				sizeof(cap.tpm_version)))
+			goto out_ops;
 		str += sprintf(str,
 			       "TCG version: %d.%d\nFirmware version: %d.%d\n",
 			       cap.tpm_version.Major,
@@ -232,8 +261,10 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr,
 			       cap.tpm_version.revMajor,
 			       cap.tpm_version.revMinor);
 	}
-
-	return str - buf;
+	rc = str - buf;
+out_ops:
+	tpm_put_ops(chip);
+	return rc;
 }
 static DEVICE_ATTR_RO(caps);
 
@@ -241,10 +272,12 @@ 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);
-	if (chip == NULL)
+
+	if (tpm_try_get_ops(chip))
 		return 0;
 
 	chip->ops->cancel(chip);
+	tpm_put_ops(chip);
 	return count;
 }
 static DEVICE_ATTR_WO(cancel);
-- 
2.19.1


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

* [PATCH v11 13/16] tpm: remove TPM_TRANSMIT_UNLOCKED flag
  2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
                   ` (11 preceding siblings ...)
  2019-02-05 22:47 ` [PATCH v11 12/16] tpm: use tpm_try_get_ops() in tpm-sysfs.c Jarkko Sakkinen
@ 2019-02-05 22:47 ` Jarkko Sakkinen
  2019-02-05 22:47 ` [PATCH v11 14/16] tpm: introduce tpm_chip_start() and tpm_chip_stop() Jarkko Sakkinen
                   ` (3 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-05 22:47 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain, Jarkko Sakkinen

Added locking as part of tpm_try_get_ops() and tpm_put_ops() as they are
anyway used in most of the call sites except in tpmrm_release() where we
take the locks manually.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-chip.c       |  2 ++
 drivers/char/tpm/tpm-dev-common.c |  9 +++------
 drivers/char/tpm/tpm-interface.c  |  8 --------
 drivers/char/tpm/tpm.h            |  8 ++------
 drivers/char/tpm/tpm2-cmd.c       | 13 ++++---------
 drivers/char/tpm/tpm2-space.c     | 15 ++++++---------
 6 files changed, 17 insertions(+), 38 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 32db84683c40..157505b0f755 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -58,6 +58,7 @@ int tpm_try_get_ops(struct tpm_chip *chip)
 	if (!chip->ops)
 		goto out_lock;
 
+	mutex_lock(&chip->tpm_mutex);
 	return 0;
 out_lock:
 	up_read(&chip->ops_sem);
@@ -75,6 +76,7 @@ EXPORT_SYMBOL_GPL(tpm_try_get_ops);
  */
 void tpm_put_ops(struct tpm_chip *chip)
 {
+	mutex_unlock(&chip->tpm_mutex);
 	up_read(&chip->ops_sem);
 	put_device(&chip->dev);
 }
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 95fe652b34ff..435c09ec9056 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -33,7 +33,6 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	struct tpm_header *header = (void *)buf;
 	ssize_t ret, len;
 
-	mutex_lock(&chip->tpm_mutex);
 	ret = tpm2_prepare_space(chip, space, buf, bufsiz);
 	/* If the command is not implemented by the TPM, synthesize a
 	 * response with a TPM2_RC_COMMAND_CODE return for user-space.
@@ -46,18 +45,16 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		ret = sizeof(*header);
 	}
 	if (ret)
-		goto out_lock;
+		goto out_rc;
 
-	len = tpm_transmit(chip, buf, bufsiz, TPM_TRANSMIT_UNLOCKED);
+	len = tpm_transmit(chip, buf, bufsiz, 0);
 	if (len < 0)
 		ret = len;
 
 	if (!ret)
 		ret = tpm2_commit_space(chip, space, buf, &len);
 
-out_lock:
-	mutex_unlock(&chip->tpm_mutex);
-
+out_rc:
 	return ret ? ret : len;
 }
 
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index b4c44b94e832..4ac02990bdb2 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -227,10 +227,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
 	memcpy(save, buf, save_size);
 
 	for (;;) {
-		if (!(flags & TPM_TRANSMIT_UNLOCKED) &&
-		    !(flags & TPM_TRANSMIT_NESTED))
-			mutex_lock(&chip->tpm_mutex);
-
 		if (chip->ops->clk_enable != NULL)
 			chip->ops->clk_enable(chip, true);
 
@@ -257,10 +253,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
 		if (chip->ops->clk_enable != NULL)
 			chip->ops->clk_enable(chip, false);
 
-		if (!(flags & TPM_TRANSMIT_UNLOCKED) &&
-		    !(flags & TPM_TRANSMIT_NESTED))
-			mutex_unlock(&chip->tpm_mutex);
-
 		if (ret < 0)
 			break;
 		rc = be32_to_cpu(header->return_code);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 644f1a5c4fdd..18ef432a3fde 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -488,14 +488,10 @@ extern struct idr dev_nums_idr;
 /**
  * enum tpm_transmit_flags - flags for tpm_transmit()
  *
- * @TPM_TRANSMIT_UNLOCKED:	do not lock the chip
- * @TPM_TRANSMIT_NESTED:	discard setup steps (power management,
- *				locality) including locking (i.e. implicit
- *				UNLOCKED)
+ * %TPM_TRANSMIT_NESTED:	discard setup steps (power management, locality)
  */
 enum tpm_transmit_flags {
-	TPM_TRANSMIT_UNLOCKED	= BIT(0),
-	TPM_TRANSMIT_NESTED      = BIT(1),
+	TPM_TRANSMIT_NESTED      = BIT(0),
 };
 
 ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index f2b0e5c52a57..d6abc964ef66 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -652,17 +652,12 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
 	u32 blob_handle;
 	int rc;
 
-	mutex_lock(&chip->tpm_mutex);
-	rc = tpm2_load_cmd(chip, payload, options, &blob_handle,
-			   TPM_TRANSMIT_UNLOCKED);
+	rc = tpm2_load_cmd(chip, payload, options, &blob_handle, 0);
 	if (rc)
-		goto out;
+		return rc;
 
-	rc = tpm2_unseal_cmd(chip, payload, options, blob_handle,
-			     TPM_TRANSMIT_UNLOCKED);
-	tpm2_flush_context_cmd(chip, blob_handle, TPM_TRANSMIT_UNLOCKED);
-out:
-	mutex_unlock(&chip->tpm_mutex);
+	rc = tpm2_unseal_cmd(chip, payload, options, blob_handle, 0);
+	tpm2_flush_context_cmd(chip, blob_handle, 0);
 	return rc;
 }
 
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index b0810a63df12..9c9ccf2c0681 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -38,8 +38,7 @@ static void tpm2_flush_sessions(struct tpm_chip *chip, struct tpm_space *space)
 
 	for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
 		if (space->session_tbl[i])
-			tpm2_flush_context_cmd(chip, space->session_tbl[i],
-					       TPM_TRANSMIT_UNLOCKED);
+			tpm2_flush_context_cmd(chip, space->session_tbl[i], 0);
 	}
 }
 
@@ -83,7 +82,7 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
 	body_size = sizeof(*ctx) + be16_to_cpu(ctx->blob_size);
 	tpm_buf_append(&tbuf, &buf[*offset], body_size);
 
-	rc = tpm_transmit_cmd(chip, &tbuf, 4, TPM_TRANSMIT_UNLOCKED, NULL);
+	rc = tpm_transmit_cmd(chip, &tbuf, 4, 0, NULL);
 	if (rc < 0) {
 		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
 			 __func__, rc);
@@ -131,7 +130,7 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
 
 	tpm_buf_append_u32(&tbuf, handle);
 
-	rc = tpm_transmit_cmd(chip, &tbuf, 0, TPM_TRANSMIT_UNLOCKED, NULL);
+	rc = tpm_transmit_cmd(chip, &tbuf, 0, 0, NULL);
 	if (rc < 0) {
 		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
 			 __func__, rc);
@@ -167,8 +166,7 @@ void tpm2_flush_space(struct tpm_chip *chip)
 
 	for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
 		if (space->context_tbl[i] && ~space->context_tbl[i])
-			tpm2_flush_context_cmd(chip, space->context_tbl[i],
-					       TPM_TRANSMIT_UNLOCKED);
+			tpm2_flush_context_cmd(chip, space->context_tbl[i], 0);
 
 	tpm2_flush_sessions(chip, space);
 }
@@ -417,7 +415,7 @@ static int tpm2_map_response_header(struct tpm_chip *chip, u32 cc, u8 *rsp,
 
 	return 0;
 out_no_slots:
-	tpm2_flush_context_cmd(chip, phandle, TPM_TRANSMIT_UNLOCKED);
+	tpm2_flush_context_cmd(chip, phandle, 0);
 	dev_warn(&chip->dev, "%s: out of slots for 0x%08X\n", __func__,
 		 phandle);
 	return -ENOMEM;
@@ -504,8 +502,7 @@ static int tpm2_save_space(struct tpm_chip *chip)
 		} else if (rc)
 			return rc;
 
-		tpm2_flush_context_cmd(chip, space->context_tbl[i],
-				       TPM_TRANSMIT_UNLOCKED);
+		tpm2_flush_context_cmd(chip, space->context_tbl[i], 0);
 		space->context_tbl[i] = ~0;
 	}
 
-- 
2.19.1


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

* [PATCH v11 14/16] tpm: introduce tpm_chip_start() and tpm_chip_stop()
  2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
                   ` (12 preceding siblings ...)
  2019-02-05 22:47 ` [PATCH v11 13/16] tpm: remove TPM_TRANSMIT_UNLOCKED flag Jarkko Sakkinen
@ 2019-02-05 22:47 ` Jarkko Sakkinen
  2019-02-05 22:47 ` [PATCH v11 15/16] tpm: take TPM chip power gating out of tpm_transmit() Jarkko Sakkinen
                   ` (2 subsequent siblings)
  16 siblings, 0 replies; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-05 22:47 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain, Jarkko Sakkinen

Encapsulate power gating and locality functionality to tpm_chip_start()
and tpm_chip_stop() in order to clean up the branching mess in
tpm_transmit().

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-chip.c      | 110 +++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm-interface.c |  84 +----------------------
 drivers/char/tpm/tpm.h           |   2 +
 3 files changed, 115 insertions(+), 81 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 157505b0f755..65f1561eba81 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -37,6 +37,116 @@ struct class *tpm_class;
 struct class *tpmrm_class;
 dev_t tpm_devt;
 
+static int tpm_request_locality(struct tpm_chip *chip, unsigned int flags)
+{
+	int rc;
+
+	if (flags & TPM_TRANSMIT_NESTED)
+		return 0;
+
+	if (!chip->ops->request_locality)
+		return 0;
+
+	rc = chip->ops->request_locality(chip, 0);
+	if (rc < 0)
+		return rc;
+
+	chip->locality = rc;
+	return 0;
+}
+
+static void tpm_relinquish_locality(struct tpm_chip *chip, unsigned int flags)
+{
+	int rc;
+
+	if (flags & TPM_TRANSMIT_NESTED)
+		return;
+
+	if (!chip->ops->relinquish_locality)
+		return;
+
+	rc = chip->ops->relinquish_locality(chip, chip->locality);
+	if (rc)
+		dev_err(&chip->dev, "%s: : error %d\n", __func__, rc);
+
+	chip->locality = -1;
+}
+
+static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags)
+{
+	if (flags & TPM_TRANSMIT_NESTED)
+		return 0;
+
+	if (!chip->ops->cmd_ready)
+		return 0;
+
+	return chip->ops->cmd_ready(chip);
+}
+
+static int tpm_go_idle(struct tpm_chip *chip, unsigned int flags)
+{
+	if (flags & TPM_TRANSMIT_NESTED)
+		return 0;
+
+	if (!chip->ops->go_idle)
+		return 0;
+
+	return chip->ops->go_idle(chip);
+}
+
+/**
+ * tpm_chip_start() - power on the TPM
+ * @chip:	a TPM chip to use
+ * @flags:	TPM transmit flags
+ *
+ * Return:
+ * * The response length	- OK
+ * * -errno			- A system error
+ */
+int tpm_chip_start(struct tpm_chip *chip, unsigned int flags)
+{
+	int ret;
+
+	if (chip->ops->clk_enable)
+		chip->ops->clk_enable(chip, true);
+
+	if (chip->locality == -1) {
+		ret = tpm_request_locality(chip, flags);
+		if (ret) {
+			chip->ops->clk_enable(chip, false);
+			return ret;
+		}
+	}
+
+	ret = tpm_cmd_ready(chip, flags);
+	if (ret) {
+		tpm_relinquish_locality(chip, flags);
+		if (chip->ops->clk_enable)
+			chip->ops->clk_enable(chip, false);
+		return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * tpm_chip_stop() - power off the TPM
+ * @chip:	a TPM chip to use
+ * @flags:	TPM transmit flags
+ *
+ * Return:
+ * * The response length	- OK
+ * * -errno			- A system error
+ */
+void tpm_chip_stop(struct tpm_chip *chip, unsigned int flags)
+{
+	tpm_go_idle(chip, flags);
+	tpm_relinquish_locality(chip, flags);
+	if (chip->ops->clk_enable)
+		chip->ops->clk_enable(chip, false);
+}
+
+
 /**
  * tpm_try_get_ops() - Get a ref to the tpm_chip
  * @chip: Chip to ref
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 4ac02990bdb2..f7b7e4e75fcf 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -62,64 +62,6 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
 }
 EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
 
-static int tpm_request_locality(struct tpm_chip *chip, unsigned int flags)
-{
-	int rc;
-
-	if (flags & TPM_TRANSMIT_NESTED)
-		return 0;
-
-	if (!chip->ops->request_locality)
-		return 0;
-
-	rc = chip->ops->request_locality(chip, 0);
-	if (rc < 0)
-		return rc;
-
-	chip->locality = rc;
-
-	return 0;
-}
-
-static void tpm_relinquish_locality(struct tpm_chip *chip, unsigned int flags)
-{
-	int rc;
-
-	if (flags & TPM_TRANSMIT_NESTED)
-		return;
-
-	if (!chip->ops->relinquish_locality)
-		return;
-
-	rc = chip->ops->relinquish_locality(chip, chip->locality);
-	if (rc)
-		dev_err(&chip->dev, "%s: : error %d\n", __func__, rc);
-
-	chip->locality = -1;
-}
-
-static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags)
-{
-	if (flags & TPM_TRANSMIT_NESTED)
-		return 0;
-
-	if (!chip->ops->cmd_ready)
-		return 0;
-
-	return chip->ops->cmd_ready(chip);
-}
-
-static int tpm_go_idle(struct tpm_chip *chip, unsigned int flags)
-{
-	if (flags & TPM_TRANSMIT_NESTED)
-		return 0;
-
-	if (!chip->ops->go_idle)
-		return 0;
-
-	return chip->ops->go_idle(chip);
-}
-
 static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz,
 				unsigned int flags)
 {
@@ -212,7 +154,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
 	/* space for header and handles */
 	u8 save[TPM_HEADER_SIZE + 3*sizeof(u32)];
 	unsigned int delay_msec = TPM2_DURATION_SHORT;
-	bool has_locality = false;
 	u32 rc = 0;
 	ssize_t ret;
 	const size_t save_size = min(sizeof(save), bufsiz);
@@ -227,32 +168,13 @@ ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
 	memcpy(save, buf, save_size);
 
 	for (;;) {
-		if (chip->ops->clk_enable != NULL)
-			chip->ops->clk_enable(chip, true);
-
-		if (chip->locality == -1) {
-			ret = tpm_request_locality(chip, flags);
-			if (ret)
-				goto out_locality;
-			has_locality = true;
-		}
-
-		ret = tpm_cmd_ready(chip, flags);
+		ret = tpm_chip_start(chip, flags);
 		if (ret)
-			goto out_locality;
+			return ret;
 
 		ret = tpm_try_transmit(chip, buf, bufsiz, flags);
 
-		/* This may fail but do not override ret. */
-		tpm_go_idle(chip, flags);
-
-out_locality:
-		if (has_locality)
-			tpm_relinquish_locality(chip, flags);
-
-		if (chip->ops->clk_enable != NULL)
-			chip->ops->clk_enable(chip, false);
-
+		tpm_chip_stop(chip, flags);
 		if (ret < 0)
 			break;
 		rc = be32_to_cpu(header->return_code);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 18ef432a3fde..2d6d934f1c8b 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -523,6 +523,8 @@ static inline void tpm_msleep(unsigned int delay_msec)
 		     delay_msec * 1000);
 };
 
+int tpm_chip_start(struct tpm_chip *chip, unsigned int flags);
+void tpm_chip_stop(struct tpm_chip *chip, unsigned int flags);
 struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
 __must_check int tpm_try_get_ops(struct tpm_chip *chip);
 void tpm_put_ops(struct tpm_chip *chip);
-- 
2.19.1


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

* [PATCH v11 15/16] tpm: take TPM chip power gating out of tpm_transmit()
  2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
                   ` (13 preceding siblings ...)
  2019-02-05 22:47 ` [PATCH v11 14/16] tpm: introduce tpm_chip_start() and tpm_chip_stop() Jarkko Sakkinen
@ 2019-02-05 22:47 ` Jarkko Sakkinen
  2019-02-07 23:32   ` Stefan Berger
  2019-02-05 22:47 ` [PATCH v11 16/16] tpm: remove @flags from tpm_transmit() Jarkko Sakkinen
  2019-02-06 12:06 ` [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
  16 siblings, 1 reply; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-05 22:47 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain, Jarkko Sakkinen

Call tpm_chip_start() and tpm_chip_stop() in

* tpm_try_get_ops() and tpm_put_ops()
* tpm_chip_register()
* tpm2_del_space()

And remove these calls from tpm_transmit(). The core reason for this
change is that in tpm_vtpm_proxy a locality change requires a virtual
TPM command (a command made up just for that driver).

The consequence of this is that this commit removes the remaining nested
calls.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-chip.c       | 25 ++++++++++++-------------
 drivers/char/tpm/tpm-interface.c  |  6 ------
 drivers/char/tpm/tpm.h            |  9 ---------
 drivers/char/tpm/tpm2-space.c     |  5 ++++-
 drivers/char/tpm/tpm_vtpm_proxy.c |  3 +--
 5 files changed, 17 insertions(+), 31 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 65f1561eba81..7ad4d9045e4c 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -41,9 +41,6 @@ static int tpm_request_locality(struct tpm_chip *chip, unsigned int flags)
 {
 	int rc;
 
-	if (flags & TPM_TRANSMIT_NESTED)
-		return 0;
-
 	if (!chip->ops->request_locality)
 		return 0;
 
@@ -59,9 +56,6 @@ static void tpm_relinquish_locality(struct tpm_chip *chip, unsigned int flags)
 {
 	int rc;
 
-	if (flags & TPM_TRANSMIT_NESTED)
-		return;
-
 	if (!chip->ops->relinquish_locality)
 		return;
 
@@ -74,9 +68,6 @@ static void tpm_relinquish_locality(struct tpm_chip *chip, unsigned int flags)
 
 static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags)
 {
-	if (flags & TPM_TRANSMIT_NESTED)
-		return 0;
-
 	if (!chip->ops->cmd_ready)
 		return 0;
 
@@ -85,9 +76,6 @@ static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags)
 
 static int tpm_go_idle(struct tpm_chip *chip, unsigned int flags)
 {
-	if (flags & TPM_TRANSMIT_NESTED)
-		return 0;
-
 	if (!chip->ops->go_idle)
 		return 0;
 
@@ -166,11 +154,17 @@ int tpm_try_get_ops(struct tpm_chip *chip)
 
 	down_read(&chip->ops_sem);
 	if (!chip->ops)
-		goto out_lock;
+		goto out_ops;
 
 	mutex_lock(&chip->tpm_mutex);
+	rc = tpm_chip_start(chip, 0);
+	if (rc)
+		goto out_lock;
+
 	return 0;
 out_lock:
+	mutex_unlock(&chip->tpm_mutex);
+out_ops:
 	up_read(&chip->ops_sem);
 	put_device(&chip->dev);
 	return rc;
@@ -186,6 +180,7 @@ EXPORT_SYMBOL_GPL(tpm_try_get_ops);
  */
 void tpm_put_ops(struct tpm_chip *chip)
 {
+	tpm_chip_stop(chip, 0);
 	mutex_unlock(&chip->tpm_mutex);
 	up_read(&chip->ops_sem);
 	put_device(&chip->dev);
@@ -563,7 +558,11 @@ int tpm_chip_register(struct tpm_chip *chip)
 {
 	int rc;
 
+	rc = tpm_chip_start(chip, 0);
+	if (rc)
+		return rc;
 	rc = tpm_auto_startup(chip);
+	tpm_chip_stop(chip, 0);
 	if (rc)
 		return rc;
 
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index f7b7e4e75fcf..f20c78055731 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -168,13 +168,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
 	memcpy(save, buf, save_size);
 
 	for (;;) {
-		ret = tpm_chip_start(chip, flags);
-		if (ret)
-			return ret;
-
 		ret = tpm_try_transmit(chip, buf, bufsiz, flags);
-
-		tpm_chip_stop(chip, flags);
 		if (ret < 0)
 			break;
 		rc = be32_to_cpu(header->return_code);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2d6d934f1c8b..53e4208759ee 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -485,15 +485,6 @@ extern const struct file_operations tpm_fops;
 extern const struct file_operations tpmrm_fops;
 extern struct idr dev_nums_idr;
 
-/**
- * enum tpm_transmit_flags - flags for tpm_transmit()
- *
- * %TPM_TRANSMIT_NESTED:	discard setup steps (power management, locality)
- */
-enum tpm_transmit_flags {
-	TPM_TRANSMIT_NESTED      = BIT(0),
-};
-
 ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
 		     unsigned int flags);
 ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf,
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 9c9ccf2c0681..6c6ad2d4d31b 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -60,7 +60,10 @@ int tpm2_init_space(struct tpm_space *space)
 void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
 {
 	mutex_lock(&chip->tpm_mutex);
-	tpm2_flush_sessions(chip, space);
+	if (!tpm_chip_start(chip, 0)) {
+		tpm2_flush_sessions(chip, space);
+		tpm_chip_stop(chip, 0);
+	}
 	mutex_unlock(&chip->tpm_mutex);
 	kfree(space->context_buf);
 	kfree(space->session_buf);
diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index e8a1da2810a9..a4bb60e163cc 100644
--- a/drivers/char/tpm/tpm_vtpm_proxy.c
+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
@@ -417,8 +417,7 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
 
 	proxy_dev->state |= STATE_DRIVER_COMMAND;
 
-	rc = tpm_transmit_cmd(chip, &buf, 0, TPM_TRANSMIT_NESTED,
-			      "attempting to set locality");
+	rc = tpm_transmit_cmd(chip, &buf, 0, 0, "attempting to set locality");
 
 	proxy_dev->state &= ~STATE_DRIVER_COMMAND;
 
-- 
2.19.1


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

* [PATCH v11 16/16] tpm: remove @flags from tpm_transmit()
  2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
                   ` (14 preceding siblings ...)
  2019-02-05 22:47 ` [PATCH v11 15/16] tpm: take TPM chip power gating out of tpm_transmit() Jarkko Sakkinen
@ 2019-02-05 22:47 ` Jarkko Sakkinen
  2019-02-06 12:06 ` [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
  16 siblings, 0 replies; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-05 22:47 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain, Jarkko Sakkinen

Remove @flags from tpm_transmit() API. It is no longer used for
anything.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-chip.c       | 32 ++++++++++-----------
 drivers/char/tpm/tpm-dev-common.c |  2 +-
 drivers/char/tpm/tpm-interface.c  | 18 ++++--------
 drivers/char/tpm/tpm-sysfs.c      |  2 +-
 drivers/char/tpm/tpm.h            | 13 ++++-----
 drivers/char/tpm/tpm1-cmd.c       | 14 ++++-----
 drivers/char/tpm/tpm2-cmd.c       | 48 ++++++++++++++-----------------
 drivers/char/tpm/tpm2-space.c     | 16 +++++------
 drivers/char/tpm/tpm_vtpm_proxy.c |  2 +-
 9 files changed, 65 insertions(+), 82 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 7ad4d9045e4c..2019635d0f70 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -37,7 +37,7 @@ struct class *tpm_class;
 struct class *tpmrm_class;
 dev_t tpm_devt;
 
-static int tpm_request_locality(struct tpm_chip *chip, unsigned int flags)
+static int tpm_request_locality(struct tpm_chip *chip)
 {
 	int rc;
 
@@ -52,7 +52,7 @@ static int tpm_request_locality(struct tpm_chip *chip, unsigned int flags)
 	return 0;
 }
 
-static void tpm_relinquish_locality(struct tpm_chip *chip, unsigned int flags)
+static void tpm_relinquish_locality(struct tpm_chip *chip)
 {
 	int rc;
 
@@ -66,7 +66,7 @@ static void tpm_relinquish_locality(struct tpm_chip *chip, unsigned int flags)
 	chip->locality = -1;
 }
 
-static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags)
+static int tpm_cmd_ready(struct tpm_chip *chip)
 {
 	if (!chip->ops->cmd_ready)
 		return 0;
@@ -74,7 +74,7 @@ static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags)
 	return chip->ops->cmd_ready(chip);
 }
 
-static int tpm_go_idle(struct tpm_chip *chip, unsigned int flags)
+static int tpm_go_idle(struct tpm_chip *chip)
 {
 	if (!chip->ops->go_idle)
 		return 0;
@@ -85,13 +85,12 @@ static int tpm_go_idle(struct tpm_chip *chip, unsigned int flags)
 /**
  * tpm_chip_start() - power on the TPM
  * @chip:	a TPM chip to use
- * @flags:	TPM transmit flags
  *
  * Return:
  * * The response length	- OK
  * * -errno			- A system error
  */
-int tpm_chip_start(struct tpm_chip *chip, unsigned int flags)
+int tpm_chip_start(struct tpm_chip *chip)
 {
 	int ret;
 
@@ -99,16 +98,16 @@ int tpm_chip_start(struct tpm_chip *chip, unsigned int flags)
 		chip->ops->clk_enable(chip, true);
 
 	if (chip->locality == -1) {
-		ret = tpm_request_locality(chip, flags);
+		ret = tpm_request_locality(chip);
 		if (ret) {
 			chip->ops->clk_enable(chip, false);
 			return ret;
 		}
 	}
 
-	ret = tpm_cmd_ready(chip, flags);
+	ret = tpm_cmd_ready(chip);
 	if (ret) {
-		tpm_relinquish_locality(chip, flags);
+		tpm_relinquish_locality(chip);
 		if (chip->ops->clk_enable)
 			chip->ops->clk_enable(chip, false);
 		return ret;
@@ -120,16 +119,15 @@ int tpm_chip_start(struct tpm_chip *chip, unsigned int flags)
 /**
  * tpm_chip_stop() - power off the TPM
  * @chip:	a TPM chip to use
- * @flags:	TPM transmit flags
  *
  * Return:
  * * The response length	- OK
  * * -errno			- A system error
  */
-void tpm_chip_stop(struct tpm_chip *chip, unsigned int flags)
+void tpm_chip_stop(struct tpm_chip *chip)
 {
-	tpm_go_idle(chip, flags);
-	tpm_relinquish_locality(chip, flags);
+	tpm_go_idle(chip);
+	tpm_relinquish_locality(chip);
 	if (chip->ops->clk_enable)
 		chip->ops->clk_enable(chip, false);
 }
@@ -157,7 +155,7 @@ int tpm_try_get_ops(struct tpm_chip *chip)
 		goto out_ops;
 
 	mutex_lock(&chip->tpm_mutex);
-	rc = tpm_chip_start(chip, 0);
+	rc = tpm_chip_start(chip);
 	if (rc)
 		goto out_lock;
 
@@ -180,7 +178,7 @@ EXPORT_SYMBOL_GPL(tpm_try_get_ops);
  */
 void tpm_put_ops(struct tpm_chip *chip)
 {
-	tpm_chip_stop(chip, 0);
+	tpm_chip_stop(chip);
 	mutex_unlock(&chip->tpm_mutex);
 	up_read(&chip->ops_sem);
 	put_device(&chip->dev);
@@ -558,11 +556,11 @@ int tpm_chip_register(struct tpm_chip *chip)
 {
 	int rc;
 
-	rc = tpm_chip_start(chip, 0);
+	rc = tpm_chip_start(chip);
 	if (rc)
 		return rc;
 	rc = tpm_auto_startup(chip);
-	tpm_chip_stop(chip, 0);
+	tpm_chip_stop(chip);
 	if (rc)
 		return rc;
 
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 435c09ec9056..8856cce5a23b 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -47,7 +47,7 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	if (ret)
 		goto out_rc;
 
-	len = tpm_transmit(chip, buf, bufsiz, 0);
+	len = tpm_transmit(chip, buf, bufsiz);
 	if (len < 0)
 		ret = len;
 
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index f20c78055731..ce39fe043bcb 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -62,8 +62,7 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
 }
 EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
 
-static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz,
-				unsigned int flags)
+static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
 {
 	struct tpm_header *header = buf;
 	int rc;
@@ -134,7 +133,6 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz,
  * @chip:	a TPM chip to use
  * @buf:	a TPM command buffer
  * @bufsiz:	length of the TPM command buffer
- * @flags:	TPM transmit flags
  *
  * A wrapper around tpm_try_transmit() that handles TPM2_RC_RETRY returns from
  * the TPM and retransmits the command after a delay up to a maximum wait of
@@ -147,8 +145,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz,
  * * The response length	- OK
  * * -errno			- A system error
  */
-ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
-		     unsigned int flags)
+ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz)
 {
 	struct tpm_header *header = (struct tpm_header *)buf;
 	/* space for header and handles */
@@ -168,7 +165,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
 	memcpy(save, buf, save_size);
 
 	for (;;) {
-		ret = tpm_try_transmit(chip, buf, bufsiz, flags);
+		ret = tpm_try_transmit(chip, buf, bufsiz);
 		if (ret < 0)
 			break;
 		rc = be32_to_cpu(header->return_code);
@@ -201,7 +198,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
  * @chip:			a TPM chip to use
  * @buf:			a TPM command buffer
  * @min_rsp_body_length:	minimum expected length of response body
- * @flags:			TPM transmit flags
  * @desc:			command description used in the error message
  *
  * Return:
@@ -210,14 +206,13 @@ ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
  * * TPM_RC	- A TPM error
  */
 ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf,
-			 size_t min_rsp_body_length, unsigned int flags,
-			 const char *desc)
+			 size_t min_rsp_body_length, const char *desc)
 {
 	const struct tpm_header *header = (struct tpm_header *)buf->data;
 	int err;
 	ssize_t len;
 
-	len = tpm_transmit(chip, buf->data, PAGE_SIZE, flags);
+	len = tpm_transmit(chip, buf->data, PAGE_SIZE);
 	if (len <  0)
 		return len;
 
@@ -366,8 +361,7 @@ int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen)
 		goto out;
 
 	memcpy(buf.data, cmd, buflen);
-	rc = tpm_transmit_cmd(chip, &buf, 0, 0,
-			      "attempting to a send a command");
+	rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to a send a command");
 	tpm_buf_destroy(&buf);
 out:
 	tpm_put_ops(chip);
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index 3733491671ca..533a260d744e 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -55,7 +55,7 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
 	tpm_buf_append(&tpm_buf, anti_replay, sizeof(anti_replay));
 
 	if (tpm_transmit_cmd(chip, &tpm_buf, READ_PUBEK_RESULT_MIN_BODY_SIZE,
-			      0, "attempting to read the PUBEK"))
+			     "attempting to read the PUBEK"))
 		goto out_buf;
 
 	out = (struct tpm_readpubek_out *)&tpm_buf.data[10];
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 53e4208759ee..183e2b93e0fe 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -485,11 +485,9 @@ extern const struct file_operations tpm_fops;
 extern const struct file_operations tpmrm_fops;
 extern struct idr dev_nums_idr;
 
-ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
-		     unsigned int flags);
+ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz);
 ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf,
-			 size_t min_rsp_body_length, unsigned int flags,
-			 const char *desc);
+			 size_t min_rsp_body_length, const char *desc);
 int tpm_get_timeouts(struct tpm_chip *);
 int tpm_auto_startup(struct tpm_chip *chip);
 
@@ -514,8 +512,8 @@ static inline void tpm_msleep(unsigned int delay_msec)
 		     delay_msec * 1000);
 };
 
-int tpm_chip_start(struct tpm_chip *chip, unsigned int flags);
-void tpm_chip_stop(struct tpm_chip *chip, unsigned int flags);
+int tpm_chip_start(struct tpm_chip *chip);
+void tpm_chip_stop(struct tpm_chip *chip);
 struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
 __must_check int tpm_try_get_ops(struct tpm_chip *chip);
 void tpm_put_ops(struct tpm_chip *chip);
@@ -548,8 +546,7 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
 int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
 		    struct tpm2_digest *digests);
 int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
-void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
-			    unsigned int flags);
+void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
 int tpm2_seal_trusted(struct tpm_chip *chip,
 		      struct trusted_key_payload *payload,
 		      struct trusted_key_options *options);
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 5b5f8bcc6210..ec5f3693c096 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -334,7 +334,7 @@ static int tpm1_startup(struct tpm_chip *chip)
 
 	tpm_buf_append_u16(&buf, TPM_ST_CLEAR);
 
-	rc = tpm_transmit_cmd(chip, &buf, 0, 0, "attempting to start the TPM");
+	rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to start the TPM");
 	tpm_buf_destroy(&buf);
 	return rc;
 }
@@ -458,7 +458,7 @@ int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
 	tpm_buf_append_u32(&buf, pcr_idx);
 	tpm_buf_append(&buf, hash, TPM_DIGEST_SIZE);
 
-	rc = tpm_transmit_cmd(chip, &buf, TPM_DIGEST_SIZE, 0, log_msg);
+	rc = tpm_transmit_cmd(chip, &buf, TPM_DIGEST_SIZE, log_msg);
 	tpm_buf_destroy(&buf);
 	return rc;
 }
@@ -488,7 +488,7 @@ ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
 		tpm_buf_append_u32(&buf, 4);
 		tpm_buf_append_u32(&buf, subcap_id);
 	}
-	rc = tpm_transmit_cmd(chip, &buf, min_cap_length, 0, desc);
+	rc = tpm_transmit_cmd(chip, &buf, min_cap_length, desc);
 	if (!rc)
 		*cap = *(cap_t *)&buf.data[TPM_HEADER_SIZE + 4];
 	tpm_buf_destroy(&buf);
@@ -529,7 +529,7 @@ int tpm1_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
 	do {
 		tpm_buf_append_u32(&buf, num_bytes);
 
-		rc = tpm_transmit_cmd(chip, &buf, sizeof(out->rng_data_len), 0,
+		rc = tpm_transmit_cmd(chip, &buf, sizeof(out->rng_data_len),
 				      "attempting get random");
 		if (rc)
 			goto out;
@@ -574,7 +574,7 @@ int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf)
 
 	tpm_buf_append_u32(&buf, pcr_idx);
 
-	rc = tpm_transmit_cmd(chip, &buf, TPM_DIGEST_SIZE, 0,
+	rc = tpm_transmit_cmd(chip, &buf, TPM_DIGEST_SIZE,
 			      "attempting to read a pcr value");
 	if (rc)
 		goto out;
@@ -608,7 +608,7 @@ static int tpm1_continue_selftest(struct tpm_chip *chip)
 	if (rc)
 		return rc;
 
-	rc = tpm_transmit_cmd(chip, &buf, 0, 0, "continue selftest");
+	rc = tpm_transmit_cmd(chip, &buf, 0, "continue selftest");
 	tpm_buf_destroy(&buf);
 	return rc;
 }
@@ -734,7 +734,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
 		return rc;
 	/* now do the actual savestate */
 	for (try = 0; try < TPM_RETRY; try++) {
-		rc = tpm_transmit_cmd(chip, &buf, 0, 0, NULL);
+		rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
 		/*
 		 * If the TPM indicates that it is too busy to respond to
 		 * this command then retry before giving up.  It can take
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index d6abc964ef66..971d46efaca5 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -197,7 +197,7 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf)
 	tpm_buf_append(&buf, (const unsigned char *)pcr_select,
 		       sizeof(pcr_select));
 
-	rc = tpm_transmit_cmd(chip, &buf, 0, 0, res_buf ?
+	rc = tpm_transmit_cmd(chip, &buf, 0, res_buf ?
 			      "attempting to read a pcr value" : NULL);
 	if (rc == 0 && res_buf) {
 		out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
@@ -264,8 +264,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
 		}
 	}
 
-	rc = tpm_transmit_cmd(chip, &buf, 0, 0,
-			      "attempting extend a PCR value");
+	rc = tpm_transmit_cmd(chip, &buf, 0, "attempting extend a PCR value");
 
 	tpm_buf_destroy(&buf);
 
@@ -312,7 +311,7 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
 		err = tpm_transmit_cmd(chip, &buf,
 				       offsetof(struct tpm2_get_random_out,
 						buffer),
-				       0, "attempting get random");
+				       "attempting get random");
 		if (err)
 			goto out;
 
@@ -341,14 +340,11 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
 }
 
 /**
- * tpm2_flush_context_cmd() - execute a TPM2_FlushContext command
+ * tpm2_flush_context() - execute a TPM2_FlushContext command
  * @chip:	TPM chip to use
  * @handle:	context handle
- * @flags:	tpm transmit flags - bitmap
- *
  */
-void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
-			    unsigned int flags)
+void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
 {
 	struct tpm_buf buf;
 	int rc;
@@ -362,7 +358,7 @@ void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
 
 	tpm_buf_append_u32(&buf, handle);
 
-	tpm_transmit_cmd(chip, &buf, 0, flags, "flushing context");
+	tpm_transmit_cmd(chip, &buf, 0, "flushing context");
 	tpm_buf_destroy(&buf);
 }
 
@@ -476,7 +472,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 		goto out;
 	}
 
-	rc = tpm_transmit_cmd(chip, &buf, 4, 0, "sealing data");
+	rc = tpm_transmit_cmd(chip, &buf, 4, "sealing data");
 	if (rc)
 		goto out;
 
@@ -513,7 +509,6 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
  * @payload: the key data in clear and encrypted form
  * @options: authentication values and other options
  * @blob_handle: returned blob handle
- * @flags: tpm transmit flags
  *
  * Return: 0 on success.
  *        -E2BIG on wrong payload size.
@@ -523,7 +518,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 static int tpm2_load_cmd(struct tpm_chip *chip,
 			 struct trusted_key_payload *payload,
 			 struct trusted_key_options *options,
-			 u32 *blob_handle, unsigned int flags)
+			 u32 *blob_handle)
 {
 	struct tpm_buf buf;
 	unsigned int private_len;
@@ -558,7 +553,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 		goto out;
 	}
 
-	rc = tpm_transmit_cmd(chip, &buf, 4, flags, "loading blob");
+	rc = tpm_transmit_cmd(chip, &buf, 4, "loading blob");
 	if (!rc)
 		*blob_handle = be32_to_cpup(
 			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
@@ -579,7 +574,6 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
  * @payload: the key data in clear and encrypted form
  * @options: authentication values and other options
  * @blob_handle: blob handle
- * @flags: tpm_transmit_cmd flags
  *
  * Return: 0 on success
  *         -EPERM on tpm error status
@@ -588,7 +582,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 static int tpm2_unseal_cmd(struct tpm_chip *chip,
 			   struct trusted_key_payload *payload,
 			   struct trusted_key_options *options,
-			   u32 blob_handle, unsigned int flags)
+			   u32 blob_handle)
 {
 	struct tpm_buf buf;
 	u16 data_len;
@@ -608,7 +602,7 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
 			     options->blobauth /* hmac */,
 			     TPM_DIGEST_SIZE);
 
-	rc = tpm_transmit_cmd(chip, &buf, 6, flags, "unsealing");
+	rc = tpm_transmit_cmd(chip, &buf, 6, "unsealing");
 	if (rc > 0)
 		rc = -EPERM;
 
@@ -652,12 +646,12 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
 	u32 blob_handle;
 	int rc;
 
-	rc = tpm2_load_cmd(chip, payload, options, &blob_handle, 0);
+	rc = tpm2_load_cmd(chip, payload, options, &blob_handle);
 	if (rc)
 		return rc;
 
-	rc = tpm2_unseal_cmd(chip, payload, options, blob_handle, 0);
-	tpm2_flush_context_cmd(chip, blob_handle, 0);
+	rc = tpm2_unseal_cmd(chip, payload, options, blob_handle);
+	tpm2_flush_context(chip, blob_handle);
 	return rc;
 }
 
@@ -693,7 +687,7 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,  u32 *value,
 	tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
 	tpm_buf_append_u32(&buf, property_id);
 	tpm_buf_append_u32(&buf, 1);
-	rc = tpm_transmit_cmd(chip, &buf, 0, 0, NULL);
+	rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
 	if (!rc) {
 		out = (struct tpm2_get_cap_out *)
 			&buf.data[TPM_HEADER_SIZE];
@@ -723,7 +717,7 @@ void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type)
 	if (rc)
 		return;
 	tpm_buf_append_u16(&buf, shutdown_type);
-	tpm_transmit_cmd(chip, &buf, 0, 0, "stopping the TPM");
+	tpm_transmit_cmd(chip, &buf, 0, "stopping the TPM");
 	tpm_buf_destroy(&buf);
 }
 
@@ -752,7 +746,7 @@ static int tpm2_do_selftest(struct tpm_chip *chip)
 			return rc;
 
 		tpm_buf_append_u8(&buf, full);
-		rc = tpm_transmit_cmd(chip, &buf, 0, 0,
+		rc = tpm_transmit_cmd(chip, &buf, 0,
 				      "attempting the self test");
 		tpm_buf_destroy(&buf);
 
@@ -789,7 +783,7 @@ int tpm2_probe(struct tpm_chip *chip)
 	tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
 	tpm_buf_append_u32(&buf, TPM_PT_TOTAL_COMMANDS);
 	tpm_buf_append_u32(&buf, 1);
-	rc = tpm_transmit_cmd(chip, &buf, 0, 0, NULL);
+	rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
 	/* We ignore TPM return codes on purpose. */
 	if (rc >=  0) {
 		out = (struct tpm_header *)buf.data;
@@ -828,7 +822,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	tpm_buf_append_u32(&buf, 0);
 	tpm_buf_append_u32(&buf, 1);
 
-	rc = tpm_transmit_cmd(chip, &buf, 9, 0, "get tpm pcr allocation");
+	rc = tpm_transmit_cmd(chip, &buf, 9, "get tpm pcr allocation");
 	if (rc)
 		goto out;
 
@@ -899,7 +893,7 @@ static int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip)
 	tpm_buf_append_u32(&buf, TPM2_CC_FIRST);
 	tpm_buf_append_u32(&buf, nr_commands);
 
-	rc = tpm_transmit_cmd(chip, &buf, 9 + 4 * nr_commands, 0, NULL);
+	rc = tpm_transmit_cmd(chip, &buf, 9 + 4 * nr_commands, NULL);
 	if (rc) {
 		tpm_buf_destroy(&buf);
 		goto out;
@@ -956,7 +950,7 @@ static int tpm2_startup(struct tpm_chip *chip)
 		return rc;
 
 	tpm_buf_append_u16(&buf, TPM2_SU_CLEAR);
-	rc = tpm_transmit_cmd(chip, &buf, 0, 0, "attempting to start the TPM");
+	rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to start the TPM");
 	tpm_buf_destroy(&buf);
 
 	return rc;
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 6c6ad2d4d31b..a9ced49546ad 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -38,7 +38,7 @@ static void tpm2_flush_sessions(struct tpm_chip *chip, struct tpm_space *space)
 
 	for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
 		if (space->session_tbl[i])
-			tpm2_flush_context_cmd(chip, space->session_tbl[i], 0);
+			tpm2_flush_context(chip, space->session_tbl[i]);
 	}
 }
 
@@ -60,9 +60,9 @@ int tpm2_init_space(struct tpm_space *space)
 void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
 {
 	mutex_lock(&chip->tpm_mutex);
-	if (!tpm_chip_start(chip, 0)) {
+	if (!tpm_chip_start(chip)) {
 		tpm2_flush_sessions(chip, space);
-		tpm_chip_stop(chip, 0);
+		tpm_chip_stop(chip);
 	}
 	mutex_unlock(&chip->tpm_mutex);
 	kfree(space->context_buf);
@@ -85,7 +85,7 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
 	body_size = sizeof(*ctx) + be16_to_cpu(ctx->blob_size);
 	tpm_buf_append(&tbuf, &buf[*offset], body_size);
 
-	rc = tpm_transmit_cmd(chip, &tbuf, 4, 0, NULL);
+	rc = tpm_transmit_cmd(chip, &tbuf, 4, NULL);
 	if (rc < 0) {
 		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
 			 __func__, rc);
@@ -133,7 +133,7 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
 
 	tpm_buf_append_u32(&tbuf, handle);
 
-	rc = tpm_transmit_cmd(chip, &tbuf, 0, 0, NULL);
+	rc = tpm_transmit_cmd(chip, &tbuf, 0, NULL);
 	if (rc < 0) {
 		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
 			 __func__, rc);
@@ -169,7 +169,7 @@ void tpm2_flush_space(struct tpm_chip *chip)
 
 	for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
 		if (space->context_tbl[i] && ~space->context_tbl[i])
-			tpm2_flush_context_cmd(chip, space->context_tbl[i], 0);
+			tpm2_flush_context(chip, space->context_tbl[i]);
 
 	tpm2_flush_sessions(chip, space);
 }
@@ -418,7 +418,7 @@ static int tpm2_map_response_header(struct tpm_chip *chip, u32 cc, u8 *rsp,
 
 	return 0;
 out_no_slots:
-	tpm2_flush_context_cmd(chip, phandle, 0);
+	tpm2_flush_context(chip, phandle);
 	dev_warn(&chip->dev, "%s: out of slots for 0x%08X\n", __func__,
 		 phandle);
 	return -ENOMEM;
@@ -505,7 +505,7 @@ static int tpm2_save_space(struct tpm_chip *chip)
 		} else if (rc)
 			return rc;
 
-		tpm2_flush_context_cmd(chip, space->context_tbl[i], 0);
+		tpm2_flush_context(chip, space->context_tbl[i]);
 		space->context_tbl[i] = ~0;
 	}
 
diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index a4bb60e163cc..26a2be555288 100644
--- a/drivers/char/tpm/tpm_vtpm_proxy.c
+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
@@ -417,7 +417,7 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
 
 	proxy_dev->state |= STATE_DRIVER_COMMAND;
 
-	rc = tpm_transmit_cmd(chip, &buf, 0, 0, "attempting to set locality");
+	rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to set locality");
 
 	proxy_dev->state &= ~STATE_DRIVER_COMMAND;
 
-- 
2.19.1


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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
                   ` (15 preceding siblings ...)
  2019-02-05 22:47 ` [PATCH v11 16/16] tpm: remove @flags from tpm_transmit() Jarkko Sakkinen
@ 2019-02-06 12:06 ` Jarkko Sakkinen
  2019-02-07 18:41   ` Alexander Steffen
  16 siblings, 1 reply; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-06 12:06 UTC (permalink / raw)
  To: linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain

On Wed, Feb 06, 2019 at 12:47:07AM +0200, Jarkko Sakkinen wrote:
> Make the changes necessary to detach TPM space code and TPM activation
> code out of the tpm_transmit() flow because of both of these can cause
> nested tpm_transmit() calls. The nesteds calls make the whole flow hard
> to maintain, and thus, it is better to just fix things now before this
> turns into a bigger mess.
> 
> v11:
> * Drop the patch that tries to flush TPM space on system. Not a proper
>   fallback + out of scope for this patch set.
> 
> v10:
> * Use void pointers to avoid unnecessary casts in functions paramaters
>   where it makes sense.
> 
> v9:
> * Fixed again tpm_try_get_ops().
> * Added missing reviewed-by's.
> 
> v8:
> * Re-add the check for ret < 0 after calling tpm_try_transmit() that
>   was dropped by mistake while moving code.
> * Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
>   fails.
> 
> v7:
> *  Reorganize series so that more trivial and self-contained changes are
>    in the head.
> 
> v6:
> * When tpm_validate_commmand() was moved to tpm2-space.c, the struct for
>   the TPM header was incorrectly declared as struct tpm_input_header.
> * Fix return value in tpm_validate_command().
> 
> v5:
> * Add the missing rev's from Stefan Berger.
> 
> v4:
> * Return 0 from pcrs_show() when tpm1_pcr_read() fails.
> * Fix error handling flow in tpm_try_transmit().
> * Replace struct tpm_input_header and struct tpm_output_header with
>   struct tpm_header.
> 
> v3:
> * Encapsulate power gating code to tpm_chip_start() and tpm_chip_stop().
> * Move TPM power gating code and locking to tpm_try_get_ops() and
>   tpm_put_ops().
> * Call power gating code directly in tpm_chip_register() and
>   tpm2_del_space().
> 
> v2:
> * Print tpm2_commit_space() error inside tpm2_commit_space()
> * Error code was not printed when recv() callback failed. It is
>   fixed in this version.
> * Added a patch that removes @space from tpm_transmit().
> * Fixed a regression in earlier series. Forgot to amend the change
>   from the staging area that renames NESTED to UNLOCKED in tpm2-space.c.
> 
> Jarkko Sakkinen (16):
>   tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
>   tpm: fix invalid return value in pubek_show()
>   tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
>   tpm: print tpm2_commit_space() error inside tpm2_commit_space()
>   tpm: declare struct tpm_header
>   tpm: access command header through struct in tpm_try_transmit()
>   tpm: encapsulate tpm_dev_transmit()
>   tpm: clean up tpm_try_transmit() error handling flow
>   tpm: move tpm_validate_commmand() to tpm2-space.c
>   tpm: move TPM space code out of tpm_transmit()
>   tpm: remove @space from tpm_transmit()
>   tpm: use tpm_try_get_ops() in tpm-sysfs.c.
>   tpm: remove TPM_TRANSMIT_UNLOCKED flag
>   tpm: introduce tpm_chip_start() and tpm_chip_stop()
>   tpm: take TPM chip power gating out of tpm_transmit()
>   tpm: remove @flags from tpm_transmit()
> 
>  drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
>  drivers/char/tpm/tpm-dev-common.c |  44 ++++-
>  drivers/char/tpm/tpm-interface.c  | 264 ++++++------------------------
>  drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
>  drivers/char/tpm/tpm.h            |  64 +++-----
>  drivers/char/tpm/tpm1-cmd.c       |  28 +---
>  drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
>  drivers/char/tpm/tpm2-space.c     |  91 +++++++---
>  drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
>  drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
>  drivers/char/tpm/xen-tpmfront.c   |   2 +-
>  11 files changed, 408 insertions(+), 421 deletions(-)
> 
> -- 
> 2.19.1
> 

Applied to master and next.

/Jarkko

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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-06 12:06 ` [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
@ 2019-02-07 18:41   ` Alexander Steffen
  2019-02-07 21:14     ` Stefan Berger
  2019-02-07 21:29     ` Jarkko Sakkinen
  0 siblings, 2 replies; 42+ messages in thread
From: Alexander Steffen @ 2019-02-07 18:41 UTC (permalink / raw)
  To: Jarkko Sakkinen, linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain

On 06.02.2019 13:06, Jarkko Sakkinen wrote:
> On Wed, Feb 06, 2019 at 12:47:07AM +0200, Jarkko Sakkinen wrote:
>> Make the changes necessary to detach TPM space code and TPM activation
>> code out of the tpm_transmit() flow because of both of these can cause
>> nested tpm_transmit() calls. The nesteds calls make the whole flow hard
>> to maintain, and thus, it is better to just fix things now before this
>> turns into a bigger mess.
>>
>> v11:
>> * Drop the patch that tries to flush TPM space on system. Not a proper
>>    fallback + out of scope for this patch set.
>>
>> v10:
>> * Use void pointers to avoid unnecessary casts in functions paramaters
>>    where it makes sense.
>>
>> v9:
>> * Fixed again tpm_try_get_ops().
>> * Added missing reviewed-by's.
>>
>> v8:
>> * Re-add the check for ret < 0 after calling tpm_try_transmit() that
>>    was dropped by mistake while moving code.
>> * Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
>>    fails.
>>
>> v7:
>> *  Reorganize series so that more trivial and self-contained changes are
>>     in the head.
>>
>> v6:
>> * When tpm_validate_commmand() was moved to tpm2-space.c, the struct for
>>    the TPM header was incorrectly declared as struct tpm_input_header.
>> * Fix return value in tpm_validate_command().
>>
>> v5:
>> * Add the missing rev's from Stefan Berger.
>>
>> v4:
>> * Return 0 from pcrs_show() when tpm1_pcr_read() fails.
>> * Fix error handling flow in tpm_try_transmit().
>> * Replace struct tpm_input_header and struct tpm_output_header with
>>    struct tpm_header.
>>
>> v3:
>> * Encapsulate power gating code to tpm_chip_start() and tpm_chip_stop().
>> * Move TPM power gating code and locking to tpm_try_get_ops() and
>>    tpm_put_ops().
>> * Call power gating code directly in tpm_chip_register() and
>>    tpm2_del_space().
>>
>> v2:
>> * Print tpm2_commit_space() error inside tpm2_commit_space()
>> * Error code was not printed when recv() callback failed. It is
>>    fixed in this version.
>> * Added a patch that removes @space from tpm_transmit().
>> * Fixed a regression in earlier series. Forgot to amend the change
>>    from the staging area that renames NESTED to UNLOCKED in tpm2-space.c.
>>
>> Jarkko Sakkinen (16):
>>    tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
>>    tpm: fix invalid return value in pubek_show()
>>    tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
>>    tpm: print tpm2_commit_space() error inside tpm2_commit_space()
>>    tpm: declare struct tpm_header
>>    tpm: access command header through struct in tpm_try_transmit()
>>    tpm: encapsulate tpm_dev_transmit()
>>    tpm: clean up tpm_try_transmit() error handling flow
>>    tpm: move tpm_validate_commmand() to tpm2-space.c
>>    tpm: move TPM space code out of tpm_transmit()
>>    tpm: remove @space from tpm_transmit()
>>    tpm: use tpm_try_get_ops() in tpm-sysfs.c.
>>    tpm: remove TPM_TRANSMIT_UNLOCKED flag
>>    tpm: introduce tpm_chip_start() and tpm_chip_stop()
>>    tpm: take TPM chip power gating out of tpm_transmit()
>>    tpm: remove @flags from tpm_transmit()
>>
>>   drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
>>   drivers/char/tpm/tpm-dev-common.c |  44 ++++-
>>   drivers/char/tpm/tpm-interface.c  | 264 ++++++------------------------
>>   drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
>>   drivers/char/tpm/tpm.h            |  64 +++-----
>>   drivers/char/tpm/tpm1-cmd.c       |  28 +---
>>   drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
>>   drivers/char/tpm/tpm2-space.c     |  91 +++++++---
>>   drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
>>   drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
>>   drivers/char/tpm/xen-tpmfront.c   |   2 +-
>>   11 files changed, 408 insertions(+), 421 deletions(-)
>>
>> -- 
>> 2.19.1
>>
> 
> Applied to master and next.

Something in this series seems to break basic TPM communication for me.

For TPM2.0s the probe command fails, causing them to be misdetected as 
TPM1.2s:

---
tpm tpm0: tpm_try_transmit: tpm_send: error -5
tpm_tis MSFT0101:00: 1.2 TPM (device-id 0x1A, rev-id 22)
tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
---
tpm tpm0: tpm_try_transmit: tpm_send: error -5
tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 22)
tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
---

And for something that actually is a TPM1.2 it fails in a similar way:

---
tpm_i2c_infineon 1-0020: 1.2 TPM (device-id 0x1A)
tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
---
tpm tpm0: tpm_try_transmit: tpm_send: error -5
tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 16)
tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
tpm_tis_spi: probe of spi0.1 failed with error -14
---

I see this problem across my entire range of TPM devices and test 
platforms. Any idea what could be wrong here?

Alexander

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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-07 18:41   ` Alexander Steffen
@ 2019-02-07 21:14     ` Stefan Berger
  2019-02-07 21:29     ` Jarkko Sakkinen
  1 sibling, 0 replies; 42+ messages in thread
From: Stefan Berger @ 2019-02-07 21:14 UTC (permalink / raw)
  To: Alexander Steffen, Jarkko Sakkinen, linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain

On 2/7/19 1:41 PM, Alexander Steffen wrote:
> On 06.02.2019 13:06, Jarkko Sakkinen wrote:
>> On Wed, Feb 06, 2019 at 12:47:07AM +0200, Jarkko Sakkinen wrote:
>>> Make the changes necessary to detach TPM space code and TPM activation
>>> code out of the tpm_transmit() flow because of both of these can cause
>>> nested tpm_transmit() calls. The nesteds calls make the whole flow hard
>>> to maintain, and thus, it is better to just fix things now before this
>>> turns into a bigger mess.
>>>
>>> v11:
>>> * Drop the patch that tries to flush TPM space on system. Not a proper
>>>    fallback + out of scope for this patch set.
>>>
>>> v10:
>>> * Use void pointers to avoid unnecessary casts in functions paramaters
>>>    where it makes sense.
>>>
>>> v9:
>>> * Fixed again tpm_try_get_ops().
>>> * Added missing reviewed-by's.
>>>
>>> v8:
>>> * Re-add the check for ret < 0 after calling tpm_try_transmit() that
>>>    was dropped by mistake while moving code.
>>> * Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
>>>    fails.
>>>
>>> v7:
>>> *  Reorganize series so that more trivial and self-contained changes 
>>> are
>>>     in the head.
>>>
>>> v6:
>>> * When tpm_validate_commmand() was moved to tpm2-space.c, the struct 
>>> for
>>>    the TPM header was incorrectly declared as struct tpm_input_header.
>>> * Fix return value in tpm_validate_command().
>>>
>>> v5:
>>> * Add the missing rev's from Stefan Berger.
>>>
>>> v4:
>>> * Return 0 from pcrs_show() when tpm1_pcr_read() fails.
>>> * Fix error handling flow in tpm_try_transmit().
>>> * Replace struct tpm_input_header and struct tpm_output_header with
>>>    struct tpm_header.
>>>
>>> v3:
>>> * Encapsulate power gating code to tpm_chip_start() and 
>>> tpm_chip_stop().
>>> * Move TPM power gating code and locking to tpm_try_get_ops() and
>>>    tpm_put_ops().
>>> * Call power gating code directly in tpm_chip_register() and
>>>    tpm2_del_space().
>>>
>>> v2:
>>> * Print tpm2_commit_space() error inside tpm2_commit_space()
>>> * Error code was not printed when recv() callback failed. It is
>>>    fixed in this version.
>>> * Added a patch that removes @space from tpm_transmit().
>>> * Fixed a regression in earlier series. Forgot to amend the change
>>>    from the staging area that renames NESTED to UNLOCKED in 
>>> tpm2-space.c.
>>>
>>> Jarkko Sakkinen (16):
>>>    tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
>>>    tpm: fix invalid return value in pubek_show()
>>>    tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
>>>    tpm: print tpm2_commit_space() error inside tpm2_commit_space()
>>>    tpm: declare struct tpm_header
>>>    tpm: access command header through struct in tpm_try_transmit()
>>>    tpm: encapsulate tpm_dev_transmit()
>>>    tpm: clean up tpm_try_transmit() error handling flow
>>>    tpm: move tpm_validate_commmand() to tpm2-space.c
>>>    tpm: move TPM space code out of tpm_transmit()
>>>    tpm: remove @space from tpm_transmit()
>>>    tpm: use tpm_try_get_ops() in tpm-sysfs.c.
>>>    tpm: remove TPM_TRANSMIT_UNLOCKED flag
>>>    tpm: introduce tpm_chip_start() and tpm_chip_stop()
>>>    tpm: take TPM chip power gating out of tpm_transmit()
>>>    tpm: remove @flags from tpm_transmit()
>>>
>>>   drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
>>>   drivers/char/tpm/tpm-dev-common.c |  44 ++++-
>>>   drivers/char/tpm/tpm-interface.c  | 264 
>>> ++++++------------------------
>>>   drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
>>>   drivers/char/tpm/tpm.h            |  64 +++-----
>>>   drivers/char/tpm/tpm1-cmd.c       |  28 +---
>>>   drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
>>>   drivers/char/tpm/tpm2-space.c     |  91 +++++++---
>>>   drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
>>>   drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
>>>   drivers/char/tpm/xen-tpmfront.c   |   2 +-
>>>   11 files changed, 408 insertions(+), 421 deletions(-)
>>>
>>> -- 
>>> 2.19.1
>>>
>>
>> Applied to master and next.
>
> Something in this series seems to break basic TPM communication for me.
>
> For TPM2.0s the probe command fails, causing them to be misdetected as 
> TPM1.2s:
>
> ---
> tpm tpm0: tpm_try_transmit: tpm_send: error -5
> tpm_tis MSFT0101:00: 1.2 TPM (device-id 0x1A, rev-id 22)
> tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
> ---
> tpm tpm0: tpm_try_transmit: tpm_send: error -5
> tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 22)
> tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
> ---
>
> And for something that actually is a TPM1.2 it fails in a similar way:
>
> ---
> tpm_i2c_infineon 1-0020: 1.2 TPM (device-id 0x1A)
> tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
> ---
> tpm tpm0: tpm_try_transmit: tpm_send: error -5
> tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 16)
> tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
> tpm_tis_spi: probe of spi0.1 failed with error -14
> ---
>
> I see this problem across my entire range of TPM devices and test 
> platforms. Any idea what could be wrong here?


Can you bisect the series ?

    Stefan



>
>
> Alexander
>


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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-07 18:41   ` Alexander Steffen
  2019-02-07 21:14     ` Stefan Berger
@ 2019-02-07 21:29     ` Jarkko Sakkinen
  2019-02-07 23:29       ` Stefan Berger
  1 sibling, 1 reply; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-07 21:29 UTC (permalink / raw)
  To: Alexander Steffen
  Cc: linux-integrity, linux-kernel, linux-security-module,
	Peter Huewe, Jason Gunthorpe, Tomas Winkler, Tadeusz Struk,
	Stefan Berger, Nayna Jain

On Thu, Feb 07, 2019 at 07:41:56PM +0100, Alexander Steffen wrote:
> On 06.02.2019 13:06, Jarkko Sakkinen wrote:
> > On Wed, Feb 06, 2019 at 12:47:07AM +0200, Jarkko Sakkinen wrote:
> > > Make the changes necessary to detach TPM space code and TPM activation
> > > code out of the tpm_transmit() flow because of both of these can cause
> > > nested tpm_transmit() calls. The nesteds calls make the whole flow hard
> > > to maintain, and thus, it is better to just fix things now before this
> > > turns into a bigger mess.
> > > 
> > > v11:
> > > * Drop the patch that tries to flush TPM space on system. Not a proper
> > >    fallback + out of scope for this patch set.
> > > 
> > > v10:
> > > * Use void pointers to avoid unnecessary casts in functions paramaters
> > >    where it makes sense.
> > > 
> > > v9:
> > > * Fixed again tpm_try_get_ops().
> > > * Added missing reviewed-by's.
> > > 
> > > v8:
> > > * Re-add the check for ret < 0 after calling tpm_try_transmit() that
> > >    was dropped by mistake while moving code.
> > > * Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
> > >    fails.
> > > 
> > > v7:
> > > *  Reorganize series so that more trivial and self-contained changes are
> > >     in the head.
> > > 
> > > v6:
> > > * When tpm_validate_commmand() was moved to tpm2-space.c, the struct for
> > >    the TPM header was incorrectly declared as struct tpm_input_header.
> > > * Fix return value in tpm_validate_command().
> > > 
> > > v5:
> > > * Add the missing rev's from Stefan Berger.
> > > 
> > > v4:
> > > * Return 0 from pcrs_show() when tpm1_pcr_read() fails.
> > > * Fix error handling flow in tpm_try_transmit().
> > > * Replace struct tpm_input_header and struct tpm_output_header with
> > >    struct tpm_header.
> > > 
> > > v3:
> > > * Encapsulate power gating code to tpm_chip_start() and tpm_chip_stop().
> > > * Move TPM power gating code and locking to tpm_try_get_ops() and
> > >    tpm_put_ops().
> > > * Call power gating code directly in tpm_chip_register() and
> > >    tpm2_del_space().
> > > 
> > > v2:
> > > * Print tpm2_commit_space() error inside tpm2_commit_space()
> > > * Error code was not printed when recv() callback failed. It is
> > >    fixed in this version.
> > > * Added a patch that removes @space from tpm_transmit().
> > > * Fixed a regression in earlier series. Forgot to amend the change
> > >    from the staging area that renames NESTED to UNLOCKED in tpm2-space.c.
> > > 
> > > Jarkko Sakkinen (16):
> > >    tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
> > >    tpm: fix invalid return value in pubek_show()
> > >    tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
> > >    tpm: print tpm2_commit_space() error inside tpm2_commit_space()
> > >    tpm: declare struct tpm_header
> > >    tpm: access command header through struct in tpm_try_transmit()
> > >    tpm: encapsulate tpm_dev_transmit()
> > >    tpm: clean up tpm_try_transmit() error handling flow
> > >    tpm: move tpm_validate_commmand() to tpm2-space.c
> > >    tpm: move TPM space code out of tpm_transmit()
> > >    tpm: remove @space from tpm_transmit()
> > >    tpm: use tpm_try_get_ops() in tpm-sysfs.c.
> > >    tpm: remove TPM_TRANSMIT_UNLOCKED flag
> > >    tpm: introduce tpm_chip_start() and tpm_chip_stop()
> > >    tpm: take TPM chip power gating out of tpm_transmit()
> > >    tpm: remove @flags from tpm_transmit()
> > > 
> > >   drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
> > >   drivers/char/tpm/tpm-dev-common.c |  44 ++++-
> > >   drivers/char/tpm/tpm-interface.c  | 264 ++++++------------------------
> > >   drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
> > >   drivers/char/tpm/tpm.h            |  64 +++-----
> > >   drivers/char/tpm/tpm1-cmd.c       |  28 +---
> > >   drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
> > >   drivers/char/tpm/tpm2-space.c     |  91 +++++++---
> > >   drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
> > >   drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
> > >   drivers/char/tpm/xen-tpmfront.c   |   2 +-
> > >   11 files changed, 408 insertions(+), 421 deletions(-)
> > > 
> > > -- 
> > > 2.19.1
> > > 
> > 
> > Applied to master and next.
> 
> Something in this series seems to break basic TPM communication for me.
> 
> For TPM2.0s the probe command fails, causing them to be misdetected as
> TPM1.2s:
> 
> ---
> tpm tpm0: tpm_try_transmit: tpm_send: error -5
> tpm_tis MSFT0101:00: 1.2 TPM (device-id 0x1A, rev-id 22)
> tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
> ---
> tpm tpm0: tpm_try_transmit: tpm_send: error -5
> tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 22)
> tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
> ---
> 
> And for something that actually is a TPM1.2 it fails in a similar way:
> 
> ---
> tpm_i2c_infineon 1-0020: 1.2 TPM (device-id 0x1A)
> tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
> ---
> tpm tpm0: tpm_try_transmit: tpm_send: error -5
> tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 16)
> tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
> tpm_tis_spi: probe of spi0.1 failed with error -14
> ---
> 
> I see this problem across my entire range of TPM devices and test platforms.
> Any idea what could be wrong here?

Weird.

Can you run a bisect?

/Jarkko

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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-07 21:29     ` Jarkko Sakkinen
@ 2019-02-07 23:29       ` Stefan Berger
  2019-02-08  0:33         ` Jarkko Sakkinen
  0 siblings, 1 reply; 42+ messages in thread
From: Stefan Berger @ 2019-02-07 23:29 UTC (permalink / raw)
  To: Jarkko Sakkinen, Alexander Steffen
  Cc: linux-integrity, linux-kernel, linux-security-module,
	Peter Huewe, Jason Gunthorpe, Tomas Winkler, Tadeusz Struk,
	Stefan Berger, Nayna Jain

On 2/7/19 4:29 PM, Jarkko Sakkinen wrote:
> On Thu, Feb 07, 2019 at 07:41:56PM +0100, Alexander Steffen wrote:
>> On 06.02.2019 13:06, Jarkko Sakkinen wrote:
>>> On Wed, Feb 06, 2019 at 12:47:07AM +0200, Jarkko Sakkinen wrote:
>>>> Make the changes necessary to detach TPM space code and TPM activation
>>>> code out of the tpm_transmit() flow because of both of these can cause
>>>> nested tpm_transmit() calls. The nesteds calls make the whole flow hard
>>>> to maintain, and thus, it is better to just fix things now before this
>>>> turns into a bigger mess.
>>>>
>>>> v11:
>>>> * Drop the patch that tries to flush TPM space on system. Not a proper
>>>>     fallback + out of scope for this patch set.
>>>>
>>>> v10:
>>>> * Use void pointers to avoid unnecessary casts in functions paramaters
>>>>     where it makes sense.
>>>>
>>>> v9:
>>>> * Fixed again tpm_try_get_ops().
>>>> * Added missing reviewed-by's.
>>>>
>>>> v8:
>>>> * Re-add the check for ret < 0 after calling tpm_try_transmit() that
>>>>     was dropped by mistake while moving code.
>>>> * Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
>>>>     fails.
>>>>
>>>> v7:
>>>> *  Reorganize series so that more trivial and self-contained changes are
>>>>      in the head.
>>>>
>>>> v6:
>>>> * When tpm_validate_commmand() was moved to tpm2-space.c, the struct for
>>>>     the TPM header was incorrectly declared as struct tpm_input_header.
>>>> * Fix return value in tpm_validate_command().
>>>>
>>>> v5:
>>>> * Add the missing rev's from Stefan Berger.
>>>>
>>>> v4:
>>>> * Return 0 from pcrs_show() when tpm1_pcr_read() fails.
>>>> * Fix error handling flow in tpm_try_transmit().
>>>> * Replace struct tpm_input_header and struct tpm_output_header with
>>>>     struct tpm_header.
>>>>
>>>> v3:
>>>> * Encapsulate power gating code to tpm_chip_start() and tpm_chip_stop().
>>>> * Move TPM power gating code and locking to tpm_try_get_ops() and
>>>>     tpm_put_ops().
>>>> * Call power gating code directly in tpm_chip_register() and
>>>>     tpm2_del_space().
>>>>
>>>> v2:
>>>> * Print tpm2_commit_space() error inside tpm2_commit_space()
>>>> * Error code was not printed when recv() callback failed. It is
>>>>     fixed in this version.
>>>> * Added a patch that removes @space from tpm_transmit().
>>>> * Fixed a regression in earlier series. Forgot to amend the change
>>>>     from the staging area that renames NESTED to UNLOCKED in tpm2-space.c.
>>>>
>>>> Jarkko Sakkinen (16):
>>>>     tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
>>>>     tpm: fix invalid return value in pubek_show()
>>>>     tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
>>>>     tpm: print tpm2_commit_space() error inside tpm2_commit_space()
>>>>     tpm: declare struct tpm_header
>>>>     tpm: access command header through struct in tpm_try_transmit()
>>>>     tpm: encapsulate tpm_dev_transmit()
>>>>     tpm: clean up tpm_try_transmit() error handling flow
>>>>     tpm: move tpm_validate_commmand() to tpm2-space.c
>>>>     tpm: move TPM space code out of tpm_transmit()
>>>>     tpm: remove @space from tpm_transmit()
>>>>     tpm: use tpm_try_get_ops() in tpm-sysfs.c.
>>>>     tpm: remove TPM_TRANSMIT_UNLOCKED flag
>>>>     tpm: introduce tpm_chip_start() and tpm_chip_stop()
>>>>     tpm: take TPM chip power gating out of tpm_transmit()
>>>>     tpm: remove @flags from tpm_transmit()
>>>>
>>>>    drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
>>>>    drivers/char/tpm/tpm-dev-common.c |  44 ++++-
>>>>    drivers/char/tpm/tpm-interface.c  | 264 ++++++------------------------
>>>>    drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
>>>>    drivers/char/tpm/tpm.h            |  64 +++-----
>>>>    drivers/char/tpm/tpm1-cmd.c       |  28 +---
>>>>    drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
>>>>    drivers/char/tpm/tpm2-space.c     |  91 +++++++---
>>>>    drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
>>>>    drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
>>>>    drivers/char/tpm/xen-tpmfront.c   |   2 +-
>>>>    11 files changed, 408 insertions(+), 421 deletions(-)
>>>>
>>>> -- 
>>>> 2.19.1
>>>>
>>> Applied to master and next.
>> Something in this series seems to break basic TPM communication for me.
>>
>> For TPM2.0s the probe command fails, causing them to be misdetected as
>> TPM1.2s:
>>
>> ---
>> tpm tpm0: tpm_try_transmit: tpm_send: error -5
>> tpm_tis MSFT0101:00: 1.2 TPM (device-id 0x1A, rev-id 22)
>> tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
>> ---
>> tpm tpm0: tpm_try_transmit: tpm_send: error -5
>> tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 22)
>> tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
>> ---
>>
>> And for something that actually is a TPM1.2 it fails in a similar way:
>>
>> ---
>> tpm_i2c_infineon 1-0020: 1.2 TPM (device-id 0x1A)
>> tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
>> ---
>> tpm tpm0: tpm_try_transmit: tpm_send: error -5
>> tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 16)
>> tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
>> tpm_tis_spi: probe of spi0.1 failed with error -14
>> ---
>>
>> I see this problem across my entire range of TPM devices and test platforms.
>> Any idea what could be wrong here?
> Weird.
>
> Can you run a bisect?

There are 2 bugs and the following overall patch against your tree fixes 
them. Let me comment on the individual patches in v17. I missed those 
obviously when testing with the tpm_vtpm_proxy...



diff --git a/drivers/char/tpm/tpm-interface.c 
b/drivers/char/tpm/tpm-interface.c
index 02e8cffd1163..34c0da55d885 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -94,6 +94,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, 
void *buf, size_t bufsiz)
          return rc;
      }

+    rc = 0;
      if (chip->flags & TPM_CHIP_FLAG_IRQ)
          goto out_recv;

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index e74c5b7b64bf..52afe20cc8a1 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -799,7 +799,9 @@ int tpm2_probe(struct tpm_chip *chip)
      tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
      tpm_buf_append_u32(&buf, TPM_PT_TOTAL_COMMANDS);
      tpm_buf_append_u32(&buf, 1);
+    tpm_chip_start(chip);
      rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
+    tpm_chip_stop(chip);
      /* We ignore TPM return codes on purpose. */
      if (rc >=  0) {
          out = (struct tpm_header *)buf.data;

>
> /Jarkko
>


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

* Re: [PATCH v11 15/16] tpm: take TPM chip power gating out of tpm_transmit()
  2019-02-05 22:47 ` [PATCH v11 15/16] tpm: take TPM chip power gating out of tpm_transmit() Jarkko Sakkinen
@ 2019-02-07 23:32   ` Stefan Berger
  2019-02-08  0:02     ` Jerry Snitselaar
  0 siblings, 1 reply; 42+ messages in thread
From: Stefan Berger @ 2019-02-07 23:32 UTC (permalink / raw)
  To: Jarkko Sakkinen, linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain

On 2/5/19 5:47 PM, Jarkko Sakkinen wrote:
> Call tpm_chip_start() and tpm_chip_stop() in
>
> * tpm_try_get_ops() and tpm_put_ops()
> * tpm_chip_register()
> * tpm2_del_space()
>
> And remove these calls from tpm_transmit(). The core reason for this
> change is that in tpm_vtpm_proxy a locality change requires a virtual
> TPM command (a command made up just for that driver).
>
> The consequence of this is that this commit removes the remaining nested
> calls.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> Tested-by: Stefan Berger <stefanb@linux.ibm.com>
> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>   drivers/char/tpm/tpm-chip.c       | 25 ++++++++++++-------------
>   drivers/char/tpm/tpm-interface.c  |  6 ------
>   drivers/char/tpm/tpm.h            |  9 ---------
>   drivers/char/tpm/tpm2-space.c     |  5 ++++-
>   drivers/char/tpm/tpm_vtpm_proxy.c |  3 +--
>   5 files changed, 17 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 65f1561eba81..7ad4d9045e4c 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -41,9 +41,6 @@ static int tpm_request_locality(struct tpm_chip *chip, unsigned int flags)
>   {
>   	int rc;
>
> -	if (flags & TPM_TRANSMIT_NESTED)
> -		return 0;
> -
>   	if (!chip->ops->request_locality)
>   		return 0;
>
> @@ -59,9 +56,6 @@ static void tpm_relinquish_locality(struct tpm_chip *chip, unsigned int flags)
>   {
>   	int rc;
>
> -	if (flags & TPM_TRANSMIT_NESTED)
> -		return;
> -
>   	if (!chip->ops->relinquish_locality)
>   		return;
>
> @@ -74,9 +68,6 @@ static void tpm_relinquish_locality(struct tpm_chip *chip, unsigned int flags)
>
>   static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags)
>   {
> -	if (flags & TPM_TRANSMIT_NESTED)
> -		return 0;
> -
>   	if (!chip->ops->cmd_ready)
>   		return 0;
>
> @@ -85,9 +76,6 @@ static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags)
>
>   static int tpm_go_idle(struct tpm_chip *chip, unsigned int flags)
>   {
> -	if (flags & TPM_TRANSMIT_NESTED)
> -		return 0;
> -
>   	if (!chip->ops->go_idle)
>   		return 0;
>
> @@ -166,11 +154,17 @@ int tpm_try_get_ops(struct tpm_chip *chip)
>
>   	down_read(&chip->ops_sem);
>   	if (!chip->ops)
> -		goto out_lock;
> +		goto out_ops;
>
>   	mutex_lock(&chip->tpm_mutex);
> +	rc = tpm_chip_start(chip, 0);
> +	if (rc)
> +		goto out_lock;
> +
>   	return 0;
>   out_lock:
> +	mutex_unlock(&chip->tpm_mutex);
> +out_ops:
>   	up_read(&chip->ops_sem);
>   	put_device(&chip->dev);
>   	return rc;
> @@ -186,6 +180,7 @@ EXPORT_SYMBOL_GPL(tpm_try_get_ops);
>    */
>   void tpm_put_ops(struct tpm_chip *chip)
>   {
> +	tpm_chip_stop(chip, 0);
>   	mutex_unlock(&chip->tpm_mutex);
>   	up_read(&chip->ops_sem);
>   	put_device(&chip->dev);
> @@ -563,7 +558,11 @@ int tpm_chip_register(struct tpm_chip *chip)
>   {
>   	int rc;
>
> +	rc = tpm_chip_start(chip, 0);
> +	if (rc)
> +		return rc;
>   	rc = tpm_auto_startup(chip);
> +	tpm_chip_stop(chip, 0);
>   	if (rc)
>   		return rc;
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index f7b7e4e75fcf..f20c78055731 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -168,13 +168,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
>   	memcpy(save, buf, save_size);
>
>   	for (;;) {
> -		ret = tpm_chip_start(chip, flags);
> -		if (ret)
> -			return ret;
> -
>   		ret = tpm_try_transmit(chip, buf, bufsiz, flags);
> -
> -		tpm_chip_stop(chip, flags);
>   		if (ret < 0)
>   			break;
>   		rc = be32_to_cpu(header->return_code);
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 2d6d934f1c8b..53e4208759ee 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -485,15 +485,6 @@ extern const struct file_operations tpm_fops;
>   extern const struct file_operations tpmrm_fops;
>   extern struct idr dev_nums_idr;
>
> -/**
> - * enum tpm_transmit_flags - flags for tpm_transmit()
> - *
> - * %TPM_TRANSMIT_NESTED:	discard setup steps (power management, locality)
> - */
> -enum tpm_transmit_flags {
> -	TPM_TRANSMIT_NESTED      = BIT(0),
> -};
> -
>   ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
>   		     unsigned int flags);
>   ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf,
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index 9c9ccf2c0681..6c6ad2d4d31b 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -60,7 +60,10 @@ int tpm2_init_space(struct tpm_space *space)
>   void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
>   {
>   	mutex_lock(&chip->tpm_mutex);
> -	tpm2_flush_sessions(chip, space);
> +	if (!tpm_chip_start(chip, 0)) {
> +		tpm2_flush_sessions(chip, space);
> +		tpm_chip_stop(chip, 0);
> +	}
>   	mutex_unlock(&chip->tpm_mutex);
>   	kfree(space->context_buf);
>   	kfree(space->session_buf);
> diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
> index e8a1da2810a9..a4bb60e163cc 100644
> --- a/drivers/char/tpm/tpm_vtpm_proxy.c
> +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> @@ -417,8 +417,7 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
>
>   	proxy_dev->state |= STATE_DRIVER_COMMAND;
>
> -	rc = tpm_transmit_cmd(chip, &buf, 0, TPM_TRANSMIT_NESTED,
> -			      "attempting to set locality");
> +	rc = tpm_transmit_cmd(chip, &buf, 0, 0, "attempting to set locality");
>
>   	proxy_dev->state &= ~STATE_DRIVER_COMMAND;
>
This patch seems to be missing a hunk along these lines here

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index e74c5b7b64bf..52afe20cc8a1 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -799,7 +799,9 @@ int tpm2_probe(struct tpm_chip *chip)
      tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
      tpm_buf_append_u32(&buf, TPM_PT_TOTAL_COMMANDS);
      tpm_buf_append_u32(&buf, 1);
+    tpm_chip_start(chip, 0);
      rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
+    tpm_chip_stop(chip, 0);
      /* We ignore TPM return codes on purpose. */
      if (rc >=  0) {
          out = (struct tpm_header *)buf.data;


Of course you need to check the error from tpm_chip_start().


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

* Re: [PATCH v11 08/16] tpm: clean up tpm_try_transmit() error handling flow
  2019-02-05 22:47 ` [PATCH v11 08/16] tpm: clean up tpm_try_transmit() error handling flow Jarkko Sakkinen
@ 2019-02-07 23:36   ` Stefan Berger
  0 siblings, 0 replies; 42+ messages in thread
From: Stefan Berger @ 2019-02-07 23:36 UTC (permalink / raw)
  To: Jarkko Sakkinen, linux-integrity
  Cc: linux-kernel, linux-security-module, Peter Huewe,
	Jason Gunthorpe, Tomas Winkler, Tadeusz Struk, Stefan Berger,
	Nayna Jain

On 2/5/19 5:47 PM, Jarkko Sakkinen wrote:
> Move locking, locality handling and power management to tpm_transmit()
> in order to simplify the flow.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> Tested-by: Stefan Berger <stefanb@linux.ibm.com>
> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>   drivers/char/tpm/tpm-interface.c | 94 +++++++++++++++-----------------
>   drivers/char/tpm/tpm.h           |  1 +
>   drivers/char/tpm/tpm2-space.c    |  2 +-
>   3 files changed, 45 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index c28ffef92f1a..f5f5224f68b0 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -167,7 +167,6 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
>   	ssize_t len = 0;
>   	u32 count, ordinal;
>   	unsigned long stop;
> -	bool need_locality;
>
>   	rc = tpm_validate_command(chip, space, buf, bufsiz);
>   	if (rc == -EINVAL)
> @@ -197,37 +196,16 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
>   		return -E2BIG;
>   	}
>
> -	if (!(flags & TPM_TRANSMIT_UNLOCKED) && !(flags & TPM_TRANSMIT_NESTED))
> -		mutex_lock(&chip->tpm_mutex);
> -
> -	if (chip->ops->clk_enable != NULL)
> -		chip->ops->clk_enable(chip, true);
> -
> -	/* Store the decision as chip->locality will be changed. */
> -	need_locality = chip->locality == -1;
> -
> -	if (need_locality) {
> -		rc = tpm_request_locality(chip, flags);
> -		if (rc < 0) {
> -			need_locality = false;
> -			goto out_locality;
> -		}
> -	}
> -
> -	rc = tpm_cmd_ready(chip, flags);
> -	if (rc)
> -		goto out_locality;
> -
>   	rc = tpm2_prepare_space(chip, space, ordinal, buf);
>   	if (rc)
> -		goto out;
> +		return rc;
>
>   	rc = chip->ops->send(chip, buf, count);
>   	if (rc < 0) {
>   		if (rc != -EPIPE)
>   			dev_err(&chip->dev,
>   				"%s: tpm_send: error %d\n", __func__, rc);
> -		goto out;
> +		goto out_rc;
>   	}
>
>   	if (chip->flags & TPM_CHIP_FLAG_IRQ)
> @@ -243,7 +221,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
>   		if (chip->ops->req_canceled(chip, status)) {
>   			dev_err(&chip->dev, "Operation Canceled\n");
>   			rc = -ECANCELED;
> -			goto out;
> +			goto out_rc;
>   		}
>
>   		tpm_msleep(TPM_TIMEOUT_POLL);
> @@ -253,40 +231,20 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
>   	chip->ops->cancel(chip);
>   	dev_err(&chip->dev, "Operation Timed out\n");
>   	rc = -ETIME;
> -	goto out;
> +	goto out_rc;
>
>   out_recv:
>   	len = chip->ops->recv(chip, buf, bufsiz);
>   	if (len < 0) {
>   		rc = len;
> -		dev_err(&chip->dev,
> -			"tpm_transmit: tpm_recv: error %d\n", rc);
> -		goto out;
> -	} else if (len < TPM_HEADER_SIZE) {
> +		dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
> +	} else if (len < TPM_HEADER_SIZE || len != be32_to_cpu(header->length))
>   		rc = -EFAULT;
> -		goto out;
> -	}


Add an else branch here for 'rc = 0' because it is otherwise set from 
'rc = chip->ops->send(chip, buf, count);'. Or add it further above...



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

* Re: [PATCH v11 15/16] tpm: take TPM chip power gating out of tpm_transmit()
  2019-02-07 23:32   ` Stefan Berger
@ 2019-02-08  0:02     ` Jerry Snitselaar
  0 siblings, 0 replies; 42+ messages in thread
From: Jerry Snitselaar @ 2019-02-08  0:02 UTC (permalink / raw)
  To: Stefan Berger
  Cc: Jarkko Sakkinen, linux-integrity, linux-kernel,
	linux-security-module, Peter Huewe, Jason Gunthorpe,
	Tomas Winkler, Tadeusz Struk, Stefan Berger, Nayna Jain

On Thu Feb 07 19, Stefan Berger wrote:
>On 2/5/19 5:47 PM, Jarkko Sakkinen wrote:
>>Call tpm_chip_start() and tpm_chip_stop() in
>>
>>* tpm_try_get_ops() and tpm_put_ops()
>>* tpm_chip_register()
>>* tpm2_del_space()
>>
>>And remove these calls from tpm_transmit(). The core reason for this
>>change is that in tpm_vtpm_proxy a locality change requires a virtual
>>TPM command (a command made up just for that driver).
>>
>>The consequence of this is that this commit removes the remaining nested
>>calls.
>>
>>Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>>Tested-by: Stefan Berger <stefanb@linux.ibm.com>
>>Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
>>Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>>---
>>  drivers/char/tpm/tpm-chip.c       | 25 ++++++++++++-------------
>>  drivers/char/tpm/tpm-interface.c  |  6 ------
>>  drivers/char/tpm/tpm.h            |  9 ---------
>>  drivers/char/tpm/tpm2-space.c     |  5 ++++-
>>  drivers/char/tpm/tpm_vtpm_proxy.c |  3 +--
>>  5 files changed, 17 insertions(+), 31 deletions(-)
>>
>>diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>>index 65f1561eba81..7ad4d9045e4c 100644
>>--- a/drivers/char/tpm/tpm-chip.c
>>+++ b/drivers/char/tpm/tpm-chip.c
>>@@ -41,9 +41,6 @@ static int tpm_request_locality(struct tpm_chip *chip, unsigned int flags)
>>  {
>>  	int rc;
>>
>>-	if (flags & TPM_TRANSMIT_NESTED)
>>-		return 0;
>>-
>>  	if (!chip->ops->request_locality)
>>  		return 0;
>>
>>@@ -59,9 +56,6 @@ static void tpm_relinquish_locality(struct tpm_chip *chip, unsigned int flags)
>>  {
>>  	int rc;
>>
>>-	if (flags & TPM_TRANSMIT_NESTED)
>>-		return;
>>-
>>  	if (!chip->ops->relinquish_locality)
>>  		return;
>>
>>@@ -74,9 +68,6 @@ static void tpm_relinquish_locality(struct tpm_chip *chip, unsigned int flags)
>>
>>  static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags)
>>  {
>>-	if (flags & TPM_TRANSMIT_NESTED)
>>-		return 0;
>>-
>>  	if (!chip->ops->cmd_ready)
>>  		return 0;
>>
>>@@ -85,9 +76,6 @@ static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags)
>>
>>  static int tpm_go_idle(struct tpm_chip *chip, unsigned int flags)
>>  {
>>-	if (flags & TPM_TRANSMIT_NESTED)
>>-		return 0;
>>-
>>  	if (!chip->ops->go_idle)
>>  		return 0;
>>
>>@@ -166,11 +154,17 @@ int tpm_try_get_ops(struct tpm_chip *chip)
>>
>>  	down_read(&chip->ops_sem);
>>  	if (!chip->ops)
>>-		goto out_lock;
>>+		goto out_ops;
>>
>>  	mutex_lock(&chip->tpm_mutex);
>>+	rc = tpm_chip_start(chip, 0);
>>+	if (rc)
>>+		goto out_lock;
>>+
>>  	return 0;
>>  out_lock:
>>+	mutex_unlock(&chip->tpm_mutex);
>>+out_ops:
>>  	up_read(&chip->ops_sem);
>>  	put_device(&chip->dev);
>>  	return rc;
>>@@ -186,6 +180,7 @@ EXPORT_SYMBOL_GPL(tpm_try_get_ops);
>>   */
>>  void tpm_put_ops(struct tpm_chip *chip)
>>  {
>>+	tpm_chip_stop(chip, 0);
>>  	mutex_unlock(&chip->tpm_mutex);
>>  	up_read(&chip->ops_sem);
>>  	put_device(&chip->dev);
>>@@ -563,7 +558,11 @@ int tpm_chip_register(struct tpm_chip *chip)
>>  {
>>  	int rc;
>>
>>+	rc = tpm_chip_start(chip, 0);
>>+	if (rc)
>>+		return rc;
>>  	rc = tpm_auto_startup(chip);
>>+	tpm_chip_stop(chip, 0);
>>  	if (rc)
>>  		return rc;
>>
>>diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>>index f7b7e4e75fcf..f20c78055731 100644
>>--- a/drivers/char/tpm/tpm-interface.c
>>+++ b/drivers/char/tpm/tpm-interface.c
>>@@ -168,13 +168,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
>>  	memcpy(save, buf, save_size);
>>
>>  	for (;;) {
>>-		ret = tpm_chip_start(chip, flags);
>>-		if (ret)
>>-			return ret;
>>-
>>  		ret = tpm_try_transmit(chip, buf, bufsiz, flags);
>>-
>>-		tpm_chip_stop(chip, flags);
>>  		if (ret < 0)
>>  			break;
>>  		rc = be32_to_cpu(header->return_code);
>>diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>>index 2d6d934f1c8b..53e4208759ee 100644
>>--- a/drivers/char/tpm/tpm.h
>>+++ b/drivers/char/tpm/tpm.h
>>@@ -485,15 +485,6 @@ extern const struct file_operations tpm_fops;
>>  extern const struct file_operations tpmrm_fops;
>>  extern struct idr dev_nums_idr;
>>
>>-/**
>>- * enum tpm_transmit_flags - flags for tpm_transmit()
>>- *
>>- * %TPM_TRANSMIT_NESTED:	discard setup steps (power management, locality)
>>- */
>>-enum tpm_transmit_flags {
>>-	TPM_TRANSMIT_NESTED      = BIT(0),
>>-};
>>-
>>  ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
>>  		     unsigned int flags);
>>  ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf,
>>diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
>>index 9c9ccf2c0681..6c6ad2d4d31b 100644
>>--- a/drivers/char/tpm/tpm2-space.c
>>+++ b/drivers/char/tpm/tpm2-space.c
>>@@ -60,7 +60,10 @@ int tpm2_init_space(struct tpm_space *space)
>>  void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
>>  {
>>  	mutex_lock(&chip->tpm_mutex);
>>-	tpm2_flush_sessions(chip, space);
>>+	if (!tpm_chip_start(chip, 0)) {
>>+		tpm2_flush_sessions(chip, space);
>>+		tpm_chip_stop(chip, 0);
>>+	}
>>  	mutex_unlock(&chip->tpm_mutex);
>>  	kfree(space->context_buf);
>>  	kfree(space->session_buf);
>>diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
>>index e8a1da2810a9..a4bb60e163cc 100644
>>--- a/drivers/char/tpm/tpm_vtpm_proxy.c
>>+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
>>@@ -417,8 +417,7 @@ static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
>>
>>  	proxy_dev->state |= STATE_DRIVER_COMMAND;
>>
>>-	rc = tpm_transmit_cmd(chip, &buf, 0, TPM_TRANSMIT_NESTED,
>>-			      "attempting to set locality");
>>+	rc = tpm_transmit_cmd(chip, &buf, 0, 0, "attempting to set locality");
>>
>>  	proxy_dev->state &= ~STATE_DRIVER_COMMAND;
>>
>This patch seems to be missing a hunk along these lines here
>
>diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>index e74c5b7b64bf..52afe20cc8a1 100644
>--- a/drivers/char/tpm/tpm2-cmd.c
>+++ b/drivers/char/tpm/tpm2-cmd.c
>@@ -799,7 +799,9 @@ int tpm2_probe(struct tpm_chip *chip)
>     tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
>     tpm_buf_append_u32(&buf, TPM_PT_TOTAL_COMMANDS);
>     tpm_buf_append_u32(&buf, 1);
>+    tpm_chip_start(chip, 0);
>     rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
>+    tpm_chip_stop(chip, 0);
>     /* We ignore TPM return codes on purpose. */
>     if (rc >=  0) {
>         out = (struct tpm_header *)buf.data;
>
>
>Of course you need to check the error from tpm_chip_start().
>

Reproduced here with discrete chip. Will test fix in a bit.

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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-07 23:29       ` Stefan Berger
@ 2019-02-08  0:33         ` Jarkko Sakkinen
  2019-02-08  1:51           ` Stefan Berger
  0 siblings, 1 reply; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-08  0:33 UTC (permalink / raw)
  To: Stefan Berger
  Cc: Alexander Steffen, linux-integrity, linux-kernel,
	linux-security-module, Peter Huewe, Jason Gunthorpe,
	Tomas Winkler, Tadeusz Struk, Stefan Berger, Nayna Jain

On Thu, Feb 07, 2019 at 06:29:43PM -0500, Stefan Berger wrote:
> On 2/7/19 4:29 PM, Jarkko Sakkinen wrote:
> > On Thu, Feb 07, 2019 at 07:41:56PM +0100, Alexander Steffen wrote:
> > > On 06.02.2019 13:06, Jarkko Sakkinen wrote:
> > > > On Wed, Feb 06, 2019 at 12:47:07AM +0200, Jarkko Sakkinen wrote:
> > > > > Make the changes necessary to detach TPM space code and TPM activation
> > > > > code out of the tpm_transmit() flow because of both of these can cause
> > > > > nested tpm_transmit() calls. The nesteds calls make the whole flow hard
> > > > > to maintain, and thus, it is better to just fix things now before this
> > > > > turns into a bigger mess.
> > > > > 
> > > > > v11:
> > > > > * Drop the patch that tries to flush TPM space on system. Not a proper
> > > > >     fallback + out of scope for this patch set.
> > > > > 
> > > > > v10:
> > > > > * Use void pointers to avoid unnecessary casts in functions paramaters
> > > > >     where it makes sense.
> > > > > 
> > > > > v9:
> > > > > * Fixed again tpm_try_get_ops().
> > > > > * Added missing reviewed-by's.
> > > > > 
> > > > > v8:
> > > > > * Re-add the check for ret < 0 after calling tpm_try_transmit() that
> > > > >     was dropped by mistake while moving code.
> > > > > * Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
> > > > >     fails.
> > > > > 
> > > > > v7:
> > > > > *  Reorganize series so that more trivial and self-contained changes are
> > > > >      in the head.
> > > > > 
> > > > > v6:
> > > > > * When tpm_validate_commmand() was moved to tpm2-space.c, the struct for
> > > > >     the TPM header was incorrectly declared as struct tpm_input_header.
> > > > > * Fix return value in tpm_validate_command().
> > > > > 
> > > > > v5:
> > > > > * Add the missing rev's from Stefan Berger.
> > > > > 
> > > > > v4:
> > > > > * Return 0 from pcrs_show() when tpm1_pcr_read() fails.
> > > > > * Fix error handling flow in tpm_try_transmit().
> > > > > * Replace struct tpm_input_header and struct tpm_output_header with
> > > > >     struct tpm_header.
> > > > > 
> > > > > v3:
> > > > > * Encapsulate power gating code to tpm_chip_start() and tpm_chip_stop().
> > > > > * Move TPM power gating code and locking to tpm_try_get_ops() and
> > > > >     tpm_put_ops().
> > > > > * Call power gating code directly in tpm_chip_register() and
> > > > >     tpm2_del_space().
> > > > > 
> > > > > v2:
> > > > > * Print tpm2_commit_space() error inside tpm2_commit_space()
> > > > > * Error code was not printed when recv() callback failed. It is
> > > > >     fixed in this version.
> > > > > * Added a patch that removes @space from tpm_transmit().
> > > > > * Fixed a regression in earlier series. Forgot to amend the change
> > > > >     from the staging area that renames NESTED to UNLOCKED in tpm2-space.c.
> > > > > 
> > > > > Jarkko Sakkinen (16):
> > > > >     tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
> > > > >     tpm: fix invalid return value in pubek_show()
> > > > >     tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
> > > > >     tpm: print tpm2_commit_space() error inside tpm2_commit_space()
> > > > >     tpm: declare struct tpm_header
> > > > >     tpm: access command header through struct in tpm_try_transmit()
> > > > >     tpm: encapsulate tpm_dev_transmit()
> > > > >     tpm: clean up tpm_try_transmit() error handling flow
> > > > >     tpm: move tpm_validate_commmand() to tpm2-space.c
> > > > >     tpm: move TPM space code out of tpm_transmit()
> > > > >     tpm: remove @space from tpm_transmit()
> > > > >     tpm: use tpm_try_get_ops() in tpm-sysfs.c.
> > > > >     tpm: remove TPM_TRANSMIT_UNLOCKED flag
> > > > >     tpm: introduce tpm_chip_start() and tpm_chip_stop()
> > > > >     tpm: take TPM chip power gating out of tpm_transmit()
> > > > >     tpm: remove @flags from tpm_transmit()
> > > > > 
> > > > >    drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
> > > > >    drivers/char/tpm/tpm-dev-common.c |  44 ++++-
> > > > >    drivers/char/tpm/tpm-interface.c  | 264 ++++++------------------------
> > > > >    drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
> > > > >    drivers/char/tpm/tpm.h            |  64 +++-----
> > > > >    drivers/char/tpm/tpm1-cmd.c       |  28 +---
> > > > >    drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
> > > > >    drivers/char/tpm/tpm2-space.c     |  91 +++++++---
> > > > >    drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
> > > > >    drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
> > > > >    drivers/char/tpm/xen-tpmfront.c   |   2 +-
> > > > >    11 files changed, 408 insertions(+), 421 deletions(-)
> > > > > 
> > > > > -- 
> > > > > 2.19.1
> > > > > 
> > > > Applied to master and next.
> > > Something in this series seems to break basic TPM communication for me.
> > > 
> > > For TPM2.0s the probe command fails, causing them to be misdetected as
> > > TPM1.2s:
> > > 
> > > ---
> > > tpm tpm0: tpm_try_transmit: tpm_send: error -5
> > > tpm_tis MSFT0101:00: 1.2 TPM (device-id 0x1A, rev-id 22)
> > > tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
> > > ---
> > > tpm tpm0: tpm_try_transmit: tpm_send: error -5
> > > tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 22)
> > > tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
> > > ---
> > > 
> > > And for something that actually is a TPM1.2 it fails in a similar way:
> > > 
> > > ---
> > > tpm_i2c_infineon 1-0020: 1.2 TPM (device-id 0x1A)
> > > tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
> > > ---
> > > tpm tpm0: tpm_try_transmit: tpm_send: error -5
> > > tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 16)
> > > tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
> > > tpm_tis_spi: probe of spi0.1 failed with error -14
> > > ---
> > > 
> > > I see this problem across my entire range of TPM devices and test platforms.
> > > Any idea what could be wrong here?
> > Weird.
> > 
> > Can you run a bisect?
> 
> There are 2 bugs and the following overall patch against your tree fixes
> them. Let me comment on the individual patches in v17. I missed those
> obviously when testing with the tpm_vtpm_proxy...
> 
> 
> 
> diff --git a/drivers/char/tpm/tpm-interface.c
> b/drivers/char/tpm/tpm-interface.c
> index 02e8cffd1163..34c0da55d885 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -94,6 +94,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
> void *buf, size_t bufsiz)
>          return rc;
>      }
> 
> +    rc = 0;
>      if (chip->flags & TPM_CHIP_FLAG_IRQ)
>          goto out_recv;

What why?

> 
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index e74c5b7b64bf..52afe20cc8a1 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -799,7 +799,9 @@ int tpm2_probe(struct tpm_chip *chip)
>      tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
>      tpm_buf_append_u32(&buf, TPM_PT_TOTAL_COMMANDS);
>      tpm_buf_append_u32(&buf, 1);
> +    tpm_chip_start(chip);
>      rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
> +    tpm_chip_stop(chip);

Thanks Stefan! I added call to tpm_tis_core as tpm2-cmd.c is to be kept
out of chip management common case being that you call tpm_try_get_ops(),
do 1-N TPM commands and release with tpm_put_ops(). These functions take
care starting and stopping the chip.

I fixed the 2nd issue in the master.

 /Jarkko

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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-08  0:33         ` Jarkko Sakkinen
@ 2019-02-08  1:51           ` Stefan Berger
  2019-02-08  2:14             ` Stefan Berger
  2019-02-08 11:14             ` Jarkko Sakkinen
  0 siblings, 2 replies; 42+ messages in thread
From: Stefan Berger @ 2019-02-08  1:51 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Alexander Steffen, linux-integrity, linux-kernel,
	linux-security-module, Peter Huewe, Jason Gunthorpe,
	Tomas Winkler, Tadeusz Struk, Stefan Berger, Nayna Jain

On 2/7/19 7:33 PM, Jarkko Sakkinen wrote:
> On Thu, Feb 07, 2019 at 06:29:43PM -0500, Stefan Berger wrote:
>> On 2/7/19 4:29 PM, Jarkko Sakkinen wrote:
>>> On Thu, Feb 07, 2019 at 07:41:56PM +0100, Alexander Steffen wrote:
>>>> On 06.02.2019 13:06, Jarkko Sakkinen wrote:
>>>>> On Wed, Feb 06, 2019 at 12:47:07AM +0200, Jarkko Sakkinen wrote:
>>>>>> Make the changes necessary to detach TPM space code and TPM activation
>>>>>> code out of the tpm_transmit() flow because of both of these can cause
>>>>>> nested tpm_transmit() calls. The nesteds calls make the whole flow hard
>>>>>> to maintain, and thus, it is better to just fix things now before this
>>>>>> turns into a bigger mess.
>>>>>>
>>>>>> v11:
>>>>>> * Drop the patch that tries to flush TPM space on system. Not a proper
>>>>>>      fallback + out of scope for this patch set.
>>>>>>
>>>>>> v10:
>>>>>> * Use void pointers to avoid unnecessary casts in functions paramaters
>>>>>>      where it makes sense.
>>>>>>
>>>>>> v9:
>>>>>> * Fixed again tpm_try_get_ops().
>>>>>> * Added missing reviewed-by's.
>>>>>>
>>>>>> v8:
>>>>>> * Re-add the check for ret < 0 after calling tpm_try_transmit() that
>>>>>>      was dropped by mistake while moving code.
>>>>>> * Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
>>>>>>      fails.
>>>>>>
>>>>>> v7:
>>>>>> *  Reorganize series so that more trivial and self-contained changes are
>>>>>>       in the head.
>>>>>>
>>>>>> v6:
>>>>>> * When tpm_validate_commmand() was moved to tpm2-space.c, the struct for
>>>>>>      the TPM header was incorrectly declared as struct tpm_input_header.
>>>>>> * Fix return value in tpm_validate_command().
>>>>>>
>>>>>> v5:
>>>>>> * Add the missing rev's from Stefan Berger.
>>>>>>
>>>>>> v4:
>>>>>> * Return 0 from pcrs_show() when tpm1_pcr_read() fails.
>>>>>> * Fix error handling flow in tpm_try_transmit().
>>>>>> * Replace struct tpm_input_header and struct tpm_output_header with
>>>>>>      struct tpm_header.
>>>>>>
>>>>>> v3:
>>>>>> * Encapsulate power gating code to tpm_chip_start() and tpm_chip_stop().
>>>>>> * Move TPM power gating code and locking to tpm_try_get_ops() and
>>>>>>      tpm_put_ops().
>>>>>> * Call power gating code directly in tpm_chip_register() and
>>>>>>      tpm2_del_space().
>>>>>>
>>>>>> v2:
>>>>>> * Print tpm2_commit_space() error inside tpm2_commit_space()
>>>>>> * Error code was not printed when recv() callback failed. It is
>>>>>>      fixed in this version.
>>>>>> * Added a patch that removes @space from tpm_transmit().
>>>>>> * Fixed a regression in earlier series. Forgot to amend the change
>>>>>>      from the staging area that renames NESTED to UNLOCKED in tpm2-space.c.
>>>>>>
>>>>>> Jarkko Sakkinen (16):
>>>>>>      tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
>>>>>>      tpm: fix invalid return value in pubek_show()
>>>>>>      tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
>>>>>>      tpm: print tpm2_commit_space() error inside tpm2_commit_space()
>>>>>>      tpm: declare struct tpm_header
>>>>>>      tpm: access command header through struct in tpm_try_transmit()
>>>>>>      tpm: encapsulate tpm_dev_transmit()
>>>>>>      tpm: clean up tpm_try_transmit() error handling flow
>>>>>>      tpm: move tpm_validate_commmand() to tpm2-space.c
>>>>>>      tpm: move TPM space code out of tpm_transmit()
>>>>>>      tpm: remove @space from tpm_transmit()
>>>>>>      tpm: use tpm_try_get_ops() in tpm-sysfs.c.
>>>>>>      tpm: remove TPM_TRANSMIT_UNLOCKED flag
>>>>>>      tpm: introduce tpm_chip_start() and tpm_chip_stop()
>>>>>>      tpm: take TPM chip power gating out of tpm_transmit()
>>>>>>      tpm: remove @flags from tpm_transmit()
>>>>>>
>>>>>>     drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
>>>>>>     drivers/char/tpm/tpm-dev-common.c |  44 ++++-
>>>>>>     drivers/char/tpm/tpm-interface.c  | 264 ++++++------------------------
>>>>>>     drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
>>>>>>     drivers/char/tpm/tpm.h            |  64 +++-----
>>>>>>     drivers/char/tpm/tpm1-cmd.c       |  28 +---
>>>>>>     drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
>>>>>>     drivers/char/tpm/tpm2-space.c     |  91 +++++++---
>>>>>>     drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
>>>>>>     drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
>>>>>>     drivers/char/tpm/xen-tpmfront.c   |   2 +-
>>>>>>     11 files changed, 408 insertions(+), 421 deletions(-)
>>>>>>
>>>>>> -- 
>>>>>> 2.19.1
>>>>>>
>>>>> Applied to master and next.
>>>> Something in this series seems to break basic TPM communication for me.
>>>>
>>>> For TPM2.0s the probe command fails, causing them to be misdetected as
>>>> TPM1.2s:
>>>>
>>>> ---
>>>> tpm tpm0: tpm_try_transmit: tpm_send: error -5
>>>> tpm_tis MSFT0101:00: 1.2 TPM (device-id 0x1A, rev-id 22)
>>>> tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
>>>> ---
>>>> tpm tpm0: tpm_try_transmit: tpm_send: error -5
>>>> tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 22)
>>>> tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
>>>> ---
>>>>
>>>> And for something that actually is a TPM1.2 it fails in a similar way:
>>>>
>>>> ---
>>>> tpm_i2c_infineon 1-0020: 1.2 TPM (device-id 0x1A)
>>>> tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
>>>> ---
>>>> tpm tpm0: tpm_try_transmit: tpm_send: error -5
>>>> tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 16)
>>>> tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
>>>> tpm_tis_spi: probe of spi0.1 failed with error -14
>>>> ---
>>>>
>>>> I see this problem across my entire range of TPM devices and test platforms.
>>>> Any idea what could be wrong here?
>>> Weird.
>>>
>>> Can you run a bisect?
>> There are 2 bugs and the following overall patch against your tree fixes
>> them. Let me comment on the individual patches in v17. I missed those
>> obviously when testing with the tpm_vtpm_proxy...
>>
>>
>>
>> diff --git a/drivers/char/tpm/tpm-interface.c
>> b/drivers/char/tpm/tpm-interface.c
>> index 02e8cffd1163..34c0da55d885 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -94,6 +94,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
>> void *buf, size_t bufsiz)
>>           return rc;
>>       }
>>
>> +    rc = 0;
>>       if (chip->flags & TPM_CHIP_FLAG_IRQ)
>>           goto out_recv;
> What why?


This fix seems to only be necessary when bisecting. You may want to 
apply it!


>
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index e74c5b7b64bf..52afe20cc8a1 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -799,7 +799,9 @@ int tpm2_probe(struct tpm_chip *chip)
>>       tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
>>       tpm_buf_append_u32(&buf, TPM_PT_TOTAL_COMMANDS);
>>       tpm_buf_append_u32(&buf, 1);
>> +    tpm_chip_start(chip);
>>       rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
>> +    tpm_chip_stop(chip);
> Thanks Stefan! I added call to tpm_tis_core as tpm2-cmd.c is to be kept
> out of chip management common case being that you call tpm_try_get_ops(),
> do 1-N TPM commands and release with tpm_put_ops(). These functions take
> care starting and stopping the chip.
>
> I fixed the 2nd issue in the master.

You also need to export the tpm_chip_start/stop symbols.


Master now seems to (still) have a problem when rmmod'ing:

A TPM error (325) occurred stopping the TPM. This happens at the same 
patch where I had to add the tpm_chip_start/stop above.




>
>   /Jarkko
>


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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-08  1:51           ` Stefan Berger
@ 2019-02-08  2:14             ` Stefan Berger
  2019-02-08 11:50               ` Jarkko Sakkinen
  2019-02-08 13:28               ` Alexander Steffen
  2019-02-08 11:14             ` Jarkko Sakkinen
  1 sibling, 2 replies; 42+ messages in thread
From: Stefan Berger @ 2019-02-08  2:14 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Alexander Steffen, linux-integrity, linux-kernel,
	linux-security-module, Peter Huewe, Jason Gunthorpe,
	Tomas Winkler, Tadeusz Struk, Stefan Berger, Nayna Jain

On 2/7/19 8:51 PM, Stefan Berger wrote:
> On 2/7/19 7:33 PM, Jarkko Sakkinen wrote:
>> On Thu, Feb 07, 2019 at 06:29:43PM -0500, Stefan Berger wrote:
>>> On 2/7/19 4:29 PM, Jarkko Sakkinen wrote:
>
>
>>
>>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>>> index e74c5b7b64bf..52afe20cc8a1 100644
>>> --- a/drivers/char/tpm/tpm2-cmd.c
>>> +++ b/drivers/char/tpm/tpm2-cmd.c
>>> @@ -799,7 +799,9 @@ int tpm2_probe(struct tpm_chip *chip)
>>>       tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
>>>       tpm_buf_append_u32(&buf, TPM_PT_TOTAL_COMMANDS);
>>>       tpm_buf_append_u32(&buf, 1);
>>> +    tpm_chip_start(chip);
>>>       rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
>>> +    tpm_chip_stop(chip);
>> Thanks Stefan! I added call to tpm_tis_core as tpm2-cmd.c is to be kept
>> out of chip management common case being that you call 
>> tpm_try_get_ops(),
>> do 1-N TPM commands and release with tpm_put_ops(). These functions take
>> care starting and stopping the chip.
>>
>> I fixed the 2nd issue in the master.
>
> You also need to export the tpm_chip_start/stop symbols.
>
>
> Master now seems to (still) have a problem when rmmod'ing:
>
> A TPM error (325) occurred stopping the TPM. This happens at the same 
> patch where I had to add the tpm_chip_start/stop above.
>
>

Here's my current overall patch against your master. I suppose the other 
tpm2_shutdown() for power management doesn't need the chip start ?

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 9865776ee2cd..f7147706a9c3 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -115,6 +115,7 @@ int tpm_chip_start(struct tpm_chip *chip)

      return 0;
  }
+EXPORT_SYMBOL_GPL(tpm_chip_start);

  /**
   * tpm_chip_stop() - power off the TPM
@@ -131,7 +132,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
      if (chip->ops->clk_enable)
          chip->ops->clk_enable(chip, false);
  }
-
+EXPORT_SYMBOL_GPL(tpm_chip_stop);

  /**
   * tpm_try_get_ops() - Get a ref to the tpm_chip
@@ -474,8 +475,11 @@ static void tpm_del_char_device(struct tpm_chip *chip)

      /* Make the driver uncallable. */
      down_write(&chip->ops_sem);
-    if (chip->flags & TPM_CHIP_FLAG_TPM2)
+    if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+        tpm_chip_start(chip, 0);
          tpm2_shutdown(chip, TPM2_SU_CLEAR);
+        tpm_chip_stop(chip, 0);
+    }
      chip->ops = NULL;
      up_write(&chip->ops_sem);
  }
diff --git a/drivers/char/tpm/tpm-interface.c 
b/drivers/char/tpm/tpm-interface.c
index 02e8cffd1163..fcd845ad8c3c 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -124,6 +124,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip 
*chip, void *buf, size_t bufsiz)
          dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
      } else if (len < TPM_HEADER_SIZE || len != 
be32_to_cpu(header->length))
          rc = -EFAULT;
+    else
+        rc = 0;

      return rc ? rc : len;
  }




>
>
>>
>>   /Jarkko
>>
>


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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-08  1:51           ` Stefan Berger
  2019-02-08  2:14             ` Stefan Berger
@ 2019-02-08 11:14             ` Jarkko Sakkinen
  2019-02-08 12:05               ` Stefan Berger
  1 sibling, 1 reply; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-08 11:14 UTC (permalink / raw)
  To: Stefan Berger
  Cc: Alexander Steffen, linux-integrity, linux-kernel,
	linux-security-module, Peter Huewe, Jason Gunthorpe,
	Tomas Winkler, Tadeusz Struk, Stefan Berger, Nayna Jain

On Thu, Feb 07, 2019 at 08:51:15PM -0500, Stefan Berger wrote:
> > > +    rc = 0;
> > >       if (chip->flags & TPM_CHIP_FLAG_IRQ)
> > >           goto out_recv;
> > What why?
> 
> 
> This fix seems to only be necessary when bisecting. You may want to apply
> it!

I don't want to apply it because I don't understand why it would
be needed. Can you give a short explanation?

/Jarkko

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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-08  2:14             ` Stefan Berger
@ 2019-02-08 11:50               ` Jarkko Sakkinen
  2019-02-08 12:22                 ` Stefan Berger
  2019-02-08 13:28               ` Alexander Steffen
  1 sibling, 1 reply; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-08 11:50 UTC (permalink / raw)
  To: Stefan Berger
  Cc: Alexander Steffen, linux-integrity, linux-kernel,
	linux-security-module, Peter Huewe, Jason Gunthorpe,
	Tomas Winkler, Tadeusz Struk, Stefan Berger, Nayna Jain

On Thu, Feb 07, 2019 at 09:14:54PM -0500, Stefan Berger wrote:
> +EXPORT_SYMBOL_GPL(tpm_chip_start);
> 
>  /**
>   * tpm_chip_stop() - power off the TPM
> @@ -131,7 +132,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
>      if (chip->ops->clk_enable)
>          chip->ops->clk_enable(chip, false);
>  }
> -
> +EXPORT_SYMBOL_GPL(tpm_chip_stop);

These are already fixed.

>  /**
>   * tpm_try_get_ops() - Get a ref to the tpm_chip
> @@ -474,8 +475,11 @@ static void tpm_del_char_device(struct tpm_chip *chip)
> 
>      /* Make the driver uncallable. */
>      down_write(&chip->ops_sem);
> -    if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +    if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +        tpm_chip_start(chip, 0);
>          tpm2_shutdown(chip, TPM2_SU_CLEAR);
> +        tpm_chip_stop(chip, 0);
> +    }

Thanks!

Needs to be also done in tpm_pm_suspend() but there it is also better to
take locks because in-kernel subsystem might be using TPM even user
space has been freezed (e.g. hibernate possibly in future).

mutex_lock(&chip->tpm_mutex);
if (!tpm_chip_start(chip, 0)) {
	tpm2_shutdown(chip, TPM2_SU_CLEAR);
	tpm_chip_stop(chip, 0);
}
mutex_unlock(&chip->tpm_mutex);

Fixed now.

>      chip->ops = NULL;
>      up_write(&chip->ops_sem);
>  }
> diff --git a/drivers/char/tpm/tpm-interface.c
> b/drivers/char/tpm/tpm-interface.c
> index 02e8cffd1163..fcd845ad8c3c 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -124,6 +124,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
> void *buf, size_t bufsiz)
>          dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
>      } else if (len < TPM_HEADER_SIZE || len != be32_to_cpu(header->length))
>          rc = -EFAULT;
> +    else
> +        rc = 0;

Why is this needed?

/Jarkko

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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-08 11:14             ` Jarkko Sakkinen
@ 2019-02-08 12:05               ` Stefan Berger
  2019-02-08 13:02                 ` Jarkko Sakkinen
  0 siblings, 1 reply; 42+ messages in thread
From: Stefan Berger @ 2019-02-08 12:05 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Alexander Steffen, linux-integrity, linux-kernel,
	linux-security-module, Peter Huewe, Jason Gunthorpe,
	Tomas Winkler, Tadeusz Struk, Stefan Berger, Nayna Jain

On 2/8/19 6:14 AM, Jarkko Sakkinen wrote:
> On Thu, Feb 07, 2019 at 08:51:15PM -0500, Stefan Berger wrote:
>>>> +    rc = 0;
>>>>        if (chip->flags & TPM_CHIP_FLAG_IRQ)
>>>>            goto out_recv;
>>> What why?
>>
>> This fix seems to only be necessary when bisecting. You may want to apply
>> it!
> I don't want to apply it because I don't understand why it would
> be needed. Can you give a short explanation?


See my comment on [PATCH v11 08/16]. It needs to be added in that patch 
since otherwise rc holds a non-zero value on function exit, which is 
wrong at that point.


>
> /Jarkko
>


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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-08 11:50               ` Jarkko Sakkinen
@ 2019-02-08 12:22                 ` Stefan Berger
  2019-02-08 13:12                   ` Jarkko Sakkinen
  0 siblings, 1 reply; 42+ messages in thread
From: Stefan Berger @ 2019-02-08 12:22 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Alexander Steffen, linux-integrity, linux-kernel,
	linux-security-module, Peter Huewe, Jason Gunthorpe,
	Tomas Winkler, Tadeusz Struk, Stefan Berger, Nayna Jain

On 2/8/19 6:50 AM, Jarkko Sakkinen wrote:
> On Thu, Feb 07, 2019 at 09:14:54PM -0500, Stefan Berger wrote:
>
>>       chip->ops = NULL;
>>       up_write(&chip->ops_sem);
>>   }
>> diff --git a/drivers/char/tpm/tpm-interface.c
>> b/drivers/char/tpm/tpm-interface.c
>> index 02e8cffd1163..fcd845ad8c3c 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -124,6 +124,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
>> void *buf, size_t bufsiz)
>>           dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
>>       } else if (len < TPM_HEADER_SIZE || len != be32_to_cpu(header->length))
>>           rc = -EFAULT;
>> +    else
>> +        rc = 0;
> Why is this needed?

Because it holds a non-zero value, which is wrong at this point. Below 
it is:

return rc ? rc : len;

It will always return that rc and never 'len'.

It's not just needed for bisecting. I still need it with your latest 
tree. That's the only change I need with my current testing of 
tpm_vtpm_proxy, TIS + TPM 1.2 , TIS + TPM 2.0 , and CRB + TPM 2.0 (with 
QEMU :-) ).

    Stefan



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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-08 12:05               ` Stefan Berger
@ 2019-02-08 13:02                 ` Jarkko Sakkinen
  2019-02-08 13:10                   ` Stefan Berger
  0 siblings, 1 reply; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-08 13:02 UTC (permalink / raw)
  To: Stefan Berger
  Cc: Alexander Steffen, linux-integrity, linux-kernel,
	linux-security-module, Peter Huewe, Jason Gunthorpe,
	Tomas Winkler, Tadeusz Struk, Stefan Berger, Nayna Jain

On Fri, Feb 08, 2019 at 07:05:26AM -0500, Stefan Berger wrote:
> See my comment on [PATCH v11 08/16]. It needs to be added in that patch
> since otherwise rc holds a non-zero value on function exit, which is wrong
> at that point.

The snippet in question:

rc = chip->ops->send(chip, buf, count);
if (rc < 0) {
	if (rc != -EPIPE)
		dev_err(&chip->dev,
			"%s: tpm_send: error %d\n", __func__, rc);
	return rc;
}

if (chip->flags & TPM_CHIP_FLAG_IRQ)
	goto out_recv;

'send()' ought to return zero on success case.

This is how the snippet was before applying any patches scheduled for
v5.1:

rc = chip->ops->send(chip, buf, count);
if (rc < 0) {
	if (rc != -EPIPE)
		dev_err(&chip->dev,
			"%s: tpm_send: error %d\n", __func__, rc);
	return rc;
}

if (chip->flags & TPM_CHIP_FLAG_IRQ)
	goto out_recv;

Does not compute.

/Jarkko

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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-08 13:02                 ` Jarkko Sakkinen
@ 2019-02-08 13:10                   ` Stefan Berger
  2019-02-08 13:17                     ` Jarkko Sakkinen
  0 siblings, 1 reply; 42+ messages in thread
From: Stefan Berger @ 2019-02-08 13:10 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Alexander Steffen, linux-integrity, linux-kernel,
	linux-security-module, Peter Huewe, Jason Gunthorpe,
	Tomas Winkler, Tadeusz Struk, Stefan Berger, Nayna Jain

On 2/8/19 8:02 AM, Jarkko Sakkinen wrote:
> On Fri, Feb 08, 2019 at 07:05:26AM -0500, Stefan Berger wrote:
>> See my comment on [PATCH v11 08/16]. It needs to be added in that patch
>> since otherwise rc holds a non-zero value on function exit, which is wrong
>> at that point.
> The snippet in question:
>
> rc = chip->ops->send(chip, buf, count);
> if (rc < 0) {
> 	if (rc != -EPIPE)
> 		dev_err(&chip->dev,
> 			"%s: tpm_send: error %d\n", __func__, rc);
> 	return rc;
> }
>
> if (chip->flags & TPM_CHIP_FLAG_IRQ)
> 	goto out_recv;
>
> 'send()' ought to return zero on success case.
>
> This is how the snippet was before applying any patches scheduled for
> v5.1:
>
> rc = chip->ops->send(chip, buf, count);
> if (rc < 0) {
> 	if (rc != -EPIPE)
> 		dev_err(&chip->dev,
> 			"%s: tpm_send: error %d\n", __func__, rc);
> 	return rc;
> }
>
> if (chip->flags & TPM_CHIP_FLAG_IRQ)
> 	goto out_recv;
>
> Does not compute.

tpm_tis_send_main returns 'len' and that's what we have here.



>
> /Jarkko
>


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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-08 12:22                 ` Stefan Berger
@ 2019-02-08 13:12                   ` Jarkko Sakkinen
  0 siblings, 0 replies; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-08 13:12 UTC (permalink / raw)
  To: Stefan Berger
  Cc: Alexander Steffen, linux-integrity, linux-kernel,
	linux-security-module, Peter Huewe, Jason Gunthorpe,
	Tomas Winkler, Tadeusz Struk, Stefan Berger, Nayna Jain

On Fri, Feb 08, 2019 at 07:22:48AM -0500, Stefan Berger wrote:
> On 2/8/19 6:50 AM, Jarkko Sakkinen wrote:
> > On Thu, Feb 07, 2019 at 09:14:54PM -0500, Stefan Berger wrote:
> > 
> > >       chip->ops = NULL;
> > >       up_write(&chip->ops_sem);
> > >   }
> > > diff --git a/drivers/char/tpm/tpm-interface.c
> > > b/drivers/char/tpm/tpm-interface.c
> > > index 02e8cffd1163..fcd845ad8c3c 100644
> > > --- a/drivers/char/tpm/tpm-interface.c
> > > +++ b/drivers/char/tpm/tpm-interface.c
> > > @@ -124,6 +124,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
> > > void *buf, size_t bufsiz)
> > >           dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
> > >       } else if (len < TPM_HEADER_SIZE || len != be32_to_cpu(header->length))
> > >           rc = -EFAULT;
> > > +    else
> > > +        rc = 0;
> > Why is this needed?
> 
> Because it holds a non-zero value, which is wrong at this point. Below it
> is:
> 
> return rc ? rc : len;
> 
> It will always return that rc and never 'len'.
> 
> It's not just needed for bisecting. I still need it with your latest tree.
> That's the only change I need with my current testing of tpm_vtpm_proxy, TIS
> + TPM 1.2 , TIS + TPM 2.0 , and CRB + TPM 2.0 (with QEMU :-) ).

The code is unchaged. If there was a regression that would have been
ages.

/Jarkko

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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-08 13:10                   ` Stefan Berger
@ 2019-02-08 13:17                     ` Jarkko Sakkinen
  2019-02-08 13:33                       ` Jarkko Sakkinen
  0 siblings, 1 reply; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-08 13:17 UTC (permalink / raw)
  To: Stefan Berger
  Cc: Alexander Steffen, linux-integrity, linux-kernel,
	linux-security-module, Peter Huewe, Jason Gunthorpe,
	Tomas Winkler, Tadeusz Struk, Stefan Berger, Nayna Jain

On Fri, Feb 08, 2019 at 08:10:32AM -0500, Stefan Berger wrote:
> On 2/8/19 8:02 AM, Jarkko Sakkinen wrote:
> > On Fri, Feb 08, 2019 at 07:05:26AM -0500, Stefan Berger wrote:
> > > See my comment on [PATCH v11 08/16]. It needs to be added in that patch
> > > since otherwise rc holds a non-zero value on function exit, which is wrong
> > > at that point.
> > The snippet in question:
> > 
> > rc = chip->ops->send(chip, buf, count);
> > if (rc < 0) {
> > 	if (rc != -EPIPE)
> > 		dev_err(&chip->dev,
> > 			"%s: tpm_send: error %d\n", __func__, rc);
> > 	return rc;
> > }
> > 
> > if (chip->flags & TPM_CHIP_FLAG_IRQ)
> > 	goto out_recv;
> > 
> > 'send()' ought to return zero on success case.
> > 
> > This is how the snippet was before applying any patches scheduled for
> > v5.1:
> > 
> > rc = chip->ops->send(chip, buf, count);
> > if (rc < 0) {
> > 	if (rc != -EPIPE)
> > 		dev_err(&chip->dev,
> > 			"%s: tpm_send: error %d\n", __func__, rc);
> > 	return rc;
> > }
> > 
> > if (chip->flags & TPM_CHIP_FLAG_IRQ)
> > 	goto out_recv;
> > 
> > Does not compute.
> 
> tpm_tis_send_main returns 'len' and that's what we have here.

Before doing any kind of code change, we should at least know what
has caused this that it has worked before.

And also which commit caused the regression to happen, because it
looks like a bug in tpm_tis_core, not in the main TPM driver. It
would need the fixes tag and cc to stable.

/Jarkko

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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-08  2:14             ` Stefan Berger
  2019-02-08 11:50               ` Jarkko Sakkinen
@ 2019-02-08 13:28               ` Alexander Steffen
  2019-02-08 14:09                 ` Jarkko Sakkinen
  1 sibling, 1 reply; 42+ messages in thread
From: Alexander Steffen @ 2019-02-08 13:28 UTC (permalink / raw)
  To: Stefan Berger, Jarkko Sakkinen
  Cc: linux-integrity, linux-kernel, linux-security-module,
	Peter Huewe, Jason Gunthorpe, Tomas Winkler, Tadeusz Struk,
	Stefan Berger, Nayna Jain

On 08.02.2019 03:14, Stefan Berger wrote:
> On 2/7/19 8:51 PM, Stefan Berger wrote:
>> On 2/7/19 7:33 PM, Jarkko Sakkinen wrote:
>>> On Thu, Feb 07, 2019 at 06:29:43PM -0500, Stefan Berger wrote:
>>>> On 2/7/19 4:29 PM, Jarkko Sakkinen wrote:
>>
>>
>>>
>>>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>>>> index e74c5b7b64bf..52afe20cc8a1 100644
>>>> --- a/drivers/char/tpm/tpm2-cmd.c
>>>> +++ b/drivers/char/tpm/tpm2-cmd.c
>>>> @@ -799,7 +799,9 @@ int tpm2_probe(struct tpm_chip *chip)
>>>>       tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
>>>>       tpm_buf_append_u32(&buf, TPM_PT_TOTAL_COMMANDS);
>>>>       tpm_buf_append_u32(&buf, 1);
>>>> +    tpm_chip_start(chip);
>>>>       rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
>>>> +    tpm_chip_stop(chip);
>>> Thanks Stefan! I added call to tpm_tis_core as tpm2-cmd.c is to be kept
>>> out of chip management common case being that you call 
>>> tpm_try_get_ops(),
>>> do 1-N TPM commands and release with tpm_put_ops(). These functions take
>>> care starting and stopping the chip.
>>>
>>> I fixed the 2nd issue in the master.
>>
>> You also need to export the tpm_chip_start/stop symbols.
>>
>>
>> Master now seems to (still) have a problem when rmmod'ing:
>>
>> A TPM error (325) occurred stopping the TPM. This happens at the same 
>> patch where I had to add the tpm_chip_start/stop above.
>>
>>
> 
> Here's my current overall patch against your master. I suppose the other 
> tpm2_shutdown() for power management doesn't need the chip start ?
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 9865776ee2cd..f7147706a9c3 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -115,6 +115,7 @@ int tpm_chip_start(struct tpm_chip *chip)
> 
>       return 0;
>   }
> +EXPORT_SYMBOL_GPL(tpm_chip_start);
> 
>   /**
>    * tpm_chip_stop() - power off the TPM
> @@ -131,7 +132,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
>       if (chip->ops->clk_enable)
>           chip->ops->clk_enable(chip, false);
>   }
> -
> +EXPORT_SYMBOL_GPL(tpm_chip_stop);
> 
>   /**
>    * tpm_try_get_ops() - Get a ref to the tpm_chip
> @@ -474,8 +475,11 @@ static void tpm_del_char_device(struct tpm_chip *chip)
> 
>       /* Make the driver uncallable. */
>       down_write(&chip->ops_sem);
> -    if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +    if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +        tpm_chip_start(chip, 0);
>           tpm2_shutdown(chip, TPM2_SU_CLEAR);
> +        tpm_chip_stop(chip, 0);
> +    }
>       chip->ops = NULL;
>       up_write(&chip->ops_sem);
>   }
> diff --git a/drivers/char/tpm/tpm-interface.c 
> b/drivers/char/tpm/tpm-interface.c
> index 02e8cffd1163..fcd845ad8c3c 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -124,6 +124,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip 
> *chip, void *buf, size_t bufsiz)
>           dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
>       } else if (len < TPM_HEADER_SIZE || len != 
> be32_to_cpu(header->length))
>           rc = -EFAULT;
> +    else
> +        rc = 0;
> 
>       return rc ? rc : len;
>   }

Applying those changes fixes all the issues I saw, thanks. I'll retest 
Jarkko's tree without custom patches, once you've agreed on the rc question.

Alexander

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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-08 13:17                     ` Jarkko Sakkinen
@ 2019-02-08 13:33                       ` Jarkko Sakkinen
  2019-02-08 14:02                         ` Stefan Berger
  0 siblings, 1 reply; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-08 13:33 UTC (permalink / raw)
  To: Stefan Berger
  Cc: Alexander Steffen, linux-integrity, linux-kernel,
	linux-security-module, Peter Huewe, Jason Gunthorpe,
	Tomas Winkler, Tadeusz Struk, Stefan Berger, Nayna Jain

On Fri, Feb 08, 2019 at 03:17:54PM +0200, Jarkko Sakkinen wrote:
> On Fri, Feb 08, 2019 at 08:10:32AM -0500, Stefan Berger wrote:
> > On 2/8/19 8:02 AM, Jarkko Sakkinen wrote:
> > > On Fri, Feb 08, 2019 at 07:05:26AM -0500, Stefan Berger wrote:
> > > > See my comment on [PATCH v11 08/16]. It needs to be added in that patch
> > > > since otherwise rc holds a non-zero value on function exit, which is wrong
> > > > at that point.
> > > The snippet in question:
> > > 
> > > rc = chip->ops->send(chip, buf, count);
> > > if (rc < 0) {
> > > 	if (rc != -EPIPE)
> > > 		dev_err(&chip->dev,
> > > 			"%s: tpm_send: error %d\n", __func__, rc);
> > > 	return rc;
> > > }
> > > 
> > > if (chip->flags & TPM_CHIP_FLAG_IRQ)
> > > 	goto out_recv;
> > > 
> > > 'send()' ought to return zero on success case.
> > > 
> > > This is how the snippet was before applying any patches scheduled for
> > > v5.1:
> > > 
> > > rc = chip->ops->send(chip, buf, count);
> > > if (rc < 0) {
> > > 	if (rc != -EPIPE)
> > > 		dev_err(&chip->dev,
> > > 			"%s: tpm_send: error %d\n", __func__, rc);
> > > 	return rc;
> > > }
> > > 
> > > if (chip->flags & TPM_CHIP_FLAG_IRQ)
> > > 	goto out_recv;
> > > 
> > > Does not compute.
> > 
> > tpm_tis_send_main returns 'len' and that's what we have here.
> 
> Before doing any kind of code change, we should at least know what
> has caused this that it has worked before.
> 
> And also which commit caused the regression to happen, because it
> looks like a bug in tpm_tis_core, not in the main TPM driver. It
> would need the fixes tag and cc to stable.

Found the root cause.

len = chip->ops->recv(chip, buf, bufsiz);
if (len < 0) {
	rc = len;
	dev_err(&chip->dev,
		"tpm_transmit: tpm_recv: error %d\n", rc);
	goto out;
} else if (len < TPM_HEADER_SIZE) {
	rc = -EFAULT;
	goto out;
}

if (len != be32_to_cpu(header->length)) {
	rc = -EFAULT;
	goto out;
}

rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
if (rc)
	dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc);

That unconditional call to commit space masked the bugs in the device
drivers. I'll provide fixes shortly.

/Jarkko

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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-08 13:33                       ` Jarkko Sakkinen
@ 2019-02-08 14:02                         ` Stefan Berger
  2019-02-08 14:08                           ` Jarkko Sakkinen
  0 siblings, 1 reply; 42+ messages in thread
From: Stefan Berger @ 2019-02-08 14:02 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Alexander Steffen, linux-integrity, linux-kernel,
	linux-security-module, Peter Huewe, Jason Gunthorpe,
	Tomas Winkler, Tadeusz Struk, Stefan Berger, Nayna Jain

On 2/8/19 8:33 AM, Jarkko Sakkinen wrote:
>
> if (len != be32_to_cpu(header->length)) {
> 	rc = -EFAULT;
> 	goto out;
> }
>
> rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
> if (rc)
> 	dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc);
>
> That unconditional call to commit space masked the bugs in the device
> drivers. I'll provide fixes shortly.

You got it! :-)


>
> /Jarkko
>


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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-08 14:02                         ` Stefan Berger
@ 2019-02-08 14:08                           ` Jarkko Sakkinen
  0 siblings, 0 replies; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-08 14:08 UTC (permalink / raw)
  To: Stefan Berger
  Cc: Alexander Steffen, linux-integrity, linux-kernel,
	linux-security-module, Peter Huewe, Jason Gunthorpe,
	Tomas Winkler, Tadeusz Struk, Stefan Berger, Nayna Jain

On Fri, Feb 08, 2019 at 09:02:22AM -0500, Stefan Berger wrote:
> On 2/8/19 8:33 AM, Jarkko Sakkinen wrote:
> > 
> > if (len != be32_to_cpu(header->length)) {
> > 	rc = -EFAULT;
> > 	goto out;
> > }
> > 
> > rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
> > if (rc)
> > 	dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc);
> > 
> > That unconditional call to commit space masked the bugs in the device
> > drivers. I'll provide fixes shortly.
> 
> You got it! :-)

Yes, and it should not be fixes in tpm-interface.c. Sent a patch set
for review.

Thanks for spotting this out!

/Jarkko

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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-08 13:28               ` Alexander Steffen
@ 2019-02-08 14:09                 ` Jarkko Sakkinen
  2019-02-08 18:02                   ` Alexander Steffen
  0 siblings, 1 reply; 42+ messages in thread
From: Jarkko Sakkinen @ 2019-02-08 14:09 UTC (permalink / raw)
  To: Alexander Steffen
  Cc: Stefan Berger, linux-integrity, linux-kernel,
	linux-security-module, Peter Huewe, Jason Gunthorpe,
	Tomas Winkler, Tadeusz Struk, Stefan Berger, Nayna Jain

On Fri, Feb 08, 2019 at 02:28:50PM +0100, Alexander Steffen wrote:
> Applying those changes fixes all the issues I saw, thanks. I'll retest
> Jarkko's tree without custom patches, once you've agreed on the rc question.

Just send a two patch patch set how I believe the regression should
be properly fixed.

Do not have right now dTPM at hand (the NUC that has one does not
respond SSH for some reason) so I need help with testing.

/Jarkko

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

* Re: [PATCH v11 00/16] Remove nested TPM operations
  2019-02-08 14:09                 ` Jarkko Sakkinen
@ 2019-02-08 18:02                   ` Alexander Steffen
  0 siblings, 0 replies; 42+ messages in thread
From: Alexander Steffen @ 2019-02-08 18:02 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Stefan Berger, linux-integrity, linux-kernel,
	linux-security-module, Peter Huewe, Jason Gunthorpe,
	Tomas Winkler, Tadeusz Struk, Stefan Berger, Nayna Jain

On 08.02.2019 15:09, Jarkko Sakkinen wrote:
> On Fri, Feb 08, 2019 at 02:28:50PM +0100, Alexander Steffen wrote:
>> Applying those changes fixes all the issues I saw, thanks. I'll retest
>> Jarkko's tree without custom patches, once you've agreed on the rc question.
> 
> Just send a two patch patch set how I believe the regression should
> be properly fixed.
> 
> Do not have right now dTPM at hand (the NUC that has one does not
> respond SSH for some reason) so I need help with testing.

Those patches seem to be changing fast :) I'll test them next week.

Alexander

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

end of thread, other threads:[~2019-02-08 18:03 UTC | newest]

Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-05 22:47 [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
2019-02-05 22:47 ` [PATCH v11 01/16] tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter Jarkko Sakkinen
2019-02-05 22:47 ` [PATCH v11 02/16] tpm: fix invalid return value in pubek_show() Jarkko Sakkinen
2019-02-05 22:47 ` [PATCH v11 03/16] tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails Jarkko Sakkinen
2019-02-05 22:47 ` [PATCH v11 04/16] tpm: print tpm2_commit_space() error inside tpm2_commit_space() Jarkko Sakkinen
2019-02-05 22:47 ` [PATCH v11 05/16] tpm: declare struct tpm_header Jarkko Sakkinen
2019-02-05 22:47 ` [PATCH v11 06/16] tpm: access command header through struct in tpm_try_transmit() Jarkko Sakkinen
2019-02-05 22:47 ` [PATCH v11 07/16] tpm: encapsulate tpm_dev_transmit() Jarkko Sakkinen
2019-02-05 22:47 ` [PATCH v11 08/16] tpm: clean up tpm_try_transmit() error handling flow Jarkko Sakkinen
2019-02-07 23:36   ` Stefan Berger
2019-02-05 22:47 ` [PATCH v11 09/16] tpm: move tpm_validate_commmand() to tpm2-space.c Jarkko Sakkinen
2019-02-05 22:47 ` [PATCH v11 10/16] tpm: move TPM space code out of tpm_transmit() Jarkko Sakkinen
2019-02-05 22:47 ` [PATCH v11 11/16] tpm: remove @space from tpm_transmit() Jarkko Sakkinen
2019-02-05 22:47 ` [PATCH v11 12/16] tpm: use tpm_try_get_ops() in tpm-sysfs.c Jarkko Sakkinen
2019-02-05 22:47 ` [PATCH v11 13/16] tpm: remove TPM_TRANSMIT_UNLOCKED flag Jarkko Sakkinen
2019-02-05 22:47 ` [PATCH v11 14/16] tpm: introduce tpm_chip_start() and tpm_chip_stop() Jarkko Sakkinen
2019-02-05 22:47 ` [PATCH v11 15/16] tpm: take TPM chip power gating out of tpm_transmit() Jarkko Sakkinen
2019-02-07 23:32   ` Stefan Berger
2019-02-08  0:02     ` Jerry Snitselaar
2019-02-05 22:47 ` [PATCH v11 16/16] tpm: remove @flags from tpm_transmit() Jarkko Sakkinen
2019-02-06 12:06 ` [PATCH v11 00/16] Remove nested TPM operations Jarkko Sakkinen
2019-02-07 18:41   ` Alexander Steffen
2019-02-07 21:14     ` Stefan Berger
2019-02-07 21:29     ` Jarkko Sakkinen
2019-02-07 23:29       ` Stefan Berger
2019-02-08  0:33         ` Jarkko Sakkinen
2019-02-08  1:51           ` Stefan Berger
2019-02-08  2:14             ` Stefan Berger
2019-02-08 11:50               ` Jarkko Sakkinen
2019-02-08 12:22                 ` Stefan Berger
2019-02-08 13:12                   ` Jarkko Sakkinen
2019-02-08 13:28               ` Alexander Steffen
2019-02-08 14:09                 ` Jarkko Sakkinen
2019-02-08 18:02                   ` Alexander Steffen
2019-02-08 11:14             ` Jarkko Sakkinen
2019-02-08 12:05               ` Stefan Berger
2019-02-08 13:02                 ` Jarkko Sakkinen
2019-02-08 13:10                   ` Stefan Berger
2019-02-08 13:17                     ` Jarkko Sakkinen
2019-02-08 13:33                       ` Jarkko Sakkinen
2019-02-08 14:02                         ` Stefan Berger
2019-02-08 14:08                           ` Jarkko Sakkinen

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