[PATCH v5 0/9] tpm: rng: Move TPM RNG functionality to driver model

[PATCH v5 0/9] tpm: rng: Move TPM RNG functionality to driver model

[Sughosh Ganu]

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@linaro.org/
[2] - https://lore.kernel.org/u-boot/20220301001125.1554442-2-sjg@chromium.org/T/#u

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

-- 
2.25.1

[PATCH v5 1/9] tpm: rng: Change tpm_get_random to return an int

[Sughosh Ganu]

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);
-- 
2.25.1

[PATCH v5 2/9] tpm: Fix the return type of tpm_startup

[Sughosh Ganu]

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);
-- 
2.25.1

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

[Sughosh Ganu]

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;
 }
-- 
2.25.1

[PATCH v5 4/9] tpm: Add the RNG child device

[Sughosh Ganu]

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),
 };
-- 
2.25.1

[PATCH v5 5/9] qemu: arm: Remove platform specific function to get RNG device

[Sughosh Ganu]

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
-- 
2.25.1

[PATCH v5 6/9] cmd: rng: Add support for selecting RNG device

[Sughosh Ganu]

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
 );
-- 
2.25.1

[PATCH v5 7/9] cmd: rng: Use a statically allocated array for random bytes

[Sughosh Ganu]

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(
-- 
2.25.1

[PATCH v5 8/9] doc: rng: Add documentation for the rng command

[Sughosh Ganu]

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.
-- 
2.25.1

[PATCH v5 9/9] test: rng: Add a UT testcase for the rng command

[Sughosh Ganu]

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);
-- 
2.25.1

Re: [PATCH v5 4/9] tpm: Add the RNG child device

[Simon Glass]

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

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

[Simon Glass]

Hi Sughosh,

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

Please don't do that

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

Needs 3 lines of text so please add more detail

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

dev_get_parent(dev)

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

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

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

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

No please leave these alone.

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

and here

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

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

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

Regards,
Simon

Re: [PATCH v5 6/9] cmd: rng: Add support for selecting RNG device

[Simon Glass]

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

Re: [PATCH v5 7/9] cmd: rng: Use a statically allocated array for random bytes

[Simon Glass]

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

Re: [PATCH v5 7/9] cmd: rng: Use a statically allocated array for random bytes

[Sughosh Ganu]

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

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

[Sughosh Ganu]

hi Simon,

On Mon, 14 Mar 2022 at 03:53, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Sun, 13 Mar 2022 at 08:48, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > The TPM device has a builtin random number generator(RNG)
> > functionality. Expose the RNG functions of the TPM device to the
> > driver model so that they can be used by the EFI_RNG_PROTOCOL if the
> > protocol is installed.
> >
> > Also change the function arguments and return type of the random
> > number functions to comply with the driver model api.
>
> Please don't do that
>
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >
> > Changes since V4:
> >
> > * Call the existing tpm_get_random API function from the TPM RNG
> >   driver, instead of the tpm{1,2}_get_random API's
> > * Introduce a new Kconfig symbol TPM_RNG and build the corresponding
> >   driver if the symbol is enabled
> > * Change the last parameter of the tpm_get_random API to have a data
> >   type of size_t instead of u32 to comply with the RNG driver model
> >   API
> >
> >  drivers/rng/Kconfig   |  7 +++++++
> >  drivers/rng/Makefile  |  1 +
> >  drivers/rng/tpm_rng.c | 23 +++++++++++++++++++++++
> >  include/tpm-v1.h      |  4 ++--
> >  include/tpm-v2.h      |  4 ++--
> >  include/tpm_api.h     |  2 +-
> >  lib/Kconfig           |  1 +
> >  lib/tpm-v1.c          | 16 +++++++++-------
> >  lib/tpm-v2.c          |  9 +++++----
> >  lib/tpm_api.c         | 16 +++++++++++-----
> >  10 files changed, 62 insertions(+), 21 deletions(-)
> >  create mode 100644 drivers/rng/tpm_rng.c
> >
> > diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
> > index b1c5ab93d1..a89fa99ffa 100644
> > --- a/drivers/rng/Kconfig
> > +++ b/drivers/rng/Kconfig
> > @@ -49,4 +49,11 @@ config RNG_IPROC200
> >         depends on DM_RNG
> >         help
> >           Enable random number generator for RPI4.
> > +
> > +config TPM_RNG
> > +       bool "Enable random number generator on TPM device"
> > +       depends on TPM
> > +       default y
> > +       help
> > +         Enable random number generator on TPM devices
>
> Needs 3 lines of text so please add more detail

