[PATCH v2 00/10] tpm: rng: Move TPM RNG functionality to driver model

[PATCH v2 00/10] 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.

Changes since V1:
* Added existing copyrights for the rng functions taken from the tpm
  library routines
* Return -EIO for TPM command returning an error
* Simplify the logic in tpm_get_random based on the review comments
  from Ilias
* Changed the help text to show order of the parameters passed
  to the rng command, based on review comment from Heinrich


[1] - https://lore.kernel.org/u-boot/20220103120738.47835-1-ilias.apalodimas@linaro.org/


Sughosh Ganu (10):
  tpm: Move tpm-utils header under the include directory
  tpm: rng: Change tpm_get_random to return an int
  tpm: Fix the return type of tpm_startup
  tpm: Move the TPM version detection functions to the uclass driver
  configs: gazerbeam: Build TPMV2 library routines
  configs: chromebook_coral: Build TPMV1 library routines
  tpm: rng: Move the TPM RNG functionality to driver model
  tpm: Add the RNG child device
  qemu: arm: Remove platform specific function to get RNG device
  cmd: rng: Add support for selecting RNG device

 board/emulation/qemu-arm/qemu-arm.c | 42 --------------
 cmd/rng.c                           | 31 +++++++---
 configs/chromebook_coral_defconfig  |  1 -
 configs/gazerbeam_defconfig         |  1 -
 drivers/rng/Makefile                |  1 +
 drivers/rng/tpm1_rng.c              | 87 +++++++++++++++++++++++++++++
 drivers/rng/tpm2_rng.c              | 86 ++++++++++++++++++++++++++++
 drivers/tpm/tpm-uclass.c            | 69 +++++++++++++++++++++--
 {lib => include}/tpm-utils.h        |  0
 include/tpm_api.h                   | 26 ++++++++-
 lib/tpm-common.c                    |  2 +-
 lib/tpm-v1.c                        | 46 +--------------
 lib/tpm-v2.c                        | 46 +--------------
 lib/tpm_api.c                       | 37 ++++++------
 14 files changed, 309 insertions(+), 166 deletions(-)
 create mode 100644 drivers/rng/tpm1_rng.c
 create mode 100644 drivers/rng/tpm2_rng.c
 rename {lib => include}/tpm-utils.h (100%)

-- 
2.25.1

[PATCH v2 01/10] tpm: Move tpm-utils header under the include directory

[Sughosh Ganu]

The random number generation functions of TPM will be moved under a
dedicated driver. With this, the function declarations along with
some other relevant macro definitions need to be moved under a
common header file directory. Move the tpm-utils.h header file under
the common include directory.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---

Changes since V1: None

 {lib => include}/tpm-utils.h | 0
 lib/tpm-common.c             | 2 +-
 lib/tpm-v1.c                 | 2 +-
 lib/tpm-v2.c                 | 2 +-
 4 files changed, 3 insertions(+), 3 deletions(-)
 rename {lib => include}/tpm-utils.h (100%)

diff --git a/lib/tpm-utils.h b/include/tpm-utils.h
similarity index 100%
rename from lib/tpm-utils.h
rename to include/tpm-utils.h
diff --git a/lib/tpm-common.c b/lib/tpm-common.c
index 82ffdc5341..26506f0b99 100644
--- a/lib/tpm-common.c
+++ b/lib/tpm-common.c
@@ -11,7 +11,7 @@
 #include <log.h>
 #include <asm/unaligned.h>
 #include <tpm-common.h>
