[PATCH v5 0/9] tpm: rng: Move TPM RNG functionality to driver model
The TPM device provides the random number generator(RNG) functionality, whereby sending a command to the TPM device results in the TPM device responding with random bytes.
There was a discussion on the mailing list earlier[1], where it was explained that platforms with a TPM device can install the EFI_RNG_PROTOCOL for getting the random bytes instead of populating the dtb with the kaslr-seed property. That would make it possible to measure the dtb.
This patchset moves the already existing functions for getting random bytes from the TPM device to drivers complying with the RNG uclass. This is done since the EFI_RNG_PROTOCOL's get_rng routine uses the RNG uclass's dm_rng_read api to get the random bytes.
The TPM uclass driver adds the RNG child device as part of it's post_probe function. The TPM uclass driver's child_pre_probe function initialises the TPM parent device for use -- this enables the RNG child device to be used subsequently.
Some additional changes have also been made to facilitate the use of the RNG devices, including extending the 'rng' command to take the RNG device as one of the command-line parameters.
This series depends on a patch[2] from Simon Glass for moving the TPM device version detection functions to the tpm_api.h header as static inline functions. [1] - https://lore.kernel.org/u-boot/20220103120738.47835-1-ilias.apalodimas@linar... [2] - https://lore.kernel.org/u-boot/20220301001125.1554442-2-sjg@chromium.org/T/#...
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 * Put a check for CONFIG_TPM_RNG for binding the RNG device with it's driver in the post_probe callback instead of putting CONFIG_{SPL,TPL}_BUILD guards * Use uclass_get_device_by_seq API to get the RNG device as suggested by Simon * Add a new patch based on review comments from Simon to not use the malloc call * Reflect the fact that a maximum of 64 bytes can be read on each invocation of the 'rng' command in the rng document
Sughosh Ganu (9): tpm: rng: Change tpm_get_random to return an int tpm: Fix the return type of tpm_startup tpm: rng: Add driver model interface for TPM RNG device tpm: Add the RNG child device qemu: arm: Remove platform specific function to get RNG device cmd: rng: Add support for selecting RNG device cmd: rng: Use a statically allocated array for random bytes doc: rng: Add documentation for the rng command test: rng: Add a UT testcase for the rng command
board/emulation/qemu-arm/qemu-arm.c | 42 ----------------------------- cmd/Kconfig | 1 + cmd/rng.c | 42 ++++++++++++++++++----------- doc/usage/index.rst | 1 + doc/usage/rng.rst | 26 ++++++++++++++++++ drivers/rng/Kconfig | 7 +++++ drivers/rng/Makefile | 1 + drivers/rng/tpm_rng.c | 23 ++++++++++++++++ drivers/tpm/tpm-uclass.c | 29 +++++++++++++++++--- include/tpm-v1.h | 4 +-- include/tpm-v2.h | 4 +-- include/tpm_api.h | 6 ++--- lib/Kconfig | 1 + lib/tpm-v1.c | 16 ++++++----- lib/tpm-v2.c | 9 ++++--- lib/tpm_api.c | 19 ++++++++----- test/dm/rng.c | 29 ++++++++++++++++++++ 17 files changed, 175 insertions(+), 85 deletions(-) create mode 100644 doc/usage/rng.rst create mode 100644 drivers/rng/tpm_rng.c
The tpm random number generation functionality will be moved to the driver model. With that, the tpm_get_random function will call the common driver model api instead of separate functions for tpmv1 and tpmv2. Return an int instead of a u32 to comply with the return value of the driver model function.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org Reviewed-by: Simon Glass sjg@chromium.org ---
Changes since V4: None
include/tpm_api.h | 4 ++-- lib/tpm_api.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/tpm_api.h b/include/tpm_api.h index 11aa14eb79..249a966942 100644 --- a/include/tpm_api.h +++ b/include/tpm_api.h @@ -274,9 +274,9 @@ u32 tpm_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 tpm_get_random(struct udevice *dev, void *data, u32 count); +int tpm_get_random(struct udevice *dev, void *data, u32 count);
/** * tpm_finalise_physical_presence() - Finalise physical presence diff --git a/lib/tpm_api.c b/lib/tpm_api.c index 4ac4612c81..7d26ea2c3a 100644 --- a/lib/tpm_api.c +++ b/lib/tpm_api.c @@ -264,7 +264,7 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm) return -ENOSYS; }
-u32 tpm_get_random(struct udevice *dev, void *data, u32 count) +int tpm_get_random(struct udevice *dev, void *data, u32 count) { if (tpm_is_v1(dev)) return tpm1_get_random(dev, data, count);
The tpm_startup function returns negative values for error conditions. Fix the return type of the function to a signed int instead of a u32.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org Reviewed-by: Simon Glass sjg@chromium.org ---
Changes since V4: None
include/tpm_api.h | 2 +- lib/tpm_api.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/tpm_api.h b/include/tpm_api.h index 249a966942..4d77a49719 100644 --- a/include/tpm_api.h +++ b/include/tpm_api.h @@ -18,7 +18,7 @@ * @param mode TPM startup mode * Return: return code of the operation */ -u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode); +int tpm_startup(struct udevice *dev, enum tpm_startup_type mode);
/** * Issue a TPM_SelfTestFull command. diff --git a/lib/tpm_api.c b/lib/tpm_api.c index 7d26ea2c3a..da48058abe 100644 --- a/lib/tpm_api.c +++ b/lib/tpm_api.c @@ -11,7 +11,8 @@ #include <tpm-v2.h> #include <tpm_api.h>
-u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode) + +int tpm_startup(struct udevice *dev, enum tpm_startup_type mode) { if (tpm_is_v1(dev)) { return tpm1_startup(dev, mode);
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
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; }
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 TPMdefault yhelpEnable 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 I2Cdiff --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 */elsereturn -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 TPMdefault yhelpEnable random number generator on TPM devicesNeeds 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 I2Cdiff --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 */elsereturn -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 TPMdefault yhelpEnable random number generator on TPM devicesNeeds 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 TPMdefault yhelpEnable random number generator on TPM devicesNeeds 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
The TPM device comes with the random number generator(RNG) functionality which is built into the TPM device. Add logic to add the RNG child device in the TPM uclass post probe callback.
The RNG device can then be used to pass a set of random bytes to the linux kernel, need for address space randomisation through the EFI_RNG_PROTOCOL interface.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org ---
Changes since V4:
* Put a check for CONFIG_TPM_RNG for binding the RNG device with it's driver in the post_probe callback instead of putting CONFIG_{SPL,TPL}_BUILD guards
drivers/tpm/tpm-uclass.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c index f67fe1019b..e1c61d26f0 100644 --- a/drivers/tpm/tpm-uclass.c +++ b/drivers/tpm/tpm-uclass.c @@ -11,10 +11,15 @@ #include <log.h> #include <linux/delay.h> #include <linux/unaligned/be_byteshift.h> +#include <tpm_api.h> #include <tpm-v1.h> #include <tpm-v2.h> #include "tpm_internal.h"
+#include <dm/lists.h> + +#define TPM_RNG_DRV_NAME "tpm-rng" + int tpm_open(struct udevice *dev) { struct tpm_ops *ops = tpm_get_ops(dev); @@ -136,12 +141,28 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size, return 0; }
+static int tpm_uclass_post_probe(struct udevice *dev) +{ + int ret; + const char *drv = TPM_RNG_DRV_NAME; + struct udevice *child; + + if (CONFIG_IS_ENABLED(TPM_RNG)) { + ret = device_bind_driver(dev, drv, "tpm-rng0", &child); + if (ret) + return log_msg_ret("bind", ret); + } + + return 0; +} + UCLASS_DRIVER(tpm) = { - .id = UCLASS_TPM, - .name = "tpm", - .flags = DM_UC_FLAG_SEQ_ALIAS, + .id = UCLASS_TPM, + .name = "tpm", + .flags = DM_UC_FLAG_SEQ_ALIAS, #if CONFIG_IS_ENABLED(OF_REAL) - .post_bind = dm_scan_fdt_dev, + .post_bind = dm_scan_fdt_dev, #endif + .post_probe = tpm_uclass_post_probe, .per_device_auto = sizeof(struct tpm_chip_priv), };
Hi Sughosh,
On Sun, 13 Mar 2022 at 08:48, Sughosh Ganu sughosh.ganu@linaro.org wrote:
The TPM device comes with the random number generator(RNG) functionality which is built into the TPM device. Add logic to add the RNG child device in the TPM uclass post probe callback.
The RNG device can then be used to pass a set of random bytes to the linux kernel, need for address space randomisation through the EFI_RNG_PROTOCOL interface.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org
Changes since V4:
- Put a check for CONFIG_TPM_RNG for binding the RNG device with it's driver in the post_probe callback instead of putting CONFIG_{SPL,TPL}_BUILD guards
drivers/tpm/tpm-uclass.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
This looks a lot better, please see below.
diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c index f67fe1019b..e1c61d26f0 100644 --- a/drivers/tpm/tpm-uclass.c +++ b/drivers/tpm/tpm-uclass.c @@ -11,10 +11,15 @@ #include <log.h> #include <linux/delay.h> #include <linux/unaligned/be_byteshift.h> +#include <tpm_api.h> #include <tpm-v1.h> #include <tpm-v2.h> #include "tpm_internal.h"
+#include <dm/lists.h>
+#define TPM_RNG_DRV_NAME "tpm-rng"
int tpm_open(struct udevice *dev) { struct tpm_ops *ops = tpm_get_ops(dev); @@ -136,12 +141,28 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size, return 0; }
+static int tpm_uclass_post_probe(struct udevice *dev) +{
int ret;const char *drv = TPM_RNG_DRV_NAME;struct udevice *child;if (CONFIG_IS_ENABLED(TPM_RNG)) {ret = device_bind_driver(dev, drv, "tpm-rng0", &child);if (ret)return log_msg_ret("bind", ret);}
This really should be in the device tree so what you are doing here is quite strange. If you want to manually bind it, please call device_find_first_child_by_uclass() first to make sure it isn't already there.
Also you should bind it in the bind() method, not in probe().
This is the code used for the same thing in the bootstd series:
struct udevice *bdev; int ret;
ret = device_find_first_child_by_uclass(parent, UCLASS_BOOTDEV, &bdev); if (ret) { if (ret != -ENODEV) { log_debug("Cannot access bootdev device\n"); return ret; }
ret = bootdev_bind(parent, drv_name, "bootdev", &bdev); if (ret) { log_debug("Cannot create bootdev device\n"); return ret; } }
return 0;+}
UCLASS_DRIVER(tpm) = {
.id = UCLASS_TPM,.name = "tpm",.flags = DM_UC_FLAG_SEQ_ALIAS,
.id = UCLASS_TPM,.name = "tpm",.flags = DM_UC_FLAG_SEQ_ALIAS,#if CONFIG_IS_ENABLED(OF_REAL)
.post_bind = dm_scan_fdt_dev,
.post_bind = dm_scan_fdt_dev,#endif
.post_probe = tpm_uclass_post_probe,
Should be post_bind.
.per_device_auto = sizeof(struct tpm_chip_priv),};
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 comes with the random number generator(RNG) functionality which is built into the TPM device. Add logic to add the RNG child device in the TPM uclass post probe callback.
The RNG device can then be used to pass a set of random bytes to the linux kernel, need for address space randomisation through the EFI_RNG_PROTOCOL interface.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org
Changes since V4:
- Put a check for CONFIG_TPM_RNG for binding the RNG device with it's driver in the post_probe callback instead of putting CONFIG_{SPL,TPL}_BUILD guards
drivers/tpm/tpm-uclass.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
This looks a lot better, please see below.
diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c index f67fe1019b..e1c61d26f0 100644 --- a/drivers/tpm/tpm-uclass.c +++ b/drivers/tpm/tpm-uclass.c @@ -11,10 +11,15 @@ #include <log.h> #include <linux/delay.h> #include <linux/unaligned/be_byteshift.h> +#include <tpm_api.h> #include <tpm-v1.h> #include <tpm-v2.h> #include "tpm_internal.h"
+#include <dm/lists.h>
+#define TPM_RNG_DRV_NAME "tpm-rng"
int tpm_open(struct udevice *dev) { struct tpm_ops *ops = tpm_get_ops(dev); @@ -136,12 +141,28 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size, return 0; }
+static int tpm_uclass_post_probe(struct udevice *dev) +{
int ret;const char *drv = TPM_RNG_DRV_NAME;struct udevice *child;if (CONFIG_IS_ENABLED(TPM_RNG)) {ret = device_bind_driver(dev, drv, "tpm-rng0", &child);if (ret)return log_msg_ret("bind", ret);}This really should be in the device tree so what you are doing here is quite strange.
Like I had mentioned in my earlier emails, the TPM device has a builtin RNG functionality, which is non-optional. So I don't understand why we need to use a device tree subnode here. Whether the device is being bound to the parent is being controlled by the TPM_RNG config that you asked me to put in my previous version, which I am doing.
If you want to manually bind it, please call
device_find_first_child_by_uclass() first to make sure it isn't already there.
Okay
Also you should bind it in the bind() method, not in probe().
Okay
This is the code used for the same thing in the bootstd series:
struct udevice *bdev; int ret;
ret = device_find_first_child_by_uclass(parent, UCLASS_BOOTDEV, &bdev); if (ret) { if (ret != -ENODEV) { log_debug("Cannot access bootdev device\n"); return ret; }
ret = bootdev_bind(parent, drv_name, "bootdev", &bdev); if (ret) { log_debug("Cannot create bootdev device\n"); return ret; } }
return 0;+}
UCLASS_DRIVER(tpm) = {
.id = UCLASS_TPM,.name = "tpm",.flags = DM_UC_FLAG_SEQ_ALIAS,
.id = UCLASS_TPM,.name = "tpm",.flags = DM_UC_FLAG_SEQ_ALIAS,#if CONFIG_IS_ENABLED(OF_REAL)
.post_bind = dm_scan_fdt_dev,
.post_bind = dm_scan_fdt_dev,#endif
.post_probe = tpm_uclass_post_probe,Should be post_bind.
Okay
-sughosh
.per_device_auto = sizeof(struct tpm_chip_priv),};
2.25.1
Regards, Simon
Hi Sughosh,
On Mon, 14 Mar 2022 at 05:43, 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 comes with the random number generator(RNG) functionality which is built into the TPM device. Add logic to add the RNG child device in the TPM uclass post probe callback.
The RNG device can then be used to pass a set of random bytes to the linux kernel, need for address space randomisation through the EFI_RNG_PROTOCOL interface.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org
Changes since V4:
- Put a check for CONFIG_TPM_RNG for binding the RNG device with it's driver in the post_probe callback instead of putting CONFIG_{SPL,TPL}_BUILD guards
drivers/tpm/tpm-uclass.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
This looks a lot better, please see below.
diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c index f67fe1019b..e1c61d26f0 100644 --- a/drivers/tpm/tpm-uclass.c +++ b/drivers/tpm/tpm-uclass.c @@ -11,10 +11,15 @@ #include <log.h> #include <linux/delay.h> #include <linux/unaligned/be_byteshift.h> +#include <tpm_api.h> #include <tpm-v1.h> #include <tpm-v2.h> #include "tpm_internal.h"
+#include <dm/lists.h>
+#define TPM_RNG_DRV_NAME "tpm-rng"
int tpm_open(struct udevice *dev) { struct tpm_ops *ops = tpm_get_ops(dev); @@ -136,12 +141,28 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size, return 0; }
+static int tpm_uclass_post_probe(struct udevice *dev) +{
int ret;const char *drv = TPM_RNG_DRV_NAME;struct udevice *child;if (CONFIG_IS_ENABLED(TPM_RNG)) {ret = device_bind_driver(dev, drv, "tpm-rng0", &child);if (ret)return log_msg_ret("bind", ret);}This really should be in the device tree so what you are doing here is quite strange.
Like I had mentioned in my earlier emails, the TPM device has a builtin RNG functionality, which is non-optional. So I don't understand why we need to use a device tree subnode here. Whether the device is being bound to the parent is being controlled by the TPM_RNG config that you asked me to put in my previous version, which I am doing.
See how PMICs work, for example. We have GPIOs, regulators and suchlike in the PMIC and we add subnodes for them in the DT. It is just how it is done.
Driver model is designed to automatically bind devices based on the device tree. There are cases where it is necessary to manually bind things, but we mustn't prevent people from doing it 'properly'.
Finally, I know you keep saying that random numbers are only needed in U-Boot proper, but if I want a random number in SPL, it may not work, since device_bind() is often not available, for code-size reasons.
So that is why I say that what you are doing is quite strange. Perhaps you are coming from a different project, which does things differently.
If you want to manually bind it, please call
device_find_first_child_by_uclass() first to make sure it isn't already there.
Okay
Also you should bind it in the bind() method, not in probe().
Okay
This is the code used for the same thing in the bootstd series:
struct udevice *bdev; int ret;
ret = device_find_first_child_by_uclass(parent, UCLASS_BOOTDEV, &bdev); if (ret) { if (ret != -ENODEV) { log_debug("Cannot access bootdev device\n"); return ret; }
ret = bootdev_bind(parent, drv_name, "bootdev", &bdev); if (ret) { log_debug("Cannot create bootdev device\n"); return ret; } }
return 0;+}
UCLASS_DRIVER(tpm) = {
.id = UCLASS_TPM,.name = "tpm",.flags = DM_UC_FLAG_SEQ_ALIAS,
.id = UCLASS_TPM,.name = "tpm",.flags = DM_UC_FLAG_SEQ_ALIAS,#if CONFIG_IS_ENABLED(OF_REAL)
.post_bind = dm_scan_fdt_dev,
.post_bind = dm_scan_fdt_dev,#endif
.post_probe = tpm_uclass_post_probe,Should be post_bind.
Okay
[..]
Regards, Simon
Hi Simon,
On Mon, 14 Mar 2022 at 20:24, Simon Glass sjg@chromium.org wrote:
[...]
}This really should be in the device tree so what you are doing here is quite strange.
Like I had mentioned in my earlier emails, the TPM device has a builtin RNG functionality, which is non-optional. So I don't understand why we need to use a device tree subnode here. Whether the device is being bound to the parent is being controlled by the TPM_RNG config that you asked me to put in my previous version, which I am doing.
See how PMICs work, for example. We have GPIOs, regulators and suchlike in the PMIC and we add subnodes for them in the DT. It is just how it is done.
Driver model is designed to automatically bind devices based on the device tree. There are cases where it is necessary to manually bind things, but we mustn't prevent people from doing it 'properly'.
There's a small difference here though. The RNG is not a device. The TPM is the device and an encoded command to that device returns a random number. There's no binding initiating etc.
Finally, I know you keep saying that random numbers are only needed in U-Boot proper, but if I want a random number in SPL, it may not work, since device_bind() is often not available, for code-size reasons.
And the entire tpm framework will fit?
So that is why I say that what you are doing is quite strange. Perhaps you are coming from a different project, which does things differently.
I don't personally find it strange. The device is already described in the DTS and I don't see a strong reason to deviate for the upstream version again.
Regards /Ilias
If you want to manually bind it, please call
device_find_first_child_by_uclass() first to make sure it isn't already there.
Okay
Also you should bind it in the bind() method, not in probe().
Okay
This is the code used for the same thing in the bootstd series:
struct udevice *bdev; int ret;
ret = device_find_first_child_by_uclass(parent, UCLASS_BOOTDEV, &bdev); if (ret) { if (ret != -ENODEV) { log_debug("Cannot access bootdev device\n"); return ret; }
ret = bootdev_bind(parent, drv_name, "bootdev", &bdev); if (ret) { log_debug("Cannot create bootdev device\n"); return ret; } }
return 0;+}
UCLASS_DRIVER(tpm) = {
.id = UCLASS_TPM,.name = "tpm",.flags = DM_UC_FLAG_SEQ_ALIAS,
.id = UCLASS_TPM,.name = "tpm",.flags = DM_UC_FLAG_SEQ_ALIAS,#if CONFIG_IS_ENABLED(OF_REAL)
.post_bind = dm_scan_fdt_dev,
.post_bind = dm_scan_fdt_dev,#endif
.post_probe = tpm_uclass_post_probe,Should be post_bind.
Okay
[..]
Regards, Simon
Hi Ilias,
On Tue, 15 Mar 2022 at 00:34, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Mon, 14 Mar 2022 at 20:24, Simon Glass sjg@chromium.org wrote:
[...]
}This really should be in the device tree so what you are doing here is quite strange.
Like I had mentioned in my earlier emails, the TPM device has a builtin RNG functionality, which is non-optional. So I don't understand why we need to use a device tree subnode here. Whether the device is being bound to the parent is being controlled by the TPM_RNG config that you asked me to put in my previous version, which I am doing.
See how PMICs work, for example. We have GPIOs, regulators and suchlike in the PMIC and we add subnodes for them in the DT. It is just how it is done.
Driver model is designed to automatically bind devices based on the device tree. There are cases where it is necessary to manually bind things, but we mustn't prevent people from doing it 'properly'.
There's a small difference here though. The RNG is not a device. The TPM is the device and an encoded command to that device returns a random number. There's no binding initiating etc.
A device is just something with a struct udevice, so I don't see the random number generator as anything different from another device. We might have a white-noise generator which produces random numbers. Just because the feature is packaged inside a single chip doesn't make it any less a device. Just like the PMIC.
Finally, I know you keep saying that random numbers are only needed in U-Boot proper, but if I want a random number in SPL, it may not work, since device_bind() is often not available, for code-size reasons.
And the entire tpm framework will fit?
Yes. For verified boot it has to, since you cannot init RAM until you have selected your A/B/recovery image.
So that is why I say that what you are doing is quite strange. Perhaps you are coming from a different project, which does things differently.
I don't personally find it strange. The device is already described in the DTS and I don't see a strong reason to deviate for the upstream version again.
Linux tends to rely a lot more on manually adding devices. It can have a pretty dramatic bloating effect on code size in U-Boot.
Anyway, so long as we can detect an existing device, as I explained below, it is fine to manually add it when it is missing.
Regards /Ilias
If you want to manually bind it, please call
device_find_first_child_by_uclass() first to make sure it isn't already there.
Okay
Also you should bind it in the bind() method, not in probe().
Okay
This is the code used for the same thing in the bootstd series:
struct udevice *bdev; int ret;
ret = device_find_first_child_by_uclass(parent, UCLASS_BOOTDEV, &bdev); if (ret) { if (ret != -ENODEV) { log_debug("Cannot access bootdev device\n"); return ret; }
ret = bootdev_bind(parent, drv_name, "bootdev", &bdev); if (ret) { log_debug("Cannot create bootdev device\n"); return ret; } }
return 0;+}
UCLASS_DRIVER(tpm) = {
.id = UCLASS_TPM,.name = "tpm",.flags = DM_UC_FLAG_SEQ_ALIAS,
.id = UCLASS_TPM,.name = "tpm",.flags = DM_UC_FLAG_SEQ_ALIAS,#if CONFIG_IS_ENABLED(OF_REAL)
.post_bind = dm_scan_fdt_dev,
.post_bind = dm_scan_fdt_dev,#endif
.post_probe = tpm_uclass_post_probe,Should be post_bind.
Okay
[..]
Regards, Simon
hi Simon,
On Wed, 16 Mar 2022 at 02:45, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Tue, 15 Mar 2022 at 00:34, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Mon, 14 Mar 2022 at 20:24, Simon Glass sjg@chromium.org wrote:
[...]
}This really should be in the device tree so what you are doing here is quite strange.
Like I had mentioned in my earlier emails, the TPM device has a builtin RNG functionality, which is non-optional. So I don't understand why we need to use a device tree subnode here. Whether the device is being bound to the parent is being controlled by the TPM_RNG config that you asked me to put in my previous version, which I am doing.
See how PMICs work, for example. We have GPIOs, regulators and suchlike in the PMIC and we add subnodes for them in the DT. It is just how it is done.
Driver model is designed to automatically bind devices based on the device tree. There are cases where it is necessary to manually bind things, but we mustn't prevent people from doing it 'properly'.
There's a small difference here though. The RNG is not a device. The TPM is the device and an encoded command to that device returns a random number. There's no binding initiating etc.
A device is just something with a struct udevice, so I don't see the random number generator as anything different from another device. We might have a white-noise generator which produces random numbers. Just because the feature is packaged inside a single chip doesn't make it any less a device. Just like the PMIC.
Finally, I know you keep saying that random numbers are only needed in U-Boot proper, but if I want a random number in SPL, it may not work, since device_bind() is often not available, for code-size reasons.
And the entire tpm framework will fit?
Yes. For verified boot it has to, since you cannot init RAM until you have selected your A/B/recovery image.
So that is why I say that what you are doing is quite strange. Perhaps you are coming from a different project, which does things differently.
I don't personally find it strange. The device is already described in the DTS and I don't see a strong reason to deviate for the upstream version again.
Linux tends to rely a lot more on manually adding devices. It can have a pretty dramatic bloating effect on code size in U-Boot.
Anyway, so long as we can detect an existing device, as I explained below, it is fine to manually add it when it is missing.
Just so that I understand what you are saying, do you want support for both approaches. Meaning, using device tree when the rng node is described in the device tree, and otherwise using the manual device binding when the device tree node is absent. Do I understand this right?
-sughosh
Regards /Ilias
If you want to manually bind it, please call
device_find_first_child_by_uclass() first to make sure it isn't already there.
Okay
Also you should bind it in the bind() method, not in probe().
Okay
This is the code used for the same thing in the bootstd series:
struct udevice *bdev; int ret;
ret = device_find_first_child_by_uclass(parent, UCLASS_BOOTDEV, &bdev); if (ret) { if (ret != -ENODEV) { log_debug("Cannot access bootdev device\n"); return ret; }
ret = bootdev_bind(parent, drv_name, "bootdev", &bdev); if (ret) { log_debug("Cannot create bootdev device\n"); return ret; } }
return 0;+}
UCLASS_DRIVER(tpm) = {
.id = UCLASS_TPM,.name = "tpm",.flags = DM_UC_FLAG_SEQ_ALIAS,
.id = UCLASS_TPM,.name = "tpm",.flags = DM_UC_FLAG_SEQ_ALIAS,#if CONFIG_IS_ENABLED(OF_REAL)
.post_bind = dm_scan_fdt_dev,
.post_bind = dm_scan_fdt_dev,#endif
.post_probe = tpm_uclass_post_probe,Should be post_bind.
Okay
[..]
Regards, Simon
Hi Sughosh,
On Mon, 21 Mar 2022 at 00:01, Sughosh Ganu sughosh.ganu@linaro.org wrote:
hi Simon,
On Wed, 16 Mar 2022 at 02:45, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Tue, 15 Mar 2022 at 00:34, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Mon, 14 Mar 2022 at 20:24, Simon Glass sjg@chromium.org wrote:
[...]
> + }
This really should be in the device tree so what you are doing here is quite strange.
Like I had mentioned in my earlier emails, the TPM device has a builtin RNG functionality, which is non-optional. So I don't understand why we need to use a device tree subnode here. Whether the device is being bound to the parent is being controlled by the TPM_RNG config that you asked me to put in my previous version, which I am doing.
See how PMICs work, for example. We have GPIOs, regulators and suchlike in the PMIC and we add subnodes for them in the DT. It is just how it is done.
Driver model is designed to automatically bind devices based on the device tree. There are cases where it is necessary to manually bind things, but we mustn't prevent people from doing it 'properly'.
There's a small difference here though. The RNG is not a device. The TPM is the device and an encoded command to that device returns a random number. There's no binding initiating etc.
A device is just something with a struct udevice, so I don't see the random number generator as anything different from another device. We might have a white-noise generator which produces random numbers. Just because the feature is packaged inside a single chip doesn't make it any less a device. Just like the PMIC.
Finally, I know you keep saying that random numbers are only needed in U-Boot proper, but if I want a random number in SPL, it may not work, since device_bind() is often not available, for code-size reasons.
And the entire tpm framework will fit?
Yes. For verified boot it has to, since you cannot init RAM until you have selected your A/B/recovery image.
So that is why I say that what you are doing is quite strange. Perhaps you are coming from a different project, which does things differently.
I don't personally find it strange. The device is already described in the DTS and I don't see a strong reason to deviate for the upstream version again.
Linux tends to rely a lot more on manually adding devices. It can have a pretty dramatic bloating effect on code size in U-Boot.
Anyway, so long as we can detect an existing device, as I explained below, it is fine to manually add it when it is missing.
Just so that I understand what you are saying, do you want support for both approaches. Meaning, using device tree when the rng node is described in the device tree, and otherwise using the manual device binding when the device tree node is absent. Do I understand this right?
Yes that's what we normally do in U-Boot.
Regards, Simon
Hi Simon,
A bit late on this, but at least I found it...
On Wed, Mar 16, 2022 at 10:15:34AM +1300, Simon Glass wrote:
Hi Ilias,
On Tue, 15 Mar 2022 at 00:34, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Mon, 14 Mar 2022 at 20:24, Simon Glass sjg@chromium.org wrote:
[...]
}This really should be in the device tree so what you are doing here is quite strange.
Like I had mentioned in my earlier emails, the TPM device has a builtin RNG functionality, which is non-optional. So I don't understand why we need to use a device tree subnode here. Whether the device is being bound to the parent is being controlled by the TPM_RNG config that you asked me to put in my previous version, which I am doing.
See how PMICs work, for example. We have GPIOs, regulators and suchlike in the PMIC and we add subnodes for them in the DT. It is just how it is done.
Driver model is designed to automatically bind devices based on the device tree. There are cases where it is necessary to manually bind things, but we mustn't prevent people from doing it 'properly'.
There's a small difference here though. The RNG is not a device. The TPM is the device and an encoded command to that device returns a random number. There's no binding initiating etc.
A device is just something with a struct udevice, so I don't see the random number generator as anything different from another device. We might have a white-noise generator which produces random numbers. Just because the feature is packaged inside a single chip doesn't make it any less a device. Just like the PMIC.
Finally, I know you keep saying that random numbers are only needed in U-Boot proper, but if I want a random number in SPL, it may not work, since device_bind() is often not available, for code-size reasons.
And the entire tpm framework will fit?
Yes. For verified boot it has to, since you cannot init RAM until you have selected your A/B/recovery image.
If so I still don't see why this needs a DT node. If, as you say, the TPM framework fits in then you just call tpm_get_random(). Getting the RNG is a series of commands to the TPM. Sou you'll need the TPM lib in SPL regardless.
So that is why I say that what you are doing is quite strange. Perhaps you are coming from a different project, which does things differently.
I don't personally find it strange. The device is already described in the DTS and I don't see a strong reason to deviate for the upstream version again.
Linux tends to rely a lot more on manually adding devices. It can have a pretty dramatic bloating effect on code size in U-Boot.
Anyway, so long as we can detect an existing device, as I explained below, it is fine to manually add it when it is missing.
You can detect a TPM. Once you add that tpm then you *know* you can get an RNG
[...]
Regards /Ilias
hi Simon,
On Mon, 14 Mar 2022 at 23:54, Simon Glass sjg@chromium.org wrote:
Hi Sughosh,
On Mon, 14 Mar 2022 at 05:43, 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 comes with the random number generator(RNG) functionality which is built into the TPM device. Add logic to add the RNG child device in the TPM uclass post probe callback.
The RNG device can then be used to pass a set of random bytes to the linux kernel, need for address space randomisation through the EFI_RNG_PROTOCOL interface.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org
Changes since V4:
- Put a check for CONFIG_TPM_RNG for binding the RNG device with it's driver in the post_probe callback instead of putting CONFIG_{SPL,TPL}_BUILD guards
drivers/tpm/tpm-uclass.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
This looks a lot better, please see below.
diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c index f67fe1019b..e1c61d26f0 100644 --- a/drivers/tpm/tpm-uclass.c +++ b/drivers/tpm/tpm-uclass.c @@ -11,10 +11,15 @@ #include <log.h> #include <linux/delay.h> #include <linux/unaligned/be_byteshift.h> +#include <tpm_api.h> #include <tpm-v1.h> #include <tpm-v2.h> #include "tpm_internal.h"
+#include <dm/lists.h>
+#define TPM_RNG_DRV_NAME "tpm-rng"
int tpm_open(struct udevice *dev) { struct tpm_ops *ops = tpm_get_ops(dev); @@ -136,12 +141,28 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size, return 0; }
+static int tpm_uclass_post_probe(struct udevice *dev) +{
int ret;const char *drv = TPM_RNG_DRV_NAME;struct udevice *child;if (CONFIG_IS_ENABLED(TPM_RNG)) {ret = device_bind_driver(dev, drv, "tpm-rng0", &child);if (ret)return log_msg_ret("bind", ret);}This really should be in the device tree so what you are doing here is quite strange.
Like I had mentioned in my earlier emails, the TPM device has a builtin RNG functionality, which is non-optional. So I don't understand why we need to use a device tree subnode here. Whether the device is being bound to the parent is being controlled by the TPM_RNG config that you asked me to put in my previous version, which I am doing.
See how PMICs work, for example. We have GPIOs, regulators and suchlike in the PMIC and we add subnodes for them in the DT. It is just how it is done.
Driver model is designed to automatically bind devices based on the device tree. There are cases where it is necessary to manually bind things, but we mustn't prevent people from doing it 'properly'.
Finally, I know you keep saying that random numbers are only needed in U-Boot proper, but if I want a random number in SPL, it may not work, since device_bind() is often not available, for code-size reasons.
So that is why I say that what you are doing is quite strange. Perhaps you are coming from a different project, which does things differently.
Well, FWIW I actually found usage of this kind of device binding in this very project. There are quite a few drivers which are using the API in the same way that is being done in this patch. And I have already mentioned the reason that I am using this method as against a device tree. Thanks.
-sughosh
If you want to manually bind it, please call
device_find_first_child_by_uclass() first to make sure it isn't already there.
Okay
Also you should bind it in the bind() method, not in probe().
Okay
This is the code used for the same thing in the bootstd series:
struct udevice *bdev; int ret;
ret = device_find_first_child_by_uclass(parent, UCLASS_BOOTDEV, &bdev); if (ret) { if (ret != -ENODEV) { log_debug("Cannot access bootdev device\n"); return ret; }
ret = bootdev_bind(parent, drv_name, "bootdev", &bdev); if (ret) { log_debug("Cannot create bootdev device\n"); return ret; } }
return 0;+}
UCLASS_DRIVER(tpm) = {
.id = UCLASS_TPM,.name = "tpm",.flags = DM_UC_FLAG_SEQ_ALIAS,
.id = UCLASS_TPM,.name = "tpm",.flags = DM_UC_FLAG_SEQ_ALIAS,#if CONFIG_IS_ENABLED(OF_REAL)
.post_bind = dm_scan_fdt_dev,
.post_bind = dm_scan_fdt_dev,#endif
.post_probe = tpm_uclass_post_probe,Should be post_bind.
Okay
[..]
Regards, Simon
The Qemu platform has a function defined to get the random number generator(RNG) device. However, the RNG device can be obtained simply by searching for a device belonging to the RNG uclass. Remove the superfluous platform function defined for the Qemu platform for getting the RNG device.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Tested-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org Reviewed-by: Simon Glass sjg@chromium.org ---
Changes since V4: None
board/emulation/qemu-arm/qemu-arm.c | 42 ----------------------------- 1 file changed, 42 deletions(-)
diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c index 16d5a97167..c9e886e44a 100644 --- a/board/emulation/qemu-arm/qemu-arm.c +++ b/board/emulation/qemu-arm/qemu-arm.c @@ -107,48 +107,6 @@ void enable_caches(void) dcache_enable(); }
-#if defined(CONFIG_EFI_RNG_PROTOCOL) -#include <efi_loader.h> -#include <efi_rng.h> - -#include <dm/device-internal.h> - -efi_status_t platform_get_rng_device(struct udevice **dev) -{ - int ret; - efi_status_t status = EFI_DEVICE_ERROR; - struct udevice *bus, *devp; - - for (uclass_first_device(UCLASS_VIRTIO, &bus); bus; - uclass_next_device(&bus)) { - for (device_find_first_child(bus, &devp); devp; - device_find_next_child(&devp)) { - if (device_get_uclass_id(devp) == UCLASS_RNG) { - *dev = devp; - status = EFI_SUCCESS; - break; - } - } - } - - if (status != EFI_SUCCESS) { - debug("No rng device found\n"); - return EFI_DEVICE_ERROR; - } - - if (*dev) { - ret = device_probe(*dev); - if (ret) - return EFI_DEVICE_ERROR; - } else { - debug("Couldn't get child device\n"); - return EFI_DEVICE_ERROR; - } - - return EFI_SUCCESS; -} -#endif /* CONFIG_EFI_RNG_PROTOCOL */ - #ifdef CONFIG_ARM64 #define __W "w" #else
hi Heinrich,
On Sun, 13 Mar 2022 at 20:18, Sughosh Ganu sughosh.ganu@linaro.org wrote:
The Qemu platform has a function defined to get the random number generator(RNG) device. However, the RNG device can be obtained simply by searching for a device belonging to the RNG uclass. Remove the superfluous platform function defined for the Qemu platform for getting the RNG device.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Tested-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org Reviewed-by: Simon Glass sjg@chromium.org
Changes since V4: None
board/emulation/qemu-arm/qemu-arm.c | 42 ----------------------------- 1 file changed, 42 deletions(-)
Can you please pick this up, as this has been reviewed, and is not related to the TPM RNG discussion. Thanks.
-sughosh
diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c index 16d5a97167..c9e886e44a 100644 --- a/board/emulation/qemu-arm/qemu-arm.c +++ b/board/emulation/qemu-arm/qemu-arm.c @@ -107,48 +107,6 @@ void enable_caches(void) dcache_enable(); }
-#if defined(CONFIG_EFI_RNG_PROTOCOL) -#include <efi_loader.h> -#include <efi_rng.h>
-#include <dm/device-internal.h>
-efi_status_t platform_get_rng_device(struct udevice **dev) -{
int ret;efi_status_t status = EFI_DEVICE_ERROR;struct udevice *bus, *devp;for (uclass_first_device(UCLASS_VIRTIO, &bus); bus;uclass_next_device(&bus)) {for (device_find_first_child(bus, &devp); devp;device_find_next_child(&devp)) {if (device_get_uclass_id(devp) == UCLASS_RNG) {*dev = devp;status = EFI_SUCCESS;break;}}}if (status != EFI_SUCCESS) {debug("No rng device found\n");return EFI_DEVICE_ERROR;}if (*dev) {ret = device_probe(*dev);if (ret)return EFI_DEVICE_ERROR;} else {debug("Couldn't get child device\n");return EFI_DEVICE_ERROR;}return EFI_SUCCESS;-} -#endif /* CONFIG_EFI_RNG_PROTOCOL */
#ifdef CONFIG_ARM64 #define __W "w"
#else
2.25.1
On 3/24/22 17:44, Sughosh Ganu wrote:
On Sun, 13 Mar 2022 at 20:18, Sughosh Ganusughosh.ganu@linaro.org wrote:
The Qemu platform has a function defined to get the random number generator(RNG) device. However, the RNG device can be obtained simply by searching for a device belonging to the RNG uclass. Remove the superfluous platform function defined for the Qemu platform for getting the RNG device.
Signed-off-by: Sughosh Ganusughosh.ganu@linaro.org Tested-by: Heinrich Schuchardtxypron.glpk@gmx.de Reviewed-by: Ilias Apalodimasilias.apalodimas@linaro.org Reviewed-by: Simon Glasssjg@chromium.org
Changes since V4: None
board/emulation/qemu-arm/qemu-arm.c | 42 ----------------------------- 1 file changed, 42 deletions(-)
Can you please pick this up, as this has been reviewed, and is not related to the TPM RNG discussion. Thanks.
-sughosh
Hello Sughosh,
I have added the patch to my queue.
Best regards
Heinrich
The 'rng' u-boot command is used for printing a select number of random bytes on the console. Currently, the RNG device from which the random bytes are read is fixed. However, a platform can have multiple RNG devices, one example being qemu, which has a virtio RNG device and the RNG pseudo device through the TPM chip.
Extend the 'rng' command so that the user can provide the RNG device number from which the random bytes are to be read. This will be the device index under the RNG uclass.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Tested-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org ---
Changes since V4:
* Use uclass_get_device_by_seq API to get the RNG device as suggested by Simon
cmd/rng.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/cmd/rng.c b/cmd/rng.c index 1ad5a096c0..2ddf27545f 100644 --- a/cmd/rng.c +++ b/cmd/rng.c @@ -13,19 +13,34 @@
static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { - size_t n = 0x40; + size_t n; struct udevice *dev; void *buf; + int devnum; int ret = CMD_RET_SUCCESS;
- if (uclass_get_device(UCLASS_RNG, 0, &dev) || !dev) { + switch (argc) { + case 1: + devnum = 0; + n = 0x40; + break; + case 2: + devnum = hextoul(argv[1], NULL); + n = 0x40; + break; + case 3: + devnum = hextoul(argv[1], NULL); + n = hextoul(argv[2], NULL); + break; + default: + return CMD_RET_USAGE; + } + + if (uclass_get_device_by_seq(UCLASS_RNG, devnum, &dev) || !dev) { printf("No RNG device\n"); return CMD_RET_FAILURE; }
- if (argc >= 2) - n = hextoul(argv[1], NULL); - buf = malloc(n); if (!buf) { printf("Out of memory\n"); @@ -46,12 +61,12 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
#ifdef CONFIG_SYS_LONGHELP static char rng_help_text[] = - "[n]\n" - " - print n random bytes\n"; + "[dev [n]]\n" + " - print n random bytes read from dev\n"; #endif
U_BOOT_CMD( - rng, 2, 0, do_rng, + rng, 3, 0, do_rng, "print bytes from the hardware random number generator", rng_help_text );
Hi Sughosh,
On Sun, 13 Mar 2022 at 08:49, Sughosh Ganu sughosh.ganu@linaro.org wrote:
The 'rng' u-boot command is used for printing a select number of random bytes on the console. Currently, the RNG device from which the random bytes are read is fixed. However, a platform can have multiple RNG devices, one example being qemu, which has a virtio RNG device and the RNG pseudo device through the TPM chip.
Extend the 'rng' command so that the user can provide the RNG device number from which the random bytes are to be read. This will be the device index under the RNG uclass.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Tested-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Changes since V4:
- Use uclass_get_device_by_seq API to get the RNG device as suggested by Simon
cmd/rng.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
with the nit below fixed
diff --git a/cmd/rng.c b/cmd/rng.c index 1ad5a096c0..2ddf27545f 100644 --- a/cmd/rng.c +++ b/cmd/rng.c @@ -13,19 +13,34 @@
static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) {
size_t n = 0x40;
size_t n; struct udevice *dev; void *buf;int devnum; int ret = CMD_RET_SUCCESS;
if (uclass_get_device(UCLASS_RNG, 0, &dev) || !dev) {
switch (argc) {case 1:devnum = 0;n = 0x40;break;case 2:devnum = hextoul(argv[1], NULL);n = 0x40;break;case 3:devnum = hextoul(argv[1], NULL);n = hextoul(argv[2], NULL);break;default:return CMD_RET_USAGE;}if (uclass_get_device_by_seq(UCLASS_RNG, devnum, &dev) || !dev) {
Please check the function comments: you can drop the '|| !dev' bit since it returns an error if no device is found.
printf("No RNG device\n"); return CMD_RET_FAILURE; }
if (argc >= 2)n = hextoul(argv[1], NULL);buf = malloc(n); if (!buf) { printf("Out of memory\n");@@ -46,12 +61,12 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
#ifdef CONFIG_SYS_LONGHELP static char rng_help_text[] =
"[n]\n"" - print n random bytes\n";
"[dev [n]]\n"" - print n random bytes read from dev\n";#endif
U_BOOT_CMD(
rng, 2, 0, do_rng,
rng, 3, 0, do_rng, "print bytes from the hardware random number generator", rng_help_text);
2.25.1
Regards, SImon
hi Heinrich,
On Mon, 14 Mar 2022 at 03:53, Simon Glass sjg@chromium.org wrote:
Hi Sughosh,
On Sun, 13 Mar 2022 at 08:49, Sughosh Ganu sughosh.ganu@linaro.org wrote:
The 'rng' u-boot command is used for printing a select number of random bytes on the console. Currently, the RNG device from which the random bytes are read is fixed. However, a platform can have multiple RNG devices, one example being qemu, which has a virtio RNG device and the RNG pseudo device through the TPM chip.
Extend the 'rng' command so that the user can provide the RNG device number from which the random bytes are to be read. This will be the device index under the RNG uclass.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Tested-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Changes since V4:
- Use uclass_get_device_by_seq API to get the RNG device as suggested by Simon
cmd/rng.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-)
Can you please pick this up, as this has been reviewed and is not related to the TPM RNG changes which are still under discussion. Thanks.
-sughosh
Reviewed-by: Simon Glass sjg@chromium.org
with the nit below fixed
diff --git a/cmd/rng.c b/cmd/rng.c index 1ad5a096c0..2ddf27545f 100644 --- a/cmd/rng.c +++ b/cmd/rng.c @@ -13,19 +13,34 @@
static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) {
size_t n = 0x40;
size_t n; struct udevice *dev; void *buf;int devnum; int ret = CMD_RET_SUCCESS;
if (uclass_get_device(UCLASS_RNG, 0, &dev) || !dev) {
switch (argc) {case 1:devnum = 0;n = 0x40;break;case 2:devnum = hextoul(argv[1], NULL);n = 0x40;break;case 3:devnum = hextoul(argv[1], NULL);n = hextoul(argv[2], NULL);break;default:return CMD_RET_USAGE;}if (uclass_get_device_by_seq(UCLASS_RNG, devnum, &dev) || !dev) {Please check the function comments: you can drop the '|| !dev' bit since it returns an error if no device is found.
printf("No RNG device\n"); return CMD_RET_FAILURE; }
if (argc >= 2)n = hextoul(argv[1], NULL);buf = malloc(n); if (!buf) { printf("Out of memory\n");@@ -46,12 +61,12 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
#ifdef CONFIG_SYS_LONGHELP static char rng_help_text[] =
"[n]\n"" - print n random bytes\n";
"[dev [n]]\n"" - print n random bytes read from dev\n";#endif
U_BOOT_CMD(
rng, 2, 0, do_rng,
rng, 3, 0, do_rng, "print bytes from the hardware random number generator", rng_help_text);
2.25.1
Regards, SImon
Use a statically allocated buffer on stack instead of using malloc for reading the random bytes. Using a local array is faster than allocating heap memory on every initiation of the command.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org ---
Changes since V4:
* New patch based on review comments from Simon to not use the malloc call
cmd/rng.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/cmd/rng.c b/cmd/rng.c index 2ddf27545f..81a23964b8 100644 --- a/cmd/rng.c +++ b/cmd/rng.c @@ -14,9 +14,9 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { size_t n; - struct udevice *dev; - void *buf; + u8 buf[64]; int devnum; + struct udevice *dev; int ret = CMD_RET_SUCCESS;
switch (argc) { @@ -41,11 +41,10 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) return CMD_RET_FAILURE; }
- buf = malloc(n); - if (!buf) { - printf("Out of memory\n"); - return CMD_RET_FAILURE; - } + if (!n) + return 0; + + n = min(n, sizeof(buf));
if (dm_rng_read(dev, buf, n)) { printf("Reading RNG failed\n"); @@ -54,15 +53,13 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, buf, n); }
- free(buf); - return ret; }
#ifdef CONFIG_SYS_LONGHELP static char rng_help_text[] = "[dev [n]]\n" - " - print n random bytes read from dev\n"; + " - print n random bytes(max 64) read from dev\n"; #endif
U_BOOT_CMD(
Hi Sughosh,
On Sun, 13 Mar 2022 at 08:49, Sughosh Ganu sughosh.ganu@linaro.org wrote:
Use a statically allocated buffer on stack instead of using malloc for reading the random bytes. Using a local array is faster than allocating heap memory on every initiation of the command.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org
Changes since V4:
- New patch based on review comments from Simon to not use the malloc call
cmd/rng.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
It might be easier to put this patch before the other one.
diff --git a/cmd/rng.c b/cmd/rng.c index 2ddf27545f..81a23964b8 100644 --- a/cmd/rng.c +++ b/cmd/rng.c @@ -14,9 +14,9 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { size_t n;
struct udevice *dev;void *buf;
u8 buf[64]; int devnum;struct udevice *dev; int ret = CMD_RET_SUCCESS; switch (argc) {@@ -41,11 +41,10 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) return CMD_RET_FAILURE; }
buf = malloc(n);if (!buf) {printf("Out of memory\n");return CMD_RET_FAILURE;}
if (!n)return 0;n = min(n, sizeof(buf)); if (dm_rng_read(dev, buf, n)) { printf("Reading RNG failed\n");
This looks like a Windows-style error. How about adding "(err=%d)", ret to this so we can see the error?
@@ -54,15 +53,13 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, buf, n); }
free(buf);return ret;}
#ifdef CONFIG_SYS_LONGHELP static char rng_help_text[] = "[dev [n]]\n"
" - print n random bytes read from dev\n";
" - print n random bytes(max 64) read from dev\n";#endif
U_BOOT_CMD(
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:49, Sughosh Ganu sughosh.ganu@linaro.org wrote:
Use a statically allocated buffer on stack instead of using malloc for reading the random bytes. Using a local array is faster than allocating heap memory on every initiation of the command.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org
Changes since V4:
- New patch based on review comments from Simon to not use the malloc call
cmd/rng.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
It might be easier to put this patch before the other one.
diff --git a/cmd/rng.c b/cmd/rng.c index 2ddf27545f..81a23964b8 100644 --- a/cmd/rng.c +++ b/cmd/rng.c @@ -14,9 +14,9 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { size_t n;
struct udevice *dev;void *buf;
u8 buf[64]; int devnum;struct udevice *dev; int ret = CMD_RET_SUCCESS; switch (argc) {@@ -41,11 +41,10 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) return CMD_RET_FAILURE; }
buf = malloc(n);if (!buf) {printf("Out of memory\n");return CMD_RET_FAILURE;}
if (!n)return 0;n = min(n, sizeof(buf)); if (dm_rng_read(dev, buf, n)) { printf("Reading RNG failed\n");This looks like a Windows-style error. How about adding "(err=%d)", ret to this so we can see the error?
Again, this is not being changed by my patch. This is existing code which is not being touched by the patch. These kinds of fixes should be taken up separately. Thanks.
-sughosh
@@ -54,15 +53,13 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, buf, n); }
free(buf);return ret;}
#ifdef CONFIG_SYS_LONGHELP static char rng_help_text[] = "[dev [n]]\n"
" - print n random bytes read from dev\n";
" - print n random bytes(max 64) read from dev\n";#endif
U_BOOT_CMD(
2.25.1
Regards, SImon
Hi Sughosh,
On Mon, 14 Mar 2022 at 05:27, 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:49, Sughosh Ganu sughosh.ganu@linaro.org wrote:
Use a statically allocated buffer on stack instead of using malloc for reading the random bytes. Using a local array is faster than allocating heap memory on every initiation of the command.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org
Changes since V4:
- New patch based on review comments from Simon to not use the malloc call
cmd/rng.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
It might be easier to put this patch before the other one.
diff --git a/cmd/rng.c b/cmd/rng.c index 2ddf27545f..81a23964b8 100644 --- a/cmd/rng.c +++ b/cmd/rng.c @@ -14,9 +14,9 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { size_t n;
struct udevice *dev;void *buf;
u8 buf[64]; int devnum;struct udevice *dev; int ret = CMD_RET_SUCCESS; switch (argc) {@@ -41,11 +41,10 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) return CMD_RET_FAILURE; }
buf = malloc(n);if (!buf) {printf("Out of memory\n");return CMD_RET_FAILURE;}
if (!n)return 0;n = min(n, sizeof(buf)); if (dm_rng_read(dev, buf, n)) { printf("Reading RNG failed\n");This looks like a Windows-style error. How about adding "(err=%d)", ret to this so we can see the error?
Again, this is not being changed by my patch. This is existing code which is not being touched by the patch. These kinds of fixes should be taken up separately. Thanks.
Ah yes I keep missing that.
- Simon
[..]
hi Heinrich,
On Sun, 13 Mar 2022 at 20:19, Sughosh Ganu sughosh.ganu@linaro.org wrote:
Use a statically allocated buffer on stack instead of using malloc for reading the random bytes. Using a local array is faster than allocating heap memory on every initiation of the command.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org
Changes since V4:
- New patch based on review comments from Simon to not use the malloc call
cmd/rng.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
Can you please pick this up, as this has been reviewed and is not related to the TPM RNG changes which are still under discussion. Thanks.
-sughosh
diff --git a/cmd/rng.c b/cmd/rng.c index 2ddf27545f..81a23964b8 100644 --- a/cmd/rng.c +++ b/cmd/rng.c @@ -14,9 +14,9 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { size_t n;
struct udevice *dev;void *buf;
u8 buf[64]; int devnum;struct udevice *dev; int ret = CMD_RET_SUCCESS; switch (argc) {@@ -41,11 +41,10 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) return CMD_RET_FAILURE; }
buf = malloc(n);if (!buf) {printf("Out of memory\n");return CMD_RET_FAILURE;}
if (!n)return 0;n = min(n, sizeof(buf)); if (dm_rng_read(dev, buf, n)) { printf("Reading RNG failed\n");@@ -54,15 +53,13 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, buf, n); }
free(buf);return ret;}
#ifdef CONFIG_SYS_LONGHELP static char rng_help_text[] = "[dev [n]]\n"
" - print n random bytes read from dev\n";
" - print n random bytes(max 64) read from dev\n";#endif
U_BOOT_CMD(
2.25.1
Add a usage document for the 'rng' u-boot command.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org Reviewed-by: Simon Glass sjg@chromium.org ---
Changes since V4:
* Reflect the fact that a maximum of 64 bytes can be read on each invocation of the 'rng' command
doc/usage/index.rst | 1 + doc/usage/rng.rst | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+) create mode 100644 doc/usage/rng.rst
diff --git a/doc/usage/index.rst b/doc/usage/index.rst index 0aacf531b2..5712a924ae 100644 --- a/doc/usage/index.rst +++ b/doc/usage/index.rst @@ -45,6 +45,7 @@ Shell commands pstore qfw reset + rng sbi sf scp03 diff --git a/doc/usage/rng.rst b/doc/usage/rng.rst new file mode 100644 index 0000000000..1a352da41a --- /dev/null +++ b/doc/usage/rng.rst @@ -0,0 +1,26 @@ +.. SPDX-License-Identifier: GPL-2.0+ + +rng command +=========== + +Synopsis +-------- + +:: + + rng [devnum [n]] + +Description +----------- + +The *rng* command reads the random number generator(RNG) device and +prints the random bytes read on the console. A maximum of 64 bytes can +be read in one invocation of the command. + +devnum + The RNG device from which the random bytes are to be + read. Defaults to 0. + +n + Number of random bytes to be read and displayed on the + console. Default value is 0x40. Max value is 0x40.
hi Heinrich,
On Sun, 13 Mar 2022 at 20:19, Sughosh Ganu sughosh.ganu@linaro.org wrote:
Add a usage document for the 'rng' u-boot command.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org Reviewed-by: Simon Glass sjg@chromium.org
Changes since V4:
- Reflect the fact that a maximum of 64 bytes can be read on each invocation of the 'rng' command
doc/usage/index.rst | 1 + doc/usage/rng.rst | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+) create mode 100644 doc/usage/rng.rst
Can you please pick this up, as this has been reviewed and is not related to the TPM RNG changes which are still under discussion. Thanks.
-sughosh
diff --git a/doc/usage/index.rst b/doc/usage/index.rst index 0aacf531b2..5712a924ae 100644 --- a/doc/usage/index.rst +++ b/doc/usage/index.rst @@ -45,6 +45,7 @@ Shell commands pstore qfw reset
- rng sbi sf scp03
diff --git a/doc/usage/rng.rst b/doc/usage/rng.rst new file mode 100644 index 0000000000..1a352da41a --- /dev/null +++ b/doc/usage/rng.rst @@ -0,0 +1,26 @@ +.. SPDX-License-Identifier: GPL-2.0+
+rng command +===========
+Synopsis +--------
+::
- rng [devnum [n]]
+Description +-----------
+The *rng* command reads the random number generator(RNG) device and +prints the random bytes read on the console. A maximum of 64 bytes can +be read in one invocation of the command.
+devnum
- The RNG device from which the random bytes are to be
- read. Defaults to 0.
+n
- Number of random bytes to be read and displayed on the
- console. Default value is 0x40. Max value is 0x40.
-- 2.25.1
The 'rng' command dumps a number of random bytes on the console. Add a set of tests for the 'rng' command. The test function performs basic sanity testing of the command.
Since a unit test is being added for the command, enable it by default in the sandbox platforms.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Simon Glass sjg@chromium.org ---
Changes since V4: None
cmd/Kconfig | 1 + test/dm/rng.c | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 5e25e45fd2..47f1e23ef0 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1810,6 +1810,7 @@ config CMD_GETTIME config CMD_RNG bool "rng command" depends on DM_RNG + default y if SANDBOX select HEXDUMP help Print bytes from the hardware random number generator. diff --git a/test/dm/rng.c b/test/dm/rng.c index 5b34c93ed6..6d1f68848d 100644 --- a/test/dm/rng.c +++ b/test/dm/rng.c @@ -25,3 +25,32 @@ static int dm_test_rng_read(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_rng_read, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT); + +/* Test the rng command */ +static int dm_test_rng_cmd(struct unit_test_state *uts) +{ + struct udevice *dev; + + ut_assertok(uclass_get_device(UCLASS_RNG, 0, &dev)); + ut_assertnonnull(dev); + + ut_assertok(console_record_reset_enable()); + + run_command("rng", 0); + ut_assert_nextlinen("00000000:"); + ut_assert_nextlinen("00000010:"); + ut_assert_nextlinen("00000020:"); + ut_assert_nextlinen("00000030:"); + ut_assert_console_end(); + + run_command("rng 0 10", 0); + ut_assert_nextlinen("00000000:"); + ut_assert_console_end(); + + run_command("rng 20", 0); + ut_assert_nextlinen("No RNG device"); + ut_assert_console_end(); + + return 0; +} +DM_TEST(dm_test_rng_cmd, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
hi Heinrich,
On Sun, 13 Mar 2022 at 20:19, Sughosh Ganu sughosh.ganu@linaro.org wrote:
The 'rng' command dumps a number of random bytes on the console. Add a set of tests for the 'rng' command. The test function performs basic sanity testing of the command.
Since a unit test is being added for the command, enable it by default in the sandbox platforms.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Simon Glass sjg@chromium.org
Changes since V4: None
cmd/Kconfig | 1 + test/dm/rng.c | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+)
Can you please pick this up as this is unrelated to the TPM RNG changes under discussion. Thanks.
-sughosh
diff --git a/cmd/Kconfig b/cmd/Kconfig index 5e25e45fd2..47f1e23ef0 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1810,6 +1810,7 @@ config CMD_GETTIME config CMD_RNG bool "rng command" depends on DM_RNG
default y if SANDBOX select HEXDUMP help Print bytes from the hardware random number generator.diff --git a/test/dm/rng.c b/test/dm/rng.c index 5b34c93ed6..6d1f68848d 100644 --- a/test/dm/rng.c +++ b/test/dm/rng.c @@ -25,3 +25,32 @@ static int dm_test_rng_read(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_rng_read, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
+/* Test the rng command */ +static int dm_test_rng_cmd(struct unit_test_state *uts) +{
struct udevice *dev;ut_assertok(uclass_get_device(UCLASS_RNG, 0, &dev));ut_assertnonnull(dev);ut_assertok(console_record_reset_enable());run_command("rng", 0);ut_assert_nextlinen("00000000:");ut_assert_nextlinen("00000010:");ut_assert_nextlinen("00000020:");ut_assert_nextlinen("00000030:");ut_assert_console_end();run_command("rng 0 10", 0);ut_assert_nextlinen("00000000:");ut_assert_console_end();run_command("rng 20", 0);ut_assert_nextlinen("No RNG device");ut_assert_console_end();return 0;+}
+DM_TEST(dm_test_rng_cmd, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
2.25.1
participants (4)
-
Heinrich Schuchardt -
Ilias Apalodimas -
Simon Glass -
Sughosh Ganu