[PATCH] tpm/eventlog: Don't abort tpm_read_log on faulty ACPI config

[PATCH] tpm/eventlog: Don't abort tpm_read_log on faulty ACPI config

[Morten Linderud]

Some vendors report faulty values in the acpi TPM2 table. This causes
the function to abort with EIO and essentially short circuits the
tpm_read_log function as we never even attempt to read the EFI
configuration table for a log.

This changes the condition to only look for a positive return value,
else hands over the eventlog discovery to the EFI configuration table
which should hopefully work better.

It's unclear to me if there is a better solution to this then just
failing. However, I do not see any clear reason why we can't properly
fallback to the EFI configuration table.

The following hardware was used to test this issue on:
    Framework Laptop (Pre-production)
    BIOS: INSYDE Corp, Revision: 3.2
    TPM Device: NTC, Firmware Revision: 7.2

Dump of the fault ACPI TPM2 table:
    [000h 0000   4]                    Signature : "TPM2"    [Trusted Platform Module hardware interface Table]
    [004h 0004   4]                 Table Length : 0000004C
    [008h 0008   1]                     Revision : 04
    [009h 0009   1]                     Checksum : 2B
    [00Ah 0010   6]                       Oem ID : "INSYDE"
    [010h 0016   8]                 Oem Table ID : "TGL-ULT"
    [018h 0024   4]                 Oem Revision : 00000002
    [01Ch 0028   4]              Asl Compiler ID : "ACPI"
    [020h 0032   4]        Asl Compiler Revision : 00040000

    [024h 0036   2]               Platform Class : 0000
    [026h 0038   2]                     Reserved : 0000
    [028h 0040   8]              Control Address : 0000000000000000
    [030h 0048   4]                 Start Method : 06 [Memory Mapped I/O]

    [034h 0052  12]            Method Parameters : 00 00 00 00 00 00 00 00 00 00 00 00
    [040h 0064   4]           Minimum Log Length : 00010000
    [044h 0068   8]                  Log Address : 000000004053D000

Signed-off-by: Morten Linderud <morten@linderud.pw>
---
 drivers/char/tpm/eventlog/acpi.c   | 5 +++--
 drivers/char/tpm/eventlog/common.c | 6 +++++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c
index 1b18ce5ebab1..9ce39cdb0bd8 100644
--- a/drivers/char/tpm/eventlog/acpi.c
+++ b/drivers/char/tpm/eventlog/acpi.c
@@ -136,8 +136,10 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
 
 	ret = -EIO;
 	virt = acpi_os_map_iomem(start, len);
-	if (!virt)
+	if (!virt) {
+		dev_warn(&chip->dev, "%s: Failed to map acpi memory\n", __func__);
 		goto err;
+	}
 
 	memcpy_fromio(log->bios_event_log, virt, len);
 
@@ -145,7 +147,6 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2 &&
 	    !tpm_is_tpm2_log(log->bios_event_log, len)) {
-		/* try EFI log next */
 		ret = -ENODEV;
 		goto err;
 	}
diff --git a/drivers/char/tpm/eventlog/common.c b/drivers/char/tpm/eventlog/common.c
index 8512ec76d526..f64256bc2f89 100644
--- a/drivers/char/tpm/eventlog/common.c
+++ b/drivers/char/tpm/eventlog/common.c
@@ -83,7 +83,11 @@ static int tpm_read_log(struct tpm_chip *chip)
 	}
 
 	rc = tpm_read_log_acpi(chip);
-	if (rc != -ENODEV)
+	/*
+	 * only return if we found a log else we try look for a
+	 * log in the EFI configuration table
+	 */
+	if (rc > 0)
 		return rc;
 
 	rc = tpm_read_log_efi(chip);
-- 
2.33.0

Re: [PATCH] tpm/eventlog: Don't abort tpm_read_log on faulty ACPI config

[Jarkko Sakkinen]

On Mon, 2021-09-20 at 22:34 +0200, Morten Linderud wrote:
> Some vendors report faulty values in the acpi TPM2 table. This causes

Nit: ACPI (not acpi)

> the function to abort with EIO and essentially short circuits the
> tpm_read_log function as we never even attempt to read the EFI
> configuration table for a log.

Nit: tpm_read_log()

> This changes the condition to only look for a positive return value,
> else hands over the eventlog discovery to the EFI configuration table

"hands over" -> "fallback"

> which should hopefully work better.

Please write in imperative form, e.g. "Change...", or perhaps in this
case "Look...". 

Hopes are somewhat irrelevant, in the context of a commit message.

> It's unclear to me if there is a better solution to this then just
> failing. However, I do not see any clear reason why we can't properly
> fallback to the EFI configuration table.

Neither hopes nor doubts help us :-)

Because the commit message did not discuss any of the code changes
that were done it is very hard to say much anything of this yet.

There's also one corner case that was not discussed in the commit
message.

The historical reason for not using TPM2 file is that old TPM2's
did not have that feature. You have to ensure that legacy hardware
does not break.

/Jarkko

Re: [PATCH] tpm/eventlog: Don't abort tpm_read_log on faulty ACPI config

[Morten Linderud]

On Tue, Sep 21, 2021 at 09:58:11PM +0300, Jarkko Sakkinen wrote:
> On Mon, 2021-09-20 at 22:34 +0200, Morten Linderud wrote:
> > Some vendors report faulty values in the acpi TPM2 table. This causes
> 
> Nit: ACPI (not acpi)
> 
> > the function to abort with EIO and essentially short circuits the
> > tpm_read_log function as we never even attempt to read the EFI
> > configuration table for a log.
> 
> Nit: tpm_read_log()
> 
> > This changes the condition to only look for a positive return value,
> > else hands over the eventlog discovery to the EFI configuration table
> 
> "hands over" -> "fallback"
> 
> > which should hopefully work better.
> 
> Please write in imperative form, e.g. "Change...", or perhaps in this
> case "Look...". 
> 
> Hopes are somewhat irrelevant, in the context of a commit message.
> 
> > It's unclear to me if there is a better solution to this then just
> > failing. However, I do not see any clear reason why we can't properly
> > fallback to the EFI configuration table.
> 
> Neither hopes nor doubts help us :-)
> 
> Because the commit message did not discuss any of the code changes
> that were done it is very hard to say much anything of this yet.

Thanks for the review! First kernel patch so all feedback is welcome :)

The code change is essentially just relaxing the return value for the ACPI log
lookup. I'm not quite sure what is missing from the commit message in that
regard? Is the second paragraph insufficient?

> There's also one corner case that was not discussed in the commit
> message.
> 
> The historical reason for not using TPM2 file is that old TPM2's
> did not have that feature. You have to ensure that legacy hardware
> does not break.

This should only relax the cases where an error which is not ENODEV is returned.
Legacy hardware that return ENODEV because the table doesn't exist, or is
missing the log start and length, should be unaffected by this change.

> /Jarkko

Cheers!

-- 
Morten Linderud
PGP: 9C02FF419FECBE16

Re: [PATCH] tpm/eventlog: Don't abort tpm_read_log on faulty ACPI config

[Jarkko Sakkinen]

On Mon, Sep 20, 2021 at 10:34:47PM +0200, Morten Linderud wrote:
> Some vendors report faulty values in the acpi TPM2 table. This causes

s/acpi/ACPI/

> the function to abort with EIO and essentially short circuits the

s/the function/tpm_read_log()/

> tpm_read_log function as we never even attempt to read the EFI
> configuration table for a log.

> 
> This changes the condition to only look for a positive return value,
> else hands over the eventlog discovery to the EFI configuration table
> which should hopefully work better.

Please, write in imperative ("Change...").

Also exlicitly state how are you changing the check for
tpm_read_log_acpi() in tpm_read_log().

You could *even* have a snippet how the checks change
here for clarity.

> It's unclear to me if there is a better solution to this then just
> failing. However, I do not see any clear reason why we can't properly
> fallback to the EFI configuration table.

This paragraph should not be part of the commit message.

Rest of the commit message made sense can you add also fixes tag
as this is clearly a bug fix?

Also, please remove the two spurious diff's from the commit that
are not relevant for a stable bug fix (pr_warn() and comment
removal).

