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

[Stephen Boyd <sboyd@kernel.org>]

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();