MIME-Version: 1.0
References: <20230228174019.4004581-1-jjhiblot@traphandler.com> <CAGETcx-w4A3fz_DPqJG+9P6ETGAPv547DcnsO52gqTO1_vijsw@mail.gmail.com>
In-Reply-To: <CAGETcx-w4A3fz_DPqJG+9P6ETGAPv547DcnsO52gqTO1_vijsw@mail.gmail.com>
From:   Rob Herring <robh+dt@kernel.org>
Date:   Tue, 28 Feb 2023 15:01:29 -0600
Message-ID: <CAL_JsqL6qFDC5uC_0PgyM_8OVEwFq+o=gPk2=PRKBDTp9XTzOw@mail.gmail.com>
Subject: Re: [PATCH] of: property: Add missing of_node_get() in parse_interrupt()
To:     Saravana Kannan <saravanak@google.com>
Cc:     jjhiblot@traphandler.com, frowand.list@gmail.com,
        gregkh@linuxfoundation.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, Marc Zyngier <maz@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Feb 28, 2023 at 1:07=E2=80=AFPM Saravana Kannan <saravanak@google.c=
om> wrote:
>
> On Tue, Feb 28, 2023 at 9:40=E2=80=AFAM Jean-Jacques Hiblot
> <jjhiblot@traphandler.com> wrote:
> >
> > From: Jean Jacques Hiblot <jjhiblot@traphandler.com>
> >
> > As all the other parsers do, parse_interrupt() must increase the refcou=
nt
> > 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 previo=
usly
> > 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. 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.

Really, I'd like to get rid of exposing of_irq_parse_one() in the first pla=
ce.

Rob