[PATCH 0/1] tpm: fix uninitalized field access

[PATCH 0/1] tpm: fix uninitalized field access

[lukas.funke-oss]

From: Lukas Funke <lukas.funke@weidmueller.com>


tpm_tis_wait_init() is using the 'chip->timeout_b' field which is
initialized in tpm_tis_init(). However, the init-function is called
*after* tpm_tis_wait_init() introducing an uninitalized field access.

This series/commit fixes the issue.



Lukas Funke (1):
  tpm: call tpm_tis_wait_init() after tpm_tis_init()

 drivers/tpm/tpm2_tis_spi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.30.2

[PATCH 1/1] tpm: call tpm_tis_wait_init() after tpm_tis_init()

[lukas.funke-oss]

From: Lukas Funke <lukas.funke@weidmueller.com>

tpm_tis_wait_init() is using the 'chip->timeout_b' field which is
initialized in tpm_tis_init(). However, the init-function is called
*after* tpm_tis_wait_init() introducing an uninitalized field access.

This commit switches both routines.

Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com>
---

 drivers/tpm/tpm2_tis_spi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
index b0fe97ab1d0..5a4dbfd3ccb 100644
--- a/drivers/tpm/tpm2_tis_spi.c
+++ b/drivers/tpm/tpm2_tis_spi.c
@@ -256,17 +256,17 @@ static int tpm_tis_spi_probe(struct udevice *dev)
 	/* Ensure a minimum amount of time elapsed since reset of the TPM */
 	mdelay(drv_data->time_before_first_cmd_ms);
 
+	tpm_tis_ops_register(dev, &phy_ops);
+	ret = tpm_tis_init(dev);
+	if (ret)
+		goto err;
+
 	ret = tpm_tis_wait_init(dev, chip->locality);
 	if (ret) {
 		log(LOGC_DM, LOGL_ERR, "%s: no device found\n", __func__);
 		return ret;
 	}
 
-	tpm_tis_ops_register(dev, &phy_ops);
-	ret = tpm_tis_init(dev);
-	if (ret)
-		goto err;
-
 	priv->pcr_count = drv_data->pcr_count;
 	priv->pcr_select_min = drv_data->pcr_select_min;
 	priv->version = TPM_V2;
-- 
2.30.2

Re: [PATCH 1/1] tpm: call tpm_tis_wait_init() after tpm_tis_init()

[Miquel Raynal]

Hi Lukas,

lukas.funke-oss@weidmueller.com wrote on Mon, 15 Jul 2024 13:23:01
+0200:

> From: Lukas Funke <lukas.funke@weidmueller.com>
> 
> tpm_tis_wait_init() is using the 'chip->timeout_b' field which is
> initialized in tpm_tis_init(). However, the init-function is called
> *after* tpm_tis_wait_init() introducing an uninitalized field access.
> 
> This commit switches both routines.
> 
> Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com>
> ---
> 
>  drivers/tpm/tpm2_tis_spi.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
> index b0fe97ab1d0..5a4dbfd3ccb 100644
> --- a/drivers/tpm/tpm2_tis_spi.c
> +++ b/drivers/tpm/tpm2_tis_spi.c
> @@ -256,17 +256,17 @@ static int tpm_tis_spi_probe(struct udevice *dev)
>  	/* Ensure a minimum amount of time elapsed since reset of the TPM */
>  	mdelay(drv_data->time_before_first_cmd_ms);
>  
> +	tpm_tis_ops_register(dev, &phy_ops);
> +	ret = tpm_tis_init(dev);
> +	if (ret)
> +		goto err;
> +

Strange, I don't remember wait the init was done after the wait_init,
but at a first glance the fix looks fine.

Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>

>  	ret = tpm_tis_wait_init(dev, chip->locality);
>  	if (ret) {
>  		log(LOGC_DM, LOGL_ERR, "%s: no device found\n", __func__);
>  		return ret;
>  	}
>  
> -	tpm_tis_ops_register(dev, &phy_ops);
> -	ret = tpm_tis_init(dev);
> -	if (ret)
> -		goto err;
> -
>  	priv->pcr_count = drv_data->pcr_count;
>  	priv->pcr_select_min = drv_data->pcr_select_min;
>  	priv->version = TPM_V2;


