Messages in this thread Patch in this message | | | From | Schspa Shi <> | Date | Fri, 13 May 2022 14:06:29 +0800 | Subject | Re: [PATCH v4 1/2] cpufreq: fix race on cpufreq online |
| |
Viresh Kumar <viresh.kumar@linaro.org> writes:
> On 12-05-22, 21:52, Schspa Shi wrote: >> When cpufreq online failed, policy->cpus are not empty while >> cpufreq sysfs file available, we may access some data freed. >> >> Take policy->clk as an example: >> >> static int cpufreq_online(unsigned int cpu) >> { >> ... >> // policy->cpus != 0 at this time >> down_write(&policy->rwsem); >> ret = cpufreq_add_dev_interface(policy); >> up_write(&policy->rwsem); >> >> down_write(&policy->rwsem); >> ... >> /* cpufreq nitialization fails in some cases */ >> if (cpufreq_driver->get && has_target()) { >> policy->cur = cpufreq_driver->get(policy->cpu); >> if (!policy->cur) { >> ret = -EIO; >> pr_err("%s: ->get() failed\n", __func__); >> goto out_destroy_policy; >> } >> } >> ... >> up_write(&policy->rwsem); >> ... >> >> return 0; >> >> out_destroy_policy: >> for_each_cpu(j, policy->real_cpus) >> remove_cpu_dev_symlink(policy, get_cpu_device(j)); >> up_write(&policy->rwsem); >> ... >> out_exit_policy: >> if (cpufreq_driver->exit) >> cpufreq_driver->exit(policy); >> clk_put(policy->clk); >> // policy->clk is a wild pointer >> ... >> ^ >> | >> Another process access >> __cpufreq_get >> cpufreq_verify_current_freq >> cpufreq_generic_get >> // acces wild pointer of policy->clk; >> | >> | >> out_offline_policy: | >> cpufreq_policy_free(policy); | >> // deleted here, and will wait for no body reference >> cpufreq_policy_put_kobj(policy); >> } >> We can fix it by clear the policy->cpus mask. >> Both show_scaling_cur_freq and show_cpuinfo_cur_freq will return an >> error by checking this mask, thus avoiding UAF. > > Instead of all above, what about this. > > Subject: Abort show/store for half initialized policy > > If policy initialization fails after the sysfs files are created, > there is a possibility that we may end up running show()/store() > callbacks for half initialized policies, which may have unpredictable > outcomes. > > Abort show/store in such a case by making sure the policy is active. > Also inactivate the policy on such failures. >
Yes, I think it's OK, let me upload a new patch V5 for it ?
>> Signed-off-by: Schspa Shi <schspa@gmail.com> >> >> --- >> >> Changelog: >> v1 -> v2: >> - Fix bad critical region enlarge which causes uninitialized >> unlock. >> - Move cpumask_clear(policy->cpus); before out_offline_policy >> v2 -> v3: >> - Remove the missed down_write() before >> cpumask_and(policy->cpus, policy->cpus, cpu_online_mask); >> v3 -> v4: >> - Seprate to two patchs. >> - Add policy_is_inactive check before sysfs access >> --- >> drivers/cpufreq/cpufreq.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index 80f535cc8a75..35dffd738580 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -953,7 +953,10 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf) >> return -EIO; >> >> down_read(&policy->rwsem); >> - ret = fattr->show(policy, buf); >> + if (unlikely(policy_is_inactive(policy))) >> + ret = -EBUSY; >> + else >> + ret = fattr->show(policy, buf); > > I like it the way I have done earlier, initialize ret to -EBUSY and > get rid of the else part and call show/store in if itself. Same for > below. >
I add a unlikely here, to avoid branch prediction failed. And move the to the fail path to avoid a register assignment to -EBUSY.
I think it's better for performance.
You can verify this at the disassamble of show function.
(gdb) disassemble /s show Dump of assembler code for function show: 0x00000000000037b8 <+0>: paciasp 0x00000000000037bc <+4>: stp x29, x30, [sp, #-48]! 0x00000000000037c0 <+8>: ldr x8, [x1, #16] 0x00000000000037c4 <+12>: stp x22, x21, [sp, #16] 0x00000000000037c8 <+16>: stp x20, x19, [sp, #32] 0x00000000000037cc <+20>: mov x29, sp 0x00000000000037d0 <+24>: cbz x8, 0x3820 <show+104> 0x00000000000037d4 <+28>: sub x22, x0, #0x1b8 0x00000000000037d8 <+32>: add x19, x22, #0x218 0x00000000000037dc <+36>: mov x0, x19 0x00000000000037e0 <+40>: mov x20, x2 0x00000000000037e4 <+44>: mov x21, x1 0x00000000000037e8 <+48>: bl 0x37e8 <show+48> 0x00000000000037ec <+52>: mov w1, #0x100 // #256 0x00000000000037f0 <+56>: mov x0, x22 0x00000000000037f4 <+60>: bl 0x37f4 <show+60> 0x00000000000037f8 <+64>: cmp x0, #0x100 0x00000000000037fc <+68>: b.eq 0x383c <show+132> // b.none // use unlikely to avoid branch prediction failed 0x0000000000003800 <+72>: ldr x8, [x21, #16] 0x0000000000003804 <+76>: mov x0, x22 0x0000000000003808 <+80>: mov x1, x20 0x000000000000380c <+84>: blr x8 0x0000000000003810 <+88>: mov x20, x0 0x0000000000003814 <+92>: mov x0, x19 0x0000000000003818 <+96>: bl 0x3818 <show+96> 0x000000000000381c <+100>: b 0x3824 <show+108> 0x0000000000003820 <+104>: mov x20, #0xfffffffffffffffb // #-5 0x0000000000003824 <+108>: mov x0, x20 0x0000000000003828 <+112>: ldp x20, x19, [sp, #32] 0x000000000000382c <+116>: ldp x22, x21, [sp, #16] 0x0000000000003830 <+120>: ldp x29, x30, [sp], #48 0x0000000000003834 <+124>: autiasp 0x0000000000003838 <+128>: ret 0x000000000000383c <+132>: mov x20, #0xfffffffffffffff0 // #-16 // this is ret = -EBUSY, which will not be assigned under normal circumstances 0x0000000000003840 <+136>: b 0x3814 <show+92>
>> up_read(&policy->rwsem); >> >> return ret; >> @@ -978,7 +981,10 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr, >> >> if (cpu_online(policy->cpu)) { >> down_write(&policy->rwsem); >> - ret = fattr->store(policy, buf, count); >> + if (unlikely(policy_is_inactive(policy))) >> + ret = -EBUSY; >> + else >> + ret = fattr->store(policy, buf, count); >> up_write(&policy->rwsem); >> } >> >> @@ -1533,6 +1539,7 @@ static int cpufreq_online(unsigned int cpu) >> for_each_cpu(j, policy->real_cpus) >> remove_cpu_dev_symlink(policy, get_cpu_device(j)); >> >> + cpumask_clear(policy->cpus); >> up_write(&policy->rwsem); >> >> out_offline_policy: >> -- >> 2.29.0
-- Schspa Shi BRs
| |