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

[Guixin Liu <kanie@linux.alibaba.com>]

在 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) {