Linux PCI subsystem development
 help / color / mirror / Atom feed
* [PATCH v4 0/4] PCI: of: warn on bogus device_type property
@ 2026-09-04 13:46 Alex Elder
  2026-09-04 13:46 ` [PATCH v4 1/4] PCI: of: avoid allocations in of_pci_prop_compatible() Alex Elder
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Alex Elder @ 2026-09-04 13:46 UTC (permalink / raw)
  To: bhelgaas, robh, saravanak
  Cc: herve.codina, daniel, mohd.anwar, lorenzo.bianconi, linux-pci,
	devicetree, linux-kernel

Add a check when adding a PCI device to ensure the device_type
property is (or is not) correctly defined when the device has
a non-null devicetree node pointer.

PCI has a well-defined bus and device discovery process.  The
PCI_DYNAMIC_OF_NODES Kconfig option allows PCI devices to *also*
have a devicetree node.  This enables certain things that are
not possible with PCI enumeration alone.

While working on a Qualcomm platform, I learned that some PCI
endpoint nodes were defined with device_type = "pci" properties.
Herve Codina pointed out that this was not correct.  Rob Herring
indicated that people seem to have trouble getting the PCI
devicetree nodes right, and asked whether we could warn if this
particular problem occurred.  During review, Herve Codina also
suggested that bridge nodes be checked to ensure they *do*
have the proper device_type property, and later, Sashiko suggested
that "pciex" (and "cardbus") also be accepted as valid bridge
device_type property values.

This series implements these checks.  A new patch in this version
removes a (now duplicate) warning issued by the devicetree code.
The last patch that was included in v3 of the series is now gone,
after Herve pointed out it the message it removed was not in fact
redundant.

The first patch prevents a possible null pointer dereference that
Sashiko pointed out some time back.  The next two patches are simple
cleanups.  The last adds the new PCI devicetree node checks and
warnings.

					-Alex

Between version 3 and version 4:
- Insert a first patch that fixes a Sashiko-reported issue
- Drop the final "duplicate warning" patch from v3
- Include "pciex" as a valid PCI bridge device_type property value
- Add Herve's Reviewed-by tag on the last patch

Version 3 is available here:
  https://lore.kernel.org/lkml/20260901011338.1323243-1-elder@riscstar.com/

Between version 2 and version 3:
- Drop a patch that made a change only needed by a different series
- Switch a function header to use kernel-doc format
- Add a warning if a PCI bridge node has no device_type property
- Added a patch to remove a duplicate warning in the devicetree code

Version 2 is available here:
  https://lore.kernel.org/lkml/20260812172247.276554-1-elder@riscstar.com/

Between version 1 and version 2:
- Check the PCI devicetree node even when PCI_DYNAMIC_OF_NODES is
  not enabled

Version 1 is available here:
  https://lore.kernel.org/lkml/20260807194100.455599-1-elder@riscstar.com/

Alex Elder (4):
  PCI: of: avoid allocations in of_pci_prop_compatible()
  PCI: of: drop the reg_num argument to of_pci_set_address()
  PCI: of: don't zero flags in of_pci_get_addr_flags()
  PCI: of: introduce of_pci_verify_node()

 drivers/pci/bus.c         |  1 +
 drivers/pci/of.c          | 32 ++++++++++++++++++++++++++++
 drivers/pci/of_property.c | 45 ++++++++++++++++++++++-----------------
 drivers/pci/pci.h         |  3 +++
 4 files changed, 62 insertions(+), 19 deletions(-)


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.53.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v4 1/4] PCI: of: avoid allocations in of_pci_prop_compatible()
  2026-09-04 13:46 [PATCH v4 0/4] PCI: of: warn on bogus device_type property Alex Elder
@ 2026-09-04 13:46 ` Alex Elder
  2026-09-04 13:59   ` sashiko-bot
  2026-09-04 13:46 ` [PATCH v4 2/4] PCI: of: drop the reg_num argument to of_pci_set_address() Alex Elder
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Alex Elder @ 2026-09-04 13:46 UTC (permalink / raw)
  To: bhelgaas, robh, saravanak
  Cc: herve.codina, daniel, mohd.anwar, lorenzo.bianconi, linux-pci,
	devicetree, linux-kernel, Sashiko

