drivers/pci/rom.c | 86 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 69 insertions(+), 17 deletions(-)
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.
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>
---
v2 -> v3:
- Add pci_rom_header_valid() helper for checking image addr and signature.
- Add pci_rom_data_struct_valid() helper for checking data struct add
and signature.
- Handle overflow issue when adding addr with size.
- Handle alignment fault when running on arm64.
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 | 86 +++++++++++++++++++++++++++++++++++++----------
1 file changed, 69 insertions(+), 17 deletions(-)
diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index e18d3a4383ba..8b4221d9fc17 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -69,6 +69,68 @@ void pci_disable_rom(struct pci_dev *pdev)
}
EXPORT_SYMBOL_GPL(pci_disable_rom);
+#define PCI_ROM_HEADER_SIZE 0x1A
+
+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;
+
+ 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)) {
+ /* 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)
+{
+ uintptr_t rom_end = (uintptr_t)rom + size;
+ uintptr_t end;
+ u16 data_len;
+
+ if (!IS_ALIGNED((uintptr_t)pds, 4))
+ return false;
+
+ /* Before reading length, check range. */
+ if (check_add_overflow((uintptr_t)pds, 0x0B, &end))
+ return false;
+
+ if (pds > rom && end < rom_end) {
+ data_len = readw(pds + 0x0A);
+ if (!data_len || data_len == 0xFFFF ||
+ check_add_overflow((uintptr_t)pds, data_len, &end))
+ return false;
+
+ if (end < rom_end) {
+ if (readl(pds) == 0x52494350)
+ return true;
+ pci_info(pdev, "Invalid PCI ROM data signature: expecting 0x52494350, got %#010x\n",
+ readl(pds));
+ }
+ }
+ return false;
+}
+
/**
* pci_get_rom_size - obtain the actual size of the ROM image
* @pdev: target PCI device
@@ -90,31 +152,21 @@ static size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom,
image = rom;
do {
void __iomem *pds;
- /* 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));
+
+ 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 (readl(pds) != 0x52494350) {
- pci_info(pdev, "Invalid PCI ROM data signature: expecting 0x52494350, got %#010x\n",
- readl(pds));
+ if (!pci_rom_data_struct_valid(pdev, pds, rom, size))
break;
- }
+
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 (!pci_rom_header_valid(pdev, image, rom, size, (bool)last_image))
break;
- if (!last_image) {
- if (readw(image) != 0xAA55) {
- pci_info(pdev, "No more image in the PCI ROM\n");
- break;
- }
- }
} while (length && !last_image);
/* never return a size larger than the PCI resource window */
--
2.43.0On 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.
> + 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;
/* ARM requires proper alignment */
/// Find a better comment text for above.
if (!IS_ALIGNED((unsigned long)image, 2UL))
return false;
> + /* 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?
> } while (length && !last_image);
--
With Best Regards,
Andy Shevchenko
在 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);
© 2016 - 2025 Red Hat, Inc.