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

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Ilias,

ilias.apalodimas@linaro.org wrote on Wed, 17 Apr 2024 08:40:14 +0300:

> Hi Miquel
> 
> On Mon, 8 Apr 2024 at 10:23, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hello,
> >
> > ilias.apalodimas@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@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@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?

No, this is functionally right, but the code is not clear like that.

> If so I'll send a PR to Tom as is and rework it as suggested later

Well, that's not how contribution work usually. Is there an emergency
in getting this merged?

Thanks,
Miquèl