Thanks,
Miquèl

Re: [PATCH 1/1] tpm: call tpm_tis_wait_init() after tpm_tis_init()

[Simon Glass]

Hi,

On Mon, 15 Jul 2024 at 12:23, <lukas.funke-oss@weidmueller.com> wrote:
>
> From: Lukas Funke <lukas.funke@weidmueller.com>
>
> tpm_tis_wait_init() is using the 'chip->timeout_b' field which is
> initialized in tpm_tis_init(). However, the init-function is called
> *after* tpm_tis_wait_init() introducing an uninitalized field access.
>
> This commit switches both routines.
>
> Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com>
> ---
>
>  drivers/tpm/tpm2_tis_spi.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
> index b0fe97ab1d0..5a4dbfd3ccb 100644
> --- a/drivers/tpm/tpm2_tis_spi.c
> +++ b/drivers/tpm/tpm2_tis_spi.c
> @@ -256,17 +256,17 @@ static int tpm_tis_spi_probe(struct udevice *dev)
>         /* Ensure a minimum amount of time elapsed since reset of the TPM */
>         mdelay(drv_data->time_before_first_cmd_ms);
>
> +       tpm_tis_ops_register(dev, &phy_ops);
> +       ret = tpm_tis_init(dev);
> +       if (ret)
> +               goto err;
> +
>         ret = tpm_tis_wait_init(dev, chip->locality);
>         if (ret) {
>                 log(LOGC_DM, LOGL_ERR, "%s: no device found\n", __func__);
>                 return ret;
>         }
>
> -       tpm_tis_ops_register(dev, &phy_ops);
> -       ret = tpm_tis_init(dev);
> -       if (ret)
> -               goto err;
> -
>         priv->pcr_count = drv_data->pcr_count;
>         priv->pcr_select_min = drv_data->pcr_select_min;
>         priv->version = TPM_V2;
> --
> 2.30.2
>

This needs a Fixes tag for a5c30c26b28 (HEAD) tpm: Use the new API on
tpm2 spi driver

The old code set up the timeouts first, then did the wait_init.
Presumably the point of wait_init is to wait before doing the init, so
we should try to keep that behaviour, unless it is actually wrong.

So my thought would be to move the setup of the required timeout into
tpm_tis_ops_register(), instead.

Regards,
Simon

Re: [PATCH 1/1] tpm: call tpm_tis_wait_init() after tpm_tis_init()

[Ilias Apalodimas]

Hi all

On Sun, 21 Jul 2024 at 13:08, Simon Glass <sjg@chromium.org> wrote:
>
> Hi,
>
> On Mon, 15 Jul 2024 at 12:23, <lukas.funke-oss@weidmueller.com> wrote:
> >
> > From: Lukas Funke <lukas.funke@weidmueller.com>
> >
> > tpm_tis_wait_init() is using the 'chip->timeout_b' field which is
> > initialized in tpm_tis_init(). However, the init-function is called
> > *after* tpm_tis_wait_init() introducing an uninitalized field access.
> >
> > This commit switches both routines.
> >
> > Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com>
> > ---
> >
> >  drivers/tpm/tpm2_tis_spi.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
> > index b0fe97ab1d0..5a4dbfd3ccb 100644
> > --- a/drivers/tpm/tpm2_tis_spi.c
> > +++ b/drivers/tpm/tpm2_tis_spi.c
> > @@ -256,17 +256,17 @@ static int tpm_tis_spi_probe(struct udevice *dev)
> >         /* Ensure a minimum amount of time elapsed since reset of the TPM */
> >         mdelay(drv_data->time_before_first_cmd_ms);
> >
> > +       tpm_tis_ops_register(dev, &phy_ops);
> > +       ret = tpm_tis_init(dev);
> > +       if (ret)
> > +               goto err;
> > +
> >         ret = tpm_tis_wait_init(dev, chip->locality);
> >         if (ret) {
> >                 log(LOGC_DM, LOGL_ERR, "%s: no device found\n", __func__);
> >                 return ret;
> >         }
> >
> > -       tpm_tis_ops_register(dev, &phy_ops);
> > -       ret = tpm_tis_init(dev);
> > -       if (ret)
> > -               goto err;
> > -
> >         priv->pcr_count = drv_data->pcr_count;
> >         priv->pcr_select_min = drv_data->pcr_select_min;
> >         priv->version = TPM_V2;
> > --
> > 2.30.2
> >
>
> This needs a Fixes tag for a5c30c26b28 (HEAD) tpm: Use the new API on
> tpm2 spi driver
>

