[PATCH v7 0/2] PCI: Fix crash when access broken rom

[PATCH v7 0/2] PCI: Fix crash when access broken rom

[Guixin Liu]

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 aligment 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 | 130 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 108 insertions(+), 22 deletions(-)

-- 
2.43.0

[PATCH v7 1/2] PCI: Introduce named defines for pci rom

[Guixin Liu]

This is a preparation patch for the next fix patch.
Convert some magic numbers to named defines.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/pci/rom.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index e18d3a4383ba..3e00611fa76b 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -9,9 +9,20 @@
 #include <linux/export.h>
 #include <linux/pci.h>
 #include <linux/slab.h>
+#include <linux/bits.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_LEN_UNIT_SZ_512		512
+#define PCI_ROM_IMAGE_SIGNATURE			0xAA55
+#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
-- 
2.43.0

Re: [PATCH v7 1/2] PCI: Introduce named defines for pci rom

[Ilpo Järvinen]

On Thu, 11 Dec 2025, Guixin Liu wrote:

> This is a preparation patch for the next fix patch.
> Convert some magic numbers to named defines.
> 
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
>  drivers/pci/rom.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
> index e18d3a4383ba..3e00611fa76b 100644
> --- a/drivers/pci/rom.c
> +++ b/drivers/pci/rom.c
> @@ -9,9 +9,20 @@
>  #include <linux/export.h>
>  #include <linux/pci.h>
>  #include <linux/slab.h>
> +#include <linux/bits.h>

Includes should always be ordered alphabetically when they already are so.

Even in to other cases it's better try to place them close to the 
right place wrt. other headers when it comes to alphabetical order, even 
if some pre-existing headers would violate it still.

>  #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_LEN_UNIT_SZ_512		512
> +#define PCI_ROM_IMAGE_SIGNATURE			0xAA55
> +#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

You should convert the literals in the code too in this patch already.

I know it's a bit work to rebase the second patch on top of this, but you 
actually don't need to do it that way (you can just make changes to this 
patch and then use git diff to produce the very same end result so you 
don't need to resolve any rebase conflicts).

-- 
 i.

Re: [PATCH v7 1/2] PCI: Introduce named defines for pci rom

[Guixin Liu]

在 2025/12/11 17:33, Ilpo Järvinen 写道:
> On Thu, 11 Dec 2025, Guixin Liu wrote:
>
>> This is a preparation patch for the next fix patch.
>> Convert some magic numbers to named defines.
>>
>> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
>> ---
>>   drivers/pci/rom.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
>> index e18d3a4383ba..3e00611fa76b 100644
>> --- a/drivers/pci/rom.c
>> +++ b/drivers/pci/rom.c
>> @@ -9,9 +9,20 @@
>>   #include <linux/export.h>
>>   #include <linux/pci.h>
>>   #include <linux/slab.h>
>> +#include <linux/bits.h>
> Includes should always be ordered alphabetically when they already are so.
>
> Even in to other cases it's better try to place them close to the
> right place wrt. other headers when it comes to alphabetical order, even
> if some pre-existing headers would violate it still.
OK, Changed in v8, thanks.
>>   #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_LEN_UNIT_SZ_512		512
>> +#define PCI_ROM_IMAGE_SIGNATURE			0xAA55
>> +#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
> You should convert the literals in the code too in this patch already.
>
> I know it's a bit work to rebase the second patch on top of this, but you
> actually don't need to do it that way (you can just make changes to this
> patch and then use git diff to produce the very same end result so you
> don't need to resolve any rebase conflicts).
OK, I wil do this.

Best Regards,
Guixin Liu

Re: [PATCH v7 1/2] PCI: Introduce named defines for pci rom

[Andy Shevchenko]

On Thu, Dec 11, 2025 at 11:33:07AM +0200, Ilpo Järvinen wrote:
> On Thu, 11 Dec 2025, Guixin Liu wrote:

...

> I know it's a bit work to rebase the second patch on top of this, but you 
> actually don't need to do it that way (you can just make changes to this 
> patch and then use git diff to produce the very same end result so you 
> don't need to resolve any rebase conflicts).

Usually it is even easier than that with help of revert and interactive rebase.
At least that's how I reshuffle changes between patches.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v7 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 | 119 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 97 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index 3e00611fa76b..8cbcc55946a4 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -10,6 +10,9 @@
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <linux/bits.h>
+#include <linux/align.h>
+#include <linux/overflow.h>
+#include <linux/io.h>
 
 #include "pci.h"
 
