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

[Alex Elder <elder@riscstar.com>]

On 8/7/26 2:54 PM, sashiko-bot@kernel.org wrote:
> 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 <elder@riscstar.com>
> 
> 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?

There are two device nodes at play here.  The first is
passed in as an argument, np, and although this is referring
to the other one, I analyzed it anyway.

The only place that pointer is used is in the three
of_changeset_add_prop_u32*() calls at the end of the
function.  No reference is taken or dropped to that
node in this function, and presumably that is up to
the caller to manage.

The only caller is of_pci_add_properties(), which similarly
gets the node pointer as an argument, and relies on its
caller for managing its references.  This function's only
caller is of_pci_make_dev_node(), whose purpose is literally
to create a new device node for the purpose of attaching
it to a PCI device node that has none.

So I don't think there is any leak of the np variable passed
in as an argument.


The second device node is pnode, which comes either from
pci_device_to_OF_node() or from pci_bus_to_OF_node().
Neither of those functions takes a reference to the
node pointer they return (assuming it's non-null).

That pointer is assigned to each out_irq[x].np field
before it's passed to of_irq_parse_raw().  The value
returned from of_irq_parse_raw() will either be a null
pointer or will be another device node *with a reference*.

We'll refer to this as *THE* reference.

If this branch is taken:

         if (out_irq->args_count != intsize)
                 goto fail;

The code at fail will properly drop the reference:

fail:
         of_node_put(ipar);

         return rc;

The next block of code gets *another* reference to ipar,
then follows the OF parent pointers up, dropping the
reference taken as each new parent is found.  Once
that loop exits, we still seem to hold THE reference,
though others seem to come and go properly.

Next there is a "big while loop", and it looks to me
like this return statement:

                         pr_debug(" -> got it !\n");
                         return 0;

...does indeed leak a reference to ipar.

There is more to study on this function to be sure this is
right.  But based on this quick look, the Sashiko report
seems right.

==> Can anyone else confirm or refute this?

If nobody tells me this is wrong, I will plan to submit
a bug fix for this.

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

Yes it can.  of_changeset_add_prop_string_array() blindly calls
strlen() on each array string provided.

I will create a bug fix for this issue as well.

					-Alex

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