Re: [PATCH 1/2 v3] tpm: add a function that performs selftest + startup

Simon Glass
2023-01-28T22:05:14Z

Hi Ilias,

On Thu, 26 Jan 2023 at 01:18, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> As described in [0] if a command requires use of an untested algorithm
> or functional module, the TPM performs the test and then completes the
> command actions.
>
> Since we don't check for TPM_RC_NEEDS_TEST (which is the return code of
> the TPM in that case) and even if we would, it would complicate our TPM
> code for no apparent reason, add a wrapper function that performs both
> the selftest and the startup sequence of the TPM.
>
> It's worth noting that this is implemented on TPMv2.0. The code for
> 1.2 would look similar, but I don't have a device available to test.
>
> [0]
> https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-1-Architecture-01.07-2014-03-13.pdf
> §12.3 Self-test modes
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> Changes since v2:
> - add tpm_init() to auto start
>
> Changes since v1:
> - Remove a superfluous if statement
> - Move function comments to the header file
> include/tpm-v2.h | 19 +++++++++++++++++++
> include/tpm_api.h | 8 ++++++++
> lib/tpm-v2.c | 24 ++++++++++++++++++++++++
> lib/tpm_api.c | 8 ++++++++
> 4 files changed, 59 insertions(+)

I think this is a good idea, but it should be implemented at the API
level. Please see below.

>
> diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> index 737e57551d73..1c644f0048f6 100644
> --- a/include/tpm-v2.h
> +++ b/include/tpm-v2.h
> @@ -688,4 +688,23 @@ u32 tpm2_report_state(struct udevice *dev, uint vendor_cmd, uint vendor_subcmd,
> u32 tpm2_enable_nvcommits(struct udevice *dev, uint vendor_cmd,
> uint vendor_subcmd);
>
> +/**
> + * tpm2_auto_start() - start up the TPM and perform selftests.
> + * If a testable function has not been tested and is
> + * requested the TPM2 will return TPM_RC_NEEDS_TEST.
> + *
> + *
> + *

drop extra lines

> + * @param dev TPM device
> + * Return: TPM2_RC_TESTING, if TPM2 self-test has been received and the tests are

80 cols

> + * not complete.
> + * TPM2_RC_SUCCESS, if testing of all functions is complete without
> + * functional failures.
> + * TPM2_RC_FAILURE, if any test failed.
> + * TPM2_RC_INITIALIZE, if the TPM has not gone through the Startup
> + * sequence
> +
> + */
> +u32 tpm2_auto_start(struct udevice *dev);
> +
> #endif /* __TPM_V2_H */
> diff --git a/include/tpm_api.h b/include/tpm_api.h
> index 8979d9d6df7e..022a8bbaeca6 100644
> --- a/include/tpm_api.h
> +++ b/include/tpm_api.h
> @@ -331,4 +331,12 @@ static inline bool tpm_is_v2(struct udevice *dev)
> return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
> }
>
> +/**
> + * tpm_auto_start() - start up the TPM and perform selftests
> + *
> + * @param dev TPM device
> + * Return: return code of the operation (0 = success)
> + */
> +u32 tpm_auto_start(struct udevice *dev);
> +
> #endif /* __TPM_API_H */
> diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> index 697b982e079f..2141d58632ff 100644
> --- a/lib/tpm-v2.c
> +++ b/lib/tpm-v2.c
> @@ -44,6 +44,30 @@ u32 tpm2_self_test(struct udevice *dev, enum tpm2_yes_no full_test)
> return tpm_sendrecv_command(dev, command_v2, NULL, NULL);
> }
>
> +u32 tpm2_auto_start(struct udevice *dev)
> +{
> + u32 rc;
> +
> + /*
> + * the tpm_init() will return -EBUSY if the init has already happened
> + * The selftest and startup code can run multiple times with no side effects

80 cols

> + */
> + rc = tpm_init(dev);
> + if (rc && rc != -EBUSY)

Does that work, with rc being unsigned?

> + return rc;
> + rc = tpm2_self_test(dev, TPMI_YES);

Can we call tpm_self_test_full() ? If not, please update the API as needed.

> +
> + if (rc == TPM2_RC_INITIALIZE) {
> + rc = tpm2_startup(dev, TPM2_SU_CLEAR);

Should call tpm_startup()

> + if (rc)
> + return rc;
> +
> + rc = tpm2_self_test(dev, TPMI_YES);

Again, tpm_self_test_full(). We are trying to provide a TPM API that
covers v1 and v2, to the extent possible.

> + }
> +
> + return rc;
> +}
> +

Also please add a test for this to test/dm/tpm.c

> u32 tpm2_clear(struct udevice *dev, u32 handle, const char *pw,
> const ssize_t pw_sz)
> {
> diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> index 7e8df8795ef3..5b2c11a277cc 100644
> --- a/lib/tpm_api.c
> +++ b/lib/tpm_api.c
> @@ -35,6 +35,14 @@ u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
> }
> }
>
> +u32 tpm_auto_start(struct udevice *dev)
> +{
> + if (tpm_is_v2(dev))
> + return tpm2_auto_start(dev);

Hopefully all the code from above can move here.

> +
> + return -ENOSYS;
> +}
> +
> u32 tpm_resume(struct udevice *dev)
> {
> if (tpm_is_v1(dev))
> --
> 2.38.1
>

Regards,
Simon