| Message ID | 1578303861-7217-1-git-send-email-qiwuchen55@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2] cpufreq: s3c: avoid use after free issue in xxx_cpufreq_reboot_notifier_evt() | expand |
On 06-01-20, 17:44, qiwuchen55@gmail.com wrote: > From: chenqiwu <chenqiwu@xiaomi.com> > > There is a potential UAF issue in xxx_cpufreq_reboot_notifier_evt() that > the cpufreq policy of cpu0 has been released before using it. So we should > make a judgement to avoid it. Again, the subject and description are incorrect here. This isn't a user after free problem as we were already calling cpufreq_cpu_get(). The problem was that the balancing of refcount wasn't done properly. > Signed-off-by: chenqiwu <chenqiwu@xiaomi.com> > --- > changes in v2: > - use the combination of cpufreq_cpu_get() and cpufreq_cpu_put() > instead of cpufreq_get_policy() in s3c2416-cpufreq.c > --- > drivers/cpufreq/s3c2416-cpufreq.c | 12 +++++++++++- > drivers/cpufreq/s5pv210-cpufreq.c | 11 ++++++++++- > 2 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpufreq/s3c2416-cpufreq.c b/drivers/cpufreq/s3c2416-cpufreq.c > index 1069103..f07c5d1 100644 > --- a/drivers/cpufreq/s3c2416-cpufreq.c > +++ b/drivers/cpufreq/s3c2416-cpufreq.c > @@ -304,6 +304,7 @@ static int s3c2416_cpufreq_reboot_notifier_evt(struct notifier_block *this, > { > struct s3c2416_data *s3c_freq = &s3c2416_cpufreq; > int ret; > + struct cpufreq_policy *policy; > > mutex_lock(&cpufreq_lock); > > @@ -318,7 +319,16 @@ static int s3c2416_cpufreq_reboot_notifier_evt(struct notifier_block *this, > */ > if (s3c_freq->is_dvs) { > pr_debug("cpufreq: leave dvs on reboot\n"); > - ret = cpufreq_driver_target(cpufreq_cpu_get(0), FREQ_SLEEP, 0); > + > + policy = cpufreq_cpu_get(0); > + if (!policy) { > + pr_debug("cpufreq: get no policy for cpu0\n"); > + return NOTIFY_BAD; > + } > + > + ret = cpufreq_driver_target(&policy, FREQ_SLEEP, 0); > + cpufreq_cpu_put(policy); > + > if (ret < 0) > return NOTIFY_BAD; > } > diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c > index 5d10030..e84281e 100644 > --- a/drivers/cpufreq/s5pv210-cpufreq.c > +++ b/drivers/cpufreq/s5pv210-cpufreq.c > @@ -555,8 +555,17 @@ static int s5pv210_cpufreq_reboot_notifier_event(struct notifier_block *this, > unsigned long event, void *ptr) > { > int ret; > + struct cpufreq_policy *policy; > + > + policy = cpufreq_cpu_get(0); > + if (!policy) { > + pr_debug("cpufreq: get no policy for cpu0\n"); > + return NOTIFY_BAD; > + } > + > + ret = cpufreq_driver_target(policy, SLEEP_FREQ, 0); > + cpufreq_cpu_put(policy); > > - ret = cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ, 0); > if (ret < 0) > return NOTIFY_BAD; > > -- > 1.9.1
On Tue, Jan 07, 2020 at 11:01:47AM +0530, Viresh Kumar wrote: > On 06-01-20, 17:44, qiwuchen55@gmail.com wrote: > > From: chenqiwu <chenqiwu@xiaomi.com> > > > > There is a potential UAF issue in xxx_cpufreq_reboot_notifier_evt() that > > the cpufreq policy of cpu0 has been released before using it. So we should > > make a judgement to avoid it. > > Again, the subject and description are incorrect here. This isn't a user after > free problem as we were already calling cpufreq_cpu_get(). The problem was that > the balancing of refcount wasn't done properly. > > Yeah, I will rewrite the title and commit message, and resend this as patch v3. Thanks! > > Signed-off-by: chenqiwu <chenqiwu@xiaomi.com> > > --- > > changes in v2: > > - use the combination of cpufreq_cpu_get() and cpufreq_cpu_put() > > instead of cpufreq_get_policy() in s3c2416-cpufreq.c > > --- > > drivers/cpufreq/s3c2416-cpufreq.c | 12 +++++++++++- > > drivers/cpufreq/s5pv210-cpufreq.c | 11 ++++++++++- > > 2 files changed, 21 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/cpufreq/s3c2416-cpufreq.c b/drivers/cpufreq/s3c2416-cpufreq.c > > index 1069103..f07c5d1 100644 > > --- a/drivers/cpufreq/s3c2416-cpufreq.c > > +++ b/drivers/cpufreq/s3c2416-cpufreq.c > > @@ -304,6 +304,7 @@ static int s3c2416_cpufreq_reboot_notifier_evt(struct notifier_block *this, > > { > > struct s3c2416_data *s3c_freq = &s3c2416_cpufreq; > > int ret; > > + struct cpufreq_policy *policy; > > > > mutex_lock(&cpufreq_lock); > > > > @@ -318,7 +319,16 @@ static int s3c2416_cpufreq_reboot_notifier_evt(struct notifier_block *this, > > */ > > if (s3c_freq->is_dvs) { > > pr_debug("cpufreq: leave dvs on reboot\n"); > > - ret = cpufreq_driver_target(cpufreq_cpu_get(0), FREQ_SLEEP, 0); > > + > > + policy = cpufreq_cpu_get(0); > > + if (!policy) { > > + pr_debug("cpufreq: get no policy for cpu0\n"); > > + return NOTIFY_BAD; > > + } > > + > > + ret = cpufreq_driver_target(&policy, FREQ_SLEEP, 0); > > + cpufreq_cpu_put(policy); > > + > > if (ret < 0) > > return NOTIFY_BAD; > > } > > diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c > > index 5d10030..e84281e 100644 > > --- a/drivers/cpufreq/s5pv210-cpufreq.c > > +++ b/drivers/cpufreq/s5pv210-cpufreq.c > > @@ -555,8 +555,17 @@ static int s5pv210_cpufreq_reboot_notifier_event(struct notifier_block *this, > > unsigned long event, void *ptr) > > { > > int ret; > > + struct cpufreq_policy *policy; > > + > > + policy = cpufreq_cpu_get(0); > > + if (!policy) { > > + pr_debug("cpufreq: get no policy for cpu0\n"); > > + return NOTIFY_BAD; > > + } > > + > > + ret = cpufreq_driver_target(policy, SLEEP_FREQ, 0); > > + cpufreq_cpu_put(policy); > > > > - ret = cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ, 0); > > if (ret < 0) > > return NOTIFY_BAD; > > > > -- > > 1.9.1 > > -- > viresh -- Qiwu
diff --git a/drivers/cpufreq/s3c2416-cpufreq.c b/drivers/cpufreq/s3c2416-cpufreq.c index 1069103..f07c5d1 100644 --- a/drivers/cpufreq/s3c2416-cpufreq.c +++ b/drivers/cpufreq/s3c2416-cpufreq.c @@ -304,6 +304,7 @@ static int s3c2416_cpufreq_reboot_notifier_evt(struct notifier_block *this, { struct s3c2416_data *s3c_freq = &s3c2416_cpufreq; int ret; + struct cpufreq_policy *policy; mutex_lock(&cpufreq_lock); @@ -318,7 +319,16 @@ static int s3c2416_cpufreq_reboot_notifier_evt(struct notifier_block *this, */ if (s3c_freq->is_dvs) { pr_debug("cpufreq: leave dvs on reboot\n"); - ret = cpufreq_driver_target(cpufreq_cpu_get(0), FREQ_SLEEP, 0); + + policy = cpufreq_cpu_get(0); + if (!policy) { + pr_debug("cpufreq: get no policy for cpu0\n"); + return NOTIFY_BAD; + } + + ret = cpufreq_driver_target(&policy, FREQ_SLEEP, 0); + cpufreq_cpu_put(policy); + if (ret < 0) return NOTIFY_BAD; } diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c index 5d10030..e84281e 100644 --- a/drivers/cpufreq/s5pv210-cpufreq.c +++ b/drivers/cpufreq/s5pv210-cpufreq.c @@ -555,8 +555,17 @@ static int s5pv210_cpufreq_reboot_notifier_event(struct notifier_block *this, unsigned long event, void *ptr) { int ret; + struct cpufreq_policy *policy; + + policy = cpufreq_cpu_get(0); + if (!policy) { + pr_debug("cpufreq: get no policy for cpu0\n"); + return NOTIFY_BAD; + } + + ret = cpufreq_driver_target(policy, SLEEP_FREQ, 0); + cpufreq_cpu_put(policy); - ret = cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ, 0); if (ret < 0) return NOTIFY_BAD;