Three compatible strings are formatted in of_pci_prop_compatible().
Their sizes are known in advance, and the largest is 16 bytes.

Rather than dynamically allocating the space for those strings, just
set aside a buffer on the stack large enough to hold all three.

This avoids a problem that Sashiko pointed out, where an allocation
failure would cause subsequent crash because strlen() is called
unconditionally in of_changeset_add_prop_string_array().

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/sashiko-reviews/a647bd56-7dc8-4fec-9d96-834622054cdf@riscstar.com
Signed-off-by: Alex Elder <elder@riscstar.com>
---
v4: - Added this fix to the beginning of the series

 drivers/pci/of_property.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
index 75a358f73e694..a0632f932b5c6 100644
--- a/drivers/pci/of_property.c
+++ b/drivers/pci/of_property.c
@@ -324,27 +324,36 @@ static int of_pci_prop_intr_map(struct pci_dev *pdev, struct of_changeset *ocs,
 	return ret;
 }
 
+/* The three compatible property strings have max sizes 12+1, 15+1, and 13+1 */
+#define PROP_SIZE	16	/* Max size of each compatible string */
 static int of_pci_prop_compatible(struct pci_dev *pdev,
 				  struct of_changeset *ocs,
 				  struct device_node *np)
 {
 	const char *compat_strs[PROP_COMPAT_NUM] = { 0 };
+	char buf[PROP_COMPAT_NUM * PROP_SIZE] = { };
+	char bufp = buf;
 	int i, ret;
 
-	compat_strs[PROP_COMPAT_PCI_VVVV_DDDD] =
-		kasprintf(GFP_KERNEL, "pci%x,%x", pdev->vendor, pdev->device);
-	compat_strs[PROP_COMPAT_PCICLASS_CCSSPP] =
-		kasprintf(GFP_KERNEL, "pciclass,%06x", pdev->class);
-	compat_strs[PROP_COMPAT_PCICLASS_CCSS] =
-		kasprintf(GFP_KERNEL, "pciclass,%04x", pdev->class >> 8);
+	ret = snprintf(bufp, PROP_SIZE, "pci%x,%x", pdev->vendor, pdev->device);
+	if (ret >= PROP_SIZE)
+		return -EINVAL;
+	compat_strs[PROP_COMPAT_PCI_VVVV_DDDD] = bufp;
+	bufp += ret + 1;
 
-	ret = of_changeset_add_prop_string_array(ocs, np, "compatible",
-						 compat_strs, PROP_COMPAT_NUM);
-	for (i = 0; i < PROP_COMPAT_NUM; i++)
-		kfree(compat_strs[i]);
+	ret = snprintf(bufp, PROP_SIZE, "pciclass,%06x", pdev->class);
+	if (ret >= PROP_SIZE)
+		return -EINVAL;
+	compat_strs[PROP_COMPAT_PCICLASS_CCSSPP] = bufp;
+	bufp += ret + 1;
 
-	return ret;
+	ret = snprintf(bufp, PROP_SIZE, "pciclass,%04x", pdev->class >> 8);
+	compat_strs[PROP_COMPAT_PCICLASS_CCSS] = bufp;
+
+	return of_changeset_add_prop_string_array(ocs, np, "compatible",
+						  compat_strs, PROP_COMPAT_NUM);
 }
