[PATCH v11 0/2] PCI: Fix crash when access broken ROM

[PATCH v11 0/2] PCI: Fix crash when access broken ROM

[Guixin Liu]

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 | 137 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 115 insertions(+), 22 deletions(-)

-- 
2.32.0.3.g01195cf9f

[PATCH v11 1/2] PCI: Introduce named defines for PCI ROM

[Guixin Liu]

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>
---
 drivers/pci/rom.c | 38 +++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index e18d3a4383ba..4f7641b93b4b 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));
+		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.32.0.3.g01195cf9f

[PATCH v11 2/2] PCI: Check ROM header and data structure addr before accessing

[Guixin Liu]

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 | 113 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 95 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index 4f7641b93b4b..d8abed669fac 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>
@@ -84,6 +87,91 @@ 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 last_image)
+{
+	unsigned long rom_end = (unsigned long)rom + size - 1;
+	unsigned long header_end;
+	u16 signature;
+
+	/*
+	 * Some CPU architectures require IOMEM access addresses to
+	 * be aligned, for example arm64, so since we're about to
+	 * call readw(), we check here for 2-byte alignment.
+	 */
+	if (!IS_ALIGNED((unsigned long)image, 2))
+		return false;
+
+	if (check_add_overflow((unsigned long)image, PCI_ROM_HEADER_SIZE,
+				&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)
+		return true;
+
+	if (last_image) {
+		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;
+}
+
+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;
+
+	/* Before reading length, check addr range. */
+	if (check_add_overflow((unsigned long)pds, PCI_ROM_DATA_STRUCT_LEN + 1,
+				&end))
+		return false;
+
+	if (pds < rom || end > rom_end)
+		return false;
+
+	data_len = readw(pds + PCI_ROM_DATA_STRUCT_LEN);
+	if (!data_len || data_len == U16_MAX)
+		return false;
+
+	if (check_add_overflow((unsigned long)pds, data_len, &end))
+		return false;
+
+	if (end > rom_end)
+		return false;
+
+	signature = readl(pds);
+	if (signature == PCI_ROM_DATA_STRUCT_SIGNATURE)
+		return true;
+
+	pci_info(pdev, "Invalid PCI ROM data signature: expecting %#010x, got %#010x\n",
+		 PCI_ROM_DATA_STRUCT_SIGNATURE, signature);
+	return false;
+}
+
 /**
  * pci_get_rom_size - obtain the actual size of the ROM image
  * @pdev: target PCI device
@@ -99,38 +187,27 @@ 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 (!pci_rom_is_header_valid(pdev, image, rom, size, last_image))
 			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.32.0.3.g01195cf9f

Re: [PATCH v11 0/2] PCI: Fix crash when access broken ROM

[Guixin Liu]

Gentling ping...

Hi Bjorn and Ilpo, Could you please review these two patches?

In this version, I fixed the case issues in the first patch title.

Best Regards,
Guixin Liu

在 2026/1/30 16:07, Guixin Liu 写道:
> 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 | 137 ++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 115 insertions(+), 22 deletions(-)
> 

Re: [PATCH v11 0/2] PCI: Fix crash when access broken ROM

[Bjorn Helgaas]

On Mon, Feb 09, 2026 at 02:43:39PM +0800, Guixin Liu wrote:
> Gentling ping...
> 
> Hi Bjorn and Ilpo, Could you please review these two patches?
> 
> In this version, I fixed the case issues in the first patch title.

Will do, thanks.  Since the v7.0 merge window is open, this is on my
list to look at as soon as v7.0-rc1 is tagged, probably on Feb 22.

> 在 2026/1/30 16:07, Guixin Liu 写道:
> > 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 | 137 ++++++++++++++++++++++++++++++++++++++--------
> >   1 file changed, 115 insertions(+), 22 deletions(-)
> > 
> 

Re: [PATCH v11 0/2] PCI: Fix crash when access broken ROM

[Guixin Liu]

在 2026/2/10 01:54, Bjorn Helgaas 写道:
> On Mon, Feb 09, 2026 at 02:43:39PM +0800, Guixin Liu wrote:
>> Gentling ping...
>>
>> Hi Bjorn and Ilpo, Could you please review these two patches?
>>
>> In this version, I fixed the case issues in the first patch title.
> Will do, thanks.  Since the v7.0 merge window is open, this is on my
> list to look at as soon as v7.0-rc1 is tagged, probably on Feb 22.
Hi Bjorn, looks like these two patches still haven't been merged, Would 
it be possible to get them merged for the 7.0 release? Best Regards, 
Guixin Liu
>
>> 在 2026/1/30 16:07, Guixin Liu 写道:
>>> 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 | 137 ++++++++++++++++++++++++++++++++++++++--------
>>>    1 file changed, 115 insertions(+), 22 deletions(-)
>>>

Re: [PATCH v11 0/2] PCI: Fix crash when access broken ROM

[Andy Shevchenko]

On Fri, Apr 24, 2026 at 02:32:06PM +0800, Guixin Liu wrote:
> 在 2026/2/10 01:54, Bjorn Helgaas 写道:
> > On Mon, Feb 09, 2026 at 02:43:39PM +0800, Guixin Liu wrote:

...

> > > In this version, I fixed the case issues in the first patch title.
> > Will do, thanks.  Since the v7.0 merge window is open, this is on my
> > list to look at as soon as v7.0-rc1 is tagged, probably on Feb 22.
> Hi Bjorn, looks like these two patches still haven't been merged, Would it
> be possible to get them merged for the 7.0 release?

v7.0 has already been released. Now it's a v7.1-rc1 candidate at best.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 0/2] PCI: Fix crash when access broken ROM

[Guixin Liu]

Hi Bjorn, gentling ping...


Best Regards,
Guixin Liu


在 2026/4/24 14:32, Guixin Liu 写道:
>
>
> 在 2026/2/10 01:54, Bjorn Helgaas 写道:
>> On Mon, Feb 09, 2026 at 02:43:39PM +0800, Guixin Liu wrote:
>>> Gentling ping...
>>>
>>> Hi Bjorn and Ilpo, Could you please review these two patches?
>>>
>>> In this version, I fixed the case issues in the first patch title.
>> Will do, thanks.  Since the v7.0 merge window is open, this is on my
>> list to look at as soon as v7.0-rc1 is tagged, probably on Feb 22.
> Hi Bjorn, looks like these two patches still haven't been merged, 
> Would it be possible to get them merged for the 7.0 release? Best 
> Regards, Guixin Liu
>>
>>> 在 2026/1/30 16:07, Guixin Liu 写道:
>>>> 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 | 137 
>>>> ++++++++++++++++++++++++++++++++++++++--------
>>>>    1 file changed, 115 insertions(+), 22 deletions(-)
>>>>
>

Re: [PATCH v11 2/2] PCI: Check ROM header and data structure addr before accessing

[Bjorn Helgaas]

On Fri, Jan 30, 2026 at 04:07:29PM +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.
> 
> 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 | 113 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 95 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
> index 4f7641b93b4b..d8abed669fac 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>
> @@ -84,6 +87,91 @@ 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 last_image)
> +{
> +	unsigned long rom_end = (unsigned long)rom + size - 1;
> +	unsigned long header_end;
> +	u16 signature;
> +
> +	/*
> +	 * Some CPU architectures require IOMEM access addresses to
> +	 * be aligned, for example arm64, so since we're about to
> +	 * call readw(), we check here for 2-byte alignment.
> +	 */

I think PCI Firmware r3.3, sec 5.1, actually requires 512-byte
alignment, but I guess we haven't enforced that before.  Worth
mentioning the spec requirement to show that this isn't just an
arbitrary thing to accommodate a weird CPU architecture.

> +	if (!IS_ALIGNED((unsigned long)image, 2))
> +		return false;
> +
> +	if (check_add_overflow((unsigned long)image, PCI_ROM_HEADER_SIZE,
> +				&header_end))
> +		return false;
> +
> +	if (image < rom || header_end > rom_end)
> +		return false;

From Sashiko:

  Does this correctly handle a ROM structure that fits exactly at the
  end of the window?

  Since header_end is calculated exclusively and rom_end inclusively,
  a perfectly sized structure will have header_end equal to rom_end +
  1, causing the header_end > rom_end check to incorrectly evaluate to
  true and reject the ROM. This same inclusive versus exclusive
  boundary mismatch also happens in pci_rom_is_data_struct_valid()
  when checking the end pointer.

> +
> +	/* Standard PCI ROMs start out with these bytes 55 AA */
> +	signature = readw(image);
> +	if (signature == PCI_ROM_IMAGE_SIGNATURE)
> +		return true;

I think this and pci_rom_is_data_struct_valid() would read better if
every test was a check for failure instead of having a bunch of
failure returns, followed by a success return, followed by another
failure return.  E.g.,

  if (signature != PCI_ROM_IMAGE_SIGNATURE) {
    if (last_image) {
      ...
    }
    return false;
  }

  return true;

> +	if (last_image) {
> +		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");
> +	}

I'm not completely convinced that it's worth passing in last_image.  I
suppose the reason was to make the messages exactly the same as
before?

Even in the "!last_image" case, I think it might be worth printing the
signature we got.  The "No more image" message means that the ROM
format isn't strictly conforming, doesn't it?  Maybe the same message
would suffice for both "last_image" and "!last_image"?

> +	return false;
> +}
> +
> +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;
> +
> +	/* Before reading length, check addr range. */
> +	if (check_add_overflow((unsigned long)pds, PCI_ROM_DATA_STRUCT_LEN + 1,
> +				&end))
> +		return false;
> +
> +	if (pds < rom || end > rom_end)
> +		return false;
> +
> +	data_len = readw(pds + PCI_ROM_DATA_STRUCT_LEN);
> +	if (!data_len || data_len == U16_MAX)
> +		return false;
> +
> +	if (check_add_overflow((unsigned long)pds, data_len, &end))
> +		return false;
> +
> +	if (end > rom_end)
> +		return false;

More from Sashiko:

  Does pci_rom_is_data_struct_valid() need to enforce a minimum safe
  size for data_len?

  If a malformed device advertises a small data_len (e.g., 12 bytes),
  validation passes here, but the subsequent reads in
  pci_get_rom_size() for PCI_ROM_IMAGE_LEN and
  PCI_ROM_LAST_IMAGE_INDICATOR could access unmapped memory past the
  ROM boundary.

> +	signature = readl(pds);
> +	if (signature == PCI_ROM_DATA_STRUCT_SIGNATURE)
> +		return true;

Seems like it would be nicer to check the signature first, before
checking the data_len.  If the signature is bad, we log a hint about
what went wrong, but we don't log anything if data_len is bad.

> +	pci_info(pdev, "Invalid PCI ROM data signature: expecting %#010x, got %#010x\n",
> +		 PCI_ROM_DATA_STRUCT_SIGNATURE, signature);
> +	return false;
> +}
> +
>  /**
>   * pci_get_rom_size - obtain the actual size of the ROM image
>   * @pdev: target PCI device
> @@ -99,38 +187,27 @@ 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 (!pci_rom_is_header_valid(pdev, image, rom, size, last_image))
>  			break;

More from Sashiko.  I'm not sure about this one.

  Does this log a false-positive warning when processing the final
  image?

  When last_image is true, the image pointer is advanced to the end of
  the ROM and passed into pci_rom_is_header_valid().

  Because last_image is passed as true to the helper, the signature
  check will fail and log an invalid header signature error for a
  perfectly valid device instead of gracefully finishing the loop.

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

Re: [PATCH v11 1/2] PCI: Introduce named defines for PCI ROM

[Krzysztof Wilczyński]

Hello,

> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
>  		/* Standard PCI ROMs start out with these bytes 55 AA */

