* [PATCH v2] tpm: display warning if using gpio reset with TPM @ 2024-03-27 22:12 Tim Harvey 2024-03-28 7:08 ` Ilias Apalodimas 0 siblings, 1 reply; 8+ messages in thread From: Tim Harvey @ 2024-03-27 22:12 UTC (permalink / raw) To: u-boot, Ilias Apalodimas Cc: Miquel Raynal, Jorge Ramirez-Ortiz, Adam Ford, Rasmus Villemoes, Tim Harvey 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); -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] tpm: display warning if using gpio reset with TPM 2024-03-27 22:12 [PATCH v2] tpm: display warning if using gpio reset with TPM Tim Harvey @ 2024-03-28 7:08 ` Ilias Apalodimas 2024-04-08 7:23 ` Miquel Raynal 0 siblings, 1 reply; 8+ messages in thread From: Ilias Apalodimas @ 2024-03-28 7:08 UTC (permalink / raw) To: Tim Harvey Cc: u-boot, Miquel Raynal, Jorge Ramirez-Ortiz, Adam Ford, Rasmus Villemoes 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); > -- > 2.25.1 > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] tpm: display warning if using gpio reset with TPM 2024-03-28 7:08 ` Ilias Apalodimas @ 2024-04-08 7:23 ` Miquel Raynal 2024-04-17 5:40 ` Ilias Apalodimas 0 siblings, 1 reply; 8+ messages in thread From: Miquel Raynal @ 2024-04-08 7:23 UTC (permalink / raw) To: Ilias Apalodimas Cc: Tim Harvey, u-boot, Jorge Ramirez-Ortiz, Adam Ford, Rasmus Villemoes 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. Thanks, Miquèl ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] tpm: display warning if using gpio reset with TPM 2024-04-08 7:23 ` Miquel Raynal @ 2024-04-17 5:40 ` Ilias Apalodimas 2024-04-17 6:48 ` Miquel Raynal 0 siblings, 1 reply; 8+ messages in thread From: Ilias Apalodimas @ 2024-04-17 5:40 UTC (permalink / raw) To: Miquel Raynal Cc: Tim Harvey, u-boot, Jorge Ramirez-Ortiz, Adam Ford, Rasmus Villemoes 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? If so I'll send a PR to Tom as is and rework it as suggested later Thanks /Ilias > > Thanks, > Miquèl ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] tpm: display warning if using gpio reset with TPM 2024-04-17 5:40 ` Ilias Apalodimas @ 2024-04-17 6:48 ` Miquel Raynal 2024-04-17 7:00 ` Ilias Apalodimas 0 siblings, 1 reply; 8+ messages in thread From: Miquel Raynal @ 2024-04-17 6:48 UTC (permalink / raw) To: Ilias Apalodimas Cc: Tim Harvey, u-boot, Jorge Ramirez-Ortiz, Adam Ford, Rasmus Villemoes 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] tpm: display warning if using gpio reset with TPM 2024-04-17 6:48 ` Miquel Raynal @ 2024-04-17 7:00 ` Ilias Apalodimas 2024-04-17 14:58 ` Tim Harvey 0 siblings, 1 reply; 8+ messages in thread From: Ilias Apalodimas @ 2024-04-17 7:00 UTC (permalink / raw) To: Miquel Raynal Cc: Tim Harvey, u-boot, Jorge Ramirez-Ortiz, Adam Ford, Rasmus Villemoes On Wed, 17 Apr 2024 at 09:48, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > 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? 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] tpm: display warning if using gpio reset with TPM 2024-04-17 7:00 ` Ilias Apalodimas @ 2024-04-17 14:58 ` Tim Harvey 2024-04-17 15:07 ` Ilias Apalodimas 0 siblings, 1 reply; 8+ messages in thread From: Tim Harvey @ 2024-04-17 14:58 UTC (permalink / raw) To: Ilias Apalodimas Cc: Miquel Raynal, u-boot, Jorge Ramirez-Ortiz, Adam Ford, Rasmus Villemoes On Wed, Apr 17, 2024 at 12:01 AM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Wed, 17 Apr 2024 at 09:48, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > 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? > > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] tpm: display warning if using gpio reset with TPM 2024-04-17 14:58 ` Tim Harvey @ 2024-04-17 15:07 ` Ilias Apalodimas 0 siblings, 0 replies; 8+ messages in thread From: Ilias Apalodimas @ 2024-04-17 15:07 UTC (permalink / raw) To: Tim Harvey Cc: Miquel Raynal, u-boot, Jorge Ramirez-Ortiz, Adam Ford, Rasmus Villemoes Hi Tim! > > > > > > > > > > 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. Thanks! /Ilias > > Best Regards, > > Tim ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-04-17 15:08 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-27 22:12 [PATCH v2] tpm: display warning if using gpio reset with TPM Tim Harvey 2024-03-28 7:08 ` Ilias Apalodimas 2024-04-08 7:23 ` Miquel Raynal 2024-04-17 5:40 ` Ilias Apalodimas 2024-04-17 6:48 ` Miquel Raynal 2024-04-17 7:00 ` Ilias Apalodimas 2024-04-17 14:58 ` Tim Harvey 2024-04-17 15:07 ` Ilias Apalodimas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox