[PATCH] cpufreq: s3c: avoid use after free issue in xxx_cpufreq_reboot_notifier_evt()

[PATCH] cpufreq: s3c: avoid use after free issue in xxx_cpufreq_reboot_notifier_evt()

[qiwuchen55]

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.

Signed-off-by: chenqiwu <chenqiwu@xiaomi.com>
---
 drivers/cpufreq/s3c2416-cpufreq.c | 11 ++++++++++-
 drivers/cpufreq/s5pv210-cpufreq.c | 10 +++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/s3c2416-cpufreq.c b/drivers/cpufreq/s3c2416-cpufreq.c
index 1069103..0f576ba 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,15 @@ 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);
+
+		memset(&policy, 0, sizeof(policy));
+		ret = cpufreq_get_policy(&policy, 0);
+		if (ret < 0) {
+			pr_debug("cpufreq: get no policy for cpu0\n");
+			return NOTIFY_BAD;
+		}
+
+		ret = cpufreq_driver_target(&policy, FREQ_SLEEP, 0);
 		if (ret < 0)
 			return NOTIFY_BAD;
 	}
diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
index 5d10030..d99b4b1 100644
--- a/drivers/cpufreq/s5pv210-cpufreq.c
+++ b/drivers/cpufreq/s5pv210-cpufreq.c
@@ -555,8 +555,16 @@ static int s5pv210_cpufreq_reboot_notifier_event(struct notifier_block *this,
 						 unsigned long event, void *ptr)
 {
 	int ret;
+	struct cpufreq_policy *policy;
 
-	ret = cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ, 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, SLEEP_FREQ, 0);
+	cpufreq_cpu_put(policy);
 	if (ret < 0)
 		return NOTIFY_BAD;
 
-- 
1.9.1

[PATCH] cpufreq: s3c: avoid use after free issue in xxx_cpufreq_reboot_notifier_evt()

[qiwuchen55]

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.

Signed-off-by: chenqiwu <chenqiwu@xiaomi.com>
---
 drivers/cpufreq/s3c2416-cpufreq.c | 11 ++++++++++-
 drivers/cpufreq/s5pv210-cpufreq.c | 10 +++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/s3c2416-cpufreq.c b/drivers/cpufreq/s3c2416-cpufreq.c
index 1069103..0f576ba 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,15 @@ 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);
+
+		memset(&policy, 0, sizeof(policy));
+		ret = cpufreq_get_policy(&policy, 0);
+		if (ret < 0) {
+			pr_debug("cpufreq: get no policy for cpu0\n");
+			return NOTIFY_BAD;
+		}
+
+		ret = cpufreq_driver_target(&policy, FREQ_SLEEP, 0);
 		if (ret < 0)
 			return NOTIFY_BAD;
 	}
diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
index 5d10030..d99b4b1 100644
--- a/drivers/cpufreq/s5pv210-cpufreq.c
+++ b/drivers/cpufreq/s5pv210-cpufreq.c
@@ -555,8 +555,16 @@ static int s5pv210_cpufreq_reboot_notifier_event(struct notifier_block *this,
 						 unsigned long event, void *ptr)
 {
 	int ret;
+	struct cpufreq_policy *policy;
 
-	ret = cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ, 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, SLEEP_FREQ, 0);
+	cpufreq_cpu_put(policy);
 	if (ret < 0)
 		return NOTIFY_BAD;
 
-- 
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: s3c: avoid use after free issue in xxx_cpufreq_reboot_notifier_evt()

[Viresh Kumar]

On 28-12-19, 14:43, 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.

There is no UAF problem here, but that we do cpufreq_cpu_get() with a
corresponding cpufreq_cpu_put().

