Re: [PATCH v15 1/6] tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception
From: Jarkko Sakkinen <jarkko@kernel.org>
Date: 2021-09-14 16:58:19
Also in:
linux-integrity, lkml
On Tue, 2021-09-14 at 18:10 +0300, amirmizi6@gmail.com wrote:
From: Amir Mizinski <redacted> Detected an incorrect implementation of the send command. Currently, the driver polls the TPM_STS.stsValid field until TRUE; then it reads TPM_STS register again to verify only that TPM_STS.expect field is FALSE (i.e., it ignores TPM_STS.stsValid). Since TPM_STS.stsValid represents the TPM_STS.expect validity, both fields fields should be checked in the same TPM_STS register read value.
This is missing description of what kind of error/consquence this caused. Perhaps you got something to the klog, or how did you find out about the issue? Since you have reproduced, please connect it to the reality.
Modify the signature of 'wait_for_tpm_stat()', add an additional "mask_result" parameter to its call and rename it to 'tpm_tis_wait_for_stat()' for better alignment with other naming. 'tpm_tis_wait_for_stat()' is now polling the TPM_STS with a mask and waits for the value in mask_result. Add the ability to check if certain TPM_STS bits have been cleared.
The commit description is probably out of sync (not only rename, there is no parameter called mask_result). It's also lacking description, how this new parameter is taken advantage of. E.g. "Use the new parameter to check that status TPM_STS_VALID is set, in addition that TPM_STS_EXPECT is zeroed. This prevents a racy checkk
quoted hunk ↗ jump to hunk
In addition, the send command was changed to comply with TCG_DesignPrinciples_TPM2p0Driver_vp24_pubrev.pdf as follows: - send all command bytes in one loop - remove special handling of the last byte Suggested-by: Benoit Houyere <redacted> Signed-off-by: Amir Mizinski <redacted> --- drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++++++-------------------------- 1 file changed, 25 insertions(+), 43 deletions(-)diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index 69579ef..7d5854b 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c@@ -44,9 +44,9 @@ static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask, return false; } -static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, - unsigned long timeout, wait_queue_head_t *queue, - bool check_cancel) +static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, u8 stat, + unsigned long timeout, + wait_queue_head_t *queue, bool check_cancel)
This naming is not too great, considering that there is already local variable called status.
quoted hunk ↗ jump to hunk
{ unsigned long stop; long rc;@@ -55,7 +55,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, /* check current status */ status = chip->ops->status(chip); - if ((status & mask) == mask) + if ((status & mask) == stat) return 0; stop = jiffies + timeout;@@ -83,7 +83,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, usleep_range(TPM_TIMEOUT_USECS_MIN, TPM_TIMEOUT_USECS_MAX); status = chip->ops->status(chip); - if ((status & mask) == mask) + if ((status & mask) == stat) return 0; } while (time_before(jiffies, stop)); }@@ -260,9 +260,10 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count) while (size < count) { rc = wait_for_tpm_stat(chip, - TPM_STS_DATA_AVAIL | TPM_STS_VALID, - chip->timeout_c, - &priv->read_queue, true); + TPM_STS_DATA_AVAIL | TPM_STS_VALID, + TPM_STS_DATA_AVAIL | TPM_STS_VALID, + chip->timeout_c, &priv->read_queue, + true); if (rc < 0) return rc; burstcnt = get_burstcount(chip);@@ -315,8 +316,9 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count) goto out; } - if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c, - &priv->int_queue, false) < 0) { + if (wait_for_tpm_stat(chip, TPM_STS_VALID, TPM_STS_VALID, + chip->timeout_c, &priv->int_queue, + false) < 0) { size = -ETIME; goto out; }@@ -342,61 +344,40 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len) struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); int rc, status, burstcnt; size_t count = 0; - bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND; status = tpm_tis_status(chip); if ((status & TPM_STS_COMMAND_READY) == 0) { tpm_tis_ready(chip); - if (wait_for_tpm_stat - (chip, TPM_STS_COMMAND_READY, chip->timeout_b, - &priv->int_queue, false) < 0) { + if (wait_for_tpm_stat(chip, TPM_STS_COMMAND_READY, + TPM_STS_COMMAND_READY, + chip->timeout_b, &priv->int_queue, + false) < 0) { rc = -ETIME; goto out_err; } } - while (count < len - 1) { + while (count < len) {
This.
burstcnt = get_burstcount(chip);
if (burstcnt < 0) {
dev_err(&chip->dev, "Unable to read burstcount\n");
rc = burstcnt;
goto out_err;
}
- burstcnt = min_t(int, burstcnt, len - count - 1);
+ burstcnt = min_t(int, burstcnt, len - count);What are these two changes (loop condition and the right above change)?
quoted hunk ↗ jump to hunk
rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), burstcnt, buf + count); if (rc < 0) goto out_err; count += burstcnt; - - if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c, - &priv->int_queue, false) < 0) { - rc = -ETIME; - goto out_err; - } - status = tpm_tis_status(chip); - if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) { - rc = -EIO; - goto out_err; - } } - - /* write last byte */ - rc = tpm_tis_write8(priv, TPM_DATA_FIFO(priv->locality), buf[count]); - if (rc < 0) - goto out_err; - - if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c, - &priv->int_queue, false) < 0) { + if (wait_for_tpm_stat(chip, TPM_STS_VALID | TPM_STS_DATA_EXPECT, + TPM_STS_VALID, chip->timeout_a, + &priv->int_queue, false) < 0) { rc = -ETIME; goto out_err; } - status = tpm_tis_status(chip); - if (!itpm && (status & TPM_STS_DATA_EXPECT) != 0) { - rc = -EIO; - goto out_err; - } return 0;@@ -451,9 +432,10 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len) ordinal = be32_to_cpu(*((__be32 *) (buf + 6))); dur = tpm_calc_ordinal_duration(chip, ordinal); - if (wait_for_tpm_stat - (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID, dur, - &priv->read_queue, false) < 0) { + if (wait_for_tpm_stat(chip, + TPM_STS_DATA_AVAIL | TPM_STS_VALID, + TPM_STS_DATA_AVAIL | TPM_STS_VALID, + dur, &priv->read_queue, false) < 0) { rc = -ETIME; goto out_err; }
/Jarkko