-#include "tpm-utils.h"
+#include <tpm-utils.h>
 
 enum tpm_version tpm_get_version(struct udevice *dev)
 {
diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
index 22a769c587..467992e04e 100644
--- a/lib/tpm-v1.c
+++ b/lib/tpm-v1.c
@@ -13,7 +13,7 @@
 #include <u-boot/sha1.h>
 #include <tpm-common.h>
 #include <tpm-v1.h>
-#include "tpm-utils.h"
+#include <tpm-utils.h>
 
 #ifdef CONFIG_TPM_AUTH_SESSIONS
 
diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
index 1bf627853a..2f16b0007b 100644
--- a/lib/tpm-v2.c
+++ b/lib/tpm-v2.c
@@ -9,7 +9,7 @@
 #include <tpm-common.h>
 #include <tpm-v2.h>
 #include <linux/bitops.h>
-#include "tpm-utils.h"
+#include <tpm-utils.h>
 
 u32 tpm2_startup(struct udevice *dev, enum tpm2_startup_types mode)
 {
-- 
2.25.1

[PATCH v2 02/10] 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>
---

Changes since V1: 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 ef45b43a8f..b9e3e8b5e6 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 4c662640a9..1bbe33a3fc 100644
--- a/lib/tpm_api.c
+++ b/lib/tpm_api.c
@@ -274,7 +274,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 (is_tpm1(dev))
 		return tpm1_get_random(dev, data, count);
-- 
2.25.1

[PATCH v2 03/10] 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>
---

Changes since V1: None

 include/tpm_api.h | 2 +-
 lib/tpm_api.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/tpm_api.h b/include/tpm_api.h
index b9e3e8b5e6..fb6ee14e23 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 1bbe33a3fc..b762202866 100644
--- a/lib/tpm_api.c
+++ b/lib/tpm_api.c
@@ -21,7 +21,7 @@ static bool is_tpm2(struct udevice *dev)
 	return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
 }
 
-u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
+int tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
 {
 	if (is_tpm1(dev)) {
 		return tpm1_startup(dev, mode);
-- 
2.25.1

[PATCH v2 04/10] tpm: Move the TPM version detection functions to the uclass driver

[Sughosh Ganu]

Make the TPM version detection functions as external symbols and move
them to the TPM uclass driver. These are useful functions to check the
TPM device version and should not be static functions.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---

Changes since V1: None

 drivers/tpm/tpm-uclass.c | 11 +++++++++++
 include/tpm_api.h        | 20 ++++++++++++++++++++
 lib/tpm_api.c            | 10 ----------
 3 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
index f67fe1019b..8619da89d8 100644
--- a/drivers/tpm/tpm-uclass.c
+++ b/drivers/tpm/tpm-uclass.c
@@ -11,10 +11,21 @@
 #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"
 
+bool is_tpm1(struct udevice *dev)
+{
+	return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
+}
+
+bool is_tpm2(struct udevice *dev)
+{
+	return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
+}
+
 int tpm_open(struct udevice *dev)
 {
 	struct tpm_ops *ops = tpm_get_ops(dev);
diff --git a/include/tpm_api.h b/include/tpm_api.h
index fb6ee14e23..c19639a688 100644
--- a/include/tpm_api.h
+++ b/include/tpm_api.h
@@ -11,6 +11,26 @@
 #include <tpm-v1.h>
 #include <tpm-v2.h>
 
+/**
+ * is_tpm1() - Check if it is a tpmv1 device
+ * @param dev		TPM device
+ *
+ * Check if the TPM device is a TPMv1 device
+ *
+ * Return: 1 if TPMv1, 0 otherwise
+ */
+bool is_tpm1(struct udevice *dev);
+
+/**
+ * is_tpm2() - Check if it is a tpmv2 device
+ * @param dev		TPM device
+ *
+ * Check if the TPM device is a TPMv2 device
+ *
+ * Return: 1 if TPMv2, 0 otherwise
+ */
+bool is_tpm2(struct udevice *dev);
+
 /**
  * Issue a TPM_Startup command.
  *
diff --git a/lib/tpm_api.c b/lib/tpm_api.c
index b762202866..9dd9606fa8 100644
--- a/lib/tpm_api.c
+++ b/lib/tpm_api.c
@@ -11,16 +11,6 @@
 #include <tpm-v2.h>
 #include <tpm_api.h>
 
-static bool is_tpm1(struct udevice *dev)
-{
-	return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
-}
-
-static bool is_tpm2(struct udevice *dev)
-{
-	return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
-}
-
 int tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
 {
 	if (is_tpm1(dev)) {
-- 
2.25.1

[PATCH v2 05/10] configs: gazerbeam: Build TPMV2 library routines

[Sughosh Ganu]

The TPM code currently does a runtime detection of the TPM version and
calls appropriate functions. Gazerbeam is one of the platforms where
the TPMV2 code is disabled at build time. With this, calling TPM api's
from the TPM uclass driver results in link errors. Enable TPMV2
library routines and determine the TPM version at runtime like other
platforms.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---

Changes since V1: None

 configs/gazerbeam_defconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configs/gazerbeam_defconfig b/configs/gazerbeam_defconfig
index 199afb4d16..b18194aa0f 100644
--- a/configs/gazerbeam_defconfig
+++ b/configs/gazerbeam_defconfig
@@ -213,7 +213,6 @@ CONFIG_TIMER=y
 CONFIG_MPC83XX_TIMER=y
 CONFIG_TPM_ATMEL_TWI=y
 CONFIG_TPM_AUTH_SESSIONS=y
-# CONFIG_TPM_V2 is not set
 CONFIG_DM_VIDEO=y
 CONFIG_DISPLAY=y
 CONFIG_LOGICORE_DP_TX=y
-- 
2.25.1

[PATCH v2 06/10] configs: chromebook_coral: Build TPMV1 library routines

[Sughosh Ganu]

The TPM code currently does a runtime detection of the TPM version and
calls appropriate functions. Chromebook Coral is one of the platforms
where the TPMV1 code is disabled at build time. With this, calling TPM
api's from the TPM uclass driver results in link errors. Enable TPMV1
library routines and determine the TPM version at runtime like other
platforms.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---

Changes since V1: None

 configs/chromebook_coral_defconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configs/chromebook_coral_defconfig b/configs/chromebook_coral_defconfig
index 0cd8f39aa3..4704ce25c8 100644
--- a/configs/chromebook_coral_defconfig
+++ b/configs/chromebook_coral_defconfig
@@ -104,7 +104,6 @@ CONFIG_SPI=y
 CONFIG_ICH_SPI=y
 # CONFIG_SYSINFO_SMBIOS is not set
 CONFIG_TPL_SYSRESET=y
-# CONFIG_TPM_V1 is not set
 CONFIG_TPM2_CR50_I2C=y
 CONFIG_USB_XHCI_HCD=y
 CONFIG_USB_STORAGE=y
-- 
2.25.1

[PATCH v2 07/10] tpm: rng: Move the TPM RNG functionality to driver model

[Sughosh Ganu]

Currently, the TPM random number generator(RNG) functions are defined
as part of the library functions under the corresponding tpm files for
tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers
complying with the driver model.

Also make changes to the tpm_get_random function to have it call the
TPM RNG driver functions instead of the library functions.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---

Changes since V1:
* Added existing copyrights for the rng functions taken from the tpm
  library routines
* Return -EIO for TPM command returning an error
* Simplify the logic in tpm_get_random based on the review comments
  from Ilias

 drivers/rng/Makefile   |  1 +
 drivers/rng/tpm1_rng.c | 87 ++++++++++++++++++++++++++++++++++++++++++
 drivers/rng/tpm2_rng.c | 86 +++++++++++++++++++++++++++++++++++++++++
 lib/tpm-v1.c           | 44 ---------------------
 lib/tpm-v2.c           | 44 ---------------------
 lib/tpm_api.c          | 23 +++++++++--
 6 files changed, 193 insertions(+), 92 deletions(-)
 create mode 100644 drivers/rng/tpm1_rng.c
 create mode 100644 drivers/rng/tpm2_rng.c

diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
index 39f7ee3f03..129cfbd006 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) += tpm1_rng.o tpm2_rng.o
diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c
new file mode 100644
index 0000000000..7e629756b3
--- /dev/null
+++ b/drivers/rng/tpm1_rng.c
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2013 The Chromium OS Authors.
+ * Coypright (c) 2013 Guntermann & Drunck GmbH
+ * Copyright (c) 2022, Linaro Limited
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <rng.h>
+#include <tpm-utils.h>
+#include <tpm-v1.h>
+
+#include <linux/errno.h>
+
+#define TPM_HEADER_SIZE		10
+
+#define TPMV1_DATA_OFFSET	14
+
+/**
+ * tpm1_rng_read() - Read the random bytes from TPMv1 device
+ * @param dev		TPMv1 RNG device
+ * @param data		data buffer to write random bytes
+ * @param count		number of random bytes to read from
+ *                      the device
+ *
+ * Function to read the random bytes from the RNG pseudo device
+ * built into the TPMv1 device. Reads 'count' number of bytes
+ * from the random number generator and copies them into the
+ * 'data' buffer.
+ *
+ * Return: 0 if OK, -ve on error.
+ *
+ */
+static int tpm1_rng_read(struct udevice *dev, void *data, size_t count)
+{
+	const u8 command[14] = {
+		0x0, 0xc1,		/* TPM_TAG */
+		0x0, 0x0, 0x0, 0xe,	/* parameter size */
+		0x0, 0x0, 0x0, 0x46,	/* TPM_COMMAND_CODE */
+	};
+	const size_t length_offset = TPM_HEADER_SIZE;
+	const size_t data_size_offset = TPM_HEADER_SIZE;
+	const size_t data_offset = TPMV1_DATA_OFFSET;
+	u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
+	size_t response_length = sizeof(response);
+	u32 data_size;
+	u8 *out = data;
+
+	while (count > 0) {
+		u32 this_bytes = min(count,
+				     sizeof(response) - data_offset);
+		u32 err;
+
+		if (pack_byte_string(buf, sizeof(buf), "sd",
+				     0, command, sizeof(command),
+				     length_offset, this_bytes))
+			return -EIO;
+		err = tpm_sendrecv_command(dev->parent, buf, response,
+					   &response_length);
+		if (err)
+			return err;
+		if (unpack_byte_string(response, response_length, "d",
+				       data_size_offset, &data_size))
+			return -EIO;
+		if (data_size > count)
+			return TPM_LIB_ERROR;
+		if (unpack_byte_string(response, response_length, "s",
+				       data_offset, out, data_size))
+			return -EIO;
+
+		count -= data_size;
+		out += data_size;
+	}
+
+	return 0;
+}
+
+static const struct dm_rng_ops tpm1_rng_ops = {
+	.read = tpm1_rng_read,
+};
+
+U_BOOT_DRIVER(tpm1_rng) = {
+	.name		= "tpm1-rng",
+	.id		= UCLASS_RNG,
+	.ops		= &tpm1_rng_ops,
+};
diff --git a/drivers/rng/tpm2_rng.c b/drivers/rng/tpm2_rng.c
new file mode 100644
index 0000000000..e7f6be46e4
--- /dev/null
+++ b/drivers/rng/tpm2_rng.c
@@ -0,0 +1,86 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2018 Bootlin
+ * Copyright (c) 2022, Linaro Limited
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <rng.h>
+#include <tpm-utils.h>
+#include <tpm-v2.h>
+
+#include <linux/errno.h>
+
+#define TPM_HEADER_SIZE		10
+
+#define TPMV2_DATA_OFFSET	12
+
+/**
+ * tpm2_rng_read() - Read the random bytes from TPMv2 device
+ * @param dev		TPMv2 RNG device
+ * @param data		data buffer to write random bytes
+ * @param count		number of random bytes to read from
+ *                      the device
+ *
+ * Function to read the random bytes from the RNG pseudo device
+ * built into the TPMv2 device. Reads 'count' number of bytes
+ * from the random number generator and copies them into the
+ * 'data' buffer.
+ *
+ * Return: 0 if OK, -ve on error.
+ *
+ */
+static int tpm2_rng_read(struct udevice *dev, void *data, size_t count)
+{
+	const u8 command_v2[10] = {
+		tpm_u16(TPM2_ST_NO_SESSIONS),
+		tpm_u32(12),
+		tpm_u32(TPM2_CC_GET_RANDOM),
+	};
+	u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
+
+	const size_t data_size_offset = TPM_HEADER_SIZE;
+	const size_t data_offset = TPMV2_DATA_OFFSET;
+	size_t response_length = sizeof(response);
+	u32 data_size;
+	u8 *out = data;
+
+	while (count > 0) {
+		u32 this_bytes = min((size_t)count,
+				     sizeof(response) - data_offset);
+		u32 err;
+
+		if (pack_byte_string(buf, sizeof(buf), "sw",
+				     0, command_v2, sizeof(command_v2),
+				     sizeof(command_v2), this_bytes))
+			return -EIO;
+		err = tpm_sendrecv_command(dev->parent, buf, response,
+					   &response_length);
+		if (err)
+			return err;
+		if (unpack_byte_string(response, response_length, "w",
+				       data_size_offset, &data_size))
+			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 -EIO;
+
+		count -= data_size;
+		out += data_size;
+	}
+
+	return 0;
+}
+
+static const struct dm_rng_ops tpm2_rng_ops = {
+	.read = tpm2_rng_read,
+};
+
+U_BOOT_DRIVER(tpm2_rng) = {
+	.name		= "tpm2-rng",
+	.id		= UCLASS_RNG,
+	.ops		= &tpm2_rng_ops,
+};
diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
index 467992e04e..71cc90a2ab 100644
--- a/lib/tpm-v1.c
+++ b/lib/tpm-v1.c
@@ -868,47 +868,3 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
 #endif /* CONFIG_TPM_LOAD_KEY_BY_SHA1 */
 
 #endif /* CONFIG_TPM_AUTH_SESSIONS */
-
-u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
-{
-	const u8 command[14] = {
-		0x0, 0xc1,		/* TPM_TAG */
-		0x0, 0x0, 0x0, 0xe,	/* parameter size */
-		0x0, 0x0, 0x0, 0x46,	/* TPM_COMMAND_CODE */
-	};
-	const size_t length_offset = 10;
-	const size_t data_size_offset = 10;
-	const size_t data_offset = 14;
-	u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
-	size_t response_length = sizeof(response);
-	u32 data_size;
-	u8 *out = data;
-
-	while (count > 0) {
-		u32 this_bytes = min((size_t)count,
-				     sizeof(response) - data_offset);
-		u32 err;
-
-		if (pack_byte_string(buf, sizeof(buf), "sd",
-				     0, command, sizeof(command),
-				     length_offset, this_bytes))
-			return TPM_LIB_ERROR;
-		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;
-		if (data_size > count)
-			return TPM_LIB_ERROR;
-		if (unpack_byte_string(response, response_length, "s",
-				       data_offset, out, data_size))
-			return TPM_LIB_ERROR;
-
-		count -= data_size;
-		out += data_size;
-	}
-
-	return 0;
-}
diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
index 2f16b0007b..c1456c1974 100644
--- a/lib/tpm-v2.c
+++ b/lib/tpm-v2.c
@@ -562,50 +562,6 @@ 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)
-{
-	const u8 command_v2[10] = {
-		tpm_u16(TPM2_ST_NO_SESSIONS),
-		tpm_u32(12),
-		tpm_u32(TPM2_CC_GET_RANDOM),
-	};
-	u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
-
-	const size_t data_size_offset = 10;
-	const size_t data_offset = 12;
-	size_t response_length = sizeof(response);
-	u32 data_size;
-	u8 *out = data;
-
-	while (count > 0) {
-		u32 this_bytes = min((size_t)count,
-				     sizeof(response) - data_offset);
-		u32 err;
-
-		if (pack_byte_string(buf, sizeof(buf), "sw",
-				     0, command_v2, sizeof(command_v2),
-				     sizeof(command_v2), this_bytes))
-			return TPM_LIB_ERROR;
-		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;
-		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;
-
-		count -= data_size;
-		out += data_size;
-	}
-
-	return 0;
-}
-
 u32 tpm2_write_lock(struct udevice *dev, u32 index)
 {
 	u8 command_v2[COMMAND_BUFFER_SIZE] = {
diff --git a/lib/tpm_api.c b/lib/tpm_api.c
index 9dd9606fa8..5492f89959 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>
@@ -264,12 +265,26 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
 		return -ENOSYS;
 }
 
+#if CONFIG_IS_ENABLED(DM_RNG)
 int tpm_get_random(struct udevice *dev, void *data, u32 count)
 {
+	int ret = -ENOSYS;
+	struct udevice *rng_dev;
+
 	if (is_tpm1(dev))
-		return tpm1_get_random(dev, data, count);
+		ret = uclass_get_device_by_driver(UCLASS_RNG,
+						  DM_DRIVER_GET(tpm1_rng),
+						  &rng_dev);
 	else if (is_tpm2(dev))
-		return -ENOSYS; /* not implemented yet */
-	else
-		return -ENOSYS;
+		ret = uclass_get_device_by_driver(UCLASS_RNG,
+						  DM_DRIVER_GET(tpm2_rng),
+						  &rng_dev);
+
+	if (ret) {
+		log_err("Getting tpm rng device failed\n");
+		return ret;
+	}
+
+	return dm_rng_read(rng_dev, data, (size_t)count);
 }
+#endif /* DM_RNG */
-- 
2.25.1

[PATCH v2 08/10] 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 V1: None

 drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 4 deletions(-)

diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
index 8619da89d8..383cc7bc48 100644
--- a/drivers/tpm/tpm-uclass.c
+++ b/drivers/tpm/tpm-uclass.c
@@ -16,6 +16,11 @@
 #include <tpm-v2.h>
 #include "tpm_internal.h"
 
+#include <dm/lists.h>
+
+#define TPM_RNG1_DRV_NAME	"tpm1-rng"
+#define TPM_RNG2_DRV_NAME	"tpm2-rng"
+
 bool is_tpm1(struct udevice *dev)
 {
 	return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
@@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_TPM)
+static int tpm_uclass_post_probe(struct udevice *dev)
+{
+	int ret;
+	const char *drv = is_tpm1(dev) ? TPM_RNG1_DRV_NAME : TPM_RNG2_DRV_NAME;
+	struct udevice *child;
+
+	ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
+	if (ret == -ENOENT) {
+		log_err("No driver configured for tpm-rng device\n");
+		return 0;
+	}
+
+	if (ret) {
+		log_err("Unable to bind rng driver with the tpm-rng device\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tpm_uclass_child_pre_probe(struct udevice *dev)
+{
+	int ret;
+
+	ret = tpm_open(dev->parent);
+	if (ret == -EBUSY) {
+		log_info("TPM device already opened\n");
+	} else if (ret) {
+		log_err("Unable to open TPM device\n");
+		return ret;
+	}
+
+	ret = tpm_startup(dev->parent, TPM_ST_CLEAR);
+	if (ret)
+		log_err("Unable to start TPM device\n");
+
+	return ret;
+}
+#endif /* CONFIG_TPM */
+
 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
+#if IS_ENABLED(CONFIG_TPM)
+	.post_probe		= tpm_uclass_post_probe,
+	.child_pre_probe	= tpm_uclass_child_pre_probe,
 #endif
 	.per_device_auto	= sizeof(struct tpm_chip_priv),
 };
-- 
2.25.1

[PATCH v2 09/10] 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>
---

Changes since V1: 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 v2 10/10] 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>
---

Changes since V1:
* Changed the help text to show order of the parameters passed
  to the rng command, based on review comment from Heinrich

 cmd/rng.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/cmd/rng.c b/cmd/rng.c
index 1ad5a096c0..bb89cfa784 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(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

Re: [PATCH v2 02/10] tpm: rng: Change tpm_get_random to return an int

[Simon Glass]

On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> 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>
> ---
>
> Changes since V1: None
>
>  include/tpm_api.h | 4 ++--
>  lib/tpm_api.c     | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 07/10] tpm: rng: Move the TPM RNG functionality to driver model

[Simon Glass]

Hi Sughosh,

On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> Currently, the TPM random number generator(RNG) functions are defined
> as part of the library functions under the corresponding tpm files for
> tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers
> complying with the driver model.
>
> Also make changes to the tpm_get_random function to have it call the
> TPM RNG driver functions instead of the library functions.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>
> Changes since V1:
> * Added existing copyrights for the rng functions taken from the tpm
>   library routines
> * Return -EIO for TPM command returning an error
> * Simplify the logic in tpm_get_random based on the review comments
>   from Ilias
>
>  drivers/rng/Makefile   |  1 +
>  drivers/rng/tpm1_rng.c | 87 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/rng/tpm2_rng.c | 86 +++++++++++++++++++++++++++++++++++++++++
>  lib/tpm-v1.c           | 44 ---------------------
>  lib/tpm-v2.c           | 44 ---------------------
>  lib/tpm_api.c          | 23 +++++++++--
>  6 files changed, 193 insertions(+), 92 deletions(-)
>  create mode 100644 drivers/rng/tpm1_rng.c
>  create mode 100644 drivers/rng/tpm2_rng.c
>
> diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> index 39f7ee3f03..129cfbd006 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) += tpm1_rng.o tpm2_rng.o
> diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c
> new file mode 100644
> index 0000000000..7e629756b3
> --- /dev/null
> +++ b/drivers/rng/tpm1_rng.c
> @@ -0,0 +1,87 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2013 The Chromium OS Authors.
> + * Coypright (c) 2013 Guntermann & Drunck GmbH
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <rng.h>
> +#include <tpm-utils.h>
> +#include <tpm-v1.h>
> +
> +#include <linux/errno.h>
> +
> +#define TPM_HEADER_SIZE                10
> +
> +#define TPMV1_DATA_OFFSET      14
> +
> +/**
> + * tpm1_rng_read() - Read the random bytes from TPMv1 device
> + * @param dev          TPMv1 RNG device
> + * @param data         data buffer to write random bytes
> + * @param count                number of random bytes to read from
> + *                      the device
> + *
> + * Function to read the random bytes from the RNG pseudo device
> + * built into the TPMv1 device. Reads 'count' number of bytes
> + * from the random number generator and copies them into the
> + * 'data' buffer.
> + *
> + * Return: 0 if OK, -ve on error.
> + *
> + */
> +static int tpm1_rng_read(struct udevice *dev, void *data, size_t count)
> +{
> +       const u8 command[14] = {
> +               0x0, 0xc1,              /* TPM_TAG */
> +               0x0, 0x0, 0x0, 0xe,     /* parameter size */
> +               0x0, 0x0, 0x0, 0x46,    /* TPM_COMMAND_CODE */
> +       };
> +       const size_t length_offset = TPM_HEADER_SIZE;
> +       const size_t data_size_offset = TPM_HEADER_SIZE;
> +       const size_t data_offset = TPMV1_DATA_OFFSET;
> +       u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> +       size_t response_length = sizeof(response);
> +       u32 data_size;
> +       u8 *out = data;
> +

The current model is that all TPM calls are set up in lib/tpm and I
don't think we should change it. You should be able to move these
functions into lib/tpm and add your random_read function to tpm_api.h

Regards,
Simon

Re: [PATCH v2 01/10] tpm: Move tpm-utils header under the include directory

[Simon Glass]

Hi Sughosh,

On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The random number generation functions of TPM will be moved under a
> dedicated driver. With this, the function declarations along with
> some other relevant macro definitions need to be moved under a
> common header file directory. Move the tpm-utils.h header file under
> the common include directory.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>
> Changes since V1: None
>
>  {lib => include}/tpm-utils.h | 0
>  lib/tpm-common.c             | 2 +-
>  lib/tpm-v1.c                 | 2 +-
>  lib/tpm-v2.c                 | 2 +-
>  4 files changed, 3 insertions(+), 3 deletions(-)
>  rename {lib => include}/tpm-utils.h (100%)
>

Having looked at this a few days ago I don't think it is a good idea.
These functions and methods should sit within lib/tpm, which is where
messages are packed and unpacked.

Regards,
Simon

Re: [PATCH v2 03/10] tpm: Fix the return type of tpm_startup

[Simon Glass]

On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> 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>
> ---
>
> Changes since V1: None
>
>  include/tpm_api.h | 2 +-
>  lib/tpm_api.c     | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 04/10] tpm: Move the TPM version detection functions to the uclass driver

[Simon Glass]

On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> Make the TPM version detection functions as external symbols and move
> them to the TPM uclass driver. These are useful functions to check the
> TPM device version and should not be static functions.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>
> Changes since V1: None
>
>  drivers/tpm/tpm-uclass.c | 11 +++++++++++
>  include/tpm_api.h        | 20 ++++++++++++++++++++
>  lib/tpm_api.c            | 10 ----------
>  3 files changed, 31 insertions(+), 10 deletions(-)
>

I just sent a similar patch a few days ago.

> diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> index f67fe1019b..8619da89d8 100644
> --- a/drivers/tpm/tpm-uclass.c
> +++ b/drivers/tpm/tpm-uclass.c
> @@ -11,10 +11,21 @@
>  #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"
>
> +bool is_tpm1(struct udevice *dev)
> +{
> +       return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> +}
> +
> +bool is_tpm2(struct udevice *dev)
> +{
> +       return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
> +}
> +

I think a better name is tpm_is_v1() since this is in the tpm uclass.
It should have a tpm_ prefix.

Regards,
Simon

[..]

Re: [PATCH v2 06/10] configs: chromebook_coral: Build TPMV1 library routines

[Simon Glass]

Hi Sughosh,

On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The TPM code currently does a runtime detection of the TPM version and
> calls appropriate functions. Chromebook Coral is one of the platforms
> where the TPMV1 code is disabled at build time. With this, calling TPM
> api's from the TPM uclass driver results in link errors. Enable TPMV1
> library routines and determine the TPM version at runtime like other
> platforms.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>
> Changes since V1: None
>
>  configs/chromebook_coral_defconfig | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/configs/chromebook_coral_defconfig b/configs/chromebook_coral_defconfig
> index 0cd8f39aa3..4704ce25c8 100644
> --- a/configs/chromebook_coral_defconfig
> +++ b/configs/chromebook_coral_defconfig
> @@ -104,7 +104,6 @@ CONFIG_SPI=y
>  CONFIG_ICH_SPI=y
>  # CONFIG_SYSINFO_SMBIOS is not set
>  CONFIG_TPL_SYSRESET=y
> -# CONFIG_TPM_V1 is not set
>  CONFIG_TPM2_CR50_I2C=y
>  CONFIG_USB_XHCI_HCD=y
>  CONFIG_USB_STORAGE=y
> --
> 2.25.1

This board does not have a v1 TPM so we don't want to waste code space
adding it.

The current code works fine and supports both a build-time and
run-time check. What has gone wrong?

Regards,
Simon

Re: [PATCH v2 08/10] tpm: Add the RNG child device

[Simon Glass]

Hi Sughosh,

On Mon, 28 Feb 2022 at 05:07, 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 V1: None
>
>  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> index 8619da89d8..383cc7bc48 100644
> --- a/drivers/tpm/tpm-uclass.c
> +++ b/drivers/tpm/tpm-uclass.c
> @@ -16,6 +16,11 @@
>  #include <tpm-v2.h>
>  #include "tpm_internal.h"
>
> +#include <dm/lists.h>
> +
> +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> +
>  bool is_tpm1(struct udevice *dev)
>  {
>         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
>         return 0;
>  }
>
> +#if IS_ENABLED(CONFIG_TPM)

This should be in the Makefile so that we only build this file if TPM
is enabled.

> +static int tpm_uclass_post_probe(struct udevice *dev)
> +{
> +       int ret;
> +       const char *drv = is_tpm1(dev) ? TPM_RNG1_DRV_NAME : TPM_RNG2_DRV_NAME;
> +       struct udevice *child;
> +
> +       ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
> +       if (ret == -ENOENT) {
> +               log_err("No driver configured for tpm-rng device\n");
> +               return 0;
> +       }
> +
> +       if (ret) {
> +               log_err("Unable to bind rng driver with the tpm-rng device\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int tpm_uclass_child_pre_probe(struct udevice *dev)
> +{
> +       int ret;
> +
> +       ret = tpm_open(dev->parent);
> +       if (ret == -EBUSY) {
> +               log_info("TPM device already opened\n");
> +       } else if (ret) {
> +               log_err("Unable to open TPM device\n");
> +               return ret;
> +       }
> +
> +       ret = tpm_startup(dev->parent, TPM_ST_CLEAR);
> +       if (ret)
> +               log_err("Unable to start TPM device\n");
> +
> +       return ret;
> +}
> +#endif /* CONFIG_TPM */
> +
>  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
> +#if IS_ENABLED(CONFIG_TPM)
> +       .post_probe             = tpm_uclass_post_probe,
> +       .child_pre_probe        = tpm_uclass_child_pre_probe,
>  #endif
>         .per_device_auto        = sizeof(struct tpm_chip_priv),
>  };
> --
> 2.25.1
>

Regards,
Simon

Re: [PATCH v2 10/10] cmd: rng: Add support for selecting RNG device

[Simon Glass]

Hi Sughosh,

On Mon, 28 Feb 2022 at 05:07, 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>
> ---
>
> Changes since V1:
> * Changed the help text to show order of the parameters passed
>   to the rng command, based on review comment from Heinrich
>
>  cmd/rng.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)

Please add a test for the command and also add doc/usage

Regards,
Simon

Re: [PATCH v2 07/10] tpm: rng: Move the TPM RNG functionality to driver model

[Sughosh Ganu]

hi Simon,

On Tue, 1 Mar 2022 at 20:28, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > Currently, the TPM random number generator(RNG) functions are defined
> > as part of the library functions under the corresponding tpm files for
> > tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers
> > complying with the driver model.
> >
> > Also make changes to the tpm_get_random function to have it call the
> > TPM RNG driver functions instead of the library functions.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >
> > Changes since V1:
> > * Added existing copyrights for the rng functions taken from the tpm
> >   library routines
> > * Return -EIO for TPM command returning an error
> > * Simplify the logic in tpm_get_random based on the review comments
> >   from Ilias
> >
> >  drivers/rng/Makefile   |  1 +
> >  drivers/rng/tpm1_rng.c | 87 ++++++++++++++++++++++++++++++++++++++++++
> >  drivers/rng/tpm2_rng.c | 86 +++++++++++++++++++++++++++++++++++++++++
> >  lib/tpm-v1.c           | 44 ---------------------
> >  lib/tpm-v2.c           | 44 ---------------------
> >  lib/tpm_api.c          | 23 +++++++++--
> >  6 files changed, 193 insertions(+), 92 deletions(-)
> >  create mode 100644 drivers/rng/tpm1_rng.c
> >  create mode 100644 drivers/rng/tpm2_rng.c
> >
> > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> > index 39f7ee3f03..129cfbd006 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) += tpm1_rng.o tpm2_rng.o
> > diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c
> > new file mode 100644
> > index 0000000000..7e629756b3
> > --- /dev/null
> > +++ b/drivers/rng/tpm1_rng.c
> > @@ -0,0 +1,87 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2013 The Chromium OS Authors.
> > + * Coypright (c) 2013 Guntermann & Drunck GmbH
> > + * Copyright (c) 2022, Linaro Limited
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <rng.h>
> > +#include <tpm-utils.h>
> > +#include <tpm-v1.h>
> > +
> > +#include <linux/errno.h>
> > +
> > +#define TPM_HEADER_SIZE                10
> > +
> > +#define TPMV1_DATA_OFFSET      14
> > +
> > +/**
> > + * tpm1_rng_read() - Read the random bytes from TPMv1 device
> > + * @param dev          TPMv1 RNG device
> > + * @param data         data buffer to write random bytes
> > + * @param count                number of random bytes to read from
> > + *                      the device
> > + *
> > + * Function to read the random bytes from the RNG pseudo device
> > + * built into the TPMv1 device. Reads 'count' number of bytes
> > + * from the random number generator and copies them into the
> > + * 'data' buffer.
> > + *
> > + * Return: 0 if OK, -ve on error.
> > + *
> > + */
> > +static int tpm1_rng_read(struct udevice *dev, void *data, size_t count)
> > +{
> > +       const u8 command[14] = {
> > +               0x0, 0xc1,              /* TPM_TAG */
> > +               0x0, 0x0, 0x0, 0xe,     /* parameter size */
> > +               0x0, 0x0, 0x0, 0x46,    /* TPM_COMMAND_CODE */
> > +       };
> > +       const size_t length_offset = TPM_HEADER_SIZE;
> > +       const size_t data_size_offset = TPM_HEADER_SIZE;
> > +       const size_t data_offset = TPMV1_DATA_OFFSET;
> > +       u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> > +       size_t response_length = sizeof(response);
> > +       u32 data_size;
> > +       u8 *out = data;
> > +
>
> The current model is that all TPM calls are set up in lib/tpm and I
> don't think we should change it. You should be able to move these
> functions into lib/tpm and add your random_read function to tpm_api.h

I moved these functions under separate drivers as I thought that
looked cleaner as against exporting the driver interface in
tpm-v{1,2}.c. If you strongly feel that these should remain under
lib/tpm, I will make the change. But I believe you do not have a
problem with exporting these rng functions as part of the driver
model. We do need that so that the EFI_RNG_PROTOCOL can use these for
getting the random bytes.

-sughosh

>
> Regards,
> Simon

Re: [PATCH v2 04/10] tpm: Move the TPM version detection functions to the uclass driver

[Sughosh Ganu]

hi Simon,

On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
>
> On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > Make the TPM version detection functions as external symbols and move
> > them to the TPM uclass driver. These are useful functions to check the
> > TPM device version and should not be static functions.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> >
> > Changes since V1: None
> >
> >  drivers/tpm/tpm-uclass.c | 11 +++++++++++
> >  include/tpm_api.h        | 20 ++++++++++++++++++++
> >  lib/tpm_api.c            | 10 ----------
> >  3 files changed, 31 insertions(+), 10 deletions(-)
> >
>
> I just sent a similar patch a few days ago.
>
> > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > index f67fe1019b..8619da89d8 100644
> > --- a/drivers/tpm/tpm-uclass.c
> > +++ b/drivers/tpm/tpm-uclass.c
> > @@ -11,10 +11,21 @@
> >  #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"
> >
> > +bool is_tpm1(struct udevice *dev)
> > +{
> > +       return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > +}
> > +
> > +bool is_tpm2(struct udevice *dev)
> > +{
> > +       return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
> > +}
> > +
>
> I think a better name is tpm_is_v1() since this is in the tpm uclass.
> It should have a tpm_ prefix.

Okay, but do we keep this patch, or use the approach which you have
taken to define these as inline functions in tpm_api.h. If we are to
keep these definitions in the uclass driver, I will rename them as you
suggest. Thanks.

-sughosh

>
> Regards,
> Simon
>
> [..]

Re: [PATCH v2 06/10] configs: chromebook_coral: Build TPMV1 library routines

[Sughosh Ganu]

hi Simon,

On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > The TPM code currently does a runtime detection of the TPM version and
> > calls appropriate functions. Chromebook Coral is one of the platforms
> > where the TPMV1 code is disabled at build time. With this, calling TPM
> > api's from the TPM uclass driver results in link errors. Enable TPMV1
> > library routines and determine the TPM version at runtime like other
> > platforms.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >
> > Changes since V1: None
> >
> >  configs/chromebook_coral_defconfig | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/configs/chromebook_coral_defconfig b/configs/chromebook_coral_defconfig
> > index 0cd8f39aa3..4704ce25c8 100644
> > --- a/configs/chromebook_coral_defconfig
> > +++ b/configs/chromebook_coral_defconfig
> > @@ -104,7 +104,6 @@ CONFIG_SPI=y
> >  CONFIG_ICH_SPI=y
> >  # CONFIG_SYSINFO_SMBIOS is not set
> >  CONFIG_TPL_SYSRESET=y
> > -# CONFIG_TPM_V1 is not set
> >  CONFIG_TPM2_CR50_I2C=y
> >  CONFIG_USB_XHCI_HCD=y
> >  CONFIG_USB_STORAGE=y
> > --
> > 2.25.1
>
> This board does not have a v1 TPM so we don't want to waste code space
> adding it.

Yes, but because the version detection is happening at runtime, we
need both the files to be compiled if we call any of the tpm api from
outside lib/tpm. When I call the tpm_startup function from the
child_pre_probe callback in tpm-uclass.c, I get link errors for the
TPM v2 functions. Similarly for Gazerbeam board.

>
> The current code works fine and supports both a build-time and
> run-time check. What has gone wrong?

Does not work when an api is called from the tpm uclass driver.

-sughosh

>
> Regards,
> Simon

Re: [PATCH v2 08/10] tpm: Add the RNG child device

[Sughosh Ganu]

hi Simon,

On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Mon, 28 Feb 2022 at 05:07, 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 V1: None
> >
> >  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 54 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > index 8619da89d8..383cc7bc48 100644
> > --- a/drivers/tpm/tpm-uclass.c
> > +++ b/drivers/tpm/tpm-uclass.c
> > @@ -16,6 +16,11 @@
> >  #include <tpm-v2.h>
> >  #include "tpm_internal.h"
> >
> > +#include <dm/lists.h>
> > +
> > +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> > +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> > +
> >  bool is_tpm1(struct udevice *dev)
> >  {
> >         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> >         return 0;
> >  }
> >
> > +#if IS_ENABLED(CONFIG_TPM)
>
> This should be in the Makefile so that we only build this file if TPM
> is enabled.

The Makefile allows for the tpm uclass driver to be built for SPL and
TPL stages as well. The addition of the RNG device is to be done only
in the u-boot proper stage, since we enable RNG support only in u-boot
proper. Thanks.

-sughosh

>
> > +static int tpm_uclass_post_probe(struct udevice *dev)
> > +{
> > +       int ret;
> > +       const char *drv = is_tpm1(dev) ? TPM_RNG1_DRV_NAME : TPM_RNG2_DRV_NAME;
> > +       struct udevice *child;
> > +
> > +       ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
> > +       if (ret == -ENOENT) {
> > +               log_err("No driver configured for tpm-rng device\n");
> > +               return 0;
> > +       }
> > +
> > +       if (ret) {
> > +               log_err("Unable to bind rng driver with the tpm-rng device\n");
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int tpm_uclass_child_pre_probe(struct udevice *dev)
> > +{
> > +       int ret;
> > +
> > +       ret = tpm_open(dev->parent);
> > +       if (ret == -EBUSY) {
> > +               log_info("TPM device already opened\n");
> > +       } else if (ret) {
> > +               log_err("Unable to open TPM device\n");
> > +               return ret;
> > +       }
> > +
> > +       ret = tpm_startup(dev->parent, TPM_ST_CLEAR);
> > +       if (ret)
> > +               log_err("Unable to start TPM device\n");
> > +
> > +       return ret;
> > +}
> > +#endif /* CONFIG_TPM */
> > +
> >  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
> > +#if IS_ENABLED(CONFIG_TPM)
> > +       .post_probe             = tpm_uclass_post_probe,
> > +       .child_pre_probe        = tpm_uclass_child_pre_probe,
> >  #endif
> >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> >  };
> > --
> > 2.25.1
> >
>
> Regards,
> Simon

Re: [PATCH v2 10/10] cmd: rng: Add support for selecting RNG device

[Sughosh Ganu]

hi Simon,

On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Mon, 28 Feb 2022 at 05:07, 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>
> > ---
> >
> > Changes since V1:
> > * Changed the help text to show order of the parameters passed
> >   to the rng command, based on review comment from Heinrich
> >
> >  cmd/rng.c | 31 +++++++++++++++++++++++--------
> >  1 file changed, 23 insertions(+), 8 deletions(-)
>
> Please add a test for the command and also add doc/usage

We already have a test for the RNG uclass, which is basically doing
the exact same thing. Do we still need one for the command? Or is it
for testing the command parameters?

-sughosh

>
> Regards,
> Simon

Re: [PATCH v2 06/10] configs: chromebook_coral: Build TPMV1 library routines

[Simon Glass]

Hi Sughosh,

On Tue, 1 Mar 2022 at 21:50, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > The TPM code currently does a runtime detection of the TPM version and
> > > calls appropriate functions. Chromebook Coral is one of the platforms
> > > where the TPMV1 code is disabled at build time. With this, calling TPM
> > > api's from the TPM uclass driver results in link errors. Enable TPMV1
> > > library routines and determine the TPM version at runtime like other
> > > platforms.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > >
> > > Changes since V1: None
> > >
> > >  configs/chromebook_coral_defconfig | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/configs/chromebook_coral_defconfig b/configs/chromebook_coral_defconfig
> > > index 0cd8f39aa3..4704ce25c8 100644
> > > --- a/configs/chromebook_coral_defconfig
> > > +++ b/configs/chromebook_coral_defconfig
> > > @@ -104,7 +104,6 @@ CONFIG_SPI=y
> > >  CONFIG_ICH_SPI=y
> > >  # CONFIG_SYSINFO_SMBIOS is not set
> > >  CONFIG_TPL_SYSRESET=y
> > > -# CONFIG_TPM_V1 is not set
> > >  CONFIG_TPM2_CR50_I2C=y
> > >  CONFIG_USB_XHCI_HCD=y
> > >  CONFIG_USB_STORAGE=y
> > > --
> > > 2.25.1
> >
> > This board does not have a v1 TPM so we don't want to waste code space
> > adding it.
>
> Yes, but because the version detection is happening at runtime, we
> need both the files to be compiled if we call any of the tpm api from
> outside lib/tpm. When I call the tpm_startup function from the
> child_pre_probe callback in tpm-uclass.c, I get link errors for the
> TPM v2 functions. Similarly for Gazerbeam board.
>
> >
> > The current code works fine and supports both a build-time and
> > run-time check. What has gone wrong?
>
> Does not work when an api is called from the tpm uclass driver.

OK, I see, then the tpm_is_v1() functions need to stay in the header
file, to fix that.

Regards,
Simon

Re: [PATCH v2 06/10] configs: chromebook_coral: Build TPMV1 library routines

[Sughosh Ganu]

hi Simon,

On Wed, 2 Mar 2022 at 21:02, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Tue, 1 Mar 2022 at 21:50, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > The TPM code currently does a runtime detection of the TPM version and
> > > > calls appropriate functions. Chromebook Coral is one of the platforms
> > > > where the TPMV1 code is disabled at build time. With this, calling TPM
> > > > api's from the TPM uclass driver results in link errors. Enable TPMV1
> > > > library routines and determine the TPM version at runtime like other
> > > > platforms.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > ---
> > > >
> > > > Changes since V1: None
> > > >
> > > >  configs/chromebook_coral_defconfig | 1 -
> > > >  1 file changed, 1 deletion(-)
> > > >
> > > > diff --git a/configs/chromebook_coral_defconfig b/configs/chromebook_coral_defconfig
> > > > index 0cd8f39aa3..4704ce25c8 100644
> > > > --- a/configs/chromebook_coral_defconfig
> > > > +++ b/configs/chromebook_coral_defconfig
> > > > @@ -104,7 +104,6 @@ CONFIG_SPI=y
> > > >  CONFIG_ICH_SPI=y
> > > >  # CONFIG_SYSINFO_SMBIOS is not set
> > > >  CONFIG_TPL_SYSRESET=y
> > > > -# CONFIG_TPM_V1 is not set
> > > >  CONFIG_TPM2_CR50_I2C=y
> > > >  CONFIG_USB_XHCI_HCD=y
> > > >  CONFIG_USB_STORAGE=y
> > > > --
> > > > 2.25.1
> > >
> > > This board does not have a v1 TPM so we don't want to waste code space
> > > adding it.
> >
> > Yes, but because the version detection is happening at runtime, we
> > need both the files to be compiled if we call any of the tpm api from
> > outside lib/tpm. When I call the tpm_startup function from the
> > child_pre_probe callback in tpm-uclass.c, I get link errors for the
> > TPM v2 functions. Similarly for Gazerbeam board.
> >
> > >
> > > The current code works fine and supports both a build-time and
> > > run-time check. What has gone wrong?
> >
> > Does not work when an api is called from the tpm uclass driver.
>
> OK, I see, then the tpm_is_v1() functions need to stay in the header
> file, to fix that.

The link errors are not for the tpm_is_v{1,2} functions -- I had
already moved them to the tpm uclass driver in a previous patch of my
patchset. The link errors are for the tpm{1,2}_startup functions being
undefined for the chromebook_coral and gazerboam platforms. I think
when the tpm_startup function is getting called from within the same
module(lib/tpm) the compiler optimises out the calls to the
non-existing functions. But when called from a different directory,
like the tpm uclass driver in my case, we get link errors.

-sughosh

>
> Regards,
> Simon

Re: [PATCH v2 07/10] tpm: rng: Move the TPM RNG functionality to driver model

[Simon Glass]

Hi Sughosh,

On Tue, 1 Mar 2022 at 21:36, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Tue, 1 Mar 2022 at 20:28, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > Currently, the TPM random number generator(RNG) functions are defined
> > > as part of the library functions under the corresponding tpm files for
> > > tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers
> > > complying with the driver model.
> > >
> > > Also make changes to the tpm_get_random function to have it call the
> > > TPM RNG driver functions instead of the library functions.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > >
> > > Changes since V1:
> > > * Added existing copyrights for the rng functions taken from the tpm
> > >   library routines
> > > * Return -EIO for TPM command returning an error
> > > * Simplify the logic in tpm_get_random based on the review comments
> > >   from Ilias
> > >
> > >  drivers/rng/Makefile   |  1 +
> > >  drivers/rng/tpm1_rng.c | 87 ++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/rng/tpm2_rng.c | 86 +++++++++++++++++++++++++++++++++++++++++
> > >  lib/tpm-v1.c           | 44 ---------------------
> > >  lib/tpm-v2.c           | 44 ---------------------
> > >  lib/tpm_api.c          | 23 +++++++++--
> > >  6 files changed, 193 insertions(+), 92 deletions(-)
> > >  create mode 100644 drivers/rng/tpm1_rng.c
> > >  create mode 100644 drivers/rng/tpm2_rng.c
> > >
> > > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> > > index 39f7ee3f03..129cfbd006 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) += tpm1_rng.o tpm2_rng.o
> > > diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c
> > > new file mode 100644
> > > index 0000000000..7e629756b3
> > > --- /dev/null
> > > +++ b/drivers/rng/tpm1_rng.c
> > > @@ -0,0 +1,87 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (c) 2013 The Chromium OS Authors.
> > > + * Coypright (c) 2013 Guntermann & Drunck GmbH
> > > + * Copyright (c) 2022, Linaro Limited
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <dm.h>
> > > +#include <rng.h>
> > > +#include <tpm-utils.h>
> > > +#include <tpm-v1.h>
> > > +
> > > +#include <linux/errno.h>
> > > +
> > > +#define TPM_HEADER_SIZE                10
> > > +
> > > +#define TPMV1_DATA_OFFSET      14
> > > +
> > > +/**
> > > + * tpm1_rng_read() - Read the random bytes from TPMv1 device
> > > + * @param dev          TPMv1 RNG device
> > > + * @param data         data buffer to write random bytes
> > > + * @param count                number of random bytes to read from
> > > + *                      the device
> > > + *
> > > + * Function to read the random bytes from the RNG pseudo device
> > > + * built into the TPMv1 device. Reads 'count' number of bytes
> > > + * from the random number generator and copies them into the
> > > + * 'data' buffer.
> > > + *
> > > + * Return: 0 if OK, -ve on error.
> > > + *
> > > + */
> > > +static int tpm1_rng_read(struct udevice *dev, void *data, size_t count)
> > > +{
> > > +       const u8 command[14] = {
> > > +               0x0, 0xc1,              /* TPM_TAG */
> > > +               0x0, 0x0, 0x0, 0xe,     /* parameter size */
> > > +               0x0, 0x0, 0x0, 0x46,    /* TPM_COMMAND_CODE */
> > > +       };
> > > +       const size_t length_offset = TPM_HEADER_SIZE;
> > > +       const size_t data_size_offset = TPM_HEADER_SIZE;
> > > +       const size_t data_offset = TPMV1_DATA_OFFSET;
> > > +       u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> > > +       size_t response_length = sizeof(response);
> > > +       u32 data_size;
> > > +       u8 *out = data;
> > > +
> >
> > The current model is that all TPM calls are set up in lib/tpm and I
> > don't think we should change it. You should be able to move these
> > functions into lib/tpm and add your random_read function to tpm_api.h
>
> I moved these functions under separate drivers as I thought that
> looked cleaner as against exporting the driver interface in

But you are now creating TPM messages in a different file so I don't
think it is cleaner. The message pack/unpack should happen in the
tpm_... functions.

> tpm-v{1,2}.c. If you strongly feel that these should remain under
> lib/tpm, I will make the change. But I believe you do not have a
> problem with exporting these rng functions as part of the driver
> model. We do need that so that the EFI_RNG_PROTOCOL can use these for
> getting the random bytes.

I think you might misunderstand me. I mean that the code that calls
pack_byte_string() should be in lib/ like the other code. I agree that
the driver should be were you have put it, I just don't like having
the tpm message creation in that driver. It should call a tpm_...
function to create and send the message, like elsewhere.

Regards,
Simon

Re: [PATCH v2 04/10] tpm: Move the TPM version detection functions to the uclass driver

[Simon Glass]

Hi Sughosh,

On Tue, 1 Mar 2022 at 21:40, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> >
> > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > Make the TPM version detection functions as external symbols and move
> > > them to the TPM uclass driver. These are useful functions to check the
> > > TPM device version and should not be static functions.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > ---
> > >
> > > Changes since V1: None
> > >
> > >  drivers/tpm/tpm-uclass.c | 11 +++++++++++
> > >  include/tpm_api.h        | 20 ++++++++++++++++++++
> > >  lib/tpm_api.c            | 10 ----------
> > >  3 files changed, 31 insertions(+), 10 deletions(-)
> > >
> >
> > I just sent a similar patch a few days ago.
> >
> > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > > index f67fe1019b..8619da89d8 100644
> > > --- a/drivers/tpm/tpm-uclass.c
> > > +++ b/drivers/tpm/tpm-uclass.c
> > > @@ -11,10 +11,21 @@
> > >  #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"
> > >
> > > +bool is_tpm1(struct udevice *dev)
> > > +{
> > > +       return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > > +}
> > > +
> > > +bool is_tpm2(struct udevice *dev)
> > > +{
> > > +       return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
> > > +}
> > > +
> >
> > I think a better name is tpm_is_v1() since this is in the tpm uclass.
> > It should have a tpm_ prefix.
>
> Okay, but do we keep this patch, or use the approach which you have
> taken to define these as inline functions in tpm_api.h. If we are to
> keep these definitions in the uclass driver, I will rename them as you
> suggest. Thanks.

Well if you put them in the .c file then they don't work property and
you have to enable by TPMv1 and TPMv2 code. The idea of the inline
functions is that they work even if you don't enable both APIs.

Regards,
Simon

Re: [PATCH v2 08/10] tpm: Add the RNG child device

[Simon Glass]

Hi Sughosh,

On Tue, 1 Mar 2022 at 21:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Mon, 28 Feb 2022 at 05:07, 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 V1: None
> > >
> > >  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 54 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > > index 8619da89d8..383cc7bc48 100644
> > > --- a/drivers/tpm/tpm-uclass.c
> > > +++ b/drivers/tpm/tpm-uclass.c
> > > @@ -16,6 +16,11 @@
> > >  #include <tpm-v2.h>
> > >  #include "tpm_internal.h"
> > >
> > > +#include <dm/lists.h>
> > > +
> > > +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> > > +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> > > +
> > >  bool is_tpm1(struct udevice *dev)
> > >  {
> > >         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> > >         return 0;
> > >  }
> > >
> > > +#if IS_ENABLED(CONFIG_TPM)
> >
> > This should be in the Makefile so that we only build this file if TPM
> > is enabled.
>
> The Makefile allows for the tpm uclass driver to be built for SPL and
> TPL stages as well. The addition of the RNG device is to be done only
> in the u-boot proper stage, since we enable RNG support only in u-boot
> proper. Thanks.

Well in that case, create a new SPL_TPM_RAND or similar to control
enabling it in SPL. It should be explicit.

Regards,
Simon

Re: [PATCH v2 10/10] cmd: rng: Add support for selecting RNG device

[Simon Glass]

Hi Sughosh,

On Tue, 1 Mar 2022 at 21:56, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Mon, 28 Feb 2022 at 05:07, 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>
> > > ---
> > >
> > > Changes since V1:
> > > * Changed the help text to show order of the parameters passed
> > >   to the rng command, based on review comment from Heinrich
> > >
> > >  cmd/rng.c | 31 +++++++++++++++++++++++--------
> > >  1 file changed, 23 insertions(+), 8 deletions(-)
> >
> > Please add a test for the command and also add doc/usage
>
> We already have a test for the RNG uclass, which is basically doing
> the exact same thing. Do we still need one for the command? Or is it
> for testing the command parameters?

Yes and it provides test coverage. See acpi.c for an example test. It
can be quite simple I suspect.

Regards,
Simon

Re: [PATCH v2 06/10] configs: chromebook_coral: Build TPMV1 library routines

[Simon Glass]

Hi Sughosh,

On Wed, 2 Mar 2022 at 11:12, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Wed, 2 Mar 2022 at 21:02, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Tue, 1 Mar 2022 at 21:50, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > The TPM code currently does a runtime detection of the TPM version and
> > > > > calls appropriate functions. Chromebook Coral is one of the platforms
> > > > > where the TPMV1 code is disabled at build time. With this, calling TPM
> > > > > api's from the TPM uclass driver results in link errors. Enable TPMV1
> > > > > library routines and determine the TPM version at runtime like other
> > > > > platforms.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > ---
> > > > >
> > > > > Changes since V1: None
> > > > >
> > > > >  configs/chromebook_coral_defconfig | 1 -
> > > > >  1 file changed, 1 deletion(-)
> > > > >
> > > > > diff --git a/configs/chromebook_coral_defconfig b/configs/chromebook_coral_defconfig
> > > > > index 0cd8f39aa3..4704ce25c8 100644
> > > > > --- a/configs/chromebook_coral_defconfig
> > > > > +++ b/configs/chromebook_coral_defconfig
> > > > > @@ -104,7 +104,6 @@ CONFIG_SPI=y
> > > > >  CONFIG_ICH_SPI=y
> > > > >  # CONFIG_SYSINFO_SMBIOS is not set
> > > > >  CONFIG_TPL_SYSRESET=y
> > > > > -# CONFIG_TPM_V1 is not set
> > > > >  CONFIG_TPM2_CR50_I2C=y
> > > > >  CONFIG_USB_XHCI_HCD=y
> > > > >  CONFIG_USB_STORAGE=y
> > > > > --
> > > > > 2.25.1
> > > >
> > > > This board does not have a v1 TPM so we don't want to waste code space
> > > > adding it.
> > >
> > > Yes, but because the version detection is happening at runtime, we
> > > need both the files to be compiled if we call any of the tpm api from
> > > outside lib/tpm. When I call the tpm_startup function from the
> > > child_pre_probe callback in tpm-uclass.c, I get link errors for the
> > > TPM v2 functions. Similarly for Gazerbeam board.
> > >
> > > >
> > > > The current code works fine and supports both a build-time and
> > > > run-time check. What has gone wrong?
> > >
> > > Does not work when an api is called from the tpm uclass driver.
> >
> > OK, I see, then the tpm_is_v1() functions need to stay in the header
> > file, to fix that.
>
> The link errors are not for the tpm_is_v{1,2} functions -- I had
> already moved them to the tpm uclass driver in a previous patch of my
> patchset. The link errors are for the tpm{1,2}_startup functions being
> undefined for the chromebook_coral and gazerboam platforms. I think
> when the tpm_startup function is getting called from within the same
> module(lib/tpm) the compiler optimises out the calls to the
> non-existing functions. But when called from a different directory,
> like the tpm uclass driver in my case, we get link errors.

I don't think that is right, sorry.

Regards,
Simon

Re: [PATCH v2 07/10] tpm: rng: Move the TPM RNG functionality to driver model

[Sughosh Ganu]

hi Simon,

On Thu, 3 Mar 2022 at 09:17, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Tue, 1 Mar 2022 at 21:36, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Tue, 1 Mar 2022 at 20:28, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > Currently, the TPM random number generator(RNG) functions are defined
> > > > as part of the library functions under the corresponding tpm files for
> > > > tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers
> > > > complying with the driver model.
> > > >
> > > > Also make changes to the tpm_get_random function to have it call the
> > > > TPM RNG driver functions instead of the library functions.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > ---
> > > >
> > > > Changes since V1:
> > > > * Added existing copyrights for the rng functions taken from the tpm
> > > >   library routines
> > > > * Return -EIO for TPM command returning an error
> > > > * Simplify the logic in tpm_get_random based on the review comments
> > > >   from Ilias
> > > >
> > > >  drivers/rng/Makefile   |  1 +
> > > >  drivers/rng/tpm1_rng.c | 87 ++++++++++++++++++++++++++++++++++++++++++
> > > >  drivers/rng/tpm2_rng.c | 86 +++++++++++++++++++++++++++++++++++++++++
> > > >  lib/tpm-v1.c           | 44 ---------------------
> > > >  lib/tpm-v2.c           | 44 ---------------------
> > > >  lib/tpm_api.c          | 23 +++++++++--
> > > >  6 files changed, 193 insertions(+), 92 deletions(-)
> > > >  create mode 100644 drivers/rng/tpm1_rng.c
> > > >  create mode 100644 drivers/rng/tpm2_rng.c
> > > >
> > > > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> > > > index 39f7ee3f03..129cfbd006 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) += tpm1_rng.o tpm2_rng.o
> > > > diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c
> > > > new file mode 100644
> > > > index 0000000000..7e629756b3
> > > > --- /dev/null
> > > > +++ b/drivers/rng/tpm1_rng.c
> > > > @@ -0,0 +1,87 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * Copyright (c) 2013 The Chromium OS Authors.
> > > > + * Coypright (c) 2013 Guntermann & Drunck GmbH
> > > > + * Copyright (c) 2022, Linaro Limited
> > > > + */
> > > > +
> > > > +#include <common.h>
> > > > +#include <dm.h>
> > > > +#include <rng.h>
> > > > +#include <tpm-utils.h>
> > > > +#include <tpm-v1.h>
> > > > +
> > > > +#include <linux/errno.h>
> > > > +
> > > > +#define TPM_HEADER_SIZE                10
> > > > +
> > > > +#define TPMV1_DATA_OFFSET      14
> > > > +
> > > > +/**
> > > > + * tpm1_rng_read() - Read the random bytes from TPMv1 device
> > > > + * @param dev          TPMv1 RNG device
> > > > + * @param data         data buffer to write random bytes
> > > > + * @param count                number of random bytes to read from
> > > > + *                      the device
> > > > + *
> > > > + * Function to read the random bytes from the RNG pseudo device
> > > > + * built into the TPMv1 device. Reads 'count' number of bytes
> > > > + * from the random number generator and copies them into the
> > > > + * 'data' buffer.
> > > > + *
> > > > + * Return: 0 if OK, -ve on error.
> > > > + *
> > > > + */
> > > > +static int tpm1_rng_read(struct udevice *dev, void *data, size_t count)
> > > > +{
> > > > +       const u8 command[14] = {
> > > > +               0x0, 0xc1,              /* TPM_TAG */
> > > > +               0x0, 0x0, 0x0, 0xe,     /* parameter size */
> > > > +               0x0, 0x0, 0x0, 0x46,    /* TPM_COMMAND_CODE */
> > > > +       };
> > > > +       const size_t length_offset = TPM_HEADER_SIZE;
> > > > +       const size_t data_size_offset = TPM_HEADER_SIZE;
> > > > +       const size_t data_offset = TPMV1_DATA_OFFSET;
> > > > +       u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> > > > +       size_t response_length = sizeof(response);
> > > > +       u32 data_size;
> > > > +       u8 *out = data;
> > > > +
> > >
> > > The current model is that all TPM calls are set up in lib/tpm and I
> > > don't think we should change it. You should be able to move these
> > > functions into lib/tpm and add your random_read function to tpm_api.h
> >
> > I moved these functions under separate drivers as I thought that
> > looked cleaner as against exporting the driver interface in
>
> But you are now creating TPM messages in a different file so I don't
> think it is cleaner. The message pack/unpack should happen in the
> tpm_... functions.
>
> > tpm-v{1,2}.c. If you strongly feel that these should remain under
> > lib/tpm, I will make the change. But I believe you do not have a
> > problem with exporting these rng functions as part of the driver
> > model. We do need that so that the EFI_RNG_PROTOCOL can use these for
> > getting the random bytes.
>
> I think you might misunderstand me. I mean that the code that calls
> pack_byte_string() should be in lib/ like the other code. I agree that
> the driver should be were you have put it, I just don't like having
> the tpm message creation in that driver. It should call a tpm_...
> function to create and send the message, like elsewhere.

I tried putting a wrapper around the pack/unpack functions like you
suggest, but it is getting more complicated than required. Even after
putting the wrapper, there still is an issue of tpm_sendrecv being
declared in tpm-utils.h, along with some other necessary macros like
COMMAND_BUFFER_SIZE. You have mentioned in another review that you
don't want tpm-utils.h to be moved to the include/ directory. If we
are not to move tpm-utils.h and also not call functions like
pack_byte_string directly from the driver, I feel it will be easier to
simply export the rng functions to the driver model from within
lib/tpm/ instead of moving them under drivers/rng/. Will you be okay
with this.

-sughosh

>
> Regards,
> Simon

Re: [PATCH v2 08/10] tpm: Add the RNG child device

[Sughosh Ganu]

hi Simon,

On Thu, 3 Mar 2022 at 09:18, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Tue, 1 Mar 2022 at 21:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Mon, 28 Feb 2022 at 05:07, 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 V1: None
> > > >
> > > >  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 54 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > > > index 8619da89d8..383cc7bc48 100644
> > > > --- a/drivers/tpm/tpm-uclass.c
> > > > +++ b/drivers/tpm/tpm-uclass.c
> > > > @@ -16,6 +16,11 @@
> > > >  #include <tpm-v2.h>
> > > >  #include "tpm_internal.h"
> > > >
> > > > +#include <dm/lists.h>
> > > > +
> > > > +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> > > > +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> > > > +
> > > >  bool is_tpm1(struct udevice *dev)
> > > >  {
> > > >         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > > > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> > > >         return 0;
> > > >  }
> > > >
> > > > +#if IS_ENABLED(CONFIG_TPM)
> > >
> > > This should be in the Makefile so that we only build this file if TPM
> > > is enabled.
> >
> > The Makefile allows for the tpm uclass driver to be built for SPL and
> > TPL stages as well. The addition of the RNG device is to be done only
> > in the u-boot proper stage, since we enable RNG support only in u-boot
> > proper. Thanks.
>
> Well in that case, create a new SPL_TPM_RAND or similar to control
> enabling it in SPL. It should be explicit.

I think it is easier to just protect the child addition functions
under CONFIG_TPM rather than create SPL_RNG and TPL_RNG symbols. We
don't have any requirement for generating random numbers in the SPL
and TPL stages. I feel that creating new symbols just for the sake of
not putting a check for CONFIG_TPM is a bit of an overkill, especially
since we do not have any requirement for RNG devices in the SPL/TPL
stages.

-sughosh

>
> Regards,
> Simon

Re: [PATCH v2 08/10] tpm: Add the RNG child device

[Simon Glass]

Hi Sughosh,

On Thu, 3 Mar 2022 at 05:11, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Thu, 3 Mar 2022 at 09:18, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Tue, 1 Mar 2022 at 21:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Mon, 28 Feb 2022 at 05:07, 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 V1: None
> > > > >
> > > > >  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> > > > >  1 file changed, 54 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > > > > index 8619da89d8..383cc7bc48 100644
> > > > > --- a/drivers/tpm/tpm-uclass.c
> > > > > +++ b/drivers/tpm/tpm-uclass.c
> > > > > @@ -16,6 +16,11 @@
> > > > >  #include <tpm-v2.h>
> > > > >  #include "tpm_internal.h"
> > > > >
> > > > > +#include <dm/lists.h>
> > > > > +
> > > > > +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> > > > > +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> > > > > +
> > > > >  bool is_tpm1(struct udevice *dev)
> > > > >  {
> > > > >         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > > > > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > +#if IS_ENABLED(CONFIG_TPM)
> > > >
> > > > This should be in the Makefile so that we only build this file if TPM
> > > > is enabled.
> > >
> > > The Makefile allows for the tpm uclass driver to be built for SPL and
> > > TPL stages as well. The addition of the RNG device is to be done only
> > > in the u-boot proper stage, since we enable RNG support only in u-boot
> > > proper. Thanks.
> >
> > Well in that case, create a new SPL_TPM_RAND or similar to control
> > enabling it in SPL. It should be explicit.
>
> I think it is easier to just protect the child addition functions
> under CONFIG_TPM rather than create SPL_RNG and TPL_RNG symbols. We
> don't have any requirement for generating random numbers in the SPL
> and TPL stages. I feel that creating new symbols just for the sake of
> not putting a check for CONFIG_TPM is a bit of an overkill, especially
> since we do not have any requirement for RNG devices in the SPL/TPL
> stages.

What does checking for CONFIG_TPM have to do with SPL and TPL? If that
option is enabled, the feature will be active in SPL and TPL too.

Also I see another problem, on further examination. You cannot start
up the TPM in the pre_probe() function. That needs to be done under
board control. E.g. for coral it happens in the TPL (or soon VPL)
phase so cannot be done again in U-Boot proper.

So perhaps we need to remember the state of the TPM (using SPL handoff
perhaps). Also you probably need to move the startup stuff to the RNG
itself.

Perhaps we could add a new function to handle this, which can be
called from your rand driver.

int tpm_ensure_started(struct udevice *dev, enum tpm_startup_type mode)

Regards,
Simon

Re: [PATCH v2 07/10] tpm: rng: Move the TPM RNG functionality to driver model

[Simon Glass]

Hi Sughosh,

On Thu, 3 Mar 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Thu, 3 Mar 2022 at 09:17, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Tue, 1 Mar 2022 at 21:36, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Tue, 1 Mar 2022 at 20:28, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > Currently, the TPM random number generator(RNG) functions are defined
> > > > > as part of the library functions under the corresponding tpm files for
> > > > > tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers
> > > > > complying with the driver model.
> > > > >
> > > > > Also make changes to the tpm_get_random function to have it call the
> > > > > TPM RNG driver functions instead of the library functions.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > ---
> > > > >
> > > > > Changes since V1:
> > > > > * Added existing copyrights for the rng functions taken from the tpm
> > > > >   library routines
> > > > > * Return -EIO for TPM command returning an error
> > > > > * Simplify the logic in tpm_get_random based on the review comments
> > > > >   from Ilias
> > > > >
> > > > >  drivers/rng/Makefile   |  1 +
> > > > >  drivers/rng/tpm1_rng.c | 87 ++++++++++++++++++++++++++++++++++++++++++
> > > > >  drivers/rng/tpm2_rng.c | 86 +++++++++++++++++++++++++++++++++++++++++
> > > > >  lib/tpm-v1.c           | 44 ---------------------
> > > > >  lib/tpm-v2.c           | 44 ---------------------
> > > > >  lib/tpm_api.c          | 23 +++++++++--
> > > > >  6 files changed, 193 insertions(+), 92 deletions(-)
> > > > >  create mode 100644 drivers/rng/tpm1_rng.c
> > > > >  create mode 100644 drivers/rng/tpm2_rng.c
> > > > >
> > > > > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> > > > > index 39f7ee3f03..129cfbd006 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) += tpm1_rng.o tpm2_rng.o
> > > > > diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c
> > > > > new file mode 100644
> > > > > index 0000000000..7e629756b3
> > > > > --- /dev/null
> > > > > +++ b/drivers/rng/tpm1_rng.c
> > > > > @@ -0,0 +1,87 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > +/*
> > > > > + * Copyright (c) 2013 The Chromium OS Authors.
> > > > > + * Coypright (c) 2013 Guntermann & Drunck GmbH
> > > > > + * Copyright (c) 2022, Linaro Limited
> > > > > + */
> > > > > +
> > > > > +#include <common.h>
> > > > > +#include <dm.h>
> > > > > +#include <rng.h>
> > > > > +#include <tpm-utils.h>
> > > > > +#include <tpm-v1.h>
> > > > > +
> > > > > +#include <linux/errno.h>
> > > > > +
> > > > > +#define TPM_HEADER_SIZE                10
> > > > > +
> > > > > +#define TPMV1_DATA_OFFSET      14
> > > > > +
> > > > > +/**
> > > > > + * tpm1_rng_read() - Read the random bytes from TPMv1 device
> > > > > + * @param dev          TPMv1 RNG device
> > > > > + * @param data         data buffer to write random bytes
> > > > > + * @param count                number of random bytes to read from
> > > > > + *                      the device
> > > > > + *
> > > > > + * Function to read the random bytes from the RNG pseudo device
> > > > > + * built into the TPMv1 device. Reads 'count' number of bytes
> > > > > + * from the random number generator and copies them into the
> > > > > + * 'data' buffer.
> > > > > + *
> > > > > + * Return: 0 if OK, -ve on error.
> > > > > + *
> > > > > + */
> > > > > +static int tpm1_rng_read(struct udevice *dev, void *data, size_t count)
> > > > > +{
> > > > > +       const u8 command[14] = {
> > > > > +               0x0, 0xc1,              /* TPM_TAG */
> > > > > +               0x0, 0x0, 0x0, 0xe,     /* parameter size */
> > > > > +               0x0, 0x0, 0x0, 0x46,    /* TPM_COMMAND_CODE */
> > > > > +       };
> > > > > +       const size_t length_offset = TPM_HEADER_SIZE;
> > > > > +       const size_t data_size_offset = TPM_HEADER_SIZE;
> > > > > +       const size_t data_offset = TPMV1_DATA_OFFSET;
> > > > > +       u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> > > > > +       size_t response_length = sizeof(response);
> > > > > +       u32 data_size;
> > > > > +       u8 *out = data;
> > > > > +
> > > >
> > > > The current model is that all TPM calls are set up in lib/tpm and I
> > > > don't think we should change it. You should be able to move these
> > > > functions into lib/tpm and add your random_read function to tpm_api.h
> > >
> > > I moved these functions under separate drivers as I thought that
> > > looked cleaner as against exporting the driver interface in
> >
> > But you are now creating TPM messages in a different file so I don't
> > think it is cleaner. The message pack/unpack should happen in the
> > tpm_... functions.
> >
> > > tpm-v{1,2}.c. If you strongly feel that these should remain under
> > > lib/tpm, I will make the change. But I believe you do not have a
> > > problem with exporting these rng functions as part of the driver
> > > model. We do need that so that the EFI_RNG_PROTOCOL can use these for
> > > getting the random bytes.
> >
> > I think you might misunderstand me. I mean that the code that calls
> > pack_byte_string() should be in lib/ like the other code. I agree that
> > the driver should be were you have put it, I just don't like having
> > the tpm message creation in that driver. It should call a tpm_...
> > function to create and send the message, like elsewhere.
>
> I tried putting a wrapper around the pack/unpack functions like you
> suggest, but it is getting more complicated than required. Even after

See tpm1_get_random() ... can you write a function that works like
that, i.e. implements the tpm API? You are bypassing the API, which is
what I don't like.

> putting the wrapper, there still is an issue of tpm_sendrecv being
> declared in tpm-utils.h, along with some other necessary macros like
> COMMAND_BUFFER_SIZE. You have mentioned in another review that you
> don't want tpm-utils.h to be moved to the include/ directory. If we
> are not to move tpm-utils.h and also not call functions like
> pack_byte_string directly from the driver, I feel it will be easier to
> simply export the rng functions to the driver model from within
> lib/tpm/ instead of moving them under drivers/rng/. Will you be okay
> with this.

I'm just a bit lost as to what the problem is. See above. Perhaps you
can send a patch or add a bit more detail so I can understand it.

Regards,
SImon

Re: [PATCH v2 08/10] tpm: Add the RNG child device

[Sughosh Ganu]

hi Simon,

On Fri, 4 Mar 2022 at 08:07, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Thu, 3 Mar 2022 at 05:11, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Thu, 3 Mar 2022 at 09:18, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Tue, 1 Mar 2022 at 21:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Mon, 28 Feb 2022 at 05:07, 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 V1: None
> > > > > >
> > > > > >  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> > > > > >  1 file changed, 54 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > > > > > index 8619da89d8..383cc7bc48 100644
> > > > > > --- a/drivers/tpm/tpm-uclass.c
> > > > > > +++ b/drivers/tpm/tpm-uclass.c
> > > > > > @@ -16,6 +16,11 @@
> > > > > >  #include <tpm-v2.h>
> > > > > >  #include "tpm_internal.h"
> > > > > >
> > > > > > +#include <dm/lists.h>
> > > > > > +
> > > > > > +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> > > > > > +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> > > > > > +
> > > > > >  bool is_tpm1(struct udevice *dev)
> > > > > >  {
> > > > > >         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > > > > > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> > > > > >         return 0;
> > > > > >  }
> > > > > >
> > > > > > +#if IS_ENABLED(CONFIG_TPM)
> > > > >
> > > > > This should be in the Makefile so that we only build this file if TPM
> > > > > is enabled.
> > > >
> > > > The Makefile allows for the tpm uclass driver to be built for SPL and
> > > > TPL stages as well. The addition of the RNG device is to be done only
> > > > in the u-boot proper stage, since we enable RNG support only in u-boot
> > > > proper. Thanks.
> > >
> > > Well in that case, create a new SPL_TPM_RAND or similar to control
> > > enabling it in SPL. It should be explicit.
> >
> > I think it is easier to just protect the child addition functions
> > under CONFIG_TPM rather than create SPL_RNG and TPL_RNG symbols. We
> > don't have any requirement for generating random numbers in the SPL
> > and TPL stages. I feel that creating new symbols just for the sake of
> > not putting a check for CONFIG_TPM is a bit of an overkill, especially
> > since we do not have any requirement for RNG devices in the SPL/TPL
> > stages.
>
> What does checking for CONFIG_TPM have to do with SPL and TPL? If that
> option is enabled, the feature will be active in SPL and TPL too.

Maybe I am not explaining it properly. We need the addition of the RNG
child device only in the u-boot proper stage, not in the SPL and TPL
stages. The TPM uclass driver can indeed be built for the SPL and TPL
stages, while the RNG uclass is needed only for u-boot proper. So, the
addition of the RNG child device done in the TPM uclass driver should
only happen in u-boot proper, and not in SPL/TPL stages. Which is the
reason for the CONFIG_TPM check.

>
> Also I see another problem, on further examination. You cannot start
> up the TPM in the pre_probe() function. That needs to be done under
> board control. E.g. for coral it happens in the TPL (or soon VPL)
> phase so cannot be done again in U-Boot proper.

I tested running the RNG command after the TPM device has already been
probed and tpm_startup has been called. Even if I call the tpm_startup
again, I do not see any issues. Does the TPM spec prohibit calling the
initialisation function multiple times. I believe that the TPM device
should be able to handle this scenario right?

>
> So perhaps we need to remember the state of the TPM (using SPL handoff
> perhaps). Also you probably need to move the startup stuff to the RNG
> itself.

I can move the call to tpm_startup to the RNG driver's probe function.
But I thought it is better that the parent device(TPM) is initialised
before calling the probe of the child device.

>
> Perhaps we could add a new function to handle this, which can be
> called from your rand driver.
>
> int tpm_ensure_started(struct udevice *dev, enum tpm_startup_type mode)

Okay, I can add this, but the question is, does calling the
tpm_startup function cause issues? If not, maybe this is not needed?

-sughosh

>
> Regards,
> Simon

Re: [PATCH v2 07/10] tpm: rng: Move the TPM RNG functionality to driver model

[Sughosh Ganu]

hi Simon,

On Fri, 4 Mar 2022 at 08:08, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Thu, 3 Mar 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Thu, 3 Mar 2022 at 09:17, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Tue, 1 Mar 2022 at 21:36, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Tue, 1 Mar 2022 at 20:28, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > Currently, the TPM random number generator(RNG) functions are defined
> > > > > > as part of the library functions under the corresponding tpm files for
> > > > > > tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers
> > > > > > complying with the driver model.
> > > > > >
> > > > > > Also make changes to the tpm_get_random function to have it call the
> > > > > > TPM RNG driver functions instead of the library functions.
> > > > > >
> > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > ---
> > > > > >
> > > > > > Changes since V1:
> > > > > > * Added existing copyrights for the rng functions taken from the tpm
> > > > > >   library routines
> > > > > > * Return -EIO for TPM command returning an error
> > > > > > * Simplify the logic in tpm_get_random based on the review comments
> > > > > >   from Ilias
> > > > > >
> > > > > >  drivers/rng/Makefile   |  1 +
> > > > > >  drivers/rng/tpm1_rng.c | 87 ++++++++++++++++++++++++++++++++++++++++++
> > > > > >  drivers/rng/tpm2_rng.c | 86 +++++++++++++++++++++++++++++++++++++++++
> > > > > >  lib/tpm-v1.c           | 44 ---------------------
> > > > > >  lib/tpm-v2.c           | 44 ---------------------
> > > > > >  lib/tpm_api.c          | 23 +++++++++--
> > > > > >  6 files changed, 193 insertions(+), 92 deletions(-)
> > > > > >  create mode 100644 drivers/rng/tpm1_rng.c
> > > > > >  create mode 100644 drivers/rng/tpm2_rng.c
> > > > > >
> > > > > > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> > > > > > index 39f7ee3f03..129cfbd006 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) += tpm1_rng.o tpm2_rng.o
> > > > > > diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c
> > > > > > new file mode 100644
> > > > > > index 0000000000..7e629756b3
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/rng/tpm1_rng.c
> > > > > > @@ -0,0 +1,87 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > +/*
> > > > > > + * Copyright (c) 2013 The Chromium OS Authors.
> > > > > > + * Coypright (c) 2013 Guntermann & Drunck GmbH
> > > > > > + * Copyright (c) 2022, Linaro Limited
> > > > > > + */
> > > > > > +
> > > > > > +#include <common.h>
> > > > > > +#include <dm.h>
> > > > > > +#include <rng.h>
> > > > > > +#include <tpm-utils.h>
> > > > > > +#include <tpm-v1.h>
> > > > > > +
> > > > > > +#include <linux/errno.h>
> > > > > > +
> > > > > > +#define TPM_HEADER_SIZE                10
> > > > > > +
> > > > > > +#define TPMV1_DATA_OFFSET      14
> > > > > > +
> > > > > > +/**
> > > > > > + * tpm1_rng_read() - Read the random bytes from TPMv1 device
> > > > > > + * @param dev          TPMv1 RNG device
> > > > > > + * @param data         data buffer to write random bytes
> > > > > > + * @param count                number of random bytes to read from
> > > > > > + *                      the device
> > > > > > + *
> > > > > > + * Function to read the random bytes from the RNG pseudo device
> > > > > > + * built into the TPMv1 device. Reads 'count' number of bytes
> > > > > > + * from the random number generator and copies them into the
> > > > > > + * 'data' buffer.
> > > > > > + *
> > > > > > + * Return: 0 if OK, -ve on error.
> > > > > > + *
> > > > > > + */
> > > > > > +static int tpm1_rng_read(struct udevice *dev, void *data, size_t count)
> > > > > > +{
> > > > > > +       const u8 command[14] = {
> > > > > > +               0x0, 0xc1,              /* TPM_TAG */
> > > > > > +               0x0, 0x0, 0x0, 0xe,     /* parameter size */
> > > > > > +               0x0, 0x0, 0x0, 0x46,    /* TPM_COMMAND_CODE */
> > > > > > +       };
> > > > > > +       const size_t length_offset = TPM_HEADER_SIZE;
> > > > > > +       const size_t data_size_offset = TPM_HEADER_SIZE;
> > > > > > +       const size_t data_offset = TPMV1_DATA_OFFSET;
> > > > > > +       u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> > > > > > +       size_t response_length = sizeof(response);
> > > > > > +       u32 data_size;
> > > > > > +       u8 *out = data;
> > > > > > +
> > > > >
> > > > > The current model is that all TPM calls are set up in lib/tpm and I
> > > > > don't think we should change it. You should be able to move these
> > > > > functions into lib/tpm and add your random_read function to tpm_api.h
> > > >
> > > > I moved these functions under separate drivers as I thought that
> > > > looked cleaner as against exporting the driver interface in
> > >
> > > But you are now creating TPM messages in a different file so I don't
> > > think it is cleaner. The message pack/unpack should happen in the
> > > tpm_... functions.
> > >
> > > > tpm-v{1,2}.c. If you strongly feel that these should remain under
> > > > lib/tpm, I will make the change. But I believe you do not have a
> > > > problem with exporting these rng functions as part of the driver
> > > > model. We do need that so that the EFI_RNG_PROTOCOL can use these for
> > > > getting the random bytes.
> > >
> > > I think you might misunderstand me. I mean that the code that calls
> > > pack_byte_string() should be in lib/ like the other code. I agree that
> > > the driver should be were you have put it, I just don't like having
> > > the tpm message creation in that driver. It should call a tpm_...
> > > function to create and send the message, like elsewhere.
> >
> > I tried putting a wrapper around the pack/unpack functions like you
> > suggest, but it is getting more complicated than required. Even after
>
> See tpm1_get_random() ... can you write a function that works like
> that, i.e. implements the tpm API? You are bypassing the API, which is
> what I don't like.
>
> > putting the wrapper, there still is an issue of tpm_sendrecv being
> > declared in tpm-utils.h, along with some other necessary macros like
> > COMMAND_BUFFER_SIZE. You have mentioned in another review that you
> > don't want tpm-utils.h to be moved to the include/ directory. If we
> > are not to move tpm-utils.h and also not call functions like
> > pack_byte_string directly from the driver, I feel it will be easier to
> > simply export the rng functions to the driver model from within
> > lib/tpm/ instead of moving them under drivers/rng/. Will you be okay
> > with this.
>
> I'm just a bit lost as to what the problem is. See above. Perhaps you
> can send a patch or add a bit more detail so I can understand it.

I have sent a v3 patch series which takes the approach I was trying to
explain above. Please check. Thanks.

-sughosh

>
> Regards,
> SImon

Re: [PATCH v2 08/10] tpm: Add the RNG child device

[Ilias Apalodimas]

Hi,

[...]

> > >
> > > I think it is easier to just protect the child addition functions
> > > under CONFIG_TPM rather than create SPL_RNG and TPL_RNG symbols. We
> > > don't have any requirement for generating random numbers in the SPL
> > > and TPL stages. I feel that creating new symbols just for the sake of
> > > not putting a check for CONFIG_TPM is a bit of an overkill, especially
> > > since we do not have any requirement for RNG devices in the SPL/TPL
> > > stages.
> >
> > What does checking for CONFIG_TPM have to do with SPL and TPL? If that
> > option is enabled, the feature will be active in SPL and TPL too.
>
> Maybe I am not explaining it properly. We need the addition of the RNG
> child device only in the u-boot proper stage, not in the SPL and TPL
> stages. The TPM uclass driver can indeed be built for the SPL and TPL
> stages, while the RNG uclass is needed only for u-boot proper. So, the
> addition of the RNG child device done in the TPM uclass driver should
> only happen in u-boot proper, and not in SPL/TPL stages. Which is the
> reason for the CONFIG_TPM check.
>
> >
> > Also I see another problem, on further examination. You cannot start
> > up the TPM in the pre_probe() function. That needs to be done under
> > board control. E.g. for coral it happens in the TPL (or soon VPL)
> > phase so cannot be done again in U-Boot proper.
>
> I tested running the RNG command after the TPM device has already been
> probed and tpm_startup has been called. Even if I call the tpm_startup
> again, I do not see any issues. Does the TPM spec prohibit calling the
> initialisation function multiple times. I believe that the TPM device
> should be able to handle this scenario right?
[...]

We've already discussed this in [1].  Issuing multiple startup
commands will just make the TPM respond with TPM2_RC_INITIALIZE.  At
least that's my reading of [2]

[1] https://lore.kernel.org/all/CAC_iWjLQmsGLLYfnDFq17-SD3RC7-Da-M41Qc5DjWp3XX9ZHTA@mail.gmail.com/
[2] https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-3-Commands-01.38.pdf
section 9.3.1

Regards
/Ilias

Re: [PATCH v2 08/10] tpm: Add the RNG child device

[Simon Glass]

Hi Sughosh,

On Fri, 4 Mar 2022 at 06:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Fri, 4 Mar 2022 at 08:07, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Thu, 3 Mar 2022 at 05:11, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Thu, 3 Mar 2022 at 09:18, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Tue, 1 Mar 2022 at 21:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Mon, 28 Feb 2022 at 05:07, 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 V1: None
> > > > > > >
> > > > > > >  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> > > > > > >  1 file changed, 54 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > > > > > > index 8619da89d8..383cc7bc48 100644
> > > > > > > --- a/drivers/tpm/tpm-uclass.c
> > > > > > > +++ b/drivers/tpm/tpm-uclass.c
> > > > > > > @@ -16,6 +16,11 @@
> > > > > > >  #include <tpm-v2.h>
> > > > > > >  #include "tpm_internal.h"
> > > > > > >
> > > > > > > +#include <dm/lists.h>
> > > > > > > +
> > > > > > > +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> > > > > > > +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> > > > > > > +
> > > > > > >  bool is_tpm1(struct udevice *dev)
> > > > > > >  {
> > > > > > >         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > > > > > > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> > > > > > >         return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > +#if IS_ENABLED(CONFIG_TPM)
> > > > > >
> > > > > > This should be in the Makefile so that we only build this file if TPM
> > > > > > is enabled.
> > > > >
> > > > > The Makefile allows for the tpm uclass driver to be built for SPL and
> > > > > TPL stages as well. The addition of the RNG device is to be done only
> > > > > in the u-boot proper stage, since we enable RNG support only in u-boot
> > > > > proper. Thanks.
> > > >
> > > > Well in that case, create a new SPL_TPM_RAND or similar to control
> > > > enabling it in SPL. It should be explicit.
> > >
> > > I think it is easier to just protect the child addition functions
> > > under CONFIG_TPM rather than create SPL_RNG and TPL_RNG symbols. We
> > > don't have any requirement for generating random numbers in the SPL
> > > and TPL stages. I feel that creating new symbols just for the sake of
> > > not putting a check for CONFIG_TPM is a bit of an overkill, especially
> > > since we do not have any requirement for RNG devices in the SPL/TPL
> > > stages.
> >
> > What does checking for CONFIG_TPM have to do with SPL and TPL? If that
> > option is enabled, the feature will be active in SPL and TPL too.
>
> Maybe I am not explaining it properly. We need the addition of the RNG
> child device only in the u-boot proper stage, not in the SPL and TPL
> stages. The TPM uclass driver can indeed be built for the SPL and TPL
> stages, while the RNG uclass is needed only for u-boot proper. So, the
> addition of the RNG child device done in the TPM uclass driver should
> only happen in u-boot proper, and not in SPL/TPL stages. Which is the
> reason for the CONFIG_TPM check.

Let me try one more time. If this doesn't work, please chat with a colleague.

The way we do that is with Kconfig options. We don't use ad-hoc
methods to enable things in U-Boot proper but not SPL. Boards that
have CONFIG_TPM set have it set everywhere, including in SPL. So
checking for CONFIG_TPM is going to return true in both U-Boot proper
and SPL. If you want different behaviour in SPL, you need
CONFIG_SPL_TPM. But even that is indirect, as I have explained. To
enable your driver in SPL, you need a CONFIG_SPL_...

Also, you should use devicetree to provide the random number
generator, just a device_bind(). Add the node as a subnode of the TPM.
Otherwise it cannot work with OF_PLATDATA.

>
> >
> > Also I see another problem, on further examination. You cannot start
> > up the TPM in the pre_probe() function. That needs to be done under
> > board control. E.g. for coral it happens in the TPL (or soon VPL)
> > phase so cannot be done again in U-Boot proper.
>
> I tested running the RNG command after the TPM device has already been
> probed and tpm_startup has been called. Even if I call the tpm_startup
> again, I do not see any issues. Does the TPM spec prohibit calling the
> initialisation function multiple times. I believe that the TPM device
> should be able to handle this scenario right?

I hangs on coral, for example.

>
> >
> > So perhaps we need to remember the state of the TPM (using SPL handoff
> > perhaps). Also you probably need to move the startup stuff to the RNG
> > itself.
>
> I can move the call to tpm_startup to the RNG driver's probe function.
> But I thought it is better that the parent device(TPM) is initialised
> before calling the probe of the child device.

We use lazy init in U-Boot, so it should be possible to probe the TPM
without automatically starting it. Otherwise you are going to break
some of the tpm commands.

>
> >
> > Perhaps we could add a new function to handle this, which can be
> > called from your rand driver.
> >
> > int tpm_ensure_started(struct udevice *dev, enum tpm_startup_type mode)
>
> Okay, I can add this, but the question is, does calling the
> tpm_startup function cause issues? If not, maybe this is not needed?

It does on coral. I don't know whether other TPMs are tolerant of it,
but in any case it is not a good idea.

Regards,
Simon

Re: [PATCH v2 08/10] tpm: Add the RNG child device

[Sughosh Ganu]

hi Simon,

On Sun, 6 Mar 2022 at 08:37, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Fri, 4 Mar 2022 at 06:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Fri, 4 Mar 2022 at 08:07, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Thu, 3 Mar 2022 at 05:11, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Thu, 3 Mar 2022 at 09:18, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Tue, 1 Mar 2022 at 21:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > hi Simon,
> > > > > >
> > > > > > On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Sughosh,
> > > > > > >
> > > > > > > On Mon, 28 Feb 2022 at 05:07, 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 V1: None
> > > > > > > >
> > > > > > > >  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> > > > > > > >  1 file changed, 54 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > > > > > > > index 8619da89d8..383cc7bc48 100644
> > > > > > > > --- a/drivers/tpm/tpm-uclass.c
> > > > > > > > +++ b/drivers/tpm/tpm-uclass.c
> > > > > > > > @@ -16,6 +16,11 @@
> > > > > > > >  #include <tpm-v2.h>
> > > > > > > >  #include "tpm_internal.h"
> > > > > > > >
> > > > > > > > +#include <dm/lists.h>
> > > > > > > > +
> > > > > > > > +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> > > > > > > > +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> > > > > > > > +
> > > > > > > >  bool is_tpm1(struct udevice *dev)
> > > > > > > >  {
> > > > > > > >         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > > > > > > > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> > > > > > > >         return 0;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +#if IS_ENABLED(CONFIG_TPM)
> > > > > > >
> > > > > > > This should be in the Makefile so that we only build this file if TPM
> > > > > > > is enabled.
> > > > > >
> > > > > > The Makefile allows for the tpm uclass driver to be built for SPL and
> > > > > > TPL stages as well. The addition of the RNG device is to be done only
> > > > > > in the u-boot proper stage, since we enable RNG support only in u-boot
> > > > > > proper. Thanks.
> > > > >
> > > > > Well in that case, create a new SPL_TPM_RAND or similar to control
> > > > > enabling it in SPL. It should be explicit.
> > > >
> > > > I think it is easier to just protect the child addition functions
> > > > under CONFIG_TPM rather than create SPL_RNG and TPL_RNG symbols. We
> > > > don't have any requirement for generating random numbers in the SPL
> > > > and TPL stages. I feel that creating new symbols just for the sake of
> > > > not putting a check for CONFIG_TPM is a bit of an overkill, especially
> > > > since we do not have any requirement for RNG devices in the SPL/TPL
> > > > stages.
> > >
> > > What does checking for CONFIG_TPM have to do with SPL and TPL? If that
> > > option is enabled, the feature will be active in SPL and TPL too.
> >
> > Maybe I am not explaining it properly. We need the addition of the RNG
> > child device only in the u-boot proper stage, not in the SPL and TPL
> > stages. The TPM uclass driver can indeed be built for the SPL and TPL
> > stages, while the RNG uclass is needed only for u-boot proper. So, the
> > addition of the RNG child device done in the TPM uclass driver should
> > only happen in u-boot proper, and not in SPL/TPL stages. Which is the
> > reason for the CONFIG_TPM check.
>
> Let me try one more time. If this doesn't work, please chat with a colleague.
>
> The way we do that is with Kconfig options. We don't use ad-hoc
> methods to enable things in U-Boot proper but not SPL. Boards that
> have CONFIG_TPM set have it set everywhere, including in SPL. So
> checking for CONFIG_TPM is going to return true in both U-Boot proper
> and SPL. If you want different behaviour in SPL, you need
> CONFIG_SPL_TPM. But even that is indirect, as I have explained. To
> enable your driver in SPL, you need a CONFIG_SPL_...

Okay. I now get your point. So this is because these config symbols
are getting included in all the stages, so I cannot use the symbol
name to identify the spl/tpl/u-boot stages. I believe earlier, the
identification used to be done based on whether a symbol is CONFIG_xxx
as against CONFIG_SPL_xxx. Now it is being done with
CONFIG_{SPL,TPL}_BUILD symbols for differentiating between stages. I
will use these instead to have the code enabled only for the u-boot
proper stage. Thing is, I don't want to define these functions for the
spl and tpl stages since they are adding to the size with no benefits.
I do understand that currently no platform is enabling tpm drivers in
the spl/tpl stage, but the driver can be enabled in those stages.

>
> Also, you should use devicetree to provide the random number
> generator, just a device_bind(). Add the node as a subnode of the TPM.
> Otherwise it cannot work with OF_PLATDATA.

As per the documentation in of-plat.rst, the OF_PLATDATA method is to
be used only for the SPL/TPL stages where there might be a size
constraint. And the tpm rng drivers are to be enabled only in the
u-boot proper stage. So I think we would not need to put a subnode in
the devicetree. Basically, the presence of the rng functionality in
the TPM device is not optional as per my understanding, so the rng
child node can just be added in the tpm uclass driver.

>
> >
> > >
> > > Also I see another problem, on further examination. You cannot start
> > > up the TPM in the pre_probe() function. That needs to be done under
> > > board control. E.g. for coral it happens in the TPL (or soon VPL)
> > > phase so cannot be done again in U-Boot proper.
> >
> > I tested running the RNG command after the TPM device has already been
> > probed and tpm_startup has been called. Even if I call the tpm_startup
> > again, I do not see any issues. Does the TPM spec prohibit calling the
> > initialisation function multiple times. I believe that the TPM device
> > should be able to handle this scenario right?
>
> I hangs on coral, for example.
>
> >
> > >
> > > So perhaps we need to remember the state of the TPM (using SPL handoff
> > > perhaps). Also you probably need to move the startup stuff to the RNG
> > > itself.
> >
> > I can move the call to tpm_startup to the RNG driver's probe function.
> > But I thought it is better that the parent device(TPM) is initialised
> > before calling the probe of the child device.
>
> We use lazy init in U-Boot, so it should be possible to probe the TPM
> without automatically starting it. Otherwise you are going to break
> some of the tpm commands.

Yes I understand. With the patch, the tpm device is being started only
when a child device is being probed. But I can remove the startup of
the tpm device from the child_pre_probe function. It will have to be
done explicitly before the rng device is to be used.

Btw, can you please comment on patch 3 of this series[1].

-sughosh

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

>
> >
> > >
> > > Perhaps we could add a new function to handle this, which can be
> > > called from your rand driver.
> > >
> > > int tpm_ensure_started(struct udevice *dev, enum tpm_startup_type mode)
> >
> > Okay, I can add this, but the question is, does calling the
> > tpm_startup function cause issues? If not, maybe this is not needed?
>
> It does on coral. I don't know whether other TPMs are tolerant of it,
> but in any case it is not a good idea.
>
> Regards,
> Simon

Re: [PATCH v2 08/10] tpm: Add the RNG child device

[Simon Glass]

Hi Sughosh,

On Sun, 6 Mar 2022 at 00:03, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Sun, 6 Mar 2022 at 08:37, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Fri, 4 Mar 2022 at 06:44, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Fri, 4 Mar 2022 at 08:07, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Thu, 3 Mar 2022 at 05:11, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Thu, 3 Mar 2022 at 09:18, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Tue, 1 Mar 2022 at 21:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > >
> > > > > > > hi Simon,
> > > > > > >
> > > > > > > On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Sughosh,
> > > > > > > >
> > > > > > > > On Mon, 28 Feb 2022 at 05:07, 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 V1: None
> > > > > > > > >
> > > > > > > > >  drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> > > > > > > > >  1 file changed, 54 insertions(+), 4 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > > > > > > > > index 8619da89d8..383cc7bc48 100644
> > > > > > > > > --- a/drivers/tpm/tpm-uclass.c
> > > > > > > > > +++ b/drivers/tpm/tpm-uclass.c
> > > > > > > > > @@ -16,6 +16,11 @@
> > > > > > > > >  #include <tpm-v2.h>
> > > > > > > > >  #include "tpm_internal.h"
> > > > > > > > >
> > > > > > > > > +#include <dm/lists.h>
> > > > > > > > > +
> > > > > > > > > +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> > > > > > > > > +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> > > > > > > > > +
> > > > > > > > >  bool is_tpm1(struct udevice *dev)
> > > > > > > > >  {
> > > > > > > > >         return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > > > > > > > > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> > > > > > > > >         return 0;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > +#if IS_ENABLED(CONFIG_TPM)
> > > > > > > >
> > > > > > > > This should be in the Makefile so that we only build this file if TPM
> > > > > > > > is enabled.
> > > > > > >
> > > > > > > The Makefile allows for the tpm uclass driver to be built for SPL and
> > > > > > > TPL stages as well. The addition of the RNG device is to be done only
> > > > > > > in the u-boot proper stage, since we enable RNG support only in u-boot
> > > > > > > proper. Thanks.
> > > > > >
> > > > > > Well in that case, create a new SPL_TPM_RAND or similar to control
> > > > > > enabling it in SPL. It should be explicit.
> > > > >
> > > > > I think it is easier to just protect the child addition functions
> > > > > under CONFIG_TPM rather than create SPL_RNG and TPL_RNG symbols. We
> > > > > don't have any requirement for generating random numbers in the SPL
> > > > > and TPL stages. I feel that creating new symbols just for the sake of
> > > > > not putting a check for CONFIG_TPM is a bit of an overkill, especially
> > > > > since we do not have any requirement for RNG devices in the SPL/TPL
> > > > > stages.
> > > >
> > > > What does checking for CONFIG_TPM have to do with SPL and TPL? If that
> > > > option is enabled, the feature will be active in SPL and TPL too.
> > >
> > > Maybe I am not explaining it properly. We need the addition of the RNG
> > > child device only in the u-boot proper stage, not in the SPL and TPL
> > > stages. The TPM uclass driver can indeed be built for the SPL and TPL
> > > stages, while the RNG uclass is needed only for u-boot proper. So, the
> > > addition of the RNG child device done in the TPM uclass driver should
> > > only happen in u-boot proper, and not in SPL/TPL stages. Which is the
> > > reason for the CONFIG_TPM check.
> >
> > Let me try one more time. If this doesn't work, please chat with a colleague.
> >
> > The way we do that is with Kconfig options. We don't use ad-hoc
> > methods to enable things in U-Boot proper but not SPL. Boards that
> > have CONFIG_TPM set have it set everywhere, including in SPL. So
> > checking for CONFIG_TPM is going to return true in both U-Boot proper
> > and SPL. If you want different behaviour in SPL, you need
> > CONFIG_SPL_TPM. But even that is indirect, as I have explained. To
> > enable your driver in SPL, you need a CONFIG_SPL_...
>
> Okay. I now get your point. So this is because these config symbols
> are getting included in all the stages, so I cannot use the symbol
> name to identify the spl/tpl/u-boot stages. I believe earlier, the
> identification used to be done based on whether a symbol is CONFIG_xxx
> as against CONFIG_SPL_xxx. Now it is being done with
> CONFIG_{SPL,TPL}_BUILD symbols for differentiating between stages. I
> will use these instead to have the code enabled only for the u-boot
> proper stage. Thing is, I don't want to define these functions for the
> spl and tpl stages since they are adding to the size with no benefits.
> I do understand that currently no platform is enabling tpm drivers in
> the spl/tpl stage, but the driver can be enabled in those stages.

coral uses TPM in the VPL stage. In general, the TPM needs to be used
earlier than U-Boot proper, since verified boot has to be complete by
then.

>
> >
> > Also, you should use devicetree to provide the random number
> > generator, just a device_bind(). Add the node as a subnode of the TPM.
> > Otherwise it cannot work with OF_PLATDATA.
>
> As per the documentation in of-plat.rst, the OF_PLATDATA method is to
> be used only for the SPL/TPL stages where there might be a size
> constraint. And the tpm rng drivers are to be enabled only in the
> u-boot proper stage. So I think we would not need to put a subnode in
> the devicetree. Basically, the presence of the rng functionality in
> the TPM device is not optional as per my understanding, so the rng
> child node can just be added in the tpm uclass driver.

We don't need the subnode but it is normal practice to have one. If
you want to use a manual bind, that is OK, at least for U-Boot proper.

>
> >
> > >
> > > >
> > > > Also I see another problem, on further examination. You cannot start
> > > > up the TPM in the pre_probe() function. That needs to be done under
> > > > board control. E.g. for coral it happens in the TPL (or soon VPL)
> > > > phase so cannot be done again in U-Boot proper.
> > >
> > > I tested running the RNG command after the TPM device has already been
> > > probed and tpm_startup has been called. Even if I call the tpm_startup
> > > again, I do not see any issues. Does the TPM spec prohibit calling the
> > > initialisation function multiple times. I believe that the TPM device
> > > should be able to handle this scenario right?
> >
> > I hangs on coral, for example.
> >
> > >
> > > >
> > > > So perhaps we need to remember the state of the TPM (using SPL handoff
> > > > perhaps). Also you probably need to move the startup stuff to the RNG
> > > > itself.
> > >
> > > I can move the call to tpm_startup to the RNG driver's probe function.
> > > But I thought it is better that the parent device(TPM) is initialised
> > > before calling the probe of the child device.
> >
> > We use lazy init in U-Boot, so it should be possible to probe the TPM
> > without automatically starting it. Otherwise you are going to break
> > some of the tpm commands.
>
> Yes I understand. With the patch, the tpm device is being started only
> when a child device is being probed. But I can remove the startup of
> the tpm device from the child_pre_probe function. It will have to be
> done explicitly before the rng device is to be used.

Yes, that's how it should work, for now.

>
> Btw, can you please comment on patch 3 of this series[1].

I suspect it is in my queue but I have not had time yet.

Regards,
Simon



>
> -sughosh
>
> [1] - https://lists.denx.de/pipermail/u-boot/2022-March/476830.html
>
> >
> > >
> > > >
> > > > Perhaps we could add a new function to handle this, which can be
> > > > called from your rand driver.
> > > >
> > > > int tpm_ensure_started(struct udevice *dev, enum tpm_startup_type mode)
> > >
> > > Okay, I can add this, but the question is, does calling the
> > > tpm_startup function cause issues? If not, maybe this is not needed?
> >
> > It does on coral. I don't know whether other TPMs are tolerant of it,
> > but in any case it is not a good idea.
> >
> > Regards,
> > Simon