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

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

On Wed, Nov 19, 2025 at 06:11:16PM +0800, Guixin Liu wrote:

Thanks for the update, my comments below.

> We meet a crash when running stress-ng:
> 
>   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
>     ksys_read+0x63/0xe0
>     do_syscall_64+0x34/0x80
>     entry_SYSCALL_64_after_hwframe+0x78/0xe2

You missed my comment on these lines. Have you read Submitting Patches
documentation?

> 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.

...

>  static size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom,
>  			       size_t size)
>  {
> +	void __iomem *end = rom + size;
>  	void __iomem *image;
>  	int last_image;
>  	unsigned int length;

>  	image = rom;
>  	do {
>  		void __iomem *pds;
> +
> +		if (image + PCI_ROM_HEADER_SIZE >= end)
> +			break;
> +
>  		/* Standard PCI ROMs start out with these bytes 55 AA */
>  		if (readw(image) != 0xAA55) {
>  			pci_info(pdev, "Invalid PCI ROM header signature: expecting 0xaa55, got %#06x\n",
>  				 readw(image));
>  			break;
>  		}
> +
>  		/* get the PCI data structure and check its "PCIR" signature */
>  		pds = image + readw(image + 24);
> +		if (pds + PCI_ROM_DATA_STRUCT_SIZE >= end)
> +			break;
>  		if (readl(pds) != 0x52494350) {
>  			pci_info(pdev, "Invalid PCI ROM data signature: expecting 0x52494350, got %#010x\n",
>  				 readl(pds));

>  		last_image = readb(pds + 21) & 0x80;
>  		length = readw(pds + 16);
>  		image += length * 512;
> +
>  		/* Avoid iterating through memory outside the resource window */
> -		if (image >= rom + size)

> +		if (image + PCI_ROM_HEADER_SIZE >= end)

Theoretically this can overflow and become a false condition when should be
true. Check overflow.h if they have some helpers for wraparound checks.

So, first you need to validate the "end" and/or "size".

	image = rom;
	do {
		void __iomem *pds;

		if (size < PCI_ROM_HEADER_SIZE)
			break;
		...
		size -= ... // not sure if we can change this variable, though
	} while (...);

>  			break;
>  		if (!last_image) {
>  			if (readw(image) != 0xAA55) {

-- 
With Best Regards,
Andy Shevchenko