Hi Sughosh, On Sun, 13 Mar 2022 at 08:48, 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.
Please don't do that > > Signed-off-by: Sughosh Ganu <sughosh.g...@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