Re: [PATCH] of: property: Add missing of_node_get() in parse_interrupt()

[Rob Herring]

On Tue, Feb 28, 2023 at 3:58 PM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
>
> On Tue, Feb 28, 2023 at 1:01 PM Rob Herring <robh+dt@xxxxxxxxxx> wrote:
> >
> > On Tue, Feb 28, 2023 at 1:07 PM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Feb 28, 2023 at 9:40 AM Jean-Jacques Hiblot
> > > <jjhiblot@xxxxxxxxxxxxxxx> wrote:
> > > >
> > > > From: Jean Jacques Hiblot <jjhiblot@xxxxxxxxxxxxxxx>
> > > >
> > > > As all the other parsers do, parse_interrupt() must increase the refcount
> > > > of the device_node. Otherwise the refcount is decremented every time
> > > > parse_interrupt() is called on this node, leading to a potential
> > > > use-after-free.
> > > >
> > > > This is a regression introduced by commit f265f06af194 ("of: property:
> > > > Fix fw_devlink handling of interrupts/interrupts-extended"). The reason is
> > > > that of_irq_parse_one() does not increase the refcount while the previously
> > > > used of_irq_find_parent() does.
> > >
> > > Thanks for catching the issue Jean!
> > >
> > > This feels like a bug in of_irq_parse_one() to me. It's returning a
> > > reference to a node without doing a of_node_get() on it.
> > >
> > > Rob, Marc, Do you agree?
> >
> > I think you are right. If we look at the 'interrupts-extended' path,
> > it just calls of_parse_phandle_with_args() which does a get.
> >
> > > Jean,
> > >
> > > If they agree, can you please fix of_irq_parse_one() and add a
> > > of_node_put() to existing callers (if they aren't already doing a
> > > put()).
> >
> > I think it is not that simple. The correct thing for callers may also
> > be to hold the ref. We wouldn't want to just blindly do a put that is
> > clearly wrong just to keep current behavior.
>
> Right, I was just giving the approximate idea. If the caller keeps
> using the node pointer, they shouldn't do a put().
>
> > But not having the put
> > means we're leaking refcounts as calling the APIs originally had no
> > side effect. For example, IIRC, of_irq_get() is called again on each
> > deferred probe. There is no of_irq_put() because Linux IRQ numbers
> > aren't (or weren't?) refcounted.
>
> Hopefully fw_devlink will avoid a lot of these deferred probes. But if
> it comes to wasting memory (leaking) vs use after free, we should for
> the short term switch to leaking.

A refcount overflow can cause a use after free too, but I guess the
underlying kref protects against that now.

The issue is we have what was a non-refcounted API and we've halfway
bolted on refcounting for what's 99% static anyways. I really wish we
were only worrying about refcounts for the 1% of the cases that
matter.

> IRQ themselves can't be freed once they are registered with the IRQ
> framework, but I'd think the consumers can still do a get/put on an
> IRQ. So, at the least, we should be able to do some put() from the
> consumer context.
>
> > Really, I'd like to get rid of exposing of_irq_parse_one() in the first place.
>
> I don't have enough context to comment here.

Looking at the ~10 users, they are mostly cases wanting to get their
hwirq number. That then puts knowledge of the parent interrupt cell
format into the client which isn't great. There's also one case
(regulator-quirk-rcar-gen2.c) looking for shared interrupts. Probably
better ways to do both of those...

Rob