Just a small nitpick: would it make sense to move this comment above to
where the definition is now?  Like the one for "PCIR" signature.

>  		/* get the PCI data structure and check its "PCIR" signature */

I realise this has been here before, but it would be nice to capitalise
this "get" in the comment above.  However, this is also a tiny nitpick,
so feel free to ignore.

But otherwise:

  Reviewed-by: Krzysztof Wilczyński <kwilczynski@kernel.org>

Thank you!

	Krzysztof

Re: [PATCH v11 2/2] PCI: Check ROM header and data structure addr before accessing

[Guixin Liu]

在 2026/5/1 05:46, Bjorn Helgaas 写道:
> On Fri, Jan 30, 2026 at 04:07:29PM +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.
>>
>> 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 | 113 ++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 95 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
>> index 4f7641b93b4b..d8abed669fac 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>
>> @@ -84,6 +87,91 @@ 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 last_image)
>> +{
>> +	unsigned long rom_end = (unsigned long)rom + size - 1;
>> +	unsigned long header_end;
>> +	u16 signature;
>> +
>> +	/*
>> +	 * Some CPU architectures require IOMEM access addresses to
>> +	 * be aligned, for example arm64, so since we're about to
>> +	 * call readw(), we check here for 2-byte alignment.
>> +	 */
> I think PCI Firmware r3.3, sec 5.1, actually requires 512-byte
> alignment, but I guess we haven't enforced that before.  Worth
> mentioning the spec requirement to show that this isn't just an
> arbitrary thing to accommodate a weird CPU architecture.
Yes, it says "Each image must start on a 512-byte boundary and must 
contain the PCI Expansion ROM header".

OK, I'll add a 512-byte alignment check in the next version, along
with a comment citing the spec requirement.
>> +	if (!IS_ALIGNED((unsigned long)image, 2))
>> +		return false;
>> +
>> +	if (check_add_overflow((unsigned long)image, PCI_ROM_HEADER_SIZE,
>> +				&header_end))
>> +		return false;
>> +
>> +	if (image < rom || header_end > rom_end)
>> +		return false;
>  From Sashiko:
>
>    Does this correctly handle a ROM structure that fits exactly at the
>    end of the window?
>
>    Since header_end is calculated exclusively and rom_end inclusively,
>    a perfectly sized structure will have header_end equal to rom_end +
>    1, causing the header_end > rom_end check to incorrectly evaluate to
>    true and reject the ROM. This same inclusive versus exclusive
>    boundary mismatch also happens in pci_rom_is_data_struct_valid()
>    when checking the end pointer.
That's right, chagend in v12, thanks.
>> +
>> +	/* Standard PCI ROMs start out with these bytes 55 AA */
>> +	signature = readw(image);
>> +	if (signature == PCI_ROM_IMAGE_SIGNATURE)
>> +		return true;
> I think this and pci_rom_is_data_struct_valid() would read better if
> every test was a check for failure instead of having a bunch of
> failure returns, followed by a success return, followed by another
> failure return.  E.g.,
>
>    if (signature != PCI_ROM_IMAGE_SIGNATURE) {
>      if (last_image) {
>        ...
>      }
>      return false;
>    }
>
>    return true;
I think so, changed in v12, thanks.
>> +	if (last_image) {
>> +		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");
>> +	}
> I'm not completely convinced that it's worth passing in last_image.  I
> suppose the reason was to make the messages exactly the same as
> before?
>
> Even in the "!last_image" case, I think it might be worth printing the
> signature we got.  The "No more image" message means that the ROM
> format isn't strictly conforming, doesn't it?  Maybe the same message
> would suffice for both "last_image" and "!last_image"?
Commit beced88e6af43 ("PCI: Add check code for last image indicator not 
set")
added this printing to avoid print "Invalid image",
I thinks it's worth keeping as-is.
>> +	return false;
>> +}
>> +
>> +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;
>> +
>> +	/* Before reading length, check addr range. */
>> +	if (check_add_overflow((unsigned long)pds, PCI_ROM_DATA_STRUCT_LEN + 1,
>> +				&end))
>> +		return false;
>> +
>> +	if (pds < rom || end > rom_end)
>> +		return false;
>> +
>> +	data_len = readw(pds + PCI_ROM_DATA_STRUCT_LEN);
>> +	if (!data_len || data_len == U16_MAX)
>> +		return false;
>> +
>> +	if (check_add_overflow((unsigned long)pds, data_len, &end))
>> +		return false;
>> +
>> +	if (end > rom_end)
>> +		return false;
> More from Sashiko:
>
>    Does pci_rom_is_data_struct_valid() need to enforce a minimum safe
>    size for data_len?
>
>    If a malformed device advertises a small data_len (e.g., 12 bytes),
>    validation passes here, but the subsequent reads in
>    pci_get_rom_size() for PCI_ROM_IMAGE_LEN and
>    PCI_ROM_LAST_IMAGE_INDICATOR could access unmapped memory past the
>    ROM boundary.
OK, what would be a suitable min value here? 
PCI_ROM_LAST_IMAGE_INDICATOR? I think this value needs to work for both 
new and legacy devices.
>
>> +	signature = readl(pds);
>> +	if (signature == PCI_ROM_DATA_STRUCT_SIGNATURE)
>> +		return true;
> Seems like it would be nicer to check the signature first, before
> checking the data_len.  If the signature is bad, we log a hint about
> what went wrong, but we don't log anything if data_len is bad.
OK, changed in v12.
>> +	pci_info(pdev, "Invalid PCI ROM data signature: expecting %#010x, got %#010x\n",
>> +		 PCI_ROM_DATA_STRUCT_SIGNATURE, signature);
>> +	return false;
>> +}
>> +
>>   /**
>>    * pci_get_rom_size - obtain the actual size of the ROM image
>>    * @pdev: target PCI device
>> @@ -99,38 +187,27 @@ 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 (!pci_rom_is_header_valid(pdev, image, rom, size, last_image))
>>   			break;
> More from Sashiko.  I'm not sure about this one.
>
>    Does this log a false-positive warning when processing the final
>    image?
>
>    When last_image is true, the image pointer is advanced to the end of
>    the ROM and passed into pci_rom_is_header_valid().
>
>    Because last_image is passed as true to the helper, the signature
>    check will fail and log an invalid header signature error for a
>    perfectly valid device instead of gracefully finishing the loop.
I rename the "last_image" param of pci_rom_is_header_valid() to 
"expect_valid", and add a !last_image check here to print "No more 
image" instead of "Invalid sig". Best Regards, Guixin Liu
>> -		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.32.0.3.g01195cf9f
>>

Re: [PATCH v11 1/2] PCI: Introduce named defines for PCI ROM

[Guixin Liu]

Changed in v12, thanks.

Best Regards,
Guixin Liu

在 2026/5/3 00:55, Krzysztof Wilczyński 写道:
> Reviewed-by: Krzysztof Wilczyński<kwilczynski@kernel.org>