| Message ID | 20211110152745.1868099-1-vladimir.zapolskiy@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cpufreq: qcom-hw: Fix exposed stack contents | expand |
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 >
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
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 ?
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
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;
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(-)