[PATCH v4 3/8] tpm: rng: Add driver model interface for TPM RNG device
Sughosh Ganu
sughosh.ganu at linaro.org
Sun Mar 13 12:18:56 CET 2022
hi Simon,
On Sat, 12 Mar 2022 at 08:14, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Sughosh,
>
> On Wed, 9 Mar 2022 at 05:28, 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.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at 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.
>
> > - else
> > - return -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
More information about the U-Boot
mailing list