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

[Tim Harvey]

On Wed, Apr 17, 2024 at 12:01 AM Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> On Wed, 17 Apr 2024 at 09:48, Miquel Raynal <miquel.raynal at bootlin.com> wrote:
> >
> > Hi Ilias,
> >
> > ilias.apalodimas at 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 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?
> >
> > 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?
>
> Not really, it's a print message. But I don't currently have time to
> pick this up.
> Tim, would you mind reworking it as Miquel suggested?
>

I'm just catching up after being out of the office for a couple of
weeks - I'll rework it and submit another revision as soon as I have
some time.

Best Regards,

Tim