| Message ID | 20220313144802.65687-4-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | tpm: rng: Move TPM RNG functionality to driver model | expand |
Hi Sughosh, On Sun, 13 Mar 2022 at 08:48, Sughosh Ganu <sughosh.ganu@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@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
hi Simon, On Mon, 14 Mar 2022 at 03:53, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Sun, 13 Mar 2022 at 08:48, Sughosh Ganu <sughosh.ganu@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@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 Okay > > > 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 Can you please explain in a little more detail why? I have mentioned in my earlier email that I am changing this to bring it in line with the rest of the rng drivers which use a size_t data type for the number of random bytes to read. What is the issue in changing the signature? In any case, currently, the api is not being called from any other module, so it is not like changing the signature is breaking some code. > > > > > /** > > * 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. Done based on review comments from Heinrich[1]. > > > 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 Same as above. > > > 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. Okay -sughosh [1] - https://lists.denx.de/pipermail/u-boot/2022-February/476085.html
Hi Sughosh, On Mon, 14 Mar 2022 at 05:39, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Simon, > > On Mon, 14 Mar 2022 at 03:53, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Sun, 13 Mar 2022 at 08:48, Sughosh Ganu <sughosh.ganu@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@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 > > Okay > > > > > > 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 > > Can you please explain in a little more detail why? I have mentioned > in my earlier email that I am changing this to bring it in line with > the rest of the rng drivers which use a size_t data type for the > number of random bytes to read. What is the issue in changing the > signature? In any case, currently, the api is not being called from > any other module, so it is not like changing the signature is breaking > some code. Every other function in the TPM API uses u32 as the return value, so it is odd to change this. It also isn't necessary, as I have explained. We should aim for consistency. > > > > > > > > > /** > > > * 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); > > > [..] Regards, Simon
hi Simon, On Mon, 14 Mar 2022 at 23:55, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Mon, 14 Mar 2022 at 05:39, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > hi Simon, > > > > On Mon, 14 Mar 2022 at 03:53, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Sun, 13 Mar 2022 at 08:48, Sughosh Ganu <sughosh.ganu@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@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 > > > > Okay > > > > > > > > > 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 > > > > Can you please explain in a little more detail why? I have mentioned > > in my earlier email that I am changing this to bring it in line with > > the rest of the rng drivers which use a size_t data type for the > > number of random bytes to read. What is the issue in changing the > > signature? In any case, currently, the api is not being called from > > any other module, so it is not like changing the signature is breaking > > some code. > > Every other function in the TPM API uses u32 as the return value, so > it is odd to change this. It also isn't necessary, as I have > explained. We should aim for consistency. Well, you have given a R-b on another patch of this series[1] which is doing exactly the same thing. I really don't understand what is wrong in bringing the signature of the random byte generation functions in the TPM API in line with the RNG driver model. But I will not argue any more on this and revert back the signatures in my next version. Thanks. -sughosh [1] - https://lists.denx.de/pipermail/u-boot/2022-March/476519.html > > > > > > > > > > > > > > /** > > > > * 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); > > > > > > [..] > > Regards, > Simon
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 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); +} + +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); /** * 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; 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..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"); + + return ret; }
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.ganu@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