Okay

>
> >  endif
> > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> > index 39f7ee3f03..a21f3353ea 100644
> > --- a/drivers/rng/Makefile
> > +++ b/drivers/rng/Makefile
> > @@ -10,3 +10,4 @@ obj-$(CONFIG_RNG_MSM) += msm_rng.o
> >  obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
> >  obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o
> >  obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o
> > +obj-$(CONFIG_TPM_RNG) += tpm_rng.o
> > diff --git a/drivers/rng/tpm_rng.c b/drivers/rng/tpm_rng.c
> > new file mode 100644
> > index 0000000000..69b41dbbf5
> > --- /dev/null
> > +++ b/drivers/rng/tpm_rng.c
> > @@ -0,0 +1,23 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2022, Linaro Limited
> > + */
> > +
> > +#include <dm.h>
> > +#include <rng.h>
> > +#include <tpm_api.h>
> > +
> > +static int rng_tpm_random_read(struct udevice *dev, void *data, size_t count)
> > +{
> > +       return tpm_get_random(dev->parent, data, count);
>
> dev_get_parent(dev)
>
> Here you should check the return value and decide whether to return an
> error, such as -EIO
>
> > +}
> > +
> > +static const struct dm_rng_ops tpm_rng_ops = {
> > +       .read = rng_tpm_random_read,
> > +};
> > +
> > +U_BOOT_DRIVER(tpm_rng) = {
> > +       .name   = "tpm-rng",
> > +       .id     = UCLASS_RNG,
> > +       .ops    = &tpm_rng_ops,
> > +};
> > diff --git a/include/tpm-v1.h b/include/tpm-v1.h
> > index 33d53fb695..d2ff8b446d 100644
> > --- a/include/tpm-v1.h
> > +++ b/include/tpm-v1.h
> > @@ -555,9 +555,9 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
> >   * @param dev          TPM device
> >   * @param data         output buffer for the random bytes
> >   * @param count                size of output buffer
> > - * Return: return code of the operation
> > + * Return: 0 if OK, -ve on error
> >   */
> > -u32 tpm1_get_random(struct udevice *dev, void *data, u32 count);
> > +int tpm1_get_random(struct udevice *dev, void *data, size_t count);
>
> I think I mentioned this last time, but you should not change these

Can you please explain in a little more detail why? I have mentioned
in my earlier email that I am changing this to bring it in line with
the rest of the rng drivers which use a size_t data type for the
number of random bytes to read. What is the issue in changing the
signature? In any case, currently, the api is not being called from
any other module, so it is not like changing the signature is breaking
some code.

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

Done based on review comments from Heinrich[1].

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

Same as above.