@@ -80,6 +83,87 @@ 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;
+
+	/*
+	 * 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 */
+	if (readw(image) == 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, readw(image));
+	} 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;
+	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;
+
+	if (readl(pds) == 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, readl(pds));
+	return false;
+}
+
 /**
  * pci_get_rom_size - obtain the actual size of the ROM image
  * @pdev: target PCI device
@@ -95,37 +179,28 @@ static size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom,
 			       size_t size)
 {
 	void __iomem *image;
-	int last_image;
+	bool last_image;
 	unsigned int length;
 
 	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_is_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));
+		pds = image + readw(image + PCI_ROM_POINTER_TO_DATA_STRUCT);
+		if (!pci_rom_is_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)
+
+		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_LEN_UNIT_SZ_512;
+
+		if (!pci_rom_is_header_valid(pdev, image, rom, size, 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.0

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

[Ilpo Järvinen]

On Thu, 11 Dec 2025, 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 | 119 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 97 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
> index 3e00611fa76b..8cbcc55946a4 100644
> --- a/drivers/pci/rom.c
> +++ b/drivers/pci/rom.c
> @@ -10,6 +10,9 @@
>  #include <linux/pci.h>
>  #include <linux/slab.h>
>  #include <linux/bits.h>
> +#include <linux/align.h>
> +#include <linux/overflow.h>
> +#include <linux/io.h>

Order.

>  #include "pci.h"
>  
> @@ -80,6 +83,87 @@ 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;
> +
> +	/*
> +	 * 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 */
> +	if (readw(image) == 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, readw(image));

You should store the readw() value above as you use it here too so you 
don't need to read it again.

> +	} 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;
> +	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;
> +
> +	if (readl(pds) == 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, readl(pds));

Ditto.

> +	return false;
> +}
> +
>  /**
>   * pci_get_rom_size - obtain the actual size of the ROM image
>   * @pdev: target PCI device
> @@ -95,37 +179,28 @@ static size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom,
>  			       size_t size)
>  {
>  	void __iomem *image;
> -	int last_image;
> +	bool last_image;
>  	unsigned int length;
>  
>  	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_is_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));
> +		pds = image + readw(image + PCI_ROM_POINTER_TO_DATA_STRUCT);
> +		if (!pci_rom_is_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)
> +
> +		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_LEN_UNIT_SZ_512;
> +
> +		if (!pci_rom_is_header_valid(pdev, image, rom, size, 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 */
> 

-- 
 i.

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

[Guixin Liu]

在 2025/12/11 17:59, Ilpo Järvinen 写道:
> On Thu, 11 Dec 2025, 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 | 119 +++++++++++++++++++++++++++++++++++++---------
>>   1 file changed, 97 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
>> index 3e00611fa76b..8cbcc55946a4 100644
>> --- a/drivers/pci/rom.c
>> +++ b/drivers/pci/rom.c
>> @@ -10,6 +10,9 @@
>>   #include <linux/pci.h>
>>   #include <linux/slab.h>
>>   #include <linux/bits.h>
>> +#include <linux/align.h>
>> +#include <linux/overflow.h>
>> +#include <linux/io.h>
> Order.
Sure.
>>   #include "pci.h"
>>   
>> @@ -80,6 +83,87 @@ 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;
>> +
>> +	/*
>> +	 * 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 */
>> +	if (readw(image) == 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, readw(image));
> You should store the readw() value above as you use it here too so you
> don't need to read it again.
Sure.
>> +	} 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;
>> +	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;
>> +
>> +	if (readl(pds) == 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, readl(pds));
> Ditto.
>
Sure.

I will  change all in next version, thanks.

Best Regards,
Guixin Liu
>> +	return false;
>> +}
>> +
>>   /**
>>    * pci_get_rom_size - obtain the actual size of the ROM image
>>    * @pdev: target PCI device
>> @@ -95,37 +179,28 @@ static size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom,
>>   			       size_t size)
>>   {
>>   	void __iomem *image;
>> -	int last_image;
>> +	bool last_image;
>>   	unsigned int length;
>>   
>>   	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_is_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));
>> +		pds = image + readw(image + PCI_ROM_POINTER_TO_DATA_STRUCT);
>> +		if (!pci_rom_is_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)
>> +
>> +		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_LEN_UNIT_SZ_512;
>> +
>> +		if (!pci_rom_is_header_valid(pdev, image, rom, size, 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 */
>>