[{"id":3688750,"web_url":"http://patchwork.ozlabs.org/comment/3688750/","msgid":"<20260508222005.GA25034@bhelgaas>","list_archive_url":null,"date":"2026-05-08T22:20:05","subject":"Re: [PATCH v12 0/2] PCI: Fix crash when access broken ROM","submitter":{"id":67298,"url":"http://patchwork.ozlabs.org/api/people/67298/","name":"Bjorn Helgaas","email":"helgaas@kernel.org"},"content":"On Fri, May 08, 2026 at 04:21:26PM +0800, Guixin Liu wrote:\n> v11 -> v12:\n> - Add rb tag from Krzysztof Wilczyński in the first patch, thanks.\n> - Change \"get\" to \"Get\".\n> - Renamed parameter last_image → expect_valid in\n> pci_rom_is_header_valid() to better reflect its semantics: it\n> indicates whether the caller expects the image to be valid\n> (and thus whether a missing/invalid signature should be reported\n> as an error or as normal end-of-chain).\n> - Tightened image alignment check: replaced the 2-byte alignment check\n> with a 512-byte (PCI_ROM_IMAGE_SECTOR_SIZE) alignment check on image,\n> per PCI Firmware Specification r3.3, sec 5.1, which mandates that each\n> ROM image starts on a 512-byte boundary. This also satisfies the\n> natural-alignment requirement of readw() on architectures such as arm64.\n> - Updated comment to cite the PCI Firmware Spec r3.3 sec 5.1 as the\n> authoritative source for the alignment requirement, and to explain the\n> relationship between page-aligned rom, sector-aligned image,\n> and the IOMEM access constraint.\n> - Fixed off-by-one in overflow checks: check_add_overflow() now uses\n> PCI_ROM_HEADER_SIZE - 1 and data_len - 1 so that header_end / end\n> represent the inclusive last byte of the region, matching the\n> subsequent > rom_end comparison.\n> - Refactored signature-check log flow: collapsed the dual-return branches\n> into a single if (signature != PCI_ROM_IMAGE_SIGNATURE) block, emitting\n> the appropriate pci_info() based on expect_valid, then returning false;\n> success path returns true at the end.\n> - Reorder pci_rom_is_data_struct_valid() to check the \"PCIR\" signature\n> before reading data_len, so bad signatures are still logged.\n> - Collapse the signature branch to early-return on failure,\n> matching the style of pci_rom_is_header_valid().\n> - Add PCI_ROM_DATA_STRUCT_MIN_LEN (0x18), the PCI 2.x baseline PCI Data\n> Structure length.\n> - Reject data_len < PCI_ROM_DATA_STRUCT_MIN_LEN to keep the fixed-offset\n> reads (PCI_ROM_IMAGE_LEN @0x10, PCI_ROM_LAST_IMAGE_INDICATOR @0x15)\n> in pci_get_rom_size() inside the mapped ROM window.\n> - Cite PCI Firmware Spec r3.3 sec 5.1.3 Table 5-2 in the new macro's\n> comment.\n> \n> v10 -> v11:\n> - Change 'pci rom' to 'PCI ROM' of the tittle of the first patch.\n> - Add Andy Shevchenko's rb tag in the first patch, thanks. \n> \n> v9 -> v10:\n> - Reorder the header files, and not touch kernel.h\n> - Change PCI_ROM_IMAGE_LEN_UNIT_BYTES to PCI_ROM_IMAGE_SECTOR_SIZE.\n> - Add a comment for PCI_ROM_DATA_STRUCT_SIGNATURE.\n> \n> v8 -> v9:\n> - Supplemental explanation for the commit body of the first patch.\n> - Change PCI_ROM_IMAGE_LEN_UNIT_SZ_512 to PCI_ROM_IMAGE_LEN_UNIT_BYTES,\n> and change it's definition to SZ_512.\n> - Use u16 and u32 for signature val instead of unsigned short/int.\n> \n> v7 -> v8:\n> - Ordered header files alphabetically.\n> - Convert the literals too in the firt patch.\n> - Use local val to save signature instead of reading twice.\n> \n> v6 -> v7:\n> - Put all named defines to a separate patch.\n> - Change PCI_ROM_IMAGE_LEN_UNIT_BYTES to PCI_ROM_IMAGE_LEN_UNIT_SZ_512.\n> - Named BIT(7) to PCI_ROM_LAST_IMAGE_INDICATOR_BIT.\n> - Fix all other comments from Ilpo, such as including header files,\n> and alignment fault, Thanks.\n> \n> v5 -> v6:\n> - Convert some magic number to named defines, suggested by\n> Ilpo, thanks.\n> \n> v4 -> v5:\n> - Add Andy Shevchenko's rb tag, thanks.\n> - Change u64 to unsigned long.\n> - Change pci_rom_header_valid() to pci_rom_is_header_valid() and\n> change pci_rom_data_struct_valid() to pci_rom_is_data_struct_valid().\n> - Change rom_end from rom+size to rom+size-1 for more readble,\n> and also change header_end >= rom_end to header_end > rom_end, same\n> as data structure end.\n> - Change if(!last_image) to if (last_image)..\n> - Use U16_MAX instead of 0xffff.\n> - Split check_add_overflow() from data_len checking.\n> - Remove !!() when reading last_image, and Use BIT(7) instead of 0x80.\n> \n> v3 -> v4:\n> - Use \"u64\" instead of \"uintptr_t\".\n> - Invert the if statement to avoid excessive indentation.\n> - Add comment for alignment checking.\n> - Change last_image's type from int to bool.\n> \n> v2 -> v3:\n> - Add pci_rom_header_valid() helper for checking image addr and signature.\n> - Add pci_rom_data_struct_valid() helper for checking data struct add\n> and signature.\n> - Handle overflow issue when adding addr with size.\n> - Handle alignment fault when running on arm64.\n> \n> v1 -> v2:\n> - Fix commit body problems, such as blank line in \"Call Trace\" both sides,\n> thanks, (Andy Shevchenko).\n> - Remove every step checking, just check the addr is in header or data\n> struct.\n> - Add Suggested-by: Guanghui Feng tag.\n> \n> Guixin Liu (2):\n> PCI: Introduce named defines for PCI ROM\n> PCI: Check ROM header and data structure addr before accessing\n> \n> drivers/pci/rom.c | 154 +++++++++++++++++++++++++++++++++++++++-------\n> 1 file changed, 131 insertions(+), 23 deletions(-)\n\nApplied to pci/rom for v7.2, thanks for all your work on this!","headers":{"Return-Path":"\n ","X-Original-To":["incoming@patchwork.ozlabs.org","linux-pci@vger.kernel.org"],"Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256\n header.s=k20201202 header.b=fLMR5BFr;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=172.105.105.114; helo=tor.lore.kernel.org;\n envelope-from=linux-pci+bounces-54297-incoming=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=\"fLMR5BFr\"","smtp.subspace.kernel.org;\n arc=none smtp.client-ip=10.30.226.201"],"Received":["from tor.lore.kernel.org (tor.lore.kernel.org [172.105.105.114])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange x25519 server-signature ECDSA (secp384r1) server-digest SHA384)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4gC3TS4d1Xz1yK7\n\tfor ; Sat, 09 May 2026 08:20:12 +1000 (AEST)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby tor.lore.kernel.org (Postfix) with ESMTP id 198B3301A7C5\n\tfor ; Fri, 8 May 2026 22:20:10 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id 530DA39C01A;\n\tFri, 8 May 2026 22:20:07 +0000 (UTC)","from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org\n [10.30.226.201])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id 3013E39C013\n\tfor ; Fri, 8 May 2026 22:20:07 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id DFB1DC2BCB0;\n\tFri, 8 May 2026 22:20:06 +0000 (UTC)"],"ARC-Seal":"i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;\n\tt=1778278807; cv=none;\n b=ml9O812ChMUtbYI00vfErkAXgAu4TWSiBVLTT8coWuF6dgHcxR4eWqYU3hEMv2SBRyBn7WkulHMQ1qnWq3DK1xJIXndscqVZhYlOaGjTrUL24c/6/iQ0d59Gvq81tmrA0qvWi1FG8+wq98fS1yjAXw0U/5bSwgCusM9oJsfVQOc=","ARC-Message-Signature":"i=1; a=rsa-sha256; d=subspace.kernel.org;\n\ts=arc-20240116; t=1778278807; c=relaxed/simple;\n\tbh=NPutYCJSXhX9kT8mMtUc6ImrsCe8AtpGnv1xsAeo6ps=;\n\th=Date:From:To:Cc:Subject:Message-ID:MIME-Version:Content-Type:\n\t Content-Disposition:In-Reply-To;\n b=UgUeRAAj8A/T/yBFcWdpyHC6iBlj4BIbbOk8WJqN2yfeN14YDCqt/fMPAPqGfb0S9YgEPCmu6ODL9oU19Ta3AptDAfB8+GMET981pqx1B7DczPUEAjWyq+qaFXA40ZVtqz3E4uMJ+b/J6FukhDPfgH+o0eFXN6NGJRfP1ark1Kc=","ARC-Authentication-Results":"i=1; smtp.subspace.kernel.org;\n dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=fLMR5BFr; arc=none smtp.client-ip=10.30.226.201","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1778278807;\n\tbh=NPutYCJSXhX9kT8mMtUc6ImrsCe8AtpGnv1xsAeo6ps=;\n\th=Date:From:To:Cc:Subject:In-Reply-To:From;\n\tb=fLMR5BFr7Bm7yO1SfPu/zlKwIBmuZvijcb8sgqlsWTbpsy+kUuYHdiyZFeCWcLACn\n\t 19fh9KfZt4rFZ5Jd8AimIwjPdXFHHQ0I+EMaHh7WqTzpFzrL3yc7rWj7GQlI9pvquo\n\t nTqopZTc65buCC2tzGzqyNn9meh+6EllAQIOX6j/m+f9n0Hh/glDZM3HR+3lh46ILL\n\t lLoR25bVRLfj04kc5sDQJYhD8kEf1FuSgKe+ooPIwplQPkif0meWxVeWGw7XkKcXaX\n\t YSPiibNKwjIbtVmhwWN6dgvelAlOufzA1fKn294jsyLdgfarbZNoJ6YcNOtGvxy1+7\n\t MPNVQplhIXeeg==","Date":"Fri, 8 May 2026 17:20:05 -0500","From":"Bjorn Helgaas ","To":"Guixin Liu ","Cc":"Bjorn Helgaas ,\n Andy Shevchenko ,\n Ilpo =?utf-8?b?SsOkcnZpbmVu?= , Krzysztof\n\t=?utf-8?q?Wilczy=C5=84ski?= ,\n linux-pci@vger.kernel.org, xlpang@linux.alibaba.com,\n oliver.yang@linux.alibaba.com","Subject":"Re: [PATCH v12 0/2] PCI: Fix crash when access broken ROM","Message-ID":"<20260508222005.GA25034@bhelgaas>","Precedence":"bulk","X-Mailing-List":"linux-pci@vger.kernel.org","List-Id":"","List-Subscribe":"","List-Unsubscribe":"","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20260508082128.3344255-1-kanie@linux.alibaba.com>"}}]