[PATCH] tpm_tis_spi: Don't send anything during flow control

[PATCH] tpm_tis_spi: Don't send anything during flow control

[Douglas Anderson]

During flow control we are just reading from the TPM, yet our spi_xfer
has the tx_buf and rx_buf both non-NULL which means we're requesting a
full duplex transfer.

SPI is always somewhat of a full duplex protocol anyway and in theory
the other side shouldn't really be looking at what we're sending it
during flow control, but it's still a bit ugly to be sending some
"random" data when we shouldn't.

The default tpm_tis_spi_flow_control() tries to address this by
setting 'phy->iobuf[0] = 0'.  This partially avoids the problem of
sending "random" data, but since our tx_buf and rx_buf both point to
the same place I believe there is the potential of us sending the
TPM's previous byte back to it if we hit the retry loop.

Another flow control implementation, cr50_spi_flow_control(), doesn't
address this at all.

Let's clean this up and just make the tx_buf NULL before we call
flow_control().  Not only does this ensure that we're not sending any
"random" bytes but it also possibly could make the SPI controller
behave in a slightly more optimal way.

NOTE: no actual observed problems are fixed by this patch--it's was
just made based on code inspection.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/char/tpm/tpm_tis_spi_main.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
index d96755935529..8d2c581a93c6 100644
--- a/drivers/char/tpm/tpm_tis_spi_main.c
+++ b/drivers/char/tpm/tpm_tis_spi_main.c
@@ -53,8 +53,6 @@ static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy,
 
 	if ((phy->iobuf[3] & 0x01) == 0) {
 		// handle SPI wait states
-		phy->iobuf[0] = 0;
-
 		for (i = 0; i < TPM_RETRY; i++) {
 			spi_xfer->len = 1;
 			spi_message_init(&m);
@@ -104,6 +102,8 @@ int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
 		if (ret < 0)
 			goto exit;
 
+		/* Flow control transfers are receive only */
+		spi_xfer.tx_buf = NULL;
 		ret = phy->flow_control(phy, &spi_xfer);
 		if (ret < 0)
 			goto exit;
@@ -113,9 +113,8 @@ int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
 		spi_xfer.delay.value = 5;
 		spi_xfer.delay.unit = SPI_DELAY_UNIT_USECS;
 
-		if (in) {
-			spi_xfer.tx_buf = NULL;
-		} else if (out) {
+		if (out) {
+			spi_xfer.tx_buf = phy->iobuf;
 			spi_xfer.rx_buf = NULL;
 			memcpy(phy->iobuf, out, transfer_len);
 			out += transfer_len;
-- 
2.27.0.rc0.183.gde8f92d652-goog

Re: [PATCH] tpm_tis_spi: Don't send anything during flow control

[Paul Menzel]

Dear Douglas,


Thank you for the patch.

Am 29.05.20 um 00:19 schrieb Douglas Anderson:
> During flow control we are just reading from the TPM, yet our spi_xfer
> has the tx_buf and rx_buf both non-NULL which means we're requesting a
> full duplex transfer.
> 
> SPI is always somewhat of a full duplex protocol anyway and in theory
> the other side shouldn't really be looking at what we're sending it
> during flow control, but it's still a bit ugly to be sending some
> "random" data when we shouldn't.
> 
> The default tpm_tis_spi_flow_control() tries to address this by
> setting 'phy->iobuf[0] = 0'.  This partially avoids the problem of
> sending "random" data, but since our tx_buf and rx_buf both point to
> the same place I believe there is the potential of us sending the
> TPM's previous byte back to it if we hit the retry loop.
> 
> Another flow control implementation, cr50_spi_flow_control(), doesn't
> address this at all.
> 
> Let's clean this up and just make the tx_buf NULL before we call
> flow_control().  Not only does this ensure that we're not sending any
> "random" bytes but it also possibly could make the SPI controller
> behave in a slightly more optimal way.
> 
> NOTE: no actual observed problems are fixed by this patch--it's was
> just made based on code inspection.

s/it's was/it was/

Were you able to test this? Maybe in the “Chromebook QA arsenal”? Are
you already running it in production on Google Chrome OS devices?

> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>   drivers/char/tpm/tpm_tis_spi_main.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
> index d96755935529..8d2c581a93c6 100644
> --- a/drivers/char/tpm/tpm_tis_spi_main.c
> +++ b/drivers/char/tpm/tpm_tis_spi_main.c
> @@ -53,8 +53,6 @@ static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy,
>   
>   	if ((phy->iobuf[3] & 0x01) == 0) {
>   		// handle SPI wait states
> -		phy->iobuf[0] = 0;
> -
>   		for (i = 0; i < TPM_RETRY; i++) {
>   			spi_xfer->len = 1;
>   			spi_message_init(&m);
> @@ -104,6 +102,8 @@ int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
>   		if (ret < 0)
>   			goto exit;
>   
> +		/* Flow control transfers are receive only */
> +		spi_xfer.tx_buf = NULL;
>   		ret = phy->flow_control(phy, &spi_xfer);
>   		if (ret < 0)
>   			goto exit;
> @@ -113,9 +113,8 @@ int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
>   		spi_xfer.delay.value = 5;
>   		spi_xfer.delay.unit = SPI_DELAY_UNIT_USECS;
>   
> -		if (in) {
> -			spi_xfer.tx_buf = NULL;
> -		} else if (out) {
> +		if (out) {
> +			spi_xfer.tx_buf = phy->iobuf;
>   			spi_xfer.rx_buf = NULL;
>   			memcpy(phy->iobuf, out, transfer_len);
>   			out += transfer_len;

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

Re: [PATCH] tpm_tis_spi: Don't send anything during flow control

[Doug Anderson]

Hi,

On Fri, May 29, 2020 at 1:33 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Douglas,
>
>
> Thank you for the patch.
>
> Am 29.05.20 um 00:19 schrieb Douglas Anderson:
> > During flow control we are just reading from the TPM, yet our spi_xfer
> > has the tx_buf and rx_buf both non-NULL which means we're requesting a
> > full duplex transfer.
> >
> > SPI is always somewhat of a full duplex protocol anyway and in theory
> > the other side shouldn't really be looking at what we're sending it
> > during flow control, but it's still a bit ugly to be sending some
> > "random" data when we shouldn't.
> >
> > The default tpm_tis_spi_flow_control() tries to address this by
> > setting 'phy->iobuf[0] = 0'.  This partially avoids the problem of
> > sending "random" data, but since our tx_buf and rx_buf both point to
> > the same place I believe there is the potential of us sending the
> > TPM's previous byte back to it if we hit the retry loop.
> >
> > Another flow control implementation, cr50_spi_flow_control(), doesn't
> > address this at all.
> >
> > Let's clean this up and just make the tx_buf NULL before we call
> > flow_control().  Not only does this ensure that we're not sending any
> > "random" bytes but it also possibly could make the SPI controller
> > behave in a slightly more optimal way.
> >
> > NOTE: no actual observed problems are fixed by this patch--it's was
> > just made based on code inspection.
>
> s/it's was/it was/

I'll assume this is easier to get fixed up when applying the patch,
but happy to send a v2 if requested.


> Were you able to test this? Maybe in the “Chromebook QA arsenal”? Are
> you already running it in production on Google Chrome OS devices?

I've tested it locally but not much more than that.  As far as I can
tell it doesn't break anything or fix anything but still seems like a
good change to make.  ;-)

Generally Chrome OS tries to work with an "upstream first" philosophy.
That has lots of benefits, but one downside is it means that patches
get sent upstream before they've actually landed anywhere.  That being
said, even if we landed this today we'd get limited testing because of
the Chrome OS kernel trees only the Chrome OS 5.4 tree uses the
upstream driver for Cr50 (the one SPI-connected security chip that
Chromebooks use).  Cr50 originally did not get developed as "upstream
first", so all the old codebases have the old downstream driver.  This
problem doesn't affect the old downstream driver which had its own
"struct spi_transfer".  If anything it makes the upstream code behave
more like the old downstream driver.  If you want to see the old
downstream driver:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.19/drivers/char/tpm/cr50_spi.c#172


Another note is that I don't have access to any SPI-connected chips
that are _not_ Cr50.  That means that half of this change is totally
untested but looks sane.  If anyone can test it that'd be great.  :-)


> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> >   drivers/char/tpm/tpm_tis_spi_main.c | 9 ++++-----
> >   1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
> > index d96755935529..8d2c581a93c6 100644
> > --- a/drivers/char/tpm/tpm_tis_spi_main.c
> > +++ b/drivers/char/tpm/tpm_tis_spi_main.c
> > @@ -53,8 +53,6 @@ static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy,
> >
> >       if ((phy->iobuf[3] & 0x01) == 0) {
> >               // handle SPI wait states
> > -             phy->iobuf[0] = 0;
> > -
> >               for (i = 0; i < TPM_RETRY; i++) {
> >                       spi_xfer->len = 1;
> >                       spi_message_init(&m);
> > @@ -104,6 +102,8 @@ int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
> >               if (ret < 0)
> >                       goto exit;
> >
> > +             /* Flow control transfers are receive only */
> > +             spi_xfer.tx_buf = NULL;
> >               ret = phy->flow_control(phy, &spi_xfer);
> >               if (ret < 0)
> >                       goto exit;
> > @@ -113,9 +113,8 @@ int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
> >               spi_xfer.delay.value = 5;
> >               spi_xfer.delay.unit = SPI_DELAY_UNIT_USECS;
> >
> > -             if (in) {
> > -                     spi_xfer.tx_buf = NULL;
> > -             } else if (out) {
> > +             if (out) {
> > +                     spi_xfer.tx_buf = phy->iobuf;
> >                       spi_xfer.rx_buf = NULL;
> >                       memcpy(phy->iobuf, out, transfer_len);
> >                       out += transfer_len;
>
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>

Thanks for reviewing!  :-)

-Doug

Re: [PATCH] tpm_tis_spi: Don't send anything during flow control

[Jarkko Sakkinen]

On Thu, May 28, 2020 at 03:19:30PM -0700, Douglas Anderson wrote:
> During flow control we are just reading from the TPM, yet our spi_xfer
> has the tx_buf and rx_buf both non-NULL which means we're requesting a
> full duplex transfer.
> 
> SPI is always somewhat of a full duplex protocol anyway and in theory
> the other side shouldn't really be looking at what we're sending it
> during flow control, but it's still a bit ugly to be sending some
> "random" data when we shouldn't.
> 
> The default tpm_tis_spi_flow_control() tries to address this by
> setting 'phy->iobuf[0] = 0'.  This partially avoids the problem of
> sending "random" data, but since our tx_buf and rx_buf both point to
> the same place I believe there is the potential of us sending the
> TPM's previous byte back to it if we hit the retry loop.
> 
> Another flow control implementation, cr50_spi_flow_control(), doesn't
> address this at all.
> 
> Let's clean this up and just make the tx_buf NULL before we call
> flow_control().  Not only does this ensure that we're not sending any
> "random" bytes but it also possibly could make the SPI controller
> behave in a slightly more optimal way.
> 
> NOTE: no actual observed problems are fixed by this patch--it's was
> just made based on code inspection.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  drivers/char/tpm/tpm_tis_spi_main.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
> index d96755935529..8d2c581a93c6 100644
> --- a/drivers/char/tpm/tpm_tis_spi_main.c
> +++ b/drivers/char/tpm/tpm_tis_spi_main.c
> @@ -53,8 +53,6 @@ static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy,
>  
>  	if ((phy->iobuf[3] & 0x01) == 0) {
>  		// handle SPI wait states
> -		phy->iobuf[0] = 0;
> -

Why this should be removed?

/Jarkko

Re: [PATCH] tpm_tis_spi: Don't send anything during flow control

[Doug Anderson]

Hi,

On Sun, May 31, 2020 at 6:47 PM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Thu, May 28, 2020 at 03:19:30PM -0700, Douglas Anderson wrote:
> > During flow control we are just reading from the TPM, yet our spi_xfer
> > has the tx_buf and rx_buf both non-NULL which means we're requesting a
> > full duplex transfer.
> >
> > SPI is always somewhat of a full duplex protocol anyway and in theory
> > the other side shouldn't really be looking at what we're sending it
> > during flow control, but it's still a bit ugly to be sending some
> > "random" data when we shouldn't.
> >
> > The default tpm_tis_spi_flow_control() tries to address this by
> > setting 'phy->iobuf[0] = 0'.  This partially avoids the problem of
> > sending "random" data, but since our tx_buf and rx_buf both point to
> > the same place I believe there is the potential of us sending the
> > TPM's previous byte back to it if we hit the retry loop.
> >
> > Another flow control implementation, cr50_spi_flow_control(), doesn't
> > address this at all.
> >
> > Let's clean this up and just make the tx_buf NULL before we call
> > flow_control().  Not only does this ensure that we're not sending any
> > "random" bytes but it also possibly could make the SPI controller
> > behave in a slightly more optimal way.
> >
> > NOTE: no actual observed problems are fixed by this patch--it's was
> > just made based on code inspection.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> >  drivers/char/tpm/tpm_tis_spi_main.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
> > index d96755935529..8d2c581a93c6 100644
> > --- a/drivers/char/tpm/tpm_tis_spi_main.c
> > +++ b/drivers/char/tpm/tpm_tis_spi_main.c
> > @@ -53,8 +53,6 @@ static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy,
> >
> >       if ((phy->iobuf[3] & 0x01) == 0) {
> >               // handle SPI wait states
> > -             phy->iobuf[0] = 0;
> > -
>
> Why this should be removed?

As far as I can tell the only purpose of that was to make sure we were
sending 0.  Specifically "tx_buf" "rx_buf" both point to "phy->iobuf"
so setting the first byte to 0 here made sure that we weren't sending
out "random" data but were instead sending a 0.  After my change
"tx_buf" is NULL so we don't need to do this--the controller should
take charge of sending nothing on the lines (AKA sending a zero).

Does that answer your question, or were you worried about us needing
to init iobuf[0] to 0 in some other case?

-Doug

Re: [PATCH] tpm_tis_spi: Don't send anything during flow control

[Jarkko Sakkinen]

On Mon, Jun 01, 2020 at 03:54:03PM -0700, Doug Anderson wrote:
> Does that answer your question, or were you worried about us needing
> to init iobuf[0] to 0 in some other case?
> 
> -Doug

No, but it should be treated as a bug fix for CR50 implementation i.e.
for 797c0113c9a481d4554988d70b5b52fae657262f, or is there some reason
why it shouldn't?

/Jarkko

Re: [PATCH] tpm_tis_spi: Don't send anything during flow control

[Doug Anderson]

Hi,

On Thu, Jun 4, 2020 at 2:40 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Mon, Jun 01, 2020 at 03:54:03PM -0700, Doug Anderson wrote:
> > Does that answer your question, or were you worried about us needing
> > to init iobuf[0] to 0 in some other case?
> >
> > -Doug
>
> No, but it should be treated as a bug fix for CR50 implementation i.e.
> for 797c0113c9a481d4554988d70b5b52fae657262f, or is there some reason
> why it shouldn't?

As talked about in the commit message, I think this is a slight
cleanup for non-Cr50 too.  Specifically if we end up running through
the TPM_RETRY loop a second time we weren't re-initting "phy->iobuf[0]
= 0;"  That means that the 2nd time through the loop we were actually
sending the TPM back the byte that the TPM sent us the first time
through the loop.

Presumably this doesn't matter much, but it still feels nicer not to
be sending the TPM's bytes back to it when we're not really supposed
to.

Also, as mentioned in the commit message, I haven't observed this
fixing any problems.  I only came up with it from code inspection
while trying to track something else down.


-Doug

Re: [PATCH] tpm_tis_spi: Don't send anything during flow control

[Jarkko Sakkinen]

On Thu, Jun 04, 2020 at 06:48:58AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, Jun 4, 2020 at 2:40 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Mon, Jun 01, 2020 at 03:54:03PM -0700, Doug Anderson wrote:
> > > Does that answer your question, or were you worried about us needing
> > > to init iobuf[0] to 0 in some other case?
> > >
> > > -Doug
> >
> > No, but it should be treated as a bug fix for CR50 implementation i.e.
> > for 797c0113c9a481d4554988d70b5b52fae657262f, or is there some reason
> > why it shouldn't?
> 
> As talked about in the commit message, I think this is a slight
> cleanup for non-Cr50 too.  Specifically if we end up running through
> the TPM_RETRY loop a second time we weren't re-initting "phy->iobuf[0]
> = 0;"  That means that the 2nd time through the loop we were actually
> sending the TPM back the byte that the TPM sent us the first time
> through the loop.
> 
> Presumably this doesn't matter much, but it still feels nicer not to
> be sending the TPM's bytes back to it when we're not really supposed
> to.
> 
> Also, as mentioned in the commit message, I haven't observed this
> fixing any problems.  I only came up with it from code inspection
> while trying to track something else down.

Thanks, I'm happy how it is.

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko