[PATCH RESEND] x86/pci: Check signature before assigning shadow ROM

[PATCH RESEND] x86/pci: Check signature before assigning shadow ROM

[Tomita Moeko]

Modern platforms without VBIOS or UEFI CSM support do not contain
VGA ROM at 0xC0000, this is observed on Intel Ice Lake and later
systems. Check whether the VGA ROM region is a valid PCI option ROM
with 0xAA55 signature before assigning the shadow ROM to device.

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
Note:
This patch was originally sent on 2025/04/06, but did not receive any
feedback. Resending in case it was missed. The commit message is updated
in this version.

 arch/x86/pci/fixup.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index e7e71490bd25..362afaaf9874 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -317,6 +317,7 @@ static void pci_fixup_video(struct pci_dev *pdev)
 	struct pci_bus *bus;
 	u16 config;
 	struct resource *res;
+	void __iomem *rom;
 
 	/* Is VGA routed to us? */
 	bus = pdev->bus;
@@ -338,9 +339,12 @@ static void pci_fixup_video(struct pci_dev *pdev)
 		}
 		bus = bus->parent;
 	}
-	if (!vga_default_device() || pdev == vga_default_device()) {
+
+	rom = ioremap(0xC0000, 0x20000);
+	if (rom && (!vga_default_device() || pdev == vga_default_device())) {
 		pci_read_config_word(pdev, PCI_COMMAND, &config);
-		if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
+		if ((config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) &&
+		    (readw(rom) == 0xAA55)) {
 			res = &pdev->resource[PCI_ROM_RESOURCE];
 
 			pci_disable_rom(pdev);
@@ -354,6 +358,7 @@ static void pci_fixup_video(struct pci_dev *pdev)
 			dev_info(&pdev->dev, "Video device with shadowed ROM at %pR\n",
 				 res);
 		}
+		iounmap(rom);
 	}
 }
 DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID,
-- 
2.50.1

Re: [PATCH RESEND] x86/pci: Check signature before assigning shadow ROM

[Lukas Wunner]

On Sat, Aug 16, 2025 at 12:20:41AM +0800, Tomita Moeko wrote:
> Modern platforms without VBIOS or UEFI CSM support do not contain
> VGA ROM at 0xC0000, this is observed on Intel Ice Lake and later
> systems. Check whether the VGA ROM region is a valid PCI option ROM
> with 0xAA55 signature before assigning the shadow ROM to device.

Which spec is the 0xAA55 magic number coming from?

Could you add a spec reference for it in a code comment and the
commit message?

I note that arch/x86/kernel/probe_roms.c contains ...

  #define ROMSIGNATURE 0xaa55

... and a function romsignature() to check the signature.
I'm wondering why that existing check isn't sufficient?
Why is it necessary to check again elsewhere?

