[PATCH 00/10] tpm: rng: Move TPM RNG functionality to driver model

[PATCH 00/10] tpm: rng: Move TPM RNG functionality to driver model

[Sughosh Ganu]

There was a discussion on the mailing list earlier[1], where it was
explained that platforms with a TPM device can install the
EFI_RNG_PROTOCOL for getting the random bytes instead of populating
the dtb with the kaslr-seed property. That would make it possible to
measure the dtb.

This patchset moves the already existing functions for getting random
bytes from the TPM device to drivers complying with the RNG
uclass. This is done since the EFI_RNG_PROTOCOL's get_rng routine uses
the RNG uclass's dm_rng_read api to get the random bytes.

The TPM uclass driver adds the RNG child device as part of it's
post_probe function. The TPM uclass driver's child_pre_probe function
initialises the TPM parent device for use -- this enables the RNG
child device to be used subsequently.

Some additional changes have also been made to facilitate the
use of the RNG devices, including extending the 'rng' command to take
the RNG device as one of the command-line parameters.

[1] - https://lore.kernel.org/u-boot/20220103120738.47835-1-ilias.apalodimas@linaro.org/


Sughosh Ganu (10):
  tpm: Move tpm-utils header under the include directory
  tpm: rng: Change tpm_get_random to return an int
  tpm: Fix the return type of tpm_startup
  tpm: Move the TPM version detection functions to the uclass driver
  configs: gazerbeam: Build TPMV2 library routines
  configs: chromebook_coral: Build TPMV1 library routines
  tpm: rng: Move the TPM RNG functionality to driver model
  tpm: Add the RNG child device
  qemu: arm: Remove platform specific function to get RNG device
  cmd: rng: Add support for selecting RNG device

 board/emulation/qemu-arm/qemu-arm.c | 42 ------------------
 cmd/rng.c                           | 31 +++++++++----
 configs/chromebook_coral_defconfig  |  1 -
 configs/gazerbeam_defconfig         |  1 -
 drivers/rng/Makefile                |  1 +
 drivers/rng/tpm1_rng.c              | 68 ++++++++++++++++++++++++++++
 drivers/rng/tpm2_rng.c              | 68 ++++++++++++++++++++++++++++
 drivers/tpm/tpm-uclass.c            | 69 +++++++++++++++++++++++++++--
 {lib => include}/tpm-utils.h        |  0
 include/tpm_api.h                   | 26 +++++++++--
 lib/tpm-common.c                    |  2 +-
 lib/tpm-v1.c                        | 46 +------------------
 lib/tpm-v2.c                        | 46 +------------------
 lib/tpm_api.c                       | 42 +++++++++++-------
 14 files changed, 276 insertions(+), 167 deletions(-)
 create mode 100644 drivers/rng/tpm1_rng.c
 create mode 100644 drivers/rng/tpm2_rng.c
 rename {lib => include}/tpm-utils.h (100%)

-- 
2.17.1

[PATCH 01/10] tpm: Move tpm-utils header under the include directory

[Sughosh Ganu]

The random number generation functions of TPM will be moved under a
dedicated driver. With this, the function declarations along with
some other relevant macro definitions need to be moved under a
common header file directory. Move the tpm-utils.h header file under
the common include directory.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 {lib => include}/tpm-utils.h | 0
 lib/tpm-common.c             | 2 +-
 lib/tpm-v1.c                 | 2 +-
 lib/tpm-v2.c                 | 2 +-
 4 files changed, 3 insertions(+), 3 deletions(-)
 rename {lib => include}/tpm-utils.h (100%)

diff --git a/lib/tpm-utils.h b/include/tpm-utils.h
similarity index 100%
rename from lib/tpm-utils.h
rename to include/tpm-utils.h
diff --git a/lib/tpm-common.c b/lib/tpm-common.c
index 82ffdc5341..26506f0b99 100644
--- a/lib/tpm-common.c
+++ b/lib/tpm-common.c
@@ -11,7 +11,7 @@
 #include <log.h>
 #include <asm/unaligned.h>
 #include <tpm-common.h>
