Re: [PATCH v3] cpufreq/pasemi: fix an use-after-free in pas_cpufreq_cpu_init()
flat view
From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2019-07-09 04:47:08
Also in:
linux-pm, lkml
Subsystem:
cpu frequency scaling framework, linux for powerpc (32-bit and 64-bit), the rest · Maintainers:
"Rafael J. Wysocki", Viresh Kumar, Madhavan Srinivasan, Linus Torvalds
Wen Yang [off-list ref] writes:
quoted hunk ↗ jump to hunk
The cpu variable is still being used in the of_get_property() call after the of_node_put() call, which may result in use-after-free. Fixes: a9acc26b75f ("cpufreq/pasemi: fix possible object reference leak") Signed-off-by: Wen Yang <redacted> Cc: "Rafael J. Wysocki" <redacted> Cc: Viresh Kumar <viresh.kumar@linaro.org> Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-pm@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- v3: fix a leaked reference. v2: clean up the code according to the advice of viresh. drivers/cpufreq/pasemi-cpufreq.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)diff --git a/drivers/cpufreq/pasemi-cpufreq.c b/drivers/cpufreq/pasemi-cpufreq.c index 6b1e4ab..9dc5163 100644 --- a/drivers/cpufreq/pasemi-cpufreq.c +++ b/drivers/cpufreq/pasemi-cpufreq.c@@ -128,20 +128,20 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy) int cur_astate, idx; struct resource res; struct device_node *cpu, *dn; - int err = -ENODEV; + int err; cpu = of_get_cpu_node(policy->cpu, NULL); - - of_node_put(cpu); if (!cpu) - goto out; + return -ENODEV;
I don't think introducing another exit path is an improvement.
quoted hunk ↗ jump to hunk
dn = of_find_compatible_node(NULL, NULL, "1682m-sdc"); if (!dn) dn = of_find_compatible_node(NULL, NULL, "pasemi,pwrficient-sdc"); - if (!dn) + if (!dn) { + err = -ENODEV; goto out; + } err = of_address_to_resource(dn, 0, &res); of_node_put(dn); if (err)@@ -196,6 +196,7 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy) policy->cur = pas_freqs[cur_astate].frequency; ppc_proc_freq = policy->cur * 1000ul; + of_node_put(cpu); return cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency()); out_unmap_sdcpwr:@@ -204,6 +205,7 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy) out_unmap_sdcasr: iounmap(sdcasr_mapbase); out: + of_node_put(cpu); return err; }
Notice that cpu is only used for the max_freq calculation, so we could instead do:
diff --git a/drivers/cpufreq/pasemi-cpufreq.c b/drivers/cpufreq/pasemi-cpufreq.c
index 6b1e4abe3248..42a0a4b8e87d 100644
--- a/drivers/cpufreq/pasemi-cpufreq.c
+++ b/drivers/cpufreq/pasemi-cpufreq.c@@ -131,11 +131,20 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy) int err = -ENODEV; cpu = of_get_cpu_node(policy->cpu, NULL); - - of_node_put(cpu); if (!cpu) goto out; + max_freqp = of_get_property(cpu, "clock-frequency", NULL); + if (!max_freqp) { + of_node_put(cpu); + err = -EINVAL; + goto out; + } + + /* we need the freq in kHz */ + max_freq = *max_freqp / 1000; + of_node_put(cpu); + dn = of_find_compatible_node(NULL, NULL, "1682m-sdc"); if (!dn) dn = of_find_compatible_node(NULL, NULL,
@@ -172,15 +181,6 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy) pr_debug("init cpufreq on CPU %d\n", policy->cpu); - max_freqp = of_get_property(cpu, "clock-frequency", NULL); - if (!max_freqp) { - err = -EINVAL; - goto out_unmap_sdcpwr; - } - - /* we need the freq in kHz */ - max_freq = *max_freqp / 1000; - pr_debug("max clock-frequency is at %u kHz\n", max_freq); pr_debug("initializing frequency table\n");
Though arguably this function should hold a reference to cpu anyway, because it doesn't want the CPU to removed out from under it. It's a CPUfreq driver after all. cheers