[PATCH v5 3/9] tpm: rng: Add driver model interface for TPM RNG device

Simon Glass sjg at chromium.org
Sun Mar 13 23:23:19 CET 2022


Hi Sughosh,

On Sun, 13 Mar 2022 at 08:48, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> The TPM device has a builtin random number generator(RNG)
> functionality. Expose the RNG functions of the TPM device to the
> driver model so that they can be used by the EFI_RNG_PROTOCOL if the
> protocol is installed.
>
> Also change the function arguments and return type of the random
> number functions to comply with the driver model api.

Please don't do that

>
> Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> ---
>
> Changes since V4:
>
> * Call the existing tpm_get_random API function from the TPM RNG
>   driver, instead of the tpm{1,2}_get_random API's
> * Introduce a new Kconfig symbol TPM_RNG and build the corresponding
>   driver if the symbol is enabled
> * Change the last parameter of the tpm_get_random API to have a data
>   type of size_t instead of u32 to comply with the RNG driver model
>   API
>
>  drivers/rng/Kconfig   |  7 +++++++
>  drivers/rng/Makefile  |  1 +
>  drivers/rng/tpm_rng.c | 23 +++++++++++++++++++++++
>  include/tpm-v1.h      |  4 ++--
>  include/tpm-v2.h      |  4 ++--
>  include/tpm_api.h     |  2 +-
>  lib/Kconfig           |  1 +
>  lib/tpm-v1.c          | 16 +++++++++-------
>  lib/tpm-v2.c          |  9 +++++----
>  lib/tpm_api.c         | 16 +++++++++++-----
>  10 files changed, 62 insertions(+), 21 deletions(-)
>  create mode 100644 drivers/rng/tpm_rng.c
>
> diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
> index b1c5ab93d1..a89fa99ffa 100644
> --- a/drivers/rng/Kconfig
> +++ b/drivers/rng/Kconfig
> @@ -49,4 +49,11 @@ config RNG_IPROC200
>         depends on DM_RNG
>         help
>           Enable random number generator for RPI4.
> +
> +config TPM_RNG
> +       bool "Enable random number generator on TPM device"
> +       depends on TPM
> +       default y
> +       help
> +         Enable random number generator on TPM devices

Needs 3 lines of text so please add more detail

>  endif
> diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> index 39f7ee3f03..a21f3353ea 100644
> --- a/drivers/rng/Makefile
> +++ b/drivers/rng/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_RNG_MSM) += msm_rng.o
>  obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
>  obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o
>  obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o
> +obj-$(CONFIG_TPM_RNG) += tpm_rng.o
> diff --git a/drivers/rng/tpm_rng.c b/drivers/rng/tpm_rng.c
> new file mode 100644
> index 0000000000..69b41dbbf5
> --- /dev/null
> +++ b/drivers/rng/tpm_rng.c
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +#include <dm.h>
> +#include <rng.h>
> +#include <tpm_api.h>
> +
> +static int rng_tpm_random_read(struct udevice *dev, void *data, size_t count)
> +{
> +       return tpm_get_random(dev->parent, data, count);

dev_get_parent(dev)

Here you should check the return value and decide whether to return an
error, such as -EIO

> +}
> +
> +static const struct dm_rng_ops tpm_rng_ops = {
> +       .read = rng_tpm_random_read,
> +};
> +
> +U_BOOT_DRIVER(tpm_rng) = {
> +       .name   = "tpm-rng",
> +       .id     = UCLASS_RNG,
> +       .ops    = &tpm_rng_ops,
> +};
> diff --git a/include/tpm-v1.h b/include/tpm-v1.h
> index 33d53fb695..d2ff8b446d 100644
> --- a/include/tpm-v1.h
> +++ b/include/tpm-v1.h
> @@ -555,9 +555,9 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
>   * @param dev          TPM device
>   * @param data         output buffer for the random bytes
>   * @param count                size of output buffer
> - * Return: return code of the operation
> + * Return: 0 if OK, -ve on error
>   */
> -u32 tpm1_get_random(struct udevice *dev, void *data, u32 count);
> +int tpm1_get_random(struct udevice *dev, void *data, size_t count);

I think I mentioned this last time, but you should not change these

>
>  /**
>   * tpm_finalise_physical_presence() - Finalise physical presence
> diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> index e79c90b939..4fb1e7a948 100644
> --- a/include/tpm-v2.h
> +++ b/include/tpm-v2.h
> @@ -619,9 +619,9 @@ u32 tpm2_pcr_setauthvalue(struct udevice *dev, const char *pw,
>   * @param data         output buffer for the random bytes
>   * @param count                size of output buffer
>   *
> - * Return: return code of the operation
> + * Return: 0 if OK, -ve on error
>   */
> -u32 tpm2_get_random(struct udevice *dev, void *data, u32 count);
> +int tpm2_get_random(struct udevice *dev, void *data, size_t count);
>
>  /**
>   * Lock data in the TPM
> diff --git a/include/tpm_api.h b/include/tpm_api.h
> index 4d77a49719..6d9214b335 100644
> --- a/include/tpm_api.h
> +++ b/include/tpm_api.h
> @@ -276,7 +276,7 @@ u32 tpm_find_key_sha1(struct udevice *dev, const u8 auth[20],
>   * @param count                size of output buffer
>   * Return: 0 if OK, -ve on error
>   */
> -int tpm_get_random(struct udevice *dev, void *data, u32 count);
> +int tpm_get_random(struct udevice *dev, void *data, size_t count);
>
>  /**
>   * tpm_finalise_physical_presence() - Finalise physical presence
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 3c6fa99b1a..f04a9c8c79 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -341,6 +341,7 @@ source lib/crypt/Kconfig
>  config TPM
>         bool "Trusted Platform Module (TPM) Support"
>         depends on DM
> +       imply DM_RNG
>         help
>           This enables support for TPMs which can be used to provide security
>           features for your board. The TPM can be connected via LPC or I2C
> diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
> index 22a769c587..4c6bc31a64 100644
> --- a/lib/tpm-v1.c
> +++ b/lib/tpm-v1.c
> @@ -9,12 +9,14 @@
>  #include <common.h>
>  #include <dm.h>
>  #include <log.h>
> -#include <asm/unaligned.h>
> -#include <u-boot/sha1.h>
> +#include <rng.h>
>  #include <tpm-common.h>
>  #include <tpm-v1.h>
>  #include "tpm-utils.h"
>
> +#include <asm/unaligned.h>
> +#include <u-boot/sha1.h>
> +
>  #ifdef CONFIG_TPM_AUTH_SESSIONS
>
>  #ifndef CONFIG_SHA1
> @@ -869,7 +871,7 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
>
>  #endif /* CONFIG_TPM_AUTH_SESSIONS */
>
> -u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
> +int tpm1_get_random(struct udevice *dev, void *data, size_t count)
>  {
>         const u8 command[14] = {
>                 0x0, 0xc1,              /* TPM_TAG */
> @@ -892,19 +894,19 @@ u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
>                 if (pack_byte_string(buf, sizeof(buf), "sd",
>                                      0, command, sizeof(command),
>                                      length_offset, this_bytes))
> -                       return TPM_LIB_ERROR;
> +                       return -EIO;

No please leave these alone.

>                 err = tpm_sendrecv_command(dev, buf, response,
>                                            &response_length);
>                 if (err)
>                         return err;
>                 if (unpack_byte_string(response, response_length, "d",
>                                        data_size_offset, &data_size))
> -                       return TPM_LIB_ERROR;
> +                       return -EIO;
>                 if (data_size > count)
> -                       return TPM_LIB_ERROR;
> +                       return -EIO;
>                 if (unpack_byte_string(response, response_length, "s",
>                                        data_offset, out, data_size))
> -                       return TPM_LIB_ERROR;
> +                       return -EIO;
>
>                 count -= data_size;
>                 out += data_size;
> diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> index 1bf627853a..d7a23ccf7e 100644
> --- a/lib/tpm-v2.c
> +++ b/lib/tpm-v2.c
> @@ -6,6 +6,7 @@
>
>  #include <common.h>
>  #include <dm.h>
> +#include <rng.h>
>  #include <tpm-common.h>
>  #include <tpm-v2.h>
>  #include <linux/bitops.h>
> @@ -562,7 +563,7 @@ u32 tpm2_pcr_setauthvalue(struct udevice *dev, const char *pw,
>         return tpm_sendrecv_command(dev, command_v2, NULL, NULL);
>  }
>
> -u32 tpm2_get_random(struct udevice *dev, void *data, u32 count)
> +int tpm2_get_random(struct udevice *dev, void *data, size_t count)
>  {
>         const u8 command_v2[10] = {
>                 tpm_u16(TPM2_ST_NO_SESSIONS),
> @@ -585,19 +586,19 @@ u32 tpm2_get_random(struct udevice *dev, void *data, u32 count)
>                 if (pack_byte_string(buf, sizeof(buf), "sw",
>                                      0, command_v2, sizeof(command_v2),
>                                      sizeof(command_v2), this_bytes))
> -                       return TPM_LIB_ERROR;
> +                       return -EIO;

and here

>                 err = tpm_sendrecv_command(dev, buf, response,
>                                            &response_length);
>                 if (err)
>                         return err;
>                 if (unpack_byte_string(response, response_length, "w",
>                                        data_size_offset, &data_size))
> -                       return TPM_LIB_ERROR;
> +                       return -EIO;
>                 if (data_size > this_bytes)
>                         return TPM_LIB_ERROR;
>                 if (unpack_byte_string(response, response_length, "s",
>                                        data_offset, out, data_size))
> -                       return TPM_LIB_ERROR;
> +                       return -EIO;
>
>                 count -= data_size;
>                 out += data_size;
> diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> index da48058abe..8bf60cda3c 100644
> --- a/lib/tpm_api.c
> +++ b/lib/tpm_api.c
> @@ -6,6 +6,7 @@
>  #include <common.h>
>  #include <dm.h>
>  #include <log.h>
> +#include <rng.h>
>  #include <tpm_api.h>
>  #include <tpm-v1.h>
>  #include <tpm-v2.h>
> @@ -265,12 +266,17 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
>                 return -ENOSYS;
>  }
>
> -int tpm_get_random(struct udevice *dev, void *data, u32 count)
> +int tpm_get_random(struct udevice *dev, void *data, size_t count)
>  {
> +       int ret = -ENOSYS;
> +
>         if (tpm_is_v1(dev))
> -               return tpm1_get_random(dev, data, count);
> +               ret = tpm1_get_random(dev, data, count);
>         else if (tpm_is_v2(dev))
> -               return -ENOSYS; /* not implemented yet */
> -       else
> -               return -ENOSYS;
> +               ret = tpm2_get_random(dev, data, count);
> +
> +       if (ret)
> +               log_err("Failed to read random bytes\n");

I don't see this in the other functions in this file. Why add it? It
just bloats the code and there is no way for the caller to suppress
the message.

> +
> +       return ret;
>  }
> --
> 2.25.1
>

Regards,
Simon


More information about the U-Boot mailing list