hi Simon,
On Sat, 12 Mar 2022 at 08:14, Simon Glass sjg@chromium.org wrote:
Hi Sughosh,
On Wed, 9 Mar 2022 at 05:28, Sughosh Ganu sughosh.ganu@linaro.org wrote:
The TPM device has a builtin random number generator(RNG) functionality. Expose the RNG functions of the TPM device to the driver model so that they can be used by the EFI_RNG_PROTOCOL if the protocol is installed.
Also change the function arguments and return type of the random number functions to comply with the driver model api.
Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org
Changes since V3:
- Move back the driver model interface for the TPM RNG devices under drivers/rng/ directory.
- Add a rng read function for the tpm devices which calls the tpm*_get_random API's.
- Pass the TPM device pointer to the tpm*_get_random API's from the TPM RNG drivers as suggested by Simon.
drivers/rng/Makefile | 2 ++ drivers/rng/tpm1_rng.c | 24 ++++++++++++++++++++++++ drivers/rng/tpm2_rng.c | 23 +++++++++++++++++++++++ include/tpm-v1.h | 4 ++-- include/tpm-v2.h | 4 ++-- lib/tpm-v1.c | 16 +++++++++------- lib/tpm-v2.c | 9 +++++---- lib/tpm_api.c | 23 +++++++++++++++++++---- 8 files changed, 86 insertions(+), 19 deletions(-) create mode 100644 drivers/rng/tpm1_rng.c create mode 100644 drivers/rng/tpm2_rng.c
<snip>
+static int rng_tpm2_random_read(struct udevice *dev, void *data, size_t count) +{
return tpm2_get_random(dev->parent, data, count);+}
+static const struct dm_rng_ops tpm2_rng_ops = {
.read = rng_tpm2_random_read,+};
+U_BOOT_DRIVER(tpm2_rng) = {
.name = "tpm2-rng",.id = UCLASS_RNG,.ops = &tpm2_rng_ops,+};
Can't you just have one driver and have a tpm_get_random() function in tpm_api.h ?
With a call to tpm_get_random we can, yes.
diff --git a/include/tpm-v1.h b/include/tpm-v1.h index 33d53fb695..d2ff8b446d 100644 --- a/include/tpm-v1.h +++ b/include/tpm-v1.h @@ -555,9 +555,9 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
- @param dev TPM device
- @param data output buffer for the random bytes
- @param count size of output buffer
- Return: return code of the operation
*/
- Return: 0 if OK, -ve on error
-u32 tpm1_get_random(struct udevice *dev, void *data, u32 count); +int tpm1_get_random(struct udevice *dev, void *data, size_t count);
/**
- tpm_finalise_physical_presence() - Finalise physical presence
diff --git a/include/tpm-v2.h b/include/tpm-v2.h index e79c90b939..4fb1e7a948 100644 --- a/include/tpm-v2.h +++ b/include/tpm-v2.h @@ -619,9 +619,9 @@ u32 tpm2_pcr_setauthvalue(struct udevice *dev, const char *pw,
- @param data output buffer for the random bytes
- @param count size of output buffer
- Return: return code of the operation
*/
- Return: 0 if OK, -ve on error
-u32 tpm2_get_random(struct udevice *dev, void *data, u32 count); +int tpm2_get_random(struct udevice *dev, void *data, size_t count);
Do you need to change these? It would be better if the caller dealt with this.
I am putting these functions in line with the RNG uclass interface which has the count parameter as a size_t data type.
/**
- Lock data in the TPM
diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c index 22a769c587..4c6bc31a64 100644 --- a/lib/tpm-v1.c +++ b/lib/tpm-v1.c @@ -9,12 +9,14 @@ #include <common.h> #include <dm.h> #include <log.h> -#include <asm/unaligned.h> -#include <u-boot/sha1.h> +#include <rng.h> #include <tpm-common.h> #include <tpm-v1.h> #include "tpm-utils.h"
+#include <asm/unaligned.h> +#include <u-boot/sha1.h>
#ifdef CONFIG_TPM_AUTH_SESSIONS
#ifndef CONFIG_SHA1 @@ -869,7 +871,7 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
#endif /* CONFIG_TPM_AUTH_SESSIONS */
-u32 tpm1_get_random(struct udevice *dev, void *data, u32 count) +int tpm1_get_random(struct udevice *dev, void *data, size_t count) { const u8 command[14] = { 0x0, 0xc1, /* TPM_TAG */ @@ -892,19 +894,19 @@ u32 tpm1_get_random(struct udevice *dev, void *data, u32 count) if (pack_byte_string(buf, sizeof(buf), "sd", 0, command, sizeof(command), length_offset, this_bytes))
return TPM_LIB_ERROR;
return -EIO; err = tpm_sendrecv_command(dev, buf, response, &response_length); if (err) return err; if (unpack_byte_string(response, response_length, "d", data_size_offset, &data_size))
return TPM_LIB_ERROR;
return -EIO; if (data_size > count)
return TPM_LIB_ERROR;
return -EIO; if (unpack_byte_string(response, response_length, "s", data_offset, out, data_size))
return TPM_LIB_ERROR;
return -EIO; count -= data_size; out += data_size;diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c index 1bf627853a..d7a23ccf7e 100644 --- a/lib/tpm-v2.c +++ b/lib/tpm-v2.c @@ -6,6 +6,7 @@
#include <common.h> #include <dm.h> +#include <rng.h> #include <tpm-common.h> #include <tpm-v2.h> #include <linux/bitops.h> @@ -562,7 +563,7 @@ u32 tpm2_pcr_setauthvalue(struct udevice *dev, const char *pw, return tpm_sendrecv_command(dev, command_v2, NULL, NULL); }
-u32 tpm2_get_random(struct udevice *dev, void *data, u32 count) +int tpm2_get_random(struct udevice *dev, void *data, size_t count) { const u8 command_v2[10] = { tpm_u16(TPM2_ST_NO_SESSIONS), @@ -585,19 +586,19 @@ u32 tpm2_get_random(struct udevice *dev, void *data, u32 count) if (pack_byte_string(buf, sizeof(buf), "sw", 0, command_v2, sizeof(command_v2), sizeof(command_v2), this_bytes))
return TPM_LIB_ERROR;
return -EIO; err = tpm_sendrecv_command(dev, buf, response, &response_length); if (err) return err; if (unpack_byte_string(response, response_length, "w", data_size_offset, &data_size))
return TPM_LIB_ERROR;
return -EIO; if (data_size > this_bytes) return TPM_LIB_ERROR; if (unpack_byte_string(response, response_length, "s", data_offset, out, data_size))
return TPM_LIB_ERROR;
return -EIO; count -= data_size; out += data_size;diff --git a/lib/tpm_api.c b/lib/tpm_api.c index da48058abe..3584fda98c 100644 --- a/lib/tpm_api.c +++ b/lib/tpm_api.c @@ -6,6 +6,7 @@ #include <common.h> #include <dm.h> #include <log.h> +#include <rng.h> #include <tpm_api.h> #include <tpm-v1.h> #include <tpm-v2.h> @@ -265,12 +266,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)
This is the function you should call
My thinking was that the TPM drivers would use the tpm{1,2}_get_random functions directly, while tpm_get_random would be a generic API which could be called from any other module. This API would then call the driver model interface(dm_rng_read). But I see that you want this function to be called from the tpm rng driver. I have made the change in my v5.
{
int ret = -ENOSYS;struct udevice *rng_dev;if (tpm_is_v1(dev))
return tpm1_get_random(dev, data, count);
ret = uclass_get_device_by_driver(UCLASS_RNG,DM_DRIVER_GET(tpm1_rng),&rng_dev);What is going on here? This code should stay the same. Why would you go off and hunt for the device?
else if (tpm_is_v2(dev))
return -ENOSYS; /* not implemented yet */It is implemented above
Yeah I know :). This is existing code which, for some reason, was not calling the tpm2_get_random function. I am fixing this now by removing this line.
elsereturn -ENOSYS;
ret = uclass_get_device_by_driver(UCLASS_RNG,DM_DRIVER_GET(tpm1_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);?? Don't you want to call the TPM to get the random number? I am just lost.
Please see the explanation above.
-sughosh