linux-kernel - Re: [PATCH v3] PCI: Check rom header and data structure addr before accessing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a004aa05-38a2-4c0b-82ea-3b86b832d82a@linux.alibaba.com>
Date: Wed, 26 Nov 2025 20:12:26 +0800
From: Guixin Liu <kanie@...ux.alibaba.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] PCI: Check rom header and data structure addr before
 accessing



在 2025/11/26 15:49, Andy Shevchenko 写道:
> On Wed, Nov 26, 2025 at 03:26:23PM +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.
> Thanks for the update, looks much better now!
> My comments below.
>
> ...
>
>> +static inline bool pci_rom_header_valid(struct pci_dev *pdev,
>> +					void __iomem *image,
>> +					void __iomem *rom,
>> +					size_t size,
>> +					bool last_image)
>> +{
>> +	uintptr_t rom_end = (uintptr_t)rom + size;
>> +	uintptr_t header_end;
> Note: Linus told that kernel should not use uintptr_t.
>
> s/uintptr_t/unsigned long/g
>
> and here in some cases we even don't need that type at all.
OK, changed in v4.
>> +	if (check_add_overflow((uintptr_t)image, PCI_ROM_HEADER_SIZE,
>> +	    &header_end))
>> +		return false;
>> +	if (image >= rom && header_end < rom_end &&
>> +	    IS_ALIGNED((uintptr_t)image, 2)) {
> So, why not
>
> 	/* Check if we have enough space in ROM */
> 	if (image < rom || header_end > rom_end)
> 		return false;
OK, will be changed in v4.
> 	/* ARM requires proper alignment */
> /// Find a better comment text for above.
> 	if (!IS_ALIGNED((unsigned long)image, 2UL))
> 		return false;
Sure.
>> +		/* 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));
>> +	}
>> +	return false;
>> +}
> ...
>
>> +static inline bool pci_rom_data_struct_valid(struct pci_dev *pdev,
>> +					     void __iomem *pds,
>> +					     void __iomem *rom,
>> +					     size_t size)
> Similar comments as per above.
>
> ...
>
>>   	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;
>>   		length = readw(pds + 16);
>>   		image += length * 512;
>> +		if (!pci_rom_header_valid(pdev, image, rom, size, (bool)last_image))
> This casting is a bit odd. Can we avoid doing like this?
Emm, not think too much, I will change last_iamge to bool in v4.

Best Regards,
Guixin Liu
>>   	} while (length && !last_image);