On Sun, Jul 10, 2022 at 10:58 PM Jarkko Sakkinen <jarkko@xxxxxxxxxx> wrote:
>
> On Wed, Jul 06, 2022 at 12:40:43PM -0400, Jason Andryuk wrote:
> > WEC TPMs (in 1.2 mode) and NTC (in 2.0 mode) have been observer to
> > frequently, but intermittently, fail probe with:
> > tpm_tis: probe of 00:09 failed with error -1
> >
> > Added debugging output showed that the request_locality in
> > tpm_tis_core_init succeeds, but then the tpm_chip_start fails when its
> > call to tpm_request_locality -> request_locality fails.
> >
> > The access register in check_locality would show:
> > 0x80 TPM_ACCESS_VALID
> > 0x82 TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_USE
> > 0x80 TPM_ACCESS_VALID
> > continuing until it times out. TPM_ACCESS_ACTIVE_LOCALITY (0x20) doesn't
> > get set which would end the wait.
> >
> > My best guess is something racy was going on between release_locality's
> > write and request_locality's write. There is no wait in
> > release_locality to ensure that the locality is released, so the
> > subsequent request_locality could confuse the TPM?
> >
> > tpm_chip_start grabs locality 0, and updates chip->locality. Call that
> > before the TPM_INT_ENABLE write, and drop the explicit request/release
> > calls. tpm_chip_stop performs the release. With this, we switch to
> > using chip->locality instead of priv->locality. The probe failure is
> > not seen after this.
> >
> > commit 0ef333f5ba7f ("tpm: add request_locality before write
> > TPM_INT_ENABLE") added a request_locality/release_locality pair around
> > tpm_tis_write32 TPM_INT_ENABLE, but there is a read of
> > TPM_INT_ENABLE for the intmask which should also have the locality
> > grabbed. tpm_chip_start is moved before that to have the locality open
> > during the read.
> >
> > Fixes: 0ef333f5ba7f ("tpm: add request_locality before write TPM_INT_ENABLE")
> > CC: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> > ---
> > The probe failure was seen on 5.4, 5.15 and 5.17.
> >
> > commit e42acf104d6e ("tpm_tis: Clean up locality release") removed the
> > release wait. I haven't tried, but re-introducing that would probably
> > fix this issue. It's hard to know apriori when a synchronous wait is
> > needed, and they don't seem to be needed typically. Re-introducing the
> > wait would re-introduce a wait in all cases.
> >
> > Surrounding the read of TPM_INT_ENABLE with grabbing the locality may
> > not be necessary? It looks like the code only grabs a locality for
> > writing, but that asymmetry is surprising to me.
> >
> > tpm_chip and tpm_tis_data track the locality separately. Should the
> > tpm_tis_data one be removed so they don't get out of sync?
> > ---
> > drivers/char/tpm/tpm_tis_core.c | 20 ++++++++------------
> > 1 file changed, 8 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > index dc56b976d816..529c241800c0 100644
> > --- a/drivers/char/tpm/tpm_tis_core.c
> > +++ b/drivers/char/tpm/tpm_tis_core.c
> > @@ -986,8 +986,13 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> > goto out_err;
> > }
> >
> > + /* Grabs locality 0. */
> > + rc = tpm_chip_start(chip);
> > + if (rc)
> > + goto out_err;
> > +
> > /* Take control of the TPM's interrupt hardware and shut it off */
> > - rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> > + rc = tpm_tis_read32(priv, TPM_INT_ENABLE(chip->locality), &intmask);
> > if (rc < 0)
> > goto out_err;
> >
> > @@ -995,19 +1000,10 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> > TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
> > intmask &= ~TPM_GLOBAL_INT_ENABLE;
> >
> > - rc = request_locality(chip, 0);
> > - if (rc < 0) {
> > - rc = -ENODEV;
> > - goto out_err;
> > - }
> > -
> > - tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> > - release_locality(chip, 0);
> > + tpm_tis_write32(priv, TPM_INT_ENABLE(chip->locality), intmask);
> >
> > - rc = tpm_chip_start(chip);
> > - if (rc)
> > - goto out_err;
> > rc = tpm2_probe(chip);
> > + /* Releases locality 0. */
> > tpm_chip_stop(chip);
> > if (rc)
> > goto out_err;
> > --
> > 2.36.1
> >
>
> Can you test against
>
> https://lore.kernel.org/linux-integrity/20220629232653.1306735-1-LinoSanfilippo@xxxxxx/T/#t
I applied on top of 5.15.53, and the probe on boot still fails.
Manually probing works intermittently.
Regards,
Jason
