[PATCH v2 1/1] tpm: clear state post probing
Before we can start measuring the TPM must be cleared. Do this in the post_probe() method of the uclass.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: tpm_startup2() is not available on all boards. tpm_startup() takes care of translating the call. --- drivers/tpm/tpm-uclass.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c index f67fe1019b..abd9ce35e8 100644 --- a/drivers/tpm/tpm-uclass.c +++ b/drivers/tpm/tpm-uclass.c @@ -11,6 +11,7 @@ #include <log.h> #include <linux/delay.h> #include <linux/unaligned/be_byteshift.h> +#include <tpm_api.h> #include <tpm-v1.h> #include <tpm-v2.h> #include "tpm_internal.h" @@ -136,6 +137,17 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size, return 0; }
+static int dm_tpm_post_probe(struct udevice *dev) +{ + /* + * Clearing the TPM state is only possible once after a hard reset. + * As we do not know if the TPM has been cleared by a prior boot stage + * ignore the return value here. + */ + tpm_startup(dev, TPM_ST_CLEAR); + return 0; +} + UCLASS_DRIVER(tpm) = { .id = UCLASS_TPM, .name = "tpm", @@ -143,5 +155,6 @@ UCLASS_DRIVER(tpm) = { #if CONFIG_IS_ENABLED(OF_REAL) .post_bind = dm_scan_fdt_dev, #endif + .post_probe = dm_tpm_post_probe, .per_device_auto = sizeof(struct tpm_chip_priv), };
On Mon, Nov 15, 2021 at 08:30:06PM +0100, Heinrich Schuchardt wrote:
Before we can start measuring the TPM must be cleared. Do this in the post_probe() method of the uclass.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: tpm_startup2() is not available on all boards. tpm_startup() takes care of translating the call.
drivers/tpm/tpm-uclass.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c index f67fe1019b..abd9ce35e8 100644 --- a/drivers/tpm/tpm-uclass.c +++ b/drivers/tpm/tpm-uclass.c @@ -11,6 +11,7 @@ #include <log.h> #include <linux/delay.h> #include <linux/unaligned/be_byteshift.h> +#include <tpm_api.h> #include <tpm-v1.h> #include <tpm-v2.h> #include "tpm_internal.h" @@ -136,6 +137,17 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size, return 0; }
+static int dm_tpm_post_probe(struct udevice *dev) +{
- /*
* Clearing the TPM state is only possible once after a hard reset.* As we do not know if the TPM has been cleared by a prior boot stage* ignore the return value here.*/- tpm_startup(dev, TPM_ST_CLEAR);
- return 0;
+}
UCLASS_DRIVER(tpm) = { .id = UCLASS_TPM, .name = "tpm", @@ -143,5 +155,6 @@ UCLASS_DRIVER(tpm) = { #if CONFIG_IS_ENABLED(OF_REAL) .post_bind = dm_scan_fdt_dev, #endif
- .post_probe = dm_tpm_post_probe, .per_device_auto = sizeof(struct tpm_chip_priv),
};
2.32.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Hi Heinrich,
On Tue, 16 Nov 2021 at 04:08, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Mon, Nov 15, 2021 at 08:30:06PM +0100, Heinrich Schuchardt wrote:
Before we can start measuring the TPM must be cleared. Do this in the post_probe() method of the uclass.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: tpm_startup2() is not available on all boards. tpm_startup() takes care of translating the call.
drivers/tpm/tpm-uclass.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c index f67fe1019b..abd9ce35e8 100644 --- a/drivers/tpm/tpm-uclass.c +++ b/drivers/tpm/tpm-uclass.c @@ -11,6 +11,7 @@ #include <log.h> #include <linux/delay.h> #include <linux/unaligned/be_byteshift.h> +#include <tpm_api.h> #include <tpm-v1.h> #include <tpm-v2.h> #include "tpm_internal.h" @@ -136,6 +137,17 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size, return 0; }
+static int dm_tpm_post_probe(struct udevice *dev)
Please drop the dm_
+{
/** Clearing the TPM state is only possible once after a hard reset.* As we do not know if the TPM has been cleared by a prior boot stage* ignore the return value here.*/tpm_startup(dev, TPM_ST_CLEAR);
blank line before final return
return 0;+}
This should only happen once and if the TPM is set up in SPL then this seems to cause a failure if done again.
Is there another way we can deal with this? Could the TPM user decide whether it needs to be set?
For the approach you have here, I think the best option might be to add a property to the devicetree. That way the prior stage can control it.
UCLASS_DRIVER(tpm) = { .id = UCLASS_TPM, .name = "tpm", @@ -143,5 +155,6 @@ UCLASS_DRIVER(tpm) = { #if CONFIG_IS_ENABLED(OF_REAL) .post_bind = dm_scan_fdt_dev, #endif
.post_probe = dm_tpm_post_probe, .per_device_auto = sizeof(struct tpm_chip_priv),};
2.32.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Regards, Simon
Hi Simon,
On Wed, 17 Nov 2021 at 04:48, Simon Glass sjg@chromium.org wrote:
Hi Heinrich,
On Tue, 16 Nov 2021 at 04:08, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Mon, Nov 15, 2021 at 08:30:06PM +0100, Heinrich Schuchardt wrote:
Before we can start measuring the TPM must be cleared. Do this in the post_probe() method of the uclass.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: tpm_startup2() is not available on all boards. tpm_startup() takes care of translating the call.
drivers/tpm/tpm-uclass.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c index f67fe1019b..abd9ce35e8 100644 --- a/drivers/tpm/tpm-uclass.c +++ b/drivers/tpm/tpm-uclass.c @@ -11,6 +11,7 @@ #include <log.h> #include <linux/delay.h> #include <linux/unaligned/be_byteshift.h> +#include <tpm_api.h> #include <tpm-v1.h> #include <tpm-v2.h> #include "tpm_internal.h" @@ -136,6 +137,17 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size, return 0; }
+static int dm_tpm_post_probe(struct udevice *dev)
Please drop the dm_
+{
/** Clearing the TPM state is only possible once after a hard reset.* As we do not know if the TPM has been cleared by a prior boot stage* ignore the return value here.*/tpm_startup(dev, TPM_ST_CLEAR);blank line before final return
return 0;+}
This should only happen once and if the TPM is set up in SPL then this seems to cause a failure if done again.
Not really. If you run the tpm_startup twice and the TPM is already initialized you'll get TPM2_RC_INITIALIZE back. That's an 'error' you can easily check against and decide.
Is there another way we can deal with this? Could the TPM user decide whether it needs to be set?
Why? Part of the TPM init is making it usable. We are trying to move away from having to add something in the command line to make the device usable.
For the approach you have here, I think the best option might be to add a property to the devicetree. That way the prior stage can control it.
UCLASS_DRIVER(tpm) = { .id = UCLASS_TPM, .name = "tpm", @@ -143,5 +155,6 @@ UCLASS_DRIVER(tpm) = { #if CONFIG_IS_ENABLED(OF_REAL) .post_bind = dm_scan_fdt_dev, #endif
.post_probe = dm_tpm_post_probe, .per_device_auto = sizeof(struct tpm_chip_priv),};
2.32.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Regards, Simon
Hi,
(replying to both of you)
On Wed, 17 Nov 2021 at 01:18, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 17 Nov 2021 at 04:48, Simon Glass sjg@chromium.org wrote:
Hi Heinrich,
On Tue, 16 Nov 2021 at 04:08, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Mon, Nov 15, 2021 at 08:30:06PM +0100, Heinrich Schuchardt wrote:
Before we can start measuring the TPM must be cleared. Do this in the post_probe() method of the uclass.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: tpm_startup2() is not available on all boards. tpm_startup() takes care of translating the call.
drivers/tpm/tpm-uclass.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c index f67fe1019b..abd9ce35e8 100644 --- a/drivers/tpm/tpm-uclass.c +++ b/drivers/tpm/tpm-uclass.c @@ -11,6 +11,7 @@ #include <log.h> #include <linux/delay.h> #include <linux/unaligned/be_byteshift.h> +#include <tpm_api.h> #include <tpm-v1.h> #include <tpm-v2.h> #include "tpm_internal.h" @@ -136,6 +137,17 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size, return 0; }
+static int dm_tpm_post_probe(struct udevice *dev)
Please drop the dm_
+{
/** Clearing the TPM state is only possible once after a hard reset.* As we do not know if the TPM has been cleared by a prior boot stage* ignore the return value here.*/tpm_startup(dev, TPM_ST_CLEAR);blank line before final return
return 0;+}
This should only happen once and if the TPM is set up in SPL then this seems to cause a failure if done again.
Not really. If you run the tpm_startup twice and the TPM is already initialized you'll get TPM2_RC_INITIALIZE back. That's an 'error' you can easily check against and decide.
Is there another way we can deal with this? Could the TPM user decide whether it needs to be set?
Why? Part of the TPM init is making it usable. We are trying to move away from having to add something in the command line to make the device usable.
For a start, we should not start up the TPM until we need it. That goes against the U-Boot lazy-init approach. We already have a command to do it, as well as a TPM-API thing, so let's use that.
For the approach you have here, I think the best option might be to add a property to the devicetree. That way the prior stage can control it.
w.r.t. Heinrich's comment:
The idea of a prior stage adding something U-Boot specific sucks.
The problem is that we need to track the device state. If we have multiple firmware phases involved, there needs to be some coordination across stages.
As it happens I am very familiar with this problem as it comes up in Chrome OS all the time. For Coral vboot I had to make sure that the TPM was only started once.
Anyway, this patch is wrong due to the 'probe' reason I mentioned above, sorry.
UCLASS_DRIVER(tpm) = { .id = UCLASS_TPM, .name = "tpm", @@ -143,5 +155,6 @@ UCLASS_DRIVER(tpm) = { #if CONFIG_IS_ENABLED(OF_REAL) .post_bind = dm_scan_fdt_dev, #endif
.post_probe = dm_tpm_post_probe, .per_device_auto = sizeof(struct tpm_chip_priv),};
2.32.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Regards, Simon
Hi Simon,
On Wed, 17 Nov 2021 at 19:20, Simon Glass sjg@chromium.org wrote:
Hi,
(replying to both of you)
On Wed, 17 Nov 2021 at 01:18, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 17 Nov 2021 at 04:48, Simon Glass sjg@chromium.org wrote:
Hi Heinrich,
On Tue, 16 Nov 2021 at 04:08, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Mon, Nov 15, 2021 at 08:30:06PM +0100, Heinrich Schuchardt wrote:
Before we can start measuring the TPM must be cleared. Do this in the post_probe() method of the uclass.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: tpm_startup2() is not available on all boards. tpm_startup() takes care of translating the call.
drivers/tpm/tpm-uclass.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c index f67fe1019b..abd9ce35e8 100644 --- a/drivers/tpm/tpm-uclass.c +++ b/drivers/tpm/tpm-uclass.c @@ -11,6 +11,7 @@ #include <log.h> #include <linux/delay.h> #include <linux/unaligned/be_byteshift.h> +#include <tpm_api.h> #include <tpm-v1.h> #include <tpm-v2.h> #include "tpm_internal.h" @@ -136,6 +137,17 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size, return 0; }
+static int dm_tpm_post_probe(struct udevice *dev)
Please drop the dm_
+{
/** Clearing the TPM state is only possible once after a hard reset.* As we do not know if the TPM has been cleared by a prior boot stage* ignore the return value here.*/tpm_startup(dev, TPM_ST_CLEAR);blank line before final return
return 0;+}
This should only happen once and if the TPM is set up in SPL then this seems to cause a failure if done again.
Not really. If you run the tpm_startup twice and the TPM is already initialized you'll get TPM2_RC_INITIALIZE back. That's an 'error' you can easily check against and decide.
Is there another way we can deal with this? Could the TPM user decide whether it needs to be set?
Why? Part of the TPM init is making it usable. We are trying to move away from having to add something in the command line to make the device usable.
For a start, we should not start up the TPM until we need it. That goes against the U-Boot lazy-init approach. We already have a command to do it, as well as a TPM-API thing, so let's use that.
So for the EFI TCG2 case were we *dont* want the user to init the TPM manually, you suggest we go use that API and the tpm_startup()?
[...]
Cheers /Ilias
On 11/17/21 20:22, Ilias Apalodimas wrote:
Hi Simon,
On Wed, 17 Nov 2021 at 19:20, Simon Glass sjg@chromium.org wrote:
Hi,
(replying to both of you)
On Wed, 17 Nov 2021 at 01:18, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 17 Nov 2021 at 04:48, Simon Glass sjg@chromium.org wrote:
Hi Heinrich,
On Tue, 16 Nov 2021 at 04:08, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Mon, Nov 15, 2021 at 08:30:06PM +0100, Heinrich Schuchardt wrote:
Before we can start measuring the TPM must be cleared. Do this in the post_probe() method of the uclass.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: tpm_startup2() is not available on all boards. tpm_startup() takes care of translating the call.
drivers/tpm/tpm-uclass.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c index f67fe1019b..abd9ce35e8 100644 --- a/drivers/tpm/tpm-uclass.c +++ b/drivers/tpm/tpm-uclass.c @@ -11,6 +11,7 @@ #include <log.h> #include <linux/delay.h> #include <linux/unaligned/be_byteshift.h> +#include <tpm_api.h> #include <tpm-v1.h> #include <tpm-v2.h> #include "tpm_internal.h" @@ -136,6 +137,17 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size, return 0; }
+static int dm_tpm_post_probe(struct udevice *dev)
Please drop the dm_
+{
/** Clearing the TPM state is only possible once after a hard reset.* As we do not know if the TPM has been cleared by a prior boot stage* ignore the return value here.*/tpm_startup(dev, TPM_ST_CLEAR);blank line before final return
return 0;+}
This should only happen once and if the TPM is set up in SPL then this seems to cause a failure if done again.
Not really. If you run the tpm_startup twice and the TPM is already initialized you'll get TPM2_RC_INITIALIZE back. That's an 'error' you can easily check against and decide.
Is there another way we can deal with this? Could the TPM user decide whether it needs to be set?
Why? Part of the TPM init is making it usable. We are trying to move away from having to add something in the command line to make the device usable.
For a start, we should not start up the TPM until we need it. That goes against the U-Boot lazy-init approach. We already have a command to do it, as well as a TPM-API thing, so let's use that.
The TPM is started exactly when we need it. When the user executes bootefi the TPM is probed if the TCG2 protocol is enabled because we need to measure binaries. Probing always has initialized TPMs. With this patch we only additionally clear it.
This patch does not contradict lazy initialization.
Best regards
Heinrich
So for the EFI TCG2 case were we *dont* want the user to init the TPM manually, you suggest we go use that API and the tpm_startup()?
[...]
Cheers /Ilias
Hi Heinrich,
On Wed, 17 Nov 2021 at 12:28, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 11/17/21 20:22, Ilias Apalodimas wrote:
Hi Simon,
On Wed, 17 Nov 2021 at 19:20, Simon Glass sjg@chromium.org wrote:
Hi,
(replying to both of you)
On Wed, 17 Nov 2021 at 01:18, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 17 Nov 2021 at 04:48, Simon Glass sjg@chromium.org wrote:
Hi Heinrich,
On Tue, 16 Nov 2021 at 04:08, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Mon, Nov 15, 2021 at 08:30:06PM +0100, Heinrich Schuchardt wrote: > Before we can start measuring the TPM must be cleared. Do this in the > post_probe() method of the uclass. > > Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com > --- > v2: > tpm_startup2() is not available on all boards. > tpm_startup() takes care of translating the call. > --- > drivers/tpm/tpm-uclass.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c > index f67fe1019b..abd9ce35e8 100644 > --- a/drivers/tpm/tpm-uclass.c > +++ b/drivers/tpm/tpm-uclass.c > @@ -11,6 +11,7 @@ > #include <log.h> > #include <linux/delay.h> > #include <linux/unaligned/be_byteshift.h> > +#include <tpm_api.h> > #include <tpm-v1.h> > #include <tpm-v2.h> > #include "tpm_internal.h" > @@ -136,6 +137,17 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size, > return 0; > } > > +static int dm_tpm_post_probe(struct udevice *dev)
Please drop the dm_
> +{ > + /* > + * Clearing the TPM state is only possible once after a hard reset. > + * As we do not know if the TPM has been cleared by a prior boot stage > + * ignore the return value here. > + */ > + tpm_startup(dev, TPM_ST_CLEAR);
blank line before final return
> + return 0; > +}
This should only happen once and if the TPM is set up in SPL then this seems to cause a failure if done again.
Not really. If you run the tpm_startup twice and the TPM is already initialized you'll get TPM2_RC_INITIALIZE back. That's an 'error' you can easily check against and decide.
Is there another way we can deal with this? Could the TPM user decide whether it needs to be set?
Why? Part of the TPM init is making it usable. We are trying to move away from having to add something in the command line to make the device usable.
For a start, we should not start up the TPM until we need it. That goes against the U-Boot lazy-init approach. We already have a command to do it, as well as a TPM-API thing, so let's use that.
The TPM is started exactly when we need it. When the user executes bootefi the TPM is probed if the TCG2 protocol is enabled because we need to measure binaries. Probing always has initialized TPMs. With this patch we only additionally clear it.
This patch does not contradict lazy initialization.
I think it does, because you are calling a TPM operation in the probe() method. Apart from the double-init problem you are creating, I may not want to start the TPM at this point.
Regards, Simon
On 11/17/21 20:30, Simon Glass wrote:
Hi Heinrich,
On Wed, 17 Nov 2021 at 12:28, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 11/17/21 20:22, Ilias Apalodimas wrote:
Hi Simon,
On Wed, 17 Nov 2021 at 19:20, Simon Glass sjg@chromium.org wrote:
Hi,
(replying to both of you)
On Wed, 17 Nov 2021 at 01:18, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 17 Nov 2021 at 04:48, Simon Glass sjg@chromium.org wrote:
Hi Heinrich,
On Tue, 16 Nov 2021 at 04:08, Ilias Apalodimas ilias.apalodimas@linaro.org wrote: > > On Mon, Nov 15, 2021 at 08:30:06PM +0100, Heinrich Schuchardt wrote: >> Before we can start measuring the TPM must be cleared. Do this in the >> post_probe() method of the uclass. >> >> Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com >> --- >> v2: >> tpm_startup2() is not available on all boards. >> tpm_startup() takes care of translating the call. >> --- >> drivers/tpm/tpm-uclass.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c >> index f67fe1019b..abd9ce35e8 100644 >> --- a/drivers/tpm/tpm-uclass.c >> +++ b/drivers/tpm/tpm-uclass.c >> @@ -11,6 +11,7 @@ >> #include <log.h> >> #include <linux/delay.h> >> #include <linux/unaligned/be_byteshift.h> >> +#include <tpm_api.h> >> #include <tpm-v1.h> >> #include <tpm-v2.h> >> #include "tpm_internal.h" >> @@ -136,6 +137,17 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size, >> return 0; >> } >> >> +static int dm_tpm_post_probe(struct udevice *dev)
Please drop the dm_
>> +{ >> + /* >> + * Clearing the TPM state is only possible once after a hard reset. >> + * As we do not know if the TPM has been cleared by a prior boot stage >> + * ignore the return value here. >> + */ >> + tpm_startup(dev, TPM_ST_CLEAR);
blank line before final return
>> + return 0; >> +}
This should only happen once and if the TPM is set up in SPL then this seems to cause a failure if done again.
Not really. If you run the tpm_startup twice and the TPM is already initialized you'll get TPM2_RC_INITIALIZE back. That's an 'error' you can easily check against and decide.
Is there another way we can deal with this? Could the TPM user decide whether it needs to be set?
Why? Part of the TPM init is making it usable. We are trying to move away from having to add something in the command line to make the device usable.
For a start, we should not start up the TPM until we need it. That goes against the U-Boot lazy-init approach. We already have a command to do it, as well as a TPM-API thing, so let's use that.
The TPM is started exactly when we need it. When the user executes bootefi the TPM is probed if the TCG2 protocol is enabled because we need to measure binaries. Probing always has initialized TPMs. With this patch we only additionally clear it.
This patch does not contradict lazy initialization.
I think it does, because you are calling a TPM operation in the probe() method. Apart from the double-init problem you are creating, I may not want to start the TPM at this point.
A TPM is nothing that is 'started'. It is a coprocessor that is running once you switch on the board.
If the TPM is probed, you have executed a command that makes use of the TPM. Why should it not be cleared?
Best regards
Heinrich
Hi Heinrich,
On Wed, 17 Nov 2021 at 12:34, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 11/17/21 20:30, Simon Glass wrote:
Hi Heinrich,
On Wed, 17 Nov 2021 at 12:28, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 11/17/21 20:22, Ilias Apalodimas wrote:
Hi Simon,
On Wed, 17 Nov 2021 at 19:20, Simon Glass sjg@chromium.org wrote:
Hi,
(replying to both of you)
On Wed, 17 Nov 2021 at 01:18, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 17 Nov 2021 at 04:48, Simon Glass sjg@chromium.org wrote: > > Hi Heinrich, > > On Tue, 16 Nov 2021 at 04:08, Ilias Apalodimas > ilias.apalodimas@linaro.org wrote: >> >> On Mon, Nov 15, 2021 at 08:30:06PM +0100, Heinrich Schuchardt wrote: >>> Before we can start measuring the TPM must be cleared. Do this in the >>> post_probe() method of the uclass. >>> >>> Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com >>> --- >>> v2: >>> tpm_startup2() is not available on all boards. >>> tpm_startup() takes care of translating the call. >>> --- >>> drivers/tpm/tpm-uclass.c | 13 +++++++++++++ >>> 1 file changed, 13 insertions(+) >>> >>> diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c >>> index f67fe1019b..abd9ce35e8 100644 >>> --- a/drivers/tpm/tpm-uclass.c >>> +++ b/drivers/tpm/tpm-uclass.c >>> @@ -11,6 +11,7 @@ >>> #include <log.h> >>> #include <linux/delay.h> >>> #include <linux/unaligned/be_byteshift.h> >>> +#include <tpm_api.h> >>> #include <tpm-v1.h> >>> #include <tpm-v2.h> >>> #include "tpm_internal.h" >>> @@ -136,6 +137,17 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size, >>> return 0; >>> } >>> >>> +static int dm_tpm_post_probe(struct udevice *dev) > > Please drop the dm_ > >>> +{ >>> + /* >>> + * Clearing the TPM state is only possible once after a hard reset. >>> + * As we do not know if the TPM has been cleared by a prior boot stage >>> + * ignore the return value here. >>> + */ >>> + tpm_startup(dev, TPM_ST_CLEAR); > > blank line before final return > >>> + return 0; >>> +} > > This should only happen once and if the TPM is set up in SPL then this > seems to cause a failure if done again. >
Not really. If you run the tpm_startup twice and the TPM is already initialized you'll get TPM2_RC_INITIALIZE back. That's an 'error' you can easily check against and decide.
> Is there another way we can deal with this? Could the TPM user decide > whether it needs to be set?
Why? Part of the TPM init is making it usable. We are trying to move away from having to add something in the command line to make the device usable.
For a start, we should not start up the TPM until we need it. That goes against the U-Boot lazy-init approach. We already have a command to do it, as well as a TPM-API thing, so let's use that.
The TPM is started exactly when we need it. When the user executes bootefi the TPM is probed if the TCG2 protocol is enabled because we need to measure binaries. Probing always has initialized TPMs. With this patch we only additionally clear it.
This patch does not contradict lazy initialization.
I think it does, because you are calling a TPM operation in the probe() method. Apart from the double-init problem you are creating, I may not want to start the TPM at this point.
A TPM is nothing that is 'started'. It is a coprocessor that is running once you switch on the board.
If the TPM is probed, you have executed a command that makes use of the TPM. Why should it not be cleared?
Why should it be? You haven't explained the problem here.
Starting the TPM can take a noticeable amount of time.
Regards, Simon
Hi Ilias,
On Wed, 17 Nov 2021 at 12:23, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 17 Nov 2021 at 19:20, Simon Glass sjg@chromium.org wrote:
Hi,
(replying to both of you)
On Wed, 17 Nov 2021 at 01:18, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 17 Nov 2021 at 04:48, Simon Glass sjg@chromium.org wrote:
Hi Heinrich,
On Tue, 16 Nov 2021 at 04:08, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Mon, Nov 15, 2021 at 08:30:06PM +0100, Heinrich Schuchardt wrote:
Before we can start measuring the TPM must be cleared. Do this in the post_probe() method of the uclass.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: tpm_startup2() is not available on all boards. tpm_startup() takes care of translating the call.
drivers/tpm/tpm-uclass.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c index f67fe1019b..abd9ce35e8 100644 --- a/drivers/tpm/tpm-uclass.c +++ b/drivers/tpm/tpm-uclass.c @@ -11,6 +11,7 @@ #include <log.h> #include <linux/delay.h> #include <linux/unaligned/be_byteshift.h> +#include <tpm_api.h> #include <tpm-v1.h> #include <tpm-v2.h> #include "tpm_internal.h" @@ -136,6 +137,17 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size, return 0; }
+static int dm_tpm_post_probe(struct udevice *dev)
Please drop the dm_
+{
/** Clearing the TPM state is only possible once after a hard reset.* As we do not know if the TPM has been cleared by a prior boot stage* ignore the return value here.*/tpm_startup(dev, TPM_ST_CLEAR);blank line before final return
return 0;+}
This should only happen once and if the TPM is set up in SPL then this seems to cause a failure if done again.
Not really. If you run the tpm_startup twice and the TPM is already initialized you'll get TPM2_RC_INITIALIZE back. That's an 'error' you can easily check against and decide.
Is there another way we can deal with this? Could the TPM user decide whether it needs to be set?
Why? Part of the TPM init is making it usable. We are trying to move away from having to add something in the command line to make the device usable.
For a start, we should not start up the TPM until we need it. That goes against the U-Boot lazy-init approach. We already have a command to do it, as well as a TPM-API thing, so let's use that.
So for the EFI TCG2 case were we *dont* want the user to init the TPM manually, you suggest we go use that API and the tpm_startup()?
Yes, we cannot rely on the user to to init the TPM, of course :-)
You can call tpm_startup() as needed.
Regards, Simon
On 11/17/21 03:48, Simon Glass wrote:
Hi Heinrich,
On Tue, 16 Nov 2021 at 04:08, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Mon, Nov 15, 2021 at 08:30:06PM +0100, Heinrich Schuchardt wrote:
Before we can start measuring the TPM must be cleared. Do this in the post_probe() method of the uclass.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: tpm_startup2() is not available on all boards. tpm_startup() takes care of translating the call.
drivers/tpm/tpm-uclass.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c index f67fe1019b..abd9ce35e8 100644 --- a/drivers/tpm/tpm-uclass.c +++ b/drivers/tpm/tpm-uclass.c @@ -11,6 +11,7 @@ #include <log.h> #include <linux/delay.h> #include <linux/unaligned/be_byteshift.h> +#include <tpm_api.h> #include <tpm-v1.h> #include <tpm-v2.h> #include "tpm_internal.h" @@ -136,6 +137,17 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size, return 0; }
+static int dm_tpm_post_probe(struct udevice *dev)
Please drop the dm_
sure
+{
/** Clearing the TPM state is only possible once after a hard reset.* As we do not know if the TPM has been cleared by a prior boot stage* ignore the return value here.*/tpm_startup(dev, TPM_ST_CLEAR);blank line before final return
ok
return 0;+}
This should only happen once and if the TPM is set up in SPL then this seems to cause a failure if done again.
It does not create a failure. The conforming TPM just ignores the TPM_ST_CLEAR and returns an error code.
Is there another way we can deal with this? Could the TPM user decide whether it needs to be set?
For the approach you have here, I think the best option might be to add a property to the devicetree. That way the prior stage can control it.
The idea of a prior stage adding something U-Boot specific sucks.
Best regards
Heinrich
- UCLASS_DRIVER(tpm) = { .id = UCLASS_TPM, .name = "tpm",
@@ -143,5 +155,6 @@ UCLASS_DRIVER(tpm) = { #if CONFIG_IS_ENABLED(OF_REAL) .post_bind = dm_scan_fdt_dev, #endif
.post_probe = dm_tpm_post_probe, .per_device_auto = sizeof(struct tpm_chip_priv),-- 2.32.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Regards, Simon
participants (3)
-
Heinrich Schuchardt -
Ilias Apalodimas -
Simon Glass