> Signed-off-by: chenqiwu <chenqiwu@xiaomi.com>
> ---
>  drivers/cpufreq/s3c2416-cpufreq.c | 11 ++++++++++-
>  drivers/cpufreq/s5pv210-cpufreq.c | 10 +++++++++-
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/s3c2416-cpufreq.c b/drivers/cpufreq/s3c2416-cpufreq.c
> index 1069103..0f576ba 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,15 @@ 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);
> +
> +		memset(&policy, 0, sizeof(policy));
> +		ret = cpufreq_get_policy(&policy, 0);
> +		if (ret < 0) {
> +			pr_debug("cpufreq: get no policy for cpu0\n");
> +			return NOTIFY_BAD;
> +		}
> +

This doesn't make sense to me, why don't you do cpufreq_cpu_get() and
put() instead ?

> +		ret = cpufreq_driver_target(&policy, FREQ_SLEEP, 0);
>  		if (ret < 0)
>  			return NOTIFY_BAD;
>  	}
> diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
> index 5d10030..d99b4b1 100644
> --- a/drivers/cpufreq/s5pv210-cpufreq.c
> +++ b/drivers/cpufreq/s5pv210-cpufreq.c
> @@ -555,8 +555,16 @@ static int s5pv210_cpufreq_reboot_notifier_event(struct notifier_block *this,
>  						 unsigned long event, void *ptr)
>  {
>  	int ret;
> +	struct cpufreq_policy *policy;
>  
> -	ret = cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ, 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, SLEEP_FREQ, 0);
> +	cpufreq_cpu_put(policy);

Like what is done here.

Also add a blank line here.

>  	if (ret < 0)
>  		return NOTIFY_BAD;
>  
> -- 
> 1.9.1

-- 
viresh

Re: [PATCH] cpufreq: s3c: avoid use after free issue in xxx_cpufreq_reboot_notifier_evt()

[Viresh Kumar]

On 28-12-19, 14:43, 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.

There is no UAF problem here, but that we do cpufreq_cpu_get() with a
corresponding cpufreq_cpu_put().

> Signed-off-by: chenqiwu <chenqiwu@xiaomi.com>
> ---
>  drivers/cpufreq/s3c2416-cpufreq.c | 11 ++++++++++-
>  drivers/cpufreq/s5pv210-cpufreq.c | 10 +++++++++-
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/s3c2416-cpufreq.c b/drivers/cpufreq/s3c2416-cpufreq.c
> index 1069103..0f576ba 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,15 @@ 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);
> +
> +		memset(&policy, 0, sizeof(policy));
> +		ret = cpufreq_get_policy(&policy, 0);
> +		if (ret < 0) {
> +			pr_debug("cpufreq: get no policy for cpu0\n");
> +			return NOTIFY_BAD;
> +		}
> +

This doesn't make sense to me, why don't you do cpufreq_cpu_get() and
put() instead ?

> +		ret = cpufreq_driver_target(&policy, FREQ_SLEEP, 0);
>  		if (ret < 0)
>  			return NOTIFY_BAD;
>  	}
> diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
> index 5d10030..d99b4b1 100644
> --- a/drivers/cpufreq/s5pv210-cpufreq.c
> +++ b/drivers/cpufreq/s5pv210-cpufreq.c
> @@ -555,8 +555,16 @@ static int s5pv210_cpufreq_reboot_notifier_event(struct notifier_block *this,
>  						 unsigned long event, void *ptr)
>  {
>  	int ret;
> +	struct cpufreq_policy *policy;
>  
> -	ret = cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ, 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, SLEEP_FREQ, 0);
> +	cpufreq_cpu_put(policy);

Like what is done here.

Also add a blank line here.

>  	if (ret < 0)
>  		return NOTIFY_BAD;
>  
> -- 
> 1.9.1

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: s3c: avoid use after free issue in xxx_cpufreq_reboot_notifier_evt()

[chenqiwu]

