linux-kernel - Re: [PATCH v2] tpm/eventlog: Don't abort tpm_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YqF7NktlAJg26Bvd@iki.fi>
Date:   Thu, 9 Jun 2022 07:46:46 +0300
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     Morten Linderud <morten@...derud.pw>
Cc:     Peter Huewe <peterhuewe@....de>, Jason Gunthorpe <jgg@...pe.ca>,
        linux-integrity@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] tpm/eventlog: Don't abort tpm_read_log on faulty ACPI
 address

On Wed, Jun 08, 2022 at 02:31:08PM +0200, Morten Linderud wrote:
> tpm_read_log_acpi() should return -ENODEV when no eventlog from the ACPI
> table is found. If the firmware vendor includes an invalid log address
> we are unable to map from the ACPI memory and the function returns -EIO
> which would abort discovery of the eventlog.
> 
> This change ensure we always return -ENODEV in tpm_read_log_acpi() and
> fallback to the EFI configuration table.

Please do not use "we" in commit messages. Or start a sentence
with "this patch", "this commit" or "this change". It is always
best just to go down to the roots and use imperative form.

E.g. you could rephrase the last paragraph as

"Change the return value from -EIO to -ENODEV when acpi_os_map_iomem()
fails to map the event log."

> The following hardware was used to test this issue:
>     Framework Laptop (Pre-production)
>     BIOS: INSYDE Corp, Revision: 3.2
>     TPM Device: NTC, Firmware Revision: 7.2
> 
> Dump of the faulty 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@...derud.pw>
> 
> ---
> 
> v2: Tweak commit message and opt to return -ENODEV instead of loosening up the
>     if condition in tpm_read_log()
> 
> ---
>  drivers/char/tpm/eventlog/acpi.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c
> index 1b18ce5ebab1..2b15d6eebd69 100644
> --- a/drivers/char/tpm/eventlog/acpi.c
> +++ b/drivers/char/tpm/eventlog/acpi.c
> @@ -136,8 +136,12 @@ 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__);
> +		/* try EFI log next */
> +		ret = -ENODEV;
>  		goto err;
> +	}

It is wrong to try out EFI, if this fails. TPM2 ACPI table was already
detected.

>  
>  	memcpy_fromio(log->bios_event_log, virt, len);
>  
> -- 
> 2.36.1

What you are using this for? Without any actual bug report, this 
is an obvious NAK.

BR, Jarkko