Re: [PATCH v1] clk: fix invalid usage of list_for_each_entry cursor

["Łukasz Bartosik" <lb@semihalf.com>]

>
> Quoting Lukasz Bartosik (2021-03-05 11:13:07)
> > Fix invalid usage of list_for_each_entry cursor. When list
> > is empty then list cursor does not point to a valid entry
> > and therefore should not be used.
> >
> > The issue was dicovered when running 5.12-rc1 kernel on x86_64
> > with KASAN enabled:
> > BUG: KASAN: global-out-of-bounds in clk_notifier_register+0xab/0x230
> > Read of size 8 at addr ffffffffa0d10588 by task swapper/0/1
> >
> > CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc1 #1
> > Hardware name: Google Caroline/Caroline,
> > BIOS Google_Caroline.7820.430.0 07/20/2018
> > Call Trace:
> >  dump_stack+0xee/0x15c
> >  print_address_description+0x1e/0x2dc
> >  kasan_report+0x188/0x1ce
> >  ? clk_notifier_register+0xab/0x230
> >  ? clk_prepare_lock+0x15/0x7b
> >  ? clk_notifier_register+0xab/0x230
> >  clk_notifier_register+0xab/0x230
> >  dw8250_probe+0xc01/0x10d4
> > ...
> > Memory state around the buggy address:
> >  ffffffffa0d10480: 00 00 00 00 00 03 f9 f9 f9 f9 f9 f9 00 00 00 00
> >  ffffffffa0d10500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f9 f9
> > >ffffffffa0d10580: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00
> >                       ^
> >  ffffffffa0d10600: 00 00 00 00 00 00 f9 f9 f9 f9 f9 f9 00 00 00 00
> >  ffffffffa0d10680: 00 00 00 00 00 00 00 00 f9 f9 f9 f9 00 00 00 00
> >  ==================================================================
> >
> > Fixes: (b2476490ef11 clk: introduce the common clock framework)
>
> This fixes format is wrong. Use pretty format of 'Fixes: %h "%s"'
>

Just to clarify. Shouldn't the format be 'Fixes: %h (\"%s\")' as
described in https://www.kernel.org/doc/html/v5.11/process/submitting-patches.html
?

> >
>
> And this newline shouldn't be here.
>

I will remove the newline.

> > Reported-by: Lukasz Majczak <lma@semihalf.com>
> > Signed-off-by: Lukasz Bartosik <lb@semihalf.com>
> > ---
> >  drivers/clk/clk.c | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 3412a7cc03fd..bd90de885392 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -4360,6 +4360,7 @@ int clk_notifier_register(struct clk *clk, struct notifier_block *nb)
> >  {
> >         struct clk_notifier *cn;
> >         int ret = -ENOMEM;
> > +       bool entry_found = false;
> >
> >         if (!clk || !nb)
> >                 return -EINVAL;
> > @@ -4367,12 +4368,15 @@ int clk_notifier_register(struct clk *clk, struct notifier_block *nb)
> >         clk_prepare_lock();
> >
> >         /* search the list of notifiers for this clk */
> > -       list_for_each_entry(cn, &clk_notifier_list, node)
> > -               if (cn->clk == clk)
> > +       list_for_each_entry(cn, &clk_notifier_list, node) {
> > +               if (cn->clk == clk) {
> > +                       entry_found = true;
> >                         break;
> > +               }
> > +       }
> >
> >         /* if clk wasn't in the notifier list, allocate new clk_notifier */
> > -       if (cn->clk != clk) {
> > +       if (!entry_found) {
> >                 cn = kzalloc(sizeof(*cn), GFP_KERNEL);
> >                 if (!cn)
> >                         goto out;
>
> How about using list_empty()?
>
>         if (list_empty() || cn->clk != clk)
>
> Then the diff is very small.

Good point. I will use list_empty(). Thanks