>
> >                 err = tpm_sendrecv_command(dev, buf, response,
> >                                            &response_length);
> >                 if (err)
> >                         return err;
> >                 if (unpack_byte_string(response, response_length, "w",
> >                                        data_size_offset, &data_size))
> > -                       return TPM_LIB_ERROR;
> > +                       return -EIO;
> >                 if (data_size > this_bytes)
> >                         return TPM_LIB_ERROR;
> >                 if (unpack_byte_string(response, response_length, "s",
> >                                        data_offset, out, data_size))
> > -                       return TPM_LIB_ERROR;
> > +                       return -EIO;
> >
> >                 count -= data_size;
> >                 out += data_size;
> > diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> > index da48058abe..8bf60cda3c 100644
> > --- a/lib/tpm_api.c
> > +++ b/lib/tpm_api.c
> > @@ -6,6 +6,7 @@
> >  #include <common.h>
> >  #include <dm.h>
> >  #include <log.h>
> > +#include <rng.h>
> >  #include <tpm_api.h>
> >  #include <tpm-v1.h>
> >  #include <tpm-v2.h>
> > @@ -265,12 +266,17 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
> >                 return -ENOSYS;
> >  }
> >
> > -int tpm_get_random(struct udevice *dev, void *data, u32 count)
> > +int tpm_get_random(struct udevice *dev, void *data, size_t count)
> >  {
> > +       int ret = -ENOSYS;
> > +
> >         if (tpm_is_v1(dev))
> > -               return tpm1_get_random(dev, data, count);
> > +               ret = tpm1_get_random(dev, data, count);
> >         else if (tpm_is_v2(dev))
> > -               return -ENOSYS; /* not implemented yet */
> > -       else
> > -               return -ENOSYS;
> > +               ret = tpm2_get_random(dev, data, count);
> > +
> > +       if (ret)
> > +               log_err("Failed to read random bytes\n");
>
> I don't see this in the other functions in this file. Why add it? It
> just bloats the code and there is no way for the caller to suppress
> the message.

Okay

-sughosh

[1] - https://lists.denx.de/pipermail/u-boot/2022-February/476085.html

Re: [PATCH v5 4/9] tpm: Add the RNG child device

[Sughosh Ganu]

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

Re: [PATCH v5 7/9] cmd: rng: Use a statically allocated array for random bytes

[Simon Glass]

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

[..]

Re: [PATCH v5 4/9] tpm: Add the RNG child device

[Simon Glass]

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

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

[Simon Glass]

Hi Sughosh,

On Mon, 14 Mar 2022 at 05:39, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Mon, 14 Mar 2022 at 03:53, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Sun, 13 Mar 2022 at 08:48, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > The TPM device has a builtin random number generator(RNG)
> > > functionality. Expose the RNG functions of the TPM device to the
> > > driver model so that they can be used by the EFI_RNG_PROTOCOL if the
> > > protocol is installed.
> > >
> > > Also change the function arguments and return type of the random
> > > number functions to comply with the driver model api.
> >
> > Please don't do that
> >
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > >
> > > Changes since V4:
> > >
> > > * Call the existing tpm_get_random API function from the TPM RNG
> > >   driver, instead of the tpm{1,2}_get_random API's
> > > * Introduce a new Kconfig symbol TPM_RNG and build the corresponding
> > >   driver if the symbol is enabled
> > > * Change the last parameter of the tpm_get_random API to have a data
> > >   type of size_t instead of u32 to comply with the RNG driver model
> > >   API
> > >
> > >  drivers/rng/Kconfig   |  7 +++++++
> > >  drivers/rng/Makefile  |  1 +
> > >  drivers/rng/tpm_rng.c | 23 +++++++++++++++++++++++
> > >  include/tpm-v1.h      |  4 ++--
> > >  include/tpm-v2.h      |  4 ++--
> > >  include/tpm_api.h     |  2 +-
> > >  lib/Kconfig           |  1 +
> > >  lib/tpm-v1.c          | 16 +++++++++-------
> > >  lib/tpm-v2.c          |  9 +++++----
> > >  lib/tpm_api.c         | 16 +++++++++++-----
> > >  10 files changed, 62 insertions(+), 21 deletions(-)
> > >  create mode 100644 drivers/rng/tpm_rng.c
> > >
> > > diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
> > > index b1c5ab93d1..a89fa99ffa 100644
> > > --- a/drivers/rng/Kconfig
> > > +++ b/drivers/rng/Kconfig
> > > @@ -49,4 +49,11 @@ config RNG_IPROC200
> > >         depends on DM_RNG
> > >         help
> > >           Enable random number generator for RPI4.
> > > +
> > > +config TPM_RNG
> > > +       bool "Enable random number generator on TPM device"
> > > +       depends on TPM
> > > +       default y
> > > +       help
> > > +         Enable random number generator on TPM devices
> >
> > Needs 3 lines of text so please add more detail
>
> Okay
>
> >
> > >  endif
> > > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> > > index 39f7ee3f03..a21f3353ea 100644
> > > --- a/drivers/rng/Makefile
> > > +++ b/drivers/rng/Makefile
> > > @@ -10,3 +10,4 @@ obj-$(CONFIG_RNG_MSM) += msm_rng.o
> > >  obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
> > >  obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o
> > >  obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o
> > > +obj-$(CONFIG_TPM_RNG) += tpm_rng.o
> > > diff --git a/drivers/rng/tpm_rng.c b/drivers/rng/tpm_rng.c
> > > new file mode 100644
> > > index 0000000000..69b41dbbf5
> > > --- /dev/null
> > > +++ b/drivers/rng/tpm_rng.c
> > > @@ -0,0 +1,23 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * Copyright (c) 2022, Linaro Limited
> > > + */
> > > +
> > > +#include <dm.h>
> > > +#include <rng.h>
> > > +#include <tpm_api.h>
> > > +
> > > +static int rng_tpm_random_read(struct udevice *dev, void *data, size_t count)
> > > +{
> > > +       return tpm_get_random(dev->parent, data, count);
> >
> > dev_get_parent(dev)
> >
> > Here you should check the return value and decide whether to return an
> > error, such as -EIO
> >
> > > +}
> > > +
> > > +static const struct dm_rng_ops tpm_rng_ops = {
> > > +       .read = rng_tpm_random_read,
> > > +};
> > > +
> > > +U_BOOT_DRIVER(tpm_rng) = {
> > > +       .name   = "tpm-rng",
> > > +       .id     = UCLASS_RNG,
> > > +       .ops    = &tpm_rng_ops,
> > > +};
> > > diff --git a/include/tpm-v1.h b/include/tpm-v1.h
> > > index 33d53fb695..d2ff8b446d 100644
> > > --- a/include/tpm-v1.h
> > > +++ b/include/tpm-v1.h
> > > @@ -555,9 +555,9 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
> > >   * @param dev          TPM device
> > >   * @param data         output buffer for the random bytes
> > >   * @param count                size of output buffer
> > > - * Return: return code of the operation
> > > + * Return: 0 if OK, -ve on error
> > >   */
> > > -u32 tpm1_get_random(struct udevice *dev, void *data, u32 count);
> > > +int tpm1_get_random(struct udevice *dev, void *data, size_t count);
> >
> > I think I mentioned this last time, but you should not change these
>
> Can you please explain in a little more detail why? I have mentioned
> in my earlier email that I am changing this to bring it in line with
> the rest of the rng drivers which use a size_t data type for the
> number of random bytes to read. What is the issue in changing the
> signature? In any case, currently, the api is not being called from
> any other module, so it is not like changing the signature is breaking
> some code.

Every other function in the TPM API uses u32 as the return value, so
it is odd to change this. It also isn't necessary, as I have
explained. We should aim for consistency.

>
> >
> > >
> > >  /**
> > >   * tpm_finalise_physical_presence() - Finalise physical presence
> > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > > index e79c90b939..4fb1e7a948 100644
> > > --- a/include/tpm-v2.h
> > > +++ b/include/tpm-v2.h
> > > @@ -619,9 +619,9 @@ u32 tpm2_pcr_setauthvalue(struct udevice *dev, const char *pw,
> > >   * @param data         output buffer for the random bytes
> > >   * @param count                size of output buffer
> > >   *
> > > - * Return: return code of the operation
> > > + * Return: 0 if OK, -ve on error
> > >   */
> > > -u32 tpm2_get_random(struct udevice *dev, void *data, u32 count);
> > > +int tpm2_get_random(struct udevice *dev, void *data, size_t count);
> > >
> > >  /**
> > >   * Lock data in the TPM
> > > diff --git a/include/tpm_api.h b/include/tpm_api.h
> > > index 4d77a49719..6d9214b335 100644
> > > --- a/include/tpm_api.h
> > > +++ b/include/tpm_api.h
> > > @@ -276,7 +276,7 @@ u32 tpm_find_key_sha1(struct udevice *dev, const u8 auth[20],
> > >   * @param count                size of output buffer
> > >   * Return: 0 if OK, -ve on error
> > >   */
> > > -int tpm_get_random(struct udevice *dev, void *data, u32 count);
> > > +int tpm_get_random(struct udevice *dev, void *data, size_t count);
> > >

[..]

Regards,
Simon

Re: [PATCH v5 4/9] tpm: Add the RNG child device

[Ilias Apalodimas]

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

Re: [PATCH v5 4/9] tpm: Add the RNG child device

[Sughosh Ganu]

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

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

[Sughosh Ganu]

hi Simon,

On Mon, 14 Mar 2022 at 23:55, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Mon, 14 Mar 2022 at 05:39, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Mon, 14 Mar 2022 at 03:53, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Sun, 13 Mar 2022 at 08:48, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > The TPM device has a builtin random number generator(RNG)
> > > > functionality. Expose the RNG functions of the TPM device to the
> > > > driver model so that they can be used by the EFI_RNG_PROTOCOL if the
> > > > protocol is installed.
> > > >
> > > > Also change the function arguments and return type of the random
> > > > number functions to comply with the driver model api.
> > >
> > > Please don't do that
> > >
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > ---
> > > >
> > > > Changes since V4:
> > > >
> > > > * Call the existing tpm_get_random API function from the TPM RNG
> > > >   driver, instead of the tpm{1,2}_get_random API's
> > > > * Introduce a new Kconfig symbol TPM_RNG and build the corresponding
> > > >   driver if the symbol is enabled
> > > > * Change the last parameter of the tpm_get_random API to have a data
> > > >   type of size_t instead of u32 to comply with the RNG driver model
> > > >   API
> > > >
> > > >  drivers/rng/Kconfig   |  7 +++++++
> > > >  drivers/rng/Makefile  |  1 +
> > > >  drivers/rng/tpm_rng.c | 23 +++++++++++++++++++++++
> > > >  include/tpm-v1.h      |  4 ++--
> > > >  include/tpm-v2.h      |  4 ++--
> > > >  include/tpm_api.h     |  2 +-
> > > >  lib/Kconfig           |  1 +
> > > >  lib/tpm-v1.c          | 16 +++++++++-------
> > > >  lib/tpm-v2.c          |  9 +++++----
> > > >  lib/tpm_api.c         | 16 +++++++++++-----
> > > >  10 files changed, 62 insertions(+), 21 deletions(-)
> > > >  create mode 100644 drivers/rng/tpm_rng.c
> > > >
> > > > diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
> > > > index b1c5ab93d1..a89fa99ffa 100644
> > > > --- a/drivers/rng/Kconfig
> > > > +++ b/drivers/rng/Kconfig
> > > > @@ -49,4 +49,11 @@ config RNG_IPROC200
> > > >         depends on DM_RNG
> > > >         help
> > > >           Enable random number generator for RPI4.
> > > > +
> > > > +config TPM_RNG
> > > > +       bool "Enable random number generator on TPM device"
> > > > +       depends on TPM
> > > > +       default y
> > > > +       help
> > > > +         Enable random number generator on TPM devices
> > >
> > > Needs 3 lines of text so please add more detail
> >
> > Okay
> >
> > >
> > > >  endif
> > > > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> > > > index 39f7ee3f03..a21f3353ea 100644
> > > > --- a/drivers/rng/Makefile
> > > > +++ b/drivers/rng/Makefile
> > > > @@ -10,3 +10,4 @@ obj-$(CONFIG_RNG_MSM) += msm_rng.o
> > > >  obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
> > > >  obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o
> > > >  obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o
> > > > +obj-$(CONFIG_TPM_RNG) += tpm_rng.o
> > > > diff --git a/drivers/rng/tpm_rng.c b/drivers/rng/tpm_rng.c
> > > > new file mode 100644
> > > > index 0000000000..69b41dbbf5
> > > > --- /dev/null
> > > > +++ b/drivers/rng/tpm_rng.c
> > > > @@ -0,0 +1,23 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > +/*
> > > > + * Copyright (c) 2022, Linaro Limited
> > > > + */
> > > > +
> > > > +#include <dm.h>
> > > > +#include <rng.h>
> > > > +#include <tpm_api.h>
> > > > +
> > > > +static int rng_tpm_random_read(struct udevice *dev, void *data, size_t count)
> > > > +{
> > > > +       return tpm_get_random(dev->parent, data, count);
> > >
> > > dev_get_parent(dev)
> > >
> > > Here you should check the return value and decide whether to return an
> > > error, such as -EIO
> > >
> > > > +}
> > > > +
> > > > +static const struct dm_rng_ops tpm_rng_ops = {
> > > > +       .read = rng_tpm_random_read,
> > > > +};
> > > > +
> > > > +U_BOOT_DRIVER(tpm_rng) = {
> > > > +       .name   = "tpm-rng",
> > > > +       .id     = UCLASS_RNG,
> > > > +       .ops    = &tpm_rng_ops,
> > > > +};
> > > > diff --git a/include/tpm-v1.h b/include/tpm-v1.h
> > > > index 33d53fb695..d2ff8b446d 100644
> > > > --- a/include/tpm-v1.h
> > > > +++ b/include/tpm-v1.h
> > > > @@ -555,9 +555,9 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
> > > >   * @param dev          TPM device
> > > >   * @param data         output buffer for the random bytes
> > > >   * @param count                size of output buffer
> > > > - * Return: return code of the operation
> > > > + * Return: 0 if OK, -ve on error
> > > >   */
> > > > -u32 tpm1_get_random(struct udevice *dev, void *data, u32 count);
> > > > +int tpm1_get_random(struct udevice *dev, void *data, size_t count);
> > >
> > > I think I mentioned this last time, but you should not change these
> >
> > Can you please explain in a little more detail why? I have mentioned
> > in my earlier email that I am changing this to bring it in line with
> > the rest of the rng drivers which use a size_t data type for the
> > number of random bytes to read. What is the issue in changing the
> > signature? In any case, currently, the api is not being called from
> > any other module, so it is not like changing the signature is breaking
> > some code.
>
> Every other function in the TPM API uses u32 as the return value, so
> it is odd to change this. It also isn't necessary, as I have
> explained. We should aim for consistency.

Well, you have given a R-b on another patch of this series[1] which is
doing exactly the same thing. I really don't understand what is wrong
in bringing the signature of the random byte generation functions in
the TPM API in line with the RNG driver model. But I will not argue
any more on this and revert back the signatures in my next version.
Thanks.

-sughosh

[1] - https://lists.denx.de/pipermail/u-boot/2022-March/476519.html

>
> >
> > >
> > > >
> > > >  /**
> > > >   * tpm_finalise_physical_presence() - Finalise physical presence
> > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > > > index e79c90b939..4fb1e7a948 100644
> > > > --- a/include/tpm-v2.h
> > > > +++ b/include/tpm-v2.h
> > > > @@ -619,9 +619,9 @@ u32 tpm2_pcr_setauthvalue(struct udevice *dev, const char *pw,
> > > >   * @param data         output buffer for the random bytes
> > > >   * @param count                size of output buffer
> > > >   *
> > > > - * Return: return code of the operation
> > > > + * Return: 0 if OK, -ve on error
> > > >   */
> > > > -u32 tpm2_get_random(struct udevice *dev, void *data, u32 count);
> > > > +int tpm2_get_random(struct udevice *dev, void *data, size_t count);
> > > >
> > > >  /**
> > > >   * Lock data in the TPM
> > > > diff --git a/include/tpm_api.h b/include/tpm_api.h
> > > > index 4d77a49719..6d9214b335 100644
> > > > --- a/include/tpm_api.h
> > > > +++ b/include/tpm_api.h
> > > > @@ -276,7 +276,7 @@ u32 tpm_find_key_sha1(struct udevice *dev, const u8 auth[20],
> > > >   * @param count                size of output buffer
> > > >   * Return: 0 if OK, -ve on error
> > > >   */
> > > > -int tpm_get_random(struct udevice *dev, void *data, u32 count);
> > > > +int tpm_get_random(struct udevice *dev, void *data, size_t count);
> > > >
>
> [..]
>
> Regards,
> Simon

Re: [PATCH v5 4/9] tpm: Add the RNG child device

[Simon Glass]

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

Re: [PATCH v5 4/9] tpm: Add the RNG child device

[Sughosh Ganu]

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

Re: [PATCH v5 6/9] cmd: rng: Add support for selecting RNG device

[Sughosh Ganu]

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

Re: [PATCH v5 7/9] cmd: rng: Use a statically allocated array for random bytes

[Sughosh Ganu]

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
>

Re: [PATCH v5 8/9] doc: rng: Add documentation for the rng command

[Sughosh Ganu]

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
>

Re: [PATCH v5 9/9] test: rng: Add a UT testcase for the rng command

[Sughosh Ganu]

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
>

Re: [PATCH v5 5/9] qemu: arm: Remove platform specific function to get RNG device

[Sughosh Ganu]

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
>

Re: [PATCH v5 5/9] qemu: arm: Remove platform specific function to get RNG device

[Heinrich Schuchardt]

On 3/24/22 17:44, Sughosh Ganu wrote:
> 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
>

Hello Sughosh,

I have added the patch to my queue.

Best regards

Heinrich

Re: [PATCH v5 4/9] tpm: Add the RNG child device

[Simon Glass]

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

Re: [PATCH v5 4/9] tpm: Add the RNG child device

[Ilias Apalodimas]

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