On Wed, 17 Apr 2024 at 09:48, Miquel Raynal <miquel.ray...@bootlin.com> wrote: > > Hi Ilias, > > ilias.apalodi...@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.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? > > 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? Thanks /Ilias > > Thanks, > Miquèl