On Mon, Jan 06, 2020 at 11:18:11AM +0530, Viresh Kumar wrote:
> On 28-12-19, 14:43, 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.
> 
> There is no UAF problem here, but that we do cpufreq_cpu_get() with a
> corresponding cpufreq_cpu_put().
> 
> > Signed-off-by: chenqiwu <chenqiwu@xiaomi.com>
> > ---
> >  drivers/cpufreq/s3c2416-cpufreq.c | 11 ++++++++++-
> >  drivers/cpufreq/s5pv210-cpufreq.c | 10 +++++++++-
> >  2 files changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/s3c2416-cpufreq.c b/drivers/cpufreq/s3c2416-cpufreq.c
> > index 1069103..0f576ba 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,15 @@ 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);
> > +
> > +		memset(&policy, 0, sizeof(policy));
> > +		ret = cpufreq_get_policy(&policy, 0);
> > +		if (ret < 0) {
> > +			pr_debug("cpufreq: get no policy for cpu0\n");
> > +			return NOTIFY_BAD;
> > +		}
> > +
> 
> This doesn't make sense to me, why don't you do cpufreq_cpu_get() and
> put() instead ?
>
Hi viresh,
I can't explain which approach is better, but I think both approaches are
effective for the situation.
By the way, there is a possibility that the cpu0 hotplug thread will call
cpufreq_policy_free() to free cpufreq policy if cpu0 hotplug failed.
I think there should be a judgement to avoid this UAF risk if necessary,
or we just do panic if cpu0's cpufreq policy is free.

> > +		ret = cpufreq_driver_target(&policy, FREQ_SLEEP, 0);
> >  		if (ret < 0)
> >  			return NOTIFY_BAD;
> >  	}
> > diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
> > index 5d10030..d99b4b1 100644
> > --- a/drivers/cpufreq/s5pv210-cpufreq.c
> > +++ b/drivers/cpufreq/s5pv210-cpufreq.c
> > @@ -555,8 +555,16 @@ static int s5pv210_cpufreq_reboot_notifier_event(struct notifier_block *this,
> >  						 unsigned long event, void *ptr)
> >  {
> >  	int ret;
> > +	struct cpufreq_policy *policy;
> >  
> > -	ret = cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ, 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, SLEEP_FREQ, 0);
> > +	cpufreq_cpu_put(policy);
> 
> Like what is done here.
> 
> Also add a blank line here.
> 
> >  	if (ret < 0)
> >  		return NOTIFY_BAD;
> >  
> > -- 
> > 1.9.1
> 
> -- 
> viresh

Thanks for your review!
Qiwu

Re: [PATCH] cpufreq: s3c: avoid use after free issue in xxx_cpufreq_reboot_notifier_evt()

[chenqiwu]

On Mon, Jan 06, 2020 at 11:18:11AM +0530, Viresh Kumar wrote:
> On 28-12-19, 14:43, 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.
> 
> There is no UAF problem here, but that we do cpufreq_cpu_get() with a
> corresponding cpufreq_cpu_put().
> 
> > Signed-off-by: chenqiwu <chenqiwu@xiaomi.com>
> > ---
> >  drivers/cpufreq/s3c2416-cpufreq.c | 11 ++++++++++-
> >  drivers/cpufreq/s5pv210-cpufreq.c | 10 +++++++++-
> >  2 files changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/s3c2416-cpufreq.c b/drivers/cpufreq/s3c2416-cpufreq.c
> > index 1069103..0f576ba 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,15 @@ 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);
> > +
> > +		memset(&policy, 0, sizeof(policy));
> > +		ret = cpufreq_get_policy(&policy, 0);
> > +		if (ret < 0) {
> > +			pr_debug("cpufreq: get no policy for cpu0\n");
> > +			return NOTIFY_BAD;
> > +		}
> > +
> 
> This doesn't make sense to me, why don't you do cpufreq_cpu_get() and
> put() instead ?
>
Hi viresh,
I can't explain which approach is better, but I think both approaches are
effective for the situation.
By the way, there is a possibility that the cpu0 hotplug thread will call
cpufreq_policy_free() to free cpufreq policy if cpu0 hotplug failed.
I think there should be a judgement to avoid this UAF risk if necessary,
or we just do panic if cpu0's cpufreq policy is free.

