linux-kernel - Re: [PATCHv2] PCI/ACPI: _DSM PRESERVE_BOOT_CONFIG function rev id doesn't match with spec.

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20241210195001.GA3254053@bhelgaas>
Date: Tue, 10 Dec 2024 13:50:01 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Zhou Shengqing <zhoushengqing@...info.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>, linux-acpi@...r.kernel.org,
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>
Subject: Re: [PATCHv2] PCI/ACPI: _DSM PRESERVE_BOOT_CONFIG function rev id
 doesn't match with spec.

[+cc Ben, original author of a78cf9657ba5]

On Thu, Nov 14, 2024 at 03:04:24AM +0000, Zhou Shengqing wrote:
> Per PCI Firmware Specification Revision 3.3 Table 4-7 _DSM Definitions
> for PCI. Preserve PCI Boot Configuration Initial Revision ID is 2. But
> the code is 1.

This _DSM function 5 was added in PCI Firmware r3.1, released Dec 13,
2010.  It's listed in sec 4.6 with Revision 2 (as *all* the defined
functions are, even functions 1-4, which were included in r3.0 with
Revision 1).

But the actual definition that was added in r3.1 is in sec 4.6.5,
which specifies Revision ID 1.  

PCI Firmware r3.2, released Jan 26, 2015, was the newest available at
the time Ben implemented a78cf9657ba5 ("PCI/ACPI: Evaluate PCI Boot
Configuration _DSM"), and sec 4.6.5 still specified Revision ID 1.

So I think Ben's addition used the correct Revision ID (1).

PCI Firmware r3.3, released Jan 20, 2021, changed sec 4.6.5 to say
"lowest valid Revision ID value: 2"

I think it's a mistake to make the kernel change below because
platforms in the field implemented function 5 with revision 1 (per the
r3.1 and r3.2 specs), and we have no idea whether they implement
function 5 revision 2.

It's quite likely that newer platforms following r3.3 will implement
function 5 revision 2, but NOT revision 1, and the existing code won't 
work for them.

I think the fix is to try revision 1 and, if that isn't implemented,
we should try revision 2.  The semantics stayed the same, so they
should both work the same.

> Fixes: 9d7d5db8e78e ("PCI: Move PRESERVE_BOOT_CONFIG _DSM evaluation to pci_register_host_bridge()")
> Origin fixes: a78cf9657ba5 ("PCI/ACPI: Evaluate PCI Boot Configuration _DSM")
> 
> Signed-off-by: Zhou Shengqing <zhoushengqing@...info.com>
> ---
>  drivers/pci/pci-acpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index af370628e583..7a4cad0c1f00 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -132,7 +132,7 @@ bool pci_acpi_preserve_config(struct pci_host_bridge *host_bridge)
>  		 */
>  		obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev),
>  					      &pci_acpi_dsm_guid,
> -					      1, DSM_PCI_PRESERVE_BOOT_CONFIG,
> +					      2, DSM_PCI_PRESERVE_BOOT_CONFIG,
>  					      NULL, ACPI_TYPE_INTEGER);
>  		if (obj && obj->integer.value == 0)
>  			return true;
> -- 
> 2.39.2
>