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

[Guixin Liu]

在 2025/11/19 18:49, Andy Shevchenko 写道:
> 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?
oh, 4 lines, of coures, will be updated in v3.
> > 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.
Yes, this can overflow, will be changed in v3.
> 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 (...);
Sure, I will modify the code to fix the overflow issue.

Best Regards,
Guixin Liu
> >   			break;
> >   		if (!last_image) {
> >   			if (readw(image) != 0xAA55) {