hi Simon,
Thanks for looking into this. I now have a fair idea of the structure
that you are looking for this interface.
On Wed, 9 Mar 2022 at 08:05, Simon Glass <s...@chromium.org> wrote:
>
> Hi Sugosh,
>
> On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.g...@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.g...@linaro.org>
> > ---
> >
> > Changes since V2:
> >
> > * Export the existing tpm*_get_random functions to the driver model
> > instead of moving them to the drivers/rng/ directory.
> >
> > include/tpm-v1.h | 4 ++--
> > include/tpm-v2.h | 4 ++--
> > lib/tpm-v1.c | 28 ++++++++++++++++++++--------
> > lib/tpm-v2.c | 21 ++++++++++++++++-----
> > lib/tpm_api.c | 23 +++++++++++++++++++----
> > 5 files changed, 59 insertions(+), 21 deletions(-)
> >
> > 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);
> >
> > /**
> > * Lock data in the TPM
> > diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
> > index 22a769c587..57bb4a39cb 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;
> > - err = tpm_sendrecv_command(dev, buf, response,
> > + return -EIO;
> > + err = tpm_sendrecv_command(dev->parent, buf, response,
> > &response_length);
>
> Here it is a bit confused whether dev is the parent tpm device (as the
> comment for this function says), or the random device.
Yes, since this function is being exported as a rng uclass's read
callback, the first argument to this function will indeed be the rng
device. But this can be fixed with the suggestion that you have posted
below.
>
> > 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;
> > @@ -912,3 +914,13 @@ u32 tpm1_get_random(struct udevice *dev, void *data,
> > u32 count)
> >
> > return 0;
> > }
> > +
> > +static const struct dm_rng_ops tpm1_rng_ops = {
> > + .read = tpm1_get_random,
> > +};
> > +
> > +U_BOOT_DRIVER(tpm1_rng) = {
> > + .name = "tpm1-rng",
> > + .id = UCLASS_RNG,
> > + .ops = &tpm1_rng_ops,
> > +};
>
> This declaration and the ops should go in the random driver. There
> should be a op function in that driver which does the API call to the
> TPM. Here you are duplicating this driver in two places.
Actually, I am not duplicating the driver, since I removed the driver
declaration from under drivers/rng/. But I will re-introduce that and
put an API call to tpm like you are suggesting.
>
> Then you don't need to change the signature of tpm1_get_random().
>
> > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> > index 1bf627853a..18f64f0886 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;
> > - err = tpm_sendrecv_command(dev, buf, response,
> > + 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 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;
> > @@ -669,3 +670,13 @@ u32 tpm2_submit_command(struct udevice *dev, const u8
> > *sendbuf,
> > {
> > return tpm_sendrecv_command(dev, sendbuf, recvbuf, recv_size);
> > }
> > +
> > +static const struct dm_rng_ops tpm2_rng_ops = {
> > + .read = tpm2_get_random,
> > +};
> > +
> > +U_BOOT_DRIVER(tpm2_rng) = {
> > + .name = "tpm2-rng",
> > + .id = UCLASS_RNG,
> > + .ops = &tpm2_rng_ops,
> > +};
>
> See above.
>
> > 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)
> > {
> > + 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);
>
> Er, tpm_get_random() should take a tpm device. The random device
> should be handled by the caller, which should call
> tpm_get_random(rand_dev->parent...
Okay. I will make the changes as per your suggestion. Thanks for the
review of the patch.
-sughosh
>
>
> > else if (tpm_is_v2(dev))
> > - return -ENOSYS; /* not implemented yet */
> > - 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);
> > }
> > +#endif /* DM_RNG */
> > --
> > 2.25.1
> >
>
> Regards,
> Simon
