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

[Bjorn Helgaas <helgaas@kernel.org>]

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!