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

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

[Lukasz Bartosik]

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)

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;
@@ -4409,17 +4413,21 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb)
 {
 	struct clk_notifier *cn = NULL;
 	int ret = -EINVAL;
+	bool entry_found = false;
 
 	if (!clk || !nb)
 		return -EINVAL;
 
 	clk_prepare_lock();
 
-	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 (cn->clk == clk) {
+	if (entry_found) {
 		ret = srcu_notifier_chain_unregister(&cn->notifier_head, nb);
 
 		clk->core->notifier_count--;
-- 
2.17.1

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

[Stephen Boyd]

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"'

> 

And this newline shouldn't be here.

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

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

[Łukasz Bartosik]

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

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

[Łukasz Bartosik]

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

Although your comment was appealing to make the diff neat and small I
realized it won't work after making the changes.
Looking at the original code:
    /* search the list of notifiers for this clk */
    list_for_each_entry(cn, &clk_notifier_list, node)
        if (cn->clk == clk)
        break;

    /* if clk wasn't in the notifier list, allocate new clk_notifier */
    if (cn->clk != clk) {

The list cursor (cn) in the comparison above will not be pointing to a
valid entry not only when the clk_notifier_list is empty but also in
the case when clk_notifier_list list was completely traversed without
breaking from the list_for_each_entry loop on one of the entries.
Therefore making the comparison

    if (list_empty() || cn->clk != clk) {

will not help because if the list is not empty and there was no match
cn will not point to a valid entry. I have not noticed that before.
Based on that I propose to stay with my original fix.

[PATCH v2] clk: fix invalid usage of a list_for_each_entry cursor

[Lukasz Bartosik]

Fix invalid usage of a list_for_each_entry cursor. When
list is empty or if the list is completely traversed (without
breaking from the loop on one of the entries) then the 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")
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 8c1ed844b97e..d41dfbcfeba0 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4336,6 +4336,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;
@@ -4343,12 +4344,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;
@@ -4385,17 +4389,21 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb)
 {
 	struct clk_notifier *cn = NULL;
 	int ret = -EINVAL;
+	bool entry_found = false;
 
 	if (!clk || !nb)
 		return -EINVAL;
 
 	clk_prepare_lock();
 
-	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 (cn->clk == clk) {
+	if (entry_found) {
 		ret = srcu_notifier_chain_unregister(&cn->notifier_head, nb);
 
 		clk->core->notifier_count--;
-- 
2.17.1

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

[Stephen Boyd]

> > >
> > > 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
> 
> Although your comment was appealing to make the diff neat and small I
> realized it won't work after making the changes.
> Looking at the original code:
>     /* search the list of notifiers for this clk */
>     list_for_each_entry(cn, &clk_notifier_list, node)
>         if (cn->clk == clk)
>         break;
> 
>     /* if clk wasn't in the notifier list, allocate new clk_notifier */
>     if (cn->clk != clk) {
> 
> The list cursor (cn) in the comparison above will not be pointing to a
> valid entry not only when the clk_notifier_list is empty but also in
> the case when clk_notifier_list list was completely traversed without
> breaking from the list_for_each_entry loop on one of the entries.
> Therefore making the comparison
> 
>     if (list_empty() || cn->clk != clk) {
> 
> will not help because if the list is not empty and there was no match
> cn will not point to a valid entry. I have not noticed that before.
> Based on that I propose to stay with my original fix.

Ok, so then use 'goto found' approach?

----8<----
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 5052541a0986..16634d5912be 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4357,20 +4357,19 @@ int clk_notifier_register(struct clk *clk, struct notifier_block *nb)
 	/* search the list of notifiers for this clk */
 	list_for_each_entry(cn, &clk_notifier_list, node)
 		if (cn->clk == clk)
-			break;
+			goto found;
 
 	/* if clk wasn't in the notifier list, allocate new clk_notifier */
-	if (cn->clk != clk) {
-		cn = kzalloc(sizeof(*cn), GFP_KERNEL);
-		if (!cn)
-			goto out;
+	cn = kzalloc(sizeof(*cn), GFP_KERNEL);
+	if (!cn)
+		goto out;
 
-		cn->clk = clk;
-		srcu_init_notifier_head(&cn->notifier_head);
+	cn->clk = clk;
+	srcu_init_notifier_head(&cn->notifier_head);
 
-		list_add(&cn->node, &clk_notifier_list);
-	}
+	list_add(&cn->node, &clk_notifier_list);
 
+found:
 	ret = srcu_notifier_chain_register(&cn->notifier_head, nb);
 
 	clk->core->notifier_count++;

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

[Łukasz Bartosik]

>
> > > >
> > > > 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
> >
> > Although your comment was appealing to make the diff neat and small I
> > realized it won't work after making the changes.
> > Looking at the original code:
> >     /* search the list of notifiers for this clk */
> >     list_for_each_entry(cn, &clk_notifier_list, node)
> >         if (cn->clk == clk)
> >         break;
> >
> >     /* if clk wasn't in the notifier list, allocate new clk_notifier */
> >     if (cn->clk != clk) {
> >
> > The list cursor (cn) in the comparison above will not be pointing to a
> > valid entry not only when the clk_notifier_list is empty but also in
> > the case when clk_notifier_list list was completely traversed without
> > breaking from the list_for_each_entry loop on one of the entries.
> > Therefore making the comparison
> >
> >     if (list_empty() || cn->clk != clk) {
> >
> > will not help because if the list is not empty and there was no match
> > cn will not point to a valid entry. I have not noticed that before.
> > Based on that I propose to stay with my original fix.
>
> Ok, so then use 'goto found' approach?
>

I will use goto approach and send v3 patch.

> ----8<----
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 5052541a0986..16634d5912be 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -4357,20 +4357,19 @@ int clk_notifier_register(struct clk *clk, struct notifier_block *nb)
>         /* search the list of notifiers for this clk */
>         list_for_each_entry(cn, &clk_notifier_list, node)
>                 if (cn->clk == clk)
> -                       break;
> +                       goto found;
>
>         /* if clk wasn't in the notifier list, allocate new clk_notifier */
> -       if (cn->clk != clk) {
> -               cn = kzalloc(sizeof(*cn), GFP_KERNEL);
> -               if (!cn)
> -                       goto out;
> +       cn = kzalloc(sizeof(*cn), GFP_KERNEL);
> +       if (!cn)
> +               goto out;
>
> -               cn->clk = clk;
> -               srcu_init_notifier_head(&cn->notifier_head);
> +       cn->clk = clk;
> +       srcu_init_notifier_head(&cn->notifier_head);
>
> -               list_add(&cn->node, &clk_notifier_list);
> -       }
> +       list_add(&cn->node, &clk_notifier_list);
>
> +found:
>         ret = srcu_notifier_chain_register(&cn->notifier_head, nb);
>
>         clk->core->notifier_count++;

[PATCH v3] clk: fix invalid usage of a list_for_each_entry cursor

[Lukasz Bartosik]

Fix invalid usage of a list_for_each_entry cursor. When
list is empty or if the list is completely traversed (without
breaking from the loop on one of the entries) then the 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")
Reported-by: Lukasz Majczak <lma@semihalf.com>
Signed-off-by: Lukasz Bartosik <lb@semihalf.com>
---
 drivers/clk/clk.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d181c6d31d22..742cb8611ad4 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4345,20 +4345,19 @@ int clk_notifier_register(struct clk *clk, struct notifier_block *nb)
 	/* search the list of notifiers for this clk */
 	list_for_each_entry(cn, &clk_notifier_list, node)
 		if (cn->clk == clk)
-			break;
+			goto found;
 
 	/* if clk wasn't in the notifier list, allocate new clk_notifier */
-	if (cn->clk != clk) {
-		cn = kzalloc(sizeof(*cn), GFP_KERNEL);
-		if (!cn)
-			goto out;
+	cn = kzalloc(sizeof(*cn), GFP_KERNEL);
+	if (!cn)
+		goto out;
 
-		cn->clk = clk;
-		srcu_init_notifier_head(&cn->notifier_head);
+	cn->clk = clk;
+	srcu_init_notifier_head(&cn->notifier_head);
 
-		list_add(&cn->node, &clk_notifier_list);
-	}
+	list_add(&cn->node, &clk_notifier_list);
 
+found:
 	ret = srcu_notifier_chain_register(&cn->notifier_head, nb);
 
 	clk->core->notifier_count++;
@@ -4393,24 +4392,24 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb)
 
 	list_for_each_entry(cn, &clk_notifier_list, node)
 		if (cn->clk == clk)
-			break;
+			goto found;
 
-	if (cn->clk == clk) {
-		ret = srcu_notifier_chain_unregister(&cn->notifier_head, nb);
+	ret = -ENOENT;
+	goto out;
 
-		clk->core->notifier_count--;
+found:
+	ret = srcu_notifier_chain_unregister(&cn->notifier_head, nb);
 
-		/* XXX the notifier code should handle this better */
-		if (!cn->notifier_head.head) {
-			srcu_cleanup_notifier_head(&cn->notifier_head);
-			list_del(&cn->node);
-			kfree(cn);
-		}
+	clk->core->notifier_count--;
 
-	} else {
-		ret = -ENOENT;
+	/* XXX the notifier code should handle this better */
+	if (!cn->notifier_head.head) {
+		srcu_cleanup_notifier_head(&cn->notifier_head);
+		list_del(&cn->node);
+		kfree(cn);
 	}
 
+out:
 	clk_prepare_unlock();
 
 	return ret;
-- 
2.17.1

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

[Stephen Boyd]

Quoting Lukasz Bartosik (2021-03-31 08:58:22)
> Fix invalid usage of a list_for_each_entry cursor. When
> list is empty or if the list is completely traversed (without
> breaking from the loop on one of the entries) then the 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")
> Reported-by: Lukasz Majczak <lma@semihalf.com>
> Signed-off-by: Lukasz Bartosik <lb@semihalf.com>
> ---

Please stop sending these as replies to previous versions.

>  drivers/clk/clk.c | 41 ++++++++++++++++++++---------------------
>  1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index d181c6d31d22..742cb8611ad4 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -4345,20 +4345,19 @@ int clk_notifier_register(struct clk *clk, struct notifier_block *nb)
>         /* search the list of notifiers for this clk */
>         list_for_each_entry(cn, &clk_notifier_list, node)
>                 if (cn->clk == clk)
> -                       break;
> +                       goto found;
>  
>         /* if clk wasn't in the notifier list, allocate new clk_notifier */
> -       if (cn->clk != clk) {
> -               cn = kzalloc(sizeof(*cn), GFP_KERNEL);
> -               if (!cn)
> -                       goto out;
> +       cn = kzalloc(sizeof(*cn), GFP_KERNEL);
> +       if (!cn)
> +               goto out;
>  
> -               cn->clk = clk;
> -               srcu_init_notifier_head(&cn->notifier_head);
> +       cn->clk = clk;
> +       srcu_init_notifier_head(&cn->notifier_head);
>  
> -               list_add(&cn->node, &clk_notifier_list);
> -       }
> +       list_add(&cn->node, &clk_notifier_list);
>  
> +found:
>         ret = srcu_notifier_chain_register(&cn->notifier_head, nb);
>  
>         clk->core->notifier_count++;

This part looks good.

> @@ -4393,24 +4392,24 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb)
>  
>         list_for_each_entry(cn, &clk_notifier_list, node)
>                 if (cn->clk == clk)
> -                       break;
> +                       goto found;
>  
> -       if (cn->clk == clk) {
> -               ret = srcu_notifier_chain_unregister(&cn->notifier_head, nb);
> +       ret = -ENOENT;
> +       goto out;
>  
> -               clk->core->notifier_count--;
> +found:
> +       ret = srcu_notifier_chain_unregister(&cn->notifier_head, nb);

But then this part looks awful!

>  
> -               /* XXX the notifier code should handle this better */
> -               if (!cn->notifier_head.head) {
> -                       srcu_cleanup_notifier_head(&cn->notifier_head);
> -                       list_del(&cn->node);
> -                       kfree(cn);
> -               }
> +       clk->core->notifier_count--;
>  
> -       } else {
> -               ret = -ENOENT;
> +       /* XXX the notifier code should handle this better */
> +       if (!cn->notifier_head.head) {
> +               srcu_cleanup_notifier_head(&cn->notifier_head);
> +               list_del(&cn->node);
> +               kfree(cn);
>         }
>  
> +out:
>         clk_prepare_unlock();
>  
>         return ret;

Can we do this? Also, please split this part to a different patch that
we don't need to send back to stable trees. Presumably drivers aren't
calling the unregister function on an empty list or one that doesn't
contain the clk they're interested in because something should be
registered already by the register function.

Of course, the kernel-doc comment is also incorrect.  We need to get rid
of this notifier code. Sigh.

---8<---

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 5052541a0986..f73b8bbe7ec3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4395,32 +4395,28 @@ EXPORT_SYMBOL_GPL(clk_notifier_register);
  */
 int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb)
 {
-	struct clk_notifier *cn = NULL;
-	int ret = -EINVAL;
+	struct clk_notifier *cn;
+	int ret = -ENOENT;
 
 	if (!clk || !nb)
 		return -EINVAL;
 
 	clk_prepare_lock();
 
-	list_for_each_entry(cn, &clk_notifier_list, node)
-		if (cn->clk == clk)
-			break;
-
-	if (cn->clk == clk) {
-		ret = srcu_notifier_chain_unregister(&cn->notifier_head, nb);
+	list_for_each_entry(cn, &clk_notifier_list, node) {
+		if (cn->clk == clk) {
+			ret = srcu_notifier_chain_unregister(&cn->notifier_head, nb);
 
-		clk->core->notifier_count--;
+			clk->core->notifier_count--;
 
-		/* XXX the notifier code should handle this better */
-		if (!cn->notifier_head.head) {
-			srcu_cleanup_notifier_head(&cn->notifier_head);
-			list_del(&cn->node);
-			kfree(cn);
+			/* XXX the notifier code should handle this better */
+			if (!cn->notifier_head.head) {
+				srcu_cleanup_notifier_head(&cn->notifier_head);
+				list_del(&cn->node);
+				kfree(cn);
+			}
+			break;
 		}
-
-	} else {
-		ret = -ENOENT;
 	}
 
 	clk_prepare_unlock();