linux-kernel - Re: [PATCH 2/2] usb: dwc3: of_simple: don't call pm_runtime_set

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180528152030.GA3261@localhost>
Date:   Mon, 28 May 2018 17:41:48 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Roger Quadros <rogerq@...com>
Cc:     balbi@...nel.org, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] usb: dwc3: of_simple: don't call
 pm_runtime_set_active()

On Mon, May 28, 2018 at 05:36:14PM +0300, Roger Quadros wrote:
> Don't call pm_runtime_set_active() as it will prevent the device
> from being activated in the next pm_runtime_get_sync() call.
> 
> Also call pm_runtime_get_sync() before of_platform_populate().

This paragraph describes what you do, but not why do it.

> Signed-off-by: Roger Quadros <rogerq@...com>
> ---
>  drivers/usb/dwc3/dwc3-of-simple.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
> index e98d221..2cbb5c0 100644
> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_resetc_assert;
>  
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);

This breaks runtime pm as you now get a second round of clock enables
which are never balanced on runtime suspend (the clocks are first
enabled in dwc3_of_simple_clk_init() above and with your change again in
dwc3_of_simple_runtime_resume()).

On the other hand, we currently return from probe() with a positive RPM
count so perhaps the RPM callbacks can just be removed altogether (i.e.
unless some other entity drops that count at some point before
remove()).

>  	ret = of_platform_populate(np, NULL, NULL, dev);
>  	if (ret) {
>  		for (i = 0; i < simple->num_clocks; i++) {
> @@ -131,10 +134,6 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>  		goto err_resetc_assert;
>  	}
>  
> -	pm_runtime_set_active(dev);
> -	pm_runtime_enable(dev);
> -	pm_runtime_get_sync(dev);
> -
>  	return 0;
>  
>  err_resetc_assert:

Also note that there's currently a use-after-free in remove(), where
pm_runtime_put_sync() is called after the clocks have been put.
Something like the below (untested) patch should fix it.

Johan


>From 35c384c31010c344d403c26fc0a1dde0fd68ef4a Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@...nel.org>
Date: Mon, 28 May 2018 17:31:45 +0200
Subject: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove

The clocks have already been explicitly disabled and put as part of
remove() so the runtime suspend callback must not be run when balancing
the runtime PM usage count before returning.

Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer")
Signed-off-by: Johan Hovold <johan@...nel.org>
---
 drivers/usb/dwc3/dwc3-of-simple.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
index cb2ee96fd3e8..b9c869cd6585 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -165,8 +165,9 @@ static int dwc3_of_simple_remove(struct platform_device *pdev)
 
 	reset_control_put(simple->resets);
 
-	pm_runtime_put_sync(dev);
+	pm_runtime_put_noidle(dev);
 	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
 
 	return 0;
 }
-- 
2.17.0