Yes please we need a fixes tag

> The old code set up the timeouts first, then did the wait_init.
> Presumably the point of wait_init is to wait before doing the init, so
> we should try to keep that behaviour, unless it is actually wrong.
>
> So my thought would be to move the setup of the required timeout into
> tpm_tis_ops_register(), instead.

tpm_tis_ops_register() is setting up the bus accesses and I'd prefer
to keep it that way.
Since this is a static function, we can fold it in tpm_tis_init(),
which makes more sense

Thanks
/Ilias
>
> Regards,
> Simon

Re: [PATCH 1/1] tpm: call tpm_tis_wait_init() after tpm_tis_init()

[Simon Glass]

Hi Ilias

On Mon, 22 Jul 2024 at 08:35, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi all
>
> On Sun, 21 Jul 2024 at 13:08, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi,
> >
> > On Mon, 15 Jul 2024 at 12:23, <lukas.funke-oss@weidmueller.com> wrote:
> > >
> > > From: Lukas Funke <lukas.funke@weidmueller.com>
> > >
> > > tpm_tis_wait_init() is using the 'chip->timeout_b' field which is
> > > initialized in tpm_tis_init(). However, the init-function is called
> > > *after* tpm_tis_wait_init() introducing an uninitalized field access.
> > >
> > > This commit switches both routines.
> > >
> > > Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com>
> > > ---
> > >
> > >  drivers/tpm/tpm2_tis_spi.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
> > > index b0fe97ab1d0..5a4dbfd3ccb 100644
> > > --- a/drivers/tpm/tpm2_tis_spi.c
> > > +++ b/drivers/tpm/tpm2_tis_spi.c
> > > @@ -256,17 +256,17 @@ static int tpm_tis_spi_probe(struct udevice *dev)
> > >         /* Ensure a minimum amount of time elapsed since reset of the TPM */
> > >         mdelay(drv_data->time_before_first_cmd_ms);
> > >
> > > +       tpm_tis_ops_register(dev, &phy_ops);
> > > +       ret = tpm_tis_init(dev);
> > > +       if (ret)
> > > +               goto err;
> > > +
> > >         ret = tpm_tis_wait_init(dev, chip->locality);
> > >         if (ret) {
> > >                 log(LOGC_DM, LOGL_ERR, "%s: no device found\n", __func__);
> > >                 return ret;
> > >         }
> > >
> > > -       tpm_tis_ops_register(dev, &phy_ops);
> > > -       ret = tpm_tis_init(dev);
> > > -       if (ret)
> > > -               goto err;
> > > -
> > >         priv->pcr_count = drv_data->pcr_count;
> > >         priv->pcr_select_min = drv_data->pcr_select_min;
> > >         priv->version = TPM_V2;
> > > --
> > > 2.30.2
> > >
> >
> > This needs a Fixes tag for a5c30c26b28 (HEAD) tpm: Use the new API on
> > tpm2 spi driver
> >
>
> Yes please we need a fixes tag
>
> > The old code set up the timeouts first, then did the wait_init.
> > Presumably the point of wait_init is to wait before doing the init, so
> > we should try to keep that behaviour, unless it is actually wrong.
> >
> > So my thought would be to move the setup of the required timeout into
> > tpm_tis_ops_register(), instead.
>
> tpm_tis_ops_register() is setting up the bus accesses and I'd prefer
> to keep it that way.
> Since this is a static function, we can fold it in tpm_tis_init(),
> which makes more sense

Since this is used by i2c and SPI we need to make sure that both work.

