[PATCH v5 3/9] tpm: rng: Add driver model interface for TPM RNG device
Sughosh Ganu
sughosh.ganu at linaro.org
Mon Mar 14 12:39:11 CET 2022
hi Simon,
On Mon, 14 Mar 2022 at 03:53, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Sughosh,
>
> On Sun, 13 Mar 2022 at 08:48, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > The TPM device has a builtin random number generator(RNG)
> > functionality. Expose the RNG functions of the TPM device to the
> > driver model so that they can be used by the EFI_RNG_PROTOCOL if the
> > protocol is installed.
> >
> > Also change the function arguments and return type of the random
> > number functions to comply with the driver model api.
>
> Please don't do that
>
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > ---
> >
> > Changes since V4:
> >
> > * Call the existing tpm_get_random API function from the TPM RNG
> > driver, instead of the tpm{1,2}_get_random API's
> > * Introduce a new Kconfig symbol TPM_RNG and build the corresponding
> > driver if the symbol is enabled
> > * Change the last parameter of the tpm_get_random API to have a data
> > type of size_t instead of u32 to comply with the RNG driver model
> > API
> >
> > drivers/rng/Kconfig | 7 +++++++
> > drivers/rng/Makefile | 1 +
> > drivers/rng/tpm_rng.c | 23 +++++++++++++++++++++++
> > include/tpm-v1.h | 4 ++--
> > include/tpm-v2.h | 4 ++--
> > include/tpm_api.h | 2 +-
> > lib/Kconfig | 1 +
> > lib/tpm-v1.c | 16 +++++++++-------
> > lib/tpm-v2.c | 9 +++++----
> > lib/tpm_api.c | 16 +++++++++++-----
> > 10 files changed, 62 insertions(+), 21 deletions(-)
> > create mode 100644 drivers/rng/tpm_rng.c
> >
> > diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
> > index b1c5ab93d1..a89fa99ffa 100644
> > --- a/drivers/rng/Kconfig
> > +++ b/drivers/rng/Kconfig
> > @@ -49,4 +49,11 @@ config RNG_IPROC200
> > depends on DM_RNG
> > help
> > Enable random number generator for RPI4.
> > +
> > +config TPM_RNG
> > + bool "Enable random number generator on TPM device"
> > + depends on TPM
> > + default y
> > + help
> > + Enable random number generator on TPM devices
>
> Needs 3 lines of text so please add more detail
Okay
>
> > endif
> > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> > index 39f7ee3f03..a21f3353ea 100644
> > --- a/drivers/rng/Makefile
> > +++ b/drivers/rng/Makefile
> > @@ -10,3 +10,4 @@ obj-$(CONFIG_RNG_MSM) += msm_rng.o
> > obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
> > obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o
> > obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o
> > +obj-$(CONFIG_TPM_RNG) += tpm_rng.o
> > diff --git a/drivers/rng/tpm_rng.c b/drivers/rng/tpm_rng.c
> > new file mode 100644
> > index 0000000000..69b41dbbf5
> > --- /dev/null
> > +++ b/drivers/rng/tpm_rng.c
> > @@ -0,0 +1,23 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2022, Linaro Limited
> > + */
> > +
> > +#include <dm.h>
> > +#include <rng.h>
> > +#include <tpm_api.h>
> > +
> > +static int rng_tpm_random_read(struct udevice *dev, void *data, size_t count)
> > +{
> > + return tpm_get_random(dev->parent, data, count);
>
> dev_get_parent(dev)
>
> Here you should check the return value and decide whether to return an
> error, such as -EIO
>
> > +}
> > +
> > +static const struct dm_rng_ops tpm_rng_ops = {
> > + .read = rng_tpm_random_read,
> > +};
> > +
> > +U_BOOT_DRIVER(tpm_rng) = {
> > + .name = "tpm-rng",
> > + .id = UCLASS_RNG,
> > + .ops = &tpm_rng_ops,
> > +};
> > diff --git a/include/tpm-v1.h b/include/tpm-v1.h
> > index 33d53fb695..d2ff8b446d 100644
> > --- a/include/tpm-v1.h
> > +++ b/include/tpm-v1.h
> > @@ -555,9 +555,9 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
> > * @param dev TPM device
> > * @param data output buffer for the random bytes
> > * @param count size of output buffer
> > - * Return: return code of the operation
> > + * Return: 0 if OK, -ve on error
> > */
> > -u32 tpm1_get_random(struct udevice *dev, void *data, u32 count);
> > +int tpm1_get_random(struct udevice *dev, void *data, size_t count);
>
> I think I mentioned this last time, but you should not change these
Can you please explain in a little more detail why? I have mentioned
in my earlier email that I am changing this to bring it in line with
the rest of the rng drivers which use a size_t data type for the
number of random bytes to read. What is the issue in changing the
signature? In any case, currently, the api is not being called from
any other module, so it is not like changing the signature is breaking
some code.
>
> >
> > /**
> > * tpm_finalise_physical_presence() - Finalise physical presence
> > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > index e79c90b939..4fb1e7a948 100644
> > --- a/include/tpm-v2.h
> > +++ b/include/tpm-v2.h
> > @@ -619,9 +619,9 @@ u32 tpm2_pcr_setauthvalue(struct udevice *dev, const char *pw,
> > * @param data output buffer for the random bytes
> > * @param count size of output buffer
> > *
> > - * Return: return code of the operation
> > + * Return: 0 if OK, -ve on error
> > */
> > -u32 tpm2_get_random(struct udevice *dev, void *data, u32 count);
> > +int tpm2_get_random(struct udevice *dev, void *data, size_t count);
> >
> > /**
> > * Lock data in the TPM
> > diff --git a/include/tpm_api.h b/include/tpm_api.h
> > index 4d77a49719..6d9214b335 100644
> > --- a/include/tpm_api.h
> > +++ b/include/tpm_api.h
> > @@ -276,7 +276,7 @@ u32 tpm_find_key_sha1(struct udevice *dev, const u8 auth[20],
> > * @param count size of output buffer
> > * Return: 0 if OK, -ve on error
> > */
> > -int tpm_get_random(struct udevice *dev, void *data, u32 count);
> > +int tpm_get_random(struct udevice *dev, void *data, size_t count);
> >
> > /**
> > * tpm_finalise_physical_presence() - Finalise physical presence
> > diff --git a/lib/Kconfig b/lib/Kconfig
> > index 3c6fa99b1a..f04a9c8c79 100644
> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -341,6 +341,7 @@ source lib/crypt/Kconfig
> > config TPM
> > bool "Trusted Platform Module (TPM) Support"
> > depends on DM
> > + imply DM_RNG
> > help
> > This enables support for TPMs which can be used to provide security
> > features for your board. The TPM can be connected via LPC or I2C
> > diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
> > index 22a769c587..4c6bc31a64 100644
> > --- a/lib/tpm-v1.c
> > +++ b/lib/tpm-v1.c
> > @@ -9,12 +9,14 @@
> > #include <common.h>
> > #include <dm.h>
> > #include <log.h>
> > -#include <asm/unaligned.h>
> > -#include <u-boot/sha1.h>
> > +#include <rng.h>
> > #include <tpm-common.h>
> > #include <tpm-v1.h>
> > #include "tpm-utils.h"
> >
> > +#include <asm/unaligned.h>
> > +#include <u-boot/sha1.h>
> > +
> > #ifdef CONFIG_TPM_AUTH_SESSIONS
> >
> > #ifndef CONFIG_SHA1
> > @@ -869,7 +871,7 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
> >
> > #endif /* CONFIG_TPM_AUTH_SESSIONS */
> >
> > -u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
> > +int tpm1_get_random(struct udevice *dev, void *data, size_t count)
> > {
> > const u8 command[14] = {
> > 0x0, 0xc1, /* TPM_TAG */
> > @@ -892,19 +894,19 @@ u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
> > if (pack_byte_string(buf, sizeof(buf), "sd",
> > 0, command, sizeof(command),
> > length_offset, this_bytes))
> > - return TPM_LIB_ERROR;
> > + return -EIO;
>
> No please leave these alone.
Done based on review comments from Heinrich[1].
>
> > err = tpm_sendrecv_command(dev, buf, response,
> > &response_length);
> > if (err)
> > return err;
> > if (unpack_byte_string(response, response_length, "d",
> > data_size_offset, &data_size))
> > - return TPM_LIB_ERROR;
> > + return -EIO;
> > if (data_size > count)
> > - return TPM_LIB_ERROR;
> > + return -EIO;
> > if (unpack_byte_string(response, response_length, "s",
> > data_offset, out, data_size))
> > - return TPM_LIB_ERROR;
> > + return -EIO;
> >
> > count -= data_size;
> > out += data_size;
> > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> > index 1bf627853a..d7a23ccf7e 100644
> > --- a/lib/tpm-v2.c
> > +++ b/lib/tpm-v2.c
> > @@ -6,6 +6,7 @@
> >
> > #include <common.h>
> > #include <dm.h>
> > +#include <rng.h>
> > #include <tpm-common.h>
> > #include <tpm-v2.h>
> > #include <linux/bitops.h>
> > @@ -562,7 +563,7 @@ u32 tpm2_pcr_setauthvalue(struct udevice *dev, const char *pw,
> > return tpm_sendrecv_command(dev, command_v2, NULL, NULL);
> > }
> >
> > -u32 tpm2_get_random(struct udevice *dev, void *data, u32 count)
> > +int tpm2_get_random(struct udevice *dev, void *data, size_t count)
> > {
> > const u8 command_v2[10] = {
> > tpm_u16(TPM2_ST_NO_SESSIONS),
> > @@ -585,19 +586,19 @@ u32 tpm2_get_random(struct udevice *dev, void *data, u32 count)
> > if (pack_byte_string(buf, sizeof(buf), "sw",
> > 0, command_v2, sizeof(command_v2),
> > sizeof(command_v2), this_bytes))
> > - return TPM_LIB_ERROR;
> > + return -EIO;
>
> and here
Same as above.
>
> > err = tpm_sendrecv_command(dev, buf, response,
> > &response_length);
> > if (err)
> > return err;
> > if (unpack_byte_string(response, response_length, "w",
> > data_size_offset, &data_size))
> > - return TPM_LIB_ERROR;
> > + return -EIO;
> > if (data_size > this_bytes)
> > return TPM_LIB_ERROR;
> > if (unpack_byte_string(response, response_length, "s",
> > data_offset, out, data_size))
> > - return TPM_LIB_ERROR;
> > + return -EIO;
> >
> > count -= data_size;
> > out += data_size;
> > diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> > index da48058abe..8bf60cda3c 100644
> > --- a/lib/tpm_api.c
> > +++ b/lib/tpm_api.c
> > @@ -6,6 +6,7 @@
> > #include <common.h>
> > #include <dm.h>
> > #include <log.h>
> > +#include <rng.h>
> > #include <tpm_api.h>
> > #include <tpm-v1.h>
> > #include <tpm-v2.h>
> > @@ -265,12 +266,17 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
> > return -ENOSYS;
> > }
> >
> > -int tpm_get_random(struct udevice *dev, void *data, u32 count)
> > +int tpm_get_random(struct udevice *dev, void *data, size_t count)
> > {
> > + int ret = -ENOSYS;
> > +
> > if (tpm_is_v1(dev))
> > - return tpm1_get_random(dev, data, count);
> > + ret = tpm1_get_random(dev, data, count);
> > else if (tpm_is_v2(dev))
> > - return -ENOSYS; /* not implemented yet */
> > - else
> > - return -ENOSYS;
> > + ret = tpm2_get_random(dev, data, count);
> > +
> > + if (ret)
> > + log_err("Failed to read random bytes\n");
>
> I don't see this in the other functions in this file. Why add it? It
> just bloats the code and there is no way for the caller to suppress
> the message.
Okay
-sughosh
[1] - https://lists.denx.de/pipermail/u-boot/2022-February/476085.html
More information about the U-Boot
mailing list