[PATCH 2/2] tpm: tpm_crb: enhance resource mapping mechanism for supporting AMD's fTPM

["Safford, David (GE Global Research, US)" <david.safford@ge.com>]

> From: Seunghun Han <kkamagui@gmail.com>
> Sent: Friday, August 30, 2019 12:55 PM
> To: Safford, David (GE Global Research, US) <david.safford@ge.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>; Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com>; Peter Huewe <peterhuewe@gmx.de>;
> open list:TPM DEVICE DRIVER <linux-integrity@vger.kernel.org>; Linux
> Kernel Mailing List <linux-kernel@vger.kernel.org>
> Subject: EXT: Re: [PATCH 2/2] tpm: tpm_crb: enhance resource mapping
> mechanism for supporting AMD's fTPM
> 
> >
> > > From: linux-integrity-owner@vger.kernel.org <linux-integrity-
> > > owner@vger.kernel.org> On Behalf Of Seunghun Han
> > > Sent: Friday, August 30, 2019 9:55 AM
> > > To: Jason Gunthorpe <jgg@ziepe.ca>
> > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe
> > > <peterhuewe@gmx.de>; open list:TPM DEVICE DRIVER <linux-
> > > integrity@vger.kernel.org>; Linux Kernel Mailing List <linux-
> > > kernel@vger.kernel.org>
> > > Subject: EXT: Re: [PATCH 2/2] tpm: tpm_crb: enhance resource mapping
> > > mechanism for supporting AMD's fTPM
> > >
> > > >
> > > > On Fri, Aug 30, 2019 at 06:56:39PM +0900, Seunghun Han wrote:
> > > > > I got an AMD system which had a Ryzen Threadripper 1950X and MSI
> > > > > mainboard, and I had a problem with AMD's fTPM. My machine
> > > > > showed
> > > an
> > > > > error message below, and the fTPM didn't work because of it.
> > > > >
> > > > > [  5.732084] tpm_crb MSFT0101:00: can't request region for resource
> > > > >              [mem 0x79b4f000-0x79b4ffff] [  5.732089] tpm_crb:
> > > > > probe of MSFT0101:00 failed with error -16
> > > > >
> > > > > When I saw the iomem, I found two fTPM regions were in the ACPI
> > > > > NVS
> > > area.
> > > > > The regions are below.
> > > > >
> > > > > 79a39000-79b6afff : ACPI Non-volatile Storage
> > > > >   79b4b000-79b4bfff : MSFT0101:00
> > > > >   79b4f000-79b4ffff : MSFT0101:00
> > > > >
> > > > > After analyzing this issue, I found that crb_map_io() function
> > > > > called
> > > > > devm_ioremap_resource() and it failed. The ACPI NVS didn't allow
> > > > > the TPM CRB driver to assign a resource in it because a busy bit
> > > > > was set to the ACPI NVS area.
> > > > >
> > > > > To support AMD's fTPM, I added a function to check intersects
> > > > > between the TPM region and ACPI NVS before it mapped the region.
> > > > > If some intersects are detected, the function just calls
> > > > > devm_ioremap() for a workaround. If there is no intersect, it
> > > > > calls
> > > devm_ioremap_resource().
> > > > >
> > > > > Signed-off-by: Seunghun Han <kkamagui@gmail.com>
> > > > > ---
> > > > >  drivers/char/tpm/tpm_crb.c | 25 +++++++++++++++++++++++--
> > > > >  1 file changed, 23 insertions(+), 2 deletions(-)
> > > >
> > > > This still seems to result in two drivers controlling the same memory.
> > > > Does this create bugs and races during resume?
> > > >
> > > > Jason
> > >
> > > When I tested this patch in my machine, it seemed that ACPI NVS was
> > > saved after TPM CRB driver sent "TPM2_Shutdown(STATE)" to the fTPM
> > > while suspending. Then, ACPI NVS was restored while resuming.
> > > After resuming, PCRs didn't change and TPM2 tools such as
> > > tpm2_pcrlist, tpm2_extend, tpm2_getrandoms worked well.
> > > So, according to my test result, it seems that the patch doesn't
> > > create bugs and race during resume.
> > >
> > > Seunghun
> >
> > This was discussed earlier on the list.
> > The consensus was that, while safe now, this would be fragile, and
> > subject to unexpected changes in ACPI/NVS, and we really need to tell
> > NVS to exclude the regions for long term safety.
> 
> Thank you for your advice. We also discussed earlier and concluded that
> checking and raw remapping are enough to work around this. The link is
> here, https://lkml.org/lkml/2019/8/29/962 .

I don't see Matthew Garrett's agreement on that thread.

> > As separate issues, the patches do not work at all on some of my AMD
> systems.
> > First, you only force the remap if the overlap is with NVS, but I have
> > systems where the overlap is with other reserved regions. You should
> > force the remap regardless, but if it is NVS, grab the space back from NVS.
> 
> I didn't know about that. I just found the case from your thread that AMD
> system assigned TPM region into the reserved area. However, as I know, the
> reserved area has no busy bit so that TPM CRB driver could assign buffer
> resources in it. Right? In my view, if you patched your TPM driver with my
> patch series, then it could work. Would you explain what happened in TPM
> CRB driver and reserved area?

Good question. I'll try it out this weekend.

> > Second, the patch extends the wrong behavior of the current driver to
> > both buffer regions. If there is a conflict between what the device's
> > control register says, and what ACPI says, the existing driver explicitly
> "trusts the ACPI".
> > This is just wrong. The actual device will use the areas as defined by
> > its control registers regardless of what ACPI says. I talked to
> > Microsoft, and their driver trusts the control register values, and
> > doesn't even look at the ACPI values.
> 
> As you know, the original code trusts the ACPI table because of the
> workaround for broken BIOS, and this code has worked well for a long time.
> In my view, if we change this code to trust control register value, we could
> make new problems and need a lot of time to check the workaround. So, I
> want to trust the ACPI value now.

I don't think the workaround has every really worked, other than the 
Helpful firmware error it emits.  I don't think anyone has tested the
workaround with large requests. The tpm_crb device itself is telling
us the buffers it is using. Why are we ignoring that and trusting the
known bad ACPI tables? Makes no sense to me.

> >
> > In practice, I have tested several systems in which the device
> > registers show The correct 4K buffers, but the driver instead trusts
> > the ACPI values, which list just 1K buffers. 1K buffers will not work
> > for large requests, and the device is going to read and write the 4K buffers
> regardless.
> >
> > dave
> 
> I know about that. However, the device driver is not going to read and write
> 4K buffers if you patch your TPM driver with my patch series.
> One of my patches has an enhancement feature that could calculate the
> buffer size well. The TPM driver uses exactly 1K buffer for this case, not 4K
> buffer, and it works.

Have you tested large requests (well over 1K) to make sure?

dave 
> 
> Seunghun