[PATCH] tpm: display message when using gpio-reset instead of when missing it

[Ilias Apalodimas]

Hi Tim,

On Wed, 27 Mar 2024 at 17:29, Tim Harvey <tharvey at gateworks.com> wrote:
>
> On Wed, Mar 27, 2024 at 7:44 AM Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > Hi Tim,
> >
> > On Thu, 21 Mar 2024 at 20:02, Tim Harvey <tharvey at gateworks.com> wrote:
> > >
> > > Instead of displaying what looks like an error message if a
> > > gpio-reset dt prop is missing for a TPM dipslay a more
> > > informative message about resetting the TPM if the gpio is found:
> > >
> > > before:
> > > tpm_tis_spi_probe: missing reset GPIO
> > >
> > > after:
> > > tpm at 0: performing 1ms reset on gpio at 30210000:12
> > >
> > > Note that the reset dt binding prop used in this driver is not
> > > dt-compliant; it does not exist in the Linux dt-bindings documentation
> > > and the reset is not done by the Linux driver.
> >
> > Probably for a good reason. You aren't supposed to be able to reset a
> > TPM without resetting the CPUI as well no?
>
> Hi Ilias,
>
> Could you clarify what you know about TPM reset? We use the ATTPM20P
> [1] which states in the datasheet under the reset pin: "To be
> compliant with TCG requirements, this pin needs to be tied to system
> reset. TPM_Init is indicated by asserting this pin."

In short, you shouldn't be able to toggle a GPIO and reset the TPM
without resetting the CPU as well.
An attacker could boot -> log into the OS -> reset the TPM -> replay
measurements and unseal keys that he shouldn't.

>
> Our boards have a resistor loading option which routes the TPM RST# to
> an SoC GPIO or alternately to a POR# (hardware power on reset provided
> by power supply and/or PMIC). Could you point me to where in the spec
> it explains what the TPM reset should be connected to?

I am aware of the the TCG TIS spec [0] which says
"The TPM_Init (LRESET#/SPI_RST#) signal MUST be connected to the platform CPU
Reset signal such that it complies with the requirements specified in
section 1.2.7 HOST
Platform Reset in the PC Client Implementation Specification for
Conventional BIOS."
There might be other TCG specs defining this as well.

[0] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf

>
> > That being said, printing that the TPM was reset is pointless imho.
> > OTOH the existing error message at least points out a potential
> > problem in the DT.
> >
>
> I'm not sure if you are NAK'ing this patch or asking me to change it.
>
> Displaying a 'missing GPIO' message is not helpful when there is no
> GPIO in the dt bindings to begin with.

I am not NAKing it. But isn't that message more useful than printing
the TPM did a reset? At least you get a hint of something that's
missing from your DT

Thanks
/Ilias
>
> Best Regards,
>
> Tim
> [1] https://ww1.microchip.com/downloads/en/DeviceDoc/ATTPM20P-Trusted-Platform-Module-TPM-2.0-SPI-Interface-Summary-Data-Sheet-DS40002082A.pdf
>
>
> > Thanks
> > /Ilias
> > >
> > > Signed-off-by: Tim Harvey <tharvey at gateworks.com>
> > > ---
> > >  drivers/tpm/tpm2_tis_spi.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
> > > index de9cf8f21e07..944540f7a711 100644
> > > --- a/drivers/tpm/tpm2_tis_spi.c
> > > +++ b/drivers/tpm/tpm2_tis_spi.c
> > > @@ -237,14 +237,14 @@ static int tpm_tis_spi_probe(struct udevice *dev)
> > >                         /* legacy reset */
> > >                         ret = gpio_request_by_name(dev, "gpio-reset", 0,
> > >                                                    &reset_gpio, GPIOD_IS_OUT);
> > > -                       if (ret) {
> > > -                               log(LOGC_NONE, LOGL_NOTICE,
> > > -                                   "%s: missing reset GPIO\n", __func__);
> > > +                       if (ret)
> > >                                 goto init;
> > > -                       }
> > >                         log(LOGC_NONE, LOGL_NOTICE,
> > >                             "%s: gpio-reset is deprecated\n", __func__);
> > >                 }
> > > +               log(LOGC_NONE, LOGL_NOTICE,
> > > +                   "%s: performing 1ms reset on %s:%d\n", dev->name,
> > > +                   reset_gpio.dev->name, reset_gpio.offset);
> > >                 dm_gpio_set_value(&reset_gpio, 1);
> > >                 mdelay(1);
> > >                 dm_gpio_set_value(&reset_gpio, 0);
> > > --
> > > 2.25.1
> > >