| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMHTsUXt7F0uWTe7qJSi8YnT-8JcXYYrdzEwi7UhN5m9c4hzYQ@mail.gmail.com>
Date: Wed, 13 Oct 2021 11:49:49 +0300
From: Amir Mizinski <amirmizi6@...il.com>
To: Jarkko Sakkinen <jarkko@...nel.org>
Cc: Eyal.Cohen@...oton.com, Oshri Alkobi <oshrialkoby85@...il.com>,
Alexander Steffen <alexander.steffen@...ineon.com>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>, peterhuewe@....de,
jgg@...pe.ca, Arnd Bergmann <arnd@...db.de>,
Greg KH <gregkh@...uxfoundation.org>,
Benoit HOUYERE <benoit.houyere@...com>,
Eddie James <eajames@...ux.ibm.com>,
Joel Stanley <joel@....id.au>,
devicetree <devicetree@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
linux-integrity@...r.kernel.org,
IS20 Oshri Alkoby <oshri.alkoby@...oton.com>,
Tomer Maimon <tmaimon77@...il.com>, gcwilson@...ibm.com,
kgoldman@...ibm.com, IS30 Dan Morav <Dan.Morav@...oton.com>,
oren.tanami@...oton.com, shmulik.hager@...oton.com,
amir.mizinski@...oton.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
Hello Jarkko, apologies for the delay and thank you for your comments.
I'll answer your comments below.
On Tue, 14 Sept 2021 at 19:58, Jarkko Sakkinen <jarkko@...nel.org> wrote:
>
> On Tue, 2021-09-14 at 18:10 +0300, amirmizi6@...il.com wrote:
> > From: Amir Mizinski <amirmizi6@...il.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.
>
We found out about this issue in a code review, and there's no
specific error i can reproduce.
The main problem here is that the current check is meaningless. Since
TPM_STS.stsValid represents only the validity of the other bits on
TPM_STS it makes no sense to check it on its own.
Maybe it's better if i'll add a fix tag in here?
> > 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
>
Duly noted, ill fix this for next version.
>
> > 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@...com>
> > Signed-off-by: Amir Mizinski <amirmizi6@...il.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.
>
i will change this to result. is that better?
>
> > {
> > 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)?
>
These changes are related to unnecessary handling of the last byte,
this is described on the last paragraph of the commit message.
> > 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
>
Thank you,
Amir Mizinski
Powered by blists - more mailing lists