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

Sat Mar 12 03:43:44 CET 2022

Hi Sughosh,

On Sun, 6 Mar 2022 at 00:03, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> hi Simon,
>
> On Sun, 6 Mar 2022 at 08:37, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Fri, 4 Mar 2022 at 06:44, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Fri, 4 Mar 2022 at 08:07, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Thu, 3 Mar 2022 at 05:11, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Thu, 3 Mar 2022 at 09:18, Simon Glass <sjg at chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Tue, 1 Mar 2022 at 21:53, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > > >
> > > > > > > hi Simon,
> > > > > > >
> > > > > > > On Tue, 1 Mar 2022 at 20:29, Simon Glass <sjg at chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Sughosh,
> > > > > > > >
> > > > > > > > On Mon, 28 Feb 2022 at 05:07, Sughosh Ganu <sughosh.ganu at linaro.org> 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 at linaro.org>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > Changes since V1: None
> > > > > > > > >
> > > > > > > > >  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 should be in the Makefile so that we only build this file if TPM
> > > > > > > > is enabled.
> > > > > > >
> > > > > > > The Makefile allows for the tpm uclass driver to be built for SPL and
> > > > > > > TPL stages as well. The addition of the RNG device is to be done only
> > > > > > > in the u-boot proper stage, since we enable RNG support only in u-boot
> > > > > > > proper. Thanks.
> > > > > >
> > > > > > Well in that case, create a new SPL_TPM_RAND or similar to control
> > > > > > enabling it in SPL. It should be explicit.
> > > > >
> > > > > I think it is easier to just protect the child addition functions
> > > > > under CONFIG_TPM rather than create SPL_RNG and TPL_RNG symbols. We
> > > > > don't have any requirement for generating random numbers in the SPL
> > > > > and TPL stages. I feel that creating new symbols just for the sake of
> > > > > not putting a check for CONFIG_TPM is a bit of an overkill, especially
> > > > > since we do not have any requirement for RNG devices in the SPL/TPL
> > > > > stages.
> > > >
> > > > What does checking for CONFIG_TPM have to do with SPL and TPL? If that
> > > > option is enabled, the feature will be active in SPL and TPL too.
> > >
> > > Maybe I am not explaining it properly. We need the addition of the RNG
> > > child device only in the u-boot proper stage, not in the SPL and TPL
> > > stages. The TPM uclass driver can indeed be built for the SPL and TPL
> > > stages, while the RNG uclass is needed only for u-boot proper. So, the
> > > addition of the RNG child device done in the TPM uclass driver should
> > > only happen in u-boot proper, and not in SPL/TPL stages. Which is the
> > > reason for the CONFIG_TPM check.
> >
> > Let me try one more time. If this doesn't work, please chat with a colleague.
> >
> > The way we do that is with Kconfig options. We don't use ad-hoc
> > methods to enable things in U-Boot proper but not SPL. Boards that
> > have CONFIG_TPM set have it set everywhere, including in SPL. So
> > checking for CONFIG_TPM is going to return true in both U-Boot proper
> > and SPL. If you want different behaviour in SPL, you need
> > CONFIG_SPL_TPM. But even that is indirect, as I have explained. To
> > enable your driver in SPL, you need a CONFIG_SPL_...
>
> Okay. I now get your point. So this is because these config symbols
> are getting included in all the stages, so I cannot use the symbol
> name to identify the spl/tpl/u-boot stages. I believe earlier, the
> identification used to be done based on whether a symbol is CONFIG_xxx
> as against CONFIG_SPL_xxx. Now it is being done with
> CONFIG_{SPL,TPL}_BUILD symbols for differentiating between stages. I
> will use these instead to have the code enabled only for the u-boot
> proper stage. Thing is, I don't want to define these functions for the
> spl and tpl stages since they are adding to the size with no benefits.
> I do understand that currently no platform is enabling tpm drivers in
> the spl/tpl stage, but the driver can be enabled in those stages.

coral uses TPM in the VPL stage. In general, the TPM needs to be used
earlier than U-Boot proper, since verified boot has to be complete by
then.

>
> >
> > Also, you should use devicetree to provide the random number
> > generator, just a device_bind(). Add the node as a subnode of the TPM.
> > Otherwise it cannot work with OF_PLATDATA.
>
> As per the documentation in of-plat.rst, the OF_PLATDATA method is to
> be used only for the SPL/TPL stages where there might be a size
> constraint. And the tpm rng drivers are to be enabled only in the
> u-boot proper stage. So I think we would not need to put a subnode in
> the devicetree. Basically, the presence of the rng functionality in
> the TPM device is not optional as per my understanding, so the rng
> child node can just be added in the tpm uclass driver.

We don't need the subnode but it is normal practice to have one. If
you want to use a manual bind, that is OK, at least for U-Boot proper.

>
> >
> > >
> > > >
> > > > Also I see another problem, on further examination. You cannot start
> > > > up the TPM in the pre_probe() function. That needs to be done under
> > > > board control. E.g. for coral it happens in the TPL (or soon VPL)
> > > > phase so cannot be done again in U-Boot proper.
> > >
> > > I tested running the RNG command after the TPM device has already been
> > > probed and tpm_startup has been called. Even if I call the tpm_startup
> > > again, I do not see any issues. Does the TPM spec prohibit calling the
> > > initialisation function multiple times. I believe that the TPM device
> > > should be able to handle this scenario right?
> >
> > I hangs on coral, for example.
> >
> > >
> > > >
> > > > So perhaps we need to remember the state of the TPM (using SPL handoff
> > > > perhaps). Also you probably need to move the startup stuff to the RNG
> > > > itself.
> > >
> > > I can move the call to tpm_startup to the RNG driver's probe function.
> > > But I thought it is better that the parent device(TPM) is initialised
> > > before calling the probe of the child device.
> >
> > We use lazy init in U-Boot, so it should be possible to probe the TPM
> > without automatically starting it. Otherwise you are going to break
> > some of the tpm commands.
>
> Yes I understand. With the patch, the tpm device is being started only
> when a child device is being probed. But I can remove the startup of
> the tpm device from the child_pre_probe function. It will have to be
> done explicitly before the rng device is to be used.

Yes, that's how it should work, for now.

>
> Btw, can you please comment on patch 3 of this series[1].

I suspect it is in my queue but I have not had time yet.

Regards,
Simon

>
> -sughosh
>
> [1] - https://lists.denx.de/pipermail/u-boot/2022-March/476830.html
>
> >
> > >
> > > >
> > > > Perhaps we could add a new function to handle this, which can be
> > > > called from your rand driver.
> > > >
> > > > int tpm_ensure_started(struct udevice *dev, enum tpm_startup_type mode)
> > >
> > > Okay, I can add this, but the question is, does calling the
> > > tpm_startup function cause issues? If not, maybe this is not needed?
> >
> > It does on coral. I don't know whether other TPMs are tolerant of it,
> > but in any case it is not a good idea.
> >
> > Regards,
> > Simon