* [PATCH v12 1/2] PCI: Introduce named defines for PCI ROM
2026-05-08 8:21 [PATCH v12 0/2] PCI: Fix crash when access broken ROM Guixin Liu
@ 2026-05-08 8:21 ` Guixin Liu
2026-05-08 8:21 ` [PATCH v12 2/2] PCI: Check ROM header and data structure addr before accessing Guixin Liu
2026-05-08 22:20 ` [PATCH v12 0/2] PCI: Fix crash when access broken ROM Bjorn Helgaas
2 siblings, 0 replies; 4+ messages in thread
From: Guixin Liu @ 2026-05-08 8:21 UTC (permalink / raw)
To: Bjorn Helgaas, Andy Shevchenko, Ilpo Järvinen,
Krzysztof Wilczyński
Cc: linux-pci, xlpang, oliver.yang
Convert the magic numbers associated with PCI ROM into named
definitions. Some of these definitions will be used in the second
fix patch.
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Reviewed-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
---
drivers/pci/rom.c | 40 ++++++++++++++++++++++++++++------------
1 file changed, 28 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index e18d3a4383ba..d4a141bd148b 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -5,13 +5,28 @@
* (C) Copyright 2004 Jon Smirl <jonsmirl@yahoo.com>
* (C) Copyright 2004 Silicon Graphics, Inc. Jesse Barnes <jbarnes@sgi.com>
*/
+
+#include <linux/bits.h>
#include <linux/kernel.h>
#include <linux/export.h>
#include <linux/pci.h>
+#include <linux/sizes.h>
#include <linux/slab.h>
#include "pci.h"
+#define PCI_ROM_HEADER_SIZE 0x1A
+#define PCI_ROM_POINTER_TO_DATA_STRUCT 0x18
+#define PCI_ROM_LAST_IMAGE_INDICATOR 0x15
+#define PCI_ROM_LAST_IMAGE_INDICATOR_BIT BIT(7)
+#define PCI_ROM_IMAGE_LEN 0x10
+#define PCI_ROM_IMAGE_SECTOR_SIZE SZ_512
+#define PCI_ROM_IMAGE_SIGNATURE 0xAA55
+
+/* Data structure signature is "PCIR" in ASCII representation */
+#define PCI_ROM_DATA_STRUCT_SIGNATURE 0x52494350
+#define PCI_ROM_DATA_STRUCT_LEN 0x0A
+
/**
* pci_enable_rom - enable ROM decoding for a PCI device
* @pdev: PCI device to enable
@@ -91,26 +106,27 @@ static size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *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 (readw(image) != PCI_ROM_IMAGE_SIGNATURE) {
+ pci_info(pdev, "Invalid PCI ROM header signature: expecting %#06x, got %#06x\n",
+ PCI_ROM_IMAGE_SIGNATURE, readw(image));
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));
+ /* Get the PCI data structure and check its "PCIR" signature */
+ pds = image + readw(image + PCI_ROM_POINTER_TO_DATA_STRUCT);
+ if (readl(pds) != PCI_ROM_DATA_STRUCT_SIGNATURE) {
+ pci_info(pdev, "Invalid PCI ROM data signature: expecting %#010x, got %#010x\n",
+ PCI_ROM_DATA_STRUCT_SIGNATURE, readl(pds));
break;
}
- last_image = readb(pds + 21) & 0x80;
- length = readw(pds + 16);
- image += length * 512;
+ last_image = readb(pds + PCI_ROM_LAST_IMAGE_INDICATOR) &
+ PCI_ROM_LAST_IMAGE_INDICATOR_BIT;
+ length = readw(pds + PCI_ROM_IMAGE_LEN);
+ image += length * PCI_ROM_IMAGE_SECTOR_SIZE;
/* Avoid iterating through memory outside the resource window */
if (image >= rom + size)
break;
if (!last_image) {
- if (readw(image) != 0xAA55) {
+ if (readw(image) != PCI_ROM_IMAGE_SIGNATURE) {
pci_info(pdev, "No more image in the PCI ROM\n");
break;
}
--
2.43.7
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v12 2/2] PCI: Check ROM header and data structure addr before accessing
2026-05-08 8:21 [PATCH v12 0/2] PCI: Fix crash when access broken ROM Guixin Liu
2026-05-08 8:21 ` [PATCH v12 1/2] PCI: Introduce named defines for PCI ROM Guixin Liu
@ 2026-05-08 8:21 ` Guixin Liu
2026-05-08 22:20 ` [PATCH v12 0/2] PCI: Fix crash when access broken ROM Bjorn Helgaas
2 siblings, 0 replies; 4+ messages in thread
From: Guixin Liu @ 2026-05-08 8:21 UTC (permalink / raw)
To: Bjorn Helgaas, Andy Shevchenko, Ilpo Järvinen,
Krzysztof Wilczyński
Cc: linux-pci, xlpang, oliver.yang
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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
---
drivers/pci/rom.c | 128 +++++++++++++++++++++++++++++++++++++++-------
1 file changed, 110 insertions(+), 18 deletions(-)
diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index d4a141bd148b..9744f1a2923a 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -6,9 +6,12 @@
* (C) Copyright 2004 Silicon Graphics, Inc. Jesse Barnes <jbarnes@sgi.com>
*/
+#include <linux/align.h>
#include <linux/bits.h>
#include <linux/kernel.h>
#include <linux/export.h>
+#include <linux/io.h>
+#include <linux/overflow.h>
#include <linux/pci.h>
#include <linux/sizes.h>
#include <linux/slab.h>
@@ -27,6 +30,16 @@
#define PCI_ROM_DATA_STRUCT_SIGNATURE 0x52494350
#define PCI_ROM_DATA_STRUCT_LEN 0x0A
+/*
+ * Per PCI Local Bus Spec 2.x / PCI Firmware Spec r3.3 sec 5.1.3
+ * Table 5-2, a conformant PCI Data Structure is at least 24 bytes
+ * (0x18), large enough to cover every fixed field this driver
+ * reads (up to the Indicator byte at offset 0x15). Reject smaller
+ * device-claimed lengths so the follow-up readers in
+ * pci_get_rom_size() cannot escape the mapped ROM window.
+ */
+#define PCI_ROM_DATA_STRUCT_MIN_LEN 0x18
+
/**
* pci_enable_rom - enable ROM decoding for a PCI device
* @pdev: PCI device to enable
@@ -84,6 +97,95 @@ void pci_disable_rom(struct pci_dev *pdev)
}
EXPORT_SYMBOL_GPL(pci_disable_rom);
+static bool pci_rom_is_header_valid(struct pci_dev *pdev,
+ void __iomem *image,
+ void __iomem *rom,
+ size_t size,
+ bool expect_valid)
+{
+ unsigned long rom_end = (unsigned long)rom + size - 1;
+ unsigned long header_end;
+ u16 signature;
+
+ /*
+ * Per PCI Firmware Specification r3.3, sec 5.1, each image must
+ * start on a 512-byte boundary and must contain the PCI Expansion
+ * ROM header. Because @rom is page-aligned (returned by ioremap()),
+ * checking 512-byte alignment of @image is equivalent to enforcing
+ * the spec's sector-aligned layout within the ROM. This also
+ * satisfies the natural-alignment requirement of readw() on archs
+ * such as arm64 that disallow unaligned IOMEM access.
+ */
+ if (!IS_ALIGNED((unsigned long)image, PCI_ROM_IMAGE_SECTOR_SIZE))
+ return false;
+
+ if (check_add_overflow((unsigned long)image, PCI_ROM_HEADER_SIZE - 1,
+ &header_end))
+ return false;
+
+ if (image < rom || header_end > rom_end)
+ return false;
+
+ /* Standard PCI ROMs start out with these bytes 55 AA */
+ signature = readw(image);
+ if (signature != PCI_ROM_IMAGE_SIGNATURE) {
+ if (expect_valid) {
+ pci_info(pdev, "Invalid PCI ROM header signature: expecting %#06x, got %#06x\n",
+ PCI_ROM_IMAGE_SIGNATURE, signature);
+ } else {
+ pci_info(pdev, "No more image in the PCI ROM\n");
+ }
+ return false;
+ }
+
+ return true;
+}
+
+static bool pci_rom_is_data_struct_valid(struct pci_dev *pdev,
+ void __iomem *pds,
+ void __iomem *rom,
+ size_t size)
+{
+ unsigned long rom_end = (unsigned long)rom + size - 1;
+ unsigned long end;
+ u32 signature;
+ u16 data_len;
+
+ /*
+ * Some CPU architectures require IOMEM access addresses to
+ * be aligned, for example arm64, so since we're about to
+ * call readl(), we check here for 4-byte alignment.
+ */
+ if (!IS_ALIGNED((unsigned long)pds, 4))
+ return false;
+
+ if (check_add_overflow((unsigned long)pds, PCI_ROM_DATA_STRUCT_LEN + 1,
+ &end))
+ return false;
+
+ if (pds < rom || end > rom_end)
+ return false;
+
+ signature = readl(pds);
+ if (signature != PCI_ROM_DATA_STRUCT_SIGNATURE) {
+ pci_info(pdev, "Invalid PCI ROM data signature: expecting %#010x, got %#010x\n",
+ PCI_ROM_DATA_STRUCT_SIGNATURE, signature);
+ return false;
+ }
+
+ data_len = readw(pds + PCI_ROM_DATA_STRUCT_LEN);
+ if (data_len < PCI_ROM_DATA_STRUCT_MIN_LEN || data_len == U16_MAX)
+ return false;
+
+ if (check_add_overflow((unsigned long)pds, data_len - 1, &end))
+ return false;
+
+ if (end > rom_end)
+ return false;
+
+ return true;
+}
+
/**
* pci_get_rom_size - obtain the actual size of the ROM image
* @pdev: target PCI device
@@ -99,38 +201,28 @@ static size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom,
size_t size)
{
void __iomem *image;
- int last_image;
unsigned int length;
+ bool last_image;
image = rom;
do {
void __iomem *pds;
- /* Standard PCI ROMs start out with these bytes 55 AA */
- if (readw(image) != PCI_ROM_IMAGE_SIGNATURE) {
- pci_info(pdev, "Invalid PCI ROM header signature: expecting %#06x, got %#06x\n",
- PCI_ROM_IMAGE_SIGNATURE, readw(image));
+ if (!pci_rom_is_header_valid(pdev, image, rom, size, true))
break;
- }
+
/* Get the PCI data structure and check its "PCIR" signature */
pds = image + readw(image + PCI_ROM_POINTER_TO_DATA_STRUCT);
- if (readl(pds) != PCI_ROM_DATA_STRUCT_SIGNATURE) {
- pci_info(pdev, "Invalid PCI ROM data signature: expecting %#010x, got %#010x\n",
- PCI_ROM_DATA_STRUCT_SIGNATURE, readl(pds));
+ if (!pci_rom_is_data_struct_valid(pdev, pds, rom, size))
break;
- }
+
last_image = readb(pds + PCI_ROM_LAST_IMAGE_INDICATOR) &
PCI_ROM_LAST_IMAGE_INDICATOR_BIT;
length = readw(pds + PCI_ROM_IMAGE_LEN);
image += length * PCI_ROM_IMAGE_SECTOR_SIZE;
- /* Avoid iterating through memory outside the resource window */
- if (image >= rom + size)
+
+ if (!last_image &&
+ !pci_rom_is_header_valid(pdev, image, rom, size, false))
break;
- if (!last_image) {
- if (readw(image) != PCI_ROM_IMAGE_SIGNATURE) {
- 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.7
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v12 0/2] PCI: Fix crash when access broken ROM
2026-05-08 8:21 [PATCH v12 0/2] PCI: Fix crash when access broken ROM Guixin Liu
2026-05-08 8:21 ` [PATCH v12 1/2] PCI: Introduce named defines for PCI ROM Guixin Liu
2026-05-08 8:21 ` [PATCH v12 2/2] PCI: Check ROM header and data structure addr before accessing Guixin Liu
@ 2026-05-08 22:20 ` Bjorn Helgaas
2 siblings, 0 replies; 4+ messages in thread
From: Bjorn Helgaas @ 2026-05-08 22:20 UTC (permalink / raw)
To: Guixin Liu
Cc: Bjorn Helgaas, Andy Shevchenko, Ilpo Järvinen,
Krzysztof Wilczyński, linux-pci, xlpang, oliver.yang
On Fri, May 08, 2026 at 04:21:26PM +0800, Guixin Liu wrote:
> v11 -> v12:
> - Add rb tag from Krzysztof Wilczyński in the first patch, thanks.
> - Change "get" to "Get".
> - Renamed parameter last_image → expect_valid in
> pci_rom_is_header_valid() to better reflect its semantics: it
> indicates whether the caller expects the image to be valid
> (and thus whether a missing/invalid signature should be reported
> as an error or as normal end-of-chain).
> - Tightened image alignment check: replaced the 2-byte alignment check
> with a 512-byte (PCI_ROM_IMAGE_SECTOR_SIZE) alignment check on image,
> per PCI Firmware Specification r3.3, sec 5.1, which mandates that each
> ROM image starts on a 512-byte boundary. This also satisfies the
> natural-alignment requirement of readw() on architectures such as arm64.
> - Updated comment to cite the PCI Firmware Spec r3.3 sec 5.1 as the
> authoritative source for the alignment requirement, and to explain the
> relationship between page-aligned rom, sector-aligned image,
> and the IOMEM access constraint.
> - Fixed off-by-one in overflow checks: check_add_overflow() now uses
> PCI_ROM_HEADER_SIZE - 1 and data_len - 1 so that header_end / end
> represent the inclusive last byte of the region, matching the
> subsequent > rom_end comparison.
> - Refactored signature-check log flow: collapsed the dual-return branches
> into a single if (signature != PCI_ROM_IMAGE_SIGNATURE) block, emitting
> the appropriate pci_info() based on expect_valid, then returning false;
> success path returns true at the end.
> - Reorder pci_rom_is_data_struct_valid() to check the "PCIR" signature
> before reading data_len, so bad signatures are still logged.
> - Collapse the signature branch to early-return on failure,
> matching the style of pci_rom_is_header_valid().
> - Add PCI_ROM_DATA_STRUCT_MIN_LEN (0x18), the PCI 2.x baseline PCI Data
> Structure length.
> - Reject data_len < PCI_ROM_DATA_STRUCT_MIN_LEN to keep the fixed-offset
> reads (PCI_ROM_IMAGE_LEN @0x10, PCI_ROM_LAST_IMAGE_INDICATOR @0x15)
> in pci_get_rom_size() inside the mapped ROM window.
> - Cite PCI Firmware Spec r3.3 sec 5.1.3 Table 5-2 in the new macro's
> comment.
>
> v10 -> v11:
> - Change 'pci rom' to 'PCI ROM' of the tittle of the first patch.
> - Add Andy Shevchenko's rb tag in the first patch, thanks.
>
> v9 -> v10:
> - Reorder the header files, and not touch kernel.h
> - Change PCI_ROM_IMAGE_LEN_UNIT_BYTES to PCI_ROM_IMAGE_SECTOR_SIZE.
> - Add a comment for PCI_ROM_DATA_STRUCT_SIGNATURE.
>
> v8 -> v9:
> - Supplemental explanation for the commit body of the first patch.
> - Change PCI_ROM_IMAGE_LEN_UNIT_SZ_512 to PCI_ROM_IMAGE_LEN_UNIT_BYTES,
> and change it's definition to SZ_512.
> - Use u16 and u32 for signature val instead of unsigned short/int.
>
> v7 -> v8:
> - Ordered header files alphabetically.
> - Convert the literals too in the firt patch.
> - Use local val to save signature instead of reading twice.
>
> v6 -> v7:
> - Put all named defines to a separate patch.
> - Change PCI_ROM_IMAGE_LEN_UNIT_BYTES to PCI_ROM_IMAGE_LEN_UNIT_SZ_512.
> - Named BIT(7) to PCI_ROM_LAST_IMAGE_INDICATOR_BIT.
> - Fix all other comments from Ilpo, such as including header files,
> and alignment fault, Thanks.
>
> v5 -> v6:
> - Convert some magic number to named defines, suggested by
> Ilpo, thanks.
>
> v4 -> v5:
> - Add Andy Shevchenko's rb tag, thanks.
> - Change u64 to unsigned long.
> - Change pci_rom_header_valid() to pci_rom_is_header_valid() and
> change pci_rom_data_struct_valid() to pci_rom_is_data_struct_valid().
> - Change rom_end from rom+size to rom+size-1 for more readble,
> and also change header_end >= rom_end to header_end > rom_end, same
> as data structure end.
> - Change if(!last_image) to if (last_image)..
> - Use U16_MAX instead of 0xffff.
> - Split check_add_overflow() from data_len checking.
> - Remove !!() when reading last_image, and Use BIT(7) instead of 0x80.
>
> v3 -> v4:
> - Use "u64" instead of "uintptr_t".
> - Invert the if statement to avoid excessive indentation.
> - Add comment for alignment checking.
> - Change last_image's type from int to bool.
>
> 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.
>
> Guixin Liu (2):
> PCI: Introduce named defines for PCI ROM
> PCI: Check ROM header and data structure addr before accessing
>
> drivers/pci/rom.c | 154 +++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 131 insertions(+), 23 deletions(-)
Applied to pci/rom for v7.2, thanks for all your work on this!
^ permalink raw reply [flat|nested] 4+ messages in thread