> > +		ret = cpufreq_driver_target(&policy, FREQ_SLEEP, 0);
> >  		if (ret < 0)
> >  			return NOTIFY_BAD;
> >  	}
> > diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
> > index 5d10030..d99b4b1 100644
> > --- a/drivers/cpufreq/s5pv210-cpufreq.c
> > +++ b/drivers/cpufreq/s5pv210-cpufreq.c
> > @@ -555,8 +555,16 @@ static int s5pv210_cpufreq_reboot_notifier_event(struct notifier_block *this,
> >  						 unsigned long event, void *ptr)
> >  {
> >  	int ret;
> > +	struct cpufreq_policy *policy;
> >  
> > -	ret = cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ, 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, SLEEP_FREQ, 0);
> > +	cpufreq_cpu_put(policy);
> 
> Like what is done here.
> 
> Also add a blank line here.
> 
> >  	if (ret < 0)
> >  		return NOTIFY_BAD;
> >  
> > -- 
> > 1.9.1
> 
> -- 
> viresh

Thanks for your review!
Qiwu


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: s3c: avoid use after free issue in xxx_cpufreq_reboot_notifier_evt()

[Viresh Kumar]

On 06-01-20, 14:52, chenqiwu wrote:
> On Mon, Jan 06, 2020 at 11:18:11AM +0530, Viresh Kumar wrote:
> > On 28-12-19, 14:43, 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.
> > 
> > There is no UAF problem here, but that we do cpufreq_cpu_get() with a
> > corresponding cpufreq_cpu_put().
> > 
> > > Signed-off-by: chenqiwu <chenqiwu@xiaomi.com>
> > > ---
> > >  drivers/cpufreq/s3c2416-cpufreq.c | 11 ++++++++++-
> > >  drivers/cpufreq/s5pv210-cpufreq.c | 10 +++++++++-
> > >  2 files changed, 19 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/cpufreq/s3c2416-cpufreq.c b/drivers/cpufreq/s3c2416-cpufreq.c
> > > index 1069103..0f576ba 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,15 @@ 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);
> > > +
> > > +		memset(&policy, 0, sizeof(policy));
> > > +		ret = cpufreq_get_policy(&policy, 0);
> > > +		if (ret < 0) {
> > > +			pr_debug("cpufreq: get no policy for cpu0\n");
> > > +			return NOTIFY_BAD;
> > > +		}
> > > +
> > 
> > This doesn't make sense to me, why don't you do cpufreq_cpu_get() and
> > put() instead ?
> >
> Hi viresh,
> I can't explain which approach is better, but I think both approaches are
> effective for the situation.

The second one is better as it doesn't make copy of the policy, but
rather just increments the counter.

> By the way, there is a possibility that the cpu0 hotplug thread will call
> cpufreq_policy_free() to free cpufreq policy if cpu0 hotplug failed.
> I think there should be a judgement to avoid this UAF risk if necessary,
> or we just do panic if cpu0's cpufreq policy is free.

I think there are enough locks in place to avoid such issues and they
shouldn't happen.

> > > +		ret = cpufreq_driver_target(&policy, FREQ_SLEEP, 0);
> > >  		if (ret < 0)
> > >  			return NOTIFY_BAD;
> > >  	}
> > > diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
> > > index 5d10030..d99b4b1 100644
> > > --- a/drivers/cpufreq/s5pv210-cpufreq.c
> > > +++ b/drivers/cpufreq/s5pv210-cpufreq.c
> > > @@ -555,8 +555,16 @@ static int s5pv210_cpufreq_reboot_notifier_event(struct notifier_block *this,
> > >  						 unsigned long event, void *ptr)
> > >  {
> > >  	int ret;
> > > +	struct cpufreq_policy *policy;
> > >  
> > > -	ret = cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ, 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, SLEEP_FREQ, 0);
> > > +	cpufreq_cpu_put(policy);
> > 
> > Like what is done here.
> > 
> > Also add a blank line here.
> > 
> > >  	if (ret < 0)
> > >  		return NOTIFY_BAD;
> > >  
> > > -- 
> > > 1.9.1
> > 
> > -- 
> > viresh
> 
> Thanks for your review!
> Qiwu