-#include "tpm-utils.h"
+#include <tpm-utils.h>
 
 enum tpm_version tpm_get_version(struct udevice *dev)
 {
diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
index 22a769c587..467992e04e 100644
--- a/lib/tpm-v1.c
+++ b/lib/tpm-v1.c
@@ -13,7 +13,7 @@
 #include <u-boot/sha1.h>
 #include <tpm-common.h>
 #include <tpm-v1.h>
-#include "tpm-utils.h"
+#include <tpm-utils.h>
 
 #ifdef CONFIG_TPM_AUTH_SESSIONS
 
diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
index 1bf627853a..2f16b0007b 100644
--- a/lib/tpm-v2.c
+++ b/lib/tpm-v2.c
@@ -9,7 +9,7 @@
 #include <tpm-common.h>
 #include <tpm-v2.h>
 #include <linux/bitops.h>
-#include "tpm-utils.h"
+#include <tpm-utils.h>
 
 u32 tpm2_startup(struct udevice *dev, enum tpm2_startup_types mode)
 {
-- 
2.17.1

Re: [PATCH 01/10] tpm: Move tpm-utils header under the include directory

[Heinrich Schuchardt]

On 2/24/22 19:05, Sughosh Ganu wrote:
> The random number generation functions of TPM will be moved under a
> dedicated driver. With this, the function declarations along with
> some other relevant macro definitions need to be moved under a
> common header file directory. Move the tpm-utils.h header file under
> the common include directory.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>   {lib => include}/tpm-utils.h | 0
>   lib/tpm-common.c             | 2 +-
>   lib/tpm-v1.c                 | 2 +-
>   lib/tpm-v2.c                 | 2 +-
>   4 files changed, 3 insertions(+), 3 deletions(-)
>   rename {lib => include}/tpm-utils.h (100%)
>
> diff --git a/lib/tpm-utils.h b/include/tpm-utils.h
> similarity index 100%
> rename from lib/tpm-utils.h
> rename to include/tpm-utils.h
> diff --git a/lib/tpm-common.c b/lib/tpm-common.c
> index 82ffdc5341..26506f0b99 100644
> --- a/lib/tpm-common.c
> +++ b/lib/tpm-common.c
> @@ -11,7 +11,7 @@
>   #include <log.h>
>   #include <asm/unaligned.h>
>   #include <tpm-common.h>
> -#include "tpm-utils.h"
> +#include <tpm-utils.h>
>
>   enum tpm_version tpm_get_version(struct udevice *dev)
>   {
> diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
> index 22a769c587..467992e04e 100644
> --- a/lib/tpm-v1.c
> +++ b/lib/tpm-v1.c
> @@ -13,7 +13,7 @@
>   #include <u-boot/sha1.h>
>   #include <tpm-common.h>
>   #include <tpm-v1.h>
> -#include "tpm-utils.h"
> +#include <tpm-utils.h>
>
>   #ifdef CONFIG_TPM_AUTH_SESSIONS
>
> diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> index 1bf627853a..2f16b0007b 100644
> --- a/lib/tpm-v2.c
> +++ b/lib/tpm-v2.c
> @@ -9,7 +9,7 @@
>   #include <tpm-common.h>
>   #include <tpm-v2.h>
>   #include <linux/bitops.h>
> -#include "tpm-utils.h"
> +#include <tpm-utils.h>
>
>   u32 tpm2_startup(struct udevice *dev, enum tpm2_startup_types mode)
>   {

Re: [PATCH 01/10] tpm: Move tpm-utils header under the include directory

[Ilias Apalodimas]

On Thu, Feb 24, 2022 at 11:35:43PM +0530, Sughosh Ganu wrote:
> The random number generation functions of TPM will be moved under a
> dedicated driver. With this, the function declarations along with
> some other relevant macro definitions need to be moved under a
> common header file directory. Move the tpm-utils.h header file under
> the common include directory.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  {lib => include}/tpm-utils.h | 0
>  lib/tpm-common.c             | 2 +-
>  lib/tpm-v1.c                 | 2 +-
>  lib/tpm-v2.c                 | 2 +-
>  4 files changed, 3 insertions(+), 3 deletions(-)
>  rename {lib => include}/tpm-utils.h (100%)
> 
> diff --git a/lib/tpm-utils.h b/include/tpm-utils.h
> similarity index 100%
> rename from lib/tpm-utils.h
> rename to include/tpm-utils.h
> diff --git a/lib/tpm-common.c b/lib/tpm-common.c
> index 82ffdc5341..26506f0b99 100644
> --- a/lib/tpm-common.c
> +++ b/lib/tpm-common.c
> @@ -11,7 +11,7 @@
>  #include <log.h>
>  #include <asm/unaligned.h>
>  #include <tpm-common.h>
> -#include "tpm-utils.h"
> +#include <tpm-utils.h>
>  
>  enum tpm_version tpm_get_version(struct udevice *dev)
>  {
> diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
> index 22a769c587..467992e04e 100644
> --- a/lib/tpm-v1.c
> +++ b/lib/tpm-v1.c
> @@ -13,7 +13,7 @@
>  #include <u-boot/sha1.h>
>  #include <tpm-common.h>
>  #include <tpm-v1.h>
> -#include "tpm-utils.h"
> +#include <tpm-utils.h>
>  
>  #ifdef CONFIG_TPM_AUTH_SESSIONS
>  
> diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> index 1bf627853a..2f16b0007b 100644
> --- a/lib/tpm-v2.c
> +++ b/lib/tpm-v2.c
> @@ -9,7 +9,7 @@
>  #include <tpm-common.h>
>  #include <tpm-v2.h>
>  #include <linux/bitops.h>
> -#include "tpm-utils.h"
> +#include <tpm-utils.h>
>  
>  u32 tpm2_startup(struct udevice *dev, enum tpm2_startup_types mode)
>  {
> -- 
> 2.17.1
> 

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH 02/10] tpm: rng: Change tpm_get_random to return an int

[Sughosh Ganu]

The tpm random number generation functionality will be moved to the
driver model. With that, the tpm_get_random function will call the
common driver model api instead of separate functions for tpmv1 and
tpmv2. Return an int instead of a u32 to comply with the return value
of the driver model function.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 include/tpm_api.h | 4 ++--
 lib/tpm_api.c     | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/tpm_api.h b/include/tpm_api.h
index ef45b43a8f..b9e3e8b5e6 100644
--- a/include/tpm_api.h
+++ b/include/tpm_api.h
@@ -274,9 +274,9 @@ u32 tpm_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 tpm_get_random(struct udevice *dev, void *data, u32 count);
+int tpm_get_random(struct udevice *dev, void *data, u32 count);
 
 /**
  * tpm_finalise_physical_presence() - Finalise physical presence
diff --git a/lib/tpm_api.c b/lib/tpm_api.c
index 4c662640a9..1bbe33a3fc 100644
--- a/lib/tpm_api.c
+++ b/lib/tpm_api.c
@@ -274,7 +274,7 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
 		return -ENOSYS;
 }
 
-u32 tpm_get_random(struct udevice *dev, void *data, u32 count)
+int tpm_get_random(struct udevice *dev, void *data, u32 count)
 {
 	if (is_tpm1(dev))
 		return tpm1_get_random(dev, data, count);
-- 
2.17.1

Re: [PATCH 02/10] tpm: rng: Change tpm_get_random to return an int

[Heinrich Schuchardt]

On 2/24/22 19:05, Sughosh Ganu wrote:
> The tpm random number generation functionality will be moved to the
> driver model. With that, the tpm_get_random function will call the
> common driver model api instead of separate functions for tpmv1 and
> tpmv2. Return an int instead of a u32 to comply with the return value
> of the driver model function.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>   include/tpm_api.h | 4 ++--
>   lib/tpm_api.c     | 2 +-
>   2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/tpm_api.h b/include/tpm_api.h
> index ef45b43a8f..b9e3e8b5e6 100644
> --- a/include/tpm_api.h
> +++ b/include/tpm_api.h
> @@ -274,9 +274,9 @@ u32 tpm_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 tpm_get_random(struct udevice *dev, void *data, u32 count);
> +int tpm_get_random(struct udevice *dev, void *data, u32 count);
>
>   /**
>    * tpm_finalise_physical_presence() - Finalise physical presence
> diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> index 4c662640a9..1bbe33a3fc 100644
> --- a/lib/tpm_api.c
> +++ b/lib/tpm_api.c
> @@ -274,7 +274,7 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
>   		return -ENOSYS;
>   }
>
> -u32 tpm_get_random(struct udevice *dev, void *data, u32 count)
> +int tpm_get_random(struct udevice *dev, void *data, u32 count)
>   {
>   	if (is_tpm1(dev))
>   		return tpm1_get_random(dev, data, count);

Re: [PATCH 02/10] tpm: rng: Change tpm_get_random to return an int

[Ilias Apalodimas]

On Thu, 24 Feb 2022 at 20:06, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The tpm random number generation functionality will be moved to the
> driver model. With that, the tpm_get_random function will call the
> common driver model api instead of separate functions for tpmv1 and
> tpmv2. Return an int instead of a u32 to comply with the return value
> of the driver model function.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  include/tpm_api.h | 4 ++--
>  lib/tpm_api.c     | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/tpm_api.h b/include/tpm_api.h
> index ef45b43a8f..b9e3e8b5e6 100644
> --- a/include/tpm_api.h
> +++ b/include/tpm_api.h
> @@ -274,9 +274,9 @@ u32 tpm_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 tpm_get_random(struct udevice *dev, void *data, u32 count);
> +int tpm_get_random(struct udevice *dev, void *data, u32 count);
>
>  /**
>   * tpm_finalise_physical_presence() - Finalise physical presence
> diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> index 4c662640a9..1bbe33a3fc 100644
> --- a/lib/tpm_api.c
> +++ b/lib/tpm_api.c
> @@ -274,7 +274,7 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
>                 return -ENOSYS;
>  }
>
> -u32 tpm_get_random(struct udevice *dev, void *data, u32 count)
> +int tpm_get_random(struct udevice *dev, void *data, u32 count)
>  {
>         if (is_tpm1(dev))
>                 return tpm1_get_random(dev, data, count);
> --
> 2.17.1
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH 03/10] tpm: Fix the return type of tpm_startup

[Sughosh Ganu]

The tpm_startup function returns negative values for error
conditions. Fix the return type of the function to a signed int
instead of a u32.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 include/tpm_api.h | 2 +-
 lib/tpm_api.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/tpm_api.h b/include/tpm_api.h
index b9e3e8b5e6..fb6ee14e23 100644
--- a/include/tpm_api.h
+++ b/include/tpm_api.h
@@ -18,7 +18,7 @@
  * @param mode		TPM startup mode
  * Return: return code of the operation
  */
-u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode);
+int tpm_startup(struct udevice *dev, enum tpm_startup_type mode);
 
 /**
  * Issue a TPM_SelfTestFull command.
diff --git a/lib/tpm_api.c b/lib/tpm_api.c
index 1bbe33a3fc..b762202866 100644
--- a/lib/tpm_api.c
+++ b/lib/tpm_api.c
@@ -21,7 +21,7 @@ static bool is_tpm2(struct udevice *dev)
 	return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
 }
 
-u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
+int tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
 {
 	if (is_tpm1(dev)) {
 		return tpm1_startup(dev, mode);
-- 
2.17.1

Re: [PATCH 03/10] tpm: Fix the return type of tpm_startup

[Heinrich Schuchardt]

On 2/24/22 19:05, Sughosh Ganu wrote:
> The tpm_startup function returns negative values for error
> conditions. Fix the return type of the function to a signed int
> instead of a u32.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   include/tpm_api.h | 2 +-
>   lib/tpm_api.c     | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/tpm_api.h b/include/tpm_api.h
> index b9e3e8b5e6..fb6ee14e23 100644
> --- a/include/tpm_api.h
> +++ b/include/tpm_api.h
> @@ -18,7 +18,7 @@
>    * @param mode		TPM startup mode
>    * Return: return code of the operation

Should this become:

Return: 0 for success, -ve in case of error ?

If we would stop at after this patch, TPM errors would be returned as
-EPERM (= TPM_LIB_ERROR). So maybe this patch should be merged with a
later patch.

Best regards

Heinrich

>    */
> -u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode);
> +int tpm_startup(struct udevice *dev, enum tpm_startup_type mode);
>
>   /**
>    * Issue a TPM_SelfTestFull command.
> diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> index 1bbe33a3fc..b762202866 100644
> --- a/lib/tpm_api.c
> +++ b/lib/tpm_api.c
> @@ -21,7 +21,7 @@ static bool is_tpm2(struct udevice *dev)
>   	return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
>   }
>
> -u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
> +int tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
>   {
>   	if (is_tpm1(dev)) {
>   		return tpm1_startup(dev, mode);

Re: [PATCH 03/10] tpm: Fix the return type of tpm_startup

[Sughosh Ganu]

hello Heinrich,

On Fri, 25 Feb 2022 at 00:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 2/24/22 19:05, Sughosh Ganu wrote:
> > The tpm_startup function returns negative values for error
> > conditions. Fix the return type of the function to a signed int
> > instead of a u32.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   include/tpm_api.h | 2 +-
> >   lib/tpm_api.c     | 2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/tpm_api.h b/include/tpm_api.h
> > index b9e3e8b5e6..fb6ee14e23 100644
> > --- a/include/tpm_api.h
> > +++ b/include/tpm_api.h
> > @@ -18,7 +18,7 @@
> >    * @param mode              TPM startup mode
> >    * Return: return code of the operation
>
> Should this become:
>
> Return: 0 for success, -ve in case of error ?

I think there are two types of return codes coming from tpm_startup.
One is the negative values which are primarily for handling error
conditions like invalid parameters, unsupported tpm chip version etc.
The other error codes are the response from the tpm chip for the
requested operation. These are non-negative values. For a successful
response though, both tpmv1 and tpmv2 response is a 0x0. Should this
be

Return: 0 for success, -EINVAL for invalid parameters, -ENOSYS for
unsupported TPM version, TPM error codes

>
> If we would stop at after this patch, TPM errors would be returned as
> -EPERM (= TPM_LIB_ERROR). So maybe this patch should be merged with a
> later patch.

Sorry, but I don't get this. The patch is changing the return type for
tpm_startup. I think this function is not returning TPM_LIB_ERROR --
that is returned by the RNG functions of the TPM device.

-sughosh

>
> Best regards
>
> Heinrich
>
> >    */
> > -u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode);
> > +int tpm_startup(struct udevice *dev, enum tpm_startup_type mode);
> >
> >   /**
> >    * Issue a TPM_SelfTestFull command.
> > diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> > index 1bbe33a3fc..b762202866 100644
> > --- a/lib/tpm_api.c
> > +++ b/lib/tpm_api.c
> > @@ -21,7 +21,7 @@ static bool is_tpm2(struct udevice *dev)
> >       return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
> >   }
> >
> > -u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
> > +int tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
> >   {
> >       if (is_tpm1(dev)) {
> >               return tpm1_startup(dev, mode);
>

[PATCH 04/10] tpm: Move the TPM version detection functions to the uclass driver

[Sughosh Ganu]

Make the TPM version detection functions as external symbols and move
them to the TPM uclass driver. These are useful functions to check the
TPM device version and should not be static functions.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 drivers/tpm/tpm-uclass.c | 11 +++++++++++
 include/tpm_api.h        | 20 ++++++++++++++++++++
 lib/tpm_api.c            | 10 ----------
 3 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
index f67fe1019b..8619da89d8 100644
--- a/drivers/tpm/tpm-uclass.c
+++ b/drivers/tpm/tpm-uclass.c
@@ -11,10 +11,21 @@
 #include <log.h>
 #include <linux/delay.h>
 #include <linux/unaligned/be_byteshift.h>
+#include <tpm_api.h>
 #include <tpm-v1.h>
 #include <tpm-v2.h>
 #include "tpm_internal.h"
 
+bool is_tpm1(struct udevice *dev)
+{
+	return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
+}
+
+bool is_tpm2(struct udevice *dev)
+{
+	return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
+}
+
 int tpm_open(struct udevice *dev)
 {
 	struct tpm_ops *ops = tpm_get_ops(dev);
diff --git a/include/tpm_api.h b/include/tpm_api.h
index fb6ee14e23..c19639a688 100644
--- a/include/tpm_api.h
+++ b/include/tpm_api.h
@@ -11,6 +11,26 @@
 #include <tpm-v1.h>
 #include <tpm-v2.h>
 
+/**
+ * is_tpm1() - Check if it is a tpmv1 device
+ * @param dev		TPM device
+ *
+ * Check if the TPM device is a TPMv1 device
+ *
+ * Return: 1 if TPMv1, 0 otherwise
+ */
+bool is_tpm1(struct udevice *dev);
+
+/**
+ * is_tpm2() - Check if it is a tpmv2 device
+ * @param dev		TPM device
+ *
+ * Check if the TPM device is a TPMv2 device
+ *
+ * Return: 1 if TPMv2, 0 otherwise
+ */
+bool is_tpm2(struct udevice *dev);
+
 /**
  * Issue a TPM_Startup command.
  *
diff --git a/lib/tpm_api.c b/lib/tpm_api.c
index b762202866..9dd9606fa8 100644
--- a/lib/tpm_api.c
+++ b/lib/tpm_api.c
@@ -11,16 +11,6 @@
 #include <tpm-v2.h>
 #include <tpm_api.h>
 
-static bool is_tpm1(struct udevice *dev)
-{
-	return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
-}
-
-static bool is_tpm2(struct udevice *dev)
-{
-	return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
-}
-
 int tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
 {
 	if (is_tpm1(dev)) {
-- 
2.17.1

Re: [PATCH 04/10] tpm: Move the TPM version detection functions to the uclass driver

[Heinrich Schuchardt]

On 2/24/22 19:05, Sughosh Ganu wrote:
> Make the TPM version detection functions as external symbols and move
> them to the TPM uclass driver. These are useful functions to check the
> TPM device version and should not be static functions.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>   drivers/tpm/tpm-uclass.c | 11 +++++++++++
>   include/tpm_api.h        | 20 ++++++++++++++++++++
>   lib/tpm_api.c            | 10 ----------
>   3 files changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> index f67fe1019b..8619da89d8 100644
> --- a/drivers/tpm/tpm-uclass.c
> +++ b/drivers/tpm/tpm-uclass.c
> @@ -11,10 +11,21 @@
>   #include <log.h>
>   #include <linux/delay.h>
>   #include <linux/unaligned/be_byteshift.h>
> +#include <tpm_api.h>
>   #include <tpm-v1.h>
>   #include <tpm-v2.h>
>   #include "tpm_internal.h"
>
> +bool is_tpm1(struct udevice *dev)
> +{
> +	return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> +}
> +
> +bool is_tpm2(struct udevice *dev)
> +{
> +	return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
> +}
> +
>   int tpm_open(struct udevice *dev)
>   {
>   	struct tpm_ops *ops = tpm_get_ops(dev);
> diff --git a/include/tpm_api.h b/include/tpm_api.h
> index fb6ee14e23..c19639a688 100644
> --- a/include/tpm_api.h
> +++ b/include/tpm_api.h
> @@ -11,6 +11,26 @@
>   #include <tpm-v1.h>
>   #include <tpm-v2.h>
>
> +/**
> + * is_tpm1() - Check if it is a tpmv1 device
> + * @param dev		TPM device
> + *
> + * Check if the TPM device is a TPMv1 device
> + *
> + * Return: 1 if TPMv1, 0 otherwise
> + */
> +bool is_tpm1(struct udevice *dev);
> +
> +/**
> + * is_tpm2() - Check if it is a tpmv2 device
> + * @param dev		TPM device
> + *
> + * Check if the TPM device is a TPMv2 device
> + *
> + * Return: 1 if TPMv2, 0 otherwise
> + */
> +bool is_tpm2(struct udevice *dev);
> +
>   /**
>    * Issue a TPM_Startup command.
>    *
> diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> index b762202866..9dd9606fa8 100644
> --- a/lib/tpm_api.c
> +++ b/lib/tpm_api.c
> @@ -11,16 +11,6 @@
>   #include <tpm-v2.h>
>   #include <tpm_api.h>
>
> -static bool is_tpm1(struct udevice *dev)
> -{
> -	return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> -}
> -
> -static bool is_tpm2(struct udevice *dev)
> -{
> -	return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
> -}
> -
>   int tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
>   {
>   	if (is_tpm1(dev)) {

[PATCH 05/10] configs: gazerbeam: Build TPMV2 library routines

[Sughosh Ganu]

The TPM code currently does a runtime detection of the TPM version and
calls appropriate functions. Gazerbeam is one of the platforms where
the TPMV2 code is disabled at build time. With this, calling TPM api's
from the TPM uclass driver results in link errors. Enable TPMV2
library routines and determine the TPM version at runtime like other
platforms.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 configs/gazerbeam_defconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configs/gazerbeam_defconfig b/configs/gazerbeam_defconfig
index 5d8d1998d6..505cd49843 100644
--- a/configs/gazerbeam_defconfig
+++ b/configs/gazerbeam_defconfig
@@ -213,7 +213,6 @@ CONFIG_TIMER=y
 CONFIG_MPC83XX_TIMER=y
 CONFIG_TPM_ATMEL_TWI=y
 CONFIG_TPM_AUTH_SESSIONS=y
-# CONFIG_TPM_V2 is not set
 CONFIG_DM_VIDEO=y
 CONFIG_DISPLAY=y
 CONFIG_LOGICORE_DP_TX=y
-- 
2.17.1

[PATCH 06/10] configs: chromebook_coral: Build TPMV1 library routines

[Sughosh Ganu]

The TPM code currently does a runtime detection of the TPM version and
calls appropriate functions. Chromebook Coral is one of the platforms
where the TPMV1 code is disabled at build time. With this, calling TPM
api's from the TPM uclass driver results in link errors. Enable TPMV1
library routines and determine the TPM version at runtime like other
platforms.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 configs/chromebook_coral_defconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configs/chromebook_coral_defconfig b/configs/chromebook_coral_defconfig
index 0cd8f39aa3..4704ce25c8 100644
--- a/configs/chromebook_coral_defconfig
+++ b/configs/chromebook_coral_defconfig
@@ -104,7 +104,6 @@ CONFIG_SPI=y
 CONFIG_ICH_SPI=y
 # CONFIG_SYSINFO_SMBIOS is not set
 CONFIG_TPL_SYSRESET=y
-# CONFIG_TPM_V1 is not set
 CONFIG_TPM2_CR50_I2C=y
 CONFIG_USB_XHCI_HCD=y
 CONFIG_USB_STORAGE=y
-- 
2.17.1

[PATCH 07/10] tpm: rng: Move the TPM RNG functionality to driver model

[Sughosh Ganu]

Currently, the TPM random number generator(RNG) functions are defined
as part of the library functions under the corresponding tpm files for
tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers
complying with the driver model.

Also make changes to the tpm_get_random function to have it call the
TPM RNG driver functions instead of the library functions.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 drivers/rng/Makefile   |  1 +
 drivers/rng/tpm1_rng.c | 68 ++++++++++++++++++++++++++++++++++++++++++
 drivers/rng/tpm2_rng.c | 68 ++++++++++++++++++++++++++++++++++++++++++
 lib/tpm-v1.c           | 44 ---------------------------
 lib/tpm-v2.c           | 44 ---------------------------
 lib/tpm_api.c          | 28 +++++++++++++----
 6 files changed, 160 insertions(+), 93 deletions(-)
 create mode 100644 drivers/rng/tpm1_rng.c
 create mode 100644 drivers/rng/tpm2_rng.c

diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
index 39f7ee3f03..129cfbd006 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) += tpm1_rng.o tpm2_rng.o
diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c
new file mode 100644
index 0000000000..ddb20b008d
--- /dev/null
+++ b/drivers/rng/tpm1_rng.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2022, Linaro Limited
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <rng.h>
+#include <tpm-utils.h>
+#include <tpm-v1.h>
+
+#define TPM_HEADER_SIZE		10
+
+#define TPMV1_DATA_OFFSET	14
+
+static int tpm1_rng_read(struct udevice *dev, void *data, size_t count)
+{
+	const u8 command[14] = {
+		0x0, 0xc1,		/* TPM_TAG */
+		0x0, 0x0, 0x0, 0xe,	/* parameter size */
+		0x0, 0x0, 0x0, 0x46,	/* TPM_COMMAND_CODE */
+	};
+	const size_t length_offset = TPM_HEADER_SIZE;
+	const size_t data_size_offset = TPM_HEADER_SIZE;
+	const size_t data_offset = TPMV1_DATA_OFFSET;
+	u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
+	size_t response_length = sizeof(response);
+	u32 data_size;
+	u8 *out = data;
+
+	while (count > 0) {
+		u32 this_bytes = min(count,
+				     sizeof(response) - data_offset);
+		u32 err;
+
+		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->parent, 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;
+		if (data_size > count)
+			return TPM_LIB_ERROR;
+		if (unpack_byte_string(response, response_length, "s",
+				       data_offset, out, data_size))
+			return TPM_LIB_ERROR;
+
+		count -= data_size;
+		out += data_size;
+	}
+
+	return 0;
+}
+
+static const struct dm_rng_ops tpm1_rng_ops = {
+	.read = tpm1_rng_read,
+};
+
+U_BOOT_DRIVER(tpm1_rng) = {
+	.name		= "tpm1-rng",
+	.id		= UCLASS_RNG,
+	.ops		= &tpm1_rng_ops,
+};
diff --git a/drivers/rng/tpm2_rng.c b/drivers/rng/tpm2_rng.c
new file mode 100644
index 0000000000..f14528f751
--- /dev/null
+++ b/drivers/rng/tpm2_rng.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2022, Linaro Limited
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <rng.h>
+#include <tpm-utils.h>
+#include <tpm-v2.h>
+
+#define TPM_HEADER_SIZE		10
+
+#define TPMV2_DATA_OFFSET	12
+
+static int tpm2_rng_read(struct udevice *dev, void *data, size_t count)
+{
+	const u8 command_v2[10] = {
+		tpm_u16(TPM2_ST_NO_SESSIONS),
+		tpm_u32(12),
+		tpm_u32(TPM2_CC_GET_RANDOM),
+	};
+	u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
+
+	const size_t data_size_offset = TPM_HEADER_SIZE;
+	const size_t data_offset = TPMV2_DATA_OFFSET;
+	size_t response_length = sizeof(response);
+	u32 data_size;
+	u8 *out = data;
+
+	while (count > 0) {
+		u32 this_bytes = min((size_t)count,
+				     sizeof(response) - data_offset);
+		u32 err;
+
+		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->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;
+		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;
+
+		count -= data_size;
+		out += data_size;
+	}
+
+	return 0;
+}
+
+static const struct dm_rng_ops tpm2_rng_ops = {
+	.read = tpm2_rng_read,
+};
+
+U_BOOT_DRIVER(tpm2_rng) = {
+	.name		= "tpm2-rng",
+	.id		= UCLASS_RNG,
+	.ops		= &tpm2_rng_ops,
+};
diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
index 467992e04e..71cc90a2ab 100644
--- a/lib/tpm-v1.c
+++ b/lib/tpm-v1.c
@@ -868,47 +868,3 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
 #endif /* CONFIG_TPM_LOAD_KEY_BY_SHA1 */
 
 #endif /* CONFIG_TPM_AUTH_SESSIONS */
-
-u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
-{
-	const u8 command[14] = {
-		0x0, 0xc1,		/* TPM_TAG */
-		0x0, 0x0, 0x0, 0xe,	/* parameter size */
-		0x0, 0x0, 0x0, 0x46,	/* TPM_COMMAND_CODE */
-	};
-	const size_t length_offset = 10;
-	const size_t data_size_offset = 10;
-	const size_t data_offset = 14;
-	u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
-	size_t response_length = sizeof(response);
-	u32 data_size;
-	u8 *out = data;
-
-	while (count > 0) {
-		u32 this_bytes = min((size_t)count,
-				     sizeof(response) - data_offset);
-		u32 err;
-
-		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,
-					   &response_length);
-		if (err)
-			return err;
-		if (unpack_byte_string(response, response_length, "d",
-				       data_size_offset, &data_size))
-			return TPM_LIB_ERROR;
-		if (data_size > count)
-			return TPM_LIB_ERROR;
-		if (unpack_byte_string(response, response_length, "s",
-				       data_offset, out, data_size))
-			return TPM_LIB_ERROR;
-
-		count -= data_size;
-		out += data_size;
-	}
-
-	return 0;
-}
diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
index 2f16b0007b..c1456c1974 100644
--- a/lib/tpm-v2.c
+++ b/lib/tpm-v2.c
@@ -562,50 +562,6 @@ 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)
-{
-	const u8 command_v2[10] = {
-		tpm_u16(TPM2_ST_NO_SESSIONS),
-		tpm_u32(12),
-		tpm_u32(TPM2_CC_GET_RANDOM),
-	};
-	u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
-
-	const size_t data_size_offset = 10;
-	const size_t data_offset = 12;
-	size_t response_length = sizeof(response);
-	u32 data_size;
-	u8 *out = data;
-
-	while (count > 0) {
-		u32 this_bytes = min((size_t)count,
-				     sizeof(response) - data_offset);
-		u32 err;
-
-		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,
-					   &response_length);
-		if (err)
-			return err;
-		if (unpack_byte_string(response, response_length, "w",
-				       data_size_offset, &data_size))
-			return TPM_LIB_ERROR;
-		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;
-
-		count -= data_size;
-		out += data_size;
-	}
-
-	return 0;
-}
-
 u32 tpm2_write_lock(struct udevice *dev, u32 index)
 {
 	u8 command_v2[COMMAND_BUFFER_SIZE] = {
diff --git a/lib/tpm_api.c b/lib/tpm_api.c
index 9dd9606fa8..de822113b0 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>
@@ -264,12 +265,29 @@ 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)
 {
-	if (is_tpm1(dev))
-		return tpm1_get_random(dev, data, count);
-	else if (is_tpm2(dev))
-		return -ENOSYS; /* not implemented yet */
-	else
+	int ret;
+	struct udevice *rng_dev;
+
+	if (is_tpm1(dev)) {
+		ret = uclass_get_device_by_driver(UCLASS_RNG,
+						  DM_DRIVER_GET(tpm1_rng),
+						  &rng_dev);
+	} else if (is_tpm2(dev)) {
+		ret = uclass_get_device_by_driver(UCLASS_RNG,
+						  DM_DRIVER_GET(tpm2_rng),
+						  &rng_dev);
+	} else {
 		return -ENOSYS;
+	}
+
+	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.17.1

Re: [PATCH 07/10] tpm: rng: Move the TPM RNG functionality to driver model

[Heinrich Schuchardt]

On 2/24/22 19:05, Sughosh Ganu wrote:
> Currently, the TPM random number generator(RNG) functions are defined
> as part of the library functions under the corresponding tpm files for
> tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers
> complying with the driver model.
>
> Also make changes to the tpm_get_random function to have it call the
> TPM RNG driver functions instead of the library functions.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   drivers/rng/Makefile   |  1 +
>   drivers/rng/tpm1_rng.c | 68 ++++++++++++++++++++++++++++++++++++++++++
>   drivers/rng/tpm2_rng.c | 68 ++++++++++++++++++++++++++++++++++++++++++
>   lib/tpm-v1.c           | 44 ---------------------------
>   lib/tpm-v2.c           | 44 ---------------------------
>   lib/tpm_api.c          | 28 +++++++++++++----
>   6 files changed, 160 insertions(+), 93 deletions(-)
>   create mode 100644 drivers/rng/tpm1_rng.c
>   create mode 100644 drivers/rng/tpm2_rng.c
>
> diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> index 39f7ee3f03..129cfbd006 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) += tpm1_rng.o tpm2_rng.o
> diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c
> new file mode 100644
> index 0000000000..ddb20b008d
> --- /dev/null
> +++ b/drivers/rng/tpm1_rng.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <rng.h>
> +#include <tpm-utils.h>
> +#include <tpm-v1.h>
> +
> +#define TPM_HEADER_SIZE		10
> +
> +#define TPMV1_DATA_OFFSET	14
> +

Please, provide a Sphinx style function description.

> +static int tpm1_rng_read(struct udevice *dev, void *data, size_t count)
> +{
> +	const u8 command[14] = {
> +		0x0, 0xc1,		/* TPM_TAG */
> +		0x0, 0x0, 0x0, 0xe,	/* parameter size */
> +		0x0, 0x0, 0x0, 0x46,	/* TPM_COMMAND_CODE */
> +	};
> +	const size_t length_offset = TPM_HEADER_SIZE;
> +	const size_t data_size_offset = TPM_HEADER_SIZE;
> +	const size_t data_offset = TPMV1_DATA_OFFSET;
> +	u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> +	size_t response_length = sizeof(response);
> +	u32 data_size;
> +	u8 *out = data;
> +
> +	while (count > 0) {
> +		u32 this_bytes = min(count,
> +				     sizeof(response) - data_offset);
> +		u32 err;
> +
> +		if (pack_byte_string(buf, sizeof(buf), "sd",
> +				     0, command, sizeof(command),
> +				     length_offset, this_bytes))
> +			return TPM_LIB_ERROR;

Why should we return -EPERM (= TPM_LIB_ERROR)  here?

How about -EIO?

> +		err = tpm_sendrecv_command(dev->parent, 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;

-EIO

> +		if (data_size > count)
> +			return TPM_LIB_ERROR;

-EIO

> +		if (unpack_byte_string(response, response_length, "s",
> +				       data_offset, out, data_size))
> +			return TPM_LIB_ERROR;

-EIO

> +
> +		count -= data_size;
> +		out += data_size;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dm_rng_ops tpm1_rng_ops = {
> +	.read = tpm1_rng_read,
> +};
> +
> +U_BOOT_DRIVER(tpm1_rng) = {
> +	.name		= "tpm1-rng",
> +	.id		= UCLASS_RNG,
> +	.ops		= &tpm1_rng_ops,
> +};
> diff --git a/drivers/rng/tpm2_rng.c b/drivers/rng/tpm2_rng.c
> new file mode 100644
> index 0000000000..f14528f751
> --- /dev/null
> +++ b/drivers/rng/tpm2_rng.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <rng.h>
> +#include <tpm-utils.h>
> +#include <tpm-v2.h>
> +
> +#define TPM_HEADER_SIZE		10
> +
> +#define TPMV2_DATA_OFFSET	12
> +

Please, add a Sphinx style function description.

> +static int tpm2_rng_read(struct udevice *dev, void *data, size_t count)
> +{
> +	const u8 command_v2[10] = {
> +		tpm_u16(TPM2_ST_NO_SESSIONS),
> +		tpm_u32(12),
> +		tpm_u32(TPM2_CC_GET_RANDOM),
> +	};
> +	u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> +
> +	const size_t data_size_offset = TPM_HEADER_SIZE;
> +	const size_t data_offset = TPMV2_DATA_OFFSET;
> +	size_t response_length = sizeof(response);
> +	u32 data_size;
> +	u8 *out = data;
> +
> +	while (count > 0) {
> +		u32 this_bytes = min((size_t)count,
> +				     sizeof(response) - data_offset);
> +		u32 err;
> +
> +		if (pack_byte_string(buf, sizeof(buf), "sw",
> +				     0, command_v2, sizeof(command_v2),
> +				     sizeof(command_v2), this_bytes))
> +			return TPM_LIB_ERROR;

-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;

-EIO

> +		if (data_size > this_bytes)
> +			return TPM_LIB_ERROR;

-EIO

> +		if (unpack_byte_string(response, response_length, "s",
> +				       data_offset, out, data_size))
> +			return TPM_LIB_ERROR;

-EIO

Best regards

Heinrich

> +
> +		count -= data_size;
> +		out += data_size;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dm_rng_ops tpm2_rng_ops = {
> +	.read = tpm2_rng_read,
> +};
> +
> +U_BOOT_DRIVER(tpm2_rng) = {
> +	.name		= "tpm2-rng",
> +	.id		= UCLASS_RNG,
> +	.ops		= &tpm2_rng_ops,
> +};
> diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
> index 467992e04e..71cc90a2ab 100644
> --- a/lib/tpm-v1.c
> +++ b/lib/tpm-v1.c
> @@ -868,47 +868,3 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
>   #endif /* CONFIG_TPM_LOAD_KEY_BY_SHA1 */
>
>   #endif /* CONFIG_TPM_AUTH_SESSIONS */
> -
> -u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
> -{
> -	const u8 command[14] = {
> -		0x0, 0xc1,		/* TPM_TAG */
> -		0x0, 0x0, 0x0, 0xe,	/* parameter size */
> -		0x0, 0x0, 0x0, 0x46,	/* TPM_COMMAND_CODE */
> -	};
> -	const size_t length_offset = 10;
> -	const size_t data_size_offset = 10;
> -	const size_t data_offset = 14;
> -	u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> -	size_t response_length = sizeof(response);
> -	u32 data_size;
> -	u8 *out = data;
> -
> -	while (count > 0) {
> -		u32 this_bytes = min((size_t)count,
> -				     sizeof(response) - data_offset);
> -		u32 err;
> -
> -		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,
> -					   &response_length);
> -		if (err)
> -			return err;
> -		if (unpack_byte_string(response, response_length, "d",
> -				       data_size_offset, &data_size))
> -			return TPM_LIB_ERROR;
> -		if (data_size > count)
> -			return TPM_LIB_ERROR;
> -		if (unpack_byte_string(response, response_length, "s",
> -				       data_offset, out, data_size))
> -			return TPM_LIB_ERROR;
> -
> -		count -= data_size;
> -		out += data_size;
> -	}
> -
> -	return 0;
> -}
> diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> index 2f16b0007b..c1456c1974 100644
> --- a/lib/tpm-v2.c
> +++ b/lib/tpm-v2.c
> @@ -562,50 +562,6 @@ 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)
> -{
> -	const u8 command_v2[10] = {
> -		tpm_u16(TPM2_ST_NO_SESSIONS),
> -		tpm_u32(12),
> -		tpm_u32(TPM2_CC_GET_RANDOM),
> -	};
> -	u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> -
> -	const size_t data_size_offset = 10;
> -	const size_t data_offset = 12;
> -	size_t response_length = sizeof(response);
> -	u32 data_size;
> -	u8 *out = data;
> -
> -	while (count > 0) {
> -		u32 this_bytes = min((size_t)count,
> -				     sizeof(response) - data_offset);
> -		u32 err;
> -
> -		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,
> -					   &response_length);
> -		if (err)
> -			return err;
> -		if (unpack_byte_string(response, response_length, "w",
> -				       data_size_offset, &data_size))
> -			return TPM_LIB_ERROR;
> -		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;
> -
> -		count -= data_size;
> -		out += data_size;
> -	}
> -
> -	return 0;
> -}
> -
>   u32 tpm2_write_lock(struct udevice *dev, u32 index)
>   {
>   	u8 command_v2[COMMAND_BUFFER_SIZE] = {
> diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> index 9dd9606fa8..de822113b0 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>
> @@ -264,12 +265,29 @@ 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)
>   {
> -	if (is_tpm1(dev))
> -		return tpm1_get_random(dev, data, count);
> -	else if (is_tpm2(dev))
> -		return -ENOSYS; /* not implemented yet */
> -	else
> +	int ret;
> +	struct udevice *rng_dev;
> +
> +	if (is_tpm1(dev)) {
> +		ret = uclass_get_device_by_driver(UCLASS_RNG,
> +						  DM_DRIVER_GET(tpm1_rng),
> +						  &rng_dev);
> +	} else if (is_tpm2(dev)) {
> +		ret = uclass_get_device_by_driver(UCLASS_RNG,
> +						  DM_DRIVER_GET(tpm2_rng),
> +						  &rng_dev);
> +	} else {
>   		return -ENOSYS;
> +	}
> +
> +	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 */

Re: [PATCH 07/10] tpm: rng: Move the TPM RNG functionality to driver model

[Sughosh Ganu]

hello Heinrich,

On Fri, 25 Feb 2022 at 00:23, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 2/24/22 19:05, Sughosh Ganu wrote:
> > Currently, the TPM random number generator(RNG) functions are defined
> > as part of the library functions under the corresponding tpm files for
> > tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers
> > complying with the driver model.
> >
> > Also make changes to the tpm_get_random function to have it call the
> > TPM RNG driver functions instead of the library functions.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   drivers/rng/Makefile   |  1 +
> >   drivers/rng/tpm1_rng.c | 68 ++++++++++++++++++++++++++++++++++++++++++
> >   drivers/rng/tpm2_rng.c | 68 ++++++++++++++++++++++++++++++++++++++++++
> >   lib/tpm-v1.c           | 44 ---------------------------
> >   lib/tpm-v2.c           | 44 ---------------------------
> >   lib/tpm_api.c          | 28 +++++++++++++----
> >   6 files changed, 160 insertions(+), 93 deletions(-)
> >   create mode 100644 drivers/rng/tpm1_rng.c
> >   create mode 100644 drivers/rng/tpm2_rng.c
> >
> > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> > index 39f7ee3f03..129cfbd006 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) += tpm1_rng.o tpm2_rng.o
> > diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c
> > new file mode 100644
> > index 0000000000..ddb20b008d
> > --- /dev/null
> > +++ b/drivers/rng/tpm1_rng.c
> > @@ -0,0 +1,68 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2022, Linaro Limited
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <rng.h>
> > +#include <tpm-utils.h>
> > +#include <tpm-v1.h>
> > +
> > +#define TPM_HEADER_SIZE              10
> > +
> > +#define TPMV1_DATA_OFFSET    14
> > +
>
> Please, provide a Sphinx style function description.

Thanks for the review of this patch series. I will incorporate all
your review comments in the next version. Thanks.

-sughosh

>
> > +static int tpm1_rng_read(struct udevice *dev, void *data, size_t count)
> > +{
> > +     const u8 command[14] = {
> > +             0x0, 0xc1,              /* TPM_TAG */
> > +             0x0, 0x0, 0x0, 0xe,     /* parameter size */
> > +             0x0, 0x0, 0x0, 0x46,    /* TPM_COMMAND_CODE */
> > +     };
> > +     const size_t length_offset = TPM_HEADER_SIZE;
> > +     const size_t data_size_offset = TPM_HEADER_SIZE;
> > +     const size_t data_offset = TPMV1_DATA_OFFSET;
> > +     u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> > +     size_t response_length = sizeof(response);
> > +     u32 data_size;
> > +     u8 *out = data;
> > +
> > +     while (count > 0) {
> > +             u32 this_bytes = min(count,
> > +                                  sizeof(response) - data_offset);
> > +             u32 err;
> > +
> > +             if (pack_byte_string(buf, sizeof(buf), "sd",
> > +                                  0, command, sizeof(command),
> > +                                  length_offset, this_bytes))
> > +                     return TPM_LIB_ERROR;
>
> Why should we return -EPERM (= TPM_LIB_ERROR)  here?
>
> How about -EIO?
>
> > +             err = tpm_sendrecv_command(dev->parent, 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;
>
> -EIO
>
> > +             if (data_size > count)
> > +                     return TPM_LIB_ERROR;
>
> -EIO
>
> > +             if (unpack_byte_string(response, response_length, "s",
> > +                                    data_offset, out, data_size))
> > +                     return TPM_LIB_ERROR;
>
> -EIO
>
> > +
> > +             count -= data_size;
> > +             out += data_size;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct dm_rng_ops tpm1_rng_ops = {
> > +     .read = tpm1_rng_read,
> > +};
> > +
> > +U_BOOT_DRIVER(tpm1_rng) = {
> > +     .name           = "tpm1-rng",
> > +     .id             = UCLASS_RNG,
> > +     .ops            = &tpm1_rng_ops,
> > +};
> > diff --git a/drivers/rng/tpm2_rng.c b/drivers/rng/tpm2_rng.c
> > new file mode 100644
> > index 0000000000..f14528f751
> > --- /dev/null
> > +++ b/drivers/rng/tpm2_rng.c
> > @@ -0,0 +1,68 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2022, Linaro Limited
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <rng.h>
> > +#include <tpm-utils.h>
> > +#include <tpm-v2.h>
> > +
> > +#define TPM_HEADER_SIZE              10
> > +
> > +#define TPMV2_DATA_OFFSET    12
> > +
>
> Please, add a Sphinx style function description.
>
> > +static int tpm2_rng_read(struct udevice *dev, void *data, size_t count)
> > +{
> > +     const u8 command_v2[10] = {
> > +             tpm_u16(TPM2_ST_NO_SESSIONS),
> > +             tpm_u32(12),
> > +             tpm_u32(TPM2_CC_GET_RANDOM),
> > +     };
> > +     u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> > +
> > +     const size_t data_size_offset = TPM_HEADER_SIZE;
> > +     const size_t data_offset = TPMV2_DATA_OFFSET;
> > +     size_t response_length = sizeof(response);
> > +     u32 data_size;
> > +     u8 *out = data;
> > +
> > +     while (count > 0) {
> > +             u32 this_bytes = min((size_t)count,
> > +                                  sizeof(response) - data_offset);
> > +             u32 err;
> > +
> > +             if (pack_byte_string(buf, sizeof(buf), "sw",
> > +                                  0, command_v2, sizeof(command_v2),
> > +                                  sizeof(command_v2), this_bytes))
> > +                     return TPM_LIB_ERROR;
>
> -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;
>
> -EIO
>
> > +             if (data_size > this_bytes)
> > +                     return TPM_LIB_ERROR;
>
> -EIO
>
> > +             if (unpack_byte_string(response, response_length, "s",
> > +                                    data_offset, out, data_size))
> > +                     return TPM_LIB_ERROR;
>
> -EIO
>
> Best regards
>
> Heinrich
>
> > +
> > +             count -= data_size;
> > +             out += data_size;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct dm_rng_ops tpm2_rng_ops = {
> > +     .read = tpm2_rng_read,
> > +};
> > +
> > +U_BOOT_DRIVER(tpm2_rng) = {
> > +     .name           = "tpm2-rng",
> > +     .id             = UCLASS_RNG,
> > +     .ops            = &tpm2_rng_ops,
> > +};
> > diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
> > index 467992e04e..71cc90a2ab 100644
> > --- a/lib/tpm-v1.c
> > +++ b/lib/tpm-v1.c
> > @@ -868,47 +868,3 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
> >   #endif /* CONFIG_TPM_LOAD_KEY_BY_SHA1 */
> >
> >   #endif /* CONFIG_TPM_AUTH_SESSIONS */
> > -
> > -u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
> > -{
> > -     const u8 command[14] = {
> > -             0x0, 0xc1,              /* TPM_TAG */
> > -             0x0, 0x0, 0x0, 0xe,     /* parameter size */
> > -             0x0, 0x0, 0x0, 0x46,    /* TPM_COMMAND_CODE */
> > -     };
> > -     const size_t length_offset = 10;
> > -     const size_t data_size_offset = 10;
> > -     const size_t data_offset = 14;
> > -     u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> > -     size_t response_length = sizeof(response);
> > -     u32 data_size;
> > -     u8 *out = data;
> > -
> > -     while (count > 0) {
> > -             u32 this_bytes = min((size_t)count,
> > -                                  sizeof(response) - data_offset);
> > -             u32 err;
> > -
> > -             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,
> > -                                        &response_length);
> > -             if (err)
> > -                     return err;
> > -             if (unpack_byte_string(response, response_length, "d",
> > -                                    data_size_offset, &data_size))
> > -                     return TPM_LIB_ERROR;
> > -             if (data_size > count)
> > -                     return TPM_LIB_ERROR;
> > -             if (unpack_byte_string(response, response_length, "s",
> > -                                    data_offset, out, data_size))
> > -                     return TPM_LIB_ERROR;
> > -
> > -             count -= data_size;
> > -             out += data_size;
> > -     }
> > -
> > -     return 0;
> > -}
> > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> > index 2f16b0007b..c1456c1974 100644
> > --- a/lib/tpm-v2.c
> > +++ b/lib/tpm-v2.c
> > @@ -562,50 +562,6 @@ 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)
> > -{
> > -     const u8 command_v2[10] = {
> > -             tpm_u16(TPM2_ST_NO_SESSIONS),
> > -             tpm_u32(12),
> > -             tpm_u32(TPM2_CC_GET_RANDOM),
> > -     };
> > -     u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> > -
> > -     const size_t data_size_offset = 10;
> > -     const size_t data_offset = 12;
> > -     size_t response_length = sizeof(response);
> > -     u32 data_size;
> > -     u8 *out = data;
> > -
> > -     while (count > 0) {
> > -             u32 this_bytes = min((size_t)count,
> > -                                  sizeof(response) - data_offset);
> > -             u32 err;
> > -
> > -             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,
> > -                                        &response_length);
> > -             if (err)
> > -                     return err;
> > -             if (unpack_byte_string(response, response_length, "w",
> > -                                    data_size_offset, &data_size))
> > -                     return TPM_LIB_ERROR;
> > -             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;
> > -
> > -             count -= data_size;
> > -             out += data_size;
> > -     }
> > -
> > -     return 0;
> > -}
> > -
> >   u32 tpm2_write_lock(struct udevice *dev, u32 index)
> >   {
> >       u8 command_v2[COMMAND_BUFFER_SIZE] = {
> > diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> > index 9dd9606fa8..de822113b0 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>
> > @@ -264,12 +265,29 @@ 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)
> >   {
> > -     if (is_tpm1(dev))
> > -             return tpm1_get_random(dev, data, count);
> > -     else if (is_tpm2(dev))
> > -             return -ENOSYS; /* not implemented yet */
> > -     else
> > +     int ret;
> > +     struct udevice *rng_dev;
> > +
> > +     if (is_tpm1(dev)) {
> > +             ret = uclass_get_device_by_driver(UCLASS_RNG,
> > +                                               DM_DRIVER_GET(tpm1_rng),
> > +                                               &rng_dev);
> > +     } else if (is_tpm2(dev)) {
> > +             ret = uclass_get_device_by_driver(UCLASS_RNG,
> > +                                               DM_DRIVER_GET(tpm2_rng),
> > +                                               &rng_dev);
> > +     } else {
> >               return -ENOSYS;
> > +     }
> > +
> > +     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 */
>

Re: [PATCH 07/10] tpm: rng: Move the TPM RNG functionality to driver model

[Ilias Apalodimas]

On Thu, Feb 24, 2022 at 11:35:49PM +0530, Sughosh Ganu wrote:
> Currently, the TPM random number generator(RNG) functions are defined
> as part of the library functions under the corresponding tpm files for
> tpmv1 and tpmv2. Move the RNG functionality under TPM RNG drivers
> complying with the driver model.
> 
> Also make changes to the tpm_get_random function to have it call the
> TPM RNG driver functions instead of the library functions.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  drivers/rng/Makefile   |  1 +
>  drivers/rng/tpm1_rng.c | 68 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/rng/tpm2_rng.c | 68 ++++++++++++++++++++++++++++++++++++++++++
>  lib/tpm-v1.c           | 44 ---------------------------
>  lib/tpm-v2.c           | 44 ---------------------------
>  lib/tpm_api.c          | 28 +++++++++++++----
>  6 files changed, 160 insertions(+), 93 deletions(-)
>  create mode 100644 drivers/rng/tpm1_rng.c
>  create mode 100644 drivers/rng/tpm2_rng.c
> 
> diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> index 39f7ee3f03..129cfbd006 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) += tpm1_rng.o tpm2_rng.o
> diff --git a/drivers/rng/tpm1_rng.c b/drivers/rng/tpm1_rng.c
> new file mode 100644
> index 0000000000..ddb20b008d
> --- /dev/null
> +++ b/drivers/rng/tpm1_rng.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2022, Linaro Limited

As  I understand part of this code was moved around.  If so we need to keep
the original copyright as well

> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <rng.h>
> +#include <tpm-utils.h>
> +#include <tpm-v1.h>
> +
> +#define TPM_HEADER_SIZE		10
> +
> +#define TPMV1_DATA_OFFSET	14
> +
> +static int tpm1_rng_read(struct udevice *dev, void *data, size_t count)
> +{
> +	const u8 command[14] = {
> +		0x0, 0xc1,		/* TPM_TAG */
> +		0x0, 0x0, 0x0, 0xe,	/* parameter size */
> +		0x0, 0x0, 0x0, 0x46,	/* TPM_COMMAND_CODE */
> +	};
> +	const size_t length_offset = TPM_HEADER_SIZE;
> +	const size_t data_size_offset = TPM_HEADER_SIZE;
> +	const size_t data_offset = TPMV1_DATA_OFFSET;
> +	u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> +	size_t response_length = sizeof(response);
> +	u32 data_size;
> +	u8 *out = data;
> +
> +	while (count > 0) {
> +		u32 this_bytes = min(count,
> +				     sizeof(response) - data_offset);
> +		u32 err;
> +
> +		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->parent, 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;
> +		if (data_size > count)
> +			return TPM_LIB_ERROR;
> +		if (unpack_byte_string(response, response_length, "s",
> +				       data_offset, out, data_size))
> +			return TPM_LIB_ERROR;
> +
> +		count -= data_size;
> +		out += data_size;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dm_rng_ops tpm1_rng_ops = {
> +	.read = tpm1_rng_read,
> +};
> +
> +U_BOOT_DRIVER(tpm1_rng) = {
> +	.name		= "tpm1-rng",
> +	.id		= UCLASS_RNG,
> +	.ops		= &tpm1_rng_ops,
> +};
> diff --git a/drivers/rng/tpm2_rng.c b/drivers/rng/tpm2_rng.c
> new file mode 100644
> index 0000000000..f14528f751
> --- /dev/null
> +++ b/drivers/rng/tpm2_rng.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2022, Linaro Limited
> + */
> +

ditto 

> +#include <common.h>
> +#include <dm.h>
> +#include <rng.h>
> +#include <tpm-utils.h>
> +#include <tpm-v2.h>
> +
> +#define TPM_HEADER_SIZE		10
> +
> +#define TPMV2_DATA_OFFSET	12
> +
> +static int tpm2_rng_read(struct udevice *dev, void *data, size_t count)
> +{
> +	const u8 command_v2[10] = {
> +		tpm_u16(TPM2_ST_NO_SESSIONS),
> +		tpm_u32(12),
> +		tpm_u32(TPM2_CC_GET_RANDOM),
> +	};
> +	u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> +
> +	const size_t data_size_offset = TPM_HEADER_SIZE;
> +	const size_t data_offset = TPMV2_DATA_OFFSET;
> +	size_t response_length = sizeof(response);
> +	u32 data_size;
> +	u8 *out = data;
> +
> +	while (count > 0) {
> +		u32 this_bytes = min((size_t)count,
> +				     sizeof(response) - data_offset);
> +		u32 err;
> +
> +		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->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;
> +		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;
> +
> +		count -= data_size;
> +		out += data_size;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dm_rng_ops tpm2_rng_ops = {
> +	.read = tpm2_rng_read,
> +};
> +
> +U_BOOT_DRIVER(tpm2_rng) = {
> +	.name		= "tpm2-rng",
> +	.id		= UCLASS_RNG,
> +	.ops		= &tpm2_rng_ops,
> +};
> diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
> index 467992e04e..71cc90a2ab 100644
> --- a/lib/tpm-v1.c
> +++ b/lib/tpm-v1.c
> @@ -868,47 +868,3 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
>  #endif /* CONFIG_TPM_LOAD_KEY_BY_SHA1 */
>  
>  #endif /* CONFIG_TPM_AUTH_SESSIONS */
> -
> -u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
> -{
> -	const u8 command[14] = {
> -		0x0, 0xc1,		/* TPM_TAG */
> -		0x0, 0x0, 0x0, 0xe,	/* parameter size */
> -		0x0, 0x0, 0x0, 0x46,	/* TPM_COMMAND_CODE */
> -	};
> -	const size_t length_offset = 10;
> -	const size_t data_size_offset = 10;
> -	const size_t data_offset = 14;
> -	u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> -	size_t response_length = sizeof(response);
> -	u32 data_size;
> -	u8 *out = data;
> -
> -	while (count > 0) {
> -		u32 this_bytes = min((size_t)count,
> -				     sizeof(response) - data_offset);
> -		u32 err;
> -
> -		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,
> -					   &response_length);
> -		if (err)
> -			return err;
> -		if (unpack_byte_string(response, response_length, "d",
> -				       data_size_offset, &data_size))
> -			return TPM_LIB_ERROR;
> -		if (data_size > count)
> -			return TPM_LIB_ERROR;
> -		if (unpack_byte_string(response, response_length, "s",
> -				       data_offset, out, data_size))
> -			return TPM_LIB_ERROR;
> -
> -		count -= data_size;
> -		out += data_size;
> -	}
> -
> -	return 0;
> -}
> diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> index 2f16b0007b..c1456c1974 100644
> --- a/lib/tpm-v2.c
> +++ b/lib/tpm-v2.c
> @@ -562,50 +562,6 @@ 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)
> -{
> -	const u8 command_v2[10] = {
> -		tpm_u16(TPM2_ST_NO_SESSIONS),
> -		tpm_u32(12),
> -		tpm_u32(TPM2_CC_GET_RANDOM),
> -	};
> -	u8 buf[COMMAND_BUFFER_SIZE], response[COMMAND_BUFFER_SIZE];
> -
> -	const size_t data_size_offset = 10;
> -	const size_t data_offset = 12;
> -	size_t response_length = sizeof(response);
> -	u32 data_size;
> -	u8 *out = data;
> -
> -	while (count > 0) {
> -		u32 this_bytes = min((size_t)count,
> -				     sizeof(response) - data_offset);
> -		u32 err;
> -
> -		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,
> -					   &response_length);
> -		if (err)
> -			return err;
> -		if (unpack_byte_string(response, response_length, "w",
> -				       data_size_offset, &data_size))
> -			return TPM_LIB_ERROR;
> -		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;
> -
> -		count -= data_size;
> -		out += data_size;
> -	}
> -
> -	return 0;
> -}
> -
>  u32 tpm2_write_lock(struct udevice *dev, u32 index)
>  {
>  	u8 command_v2[COMMAND_BUFFER_SIZE] = {
> diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> index 9dd9606fa8..de822113b0 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>
> @@ -264,12 +265,29 @@ 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)
>  {
> -	if (is_tpm1(dev))
> -		return tpm1_get_random(dev, data, count);
> -	else if (is_tpm2(dev))
> -		return -ENOSYS; /* not implemented yet */
> -	else
> +	int ret;

int ret = -ENOSYS;

> +	struct udevice *rng_dev;
> +
> +	if (is_tpm1(dev)) {
> +		ret = uclass_get_device_by_driver(UCLASS_RNG,
> +						  DM_DRIVER_GET(tpm1_rng),
> +						  &rng_dev);
> +	} else if (is_tpm2(dev)) {
> +		ret = uclass_get_device_by_driver(UCLASS_RNG,
> +						  DM_DRIVER_GET(tpm2_rng),
> +						  &rng_dev);
> +	} else {
>  		return -ENOSYS;
> +	}

Then you can get rid of the else and the {} on all cases. 

> +
> +	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.17.1
> 

Regards
/Ilias

[PATCH 08/10] tpm: Add the RNG child device

[Sughosh Ganu]

The TPM device comes with the random number generator(RNG)
functionality which is built into the TPM device. Add logic to add the
RNG child device in the TPM uclass post probe callback.

The RNG device can then be used to pass a set of random bytes to the
linux kernel, need for address space randomisation through the
EFI_RNG_PROTOCOL interface.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 4 deletions(-)

diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
index 8619da89d8..383cc7bc48 100644
--- a/drivers/tpm/tpm-uclass.c
+++ b/drivers/tpm/tpm-uclass.c
@@ -16,6 +16,11 @@
 #include <tpm-v2.h>
 #include "tpm_internal.h"
 
+#include <dm/lists.h>
+
+#define TPM_RNG1_DRV_NAME	"tpm1-rng"
+#define TPM_RNG2_DRV_NAME	"tpm2-rng"
+
 bool is_tpm1(struct udevice *dev)
 {
 	return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
@@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_TPM)
+static int tpm_uclass_post_probe(struct udevice *dev)
+{
+	int ret;
+	const char *drv = is_tpm1(dev) ? TPM_RNG1_DRV_NAME : TPM_RNG2_DRV_NAME;
+	struct udevice *child;
+
+	ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
+	if (ret == -ENOENT) {
+		log_err("No driver configured for tpm-rng device\n");
+		return 0;
+	}
+
+	if (ret) {
+		log_err("Unable to bind rng driver with the tpm-rng device\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tpm_uclass_child_pre_probe(struct udevice *dev)
+{
+	int ret;
+
+	ret = tpm_open(dev->parent);
+	if (ret == -EBUSY) {
+		log_info("TPM device already opened\n");
+	} else if (ret) {
+		log_err("Unable to open TPM device\n");
+		return ret;
+	}
+
+	ret = tpm_startup(dev->parent, TPM_ST_CLEAR);
+	if (ret)
+		log_err("Unable to start TPM device\n");
+
+	return ret;
+}
+#endif /* CONFIG_TPM */
+
 UCLASS_DRIVER(tpm) = {
-	.id		= UCLASS_TPM,
-	.name		= "tpm",
-	.flags		= DM_UC_FLAG_SEQ_ALIAS,
+	.id			= UCLASS_TPM,
+	.name			= "tpm",
+	.flags			= DM_UC_FLAG_SEQ_ALIAS,
 #if CONFIG_IS_ENABLED(OF_REAL)
-	.post_bind	= dm_scan_fdt_dev,
+	.post_bind		= dm_scan_fdt_dev,
+#endif
+#if IS_ENABLED(CONFIG_TPM)
+	.post_probe		= tpm_uclass_post_probe,
+	.child_pre_probe	= tpm_uclass_child_pre_probe,
 #endif
 	.per_device_auto	= sizeof(struct tpm_chip_priv),
 };
-- 
2.17.1

Re: [PATCH 08/10] tpm: Add the RNG child device

[Heinrich Schuchardt]

On 2/24/22 19:05, Sughosh Ganu wrote:
> The TPM device comes with the random number generator(RNG)
> functionality which is built into the TPM device. Add logic to add the
> RNG child device in the TPM uclass post probe callback.
>
> The RNG device can then be used to pass a set of random bytes to the
> linux kernel, need for address space randomisation through the
> EFI_RNG_PROTOCOL interface.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
>   1 file changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> index 8619da89d8..383cc7bc48 100644
> --- a/drivers/tpm/tpm-uclass.c
> +++ b/drivers/tpm/tpm-uclass.c
> @@ -16,6 +16,11 @@
>   #include <tpm-v2.h>
>   #include "tpm_internal.h"
>
> +#include <dm/lists.h>
> +
> +#define TPM_RNG1_DRV_NAME	"tpm1-rng"
> +#define TPM_RNG2_DRV_NAME	"tpm2-rng"
> +
>   bool is_tpm1(struct udevice *dev)
>   {
>   	return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
>   	return 0;
>   }
>
> +#if IS_ENABLED(CONFIG_TPM)

This is is superfluous.
This file is only compiled if CONFIG_$(SPL_TPL_)TPM = y.

Best regards

Heinrich

> +static int tpm_uclass_post_probe(struct udevice *dev)
> +{
> +	int ret;
> +	const char *drv = is_tpm1(dev) ? TPM_RNG1_DRV_NAME : TPM_RNG2_DRV_NAME;
> +	struct udevice *child;
> +
> +	ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
> +	if (ret == -ENOENT) {
> +		log_err("No driver configured for tpm-rng device\n");
> +		return 0;
> +	}
> +
> +	if (ret) {
> +		log_err("Unable to bind rng driver with the tpm-rng device\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tpm_uclass_child_pre_probe(struct udevice *dev)
> +{
> +	int ret;
> +
> +	ret = tpm_open(dev->parent);
> +	if (ret == -EBUSY) {
> +		log_info("TPM device already opened\n");
> +	} else if (ret) {
> +		log_err("Unable to open TPM device\n");
> +		return ret;
> +	}
> +
> +	ret = tpm_startup(dev->parent, TPM_ST_CLEAR);
> +	if (ret)
> +		log_err("Unable to start TPM device\n");
> +
> +	return ret;
> +}
> +#endif /* CONFIG_TPM */
> +
>   UCLASS_DRIVER(tpm) = {
> -	.id		= UCLASS_TPM,
> -	.name		= "tpm",
> -	.flags		= DM_UC_FLAG_SEQ_ALIAS,
> +	.id			= UCLASS_TPM,
> +	.name			= "tpm",
> +	.flags			= DM_UC_FLAG_SEQ_ALIAS,
>   #if CONFIG_IS_ENABLED(OF_REAL)
> -	.post_bind	= dm_scan_fdt_dev,
> +	.post_bind		= dm_scan_fdt_dev,
> +#endif
> +#if IS_ENABLED(CONFIG_TPM)
> +	.post_probe		= tpm_uclass_post_probe,
> +	.child_pre_probe	= tpm_uclass_child_pre_probe,
>   #endif
>   	.per_device_auto	= sizeof(struct tpm_chip_priv),
>   };

Re: [PATCH 08/10] tpm: Add the RNG child device

[Sughosh Ganu]

On Fri, 25 Feb 2022 at 00:21, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 2/24/22 19:05, Sughosh Ganu wrote:
> > The TPM device comes with the random number generator(RNG)
> > functionality which is built into the TPM device. Add logic to add the
> > RNG child device in the TPM uclass post probe callback.
> >
> > The RNG device can then be used to pass a set of random bytes to the
> > linux kernel, need for address space randomisation through the
> > EFI_RNG_PROTOCOL interface.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> >   1 file changed, 54 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > index 8619da89d8..383cc7bc48 100644
> > --- a/drivers/tpm/tpm-uclass.c
> > +++ b/drivers/tpm/tpm-uclass.c
> > @@ -16,6 +16,11 @@
> >   #include <tpm-v2.h>
> >   #include "tpm_internal.h"
> >
> > +#include <dm/lists.h>
> > +
> > +#define TPM_RNG1_DRV_NAME    "tpm1-rng"
> > +#define TPM_RNG2_DRV_NAME    "tpm2-rng"
> > +
> >   bool is_tpm1(struct udevice *dev)
> >   {
> >       return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> > @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> >       return 0;
> >   }
> >
> > +#if IS_ENABLED(CONFIG_TPM)
>
> This is is superfluous.
> This file is only compiled if CONFIG_$(SPL_TPL_)TPM = y.

Yes, but I want the RNG child device addition to be done only in case
of u-boot proper, not for SPL and TPM. We do not have RNG support for
the SPL/TPM boot stages. So unless someone needs to enable RNG support
for SPL/TPL stages, which I doubt, this code should run only in u-boot
proper. Thanks.

-sughosh

>
> Best regards
>
> Heinrich
>
> > +static int tpm_uclass_post_probe(struct udevice *dev)
> > +{
> > +     int ret;
> > +     const char *drv = is_tpm1(dev) ? TPM_RNG1_DRV_NAME : TPM_RNG2_DRV_NAME;
> > +     struct udevice *child;
> > +
> > +     ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
> > +     if (ret == -ENOENT) {
> > +             log_err("No driver configured for tpm-rng device\n");
> > +             return 0;
> > +     }
> > +
> > +     if (ret) {
> > +             log_err("Unable to bind rng driver with the tpm-rng device\n");
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int tpm_uclass_child_pre_probe(struct udevice *dev)
> > +{
> > +     int ret;
> > +
> > +     ret = tpm_open(dev->parent);
> > +     if (ret == -EBUSY) {
> > +             log_info("TPM device already opened\n");
> > +     } else if (ret) {
> > +             log_err("Unable to open TPM device\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = tpm_startup(dev->parent, TPM_ST_CLEAR);
> > +     if (ret)
> > +             log_err("Unable to start TPM device\n");
> > +
> > +     return ret;
> > +}
> > +#endif /* CONFIG_TPM */
> > +
> >   UCLASS_DRIVER(tpm) = {
> > -     .id             = UCLASS_TPM,
> > -     .name           = "tpm",
> > -     .flags          = DM_UC_FLAG_SEQ_ALIAS,
> > +     .id                     = UCLASS_TPM,
> > +     .name                   = "tpm",
> > +     .flags                  = DM_UC_FLAG_SEQ_ALIAS,
> >   #if CONFIG_IS_ENABLED(OF_REAL)
> > -     .post_bind      = dm_scan_fdt_dev,
> > +     .post_bind              = dm_scan_fdt_dev,
> > +#endif
> > +#if IS_ENABLED(CONFIG_TPM)
> > +     .post_probe             = tpm_uclass_post_probe,
> > +     .child_pre_probe        = tpm_uclass_child_pre_probe,
> >   #endif
> >       .per_device_auto        = sizeof(struct tpm_chip_priv),
> >   };
>

Re: [PATCH 08/10] tpm: Add the RNG child device

[Heinrich Schuchardt]

On 2/25/22 06:45, Sughosh Ganu wrote:
> On Fri, 25 Feb 2022 at 00:21, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 2/24/22 19:05, Sughosh Ganu wrote:
>>> The TPM device comes with the random number generator(RNG)
>>> functionality which is built into the TPM device. Add logic to add the
>>> RNG child device in the TPM uclass post probe callback.
>>>
>>> The RNG device can then be used to pass a set of random bytes to the
>>> linux kernel, need for address space randomisation through the
>>> EFI_RNG_PROTOCOL interface.
>>>
>>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>>> ---
>>>    drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
>>>    1 file changed, 54 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
>>> index 8619da89d8..383cc7bc48 100644
>>> --- a/drivers/tpm/tpm-uclass.c
>>> +++ b/drivers/tpm/tpm-uclass.c
>>> @@ -16,6 +16,11 @@
>>>    #include <tpm-v2.h>
>>>    #include "tpm_internal.h"
>>>
>>> +#include <dm/lists.h>
>>> +
>>> +#define TPM_RNG1_DRV_NAME    "tpm1-rng"
>>> +#define TPM_RNG2_DRV_NAME    "tpm2-rng"
>>> +
>>>    bool is_tpm1(struct udevice *dev)
>>>    {
>>>        return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
>>> @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
>>>        return 0;
>>>    }
>>>
>>> +#if IS_ENABLED(CONFIG_TPM)
>>
>> This is is superfluous.
>> This file is only compiled if CONFIG_$(SPL_TPL_)TPM = y.
>
> Yes, but I want the RNG child device addition to be done only in case
> of u-boot proper, not for SPL and TPM. We do not have RNG support for
> the SPL/TPM boot stages. So unless someone needs to enable RNG support
> for SPL/TPL stages, which I doubt, this code should run only in u-boot
> proper. Thanks.

The tpm-uclass.c is never compiled in SPL or TPL. Have a look at
drivers/tpm/Kconfig. There is no CONFIG_SPL_TPM or CONFIG_TPL_TPM symbol.

Best regards

Heinrich

>
> -sughosh
>
>>
>> Best regards
>>
>> Heinrich
>>
>>> +static int tpm_uclass_post_probe(struct udevice *dev)
>>> +{
>>> +     int ret;
>>> +     const char *drv = is_tpm1(dev) ? TPM_RNG1_DRV_NAME : TPM_RNG2_DRV_NAME;
>>> +     struct udevice *child;
>>> +
>>> +     ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
>>> +     if (ret == -ENOENT) {
>>> +             log_err("No driver configured for tpm-rng device\n");
>>> +             return 0;
>>> +     }
>>> +
>>> +     if (ret) {
>>> +             log_err("Unable to bind rng driver with the tpm-rng device\n");
>>> +             return ret;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int tpm_uclass_child_pre_probe(struct udevice *dev)
>>> +{
>>> +     int ret;
>>> +
>>> +     ret = tpm_open(dev->parent);
>>> +     if (ret == -EBUSY) {
>>> +             log_info("TPM device already opened\n");
>>> +     } else if (ret) {
>>> +             log_err("Unable to open TPM device\n");
>>> +             return ret;
>>> +     }
>>> +
>>> +     ret = tpm_startup(dev->parent, TPM_ST_CLEAR);
>>> +     if (ret)
>>> +             log_err("Unable to start TPM device\n");
>>> +
>>> +     return ret;
>>> +}
>>> +#endif /* CONFIG_TPM */
>>> +
>>>    UCLASS_DRIVER(tpm) = {
>>> -     .id             = UCLASS_TPM,
>>> -     .name           = "tpm",
>>> -     .flags          = DM_UC_FLAG_SEQ_ALIAS,
>>> +     .id                     = UCLASS_TPM,
>>> +     .name                   = "tpm",
>>> +     .flags                  = DM_UC_FLAG_SEQ_ALIAS,
>>>    #if CONFIG_IS_ENABLED(OF_REAL)
>>> -     .post_bind      = dm_scan_fdt_dev,
>>> +     .post_bind              = dm_scan_fdt_dev,
>>> +#endif
>>> +#if IS_ENABLED(CONFIG_TPM)
>>> +     .post_probe             = tpm_uclass_post_probe,
>>> +     .child_pre_probe        = tpm_uclass_child_pre_probe,
>>>    #endif
>>>        .per_device_auto        = sizeof(struct tpm_chip_priv),
>>>    };
>>

Re: [PATCH 08/10] tpm: Add the RNG child device

[Sughosh Ganu]

hello Heinrich,

On Fri, 25 Feb 2022 at 12:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 2/25/22 06:45, Sughosh Ganu wrote:
> > On Fri, 25 Feb 2022 at 00:21, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 2/24/22 19:05, Sughosh Ganu wrote:
> >>> The TPM device comes with the random number generator(RNG)
> >>> functionality which is built into the TPM device. Add logic to add the
> >>> RNG child device in the TPM uclass post probe callback.
> >>>
> >>> The RNG device can then be used to pass a set of random bytes to the
> >>> linux kernel, need for address space randomisation through the
> >>> EFI_RNG_PROTOCOL interface.
> >>>
> >>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> >>> ---
> >>>    drivers/tpm/tpm-uclass.c | 58 +++++++++++++++++++++++++++++++++++++---
> >>>    1 file changed, 54 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> >>> index 8619da89d8..383cc7bc48 100644
> >>> --- a/drivers/tpm/tpm-uclass.c
> >>> +++ b/drivers/tpm/tpm-uclass.c
> >>> @@ -16,6 +16,11 @@
> >>>    #include <tpm-v2.h>
> >>>    #include "tpm_internal.h"
> >>>
> >>> +#include <dm/lists.h>
> >>> +
> >>> +#define TPM_RNG1_DRV_NAME    "tpm1-rng"
> >>> +#define TPM_RNG2_DRV_NAME    "tpm2-rng"
> >>> +
> >>>    bool is_tpm1(struct udevice *dev)
> >>>    {
> >>>        return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> >>> @@ -147,12 +152,57 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> >>>        return 0;
> >>>    }
> >>>
> >>> +#if IS_ENABLED(CONFIG_TPM)
> >>
> >> This is is superfluous.
> >> This file is only compiled if CONFIG_$(SPL_TPL_)TPM = y.
> >
> > Yes, but I want the RNG child device addition to be done only in case
> > of u-boot proper, not for SPL and TPM. We do not have RNG support for
> > the SPL/TPM boot stages. So unless someone needs to enable RNG support
> > for SPL/TPL stages, which I doubt, this code should run only in u-boot
> > proper. Thanks.
>
> The tpm-uclass.c is never compiled in SPL or TPL. Have a look at
> drivers/tpm/Kconfig. There is no CONFIG_SPL_TPM or CONFIG_TPL_TPM symbol.

The {S,T}PL_TPM symbol is actually defined under lib/Kconfig.
Currently it is not getting defined in any platform's defconfig, but
one can add it and build the tpm-uclass driver for the SPL/TPL stage
as well. I added the SPL_TPM config to the chromebook_coral and it
indeed builds the tpm-uclass driver. Thanks.

-sughosh

>
> Best regards
>
> Heinrich
>
> >
> > -sughosh
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> +static int tpm_uclass_post_probe(struct udevice *dev)
> >>> +{
> >>> +     int ret;
> >>> +     const char *drv = is_tpm1(dev) ? TPM_RNG1_DRV_NAME : TPM_RNG2_DRV_NAME;
> >>> +     struct udevice *child;
> >>> +
> >>> +     ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
> >>> +     if (ret == -ENOENT) {
> >>> +             log_err("No driver configured for tpm-rng device\n");
> >>> +             return 0;
> >>> +     }
> >>> +
> >>> +     if (ret) {
> >>> +             log_err("Unable to bind rng driver with the tpm-rng device\n");
> >>> +             return ret;
> >>> +     }
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static int tpm_uclass_child_pre_probe(struct udevice *dev)
> >>> +{
> >>> +     int ret;
> >>> +
> >>> +     ret = tpm_open(dev->parent);
> >>> +     if (ret == -EBUSY) {
> >>> +             log_info("TPM device already opened\n");
> >>> +     } else if (ret) {
> >>> +             log_err("Unable to open TPM device\n");
> >>> +             return ret;
> >>> +     }
> >>> +
> >>> +     ret = tpm_startup(dev->parent, TPM_ST_CLEAR);
> >>> +     if (ret)
> >>> +             log_err("Unable to start TPM device\n");
> >>> +
> >>> +     return ret;
> >>> +}
> >>> +#endif /* CONFIG_TPM */
> >>> +
> >>>    UCLASS_DRIVER(tpm) = {
> >>> -     .id             = UCLASS_TPM,
> >>> -     .name           = "tpm",
> >>> -     .flags          = DM_UC_FLAG_SEQ_ALIAS,
> >>> +     .id                     = UCLASS_TPM,
> >>> +     .name                   = "tpm",
> >>> +     .flags                  = DM_UC_FLAG_SEQ_ALIAS,
> >>>    #if CONFIG_IS_ENABLED(OF_REAL)
> >>> -     .post_bind      = dm_scan_fdt_dev,
> >>> +     .post_bind              = dm_scan_fdt_dev,
> >>> +#endif
> >>> +#if IS_ENABLED(CONFIG_TPM)
> >>> +     .post_probe             = tpm_uclass_post_probe,
> >>> +     .child_pre_probe        = tpm_uclass_child_pre_probe,
> >>>    #endif
> >>>        .per_device_auto        = sizeof(struct tpm_chip_priv),
> >>>    };
> >>
>

[PATCH 09/10] qemu: arm: Remove platform specific function to get RNG device

[Sughosh Ganu]

The Qemu platform has a function defined to get the random number
generator(RNG) device. However, the RNG device can be obtained simply
by searching for a device belonging to the RNG uclass. Remove the
superfluous platform function defined for the Qemu platform for
getting the RNG device.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 board/emulation/qemu-arm/qemu-arm.c | 42 -----------------------------
 1 file changed, 42 deletions(-)

diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
index 16d5a97167..c9e886e44a 100644
--- a/board/emulation/qemu-arm/qemu-arm.c
+++ b/board/emulation/qemu-arm/qemu-arm.c
@@ -107,48 +107,6 @@ void enable_caches(void)
 	 dcache_enable();
 }
 
-#if defined(CONFIG_EFI_RNG_PROTOCOL)
-#include <efi_loader.h>
-#include <efi_rng.h>
-
-#include <dm/device-internal.h>
-
-efi_status_t platform_get_rng_device(struct udevice **dev)
-{
-	int ret;
-	efi_status_t status = EFI_DEVICE_ERROR;
-	struct udevice *bus, *devp;
-
-	for (uclass_first_device(UCLASS_VIRTIO, &bus); bus;
-	     uclass_next_device(&bus)) {
-		for (device_find_first_child(bus, &devp); devp;
-		     device_find_next_child(&devp)) {
-			if (device_get_uclass_id(devp) == UCLASS_RNG) {
-				*dev = devp;
-				status = EFI_SUCCESS;
-				break;
-			}
-		}
-	}
-
-	if (status != EFI_SUCCESS) {
-		debug("No rng device found\n");
-		return EFI_DEVICE_ERROR;
-	}
-
-	if (*dev) {
-		ret = device_probe(*dev);
-		if (ret)
-			return EFI_DEVICE_ERROR;
-	} else {
-		debug("Couldn't get child device\n");
-		return EFI_DEVICE_ERROR;
-	}
-
-	return EFI_SUCCESS;
-}
-#endif /* CONFIG_EFI_RNG_PROTOCOL */
-
 #ifdef CONFIG_ARM64
 #define __W	"w"
 #else
-- 
2.17.1

Re: [PATCH 09/10] qemu: arm: Remove platform specific function to get RNG device

[Heinrich Schuchardt]

On 2/24/22 19:05, Sughosh Ganu wrote:
> The Qemu platform has a function defined to get the random number
> generator(RNG) device. However, the RNG device can be obtained simply
> by searching for a device belonging to the RNG uclass. Remove the
> superfluous platform function defined for the Qemu platform for
> getting the RNG device.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>

Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>   board/emulation/qemu-arm/qemu-arm.c | 42 -----------------------------
>   1 file changed, 42 deletions(-)
>
> diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
> index 16d5a97167..c9e886e44a 100644
> --- a/board/emulation/qemu-arm/qemu-arm.c
> +++ b/board/emulation/qemu-arm/qemu-arm.c
> @@ -107,48 +107,6 @@ void enable_caches(void)
>   	 dcache_enable();
>   }
>
> -#if defined(CONFIG_EFI_RNG_PROTOCOL)
> -#include <efi_loader.h>
> -#include <efi_rng.h>
> -
> -#include <dm/device-internal.h>
> -
> -efi_status_t platform_get_rng_device(struct udevice **dev)
> -{
> -	int ret;
> -	efi_status_t status = EFI_DEVICE_ERROR;
> -	struct udevice *bus, *devp;
> -
> -	for (uclass_first_device(UCLASS_VIRTIO, &bus); bus;
> -	     uclass_next_device(&bus)) {
> -		for (device_find_first_child(bus, &devp); devp;
> -		     device_find_next_child(&devp)) {
> -			if (device_get_uclass_id(devp) == UCLASS_RNG) {
> -				*dev = devp;
> -				status = EFI_SUCCESS;
> -				break;
> -			}
> -		}
> -	}
> -
> -	if (status != EFI_SUCCESS) {
> -		debug("No rng device found\n");
> -		return EFI_DEVICE_ERROR;
> -	}
> -
> -	if (*dev) {
> -		ret = device_probe(*dev);
> -		if (ret)
> -			return EFI_DEVICE_ERROR;
> -	} else {
> -		debug("Couldn't get child device\n");
> -		return EFI_DEVICE_ERROR;
> -	}
> -
> -	return EFI_SUCCESS;
> -}
> -#endif /* CONFIG_EFI_RNG_PROTOCOL */
> -
>   #ifdef CONFIG_ARM64
>   #define __W	"w"
>   #else

[PATCH 10/10] cmd: rng: Add support for selecting RNG device

[Sughosh Ganu]

The 'rng' u-boot command is used for printing a select number of
random bytes on the console. Currently, the RNG device from which the
random bytes are read is fixed. However, a platform can have multiple
RNG devices, one example being qemu, which has a virtio RNG device and
the RNG pseudo device through the TPM chip.

Extend the 'rng' command so that the user can provide the RNG device
number from which the random bytes are to be read. This will be the
device index under the RNG uclass.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 cmd/rng.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/cmd/rng.c b/cmd/rng.c
index 1ad5a096c0..2b197e322e 100644
--- a/cmd/rng.c
+++ b/cmd/rng.c
@@ -13,19 +13,34 @@
 
 static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
-	size_t n = 0x40;
+	size_t n;
 	struct udevice *dev;
 	void *buf;
+	int devnum;
 	int ret = CMD_RET_SUCCESS;
 
-	if (uclass_get_device(UCLASS_RNG, 0, &dev) || !dev) {
+	switch (argc) {
+	case 1:
+		devnum = 0;
+		n = 0x40;
+		break;
+	case 2:
+		devnum = hextoul(argv[1], NULL);
+		n = 0x40;
+		break;
+	case 3:
+		devnum = hextoul(argv[1], NULL);
+		n = hextoul(argv[2], NULL);
+		break;
+	default:
+		return CMD_RET_USAGE;
+	}
+
+	if (uclass_get_device(UCLASS_RNG, devnum, &dev) || !dev) {
 		printf("No RNG device\n");
 		return CMD_RET_FAILURE;
 	}
 
-	if (argc >= 2)
-		n = hextoul(argv[1], NULL);
-
 	buf = malloc(n);
 	if (!buf) {
 		printf("Out of memory\n");
@@ -46,12 +61,12 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 
 #ifdef CONFIG_SYS_LONGHELP
 static char rng_help_text[] =
-	"[n]\n"
-	"  - print n random bytes\n";
+	"[dev] [n]\n"
+	"  - print n random bytes read from dev\n";
 #endif
 
 U_BOOT_CMD(
-	rng, 2, 0, do_rng,
+	rng, 3, 0, do_rng,
 	"print bytes from the hardware random number generator",
 	rng_help_text
 );
-- 
2.17.1

Re: [PATCH 10/10] cmd: rng: Add support for selecting RNG device

[Heinrich Schuchardt]

On 2/24/22 19:05, Sughosh Ganu wrote:
> The 'rng' u-boot command is used for printing a select number of
> random bytes on the console. Currently, the RNG device from which the
> random bytes are read is fixed. However, a platform can have multiple
> RNG devices, one example being qemu, which has a virtio RNG device and
> the RNG pseudo device through the TPM chip.
>
> Extend the 'rng' command so that the user can provide the RNG device
> number from which the random bytes are to be read. This will be the
> device index under the RNG uclass.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   cmd/rng.c | 31 +++++++++++++++++++++++--------
>   1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/cmd/rng.c b/cmd/rng.c
> index 1ad5a096c0..2b197e322e 100644
> --- a/cmd/rng.c
> +++ b/cmd/rng.c
> @@ -13,19 +13,34 @@
>
>   static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   {
> -	size_t n = 0x40;
> +	size_t n;
>   	struct udevice *dev;
>   	void *buf;
> +	int devnum;
>   	int ret = CMD_RET_SUCCESS;
>
> -	if (uclass_get_device(UCLASS_RNG, 0, &dev) || !dev) {
> +	switch (argc) {
> +	case 1:
> +		devnum = 0;
> +		n = 0x40;
> +		break;
> +	case 2:
> +		devnum = hextoul(argv[1], NULL);
> +		n = 0x40;
> +		break;
> +	case 3:
> +		devnum = hextoul(argv[1], NULL);
> +		n = hextoul(argv[2], NULL);
> +		break;
> +	default:
> +		return CMD_RET_USAGE;
> +	}
> +
> +	if (uclass_get_device(UCLASS_RNG, devnum, &dev) || !dev) {
>   		printf("No RNG device\n");
>   		return CMD_RET_FAILURE;
>   	}
>
> -	if (argc >= 2)
> -		n = hextoul(argv[1], NULL);
> -
>   	buf = malloc(n);
>   	if (!buf) {
>   		printf("Out of memory\n");
> @@ -46,12 +61,12 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>
>   #ifdef CONFIG_SYS_LONGHELP
>   static char rng_help_text[] =
> -	"[n]\n"
> -	"  - print n random bytes\n";
> +	"[dev] [n]\n"

Thanks for updating the command.

I think this should be

     [dev [n]]

to indicate that dev takes precedence if only one parameter is provided.

A man-page doc/usage/rng.rst should be added in future.

Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>


> +	"  - print n random bytes read from dev\n";
>   #endif
>
>   U_BOOT_CMD(
> -	rng, 2, 0, do_rng,
> +	rng, 3, 0, do_rng,
>   	"print bytes from the hardware random number generator",
>   	rng_help_text
>   );