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

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

[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.

Additionally restructure the code for improved readability allowing for
removal of the init label.

Before:
 - board with no reset gpio
u-boot=> tpm init && tpm info
tpm_tis_spi_probe: missing reset GPIO
tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
 - board with a reset gpio
u-boot=> tpm init && tpm info
tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]

After:
 - board with no reset gpio
u-boot=> tpm init && tpm info
tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
 - board with a reset gpio
u-boot=> tpm init && tpm info
tpm@1: TPM gpio reset should not be used on secure production devices
tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]

[1] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
v3: restructure code for improved readability (recommended by Miquel)
v2: change the message to a warning
---
 drivers/tpm/tpm2_tis_spi.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
index 28079b5039a3..b0fe97ab1d08 100644
--- a/drivers/tpm/tpm2_tis_spi.c
+++ b/drivers/tpm/tpm2_tis_spi.c
@@ -237,19 +237,22 @@ 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) {
+			if (!ret) {
 				log(LOGC_NONE, LOGL_NOTICE,
-				    "%s: missing reset GPIO\n", __func__);
-				goto init;
+				    "%s: gpio-reset is deprecated\n", __func__);
 			}
-			log(LOGC_NONE, LOGL_NOTICE,
-			    "%s: gpio-reset is deprecated\n", __func__);
 		}
-		dm_gpio_set_value(&reset_gpio, 1);
-		mdelay(1);
-		dm_gpio_set_value(&reset_gpio, 0);
+
+		if (!ret) {
+			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);
+		}
 	}
-init:
+
 	/* Ensure a minimum amount of time elapsed since reset of the TPM */
 	mdelay(drv_data->time_before_first_cmd_ms);
 
-- 
2.25.1

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

[Miquel Raynal]

Hi Tim,

tharvey@gateworks.com wrote on Wed, 15 May 2024 16:21:38 -0700:

> 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.
> 
> Additionally restructure the code for improved readability allowing for
> removal of the init label.
> 
> Before:
>  - board with no reset gpio
> u-boot=> tpm init && tpm info
> tpm_tis_spi_probe: missing reset GPIO
> tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
>  - board with a reset gpio
> u-boot=> tpm init && tpm info
> tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
> 
> After:
>  - board with no reset gpio
> u-boot=> tpm init && tpm info
> tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
>  - board with a reset gpio
> u-boot=> tpm init && tpm info
> tpm@1: TPM gpio reset should not be used on secure production devices
> tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
> 
> [1] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>

Looks way cleaner, thanks.

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Miquèl

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

[Jorge Ramirez-Ortiz, Foundries]

On 16/05/24 09:09:50, Miquel Raynal wrote:
> Hi Tim,
> 
> tharvey@gateworks.com wrote on Wed, 15 May 2024 16:21:38 -0700:
> 
> > 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.
> > 
> > Additionally restructure the code for improved readability allowing for
> > removal of the init label.
> > 
> > Before:
> >  - board with no reset gpio
> > u-boot=> tpm init && tpm info
> > tpm_tis_spi_probe: missing reset GPIO
> > tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
> >  - board with a reset gpio
> > u-boot=> tpm init && tpm info
> > tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
> > 
> > After:
> >  - board with no reset gpio
> > u-boot=> tpm init && tpm info
> > tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
> >  - board with a reset gpio
> > u-boot=> tpm init && tpm info
> > tpm@1: TPM gpio reset should not be used on secure production devices
> > tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
> > 
> > [1] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf
> > 
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> 
> Looks way cleaner, thanks.
> 
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> Miquèl

nice. if needed

Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>

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

[Miquel Raynal]

Hi Jorge,

...

> > >  - board with no reset gpio
> > > u-boot=> tpm init && tpm info
> > > tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
> > >  - board with a reset gpio
> > > u-boot=> tpm init && tpm info
> > > tpm@1: TPM gpio reset should not be used on secure production devices
> > > tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
> > > 
> > > [1] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf
> > > 
> > > Signed-off-by: Tim Harvey <tharvey@gateworks.com>  
> > 
> > Looks way cleaner, thanks.
> > 
> > Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > 
> > Miquèl  
> 
> nice. if needed
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>

You cannot send your SoB like this. SoB means you are carrying some
code which complies with the license, etc.

Either you were part of the original writing and want to be credited
for that (you can be the author and first SoB, or suggest Tim to use
Co-developed-by). Or you reviewed the change (Reviewed-by), you tested
the change (Tested-by), or you are maintainer/responsible for some part
that is touched and you agree with the change (Acked-by).

Thanks,
Miquèl

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

[Jorge Ramirez-Ortiz, Foundries]

On 16/05/24 11:34:14, Miquel Raynal wrote:
> Hi Jorge,
> 
> ...
> 
> > > >  - board with no reset gpio
> > > > u-boot=> tpm init && tpm info
> > > > tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
> > > >  - board with a reset gpio
> > > > u-boot=> tpm init && tpm info
> > > > tpm@1: TPM gpio reset should not be used on secure production devices
> > > > tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
> > > > 
> > > > [1] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf
> > > > 
> > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com>  
> > > 
> > > Looks way cleaner, thanks.
> > > 
> > > Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > 
> > > Miquèl  
> > 
> > nice. if needed
> > 
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> 
> You cannot send your SoB like this. SoB means you are carrying some
> code which complies with the license, etc.
> 
> Either you were part of the original writing and want to be credited
> for that (you can be the author and first SoB, or suggest Tim to use
> Co-developed-by). Or you reviewed the change (Reviewed-by), you tested
> the change (Tested-by), or you are maintainer/responsible for some part
> that is touched and you agree with the change (Acked-by).

