Re: [PATCH] tpm: tpm_crb: Add an AMD fTPM support feature

[Seunghun Han <kkamagui@gmail.com>]

> > From: linux-integrity-owner@vger.kernel.org <linux-integrity-
> > owner@vger.kernel.org> On Behalf Of Jarkko Sakkinen
> > Sent: Monday, August 26, 2019 1:59 AM
> > To: Seunghun Han <kkamagui@gmail.com>
> > Cc: Peter Huewe <peterhuewe@gmx.de>; Thomas Gleixner
> > <tglx@linutronix.de>; linux-kernel@vger.kernel.org; linux-
> > integrity@vger.kernel.org
> > Subject: EXT: Re: [PATCH] tpm: tpm_crb: Add an AMD fTPM support feature
> >
> > On Mon, Aug 26, 2019 at 02:40:19AM +0900, Seunghun Han wrote:
> > > I'm Seunghun Han and work at the Affiliated Institute of ETRI. 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 areas and found two TPM CRB regions were in the
> > > ACPI NVS area.  The iomem regions are below.
> > >
> > > 79a39000-79b6afff : ACPI Non-volatile Storage
> > >   79b4b000-79b4bfff : MSFT0101:00
> > >   79b4f000-79b4ffff : MSFT0101:00
> > >
> > > After analyzing this issue, I found out that a busy bit was set to the
> > > ACPI NVS area, and the current Linux kernel allowed nothing to be
> > > assigned in it. I also found that the kernel couldn't calculate the
> > > sizes of command and response buffers correctly when the TPM regions
> > were two or more.
> > >
> > > To support AMD's fTPM, I removed the busy bit from the ACPI NVS area
> > > so that AMD's fTPM regions could be assigned in it. I also fixed the
> > > bug that did not calculate the sizes of command and response buffer
> > correctly.
>
> The problem is that the acpi tables are _wrong_ in this and other cases.
> They not only incorrectly report the area as reserved, but also report
> the command and response buffer sizes incorrectly. If you look at
> the addresses for the buffers listed in the crb control area, the sizes
> are correct (4Kbytes).  My patch uses the control area values, and
> everything works. The remaining problem is that if acpi reports the
> area as NVS, then the linux nvs driver will try to use the space.
> I'm looking at how to fix that. I'm not sure, if simply removing
> the busy bit is sufficient.
> Dave

Thank you for your reply. I have read your patch. However, I would
like to solve this problem without a kernel parameter. In my view, I'd
better change the crb_fixup_cmd_size() function because the TPM CRB
driver wants to get the command buffer and response buffer sizes by
using the function.

I agree on that removing the busy bit is sufficient.

Seunghun

>
> > >
> > > Signed-off-by: Seunghun Han <kkamagui@gmail.com>
> >
> > You need to split this into multiple patches e.g. if you think you've fixed a
> > bug, please write a patch with just the bug fix and nothing else.
> >
> > For further information, read the section three of
> >
> > https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> >
> > I'd also recommend to check out the earlier discussion on ACPI NVS:
> >
> > https://lore.kernel.org/linux-
> > integrity/BCA04D5D9A3B764C9B7405BBA4D4A3C035EF7BC7@ALPMBAPA12.e
> > 2k.ad.ge.com/
> >
> > /Jarkko