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

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

[Fuad Tabba]

of_msi_xlate() documents @msi_np as a filter when it points at a node on
entry, handing back a reference only when it points at NULL. The msi-map
branch does that. of_check_msi_parent() keeps the reference from the 1:1
msi-parent match either way.

pci_msi_domain_get_msi_rid() is the only caller that passes a node in,
from irq_domain_get_of_node(), and it puts nothing, so a 1:1 match there
leaks the MSI controller node.

Fixes: 119aaeed0b67 ("of/irq: Add msi-parent check to of_msi_xlate()")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260826113604.010C01F000E9@smtp.kernel.org/
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 drivers/of/irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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;
-- 
2.39.5

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

[sashiko-bot]

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?

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

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904073424.3855365-1-fuad.tabba@linux.dev?part=1

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

[Fuad Tabba]

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?

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

/fuad

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260904073424.3855365-1-fuad.tabba@linux.dev?part=1

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

[Lorenzo Pieralisi]

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

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

[Fuad Tabba]

On Fri, 4 Sept 2026 at 12:11, Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
...
> > 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).

Thanks for the clarification.

Cheers,
/fuad

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