> +++ b/arch/x86/pci/fixup.c
> @@ -317,6 +317,7 @@ static void pci_fixup_video(struct pci_dev *pdev)
>  	struct pci_bus *bus;
>  	u16 config;
>  	struct resource *res;
> +	void __iomem *rom;
>  
>  	/* Is VGA routed to us? */
>  	bus = pdev->bus;
> @@ -338,9 +339,12 @@ static void pci_fixup_video(struct pci_dev *pdev)
>  		}
>  		bus = bus->parent;
>  	}
> -	if (!vga_default_device() || pdev == vga_default_device()) {
> +
> +	rom = ioremap(0xC0000, 0x20000);

There's a code comment preceding pci_fixup_video() which says that
"BIOS [is] copied to 0xC0000 in system RAM".  So this isn't MMIO,
it's system memory and you can use memremap() instead if ioremap().

Since you're only interested in the first two bytes, you don't need
to map the whole 0x20000 bytes.

Instead of amending the if-condition ...

> +	if (rom && (!vga_default_device() || pdev == vga_default_device())) {
>  		pci_read_config_word(pdev, PCI_COMMAND, &config);
> -		if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
> +		if ((config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) &&
> +		    (readw(rom) == 0xAA55)) {
>  			res = &pdev->resource[PCI_ROM_RESOURCE];

... you could just return on failure to find a valid signature, i.e.:

+	rom = memremap(0xC0000, sizeof(sig), MEMREMAP_WB);
+	if (!rom)
+		return;
+
+	memcpy(&sig, rom, sizeof(sig));
+	memunmap(rom);
+	if (sig != 0xAA55)
+		return;

May want to emit an error on failure to memremap().

Amending the if-condition makes it messier to find an offending commit
with "git blame" (more iterations needed).  And returning early reduces
indentation levels per section 1 of Documentation/process/coding-style.rst.

Thanks,

Lukas

Re: [PATCH RESEND] x86/pci: Check signature before assigning shadow ROM

[Tomita Moeko]

Sorry for my extreme late reply...I didn't noticed this message
until I searched this topic in lore.kernel.org. Somehow it didn't
reached my inbox, or maybe I deleted it by mistake :(

On 8/17/2025 5:45 PM, Lukas Wunner wrote:
> On Sat, Aug 16, 2025 at 12:20:41AM +0800, Tomita Moeko wrote:
>> Modern platforms without VBIOS or UEFI CSM support do not contain
>> VGA ROM at 0xC0000, this is observed on Intel Ice Lake and later
>> systems. Check whether the VGA ROM region is a valid PCI option ROM
>> with 0xAA55 signature before assigning the shadow ROM to device.
> 
> Which spec is the 0xAA55 magic number coming from?
> 
> Could you add a spec reference for it in a code comment and the
> commit message?

Sorry I am unable to find the exact spec as PCI-SIG only prvide spec
access to their members. I think I should reuse the ROMSIGNATURE macro
mentioned below.
> I note that arch/x86/kernel/probe_roms.c contains ...
> 
>   #define ROMSIGNATURE 0xaa55
> 
> ... and a function romsignature() to check the signature.
> I'm wondering why that existing check isn't sufficient?
> Why is it necessary to check again elsewhere?

The check itself for accessing the Option ROM is sufficient, kernel reports
"Invalid PCI ROM header signature: expecting 0xaa55, got 0x****" error when
an invalid ROM is accessed.

However, the issue here is that the VGA BIOS region is always exposed as
PCI Option ROM on the primary VGA device, even when it doesn’t actually
contain a valid Option ROM (for example, on modern platforms without
VBIOS). In such cases, exposing this region as an Option ROM is unnecessary
and results in misleading "Invalid PCI ROM header signature" errors.
Checking the signature before exposing the region helps to avoid that.
>> +++ b/arch/x86/pci/fixup.c
>> @@ -317,6 +317,7 @@ static void pci_fixup_video(struct pci_dev *pdev)
>>  	struct pci_bus *bus;
>>  	u16 config;
>>  	struct resource *res;
>> +	void __iomem *rom;
>>  
>>  	/* Is VGA routed to us? */
>>  	bus = pdev->bus;
>> @@ -338,9 +339,12 @@ static void pci_fixup_video(struct pci_dev *pdev)
>>  		}
>>  		bus = bus->parent;
>>  	}
>> -	if (!vga_default_device() || pdev == vga_default_device()) {
>> +
>> +	rom = ioremap(0xC0000, 0x20000);
> 
> There's a code comment preceding pci_fixup_video() which says that
> "BIOS [is] copied to 0xC0000 in system RAM".  So this isn't MMIO,
> it's system memory and you can use memremap() instead if ioremap().
> 
> Since you're only interested in the first two bytes, you don't need
> to map the whole 0x20000 bytes.
> 
> Instead of amending the if-condition ...
> 
>> +	if (rom && (!vga_default_device() || pdev == vga_default_device())) {
>>  		pci_read_config_word(pdev, PCI_COMMAND, &config);
>> -		if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
>> +		if ((config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) &&
>> +		    (readw(rom) == 0xAA55)) {
>>  			res = &pdev->resource[PCI_ROM_RESOURCE];
> 
> ... you could just return on failure to find a valid signature, i.e.:
> 
> +	rom = memremap(0xC0000, sizeof(sig), MEMREMAP_WB);
> +	if (!rom)
> +		return;
> +
> +	memcpy(&sig, rom, sizeof(sig));
> +	memunmap(rom);
> +	if (sig != 0xAA55)
> +		return;
> 
> May want to emit an error on failure to memremap().
> 
> Amending the if-condition makes it messier to find an offending commit
> with "git blame" (more iterations needed).  And returning early reduces
> indentation levels per section 1 of Documentation/process/coding-style.rst.

Got it, will fix in v2.

Thanks,
Moeko
> 
> Thanks,
> 
> Lukas
>