+#undef PROP_SIZE
 
 int of_pci_add_properties(struct pci_dev *pdev, struct of_changeset *ocs,
 			  struct device_node *np)
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v4 2/4] PCI: of: drop the reg_num argument to of_pci_set_address()
  2026-09-04 13:46 [PATCH v4 0/4] PCI: of: warn on bogus device_type property Alex Elder
  2026-09-04 13:46 ` [PATCH v4 1/4] PCI: of: avoid allocations in of_pci_prop_compatible() Alex Elder
@ 2026-09-04 13:46 ` Alex Elder
  2026-09-04 14:03   ` sashiko-bot
  2026-09-04 13:46 ` [PATCH v4 3/4] PCI: of: don't zero flags in of_pci_get_addr_flags() Alex Elder
  2026-09-04 13:46 ` [PATCH v4 4/4] PCI: of: introduce of_pci_verify_node() Alex Elder
  3 siblings, 1 reply; 10+ messages in thread
From: Alex Elder @ 2026-09-04 13:46 UTC (permalink / raw)
  To: bhelgaas, robh, saravanak
  Cc: herve.codina, daniel, mohd.anwar, lorenzo.bianconi, linux-pci,
	devicetree, linux-kernel

The reg_num argument passed to of_pci_set_address() is always zero,
so get rid of it.

Reviewed-by: Herve Codina <herve.codina@bootlin.com>
Signed-off-by: Alex Elder <elder@riscstar.com>
---
 drivers/pci/of_property.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
index a0632f932b5c6..8131aa1b0b3c4 100644
--- a/drivers/pci/of_property.c
+++ b/drivers/pci/of_property.c
@@ -52,7 +52,7 @@ enum of_pci_prop_compatible {
 };
 
 static void of_pci_set_address(struct pci_dev *pdev, u32 *prop, u64 addr,
-			       u32 reg_num, u32 flags, bool reloc)
+			       u32 flags, bool reloc)
 {
 	if (pdev) {
 		prop[0] = FIELD_PREP(OF_PCI_ADDR_FIELD_BUS, pdev->bus->number) |
@@ -61,7 +61,7 @@ static void of_pci_set_address(struct pci_dev *pdev, u32 *prop, u64 addr,
 	} else
 		prop[0] = 0;
 
-	prop[0] |= flags | reg_num;
+	prop[0] |= flags;
 	if (!reloc) {
 		prop[0] |= OF_PCI_ADDR_FIELD_NONRELOC;
 		prop[1] = upper_32_bits(addr);
@@ -131,7 +131,7 @@ static int of_pci_prop_ranges(struct pci_dev *pdev, struct of_changeset *ocs,
 			continue;
 
 		val64 = pci_bus_address(pdev, &res[j] - pdev->resource);
-		of_pci_set_address(pdev, rp[i].parent_addr, val64, 0, flags,
+		of_pci_set_address(pdev, rp[i].parent_addr, val64, flags,
 				   false);
 		if (pci_is_bridge(pdev)) {
 			memcpy(rp[i].child_addr, rp[i].parent_addr,
@@ -164,7 +164,7 @@ static int of_pci_prop_reg(struct pci_dev *pdev, struct of_changeset *ocs,
 	struct of_pci_addr_pair reg = { 0 };
 
 	/* configuration space */
-	of_pci_set_address(pdev, reg.phys_addr, 0, 0, 0, true);
+	of_pci_set_address(pdev, reg.phys_addr, 0, 0, true);
 
 	return of_changeset_add_prop_u32_array(ocs, np, "reg", (u32 *)&reg,
 					       sizeof(reg) / sizeof(u32));
@@ -467,7 +467,7 @@ static int of_pci_host_bridge_prop_ranges(struct pci_host_bridge *bridge,
 		/* PCI bus address */
 		val64 = res->start;
 		of_pci_set_address(NULL, &ranges[ranges_sz],
-				   val64 - window->offset, 0, flags, false);
+				   val64 - window->offset, flags, false);
 		ranges_sz += OF_PCI_ADDRESS_CELLS;
 
 		/* Host bus address */
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v4 3/4] PCI: of: don't zero flags in of_pci_get_addr_flags()
  2026-09-04 13:46 [PATCH v4 0/4] PCI: of: warn on bogus device_type property Alex Elder
  2026-09-04 13:46 ` [PATCH v4 1/4] PCI: of: avoid allocations in of_pci_prop_compatible() Alex Elder
  2026-09-04 13:46 ` [PATCH v4 2/4] PCI: of: drop the reg_num argument to of_pci_set_address() Alex Elder
@ 2026-09-04 13:46 ` Alex Elder
  2026-09-04 14:06   ` sashiko-bot
  2026-09-04 13:46 ` [PATCH v4 4/4] PCI: of: introduce of_pci_verify_node() Alex Elder
  3 siblings, 1 reply; 10+ messages in thread
From: Alex Elder @ 2026-09-04 13:46 UTC (permalink / raw)
  To: bhelgaas, robh, saravanak
  Cc: herve.codina, daniel, mohd.anwar, lorenzo.bianconi, linux-pci,
	devicetree, linux-kernel

The flags variable whose address is passed to of_pci_get_addr_flags()
is zeroed before assigning a value to it.  Skip the zeroing and just
assign it instead.

