| Message ID | 20240321180219.1039622-1-tharvey@gateworks.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilias Apalodimas |
| Headers | show |
| Series | tpm: display message when using gpio-reset instead of when missing it | expand |
Hi Tim, On Thu, 21 Mar 2024 at 20:02, 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 dipslay a more > informative message about resetting the TPM if the gpio is found: > > before: > tpm_tis_spi_probe: missing reset GPIO > > after: > tpm@0: performing 1ms reset on gpio@30210000:12 > > Note that the reset dt binding prop used in this driver is not > dt-compliant; it does not exist in the Linux dt-bindings documentation > and the reset is not done by the Linux driver. Probably for a good reason. You aren't supposed to be able to reset a TPM without resetting the CPUI as well no? That being said, printing that the TPM was reset is pointless imho. OTOH the existing error message at least points out a potential problem in the DT. Thanks /Ilias > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > --- > 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..944540f7a711 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_NOTICE, > + "%s: performing 1ms reset on %s:%d\n", dev->name, > + reset_gpio.dev->name, reset_gpio.offset); > dm_gpio_set_value(&reset_gpio, 1); > mdelay(1); > dm_gpio_set_value(&reset_gpio, 0); > -- > 2.25.1 >
On Wed, Mar 27, 2024 at 7:44 AM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Tim, > > On Thu, 21 Mar 2024 at 20:02, 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 dipslay a more > > informative message about resetting the TPM if the gpio is found: > > > > before: > > tpm_tis_spi_probe: missing reset GPIO > > > > after: > > tpm@0: performing 1ms reset on gpio@30210000:12 > > > > Note that the reset dt binding prop used in this driver is not > > dt-compliant; it does not exist in the Linux dt-bindings documentation > > and the reset is not done by the Linux driver. > > Probably for a good reason. You aren't supposed to be able to reset a > TPM without resetting the CPUI as well no? Hi Ilias, Could you clarify what you know about TPM reset? We use the ATTPM20P [1] which states in the datasheet under the reset pin: "To be compliant with TCG requirements, this pin needs to be tied to system reset. TPM_Init is indicated by asserting this pin." Our boards have a resistor loading option which routes the TPM RST# to an SoC GPIO or alternately to a POR# (hardware power on reset provided by power supply and/or PMIC). Could you point me to where in the spec it explains what the TPM reset should be connected to? > That being said, printing that the TPM was reset is pointless imho. > OTOH the existing error message at least points out a potential > problem in the DT. > I'm not sure if you are NAK'ing this patch or asking me to change it. Displaying a 'missing GPIO' message is not helpful when there is no GPIO in the dt bindings to begin with. Best Regards, Tim [1] https://ww1.microchip.com/downloads/en/DeviceDoc/ATTPM20P-Trusted-Platform-Module-TPM-2.0-SPI-Interface-Summary-Data-Sheet-DS40002082A.pdf > Thanks > /Ilias > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > --- > > 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..944540f7a711 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_NOTICE, > > + "%s: performing 1ms reset on %s:%d\n", dev->name, > > + reset_gpio.dev->name, reset_gpio.offset); > > dm_gpio_set_value(&reset_gpio, 1); > > mdelay(1); > > dm_gpio_set_value(&reset_gpio, 0); > > -- > > 2.25.1 > >
Hi Tim, On Wed, 27 Mar 2024 at 17:29, Tim Harvey <tharvey@gateworks.com> wrote: > > On Wed, Mar 27, 2024 at 7:44 AM Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Tim, > > > > On Thu, 21 Mar 2024 at 20:02, 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 dipslay a more > > > informative message about resetting the TPM if the gpio is found: > > > > > > before: > > > tpm_tis_spi_probe: missing reset GPIO > > > > > > after: > > > tpm@0: performing 1ms reset on gpio@30210000:12 > > > > > > Note that the reset dt binding prop used in this driver is not > > > dt-compliant; it does not exist in the Linux dt-bindings documentation > > > and the reset is not done by the Linux driver. > > > > Probably for a good reason. You aren't supposed to be able to reset a > > TPM without resetting the CPUI as well no? > > Hi Ilias, > > Could you clarify what you know about TPM reset? We use the ATTPM20P > [1] which states in the datasheet under the reset pin: "To be > compliant with TCG requirements, this pin needs to be tied to system > reset. TPM_Init is indicated by asserting this pin." In short, you shouldn't be able to toggle a GPIO and reset the TPM without resetting the CPU as well. An attacker could boot -> log into the OS -> reset the TPM -> replay measurements and unseal keys that he shouldn't. > > Our boards have a resistor loading option which routes the TPM RST# to > an SoC GPIO or alternately to a POR# (hardware power on reset provided > by power supply and/or PMIC). Could you point me to where in the spec > it explains what the TPM reset should be connected to? I am aware of the the TCG TIS spec [0] which 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." There might be other TCG specs defining this as well. [0] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf > > > That being said, printing that the TPM was reset is pointless imho. > > OTOH the existing error message at least points out a potential > > problem in the DT. > > > > I'm not sure if you are NAK'ing this patch or asking me to change it. > > Displaying a 'missing GPIO' message is not helpful when there is no > GPIO in the dt bindings to begin with. I am not NAKing it. But isn't that message more useful than printing the TPM did a reset? At least you get a hint of something that's missing from your DT Thanks /Ilias > > Best Regards, > > Tim > [1] https://ww1.microchip.com/downloads/en/DeviceDoc/ATTPM20P-Trusted-Platform-Module-TPM-2.0-SPI-Interface-Summary-Data-Sheet-DS40002082A.pdf > > > > Thanks > > /Ilias > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > --- > > > 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..944540f7a711 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_NOTICE, > > > + "%s: performing 1ms reset on %s:%d\n", dev->name, > > > + reset_gpio.dev->name, reset_gpio.offset); > > > dm_gpio_set_value(&reset_gpio, 1); > > > mdelay(1); > > > dm_gpio_set_value(&reset_gpio, 0); > > > -- > > > 2.25.1 > > >
On Wed, Mar 27, 2024 at 9:16 AM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Tim, > > On Wed, 27 Mar 2024 at 17:29, Tim Harvey <tharvey@gateworks.com> wrote: > > > > On Wed, Mar 27, 2024 at 7:44 AM Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Hi Tim, > > > > > > On Thu, 21 Mar 2024 at 20:02, 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 dipslay a more > > > > informative message about resetting the TPM if the gpio is found: > > > > > > > > before: > > > > tpm_tis_spi_probe: missing reset GPIO > > > > > > > > after: > > > > tpm@0: performing 1ms reset on gpio@30210000:12 > > > > > > > > Note that the reset dt binding prop used in this driver is not > > > > dt-compliant; it does not exist in the Linux dt-bindings documentation > > > > and the reset is not done by the Linux driver. > > > > > > Probably for a good reason. You aren't supposed to be able to reset a > > > TPM without resetting the CPUI as well no? > > > > Hi Ilias, > > > > Could you clarify what you know about TPM reset? We use the ATTPM20P > > [1] which states in the datasheet under the reset pin: "To be > > compliant with TCG requirements, this pin needs to be tied to system > > reset. TPM_Init is indicated by asserting this pin." > > In short, you shouldn't be able to toggle a GPIO and reset the TPM > without resetting the CPU as well. > An attacker could boot -> log into the OS -> reset the TPM -> replay > measurements and unseal keys that he shouldn't. I suppose that makes sense. So consider someone gets into your OS and also has access to your board and electrically isolates the TPM reset from POR# (cut the trace) then with a probe resets the TPM? At least in that case they have to have physical presence as well so we should connect TPM RST# to POR# to not make it easier on them. I'm always wanting to allow GPIO access to various chip resets to deal with issues discovered down the line that require a full chip level reset - probably not the right thing to do in the case of a security device! Thanks for explaining. I'm still learning about the TPM capabilities. If using the TPM to store a key that is used for FDE you can use 'tpm2_unseal -c $OBJECT -p pcr:sha1:0,1 -o fs.key' to get your key. There must be a concept of re-sealing this object so that it can only be unsealed again with a power cycle or something? > > > > > Our boards have a resistor loading option which routes the TPM RST# to > > an SoC GPIO or alternately to a POR# (hardware power on reset provided > > by power supply and/or PMIC). Could you point me to where in the spec > > it explains what the TPM reset should be connected to? > > I am aware of the the TCG TIS spec [0] which 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." > There might be other TCG specs defining this as well. > > [0] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf > Thanks! > > > > > That being said, printing that the TPM was reset is pointless imho. > > > OTOH the existing error message at least points out a potential > > > problem in the DT. > > > > > > > I'm not sure if you are NAK'ing this patch or asking me to change it. > > > > Displaying a 'missing GPIO' message is not helpful when there is no > > GPIO in the dt bindings to begin with. > > I am not NAKing it. But isn't that message more useful than printing > the TPM did a reset? At least you get a hint of something that's > missing from your DT missing an invalid property? I deal with users all the time that think things like that are 'errors' and contact tech support. In this case its not an error because there is no gpio reset in the official dt-bindings for the tpm and its generally considered bad form to add non official properties. And from what your explaining we shouldn't have a GPIO connected to the TPM so perhaps we should remove the reset completely and perhaps even spit out a warning if present: ignore Invalid DT property gpio reset to conform with the TCG specificaiton Best regards, Tim > > Thanks > /Ilias > > > > Best Regards, > > > > Tim > > [1] https://ww1.microchip.com/downloads/en/DeviceDoc/ATTPM20P-Trusted-Platform-Module-TPM-2.0-SPI-Interface-Summary-Data-Sheet-DS40002082A.pdf > > > > > > > Thanks > > > /Ilias > > > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > > --- > > > > 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..944540f7a711 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_NOTICE, > > > > + "%s: performing 1ms reset on %s:%d\n", dev->name, > > > > + reset_gpio.dev->name, reset_gpio.offset); > > > > dm_gpio_set_value(&reset_gpio, 1); > > > > mdelay(1); > > > > dm_gpio_set_value(&reset_gpio, 0); > > > > -- > > > > 2.25.1 > > > >
On Wed, 27 Mar 2024 at 18:49, Tim Harvey <tharvey@gateworks.com> wrote: > > On Wed, Mar 27, 2024 at 9:16 AM Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Tim, > > > > On Wed, 27 Mar 2024 at 17:29, Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > On Wed, Mar 27, 2024 at 7:44 AM Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > Hi Tim, > > > > > > > > On Thu, 21 Mar 2024 at 20:02, 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 dipslay a more > > > > > informative message about resetting the TPM if the gpio is found: > > > > > > > > > > before: > > > > > tpm_tis_spi_probe: missing reset GPIO > > > > > > > > > > after: > > > > > tpm@0: performing 1ms reset on gpio@30210000:12 > > > > > > > > > > Note that the reset dt binding prop used in this driver is not > > > > > dt-compliant; it does not exist in the Linux dt-bindings documentation > > > > > and the reset is not done by the Linux driver. > > > > > > > > Probably for a good reason. You aren't supposed to be able to reset a > > > > TPM without resetting the CPUI as well no? > > > > > > Hi Ilias, > > > > > > Could you clarify what you know about TPM reset? We use the ATTPM20P > > > [1] which states in the datasheet under the reset pin: "To be > > > compliant with TCG requirements, this pin needs to be tied to system > > > reset. TPM_Init is indicated by asserting this pin." > > > > In short, you shouldn't be able to toggle a GPIO and reset the TPM > > without resetting the CPU as well. > > An attacker could boot -> log into the OS -> reset the TPM -> replay > > measurements and unseal keys that he shouldn't. > > I suppose that makes sense. So consider someone gets into your OS and > also has access to your board and electrically isolates the TPM reset > from POR# (cut the trace) then with a probe resets the TPM? At least > in that case they have to have physical presence as well so we should > connect TPM RST# to POR# to not make it easier on them. Yes, physical attacks are completely different (and we also have TPM interposer attacks, which can sniff secrets). > > I'm always wanting to allow GPIO access to various chip resets to deal > with issues discovered down the line that require a full chip level > reset - probably not the right thing to do in the case of a security > device! > > Thanks for explaining. You're welcome and yes being able to reset the TPM without bringing the CPU down as well is not a good idea > > I'm still learning about the TPM capabilities. If using the TPM to > store a key that is used for FDE you can use 'tpm2_unseal -c $OBJECT > -p pcr:sha1:0,1 -o fs.key' to get your key. There must be a concept of > re-sealing this object so that it can only be unsealed again with a > power cycle or something? You don't need to reseal it. The spec defines separator events that can be used to protect the key leaking. Once you unseal the key and decrypt your rootfs, you can do something along the lines of for i in {0..7}; do tpm2_pcrextend $i:sha256=0000000000000000000000000000000000000000000000000000000000000000; done Any attempts to read the key after the separator event will fail since PCRs won't match the sealing policy anymore. > > > > > > > > > Our boards have a resistor loading option which routes the TPM RST# to > > > an SoC GPIO or alternately to a POR# (hardware power on reset provided > > > by power supply and/or PMIC). Could you point me to where in the spec > > > it explains what the TPM reset should be connected to? > > > > I am aware of the the TCG TIS spec [0] which 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." > > There might be other TCG specs defining this as well. > > > > [0] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf > > > > Thanks! > > > > > > > > That being said, printing that the TPM was reset is pointless imho. > > > > OTOH the existing error message at least points out a potential > > > > problem in the DT. > > > > > > > > > > I'm not sure if you are NAK'ing this patch or asking me to change it. > > > > > > Displaying a 'missing GPIO' message is not helpful when there is no > > > GPIO in the dt bindings to begin with. > > > > I am not NAKing it. But isn't that message more useful than printing > > the TPM did a reset? At least you get a hint of something that's > > missing from your DT > > missing an invalid property? > > I deal with users all the time that think things like that are > 'errors' and contact tech support. In this case its not an error > because there is no gpio reset in the official dt-bindings for the tpm > and its generally considered bad form to add non official properties. > > And from what your explaining we shouldn't have a GPIO connected to > the TPM so perhaps we should remove the reset completely and perhaps > even spit out a warning if present: > ignore Invalid DT property gpio reset to conform with the TCG specification We should, but those changes predate me being appointed as a TPM maintainer. If I had to guess, I would say that was added for TPM that are connected on an external SPI bus (e.g the RPI). So what about not printing the error message, keeping the code so we won't break 'test' devices, and print a warning message like "This shouldn't be used on secure production devices"? Regards /Ilias > > Best regards, > > Tim > > > > > Thanks > > /Ilias > > > > > > Best Regards, > > > > > > Tim > > > [1] https://ww1.microchip.com/downloads/en/DeviceDoc/ATTPM20P-Trusted-Platform-Module-TPM-2.0-SPI-Interface-Summary-Data-Sheet-DS40002082A.pdf > > > > > > > > > > Thanks > > > > /Ilias > > > > > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > > > --- > > > > > 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..944540f7a711 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_NOTICE, > > > > > + "%s: performing 1ms reset on %s:%d\n", dev->name, > > > > > + reset_gpio.dev->name, reset_gpio.offset); > > > > > dm_gpio_set_value(&reset_gpio, 1); > > > > > mdelay(1); > > > > > dm_gpio_set_value(&reset_gpio, 0); > > > > > -- > > > > > 2.25.1 > > > > >
[...] > > > > missing an invalid property? > > > > I deal with users all the time that think things like that are > > 'errors' and contact tech support. In this case its not an error > > because there is no gpio reset in the official dt-bindings for the tpm > > and its generally considered bad form to add non official properties. > > > > And from what your explaining we shouldn't have a GPIO connected to > > the TPM so perhaps we should remove the reset completely and perhaps > > even spit out a warning if present: > > ignore Invalid DT property gpio reset to conform with the TCG specification > > We should, but those changes predate me being appointed as a TPM > maintainer. If I had to guess, I would say that was added for TPM that > are connected on an external SPI bus (e.g the RPI). > So what about not printing the error message, keeping the code so we > won't break 'test' devices, and print a warning message like "This > shouldn't be used on secure production devices"? Unless we can get a list of the devices that *currently* use it. If they aren't that many I am fine getting rid of the reset overall (and I can test it on the RPI4) Cheers /Ilias > > Regards > /Ilias > > > > Best regards, > > > > Tim > > > > > > > > Thanks > > > /Ilias > > > > > > > > Best Regards, > > > > > > > > Tim > > > > [1] https://ww1.microchip.com/downloads/en/DeviceDoc/ATTPM20P-Trusted-Platform-Module-TPM-2.0-SPI-Interface-Summary-Data-Sheet-DS40002082A.pdf > > > > > > > > > > > > > Thanks > > > > > /Ilias > > > > > > > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > > > > --- > > > > > > 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..944540f7a711 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_NOTICE, > > > > > > + "%s: performing 1ms reset on %s:%d\n", dev->name, > > > > > > + reset_gpio.dev->name, reset_gpio.offset); > > > > > > dm_gpio_set_value(&reset_gpio, 1); > > > > > > mdelay(1); > > > > > > dm_gpio_set_value(&reset_gpio, 0); > > > > > > -- > > > > > > 2.25.1 > > > > > >
On Wed, Mar 27, 2024 at 10:26 AM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > [...] > > > > > > > missing an invalid property? > > > > > > I deal with users all the time that think things like that are > > > 'errors' and contact tech support. In this case its not an error > > > because there is no gpio reset in the official dt-bindings for the tpm > > > and its generally considered bad form to add non official properties. > > > > > > And from what your explaining we shouldn't have a GPIO connected to > > > the TPM so perhaps we should remove the reset completely and perhaps > > > even spit out a warning if present: > > > ignore Invalid DT property gpio reset to conform with the TCG specification > > > > We should, but those changes predate me being appointed as a TPM > > maintainer. If I had to guess, I would say that was added for TPM that > > are connected on an external SPI bus (e.g the RPI). > > So what about not printing the error message, keeping the code so we > > won't break 'test' devices, and print a warning message like "This > > shouldn't be used on secure production devices"? > > Unless we can get a list of the devices that *currently* use it. If > they aren't that many I am fine getting rid of the reset overall (and > I can test it on the RPI4) > Adding Miquel who authored commit a174f0001f592 ("tpm2: tis_spi: add the possibility to reset the chip with a gpio"): On some designs, the reset line could not be connected to the SoC reset line, in this case, request the GPIO and ensure the chip gets reset. Adding Jorge who athoried commit cc5afabc9d329 ("drivers: tpm2: update reset gpio semantics"): Use the more generic reset-gpios property name. I looked through all dts in U-Boot matching 'tpm' as well as Linux to see who's using it: Adding Adam who has a 'reset-gpios' in arch/arm/dts/imx8mp-beacon-kit.dts (and somehow snuck it in upstream as well) Adding Rasmus who has a suspicious 'regulatot-tpm0-rst' (but no gpio reset in tpm) in arch/arm/dts/imx8mm-cl-iot-gate-ied-tpm0.dtso As there is at least one board that clearly uses a gpio reset for the TPM in the tpm node I think we leave the tpm reset support, eliminate the warning if its missing and print a message like "Warning: TPM gpio reset should not be used on secure production device" I'll send a v2 of my patch for review Best Regards, Tim [1] https://patchwork.ozlabs.org/project/uboot/patch/20240321180219.1039622-1-tharvey@gateworks.com/ > Cheers > /Ilias > > > > Regards > > /Ilias > > > > > > Best regards, > > > > > > Tim > > > > > > > > > > > Thanks > > > > /Ilias > > > > > > > > > > Best Regards, > > > > > > > > > > Tim > > > > > [1] https://ww1.microchip.com/downloads/en/DeviceDoc/ATTPM20P-Trusted-Platform-Module-TPM-2.0-SPI-Interface-Summary-Data-Sheet-DS40002082A.pdf > > > > > > > > > > > > > > > > Thanks > > > > > > /Ilias > > > > > > > > > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > > > > > --- > > > > > > > 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..944540f7a711 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_NOTICE, > > > > > > > + "%s: performing 1ms reset on %s:%d\n", dev->name, > > > > > > > + reset_gpio.dev->name, reset_gpio.offset); > > > > > > > dm_gpio_set_value(&reset_gpio, 1); > > > > > > > mdelay(1); > > > > > > > dm_gpio_set_value(&reset_gpio, 0); > > > > > > > -- > > > > > > > 2.25.1 > > > > > > >
On Wed, 27 Mar 2024 at 20:47, Tim Harvey <tharvey@gateworks.com> wrote: > > On Wed, Mar 27, 2024 at 10:26 AM Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > [...] > > > > > > > > > > missing an invalid property? > > > > > > > > I deal with users all the time that think things like that are > > > > 'errors' and contact tech support. In this case its not an error > > > > because there is no gpio reset in the official dt-bindings for the tpm > > > > and its generally considered bad form to add non official properties. > > > > > > > > And from what your explaining we shouldn't have a GPIO connected to > > > > the TPM so perhaps we should remove the reset completely and perhaps > > > > even spit out a warning if present: > > > > ignore Invalid DT property gpio reset to conform with the TCG specification > > > > > > We should, but those changes predate me being appointed as a TPM > > > maintainer. If I had to guess, I would say that was added for TPM that > > > are connected on an external SPI bus (e.g the RPI). > > > So what about not printing the error message, keeping the code so we > > > won't break 'test' devices, and print a warning message like "This > > > shouldn't be used on secure production devices"? > > > > Unless we can get a list of the devices that *currently* use it. If > > they aren't that many I am fine getting rid of the reset overall (and > > I can test it on the RPI4) > > > > Adding Miquel who authored commit a174f0001f592 ("tpm2: tis_spi: add > the possibility to reset the chip with a gpio"): > On some designs, the reset line could not be connected to the SoC reset > line, in this case, request the GPIO and ensure the chip gets reset. > > Adding Jorge who athoried commit cc5afabc9d329 ("drivers: tpm2: update > reset gpio semantics"): > Use the more generic reset-gpios property name. > > I looked through all dts in U-Boot matching 'tpm' as well as Linux to > see who's using it: > Adding Adam who has a 'reset-gpios' in > arch/arm/dts/imx8mp-beacon-kit.dts (and somehow snuck it in upstream > as well) > Adding Rasmus who has a suspicious 'regulatot-tpm0-rst' (but no gpio > reset in tpm) in arch/arm/dts/imx8mm-cl-iot-gate-ied-tpm0.dtso > > As there is at least one board that clearly uses a gpio reset for the > TPM in the tpm node I think we leave the tpm reset support, eliminate > the warning if its missing and print a message like "Warning: TPM gpio > reset should not be used on secure production device" > > I'll send a v2 of my patch for review Thanks. It's a bit unfortunate we ended up with broken devices, but if we remove the GPIO reset I am pretty sure the side effect will be having an unusable TPM after warm resets since PCRs will retain the pre-reboot values... /Ilias > > Best Regards, > > Tim > [1] https://patchwork.ozlabs.org/project/uboot/patch/20240321180219.1039622-1-tharvey@gateworks.com/ > > > Cheers > > /Ilias > > > > > > Regards > > > /Ilias > > > > > > > > Best regards, > > > > > > > > Tim > > > > > > > > > > > > > > Thanks > > > > > /Ilias > > > > > > > > > > > > Best Regards, > > > > > > > > > > > > Tim > > > > > > [1] https://ww1.microchip.com/downloads/en/DeviceDoc/ATTPM20P-Trusted-Platform-Module-TPM-2.0-SPI-Interface-Summary-Data-Sheet-DS40002082A.pdf > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > /Ilias > > > > > > > > > > > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > > > > > > --- > > > > > > > > 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..944540f7a711 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_NOTICE, > > > > > > > > + "%s: performing 1ms reset on %s:%d\n", dev->name, > > > > > > > > + reset_gpio.dev->name, reset_gpio.offset); > > > > > > > > dm_gpio_set_value(&reset_gpio, 1); > > > > > > > > mdelay(1); > > > > > > > > dm_gpio_set_value(&reset_gpio, 0); > > > > > > > > -- > > > > > > > > 2.25.1 > > > > > > > >
diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c index de9cf8f21e07..944540f7a711 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_NOTICE, + "%s: performing 1ms reset on %s:%d\n", dev->name, + reset_gpio.dev->name, reset_gpio.offset); dm_gpio_set_value(&reset_gpio, 1); mdelay(1); dm_gpio_set_value(&reset_gpio, 0);
Instead of displaying what looks like an error message if a gpio-reset dt prop is missing for a TPM dipslay a more informative message about resetting the TPM if the gpio is found: before: tpm_tis_spi_probe: missing reset GPIO after: tpm@0: performing 1ms reset on gpio@30210000:12 Note that the reset dt binding prop used in this driver is not dt-compliant; it does not exist in the Linux dt-bindings documentation and the reset is not done by the Linux driver. Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- drivers/tpm/tpm2_tis_spi.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)