[PATCH v2] tpm: display warning if using gpio reset with TPM

[Ilias Apalodimas]

Hi Miquel

On Mon, 8 Apr 2024 at 10:23, Miquel Raynal <miquel.raynal at bootlin.com> wrote:
>
> Hello,
>
> ilias.apalodimas at linaro.org wrote on Thu, 28 Mar 2024 09:08:37 +0200:
>
> > Thanks Tim
> >
> > On Thu, 28 Mar 2024 at 00:12, 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 display a warning that
> > > having a gpio reset on a TPM should not be used for a secure production
> > > device.
> > >
> > > TCG TIS spec [1] 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."
> > >
> > > The reasoning is that you should not be able to toggle a GPIO and reset
> > > the TPM without resetting the CPU as well because if an attacker can
> > > break into your OS via an OS level security flaw they can then reset the
> > > TPM via GPIO and replay the measurements required to unseal keys
> > > that you have otherwise protected.
> > >
> > > [1] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf
> > >
> > > Signed-off-by: Tim Harvey <tharvey at gateworks.com>
> > > ---
> > > v2: change the message to a warning and update commit desc/log
> > > ---
> > >  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..c9c83f6f0fc8 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_WARNING,
> > > +                   "%s: TPM gpio reset should not be used on secure production devices\n",
> > > +                   dev->name);
> > >                 dm_gpio_set_value(&reset_gpio, 1);
> > >                 mdelay(1);
> > >                 dm_gpio_set_value(&reset_gpio, 0);
>
> The current logic expects a reset gpio and bails out if it cannot get
> it with a (questionable) goto statement.
>
> You want to invert that logic, and expect no gpio, but only if there is
> one you want to warn.
>
> This is perfectly fine but the logic here must be clarified. I'd go for:
>
> ret = gpio_request()
> if (ret) {
>         ret = gpio_request()
>         if (!ret)
>                 warn(deprecated)
> }
>
> if (!ret) {
>         warn(dangerous)
>         toggle_value()
> }
>
> I would ideally replace the 'if (ret)' clauses with 'if (!reset_gpio)'
> which would make the checks even clearer.

Fair enough. But the code with the proposed change has no functional
problems right?
If so I'll send a PR to Tom as is and rework it as suggested later

Thanks
/Ilias
>
> Thanks,
> Miquèl