Reviewed-by: Herve Codina <herve.codina@bootlin.com>
Signed-off-by: Alex Elder <elder@riscstar.com>
---
 drivers/pci/of_property.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
index 8131aa1b0b3c4..fca522f70187a 100644
--- a/drivers/pci/of_property.c
+++ b/drivers/pci/of_property.c
@@ -82,12 +82,10 @@ static int of_pci_get_addr_flags(const struct resource *res, u32 *flags)
 	else
 		return -EINVAL;
 
-	*flags = 0;
+	*flags = FIELD_PREP(OF_PCI_ADDR_FIELD_SS, ss);
 	if (res->flags & IORESOURCE_PREFETCH)
 		*flags |= OF_PCI_ADDR_FIELD_PREFETCH;
 
-	*flags |= FIELD_PREP(OF_PCI_ADDR_FIELD_SS, ss);
-
 	return 0;
 }
 
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v4 4/4] PCI: of: introduce of_pci_verify_node()
  2026-09-04 13:46 [PATCH v4 0/4] PCI: of: warn on bogus device_type property Alex Elder
                   ` (2 preceding siblings ...)
  2026-09-04 13:46 ` [PATCH v4 3/4] PCI: of: don't zero flags in of_pci_get_addr_flags() Alex Elder
@ 2026-09-04 13:46 ` Alex Elder
  2026-09-04 14:09   ` sashiko-bot
  3 siblings, 1 reply; 10+ messages in thread
From: Alex Elder @ 2026-09-04 13:46 UTC (permalink / raw)
  To: bhelgaas, robh, saravanak
  Cc: herve.codina, daniel, mohd.anwar, lorenzo.bianconi, linux-pci,
	devicetree, linux-kernel

