linux-kernel - Re: [PATCH 1/5] clk: renesas: rzv2h: Fix use-after-free in MSTOP refcount handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAMuHMdX9goPKdSHuk0XuAPvYv7uz9Lbzcy=24RjxWpW38HamLQ@mail.gmail.com>
Date: Fri, 20 Dec 2024 10:51:45 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
Cc: Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, 
	linux-renesas-soc@...r.kernel.org, linux-clk@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>, 
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH 1/5] clk: renesas: rzv2h: Fix use-after-free in MSTOP
 refcount handling

Hi Prabhakar,

On Fri, Dec 20, 2024 at 10:37 AM Lad, Prabhakar
<prabhakar.csengg@...il.com> wrote:
> On Fri, Dec 20, 2024 at 8:42 AM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
> > On Fri, Dec 20, 2024 at 9:24 AM Lad, Prabhakar
> > <prabhakar.csengg@...il.com> wrote:
> > > On Thu, Dec 19, 2024 at 4:20 PM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
> > > > On Wed, Dec 18, 2024 at 3:20 PM Prabhakar <prabhakar.csengg@...il.com> wrote:
> > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> > > > >
> > > > > Avoid triggering a `refcount_t: addition on 0; use-after-free.` warning
> > > > > when registering a module clock with the same MSTOP configuration. The
> > > > > issue arises when a module clock is registered but not enabled, resulting
> > > > > in a `ref_cnt` of 0. Subsequent calls to `refcount_inc()` on such clocks
> > > > > cause the kernel to warn about use-after-free.
> > > > >
> > > > > [    0.113529] ------------[ cut here ]------------
> > > > > [    0.113537] refcount_t: addition on 0; use-after-free.
> > > > > [    0.113576] WARNING: CPU: 2 PID: 1 at lib/refcount.c:25 refcount_warn_saturate+0x120/0x144
> > > >
> > > > [...]
> > > >
> > > > > Resolve this by checking the `ref_cnt` value before calling
> > > > > `refcount_inc()`. If `ref_cnt` is 0, reset it to 1 using `refcount_set()`.
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > Fixes: 7bd4cb3d6b7c ("clk: renesas: rzv2h: Relocate MSTOP-related macros to the family driver")
> > > >
> > > > The description (from your [PATCH 2/5]?) does not match the commit.
> > > >
> > > Ouch!
> > >
> > > > Fixes: 7bd4cb3d6b7c43f0 ("clk: renesas: rzv2h: Add MSTOP support")
> > > >
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> > > >
> > > > > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > > > > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> > > > > @@ -565,8 +565,12 @@ static struct rzv2h_mstop
> > > > >                         continue;
> > > > >
> > > > >                 if (BUS_MSTOP(clk->mstop->idx, clk->mstop->mask) == mstop_data) {
> > > > > -                       if (rzv2h_mod_clock_is_enabled(&clock->hw))
> > > > > -                               refcount_inc(&clk->mstop->ref_cnt);
> > > > > +                       if (rzv2h_mod_clock_is_enabled(&clock->hw)) {
> > > > > +                               if (refcount_read(&clk->mstop->ref_cnt))
> > > > > +                                       refcount_inc(&clk->mstop->ref_cnt);
> > > > > +                               else
> > > > > +                                       refcount_set(&clk->mstop->ref_cnt, 1);
> > > > > +                       }
> >
> > Or simply
> >
> >     do refcount_set(&clk->mstop->ref_cnt,
> >                     refcount_read(&clk->mstop->ref_cnt) +1);
> >
> > ?
> >
> > Still, you risk some janitor replacing that by refcount_inc() regardless...
> >
> Agreed.
>
> > > > >                         return clk->mstop;
> > > > >                 }
> > > > >         }
> > > >
> > > > This makes me wonder if refcount is the right abstraction?
> > > >
> > > You mean as discussed on irc, refcount per mstop bit instead of groups
> > > is not OK too? Do you have any other better approach in mind?
> >
> > I mean if you need such silly workarounds to do a simple increment, is
> > refcount_t the right abstraction, instead of a plain atomic_t?
> >
> OK, I'll switch to the atomic_t variant. For this I will still rebase
> my work on [0]  along with atomic_t per mstop bit. Is that OK?
>
> [0] https://lore.kernel.org/all/CAMuHMdUEkN6Z7p=LspP+npB3xs4ui+D9oGG+Q15kQ-mYiaoK-A@mail.gmail.com/

That's fine. Once all issues are sorted out, I can still squash the fix into
the original commit, to avoid regressions while bisecting.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds