Message-ID: <CAD=FV=XwQ9jo4kCFeunvBed2r1m0u-0-sKozhpmU6sbpYEotDA@mail.gmail.com>
Date:   Fri, 29 May 2020 08:37:59 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Paul Menzel <pmenzel@...gen.mpg.de>
Cc:     Peter Huewe <peterhuewe@....de>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Andrey Pronin <apronin@...omium.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jason Gunthorpe <jgg@...pe.ca>,
        linux-integrity@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] tpm_tis_spi: Don't send anything during flow control

Hi,

On Fri, May 29, 2020 at 1:33 AM Paul Menzel <pmenzel@...gen.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@...omium.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@...gen.mpg.de>

Thanks for reviewing!  :-)

-Doug

Powered by blists - more mailing lists