-- 
viresh

Re: [PATCH] cpufreq: s3c: avoid use after free issue in xxx_cpufreq_reboot_notifier_evt()

[Viresh Kumar]

On 06-01-20, 14:52, chenqiwu wrote:
> On Mon, Jan 06, 2020 at 11:18:11AM +0530, Viresh Kumar wrote:
> > On 28-12-19, 14:43, 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.
> > 
> > There is no UAF problem here, but that we do cpufreq_cpu_get() with a
> > corresponding cpufreq_cpu_put().
> > 
> > > Signed-off-by: chenqiwu <chenqiwu@xiaomi.com>
> > > ---
> > >  drivers/cpufreq/s3c2416-cpufreq.c | 11 ++++++++++-
> > >  drivers/cpufreq/s5pv210-cpufreq.c | 10 +++++++++-
> > >  2 files changed, 19 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/cpufreq/s3c2416-cpufreq.c b/drivers/cpufreq/s3c2416-cpufreq.c
> > > index 1069103..0f576ba 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,15 @@ 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);
> > > +
> > > +		memset(&policy, 0, sizeof(policy));
> > > +		ret = cpufreq_get_policy(&policy, 0);
> > > +		if (ret < 0) {
> > > +			pr_debug("cpufreq: get no policy for cpu0\n");
> > > +			return NOTIFY_BAD;
> > > +		}
> > > +
> > 
> > This doesn't make sense to me, why don't you do cpufreq_cpu_get() and
> > put() instead ?
> >
> Hi viresh,
> I can't explain which approach is better, but I think both approaches are
> effective for the situation.

The second one is better as it doesn't make copy of the policy, but
rather just increments the counter.

> By the way, there is a possibility that the cpu0 hotplug thread will call
> cpufreq_policy_free() to free cpufreq policy if cpu0 hotplug failed.
> I think there should be a judgement to avoid this UAF risk if necessary,
> or we just do panic if cpu0's cpufreq policy is free.

I think there are enough locks in place to avoid such issues and they
shouldn't happen.

> > > +		ret = cpufreq_driver_target(&policy, FREQ_SLEEP, 0);
> > >  		if (ret < 0)
> > >  			return NOTIFY_BAD;
> > >  	}
> > > diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
> > > index 5d10030..d99b4b1 100644
> > > --- a/drivers/cpufreq/s5pv210-cpufreq.c
> > > +++ b/drivers/cpufreq/s5pv210-cpufreq.c
> > > @@ -555,8 +555,16 @@ static int s5pv210_cpufreq_reboot_notifier_event(struct notifier_block *this,
> > >  						 unsigned long event, void *ptr)
> > >  {
> > >  	int ret;
> > > +	struct cpufreq_policy *policy;
> > >  
> > > -	ret = cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ, 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, SLEEP_FREQ, 0);
> > > +	cpufreq_cpu_put(policy);
> > 
> > Like what is done here.
> > 
> > Also add a blank line here.
> > 
> > >  	if (ret < 0)
> > >  		return NOTIFY_BAD;
> > >  
> > > -- 
> > > 1.9.1
> > 
> > -- 
> > viresh
> 
> Thanks for your review!
> Qiwu

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: s3c: avoid use after free issue in xxx_cpufreq_reboot_notifier_evt()

[chenqiwu]

On Mon, Jan 06, 2020 at 12:25:02PM +0530, Viresh Kumar wrote:
> On 06-01-20, 14:52, chenqiwu wrote:
> > On Mon, Jan 06, 2020 at 11:18:11AM +0530, Viresh Kumar wrote:
> > > On 28-12-19, 14:43, 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.
> > > 
> > > There is no UAF problem here, but that we do cpufreq_cpu_get() with a
> > > corresponding cpufreq_cpu_put().
> > > 
> > > > Signed-off-by: chenqiwu <chenqiwu@xiaomi.com>
> > > > ---
> > > >  drivers/cpufreq/s3c2416-cpufreq.c | 11 ++++++++++-
> > > >  drivers/cpufreq/s5pv210-cpufreq.c | 10 +++++++++-
> > > >  2 files changed, 19 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/cpufreq/s3c2416-cpufreq.c b/drivers/cpufreq/s3c2416-cpufreq.c
> > > > index 1069103..0f576ba 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,15 @@ 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);
> > > > +
> > > > +		memset(&policy, 0, sizeof(policy));
> > > > +		ret = cpufreq_get_policy(&policy, 0);
> > > > +		if (ret < 0) {
> > > > +			pr_debug("cpufreq: get no policy for cpu0\n");
> > > > +			return NOTIFY_BAD;
> > > > +		}
> > > > +
> > > 
> > > This doesn't make sense to me, why don't you do cpufreq_cpu_get() and
> > > put() instead ?
> > >
> > Hi viresh,
> > I can't explain which approach is better, but I think both approaches are
> > effective for the situation.
> 
> The second one is better as it doesn't make copy of the policy, but
> rather just increments the counter.
> 
> > By the way, there is a possibility that the cpu0 hotplug thread will call
> > cpufreq_policy_free() to free cpufreq policy if cpu0 hotplug failed.
> > I think there should be a judgement to avoid this UAF risk if necessary,
> > or we just do panic if cpu0's cpufreq policy is free.
> 
> I think there are enough locks in place to avoid such issues and they
> shouldn't happen.
>

Hhh..I don't agree this, since the cpufreq policy of cpu0 may have
been released before such UAF issue happenning.
By the way, why not get invaild cpufreq policy of another cpu but
only cpu0 here?

> > > > +		ret = cpufreq_driver_target(&policy, FREQ_SLEEP, 0);
> > > >  		if (ret < 0)
> > > >  			return NOTIFY_BAD;
> > > >  	}
> > > > diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
> > > > index 5d10030..d99b4b1 100644
> > > > --- a/drivers/cpufreq/s5pv210-cpufreq.c
> > > > +++ b/drivers/cpufreq/s5pv210-cpufreq.c
> > > > @@ -555,8 +555,16 @@ static int s5pv210_cpufreq_reboot_notifier_event(struct notifier_block *this,
> > > >  						 unsigned long event, void *ptr)
> > > >  {
> > > >  	int ret;
> > > > +	struct cpufreq_policy *policy;
> > > >  
> > > > -	ret = cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ, 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, SLEEP_FREQ, 0);
> > > > +	cpufreq_cpu_put(policy);
> > > 
> > > Like what is done here.
> > > 
> > > Also add a blank line here.
> > > 
> > > >  	if (ret < 0)
> > > >  		return NOTIFY_BAD;
> > > >  
> > > > -- 
> > > > 1.9.1
> > > 
> > > -- 
> > > viresh
> > 
> > Thanks for your review!
> > Qiwu
> 
> -- 
> viresh

Thanks!
Qiwu

Re: [PATCH] cpufreq: s3c: avoid use after free issue in xxx_cpufreq_reboot_notifier_evt()

[chenqiwu]

