[PATCH 1/2] cpufreq/powernv: Fix use-after-free

[PATCH 1/2] cpufreq/powernv: Fix use-after-free

[Oliver O'Halloran]

The cpufreq driver has a use-after-free that we can hit if:

a) There's an OCC message pending when the notifier is registered, and
b) The cpufreq driver fails to register with the core.

When a) occurs the notifier schedules a workqueue item to handle the
message. The backing work_struct is located on chips[].throttle and when b)
happens we clean up by freeing the array. Once we get to the (now free)
queued item and the kernel crashes.

Cc: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
Fixes: c5e29ea ("cpufreq: powernv: Fix bugs in powernv_cpufreq_{init/exit}")
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 drivers/cpufreq/powernv-cpufreq.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index 56f4bc0..1806b1d 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -1080,6 +1080,12 @@ static int init_chip_info(void)
 
 static inline void clean_chip_info(void)
 {
+	int i;
+
+	/* flush any pending work items */
+	if (chips)
+		for (i = 0; i < nr_chips; i++)
+			cancel_work_sync(&chips[i].throttle);
 	kfree(chips);
 }
 
-- 
2.9.5

[PATCH 2/2] cpufreq/powernv: Fix unsafe notifiers

[Oliver O'Halloran]

The PowerNV cpufreq driver registers two notifiers: one to catch throttle
messages from the OCC and one to bump the CPU frequency back to normal
before a reboot. Both require the cpufreq driver to be registered in order
to function since the notifier callbacks use various cpufreq_*() functions.

Right now we register both notifiers before we've initialised the driver.
This seems to work, but we should head off any protential problems by
registering the notifiers after the driver is initialised.

Cc: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 drivers/cpufreq/powernv-cpufreq.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index 1806b1d..03798c4 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -1114,9 +1114,6 @@ static int __init powernv_cpufreq_init(void)
 	if (rc)
 		goto out;
 
-	register_reboot_notifier(&powernv_cpufreq_reboot_nb);
-	opal_message_notifier_register(OPAL_MSG_OCC, &powernv_cpufreq_opal_nb);
-
 	if (powernv_pstate_info.wof_enabled)
 		powernv_cpufreq_driver.boost_enabled = true;
 	else
@@ -1125,15 +1122,17 @@ static int __init powernv_cpufreq_init(void)
 	rc = cpufreq_register_driver(&powernv_cpufreq_driver);
 	if (rc) {
 		pr_info("Failed to register the cpufreq driver (%d)\n", rc);
-		goto cleanup_notifiers;
+		goto cleanup;
 	}
 
 	if (powernv_pstate_info.wof_enabled)
 		cpufreq_enable_boost_support();
 
+	register_reboot_notifier(&powernv_cpufreq_reboot_nb);
+	opal_message_notifier_register(OPAL_MSG_OCC, &powernv_cpufreq_opal_nb);
+
 	return 0;
-cleanup_notifiers:
-	unregister_all_notifiers();
+cleanup:
 	clean_chip_info();
 out:
 	pr_info("Platform driver disabled. System does not support PState control\n");
-- 
2.9.5

Re: [PATCH 1/2] cpufreq/powernv: Fix use-after-free

[Gautham R Shenoy]

On Thu, Feb 06, 2020 at 05:26:21PM +1100, Oliver O'Halloran wrote:
> The cpufreq driver has a use-after-free that we can hit if:
> 
> a) There's an OCC message pending when the notifier is registered, and
> b) The cpufreq driver fails to register with the core.
> 
> When a) occurs the notifier schedules a workqueue item to handle the
> message. The backing work_struct is located on chips[].throttle and when b)
> happens we clean up by freeing the array. Once we get to the (now free)
> queued item and the kernel crashes.
> 
> Cc: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
> Fixes: c5e29ea ("cpufreq: powernv: Fix bugs in powernv_cpufreq_{init/exit}")
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

Thanks for this fix Oliver.

Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>

> ---
>  drivers/cpufreq/powernv-cpufreq.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> index 56f4bc0..1806b1d 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -1080,6 +1080,12 @@ static int init_chip_info(void)
> 
>  static inline void clean_chip_info(void)
>  {
> +	int i;
> +
> +	/* flush any pending work items */
> +	if (chips)
> +		for (i = 0; i < nr_chips; i++)
> +			cancel_work_sync(&chips[i].throttle);
>  	kfree(chips);
>  }
> 
> -- 
> 2.9.5
> 

Re: [PATCH 2/2] cpufreq/powernv: Fix unsafe notifiers

[Gautham R Shenoy]