right, however there is some lenience in the process: some projects accept
the signed off, some others add the acked or reviewed and so on...

but I agree with you and I certainly do not need/want/expect to be
credited for this work. it is all good on my end :)

> 
> Thanks,
> Miquèl

btw you could have simply ignored my note btw : notice the "if needed"
part in my response. for reference, I noticed my name in CC, checked the
change, thought it was a good idea and tried to move it a long - just
chose the wrong tag for sure...

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

[Miquel Raynal]

> > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>  
> > 
> > You cannot send your SoB like this. SoB means you are carrying some
> > code which complies with the license, etc.
> > 
> > Either you were part of the original writing and want to be credited
> > for that (you can be the author and first SoB, or suggest Tim to use
> > Co-developed-by). Or you reviewed the change (Reviewed-by), you tested
> > the change (Tested-by), or you are maintainer/responsible for some part
> > that is touched and you agree with the change (Acked-by).  
> 
> right, however there is some lenience in the process: some projects accept
> the signed off, some others add the acked or reviewed and so on...
> 
> but I agree with you and I certainly do not need/want/expect to be
> credited for this work. it is all good on my end :)
> 
> > 
> > Thanks,
> > Miquèl  
> 
> btw you could have simply ignored my note btw : notice the "if needed"
> part in my response. for reference, I noticed my name in CC, checked the
> change, thought it was a good idea and tried to move it a long - just
> chose the wrong tag for sure...

And this is likely a good habit :) But now you need to send your
Reviewed-by because these tags are usually collected by the tools. If
you don't write it properly, there are very little chances it will
ever be picked-up.

Thanks,
Miquèl

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

[Ilias Apalodimas]

On Thu, 16 May 2024 at 13:40, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
>
> > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > >
> > > You cannot send your SoB like this. SoB means you are carrying some
> > > code which complies with the license, etc.
> > >
> > > Either you were part of the original writing and want to be credited
> > > for that (you can be the author and first SoB, or suggest Tim to use
> > > Co-developed-by). Or you reviewed the change (Reviewed-by), you tested
> > > the change (Tested-by), or you are maintainer/responsible for some part
> > > that is touched and you agree with the change (Acked-by).
> >
> > right, however there is some lenience in the process: some projects accept
> > the signed off, some others add the acked or reviewed and so on...
> >
> > but I agree with you and I certainly do not need/want/expect to be
> > credited for this work. it is all good on my end :)
> >
> > >
> > > Thanks,
> > > Miquèl
> >
> > btw you could have simply ignored my note btw : notice the "if needed"
> > part in my response. for reference, I noticed my name in CC, checked the
> > change, thought it was a good idea and tried to move it a long - just
> > chose the wrong tag for sure...
>
> And this is likely a good habit :) But now you need to send your
> Reviewed-by because these tags are usually collected by the tools. If
> you don't write it properly, there are very little chances it will
> ever be picked-up.

I'll pick it up manually. Jorge is it a r-b?


Thanks
/Ilias
>
> Thanks,
> Miquèl

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

[Ilias Apalodimas]

On Thu, 16 May 2024 at 02:21, 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.
>
> Additionally restructure the code for improved readability allowing for
> removal of the init label.
>
> Before:
>  - board with no reset gpio
> u-boot=> tpm init && tpm info
> tpm_tis_spi_probe: missing reset GPIO
> tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
>  - board with a reset gpio
> u-boot=> tpm init && tpm info
> tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
>
> After:
>  - board with no reset gpio
> u-boot=> tpm init && tpm info
> tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
>  - board with a reset gpio
> u-boot=> tpm init && tpm info
> tpm@1: TPM gpio reset should not be used on secure production devices
> tpm@1 v2.0: VendorID 0x1114, DeviceID 0x3205, RevisionID 0x01 [open]
>
> [1] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf
>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
> v3: restructure code for improved readability (recommended by Miquel)
> v2: change the message to a warning
> ---
>  drivers/tpm/tpm2_tis_spi.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
> index 28079b5039a3..b0fe97ab1d08 100644
> --- a/drivers/tpm/tpm2_tis_spi.c
> +++ b/drivers/tpm/tpm2_tis_spi.c
> @@ -237,19 +237,22 @@ 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) {
> +                       if (!ret) {
>                                 log(LOGC_NONE, LOGL_NOTICE,
> -                                   "%s: missing reset GPIO\n", __func__);
> -                               goto init;
> +                                   "%s: gpio-reset is deprecated\n", __func__);
>                         }
> -                       log(LOGC_NONE, LOGL_NOTICE,
> -                           "%s: gpio-reset is deprecated\n", __func__);
>                 }
> -               dm_gpio_set_value(&reset_gpio, 1);
> -               mdelay(1);
> -               dm_gpio_set_value(&reset_gpio, 0);
> +
> +               if (!ret) {
> +                       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);
> +               }
>         }
> -init:
> +
>         /* Ensure a minimum amount of time elapsed since reset of the TPM */
>         mdelay(drv_data->time_before_first_cmd_ms);
>
> --
> 2.25.1
>
Thanks Tim!

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>