[PATCH v5 3/9] tpm: rng: Add driver model interface for TPM RNG device
Sughosh Ganu
sughosh.ganu at linaro.org
Tue Mar 15 08:47:36 CET 2022
hi Simon,
On Mon, 14 Mar 2022 at 23:55, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Sughosh,
>
> On Mon, 14 Mar 2022 at 05:39, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > 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.
>
> Every other function in the TPM API uses u32 as the return value, so
> it is odd to change this. It also isn't necessary, as I have
> explained. We should aim for consistency.
Well, you have given a R-b on another patch of this series[1] which is
doing exactly the same thing. I really don't understand what is wrong
in bringing the signature of the random byte generation functions in
the TPM API in line with the RNG driver model. But I will not argue
any more on this and revert back the signatures in my next version.
Thanks.
-sughosh
[1] - https://lists.denx.de/pipermail/u-boot/2022-March/476519.html
>
> >
> > >
> > > >
> > > > /**
> > > > * tpm_finalise_physical_presence() - Finalise physical presence
> > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > > > index e79c90b939..4fb1e7a948 100644
> > > > --- a/include/tpm-v2.h
> > > > +++ b/include/tpm-v2.h
> > > > @@ -619,9 +619,9 @@ u32 tpm2_pcr_setauthvalue(struct udevice *dev, const char *pw,
> > > > * @param data output buffer for the random bytes
> > > > * @param count size of output buffer
> > > > *
> > > > - * Return: return code of the operation
> > > > + * Return: 0 if OK, -ve on error
> > > > */
> > > > -u32 tpm2_get_random(struct udevice *dev, void *data, u32 count);
> > > > +int tpm2_get_random(struct udevice *dev, void *data, size_t count);
> > > >
> > > > /**
> > > > * Lock data in the TPM
> > > > diff --git a/include/tpm_api.h b/include/tpm_api.h
> > > > index 4d77a49719..6d9214b335 100644
> > > > --- a/include/tpm_api.h
> > > > +++ b/include/tpm_api.h
> > > > @@ -276,7 +276,7 @@ u32 tpm_find_key_sha1(struct udevice *dev, const u8 auth[20],
> > > > * @param count size of output buffer
> > > > * Return: 0 if OK, -ve on error
> > > > */
> > > > -int tpm_get_random(struct udevice *dev, void *data, u32 count);
> > > > +int tpm_get_random(struct udevice *dev, void *data, size_t count);
> > > >
>
> [..]
>
> Regards,
> Simon
More information about the U-Boot
mailing list