Hi Miquel On Mon, 8 Apr 2024 at 10:23, Miquel Raynal <miquel.ray...@bootlin.com> wrote: > > Hello, > > ilias.apalodi...@linaro.org wrote on Thu, 28 Mar 2024 09:08:37 +0200: > > > Thanks Tim > > > > On Thu, 28 Mar 2024 at 00:12, Tim Harvey <thar...@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 <thar...@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
