Viresh Kumar <[email protected]> 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 <[email protected]>
>>
>> ---
>>
>> 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
On 13-05-22, 14:06, Schspa Shi wrote:
> Viresh Kumar <[email protected]> writes:
> > On 12-05-22, 21:52, Schspa Shi wrote:
> >> 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.
I am not asking you to drop it, I also added the unlikely within the
implementation of policy_is_inactive() then. It can be written as:
if (likely(!policy_is_inactive(policy)))
ret = fattr->show(policy, buf);
> And move the
> to the fail path to avoid a register assignment to -EBUSY.
We don't care about such assignments for performance to be honest.
This makes the code smaller by few lines, that's enough.
--
viresh