* [PATCH v1] clk: fix invalid usage of list_for_each_entry cursor
@ 2021-03-05 19:13 Lukasz Bartosik
2021-03-13 20:37 ` Stephen Boyd
2021-03-17 16:05 ` [PATCH v2] clk: fix invalid usage of a " Lukasz Bartosik
0 siblings, 2 replies; 9+ messages in thread
From: Lukasz Bartosik @ 2021-03-05 19:13 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd; +Cc: linux-clk, upstream
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v1] clk: fix invalid usage of list_for_each_entry cursor
2021-03-05 19:13 [PATCH v1] clk: fix invalid usage of list_for_each_entry cursor Lukasz Bartosik
@ 2021-03-13 20:37 ` Stephen Boyd
2021-03-15 10:37 ` Łukasz Bartosik
2021-03-17 16:05 ` [PATCH v2] clk: fix invalid usage of a " Lukasz Bartosik
1 sibling, 1 reply; 9+ messages in thread
From: Stephen Boyd @ 2021-03-13 20:37 UTC (permalink / raw)
To: Lukasz Bartosik, Michael Turquette; +Cc: linux-clk, upstream
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.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1] clk: fix invalid usage of list_for_each_entry cursor
2021-03-13 20:37 ` Stephen Boyd
@ 2021-03-15 10:37 ` Łukasz Bartosik
2021-03-17 10:48 ` Łukasz Bartosik
0 siblings, 1 reply; 9+ messages in thread
From: Łukasz Bartosik @ 2021-03-15 10:37 UTC (permalink / raw)
To: Stephen Boyd; +Cc: Michael Turquette, linux-clk, upstream
>
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1] clk: fix invalid usage of list_for_each_entry cursor
2021-03-15 10:37 ` Łukasz Bartosik
@ 2021-03-17 10:48 ` Łukasz Bartosik
2021-03-29 20:30 ` Stephen Boyd
0 siblings, 1 reply; 9+ messages in thread
From: Łukasz Bartosik @ 2021-03-17 10:48 UTC (permalink / raw)
To: Stephen Boyd; +Cc: Michael Turquette, linux-clk, upstream
>
> >
> > 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.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1] clk: fix invalid usage of list_for_each_entry cursor
2021-03-17 10:48 ` Łukasz Bartosik
@ 2021-03-29 20:30 ` Stephen Boyd
2021-03-31 15:57 ` Łukasz Bartosik
0 siblings, 1 reply; 9+ messages in thread
From: Stephen Boyd @ 2021-03-29 20:30 UTC (permalink / raw)
To: lb; +Cc: Michael Turquette, linux-clk, upstream
> > >
> > > 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++;
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v1] clk: fix invalid usage of list_for_each_entry cursor
2021-03-29 20:30 ` Stephen Boyd
@ 2021-03-31 15:57 ` Łukasz Bartosik
0 siblings, 0 replies; 9+ messages in thread
From: Łukasz Bartosik @ 2021-03-31 15:57 UTC (permalink / raw)
To: Stephen Boyd; +Cc: Michael Turquette, linux-clk, upstream
>
> > > >
> > > > 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++;
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] clk: fix invalid usage of a list_for_each_entry cursor
2021-03-05 19:13 [PATCH v1] clk: fix invalid usage of list_for_each_entry cursor Lukasz Bartosik
2021-03-13 20:37 ` Stephen Boyd
@ 2021-03-17 16:05 ` Lukasz Bartosik
2021-03-31 15:58 ` [PATCH v3] " Lukasz Bartosik
1 sibling, 1 reply; 9+ messages in thread
From: Lukasz Bartosik @ 2021-03-17 16:05 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd; +Cc: linux-clk, upstream
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v3] clk: fix invalid usage of a list_for_each_entry cursor
2021-03-17 16:05 ` [PATCH v2] clk: fix invalid usage of a " Lukasz Bartosik
@ 2021-03-31 15:58 ` Lukasz Bartosik
2021-03-31 16:46 ` Stephen Boyd
0 siblings, 1 reply; 9+ messages in thread
From: Lukasz Bartosik @ 2021-03-31 15:58 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd; +Cc: linux-clk, upstream
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3] clk: fix invalid usage of a list_for_each_entry cursor
2021-03-31 15:58 ` [PATCH v3] " Lukasz Bartosik
@ 2021-03-31 16:46 ` Stephen Boyd
0 siblings, 0 replies; 9+ messages in thread
From: Stephen Boyd @ 2021-03-31 16:46 UTC (permalink / raw)
To: Lukasz Bartosik, Michael Turquette; +Cc: linux-clk, upstream
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();
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-03-31 16:46 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-05 19:13 [PATCH v1] clk: fix invalid usage of list_for_each_entry cursor Lukasz Bartosik
2021-03-13 20:37 ` Stephen Boyd
2021-03-15 10:37 ` Łukasz Bartosik
2021-03-17 10:48 ` Łukasz Bartosik
2021-03-29 20:30 ` Stephen Boyd
2021-03-31 15:57 ` Łukasz Bartosik
2021-03-17 16:05 ` [PATCH v2] clk: fix invalid usage of a " Lukasz Bartosik
2021-03-31 15:58 ` [PATCH v3] " Lukasz Bartosik
2021-03-31 16:46 ` Stephen Boyd
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.