On Mon, Jan 06, 2020 at 12:25:02PM +0530, Viresh Kumar wrote:
> On 06-01-20, 14:52, chenqiwu wrote:
> > On Mon, Jan 06, 2020 at 11:18:11AM +0530, Viresh Kumar wrote:
> > > On 28-12-19, 14:43, 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.
> > > 
> > > There is no UAF problem here, but that we do cpufreq_cpu_get() with a
> > > corresponding cpufreq_cpu_put().
> > > 
> > > > Signed-off-by: chenqiwu <chenqiwu@xiaomi.com>
> > > > ---
> > > >  drivers/cpufreq/s3c2416-cpufreq.c | 11 ++++++++++-
> > > >  drivers/cpufreq/s5pv210-cpufreq.c | 10 +++++++++-
> > > >  2 files changed, 19 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/cpufreq/s3c2416-cpufreq.c b/drivers/cpufreq/s3c2416-cpufreq.c
> > > > index 1069103..0f576ba 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,15 @@ 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);
> > > > +
> > > > +		memset(&policy, 0, sizeof(policy));
> > > > +		ret = cpufreq_get_policy(&policy, 0);
> > > > +		if (ret < 0) {
> > > > +			pr_debug("cpufreq: get no policy for cpu0\n");
> > > > +			return NOTIFY_BAD;
> > > > +		}
> > > > +
> > > 
> > > This doesn't make sense to me, why don't you do cpufreq_cpu_get() and
> > > put() instead ?
> > >
> > Hi viresh,
> > I can't explain which approach is better, but I think both approaches are
> > effective for the situation.
> 
> The second one is better as it doesn't make copy of the policy, but
> rather just increments the counter.
> 
> > By the way, there is a possibility that the cpu0 hotplug thread will call
> > cpufreq_policy_free() to free cpufreq policy if cpu0 hotplug failed.
> > I think there should be a judgement to avoid this UAF risk if necessary,
> > or we just do panic if cpu0's cpufreq policy is free.
> 
> I think there are enough locks in place to avoid such issues and they
> shouldn't happen.
>

Hhh..I don't agree this, since the cpufreq policy of cpu0 may have
been released before such UAF issue happenning.
By the way, why not get invaild cpufreq policy of another cpu but
only cpu0 here?

> > > > +		ret = cpufreq_driver_target(&policy, FREQ_SLEEP, 0);
> > > >  		if (ret < 0)
> > > >  			return NOTIFY_BAD;
> > > >  	}
> > > > diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
> > > > index 5d10030..d99b4b1 100644
> > > > --- a/drivers/cpufreq/s5pv210-cpufreq.c
> > > > +++ b/drivers/cpufreq/s5pv210-cpufreq.c
> > > > @@ -555,8 +555,16 @@ static int s5pv210_cpufreq_reboot_notifier_event(struct notifier_block *this,
> > > >  						 unsigned long event, void *ptr)
> > > >  {
> > > >  	int ret;
> > > > +	struct cpufreq_policy *policy;
> > > >  
> > > > -	ret = cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ, 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, SLEEP_FREQ, 0);
> > > > +	cpufreq_cpu_put(policy);
> > > 
> > > Like what is done here.
> > > 
> > > Also add a blank line here.
> > > 
> > > >  	if (ret < 0)
> > > >  		return NOTIFY_BAD;
> > > >  
> > > > -- 
> > > > 1.9.1
> > > 
> > > -- 
> > > viresh
> > 
> > Thanks for your review!
> > Qiwu
> 
> -- 
> viresh

Thanks!
Qiwu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: s3c: avoid use after free issue in xxx_cpufreq_reboot_notifier_evt()

[Viresh Kumar]

On 06-01-20, 16:30, chenqiwu wrote:
> Hhh..I don't agree this, since the cpufreq policy of cpu0 may have
> been released before such UAF issue happenning.

That won't happen if you do cpufreq_cpu_get(), isn't it ?

> By the way, why not get invaild cpufreq policy of another cpu but
> only cpu0 here?

Probably because this platform has a single cpufreq policy which covers all the
CPUs and so it doesn't really matter which CPU you use to get the policy.

-- 
viresh

Re: [PATCH] cpufreq: s3c: avoid use after free issue in xxx_cpufreq_reboot_notifier_evt()

[Viresh Kumar]

On 06-01-20, 16:30, chenqiwu wrote:
> Hhh..I don't agree this, since the cpufreq policy of cpu0 may have
> been released before such UAF issue happenning.

That won't happen if you do cpufreq_cpu_get(), isn't it ?

> By the way, why not get invaild cpufreq policy of another cpu but
> only cpu0 here?

Probably because this platform has a single cpufreq policy which covers all the
CPUs and so it doesn't really matter which CPU you use to get the policy.

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel