From: Jarkko Sakkinen <jarkko@kernel.org>
To: amirmizi6@gmail.com, Eyal.Cohen@nuvoton.com,
oshrialkoby85@gmail.com, alexander.steffen@infineon.com,
robh+dt@kernel.org, mark.rutland@arm.com, peterhuewe@gmx.de,
jgg@ziepe.ca, arnd@arndb.de, gregkh@linuxfoundation.org,
benoit.houyere@st.com, eajames@linux.ibm.com, joel@jms.id.au
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-integrity@vger.kernel.org, oshri.alkoby@nuvoton.com,
tmaimon77@gmail.com, gcwilson@us.ibm.com, kgoldman@us.ibm.com,
Dan.Morav@nuvoton.com, oren.tanami@nuvoton.com,
shmulik.hager@nuvoton.com, amir.mizinski@nuvoton.com
Subject: Re: [PATCH v15 1/6] tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception
Date: Tue, 14 Sep 2021 19:58:12 +0300 [thread overview]
Message-ID: <d9082b1b4c2e358c97bbc815c1b06e1b05011b0e.camel@kernel.org> (raw)
In-Reply-To: <20210914151032.216122-2-amirmizi6@gmail.com>
On Tue, 2021-09-14 at 18:10 +0300, amirmizi6@gmail.com wrote:
> From: Amir Mizinski <amirmizi6@gmail.com>
>
> 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
> 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 <benoit.houyere@st.com>
> Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
> ---
> 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.
> {
> 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)?
> 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
next prev parent reply other threads:[~2021-09-14 16:58 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-14 15:10 [PATCH v15 0/6] Add tpm i2c ptp driver amirmizi6
2021-09-14 15:10 ` [PATCH v15 1/6] tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception amirmizi6
2021-09-14 16:58 ` Jarkko Sakkinen [this message]
2021-10-13 8:49 ` Amir Mizinski
2021-09-14 15:10 ` [PATCH v15 2/6] tpm: tpm_tis: Rewrite "tpm_tis_req_canceled()" amirmizi6
2021-09-14 15:10 ` [PATCH v15 3/6] tpm: Handle an exception for TPM Firmware Update mode amirmizi6
2021-09-14 15:10 ` [PATCH v15 4/6] tpm: tpm_tis: verify TPM_STS register is valid after locality request amirmizi6
2021-09-14 15:10 ` [PATCH v15 5/6] tpm: tpm_tis: add tpm_tis_i2c driver amirmizi6
2021-09-14 15:10 ` [PATCH v15 6/6] tpm: Add YAML schema for TPM TIS I2C options amirmizi6
2021-09-16 14:43 ` Rob Herring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d9082b1b4c2e358c97bbc815c1b06e1b05011b0e.camel@kernel.org \
--to=jarkko@kernel.org \
--cc=Dan.Morav@nuvoton.com \
--cc=Eyal.Cohen@nuvoton.com \
--cc=alexander.steffen@infineon.com \
--cc=amir.mizinski@nuvoton.com \
--cc=amirmizi6@gmail.com \
--cc=arnd@arndb.de \
--cc=benoit.houyere@st.com \
--cc=devicetree@vger.kernel.org \
--cc=eajames@linux.ibm.com \
--cc=gcwilson@us.ibm.com \
--cc=gregkh@linuxfoundation.org \
--cc=jgg@ziepe.ca \
--cc=joel@jms.id.au \
--cc=kgoldman@us.ibm.com \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=oren.tanami@nuvoton.com \
--cc=oshri.alkoby@nuvoton.com \
--cc=oshrialkoby85@gmail.com \
--cc=peterhuewe@gmx.de \
--cc=robh+dt@kernel.org \
--cc=shmulik.hager@nuvoton.com \
--cc=tmaimon77@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.