Re: [PATCH] PCI: Check rom image addr at every step

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

On Fri, Nov 14, 2025 at 02:34:11PM +0800, Guixin Liu wrote:
> We meet a crash when running stress-ng:

+ blank line.

>   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

Please, read
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages
and act accordingly (I think of 4 least significant lines)

+ blank line.

> Bcause of broken rom space, before calling readl(pds), pds already
> points to the rom space end (rom + size - 1), invoking readl()
> would therefore cause an out-of-bounds access and trigger a crash.
> 
> Fix this by adding every step address checking.

From the description and the code I'm not sure this is the best approach. Since
the accesses seem to be not 4-byte aligned, perhaps readl() should be split to
something shorter in such cases? Dunno, I haven't looked at the code.

Ah, it seems we are looking for the full 4 bytes to match. But then we need more, no?
See below.

...

> +#define PCI_ROM_DATA_STRUCT_OFFSET 24
> +#define PCI_ROM_LAST_IMAGE_OFFSET 21
> +#define PCI_ROM_LAST_IMAGE_LEN_OFFSET 16

Are those based on PCI specifications? Perhaps if we go this way the reference
to the spec needs to be added.

...

> static size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom,

>  	void __iomem *image;
>  	int last_image;
>  	unsigned int length;
> +	void __iomem *end = rom + size;

Can you group together IOMEM addresses?

	void __iomem *end = rom + size;
	void __iomem *image;
	int last_image;
	unsigned int length;

>  
>  	image = rom;
>  	do {
>  		void __iomem *pds;

> +		if (image + 2 >= end)
> +			break;

Shouldn't we rather check the size to be at least necessary minimum? With this
done, this check won't be needed here. Or we would need another one to check
for the length for the entire structure needed.

>  		/* 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;
>  		}

> +		if (image + PCI_ROM_DATA_STRUCT_OFFSET + 2 >= end)
> +			break;
>  		/* get the PCI data structure and check its "PCIR" signature */
> -		pds = image + readw(image + 24);
> +		pds = image + readw(image + PCI_ROM_DATA_STRUCT_OFFSET);
> +		if (pds + 4 >= end)
> +			break;
>  		if (readl(pds) != 0x52494350) {
>  			pci_info(pdev, "Invalid PCI ROM data signature: expecting 0x52494350, got %#010x\n",
>  				 readl(pds));

You also want to reconsider double readl(). Would it have side-effects? What about hot-plug?

>  			break;
>  		}
> -		last_image = readb(pds + 21) & 0x80;
> -		length = readw(pds + 16);
> +
> +		if (pds + PCI_ROM_LAST_IMAGE_OFFSET + 1 >= end)
> +			break;
> +		last_image = readb(pds + PCI_ROM_LAST_IMAGE_OFFSET) & 0x80;
> +		length = readw(pds + PCI_ROM_LAST_IMAGE_LEN_OFFSET);
>  		image += length * 512;
>  		/* Avoid iterating through memory outside the resource window */
> -		if (image >= rom + size)
> +		if (image + 2 >= end)
>  			break;
>  		if (!last_image) {
>  			if (readw(image) != 0xAA55) {

I agree that defensive programming helps, but I think it's too much in this
case. We may relax and do less, but comprehensive checks.

...

Thanks for the testing and proposing a fix, nevertheless!


-- 
With Best Regards,
Andy Shevchenko