| Message ID | 20210920203447.4124005-1-morten@linderud.pw (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm/eventlog: Don't abort tpm_read_log on faulty ACPI config | expand |
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
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!
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
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
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
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);
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(-)