On Sun, Feb 25, 2018 at 10:37:08AM +0000, Winkler, Tomas wrote:

quoted

On Sat, Feb 24, 2018 at 05:43:16PM +0200, Tomas Winkler wrote:

quoted

The correct sequence is to first request locality and only after that
perform cmd_ready handshake, otherwise the hardware will drop the
subsequent message as from the device point of view the cmd_ready
handshake wasn't performed. Symmetrically locality has to be
relinquished only after going idle handshake has completed, this
requires that go_idle has to poll for the completion and as well
locality relinquish has to poll for completion so it is not overridden
in back to back commands flow.

The issue is only visible on devices that support multiple localities.

Signed-off-by: Tomas Winkler <redacted>
Tested-by: Jarkko Sakkinen <redacted>
---
V2: poll for locality relinquish completion
V3: 1. Print error message upon locality relinquish failure
    2. Don't override rc code on error path with locality relinquish
V4: 3. Don't capture locality relinquish error code in rc, just print
	the error message.

       return value.
 drivers/char/tpm/tpm-interface.c |  20 +++++---
 drivers/char/tpm/tpm_crb.c       | 108 +++++++++++++++++++++++++++------

------

quoted

 drivers/char/tpm/tpm_tis_core.c  |   4 +-
 include/linux/tpm.h              |   2 +-
 4 files changed, 93 insertions(+), 41 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c

b/drivers/char/tpm/tpm-interface.c
index 9e80a953d693..4d74bacca5a1 100644

--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c

@@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct

tpm_space *space,

quoted

 	if (!(flags & TPM_TRANSMIT_UNLOCKED))
 		mutex_lock(&chip->tpm_mutex);

-	if (chip->dev.parent)
-		pm_runtime_get_sync(chip->dev.parent);

 	if (chip->ops->clk_enable != NULL)
 		chip->ops->clk_enable(chip, true);

@@ -439,6 +437,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct

tpm_space *space,

quoted

 		chip->locality = rc;
 	}

+	if (chip->dev.parent)
+		pm_runtime_get_sync(chip->dev.parent);
+
 	rc = tpm2_prepare_space(chip, space, ordinal, buf);
 	if (rc)
 		goto out;

@@ -499,17 +500,24 @@ ssize_t tpm_transmit(struct tpm_chip *chip,

struct tpm_space *space,

quoted

 	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);

 out:
+	if (chip->dev.parent)
+		pm_runtime_put_sync(chip->dev.parent);
+
 	if (need_locality && chip->ops->relinquish_locality) {
-		chip->ops->relinquish_locality(chip, chip->locality);
+		/* this coud be on error path, don't override error code */
+		int l_rc = chip->ops->relinquish_locality(chip, chip->locality);

Declaration should be in the beginning of the function.

No, it shouldn't. I cannot find any reference to this statement, I've already explained my reasoning in a previous mail. 
  

quoted

quoted

+
+		if (l_rc) {
+			dev_err(&chip->dev, "%s: relinquish_locality: error

%d\n",

quoted

+				__func__, l_rc);
+		}

In kernel coding style, this should be w/o curly braces.

Yep, missed that, will resubmit

quoted

I can fix these cosmetic issues

Reviewed-by: Jarkko Sakkinen <redacted>

Doesn't this need

Fixes: 877c57d0d0ca ("tpm_crb: request and relinquish locality 0")
And shouldn't this also have

Cc: stable at vger.kernel.org

?

Good points
Thanks
Tomas

Tomas, I updated v4 myself and pushed it to master/next. Please tell
me if there is anything wrong and I will fix it.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html