I believe the way it used to be, some initial values were set for the
timeouts. The naming is somewhat confusing I suppose.

Regards,
Simon

Re: [PATCH 1/1] tpm: call tpm_tis_wait_init() after tpm_tis_init()

[Ilias Apalodimas]

Hi Simon

On Tue, 23 Jul 2024 at 15:47, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias
>
> On Mon, 22 Jul 2024 at 08:35, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi all
> >
> > On Sun, 21 Jul 2024 at 13:08, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, 15 Jul 2024 at 12:23, <lukas.funke-oss@weidmueller.com> wrote:
> > > >
> > > > From: Lukas Funke <lukas.funke@weidmueller.com>
> > > >
> > > > tpm_tis_wait_init() is using the 'chip->timeout_b' field which is
> > > > initialized in tpm_tis_init(). However, the init-function is called
> > > > *after* tpm_tis_wait_init() introducing an uninitalized field access.
> > > >
> > > > This commit switches both routines.
> > > >
> > > > Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com>
> > > > ---
> > > >
> > > >  drivers/tpm/tpm2_tis_spi.c | 10 +++++-----
> > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/tpm/tpm2_tis_spi.c b/drivers/tpm/tpm2_tis_spi.c
> > > > index b0fe97ab1d0..5a4dbfd3ccb 100644
> > > > --- a/drivers/tpm/tpm2_tis_spi.c
> > > > +++ b/drivers/tpm/tpm2_tis_spi.c
> > > > @@ -256,17 +256,17 @@ static int tpm_tis_spi_probe(struct udevice *dev)
> > > >         /* Ensure a minimum amount of time elapsed since reset of the TPM */
> > > >         mdelay(drv_data->time_before_first_cmd_ms);
> > > >
> > > > +       tpm_tis_ops_register(dev, &phy_ops);
> > > > +       ret = tpm_tis_init(dev);
> > > > +       if (ret)
> > > > +               goto err;
> > > > +
> > > >         ret = tpm_tis_wait_init(dev, chip->locality);
> > > >         if (ret) {
> > > >                 log(LOGC_DM, LOGL_ERR, "%s: no device found\n", __func__);
> > > >                 return ret;
> > > >         }
> > > >
> > > > -       tpm_tis_ops_register(dev, &phy_ops);
> > > > -       ret = tpm_tis_init(dev);
> > > > -       if (ret)
> > > > -               goto err;
> > > > -
> > > >         priv->pcr_count = drv_data->pcr_count;
> > > >         priv->pcr_select_min = drv_data->pcr_select_min;
> > > >         priv->version = TPM_V2;
> > > > --
> > > > 2.30.2
> > > >
> > >
> > > This needs a Fixes tag for a5c30c26b28 (HEAD) tpm: Use the new API on
> > > tpm2 spi driver
> > >
> >
> > Yes please we need a fixes tag
> >
> > > The old code set up the timeouts first, then did the wait_init.
> > > Presumably the point of wait_init is to wait before doing the init, so
> > > we should try to keep that behaviour, unless it is actually wrong.
> > >
> > > So my thought would be to move the setup of the required timeout into
> > > tpm_tis_ops_register(), instead.
> >
> > tpm_tis_ops_register() is setting up the bus accesses and I'd prefer
> > to keep it that way.
> > Since this is a static function, we can fold it in tpm_tis_init(),
> > which makes more sense
>
> Since this is used by i2c and SPI we need to make sure that both work.

The tpm_tis_wait_init function is SPI-specific right now. It was SPI
specifric before a5c30c26b28c6 as well. The i2c driver was after the
refactoring and it doesn't call tpm_tis_wait_init(). However, I think
we should call it on all drivers.

The only bus-specific bit is how we read back from the TPM. So I think
the right thing to do here is
1. Teach tpm_tis_wait_init() to call the proper bus function rather
than the hardcoded tpm_tis_spi_read()
2. call it from tpm_tis_init()

That way we'll call it regardless of the bus the TPM happens to sit on.

Regards
/Ilias

>
> I believe the way it used to be, some initial values were set for the
> timeouts. The naming is somewhat confusing I suppose.
>
> Regards,
> Simon