On Thu, Feb 06, 2020 at 05:26:22PM +1100, Oliver O'Halloran wrote:
> The PowerNV cpufreq driver registers two notifiers: one to catch throttle
> messages from the OCC and one to bump the CPU frequency back to normal
> before a reboot. Both require the cpufreq driver to be registered in order
> to function since the notifier callbacks use various cpufreq_*() functions.
> 
> Right now we register both notifiers before we've initialised the driver.
> This seems to work, but we should head off any protential problems by
> registering the notifiers after the driver is initialised.
> 
> Cc: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>

> ---
>  drivers/cpufreq/powernv-cpufreq.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> index 1806b1d..03798c4 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -1114,9 +1114,6 @@ static int __init powernv_cpufreq_init(void)
>  	if (rc)
>  		goto out;
> 
> -	register_reboot_notifier(&powernv_cpufreq_reboot_nb);
> -	opal_message_notifier_register(OPAL_MSG_OCC, &powernv_cpufreq_opal_nb);
> -
>  	if (powernv_pstate_info.wof_enabled)
>  		powernv_cpufreq_driver.boost_enabled = true;
>  	else
> @@ -1125,15 +1122,17 @@ static int __init powernv_cpufreq_init(void)
>  	rc = cpufreq_register_driver(&powernv_cpufreq_driver);
>  	if (rc) {
>  		pr_info("Failed to register the cpufreq driver (%d)\n", rc);
> -		goto cleanup_notifiers;
> +		goto cleanup;
>  	}
> 
>  	if (powernv_pstate_info.wof_enabled)
>  		cpufreq_enable_boost_support();
> 
> +	register_reboot_notifier(&powernv_cpufreq_reboot_nb);
> +	opal_message_notifier_register(OPAL_MSG_OCC, &powernv_cpufreq_opal_nb);
> +
>  	return 0;
> -cleanup_notifiers:
> -	unregister_all_notifiers();
> +cleanup:
>  	clean_chip_info();
>  out:
>  	pr_info("Platform driver disabled. System does not support PState control\n");
> -- 
> 2.9.5
> 

Re: [PATCH 1/2] cpufreq/powernv: Fix use-after-free

[Andrew Donnellan]

On 6/2/20 5:26 pm, Oliver O'Halloran wrote:
> The cpufreq driver has a use-after-free that we can hit if:
> 
> a) There's an OCC message pending when the notifier is registered, and
> b) The cpufreq driver fails to register with the core.
> 
> When a) occurs the notifier schedules a workqueue item to handle the
> message. The backing work_struct is located on chips[].throttle and when b)
> happens we clean up by freeing the array. Once we get to the (now free)
> queued item and the kernel crashes.
> 
> Cc: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
> Fixes: c5e29ea ("cpufreq: powernv: Fix bugs in powernv_cpufreq_{init/exit}")
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

This sounds like it needs to go to stable.


-- 
Andrew Donnellan              OzLabs, ADL Canberra
ajd@linux.ibm.com             IBM Australia Limited

Re: [PATCH 1/2] cpufreq/powernv: Fix use-after-free

[Michael Ellerman]

Andrew Donnellan <ajd@linux.ibm.com> writes:
> On 6/2/20 5:26 pm, Oliver O'Halloran wrote:
>> The cpufreq driver has a use-after-free that we can hit if:
>> 
>> a) There's an OCC message pending when the notifier is registered, and
>> b) The cpufreq driver fails to register with the core.
>> 
>> When a) occurs the notifier schedules a workqueue item to handle the
>> message. The backing work_struct is located on chips[].throttle and when b)
>> happens we clean up by freeing the array. Once we get to the (now free)
>> queued item and the kernel crashes.
>> 
>> Cc: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
>> Fixes: c5e29ea ("cpufreq: powernv: Fix bugs in powernv_cpufreq_{init/exit}")
>> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
>
> This sounds like it needs to go to stable.

I tagged it for stable when applying.

cheers

Re: [PATCH 1/2] cpufreq/powernv: Fix use-after-free

[Michael Ellerman]

On Thu, 2020-02-06 at 06:26:21 UTC, Oliver O'Halloran wrote:
> The cpufreq driver has a use-after-free that we can hit if:
> 
> a) There's an OCC message pending when the notifier is registered, and
> b) The cpufreq driver fails to register with the core.
> 
> When a) occurs the notifier schedules a workqueue item to handle the
> message. The backing work_struct is located on chips[].throttle and when b)
> happens we clean up by freeing the array. Once we get to the (now free)
> queued item and the kernel crashes.
> 
> Cc: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
> Fixes: c5e29ea ("cpufreq: powernv: Fix bugs in powernv_cpufreq_{init/exit}")
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/d0a72efac89d1c35ac55197895201b7b94c5e6ef

cheers