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

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

On Wed, Nov 26, 2025 at 08:57:27PM +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.

There are few comments, but in general it looks good to me.
So, if you address the below, feel free to add
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
And thanks for pursuing this issue!

...

> ---
> v3 -> v4:
> - Use "u64" instead of "uintptr_t".

Hmm... Do we have a use cases when u64 is required on, say,
32-bit unsigned long cases?

> - Invert the if statement to avoid excessive indentation.
> - Add comment for alignment checking.
> - Change last_image's type from int to bool.

...

> +static inline bool pci_rom_header_valid(struct pci_dev *pdev,

Usually we use verb 'is' in names of the boolean functions.

pci_rom_is_header_valid()

> +					void __iomem *image,
> +					void __iomem *rom,
> +					size_t size,
> +					bool last_image)
> +{
> +	u64 rom_end = (u64)rom + size;
> +	u64 header_end;
> +
> +	/*
> +	 * Some CPU architectures require IOMEM access addresses to
> +	 * be aligned, for example arm64, so since we're about to
> +	 * call readw(), we check here for 2-byte alignment.
> +	 */
> +	if (!IS_ALIGNED((u64)image, 2))
> +		return false;
> +
> +	if (check_add_overflow((u64)image, PCI_ROM_HEADER_SIZE, &header_end))
> +		return false;
> +
> +	if (image < rom || header_end >= rom_end)

But header_end == rom_end is valid case for _header_, no? Then we should fail
later on.

> +		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));


The positive conditional can be used (easier to parse)

	if (last_image)
		...
	else
		...

> +	return false;
> +}
> +
> +static inline bool pci_rom_data_struct_valid(struct pci_dev *pdev,

pci_rom_is_data_struct_valid()

> +					     void __iomem *pds,
> +					     void __iomem *rom,
> +					     size_t size)
> +{
> +	u64 rom_end = (u64)rom + size;
> +	u64 end;
> +	u16 data_len;
> +
> +	/*
> +	 * Some CPU architectures require IOMEM access addresses to
> +	 * be aligned, for example arm64, so since we're about to
> +	 * call readl(), we check here for 4-byte alignment.
> +	 */
> +	if (!IS_ALIGNED((u64)pds, 4))
> +		return false;
> +
> +	/* Before reading length, check range. */
> +	if (check_add_overflow((u64)pds, 0x0B, &end))
> +		return false;
> +
> +	if (pds < rom || end >= rom_end)

Same Q here, wouldn't '=' be a valid case?

> +		return false;
> +
> +	data_len = readw(pds + 0x0A);
> +	if (!data_len || data_len == 0xFFFF ||

U16_MAX?

> +	    check_add_overflow((u64)pds, data_len, &end))
> +		return false;

I would split these two

	data_len = readw(pds + 0x0A);
	if (!data_len || data_len == U16_MAX)
		return false;

	if (check_add_overflow((u64)pds, data_len, &end))
		return false;

> +	if (end >= rom_end)
> +		return false;
> +
> +	if (readl(pds) == 0x52494350)
> +		return true;
> +
> +	pci_info(pdev, "Invalid PCI ROM data signature: expecting 0x52494350, got %#010x\n",
> +		 readl(pds));
> +	return false;
> +}

...

>  	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);

!!() is not needed.

		last_image = readb(pds + 21) & 0x80;

>  		length = readw(pds + 16);
>  		image += length * 512;
> +
> +		if (!pci_rom_header_valid(pdev, image, rom, size, last_image))
>  			break;
>  	} while (length && !last_image);

-- 
With Best Regards,
Andy Shevchenko