[PATCH] cpufreq: qcom-hw: Fix exposed stack contents

[PATCH] cpufreq: qcom-hw: Fix exposed stack contents

[Vladimir Zapolskiy]

On irq request it is improper to pass its designated name, which is
allocated on stack, because the irq name string is not copied, thus
there is a risk of leaking partial stack contents to userspace.

Fixes: 275157b367f4 ("cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support")
Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-hw.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index a2be0df7e174..b772d8ed9a77 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -375,7 +375,7 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
 {
 	struct qcom_cpufreq_data *data = policy->driver_data;
 	struct platform_device *pdev = cpufreq_get_driver_data();
-	char irq_name[15];
+	char *irq_name;
 	int ret;
 
 	/*
@@ -392,9 +392,11 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
 	mutex_init(&data->throttle_lock);
 	INIT_DEFERRABLE_WORK(&data->throttle_work, qcom_lmh_dcvs_poll);
 
-	snprintf(irq_name, sizeof(irq_name), "dcvsh-irq-%u", policy->cpu);
-	ret = request_threaded_irq(data->throttle_irq, NULL, qcom_lmh_dcvs_handle_irq,
-				   IRQF_ONESHOT, irq_name, data);
+	irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dcvsh-irq-%u",
+				  policy->cpu);
+	ret = devm_request_threaded_irq(&pdev->dev, data->throttle_irq, NULL,
+					qcom_lmh_dcvs_handle_irq, IRQF_ONESHOT,
+					irq_name, data);
 	if (ret) {
 		dev_err(&pdev->dev, "Error registering %s: %d\n", irq_name, ret);
 		return 0;
-- 
2.32.0

Re: [PATCH] cpufreq: qcom-hw: Fix exposed stack contents

[Bjorn Andersson]

On Wed 10 Nov 07:27 PST 2021, Vladimir Zapolskiy wrote:

> On irq request it is improper to pass its designated name, which is
> allocated on stack, because the irq name string is not copied, thus
> there is a risk of leaking partial stack contents to userspace.
> 
> Fixes: 275157b367f4 ("cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support")
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

This was already proposed and reviewed at:

https://lore.kernel.org/all/20210901084732.943248-1-ardb@kernel.org/

Could Ard's patch please be picked up by the maintainers, preferably
with above Fixes added, so we get this backported onto v5.15 stable...

> ---
>  drivers/cpufreq/qcom-cpufreq-hw.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index a2be0df7e174..b772d8ed9a77 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -375,7 +375,7 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
>  {
>  	struct qcom_cpufreq_data *data = policy->driver_data;
>  	struct platform_device *pdev = cpufreq_get_driver_data();
> -	char irq_name[15];
> +	char *irq_name;
>  	int ret;
>  
>  	/*
> @@ -392,9 +392,11 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
>  	mutex_init(&data->throttle_lock);
>  	INIT_DEFERRABLE_WORK(&data->throttle_work, qcom_lmh_dcvs_poll);
>  
> -	snprintf(irq_name, sizeof(irq_name), "dcvsh-irq-%u", policy->cpu);
> -	ret = request_threaded_irq(data->throttle_irq, NULL, qcom_lmh_dcvs_handle_irq,
> -				   IRQF_ONESHOT, irq_name, data);
> +	irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dcvsh-irq-%u",

qcom_cpufreq_hw_lmh_init() might be called multiple times through the
life cycle of the associated struct device, so I find Ard's solution
preferable.

Regards,
Bjorn

> +				  policy->cpu);
> +	ret = devm_request_threaded_irq(&pdev->dev, data->throttle_irq, NULL,
> +					qcom_lmh_dcvs_handle_irq, IRQF_ONESHOT,
> +					irq_name, data);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Error registering %s: %d\n", irq_name, ret);
>  		return 0;
> -- 
> 2.32.0
> 

Re: [PATCH] cpufreq: qcom-hw: Fix exposed stack contents

[Vladimir Zapolskiy]

Hi Bjorn,

On 11/10/21 5:40 PM, Bjorn Andersson wrote:
> On Wed 10 Nov 07:27 PST 2021, Vladimir Zapolskiy wrote:
> 
>> On irq request it is improper to pass its designated name, which is
>> allocated on stack, because the irq name string is not copied, thus
>> there is a risk of leaking partial stack contents to userspace.
>>
>> Fixes: 275157b367f4 ("cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support")
>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> 
> This was already proposed and reviewed at:
> 
> https://lore.kernel.org/all/20210901084732.943248-1-ardb@kernel.org/

thank you for pointing it out, missed the fix in my mailbox and have to
reinvent it after stumbling upon the problem.

> Could Ard's patch please be picked up by the maintainers, preferably
> with above Fixes added, so we get this backported onto v5.15 stable...
> 

Right, the bug is quite critical and needs backporting.

--
Best wishes,
Vladimir

Re: [PATCH] cpufreq: qcom-hw: Fix exposed stack contents

[Viresh Kumar]

On 10-11-21, 07:40, Bjorn Andersson wrote:
> On Wed 10 Nov 07:27 PST 2021, Vladimir Zapolskiy wrote:
> 
> > On irq request it is improper to pass its designated name, which is
> > allocated on stack, because the irq name string is not copied, thus
> > there is a risk of leaking partial stack contents to userspace.
> > 
> > Fixes: 275157b367f4 ("cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support")
> > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> 
> This was already proposed and reviewed at:
> 
> https://lore.kernel.org/all/20210901084732.943248-1-ardb@kernel.org/
> 
> Could Ard's patch please be picked up by the maintainers, preferably
> with above Fixes added, so we get this backported onto v5.15 stable...

It never reached any of the PM guys unfortunately.

Ard, can you please repost it again ?

-- 
viresh

Re: [PATCH] cpufreq: qcom-hw: Fix exposed stack contents

[Vladimir Zapolskiy]

Hi Viresh,

On 11/11/21 3:25 AM, Viresh Kumar wrote:
> On 10-11-21, 07:40, Bjorn Andersson wrote:
>> On Wed 10 Nov 07:27 PST 2021, Vladimir Zapolskiy wrote:
>>
>>> On irq request it is improper to pass its designated name, which is
>>> allocated on stack, because the irq name string is not copied, thus
>>> there is a risk of leaking partial stack contents to userspace.
>>>
>>> Fixes: 275157b367f4 ("cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support")
>>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>>
>> This was already proposed and reviewed at:
>>
>> https://lore.kernel.org/all/20210901084732.943248-1-ardb@kernel.org/
>>
>> Could Ard's patch please be picked up by the maintainers, preferably
>> with above Fixes added, so we get this backported onto v5.15 stable...
> 
> It never reached any of the PM guys unfortunately.
> 
> Ard, can you please repost it again ?
> 

I've resent the rebased change under a series of new fixes:

   https://lore.kernel.org/linux-pm/20211111154808.2024808-1-vladimir.zapolskiy@linaro.org/T/#m2bbf2c57686

--
Best wishes,
Vladimir