BR, Jarkko

Re: [PATCH] tpm/eventlog: Don't abort tpm_read_log on faulty ACPI config

[Morten Linderud]

On Wed, Oct 05, 2022 at 01:40:09AM +0300, Jarkko Sakkinen wrote:
> On Mon, Sep 20, 2021 at 10:34:47PM +0200, Morten Linderud wrote:
> > Some vendors report faulty values in the acpi TPM2 table. This causes
> 
> s/acpi/ACPI/
> 
> > the function to abort with EIO and essentially short circuits the
> 
> s/the function/tpm_read_log()/
> 
> > tpm_read_log function as we never even attempt to read the EFI
> > configuration table for a log.
> 
> > 
> > This changes the condition to only look for a positive return value,
> > else hands over the eventlog discovery to the EFI configuration table
> > which should hopefully work better.
> 
> Please, write in imperative ("Change...").
> 
> Also exlicitly state how are you changing the check for
> tpm_read_log_acpi() in tpm_read_log().
> 
> You could *even* have a snippet how the checks change
> here for clarity.
> 
> > It's unclear to me if there is a better solution to this then just
> > failing. However, I do not see any clear reason why we can't properly
> > fallback to the EFI configuration table.
> 
> This paragraph should not be part of the commit message.
> 
> Rest of the commit message made sense can you add also fixes tag
> as this is clearly a bug fix?
> 
> Also, please remove the two spurious diff's from the commit that
> are not relevant for a stable bug fix (pr_warn() and comment
> removal).

Yo,

This is the v1 of the patch which you reviewed a year ago.
https://marc.info/?l=linux-integrity&m=163225066613340&w=2

V2 mostly fixed the commit message, but there where some more pointers. I'm
happy to submit a V3 if we can agree on all the details.

V2 review is here:
https://marc.info/?l=linux-integrity&m=165475008823837&w=2

-- 
Morten Linderud
PGP: 9C02FF419FECBE16

Re: [PATCH] tpm/eventlog: Don't abort tpm_read_log on faulty ACPI config

[Jarkko Sakkinen]

On Wed, Oct 05, 2022 at 11:31:28AM +0200, Morten Linderud wrote:
> On Wed, Oct 05, 2022 at 01:40:09AM +0300, Jarkko Sakkinen wrote:
> > On Mon, Sep 20, 2021 at 10:34:47PM +0200, Morten Linderud wrote:
> > > Some vendors report faulty values in the acpi TPM2 table. This causes
> > 
> > s/acpi/ACPI/
> > 
> > > the function to abort with EIO and essentially short circuits the
> > 
> > s/the function/tpm_read_log()/
> > 
> > > tpm_read_log function as we never even attempt to read the EFI
> > > configuration table for a log.
> > 
> > > 
> > > This changes the condition to only look for a positive return value,
> > > else hands over the eventlog discovery to the EFI configuration table
> > > which should hopefully work better.
> > 
> > Please, write in imperative ("Change...").
> > 
> > Also exlicitly state how are you changing the check for
> > tpm_read_log_acpi() in tpm_read_log().
> > 
> > You could *even* have a snippet how the checks change
> > here for clarity.
> > 
> > > It's unclear to me if there is a better solution to this then just
> > > failing. However, I do not see any clear reason why we can't properly
> > > fallback to the EFI configuration table.
> > 
> > This paragraph should not be part of the commit message.
> > 
> > Rest of the commit message made sense can you add also fixes tag
> > as this is clearly a bug fix?
> > 
> > Also, please remove the two spurious diff's from the commit that
> > are not relevant for a stable bug fix (pr_warn() and comment
> > removal).
> 
> Yo,
> 
> This is the v1 of the patch which you reviewed a year ago.
> https://marc.info/?l=linux-integrity&m=163225066613340&w=2
> 
> V2 mostly fixed the commit message, but there where some more pointers. I'm
> happy to submit a V3 if we can agree on all the details.
> 
> V2 review is here:
> https://marc.info/?l=linux-integrity&m=165475008823837&w=2

Send v3 with fixes tag and it is fine.

BR, Jarkko