Commit 407d1a51921e9 ("PCI: Create device tree node for bridge") linked
the PCI enumeration process together with devicetree, creating a devicetree
node for discovered PCI bridges.  Its successor commit ae9813db1dc5a ("PCI:
Add quirks to generate device tree node for Xilinx Alveo U50") shows how
to use a PCI final fixup quirk to also create a devicetree node for a
non-bridge PCI device.  These changes allowed devicetree overlays to
describe components downstream of a PCI device, by providing a place to
attach the overlay.

Note that the dynamic devicetree node is only created if the device didn't
already have an assigned node.

Later, commit aa7b4bbcb3a1d ("arm64: dts: qcom: qcs6490-rb3gen2: Add
TC9563 PCIe switch node") *pre-defined* devicetree nodes to represent the
PCI device nodes that would (also) be discovered via the PCI enumeration
process.  The devicetree node in this case is created with the content
from the DTS file.  So when a (host) bridge is done being initialized
during PCI enumeration, no node is dynamically created (the commits
mentioned above do not apply).

Ideally, any pre-defined PCI devicetree node would contain exactly the
same information as whatever the dynamic creation process would produce
(though it could include more).

However that is not the case for the pre-defined Qualcomm RB3gen2 nodes.
And in particular, the endpoint (function) nodes include this property:

    device_type = "pci";

This is simply wrong; that property is meant only for bridge nodes.

Rob Herring requested that a runtime check to be added to spot this
specific error, only for non-bridge PCI devices.  Herve Codina further
suggested we ensure that bridge PCI devices *do* define the device_type
property, with value "pci".  We also accept "pciex" as the value of the
device_type property for bridges.

Reviewed-by: Herve Codina <herve.codina@bootlin.com>
Signed-off-by: Alex Elder <elder@riscstar.com>
---
v4: - Added "pciex" as a valid bridge device_type property value
    - Added Herve's Reviewed-by tag

 drivers/pci/bus.c |  1 +
 drivers/pci/of.c  | 32 ++++++++++++++++++++++++++++++++
 drivers/pci/pci.h |  3 +++
 3 files changed, 36 insertions(+)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 655ed53436d3e..679afbc6d3109 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -351,6 +351,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 	 * are not assigned yet for some devices.
 	 */
 	pcibios_bus_add_device(dev);
+	of_pci_verify_node(dev);
 	pci_fixup_device(pci_fixup_final, dev);
 	if (pci_is_bridge(dev))
 		of_pci_make_dev_node(dev);
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index a51dff91b196d..5a040ed836744 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -1085,3 +1085,35 @@ int of_pci_get_equalization_presets(struct device *dev,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
+
+/**
+ * of_pci_verify_node - Sanity check some PCI device node properties
+ * @pdev: The PCI device whose device node is checked
+ *
+ * PCI enumeration authoritatively discovers what we need to know about
+ * a PCI device.  A devicetree-based platform will represent a PCI root
+ * bridge with a node, but otherwise devicetree doesn't typically include
+ * many PCI nodes.  Where such nodes do exist, experience has shown that
+ * the "device_type" property is sometimes wrong, so warn about that.
+ */
+void of_pci_verify_node(struct pci_dev *pdev)
+{
+	struct device_node *np = pci_device_to_OF_node(pdev);
+	bool device_is_bridge;
+	bool device_type_pci;
+
+	/* Nothing to check if there's no pre-existing devicetree node */
+	if (!np)
+		return;
+
+	device_is_bridge = pci_is_bridge(pdev);
+	device_type_pci = of_node_is_type(np, "pci") ||
+			  of_node_is_type(np, "pciex");
+
+	/* Bridges should have device type "pci"; endpoints should not */
+	if (device_is_bridge == device_type_pci)
+		return;
+
+	dev_err(&pdev->dev, "PCI %s have \"pci\" device_type property\n",
+		device_is_bridge ? "bridge should" : "endpoint should not");
+}
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index ba3c3fddddc23..2e33d3bd4b0ba 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -1253,6 +1253,7 @@ bool of_pci_supply_present(struct device_node *np);
 int of_pci_get_equalization_presets(struct device *dev,
 				    struct pci_eq_presets *presets,
 				    int num_lanes);
+void of_pci_verify_node(struct pci_dev *pdev);
 #else
 static inline int
 of_get_pci_domain_nr(struct device_node *node)
@@ -1308,6 +1309,8 @@ static inline int of_pci_get_equalization_presets(struct device *dev,
 
 	return 0;
 }
+
+static inline void of_pci_verify_node(struct pci_dev *pdev) { }
 #endif /* CONFIG_OF */
 
 struct of_changeset;
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 1/4] PCI: of: avoid allocations in of_pci_prop_compatible()
  2026-09-04 13:46 ` [PATCH v4 1/4] PCI: of: avoid allocations in of_pci_prop_compatible() Alex Elder
@ 2026-09-04 13:59   ` sashiko-bot
  2026-09-04 14:04     ` Alex Elder
  0 siblings, 1 reply; 10+ messages in thread
From: sashiko-bot @ 2026-09-04 13:59 UTC (permalink / raw)
  To: Alex Elder; +Cc: linux-pci, devicetree, robh, conor+dt

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The local variable `bufp` is incorrectly declared as `char` instead of `char *`. This truncates the stack array pointer `buf` to an 8-bit integer, leading to severe memory corruption and an immediate kernel panic during runtime.
- [Low] The local variable `i` is declared but never used after the patch.
--

commit 0153310b625600febac7ea5aec9fbd9871d0af03
Author: Alex Elder <elder@riscstar.com>

PCI: of: avoid allocations in of_pci_prop_compatible()

This patch replaces dynamic allocations for three compatible strings with a
single stack-allocated buffer. This avoids subsequent kernel crashes where
allocation failures would unconditionally trigger strlen() during device tree
properties initialization.

Link: https://lore.kernel.org/sashiko-reviews/a647bd56-7dc8-4fec-9d96-834622054cdf@riscstar.com

> diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
> --- a/drivers/pci/of_property.c
> +++ b/drivers/pci/of_property.c
[ ... ]
>  static int of_pci_prop_compatible(struct pci_dev *pdev,
>  				  struct of_changeset *ocs,
>  				  struct device_node *np)
>  {
>  	const char *compat_strs[PROP_COMPAT_NUM] = { 0 };
> +	char buf[PROP_COMPAT_NUM * PROP_SIZE] = { };
> +	char bufp = buf;

[Severity: High]
Does this incorrectly declare bufp as a single char instead of a pointer?
Assigning the array address buf to an 8-bit char truncates the memory address.

>  	int i, ret;

[Severity: Low]
Is the variable i still needed here? The loop that used it to free the
allocated strings was removed by this patch, which will leave this as an
unused variable and cause a compiler warning.

> -	compat_strs[PROP_COMPAT_PCI_VVVV_DDDD] =
> -		kasprintf(GFP_KERNEL, "pci%x,%x", pdev->vendor, pdev->device);
> -	compat_strs[PROP_COMPAT_PCICLASS_CCSSPP] =
> -		kasprintf(GFP_KERNEL, "pciclass,%06x", pdev->class);
> -	compat_strs[PROP_COMPAT_PCICLASS_CCSS] =
> -		kasprintf(GFP_KERNEL, "pciclass,%04x", pdev->class >> 8);
> +	ret = snprintf(bufp, PROP_SIZE, "pci%x,%x", pdev->vendor, pdev->device);

[Severity: High]
Will this snprintf call trigger a kernel panic upon dereference? Because bufp
is truncated to an 8-bit integer, passing it as the output buffer pointer to
snprintf() will result in an out-of-bounds memory access (unmapped low-memory
dereference) when evaluated during system boot or PCI device enumeration.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904134607.1856121-1-elder@riscstar.com?part=1

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 2/4] PCI: of: drop the reg_num argument to of_pci_set_address()
  2026-09-04 13:46 ` [PATCH v4 2/4] PCI: of: drop the reg_num argument to of_pci_set_address() Alex Elder
@ 2026-09-04 14:03   ` sashiko-bot
  0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-09-04 14:03 UTC (permalink / raw)
  To: Alex Elder; +Cc: devicetree, conor+dt, robh, linux-pci

> The reg_num argument passed to of_pci_set_address() is always zero,
> so get rid of it.
> 
> Reviewed-by: Herve Codina <herve.codina@bootlin.com>
> Signed-off-by: Alex Elder <elder@riscstar.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904134607.1856121-1-elder@riscstar.com?part=2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 1/4] PCI: of: avoid allocations in of_pci_prop_compatible()
  2026-09-04 13:59   ` sashiko-bot
@ 2026-09-04 14:04     ` Alex Elder
  0 siblings, 0 replies; 10+ messages in thread
From: Alex Elder @ 2026-09-04 14:04 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: linux-pci, devicetree, robh, conor+dt

On 9/4/26 8:59 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The local variable `bufp` is incorrectly declared as `char` instead of `char *`. This truncates the stack array pointer `buf` to an 8-bit integer, leading to severe memory corruption and an immediate kernel panic during runtime.
> - [Low] The local variable `i` is declared but never used after the patch.

I'm very sorry about this; I'm not sure how it even happened.
I must have included the wrong patch when I prepared the series
(but I normally check builds as part of that process).  This
should not have even compiled...

I will fix this and will send out a new version tonight or
tomorrow.

					-Alex

> --
> 
> commit 0153310b625600febac7ea5aec9fbd9871d0af03
> Author: Alex Elder <elder@riscstar.com>
> 
> PCI: of: avoid allocations in of_pci_prop_compatible()
> 
> This patch replaces dynamic allocations for three compatible strings with a
> single stack-allocated buffer. This avoids subsequent kernel crashes where
> allocation failures would unconditionally trigger strlen() during device tree
> properties initialization.
> 
> Link: https://lore.kernel.org/sashiko-reviews/a647bd56-7dc8-4fec-9d96-834622054cdf@riscstar.com
> 
>> diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
>> --- a/drivers/pci/of_property.c
>> +++ b/drivers/pci/of_property.c
> [ ... ]
>>   static int of_pci_prop_compatible(struct pci_dev *pdev,
>>   				  struct of_changeset *ocs,
>>   				  struct device_node *np)
>>   {
>>   	const char *compat_strs[PROP_COMPAT_NUM] = { 0 };
>> +	char buf[PROP_COMPAT_NUM * PROP_SIZE] = { };
>> +	char bufp = buf;
> 
> [Severity: High]
> Does this incorrectly declare bufp as a single char instead of a pointer?

Yes this will be:

	char *bufp = buf;

> Assigning the array address buf to an 8-bit char truncates the memory address.
> 
>>   	int i, ret;

This will be:

	int ret;

> [Severity: Low]
> Is the variable i still needed here? The loop that used it to free the
> allocated strings was removed by this patch, which will leave this as an
> unused variable and cause a compiler warning.
> 
>> -	compat_strs[PROP_COMPAT_PCI_VVVV_DDDD] =
>> -		kasprintf(GFP_KERNEL, "pci%x,%x", pdev->vendor, pdev->device);
>> -	compat_strs[PROP_COMPAT_PCICLASS_CCSSPP] =
>> -		kasprintf(GFP_KERNEL, "pciclass,%06x", pdev->class);
>> -	compat_strs[PROP_COMPAT_PCICLASS_CCSS] =
>> -		kasprintf(GFP_KERNEL, "pciclass,%04x", pdev->class >> 8);
>> +	ret = snprintf(bufp, PROP_SIZE, "pci%x,%x", pdev->vendor, pdev->device);
> 
> [Severity: High]
> Will this snprintf call trigger a kernel panic upon dereference? Because bufp
> is truncated to an 8-bit integer, passing it as the output buffer pointer to
> snprintf() will result in an out-of-bounds memory access (unmapped low-memory
> dereference) when evaluated during system boot or PCI device enumeration.

This will be fixed by using a proper type for bufp.

> 
> [ ... ]
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 3/4] PCI: of: don't zero flags in of_pci_get_addr_flags()
  2026-09-04 13:46 ` [PATCH v4 3/4] PCI: of: don't zero flags in of_pci_get_addr_flags() Alex Elder
@ 2026-09-04 14:06   ` sashiko-bot
  0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-09-04 14:06 UTC (permalink / raw)
  To: Alex Elder; +Cc: linux-pci, devicetree, conor+dt, robh

> The flags variable whose address is passed to of_pci_get_addr_flags()
> is zeroed before assigning a value to it.  Skip the zeroing and just
> assign it instead.
> 
> Reviewed-by: Herve Codina <herve.codina@bootlin.com>
> Signed-off-by: Alex Elder <elder@riscstar.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904134607.1856121-1-elder@riscstar.com?part=3


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 4/4] PCI: of: introduce of_pci_verify_node()
  2026-09-04 13:46 ` [PATCH v4 4/4] PCI: of: introduce of_pci_verify_node() Alex Elder
@ 2026-09-04 14:09   ` sashiko-bot
  0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-09-04 14:09 UTC (permalink / raw)
  To: Alex Elder; +Cc: linux-pci, robh, conor+dt, devicetree

> Commit 407d1a51921e9 ("PCI: Create device tree node for bridge") linked
> the PCI enumeration process together with devicetree, creating a devicetree
> node for discovered PCI bridges.  Its successor commit ae9813db1dc5a ("PCI:
> Add quirks to generate device tree node for Xilinx Alveo U50") shows how
> to use a PCI final fixup quirk to also create a devicetree node for a
> non-bridge PCI device.  These changes allowed devicetree overlays to
> describe components downstream of a PCI device, by providing a place to
> attach the overlay.
> 
> Note that the dynamic devicetree node is only created if the device didn't
> already have an assigned node.
> 
> Later, commit aa7b4bbcb3a1d ("arm64: dts: qcom: qcs6490-rb3gen2: Add
> TC9563 PCIe switch node") *pre-defined* devicetree nodes to represent the
> PCI device nodes that would (also) be discovered via the PCI enumeration
> [ ... ]
> property, with value "pci".  We also accept "pciex" as the value of the
> device_type property for bridges.
> 
> Reviewed-by: Herve Codina <herve.codina@bootlin.com>
> Signed-off-by: Alex Elder <elder@riscstar.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904134607.1856121-1-elder@riscstar.com?part=4


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2026-09-04 14:09 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-04 13:46 [PATCH v4 0/4] PCI: of: warn on bogus device_type property Alex Elder
2026-09-04 13:46 ` [PATCH v4 1/4] PCI: of: avoid allocations in of_pci_prop_compatible() Alex Elder
2026-09-04 13:59   ` sashiko-bot
2026-09-04 14:04     ` Alex Elder
2026-09-04 13:46 ` [PATCH v4 2/4] PCI: of: drop the reg_num argument to of_pci_set_address() Alex Elder
2026-09-04 14:03   ` sashiko-bot
2026-09-04 13:46 ` [PATCH v4 3/4] PCI: of: don't zero flags in of_pci_get_addr_flags() Alex Elder
2026-09-04 14:06   ` sashiko-bot
2026-09-04 13:46 ` [PATCH v4 4/4] PCI: of: introduce of_pci_verify_node() Alex Elder
2026-09-04 14:09   ` sashiko-bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox