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

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

[Guixin Liu]

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
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.

Fixes: 47b975d234ea ("PCI: Avoid iterating through memory outside the resource window")
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/pci/rom.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index e18d3a4383ba..f9377ad3f89f 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -69,6 +69,10 @@ void pci_disable_rom(struct pci_dev *pdev)
 }
 EXPORT_SYMBOL_GPL(pci_disable_rom);
 
+#define PCI_ROM_DATA_STRUCT_OFFSET 24
+#define PCI_ROM_LAST_IMAGE_OFFSET 21
+#define PCI_ROM_LAST_IMAGE_LEN_OFFSET 16
+
 /**
  * pci_get_rom_size - obtain the actual size of the ROM image
  * @pdev: target PCI device
@@ -86,28 +90,41 @@ 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;
 
 	image = rom;
 	do {
 		void __iomem *pds;
+
+		if (image + 2 >= 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;
 		}
+
+		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));
 			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) {
-- 
2.43.0

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

[Guixin Liu]

Gentle ping...

BR
Guixin Liu

在 2025/11/14 14:34, Guixin Liu 写道:
> 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
> 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.
>
> Fixes: 47b975d234ea ("PCI: Avoid iterating through memory outside the resource window")
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
>   drivers/pci/rom.c | 25 +++++++++++++++++++++----
>   1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
> index e18d3a4383ba..f9377ad3f89f 100644
> --- a/drivers/pci/rom.c
> +++ b/drivers/pci/rom.c
> @@ -69,6 +69,10 @@ void pci_disable_rom(struct pci_dev *pdev)
>   }
>   EXPORT_SYMBOL_GPL(pci_disable_rom);
>   
> +#define PCI_ROM_DATA_STRUCT_OFFSET 24
> +#define PCI_ROM_LAST_IMAGE_OFFSET 21
> +#define PCI_ROM_LAST_IMAGE_LEN_OFFSET 16
> +
>   /**
>    * pci_get_rom_size - obtain the actual size of the ROM image
>    * @pdev: target PCI device
> @@ -86,28 +90,41 @@ 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;
>   
>   	image = rom;
>   	do {
>   		void __iomem *pds;
> +
> +		if (image + 2 >= 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;
>   		}
> +
> +		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));
>   			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) {

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

[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.

> 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.

...

> +#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!


-- 
With Best Regards,
Andy Shevchenko

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

[Guixin Liu]

在 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