Re: [PATCH 1/4] PCI: of: drop the reg_num argument to of_pci_set_address()

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.

> diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
> index 75a358f73e694..505226b876c56 100644
> --- a/drivers/pci/of_property.c
> +++ b/drivers/pci/of_property.c

[ ... ]

[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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1