Re: [PATCH] of/irq: Fix device node refcount leak in of_check_msi_parent()

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Fri, Sep 04, 2026 at 10:25:24AM +0100, Fuad Tabba wrote:
> On Fri, 4 Sept 2026 at 08:48, <sashiko-bot@kernel.org> wrote:
> >
> > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> >
> > Pre-existing issues:
> > - [High] of_msi_xlate() fails to traverse the device hierarchy when msi_np is NULL, completely breaking MSI ID mapping for callers such as the fsl-mc subsystem.
> > - [High] msi_node reference is leaked on error path in its_v5_pci_msi_prepare()
> > - [High] of_msi_xlate() incorrectly handles 'msi-parent' by continuing tree traversal when it mismatches.
> > --
> >
> > commit b8991c82a3994d7ebf04943226ad5d1d9490a8d4
> > Author: Fuad Tabba <fuad.tabba@linux.dev>
> >
> > of/irq: Fix device node refcount leak in of_check_msi_parent()
> >
> > The patch fixes a device node refcount leak in of_check_msi_parent(). It
> > ensures that if the node was not matched in a 1:1 mapping, the reference is
> > released correctly before returning.
> >
> > Link: https://lore.kernel.org/all/20260826113604.010C01F000E9@smtp.kernel.org/
> >
> > > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > > index ec035367c9500..109c54f346479 100644
> > > --- a/drivers/of/irq.c
> > > +++ b/drivers/of/irq.c
> > > @@ -785,7 +785,7 @@ static int of_check_msi_parent(struct device_node *dev_node, struct device_node
> > >       if ((*msi_node && *msi_node != msi_spec.np) || msi_spec.args_count != 0)
> > >               ret = -EINVAL;
> > >
> > > -     if (!ret) {
> > > +     if (!ret && !*msi_node) {
> > >               /* Return with a node reference held */
> > >               *msi_node = msi_spec.np;
> > >               return 0;
> >
> > [Severity: High]
> > This isn't a bug introduced by this patch, but of_msi_xlate() seems to
> > incorrectly handle msi-parent mismatches.
> >
> > If a device specifies an msi-parent that does not match the msi_np filter,
> > of_check_msi_parent() returns -EINVAL.
> >
> > drivers/of/irq.c:of_msi_xlate() {
> >                 /* -ENODEV: msi-map absent -> check for msi-parent */
> >                 if (msi_np && !of_check_msi_parent(parent_dev->of_node, msi_np))
> >                         break;
> > }
> >
> > The condition !of_check_msi_parent() evaluates to false, skipping the break
> > statement and allowing traversal to continue. Will this erroneously allow
> > traversal up the tree, potentially inheriting an incorrect msi-map from a
> > parent bus?
> 
> It does continue. -EINVAL also covers #msi-cells != 0, so
> of_msi_xlate() cannot tell a phandle mismatch from an msi-parent that
> is not a 1:1 mapping, and breaking on it would stop the walk for both.
> 
> Lorenzo, Rob, should the walk stop at the first node declaring an msi-parent?

First off, thank you for fixing this.

I think we must stop at the first msi-parent and apply the same (convoluted)
logic that of_msi_xlate() executes upon of_map_msi_id()'s return to make
sure ref counts are kosher (we already have an msi_spec scratch variable,
we can pass that to of_check_msi_parent() - that is, always drop the
msi_spec.np reference if !NULL and get one if !*msi_np to initialize it).

> 
> >
> > [Severity: High]
> > This is a pre-existing issue, but does of_msi_xlate() fail to traverse the
> > device hierarchy when msi_np is NULL?
> >
> > When of_map_msi_id() (which calls of_map_id()) processes an absent map
> > property while filter_np is NULL, it returns 0 instead of an error:
> >
> > drivers/of/base.c:of_map_id() {
> >         ...
> >         map = of_get_property(np, map_name, &map_bytes);
> >         if (!map) {
> >                 if (filter_np)
> >                         return -ENODEV;
> >                 /* Otherwise, no map implies no translation */
> >                 arg->args[0] = id;
> >                 arg->args_count = 1;
> >                 return 0;
> >         }
> >         ...
> > }
> >
> > In of_msi_xlate(), this return value of 0 triggers a break:
> >
> > drivers/of/irq.c:of_msi_xlate() {
> >                 if (!of_map_msi_id(parent_dev->of_node, id_in, msi_np, &msi_spec)) {
> >                         ...
> >                         /* msi-map present but no match -> stop walking */
> >                         break;
> >                 }
> > }
> >
> > Because of_map_id() returns 0 when the map is absent, wouldn't this early
> > break erroneously abort the parent traversal loop on the first iteration,
> > breaking MSI ID mapping for callers passing a NULL msi_np?
> 
> It does break early there, but not for fsl-mc, the only NULL caller:
> AFAICT, an fsl_mc_device never gets an of_node, so the walk carries on
> up to the fsl-mc platform device, which is where fsl,qoriq-mc puts
> msi-map.
> 
> > [Severity: High]
> > This isn't a bug introduced by this patch, but is there a fwnode reference
> > leak on the error path in its_v5_pci_msi_prepare()?
> >
> > drivers/irqchip/irq-gic-its-msi-parent.c:its_v5_pci_msi_prepare() {
> >         rid = pci_msi_map_rid_ctlr_node(domain->parent, pdev, &msi_node);
> >         if (!msi_node)
> >                 return -ENODEV;
> >
> >         ret = its_translate_frame_address(msi_node, &pa);
> >         if (ret)
> >                 return -ENODEV;
> >
> >         fwnode_handle_put(msi_node);
> >         ...
> > }
> >
> > If its_translate_frame_address() fails, it returns -ENODEV without dropping
> > the acquired msi_node reference. Can this leak the fwnode_handle if the
> > frame address translation fails repeatedly during driver bind or unbind?
> 
> Yes, every failed translation leaks one. I have a patch written,
> moving the put above the error check. It goes with the irqchip fixes
> rather than here.

Yep.

Thanks,
Lorenzo