Hi Sughosh,
On Wed, 9 Mar 2022 at 05:28, Sughosh Ganu <sughosh.g...@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.
>
> Signed-off-by: Sughosh Ganu <sughosh.g...@linaro.org>
> ---
>
> Changes since V3:
>
> * Move back the driver model interface for the TPM RNG devices under
> drivers/rng/ directory.
> * Add a rng read function for the tpm devices which calls the
> tpm*_get_random API's.
> * Pass the TPM device pointer to the tpm*_get_random API's from the
> TPM RNG drivers as suggested by Simon.
>
> drivers/rng/Makefile | 2 ++
> drivers/rng/tpm1_rng.c | 24 ++++++++++++++++++++++++
> drivers/rng/tpm2_rng.c | 23 +++++++++++++++++++++++
> include/tpm-v1.h | 4 ++--
> include/tpm-v2.h | 4 ++--
> lib/tpm-v1.c | 16 +++++++++-------
> lib/tpm-v2.c | 9 +++++----
> lib/tpm_api.c | 23 +++++++++++++++++++----
> 8 files changed, 86 insertions(+), 19 deletions(-)
> create mode 100644 drivers/rng/tpm1_rng.c
> create mode 100644 drivers/rng/tpm2_rng.c
>
> diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> index 39f7ee3f03..c91e4e1701 100644
> --- a/drivers/rng/Makefile
> +++ b/drivers/rng/Makefile
> @@ -10,3 +10,5 @@ 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_V1) += tpm1_rng.o
> +obj-$(CONFIG_TPM_V2) += tpm2_rng.o
> diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c
> new file mode 100644
> index 0000000000..b827e2d1c1
> --- /dev/null
> +++ b/drivers/rng/tpm1_rng.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +#include <dm.h>
> +#include <rng.h>
> +#include <tpm-v1.h>
> +
> +static int rng_tpm1_random_read(struct udevice *dev, void *data, size_t
> count)
> +{
> + return tpm1_get_random(dev->parent, data, count);
> +}
> +
> +static const struct dm_rng_ops tpm1_rng_ops = {
> + .read = rng_tpm1_random_read,
> +};
> +
> +U_BOOT_DRIVER(tpm1_rng) = {
> + .name = "tpm1-rng",
> + .id = UCLASS_RNG,
> + .ops = &tpm1_rng_ops,
> +};
> +
> diff --git a/drivers/rng/tpm2_rng.c b/drivers/rng/tpm2_rng.c
> new file mode 100644
> index 0000000000..bd1fd539f0
> --- /dev/null
> +++ b/drivers/rng/tpm2_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-v2.h>
> +
> +static int rng_tpm2_random_read(struct udevice *dev, void *data, size_t
> count)
> +{
> + return tpm2_get_random(dev->parent, data, count);
> +}
> +
> +static const struct dm_rng_ops tpm2_rng_ops = {
> + .read = rng_tpm2_random_read,
> +};
> +
> +U_BOOT_DRIVER(tpm2_rng) = {
> + .name = "tpm2-rng",
> + .id = UCLASS_RNG,
> + .ops = &tpm2_rng_ops,
> +};
Can't you just have one driver and have a tpm_get_random() function in
tpm_api.h ?
> 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);
>
> /**
> * 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);
Do you need to change these? It would be better if the caller dealt with this.
>
> /**
> * Lock data in the TPM
> 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;
> 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;
> 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..3584fda98c 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,26 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index,
> u32 *perm)
> return -ENOSYS;
> }
>
> +#if CONFIG_IS_ENABLED(DM_RNG)
> int tpm_get_random(struct udevice *dev, void *data, u32 count)
This is the function you should call
> {
> + int ret = -ENOSYS;
> + struct udevice *rng_dev;
> +
> if (tpm_is_v1(dev))
> - return tpm1_get_random(dev, data, count);
> + ret = uclass_get_device_by_driver(UCLASS_RNG,
> + DM_DRIVER_GET(tpm1_rng),
> + &rng_dev);
What is going on here? This code should stay the same. Why would you
go off and hunt for the device?
> else if (tpm_is_v2(dev))
> - return -ENOSYS; /* not implemented yet */
It is implemented above
> - else
> - return -ENOSYS;
> + ret = uclass_get_device_by_driver(UCLASS_RNG,
> + DM_DRIVER_GET(tpm1_rng),
> + &rng_dev);
> +
> + if (ret) {
> + log_err("Getting tpm rng device failed\n");
> + return ret;
> + }
> +
> + return dm_rng_read(rng_dev, data, (size_t)count);
?? Don't you want to call the TPM to get the random number? I am just lost.
> }
> +#endif /* DM_RNG */
> --
> 2.25.1
>
Regards,
Simon
