lkml.org 
[lkml]   [2022]   [May]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [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

    \
     
     \ /
      Last update: 2022-05-13 08:07    [W:2.345 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site