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

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

在 2025/11/18 23:47, Andy Shevchenko 写道:
> 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.
Will be changed in v2, thanks.
>> 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.
>
> ...
In my case, the rom start addr is 0xffa0000007f30000, the size is 
0x10000, we got a data structure addr 0xffa0000007f3ffff which point to 
the end of rom space, readl() will read 4 bytes therefore cause an 
out-of-bounds access.
>
>> +#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!
>
In v2, I will change to checking addr is in the range of the header
or data structure per spec.

Best Regards,
Guixin Liu