Re: [PATCH v3] PCI: Check rom header and data structure addr before accessing

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Nov 26, 2025 at 03:26:23PM +0800, Guixin Liu wrote:
> We meet a crash when running stress-ng on x86_64 machine:
> 
>   BUG: unable to handle page fault for address: ffa0000007f40000
>   RIP: 0010:pci_get_rom_size+0x52/0x220
>   Call Trace:
>   <TASK>
>     pci_map_rom+0x80/0x130
>     pci_read_rom+0x4b/0xe0
>     kernfs_file_read_iter+0x96/0x180
>     vfs_read+0x1b1/0x300
> 
> Our analysis reveals that the rom space's start address is
> 0xffa0000007f30000, and size is 0x10000. Because of broken rom
> space, before calling readl(pds), the pds's value is
> 0xffa0000007f3ffff, which is already pointed to the rom space
> end, invoking readl() would read 4 bytes therefore cause an
> out-of-bounds access and trigger a crash.
> Fix this by adding image header and data structure checking.
> 
> We also found another crash on arm64 machine:
> 
>   Unable to handle kernel paging request at virtual address
> ffff8000dd1393ff
>   Mem abort info:
>   ESR = 0x0000000096000021
>   EC = 0x25: DABT (current EL), IL = 32 bits
>   SET = 0, FnV = 0
>   EA = 0, S1PTW = 0
>   FSC = 0x21: alignment fault
> 
> The call trace is the same with x86_64, but the crash reason is
> that the data structure addr is not aligned with 4, and arm64
> machine report "alignment fault". Fix this by adding alignment
> checking.

Thanks for the update, looks much better now!
My comments below.

...

> +static inline bool pci_rom_header_valid(struct pci_dev *pdev,
> +					void __iomem *image,
> +					void __iomem *rom,
> +					size_t size,
> +					bool last_image)
> +{
> +	uintptr_t rom_end = (uintptr_t)rom + size;
> +	uintptr_t header_end;

Note: Linus told that kernel should not use uintptr_t.

s/uintptr_t/unsigned long/g

and here in some cases we even don't need that type at all.

> +	if (check_add_overflow((uintptr_t)image, PCI_ROM_HEADER_SIZE,
> +	    &header_end))
> +		return false;

> +	if (image >= rom && header_end < rom_end &&
> +	    IS_ALIGNED((uintptr_t)image, 2)) {

So, why not

	/* Check if we have enough space in ROM */
	if (image < rom || header_end > rom_end)
		return false;

	/* ARM requires proper alignment */
/// Find a better comment text for above.
	if (!IS_ALIGNED((unsigned long)image, 2UL))
		return false;

> +		/* Standard PCI ROMs start out with these bytes 55 AA */
> +		if (readw(image) == 0xAA55)
> +			return true;
> +
> +		if (!last_image)
> +			pci_info(pdev, "No more image in the PCI ROM\n");
> +		else
> +			pci_info(pdev, "Invalid PCI ROM header signature: expecting 0xaa55, got %#06x\n",
> +				 readw(image));
> +	}
> +	return false;
> +}

...

> +static inline bool pci_rom_data_struct_valid(struct pci_dev *pdev,
> +					     void __iomem *pds,
> +					     void __iomem *rom,
> +					     size_t size)

Similar comments as per above.

...

>  	image = rom;
>  	do {
>  		void __iomem *pds;
> +
> +		if (!pci_rom_header_valid(pdev, image, rom, size, true))
>  			break;
> +
>  		/* get the PCI data structure and check its "PCIR" signature */
>  		pds = image + readw(image + 24);
> +		if (!pci_rom_data_struct_valid(pdev, pds, rom, size))
>  			break;
> +
>  		last_image = readb(pds + 21) & 0x80;
>  		length = readw(pds + 16);
>  		image += length * 512;

> +		if (!pci_rom_header_valid(pdev, image, rom, size, (bool)last_image))

This casting is a bit odd. Can we avoid doing like this?

>  	} while (length && !last_image);

-- 
With Best Regards,
Andy Shevchenko