PCI: of: warn on bogus device_type property

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing of_node_put() in of_pci_prop_intr_map() causes a memory leak of interrupt parent device nodes.
- [High] Missing NULL checks for kasprintf() returns in of_pci_prop_compatible() leads to a kernel crash via strlen(NULL). --

--- Patch [1]: [PATCH 1/4] PCI: of: drop the reg_num argument to of_pci_set_address() --- commit 2224ebd8fd372dc1822f3ceaf7519520a5ca9bdf
Author: Alex Elder <[email protected]>

PCI: of: drop the reg_num argument to of_pci_set_address()

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

Quoted 4 lines

drivers/pci/of_property.c +0 -0

index 75a358f73e694..505226b876c56 100644

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this file I noticed a potential resource leak in of_pci_prop_intr_map().

Does this code leak the interrupt parent device nodes?

drivers/pci/of_property.c:of_pci_prop_intr_map() {
     ...
     for (pin = 1; pin <= OF_PCI_MAX_INT_PIN;  pin++) {
         i = pin - 1;
         out_irq[i].np = pnode;
         out_irq[i].args_count = 1;
         out_irq[i].args[0] = pin;
         ret = of_irq_parse_raw(laddr, &out_irq[i]);
         if (ret) {
             out_irq[i].np = NULL;
             pci_dbg(pdev, "parse irq %d failed, ret %d", pin, ret);
             continue;
         }
         of_property_read_u32(out_irq[i].np, "#address-cells",
                              &addr_sz[i]);
     }
     ...
     return 0;

failed:

     kfree(int_map);
     return ret;
}

of_irq_parse_raw() populates out_irq[i].np with a device node that has an incremented reference count.

Should there be a matching of_node_put() called on these successfully parsed nodes before the function returns?

[Severity: High]
This is a pre-existing issue, but I also noticed a potential NULL pointer dereference in of_pci_prop_compatible().

Can this crash the kernel if memory allocation fails?

drivers/pci/of_property.c:of_pci_prop_compatible() {
     ...
     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 = of_changeset_add_prop_string_array(ocs, np, "compatible",
                                              compat_strs, PROP_COMPAT_NUM);
     ...
}

If kasprintf() returns NULL, the compat_strs array will contain NULL pointers.

When this unvalidated array is passed to of_changeset_add_prop_string_array(), could it lead to a kernel crash when the string array is processed and strlen() is called on a NULL element?

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The function of_pci_is_range_resource() incorrectly evaluates IORESOURCE_MEM_64 using the resource_type() macro. The resource_type(res) macro applies the IORESOURCE_TYPE_BITS mask (0x1f00). Because IORESOURCE_MEM_64 is defined as 0x00100000, it falls completely outside this mask. Consequently, resource_type(res) & IORESOURCE_MEM_64 evaluates to a constant 0.

As a result, the condition !(resource_type(res) & IORESOURCE_MEM_64) is unconditionally true. This simplifies the entire filter condition to !(resource_type(res) & IORESOURCE_MEM). Any resource that does not have the IORESOURCE_MEM bit set—such as IORESOURCE_IO (0x100)—will cause the condition to evaluate to true, making the function return false.

This logic flaw inadvertently filters out IORESOURCE_IO resources entirely, which prevents PCI host bridges from exposing IO space mappings via the dynamically generated device tree ranges property. Furthermore, if a 64-bit memory resource were defined with IORESOURCE_MEM_64 but without IORESOURCE_MEM, it would also be incorrectly rejected. --

--- Patch [2]: [PATCH 2/4] PCI: of: don't zero flags in of_pci_get_addr_flags() --- commit d6dba8ef5457a1efb23f2b68f270883ef856bbdb
Author: Alex Elder <[email protected]>

PCI: of: don't zero flags in of_pci_get_addr_flags()

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.

Quoted 4 lines

drivers/pci/of_property.c +0 -0

index 505226b876c56..22fea1905a080 100644

[ ... ]

Quoted 2 lines

static bool of_pci_is_range_resource(const struct resource *res, u32 *flags) {

[ ... ]

Quoted 3 lines

	if (!(resource_type(res) & IORESOURCE_MEM) &&
	    !(resource_type(res) & IORESOURCE_MEM_64))
		return false;

[Severity: High]
This is a pre-existing issue, but does this correctly evaluate IORESOURCE_MEM_64?

The resource_type() macro applies the IORESOURCE_TYPE_BITS mask (0x1f00). Because IORESOURCE_MEM_64 is defined as 0x00100000, it falls outside this mask, causing resource_type(res) & IORESOURCE_MEM_64 to evaluate to a constant 0.

This makes the condition !(resource_type(res) & IORESOURCE_MEM_64) unconditionally true. This simplifies the logic to reject any resource that does not have the IORESOURCE_MEM bit set, such as IORESOURCE_IO.

Could this inadvertently filter out IORESOURCE_IO resources entirely when called from of_pci_host_bridge_prop_ranges()?

drivers/pci/of_property.c:of_pci_host_bridge_prop_ranges() {
	...
	resource_list_for_each_entry(window, &bridge->windows) {
		res = window->res;
		if (!of_pci_is_range_resource(res, &flags))
			continue;
	...
}

If IO windows are wrongly skipped here, will dynamically created PCI host bridge device tree nodes lack IO mappings in their ranges property?

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The static DT node verification function of_pci_verify_node() is mistakenly placed under #ifdef CONFIG_PCI_DYNAMIC_OF_NODES, making it a silent no-op on systems that only use statically-defined nodes.

--

--- Patch [4]: [PATCH 4/4] PCI: of: introduce of_pci_verify_node() --- commit 29170ab92dd207c740eed74438cd2e09bad49131
Author: Alex Elder <[email protected]>

PCI: of: introduce of_pci_verify_node()

This commit introduces a runtime check to verify that statically-defined (pre-existing) devicetree nodes for non-bridge PCI endpoints do not incorrectly contain the device_type = "pci" property. It adds a call to of_pci_verify_node() during PCI bus device addition to catch these errors.

drivers/pci/pci.h static inline int of_pci_get_equalization_presets(struct device *dev, · line 1268

Quoted 8 lines

drivers/pci/pci.h +1 -0

@@ -1265,6 +1265,7 @@ static inline int of_pci_get_equalization_presets(struct device *dev,
  struct of_changeset;
 
  #ifdef CONFIG_PCI_DYNAMIC_OF_NODES
+void of_pci_verify_node(struct pci_dev *pdev);

[Severity: Medium]
Since the intent is to verify statically-defined devicetree nodes, will placing this declaration inside the CONFIG_PCI_DYNAMIC_OF_NODES block accidentally disable the check on platforms that only use static nodes?

If CONFIG_PCI_DYNAMIC_OF_NODES is disabled, this falls back to the empty stub below, causing the verification to be silently skipped.

Quoted 2 lines

drivers/pci/pci.h +0 -0

  void of_pci_make_dev_node(struct pci_dev *pdev);
  void of_pci_remove_node(struct pci_dev *pdev);

[ ... ]

Quoted 3 lines

#else
+static inline void of_pci_verify_node(struct pci_dev *pdev) { }
static inline void of_pci_make_dev_node(struct pci_dev *pdev) { }