2018-11-18 12:49:03

by Jarkko Sakkinen

Subject: [PATCH v9 00/17] Removed nested TPM operations

[was Detach TPM space code out of the tpm_transmit() flow but the scope
expanded a bit.]

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.

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 (17):
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: call tpm2_flush_space() on error in tpm_try_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 | 45 ++++-
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 | 93 ++++++++---
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, 410 insertions(+), 422 deletions(-)

--
2.19.1

2018-11-18 12:49:03

by Jarkko Sakkinen

Subject: [PATCH v9 01/17] tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter

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 <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
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 d9439f9abe78..64510ed81b46 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 6f306338953b..f19b7c1ff800 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;
}

@@ -462,9 +460,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;
}
@@ -494,11 +490,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;
}
@@ -537,7 +531,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)
@@ -583,8 +577,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;
@@ -618,11 +611,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;
}

@@ -747,9 +737,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

2018-11-18 12:49:18

by Jarkko Sakkinen

Subject: [PATCH v9 02/17] tpm: fix invalid return value in pubek_show()

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 <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
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

2018-11-18 12:49:39

by Jarkko Sakkinen

Subject: [PATCH v9 03/17] tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails

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 <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
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

2018-11-18 12:49:46

by Jarkko Sakkinen

Subject: [PATCH v9 04/17] tpm: print tpm2_commit_space() error inside tpm2_commit_space()

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

Cc: James Bottomley <[email protected]>
Signed-off-by: Jarkko Sakkinen <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
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 64510ed81b46..7ac6ada8428c 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

2018-11-18 12:50:01

by Jarkko Sakkinen

Subject: [PATCH v9 05/17] tpm: declare struct tpm_header

Decleare struct tpm_header that replaces struct tpm_input_header and
struct tpm_output_header.

Signed-off-by: Jarkko Sakkinen <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
drivers/char/tpm/tpm-interface.c | 9 ++++-----
drivers/char/tpm/tpm.h | 27 ++++++++++++---------------
drivers/char/tpm/tpm2-cmd.c | 4 ++--
drivers/char/tpm/tpm2-space.c | 6 +++---
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, 28 insertions(+), 33 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 7ac6ada8428c..cb0d44c158ae 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -67,7 +67,7 @@ static int tpm_validate_command(struct tpm_chip *chip,
const u8 *cmd,
size_t len)
{
- const struct tpm_input_header *header = (const void *)cmd;
+ const struct tpm_header *header = (const void *)cmd;
int i;
u32 cc;
u32 attrs;
@@ -166,7 +166,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
u8 *buf, size_t bufsiz,
unsigned int flags)
{
- struct tpm_output_header *header = (void *)buf;
+ struct tpm_header *header = (void *)buf;
int rc;
ssize_t len = 0;
u32 count, ordinal;
@@ -316,7 +316,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 +381,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..86fa8ac0ae20 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. */
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..862ee1ce1755 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 = (void *)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 = (void *)rsp;
struct tpm2_cap_handles *data;
u32 phandle;
u32 phandle_type;
@@ -492,7 +492,7 @@ 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)
{
- struct tpm_output_header *header = (void *)buf;
+ struct tpm_header *header = (void *)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

2018-11-18 12:50:15

by Jarkko Sakkinen

Subject: [PATCH v9 06/17] tpm: access command header through struct in tpm_try_transmit()

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 <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
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 cb0d44c158ae..11af50646ed1 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -191,8 +191,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
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

2018-11-18 12:50:36

by Jarkko Sakkinen

Subject: [PATCH v9 08/17] tpm: call tpm2_flush_space() on error in tpm_try_transmit()

Always call tpm2_flush_space() on failure in tpm_try_transmit() so that
the volatile memory of the TPM gets cleared. If /dev/tpm0 does not have
sufficient permissions (usually it has), this could lead to the leakage
of TPM objects. Through /dev/tpmrm0 this issue does not raise any new
security concerns.

Cc: James Bottomley <[email protected]>
Cc: [email protected]
Fixes: 745b361e989a ("tpm:tpm: infrastructure for TPM spaces")
Signed-off-by: Jarkko Sakkinen <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
drivers/char/tpm/tpm-interface.c | 29 ++++++++++++-----------------
drivers/char/tpm/tpm.h | 1 +
drivers/char/tpm/tpm2-space.c | 2 +-
3 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 11af50646ed1..ef00b698ec02 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -224,14 +224,14 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,

rc = tpm2_prepare_space(chip, space, ordinal, buf);
if (rc)
- goto out;
+ goto out_idle;

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_space;
}

if (chip->flags & TPM_CHIP_FLAG_IRQ)
@@ -247,7 +247,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
if (chip->ops->req_canceled(chip, status)) {
dev_err(&chip->dev, "Operation Canceled\n");
rc = -ECANCELED;
- goto out;
+ goto out_space;
}

tpm_msleep(TPM_TIMEOUT_POLL);
@@ -257,28 +257,23 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
chip->ops->cancel(chip);
dev_err(&chip->dev, "Operation Timed out\n");
rc = -ETIME;
- goto out;
+ goto out_space;

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_space:
+ if (rc)
+ tpm2_flush_space(chip);
+ else
+ rc = tpm2_commit_space(chip, space, ordinal, buf, &len);

-out:
+out_idle:
/* may fail but do not override previous error value in rc */
tpm_go_idle(chip, flags);

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 86fa8ac0ae20..92638ec6ec97 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 862ee1ce1755..393d9aadadce 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

2018-11-18 12:50:53

by Jarkko Sakkinen

Subject: [PATCH v9 09/17] tpm: clean up tpm_try_transmit() error handling flow

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

Signed-off-by: Jarkko Sakkinen <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
drivers/char/tpm/tpm-interface.c | 71 ++++++++++++++++----------------
1 file changed, 35 insertions(+), 36 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index ef00b698ec02..f2280bf15339 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -171,7 +171,6 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
ssize_t len = 0;
u32 count, ordinal;
unsigned long stop;
- bool need_locality;

rc = tpm_validate_command(chip, space, buf, bufsiz);
if (rc == -EINVAL)
@@ -201,30 +200,9 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
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_idle;
+ return rc;

rc = chip->ops->send(chip, buf, count);
if (rc < 0) {
@@ -273,19 +251,6 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
else
rc = tpm2_commit_space(chip, space, ordinal, buf, &len);

-out_idle:
- /* 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;
}

@@ -315,6 +280,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,
@@ -330,7 +296,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);
--
2.19.1

2018-11-18 12:50:57

by Jarkko Sakkinen

Subject: [PATCH v9 10/17] tpm: move tpm_validate_commmand() to tpm2-space.c

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 <[email protected]>
Signed-off-by: Jarkko Sakkinen <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
drivers/char/tpm/tpm-interface.c | 72 +++++++-------------------------
drivers/char/tpm/tpm.h | 9 ++--
drivers/char/tpm/tpm2-space.c | 54 +++++++++++++++++++++---
3 files changed, 68 insertions(+), 67 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index f2280bf15339..a6ffd0960ae5 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -62,47 +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 u8 *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 (!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;
@@ -172,20 +131,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
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;
@@ -200,7 +147,18 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
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;

@@ -249,7 +207,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
if (rc)
tpm2_flush_space(chip);
else
- 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 92638ec6ec97..68d78ebba032 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, u8 *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, u8 *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 393d9aadadce..172c9f9d4c29 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 u8 *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 0;
+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;
}

@@ -489,8 +531,8 @@ static int tpm2_save_space(struct tpm_chip *chip)
return 0;
}

-int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
- u32 cc, u8 *buf, size_t *bufsiz)
+int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space, u8 *buf,
+ size_t *bufsiz)
{
struct tpm_header *header = (void *)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

2018-11-18 12:51:11

by Jarkko Sakkinen

Subject: [PATCH v9 11/17] tpm: move TPM space code out of tpm_transmit()

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 <[email protected]>
Signed-off-by: Jarkko Sakkinen <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
drivers/char/tpm/tpm-dev-common.c | 30 ++++++++++++++++++++++++++----
drivers/char/tpm/tpm-interface.c | 29 +++--------------------------
drivers/char/tpm/tpm2-space.c | 12 ++++++------
3 files changed, 35 insertions(+), 36 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index cbb0ee30b511..40c1cb09ebd2 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);
- mutex_unlock(&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.
+ */
+ 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;

- return ret;
+ len = tpm_transmit(chip, space, buf, bufsiz, TPM_TRANSMIT_UNLOCKED);
+ if (len < 0)
+ ret = len;
+
+ if (ret)
+ tpm2_flush_space(chip);
+ else
+ ret = tpm2_commit_space(chip, space, buf, &len);
+out_lock:
+ mutex_unlock(&chip->tpm_mutex);
+ 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 a6ffd0960ae5..7bec03e46043 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -147,27 +147,12 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
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_space;
+ return rc;
}

if (chip->flags & TPM_CHIP_FLAG_IRQ)
@@ -182,8 +167,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,

if (chip->ops->req_canceled(chip, status)) {
dev_err(&chip->dev, "Operation Canceled\n");
- rc = -ECANCELED;
- goto out_space;
+ return -ECANCELED;
}

tpm_msleep(TPM_TIMEOUT_POLL);
@@ -192,8 +176,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,

chip->ops->cancel(chip);
dev_err(&chip->dev, "Operation Timed out\n");
- rc = -ETIME;
- goto out_space;
+ return -ETIME;

out_recv:
len = chip->ops->recv(chip, buf, bufsiz);
@@ -203,12 +186,6 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
} else if (len < TPM_HEADER_SIZE || len != be32_to_cpu(header->length))
rc = -EFAULT;

-out_space:
- if (rc)
- tpm2_flush_space(chip);
- else
- 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 172c9f9d4c29..995c8b963d65 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

2018-11-18 12:51:17

by Jarkko Sakkinen

Subject: [PATCH v9 12/17] tpm: remove @space from tpm_transmit()

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 <[email protected]>
Signed-off-by: Jarkko Sakkinen <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
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, 42 insertions(+), 54 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 40c1cb09ebd2..c7dc54930576 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 7bec03e46043..8d75c103f280 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -120,9 +120,7 @@ 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,
+static ssize_t tpm_try_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz,
unsigned int flags)
{
struct tpm_header *header = (void *)buf;
@@ -192,7 +190,6 @@ 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
@@ -208,8 +205,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
* * 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 */
@@ -218,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);

@@ -249,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);
@@ -295,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
@@ -306,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;

@@ -463,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 68d78ebba032..10a5bcf806aa 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 f19b7c1ff800..162c255dd131 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;
}
@@ -460,7 +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, TPM_DIGEST_SIZE, 0, log_msg);
+ rc = tpm_transmit_cmd(chip, &buf, TPM_DIGEST_SIZE, 0, log_msg);
tpm_buf_destroy(&buf);
return rc;
}
@@ -490,7 +489,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);
@@ -531,8 +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,
- 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;
@@ -577,7 +575,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;
@@ -611,7 +609,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;
}
@@ -737,7 +735,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 995c8b963d65..46a58fda8cf5 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

2018-11-18 12:51:27

by Jarkko Sakkinen

Subject: [PATCH v9 14/17] tpm: remove TPM_TRANSMIT_UNLOCKED flag

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 <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
drivers/char/tpm/tpm-chip.c | 2 ++
drivers/char/tpm/tpm-dev-common.c | 4 +---
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, 15 insertions(+), 35 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 c7dc54930576..582caefcf19b 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.
@@ -48,7 +47,7 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,
if (ret)
goto out_lock;

- len = tpm_transmit(chip, buf, bufsiz, TPM_TRANSMIT_UNLOCKED);
+ len = tpm_transmit(chip, buf, bufsiz, 0);
if (len < 0)
ret = len;

@@ -57,7 +56,6 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,
else
ret = tpm2_commit_space(chip, space, buf, &len);
out_lock:
- mutex_unlock(&chip->tpm_mutex);
return ret ? ret : len;
}

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 8d75c103f280..5865b9671d20 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 10a5bcf806aa..c7c06de651a0 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 46a58fda8cf5..ced1dc91ba6f 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

2018-11-18 12:51:41

by Jarkko Sakkinen

Subject: [PATCH v9 07/17] tpm: encapsulate tpm_dev_transmit()

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 <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
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 99b5133a9d05..cbb0ee30b511 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->data_pending = ret;
@@ -78,7 +89,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;
}
@@ -163,8 +174,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

2018-11-18 12:51:51

by Jarkko Sakkinen

Subject: [PATCH v9 16/17] tpm: take TPM chip power gating out of tpm_transmit()

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 <[email protected]>
---
drivers/char/tpm/tpm-chip.c | 25 ++++++++++++-------------
drivers/char/tpm/tpm-interface.c | 4 ----
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(+), 29 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 2c79284ffd4e..21ac8da94d90 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -168,11 +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 c42a75710b70..f9d56dfd0d20 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 ced1dc91ba6f..d913715d30aa 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

2018-11-18 12:52:37

by Jarkko Sakkinen

Subject: [PATCH v9 13/17] tpm: use tpm_try_get_ops() in tpm-sysfs.c.

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 <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
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

2018-11-18 12:52:47

by Jarkko Sakkinen

Subject: [PATCH v9 15/17] tpm: introduce tpm_chip_start() and tpm_chip_stop()

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 <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
drivers/char/tpm/tpm-chip.c | 110 +++++++++++++++++++++++++++++++
drivers/char/tpm/tpm-interface.c | 86 +-----------------------
drivers/char/tpm/tpm.h | 2 +
3 files changed, 115 insertions(+), 83 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 5865b9671d20..2c79284ffd4e 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, u8 *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,11 @@ 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 c7c06de651a0..c42a75710b70 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

2018-11-18 12:53:10

by Jarkko Sakkinen

Subject: [PATCH v9 17/17] tpm: remove @flags from tpm_transmit()

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

Signed-off-by: Jarkko Sakkinen <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
---
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 582caefcf19b..024229a08876 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_lock;

- 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 21ac8da94d90..ced380b01804 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, u8 *buf, size_t bufsiz,
- unsigned int flags)
+static ssize_t tpm_try_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz)
{
struct tpm_header *header = (void *)buf;
int rc;
@@ -134,7 +133,6 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, u8 *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, u8 *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 f9d56dfd0d20..6121663b7e9f 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 162c255dd131..438c14106e19 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;
}
@@ -459,7 +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, &buf, TPM_DIGEST_SIZE, 0, log_msg);
+ rc = tpm_transmit_cmd(chip, &buf, TPM_DIGEST_SIZE, log_msg);
tpm_buf_destroy(&buf);
return rc;
}
@@ -489,7 +489,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);
@@ -530,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, &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;
@@ -575,7 +575,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;
@@ -609,7 +609,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;
}
@@ -735,7 +735,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 d913715d30aa..dac218f33645 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

2018-11-18 19:52:04

by Jason Gunthorpe

Subject: Re: [PATCH v9 14/17] tpm: remove TPM_TRANSMIT_UNLOCKED flag

On Sun, Nov 18, 2018 at 02:47:47PM +0200, Jarkko Sakkinen wrote:
> 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 <[email protected]>
> Reviewed-by: Stefan Berger <[email protected]>
> ---
> drivers/char/tpm/tpm-chip.c | 2 ++
> drivers/char/tpm/tpm-dev-common.c | 4 +---
> 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, 15 insertions(+), 35 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;

It really isn't appropriate for something called 'get' to be exclusive
like this.. Call it tpm_try_lock_ops() ?

> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index c7dc54930576..582caefcf19b 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);

The now implicit locking should be tested for using lockdep calls in
all places that assume the lock is held by the caller.

Jason

2018-11-18 22:57:53

by Tomas Winkler

Subject: RE: [PATCH v9 16/17] tpm: take TPM chip power gating out of tpm_transmit()

>
> 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 <[email protected]>

This is still NACK from my side
Tomas

> ---
> drivers/char/tpm/tpm-chip.c | 25 ++++++++++++-------------
> drivers/char/tpm/tpm-interface.c | 4 ----
> 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(+), 29 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 2c79284ffd4e..21ac8da94d90 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -168,11 +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
> c42a75710b70..f9d56dfd0d20 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 ced1dc91ba6f..d913715d30aa 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

2018-11-19 12:39:54

by Jarkko Sakkinen

Subject: Re: [PATCH v9 14/17] tpm: remove TPM_TRANSMIT_UNLOCKED flag

On Sun, Nov 18, 2018 at 12:51:04PM -0700, Jason Gunthorpe wrote:
> On Sun, Nov 18, 2018 at 02:47:47PM +0200, Jarkko Sakkinen wrote:
> > 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 <[email protected]>
> > Reviewed-by: Stefan Berger <[email protected]>
> > ---
> > drivers/char/tpm/tpm-chip.c | 2 ++
> > drivers/char/tpm/tpm-dev-common.c | 4 +---
> > 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, 15 insertions(+), 35 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;
>
> It really isn't appropriate for something called 'get' to be exclusive
> like this.. Call it tpm_try_lock_ops() ?

Would definitely be appropriate to rename it, yes.

> > diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> > index c7dc54930576..582caefcf19b 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);
>
> The now implicit locking should be tested for using lockdep calls in
> all places that assume the lock is held by the caller.

Yes. Most importantly trusted keys and IMA should be tested with lockdep
turned on.

/Jarkko

2018-11-19 12:49:25

by Jarkko Sakkinen

Subject: Re: [PATCH v9 16/17] tpm: take TPM chip power gating out of tpm_transmit()

On Sun, Nov 18, 2018 at 10:52:46PM +0000, Winkler, Tomas wrote:
> This is still NACK from my side

Last time you spoke about tboot intervention but I don't see why as even
sending a single command is not atomic in the true sense of the word
i.e. if there was a problem that would already affect the existing code
and would essentially mean that tboot itself is broken.

/Jarkko

2018-11-19 13:19:18

by Tomas Winkler

Subject: RE: [PATCH v9 16/17] tpm: take TPM chip power gating out of tpm_transmit()

>
> On Sun, Nov 18, 2018 at 10:52:46PM +0000, Winkler, Tomas wrote:
> > This is still NACK from my side
>
> Last time you spoke about tboot intervention but I don't see why as even
> sending a single command is not atomic in the true sense of the word i.e. if
> there was a problem that would already affect the existing code and would
> essentially mean that tboot itself is broken.
So I've consulted the issue, I wasn't not correct in the description. Tboot cannot acquire the locality, unless the host driver relinquish it,
so the issue is opposite, driver is expected to relinquish the locality for tboot to work correctly. This is current status, other behavior will need a different implementation on both sides.

Hopes that clears the question.

Thanks

2018-11-19 14:15:24

by Stefan Berger

Subject: Re: [PATCH v9 15/17] tpm: introduce tpm_chip_start() and tpm_chip_stop()

> +}
> +
> +/**
> + * 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

This function returns void!


> + */
> +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);
> +}
> +

2018-11-19 14:18:36

by Stefan Berger

Subject: Re: [PATCH v9 16/17] tpm: take TPM chip power gating out of tpm_transmit()

On 11/18/18 7:47 AM, 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 <[email protected]>

Reviewed-by: Stefan Berger <[email protected]>


> ---
> drivers/char/tpm/tpm-chip.c | 25 ++++++++++++-------------
> drivers/char/tpm/tpm-interface.c | 4 ----
> 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(+), 29 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 2c79284ffd4e..21ac8da94d90 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -168,11 +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 c42a75710b70..f9d56dfd0d20 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 ced1dc91ba6f..d913715d30aa 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;
>

2018-11-19 14:24:22

by Stefan Berger

Subject: Re: [PATCH v9 00/17] Removed nested TPM operations

On 11/18/18 7:47 AM, Jarkko Sakkinen wrote:
> [was Detach TPM space code out of the tpm_transmit() flow but the scope
> expanded a bit.]
>
> 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.

Tested this series with the vtpm proxy test cases:


Tested-by: Stefan Berger <[email protected]>


>
> 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 (17):
> 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: call tpm2_flush_space() on error in tpm_try_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 | 45 ++++-
> 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 | 93 ++++++++---
> 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, 410 insertions(+), 422 deletions(-)
>

2018-11-19 15:53:33

by Jarkko Sakkinen

Subject: Re: [PATCH v9 15/17] tpm: introduce tpm_chip_start() and tpm_chip_stop()

On Mon, Nov 19, 2018 at 09:13:54AM -0500, Stefan Berger wrote:
>
> > +}
> > +
> > +/**
> > + * 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
>
> This function returns void!

Sorry, my bad.

/Jarkko

2018-11-19 15:56:30

by Jarkko Sakkinen

Subject: Re: [PATCH v9 16/17] tpm: take TPM chip power gating out of tpm_transmit()

On Mon, Nov 19, 2018 at 01:17:56PM +0000, Winkler, Tomas wrote:
> > On Sun, Nov 18, 2018 at 10:52:46PM +0000, Winkler, Tomas wrote:
> > > This is still NACK from my side
> >
> > Last time you spoke about tboot intervention but I don't see why as even
> > sending a single command is not atomic in the true sense of the word i.e. if
> > there was a problem that would already affect the existing code and would
> > essentially mean that tboot itself is broken.
> So I've consulted the issue, I wasn't not correct in the description.
> Tboot cannot acquire the locality, unless the host driver relinquish
> it, so the issue is opposite, driver is expected to relinquish the
> locality for tboot to work correctly. This is current status, other
> behavior will need a different implementation on both sides.

The locality is reliquished after the command sequence (load and save
TPM space + command in /dev/tpmX case).

Is this really such a big issue? The intention is not keep the locality
reserved by the driver.

/Jarkko

2018-11-19 15:58:31

by Jarkko Sakkinen

Subject: Re: [PATCH v9 00/17] Removed nested TPM operations

On Mon, Nov 19, 2018 at 09:23:03AM -0500, Stefan Berger wrote:
> On 11/18/18 7:47 AM, Jarkko Sakkinen wrote:
> > [was Detach TPM space code out of the tpm_transmit() flow but the scope
> > expanded a bit.]
> >
> > 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.
>
> Tested this series with the vtpm proxy test cases:
>
>
> Tested-by: Stefan Berger <[email protected]>

Awesome, thank you! I'll add your tags to the next revision.

/Jarkko

2018-11-19 21:27:11

by Tomas Winkler

Subject: RE: [PATCH v9 04/17] tpm: print tpm2_commit_space() error inside tpm2_commit_space()

>
> The error logging for tpm2_commit_space() is in a wrong place. This commit
> moves it inside that function.
>
> Cc: James Bottomley <[email protected]>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> Reviewed-by: Stefan Berger <[email protected]>
> ---
> 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 64510ed81b46..7ac6ada8428c 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:
Maybe 'out' is too positive name for an error-only exit point?
or you can use if (rc) ... dev_err()
> + dev_err(&chip->dev, "%s: error %d\n", __func__, rc);
> + return rc;
> }
> --
> 2.19.1

2018-11-19 21:35:26

by Tomas Winkler

Subject: RE: [PATCH v9 05/17] tpm: declare struct tpm_header

>
> Decleare struct tpm_header that replaces struct tpm_input_header and
Typo
> struct tpm_output_header.
>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> Reviewed-by: Stefan Berger <[email protected]>
> ---
> drivers/char/tpm/tpm-interface.c | 9 ++++-----
> drivers/char/tpm/tpm.h | 27 ++++++++++++---------------
> drivers/char/tpm/tpm2-cmd.c | 4 ++--
> drivers/char/tpm/tpm2-space.c | 6 +++---
> 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, 28 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-
> interface.c
> index 7ac6ada8428c..cb0d44c158ae 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -67,7 +67,7 @@ static int tpm_validate_command(struct tpm_chip
> *chip,
> const u8 *cmd,
> size_t len)
> {
> - const struct tpm_input_header *header = (const void *)cmd;
> + const struct tpm_header *header = (const void *)cmd;


Sometimes it is casted (void *), sometimes (struct tpm_header *)
Better to use one convention

> int i;
> u32 cc;
> u32 attrs;
> @@ -166,7 +166,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip
> *chip,
> u8 *buf, size_t bufsiz,
> unsigned int flags)
> {
> - struct tpm_output_header *header = (void *)buf;
> + struct tpm_header *header = (void *)buf;
> int rc;
> ssize_t len = 0;
> u32 count, ordinal;
> @@ -316,7 +316,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
> +381,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..86fa8ac0ae20 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 {
Does it deserve kdoc?
> + __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;
No blank after ) in casting.

> + 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;
No blank after ) in casting.
> u32 len = tpm_buf_length(buf);
>
> /* Return silently if overflow has already happened. */ 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..862ee1ce1755 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 = (void *)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 = (void *)rsp;
> struct tpm2_cap_handles *data;
> u32 phandle;
> u32 phandle_type;
> @@ -492,7 +492,7 @@ 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) {
> - struct tpm_output_header *header = (void *)buf;
> + struct tpm_header *header = (void *)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

2018-11-20 16:00:07

by Jarkko Sakkinen

Subject: Re: [PATCH v9 04/17] tpm: print tpm2_commit_space() error inside tpm2_commit_space()

On Mon, Nov 19, 2018 at 09:25:22PM +0000, Winkler, Tomas wrote:
>
>
> >
> > The error logging for tpm2_commit_space() is in a wrong place. This commit
> > moves it inside that function.
> >
> > Cc: James Bottomley <[email protected]>
> > Signed-off-by: Jarkko Sakkinen <[email protected]>
> > Reviewed-by: Stefan Berger <[email protected]>
> > ---
> > 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 64510ed81b46..7ac6ada8428c 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:
> Maybe 'out' is too positive name for an error-only exit point?
> or you can use if (rc) ... dev_err()

out_err?

/Jarkko

2018-11-20 16:00:29

by Jarkko Sakkinen

Subject: Re: [PATCH v9 05/17] tpm: declare struct tpm_header

On Mon, Nov 19, 2018 at 09:33:31PM +0000, Winkler, Tomas wrote:
>
> >
> > Decleare struct tpm_header that replaces struct tpm_input_header and
> Typo

Thanks, will fix for the next version.

> > struct tpm_output_header.
> >
> > Signed-off-by: Jarkko Sakkinen <[email protected]>
> > Reviewed-by: Stefan Berger <[email protected]>
> > ---
> > drivers/char/tpm/tpm-interface.c | 9 ++++-----
> > drivers/char/tpm/tpm.h | 27 ++++++++++++---------------
> > drivers/char/tpm/tpm2-cmd.c | 4 ++--
> > drivers/char/tpm/tpm2-space.c | 6 +++---
> > 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, 28 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-
> > interface.c
> > index 7ac6ada8428c..cb0d44c158ae 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -67,7 +67,7 @@ static int tpm_validate_command(struct tpm_chip
> > *chip,
> > const u8 *cmd,
> > size_t len)
> > {
> > - const struct tpm_input_header *header = (const void *)cmd;
> > + const struct tpm_header *header = (const void *)cmd;
>
>
> Sometimes it is casted (void *), sometimes (struct tpm_header *)
> Better to use one convention

I would prefer (const void *) because it is shorter form, and no
need to duplicate the type name two times, that way also more
readable. Anyway, agreed that we do have inconsistency with these.

>
> > int i;
> > u32 cc;
> > u32 attrs;
> > @@ -166,7 +166,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip
> > *chip,
> > u8 *buf, size_t bufsiz,
> > unsigned int flags)
> > {
> > - struct tpm_output_header *header = (void *)buf;
> > + struct tpm_header *header = (void *)buf;
> > int rc;
> > ssize_t len = 0;
> > u32 count, ordinal;
> > @@ -316,7 +316,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
> > +381,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..86fa8ac0ae20 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 {
> Does it deserve kdoc?

Could be done if this is needed at some point in include/linux/tpm.h.
Do not see it right now as high priority.

> > + __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;
> No blank after ) in casting.

And also here void casting would make sense.

>
> > + 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;
> No blank after ) in casting.
> > u32 len = tpm_buf_length(buf);
> >
> > /* Return silently if overflow has already happened. */ 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..862ee1ce1755 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 = (void *)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 = (void *)rsp;
> > struct tpm2_cap_handles *data;
> > u32 phandle;
> > u32 phandle_type;
> > @@ -492,7 +492,7 @@ 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) {
> > - struct tpm_output_header *header = (void *)buf;
> > + struct tpm_header *header = (void *)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
>

Thank you. Really useful feedback.

/Jarkko

2018-11-20 16:00:33

by Tomas Winkler

Subject: RE: [PATCH v9 04/17] tpm: print tpm2_commit_space() error inside tpm2_commit_space()

>
> On Mon, Nov 19, 2018 at 09:25:22PM +0000, Winkler, Tomas wrote:
> >
> >
> > >
> > > The error logging for tpm2_commit_space() is in a wrong place. This
> > > commit moves it inside that function.
> > >
> > > Cc: James Bottomley <[email protected]>
> > > Signed-off-by: Jarkko Sakkinen <[email protected]>
> > > Reviewed-by: Stefan Berger <[email protected]>
> > > ---
> > > 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 64510ed81b46..7ac6ada8428c
> > > 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:
> > Maybe 'out' is too positive name for an error-only exit point?
> > or you can use if (rc) ... dev_err()
>
> out_err?
out_err is good, it is used in more places, so it has some of consistency.
Thanks
Tomas



>
> /Jarkko

2018-11-20 17:54:33

by Jason Gunthorpe

Subject: Re: [PATCH v9 05/17] tpm: declare struct tpm_header

On Tue, Nov 20, 2018 at 03:08:05PM +0200, Jarkko Sakkinen wrote:
> On Mon, Nov 19, 2018 at 09:33:31PM +0000, Winkler, Tomas wrote:
> >
> > >
> > > Decleare struct tpm_header that replaces struct tpm_input_header and
> > Typo
>
> Thanks, will fix for the next version.
>
> > > struct tpm_output_header.
> > >
> > > Signed-off-by: Jarkko Sakkinen <[email protected]>
> > > Reviewed-by: Stefan Berger <[email protected]>
> > > drivers/char/tpm/tpm-interface.c | 9 ++++-----
> > > drivers/char/tpm/tpm.h | 27 ++++++++++++---------------
> > > drivers/char/tpm/tpm2-cmd.c | 4 ++--
> > > drivers/char/tpm/tpm2-space.c | 6 +++---
> > > 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, 28 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-
> > > interface.c
> > > index 7ac6ada8428c..cb0d44c158ae 100644
> > > +++ b/drivers/char/tpm/tpm-interface.c
> > > @@ -67,7 +67,7 @@ static int tpm_validate_command(struct tpm_chip
> > > *chip,
> > > const u8 *cmd,
> > > size_t len)
> > > {
> > > - const struct tpm_input_header *header = (const void *)cmd;
> > > + const struct tpm_header *header = (const void *)cmd;
> >
> >
> > Sometimes it is casted (void *), sometimes (struct tpm_header *)
> > Better to use one convention
>
> I would prefer (const void *) because it is shorter form, and no
> need to duplicate the type name two times, that way also more
> readable. Anyway, agreed that we do have inconsistency with these.

It is better to pass in 'const void *cmd' than to explicitly cast like
this.

I generally discourage casts to voids to save typing, just creates
confusion what is going on. Better to try to avoid casts by
constructing the code properly.

Jason

2018-11-20 23:08:18

by Jarkko Sakkinen

Subject: Re: [PATCH v9 05/17] tpm: declare struct tpm_header

On Tue, Nov 20, 2018 at 09:01:11AM -0700, Jason Gunthorpe wrote:
> On Tue, Nov 20, 2018 at 03:08:05PM +0200, Jarkko Sakkinen wrote:
> > On Mon, Nov 19, 2018 at 09:33:31PM +0000, Winkler, Tomas wrote:
> > >
> > > >
> > > > Decleare struct tpm_header that replaces struct tpm_input_header and
> > > Typo
> >
> > Thanks, will fix for the next version.
> >
> > > > struct tpm_output_header.
> > > >
> > > > Signed-off-by: Jarkko Sakkinen <[email protected]>
> > > > Reviewed-by: Stefan Berger <[email protected]>
> > > > drivers/char/tpm/tpm-interface.c | 9 ++++-----
> > > > drivers/char/tpm/tpm.h | 27 ++++++++++++---------------
> > > > drivers/char/tpm/tpm2-cmd.c | 4 ++--
> > > > drivers/char/tpm/tpm2-space.c | 6 +++---
> > > > 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, 28 insertions(+), 33 deletions(-)
> > > >
> > > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-
> > > > interface.c
> > > > index 7ac6ada8428c..cb0d44c158ae 100644
> > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > @@ -67,7 +67,7 @@ static int tpm_validate_command(struct tpm_chip
> > > > *chip,
> > > > const u8 *cmd,
> > > > size_t len)
> > > > {
> > > > - const struct tpm_input_header *header = (const void *)cmd;
> > > > + const struct tpm_header *header = (const void *)cmd;
> > >
> > >
> > > Sometimes it is casted (void *), sometimes (struct tpm_header *)
> > > Better to use one convention
> >
> > I would prefer (const void *) because it is shorter form, and no
> > need to duplicate the type name two times, that way also more
> > readable. Anyway, agreed that we do have inconsistency with these.
>
> It is better to pass in 'const void *cmd' than to explicitly cast like
> this.
>
> I generally discourage casts to voids to save typing, just creates
> confusion what is going on. Better to try to avoid casts by
> constructing the code properly.

Definitely. Will change in the next version.

/Jarkko

2018-11-27 23:41:55

by Jarkko Sakkinen

Subject: Re: [PATCH v9 00/17] Removed nested TPM operations

On Sun, Nov 18, 2018 at 02:47:33PM +0200, Jarkko Sakkinen wrote:
> [was Detach TPM space code out of the tpm_transmit() flow but the scope
> expanded a bit.]
>
> 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.

I will take care to address the feedback from Tomas and Jason but won't
yet try to push this into v4.21. It is a patch set, which is better to
lie in linux-next before landing to a release. Probably will push the
patches soon after my v4.21 PR to linux-next. Then they will get maximum
time of exposure.

/Jarkko