drivers/pci/rom.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
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
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.
Fixes: 47b975d234ea ("PCI: Avoid iterating through memory outside the resource window")
Suggested-by: Guanghui Feng <guanghuifeng@linux.alibaba.com>
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
v1 -> v2:
- Fix commit body problems, such as blank line in "Call Trace" both sides,
thanks, (Andy Shevchenko).
- Remove every step checking, just check the addr is in header or data struct.
- Add Suggested-by: Guanghui Feng <guanghuifeng@linux.alibaba.com> tag.
drivers/pci/rom.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index e18d3a4383ba..16d601e7ffcc 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -69,6 +69,9 @@ void pci_disable_rom(struct pci_dev *pdev)
}
EXPORT_SYMBOL_GPL(pci_disable_rom);
+#define PCI_ROM_HEADER_SIZE 0x1A
+#define PCI_ROM_DATA_STRUCT_SIZE 0x1B
+
/**
* pci_get_rom_size - obtain the actual size of the ROM image
* @pdev: target PCI device
@@ -83,6 +86,7 @@ EXPORT_SYMBOL_GPL(pci_disable_rom);
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;
@@ -90,14 +94,21 @@ static size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom,
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));
@@ -106,8 +117,9 @@ static size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom,
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)
break;
if (!last_image) {
if (readw(image) != 0xAA55) {
--
2.43.0On 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?
> 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.
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 (...);
> break;
> if (!last_image) {
> if (readw(image) != 0xAA55) {
--
With Best Regards,
Andy Shevchenko
在 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) {
© 2016 - 2025 Red Hat, Inc.