[PATCH]cpufreq: add NULL check to the store method of cpufreq

[PATCH]cpufreq: add NULL check to the store method of cpufreq

[Shen Kai]

From: shenkai <shenkai8@huawei.com>

Add NULL check in the store function here to avoid NULL callback invoking.
Though some interfaces of cpufreq are set as read-only, user can still get 
write permission using chmod which can lead to a kernel crash.

The following operations can lead to a kernel crash.

chmod +w /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
echo 1 >  /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq

This bug was found on linux 4.19

Signed-off-by: shenkai <shenkai8@huawei.com>
Reported-by: linfeilong <linfeilong@huawei.com>
Reviewed-by: linfeilong <linfeilong@huawei.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index bffc11b..4ccaa96 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -947,6 +947,9 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
 	struct freq_attr *fattr = to_attr(attr);
 	ssize_t ret = -EINVAL;
 
+	if (!fattr->store)
+		return ret;
+
 	/*
 	 * cpus_read_trylock() is used here to work around a circular lock
 	 * dependency problem with respect to the cpufreq_register_driver().
-- 
2.6.4.windows.1

Re: [PATCH]cpufreq: add NULL check to the store method of cpufreq

[Rafael J. Wysocki]

On Wed, Nov 6, 2019 at 8:47 AM Shen Kai <shenkai8@huawei.com> wrote:
>
> From: shenkai <shenkai8@huawei.com>

Can you please use the proper spelling of your name here?

>
> Add NULL check in the store function here to avoid NULL callback invoking.
> Though some interfaces of cpufreq are set as read-only, user can still get
> write permission using chmod which can lead to a kernel crash.
>
> The following operations can lead to a kernel crash.
>
> chmod +w /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
> echo 1 >  /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
>
> This bug was found on linux 4.19
>
> Signed-off-by: shenkai <shenkai8@huawei.com>

And here?

> Reported-by: linfeilong <linfeilong@huawei.com>
> Reviewed-by: linfeilong <linfeilong@huawei.com>

And the proper spelling of the reporter/reviewer name here too?

> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index bffc11b..4ccaa96 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -947,6 +947,9 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
>         struct freq_attr *fattr = to_attr(attr);
>         ssize_t ret = -EINVAL;
>
> +       if (!fattr->store)
> +               return ret;
> +
>         /*
>          * cpus_read_trylock() is used here to work around a circular lock
>          * dependency problem with respect to the cpufreq_register_driver().
> --
> 2.6.4.windows.1
>
>

[PATCH]cpufreq: add NULL check to the store method of cpufreq

[Shen Kai]

From: Kai Shen <shenkai8@huawei.com>

Add NULL check in the store function here to avoid NULL callback invoking.
Though some interfaces of cpufreq are set as read-only, user can still get 
write permission using chmod which can lead to a kernel crash.

The following operations can lead to a kernel crash.

chmod +w /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
echo 1 >  /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq

This bug was found on linux 4.19

Signed-off-by: Kai Shen <shenkai8@huawei.com>
Reported-by: Feilong Lin <linfeilong@huawei.com>
Reviewed-by: Feilong Lin <linfeilong@huawei.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index bffc11b..4ccaa96 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -947,6 +947,9 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
 	struct freq_attr *fattr = to_attr(attr);
 	ssize_t ret = -EINVAL;
 
+	if (!fattr->store)
+		return ret;
+
 	/*
 	 * cpus_read_trylock() is used here to work around a circular lock
 	 * dependency problem with respect to the cpufreq_register_driver().
-- 
2.6.4.windows.1

Re: [PATCH]cpufreq: add NULL check to the store method of cpufreq

[Viresh Kumar]

On 07-11-19, 01:53, Shen Kai wrote:
> From: Kai Shen <shenkai8@huawei.com>
> 
> Add NULL check in the store function here to avoid NULL callback invoking.
> Though some interfaces of cpufreq are set as read-only, user can still get 
> write permission using chmod which can lead to a kernel crash.
> 
> The following operations can lead to a kernel crash.
> 
> chmod +w /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
> echo 1 >  /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
> 
> This bug was found on linux 4.19
> 
> Signed-off-by: Kai Shen <shenkai8@huawei.com>
> Reported-by: Feilong Lin <linfeilong@huawei.com>
> Reviewed-by: Feilong Lin <linfeilong@huawei.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index bffc11b..4ccaa96 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -947,6 +947,9 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
>  	struct freq_attr *fattr = to_attr(attr);
>  	ssize_t ret = -EINVAL;
>  
> +	if (!fattr->store)
> +		return ret;

The return error shouldn't be -EINVAL here, maybe ENOPERM or ENOENT ?

> +
>  	/*
>  	 * cpus_read_trylock() is used here to work around a circular lock
>  	 * dependency problem with respect to the cpufreq_register_driver().
> -- 
> 2.6.4.windows.1
> 

-- 
viresh

Re: [PATCH]cpufreq: add NULL check to the store method of cpufreq

[Viresh Kumar]

On 07-11-19, 08:02, Viresh Kumar wrote:
> On 07-11-19, 01:53, Shen Kai wrote:
> > From: Kai Shen <shenkai8@huawei.com>
> > 
> > Add NULL check in the store function here to avoid NULL callback invoking.
> > Though some interfaces of cpufreq are set as read-only, user can still get 
> > write permission using chmod which can lead to a kernel crash.
> > 
> > The following operations can lead to a kernel crash.
> > 
> > chmod +w /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
> > echo 1 >  /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
> > 
> > This bug was found on linux 4.19
> > 
> > Signed-off-by: Kai Shen <shenkai8@huawei.com>
> > Reported-by: Feilong Lin <linfeilong@huawei.com>
> > Reviewed-by: Feilong Lin <linfeilong@huawei.com>
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  drivers/cpufreq/cpufreq.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index bffc11b..4ccaa96 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -947,6 +947,9 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
> >  	struct freq_attr *fattr = to_attr(attr);
> >  	ssize_t ret = -EINVAL;
> >  
> > +	if (!fattr->store)
> > +		return ret;
> 
> The return error shouldn't be -EINVAL here, maybe ENOPERM or ENOENT ?

s/ENOPERM/EPERM/

-- 
viresh

[PATCH]cpufreq: add NULL check to the store method of cpufreq

[Shen Kai]

From: Kai Shen <shenkai8@huawei.com>

Add NULL check in the store function here to avoid NULL callback invoking.
Though some interfaces of cpufreq are set as read-only, user can still get 
write permission using chmod which can lead to a kernel crash.

The following operations can lead to a kernel crash.

chmod +w /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
echo 1 >  /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq

This bug was found on linux 4.19

Signed-off-by: Kai Shen <shenkai8@huawei.com>
Reported-by: Feilong Lin <linfeilong@huawei.com>
Reviewed-by: Feilong Lin <linfeilong@huawei.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index bffc11b..4ccaa96 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -947,6 +947,9 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
 	struct freq_attr *fattr = to_attr(attr);
 	ssize_t ret = -EINVAL;
 
+	if (!fattr->store)
+		return -EPERM;
+
 	/*
 	 * cpus_read_trylock() is used here to work around a circular lock
 	 * dependency problem with respect to the cpufreq_register_driver().
-- 
2.6.4.windows.1

Re: [PATCH]cpufreq: add NULL check to the store method of cpufreq

[Viresh Kumar]

On 07-11-19, 02:53, Shen Kai wrote:
> From: Kai Shen <shenkai8@huawei.com>
> 
> Add NULL check in the store function here to avoid NULL callback invoking.
> Though some interfaces of cpufreq are set as read-only, user can still get 
> write permission using chmod which can lead to a kernel crash.
> 
> The following operations can lead to a kernel crash.
> 
> chmod +w /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
> echo 1 >  /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
> 
> This bug was found on linux 4.19
> 
> Signed-off-by: Kai Shen <shenkai8@huawei.com>
> Reported-by: Feilong Lin <linfeilong@huawei.com>
> Reviewed-by: Feilong Lin <linfeilong@huawei.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index bffc11b..4ccaa96 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -947,6 +947,9 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
>  	struct freq_attr *fattr = to_attr(attr);
>  	ssize_t ret = -EINVAL;
>  
> +	if (!fattr->store)
> +		return -EPERM;
> +

And this should be -EIO, I found that after looking at all the
instances of struct kobj_type in the kernel :(

Also there is another change you need to make.

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index dd1628192310..4bfaafde9083 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -933,6 +933,9 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
        struct freq_attr *fattr = to_attr(attr);
        ssize_t ret;
 
+       if (!fattr->show)
+               return -EIO;
+
        down_read(&policy->rwsem);
        ret = fattr->show(policy, buf);
        up_read(&policy->rwsem);

as there is a write-only attribute (reset) in cpufreq stats.

-- 
viresh

[PATCH v3]cpufreq: add NULL check to the store method of cpufreq

[Shen Kai]

From: Kai Shen <shenkai8@huawei.com>

Add NULL check in the store function here to avoid NULL callback invoking.
Though some interfaces of cpufreq are set as read-only, user can still get 
write permission using chmod which can lead to a kernel crash.

The following operations can lead to a kernel crash.

chmod +w /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
echo 1 >  /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq

This bug was found on linux 4.19

Signed-off-by: Kai Shen <shenkai8@huawei.com>
Reported-by: Feilong Lin <linfeilong@huawei.com>
Reviewed-by: Feilong Lin <linfeilong@huawei.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V2->V3
- use return value -EIO instead.
- do NULL check in show method too.

 drivers/cpufreq/cpufreq.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 48a224a..bc19d6c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -933,6 +933,9 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
 	struct freq_attr *fattr = to_attr(attr);
 	ssize_t ret;
 
+	if (!fattr->show)
+		return -EIO;
+
 	down_read(&policy->rwsem);
 	ret = fattr->show(policy, buf);
 	up_read(&policy->rwsem);
@@ -947,6 +950,9 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
 	struct freq_attr *fattr = to_attr(attr);
 	ssize_t ret = -EINVAL;
 
+	if (!fattr->store)
+		return -EIO;
+
 	/*
 	 * cpus_read_trylock() is used here to work around a circular lock
 	 * dependency problem with respect to the cpufreq_register_driver().
-- 
2.6.4.windows.1

Re: [PATCH v3]cpufreq: add NULL check to the store method of cpufreq

[Viresh Kumar]

On 07-11-19, 05:08, Shen Kai wrote:
> From: Kai Shen <shenkai8@huawei.com>
> 
> Add NULL check in the store function here to avoid NULL callback invoking.
> Though some interfaces of cpufreq are set as read-only, user can still get 
> write permission using chmod which can lead to a kernel crash.
> 
> The following operations can lead to a kernel crash.
> 
> chmod +w /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
> echo 1 >  /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
> 
> This bug was found on linux 4.19
> 
> Signed-off-by: Kai Shen <shenkai8@huawei.com>
> Reported-by: Feilong Lin <linfeilong@huawei.com>
> Reviewed-by: Feilong Lin <linfeilong@huawei.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V2->V3
> - use return value -EIO instead.
> - do NULL check in show method too.
> 
>  drivers/cpufreq/cpufreq.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 48a224a..bc19d6c 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -933,6 +933,9 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
>  	struct freq_attr *fattr = to_attr(attr);
>  	ssize_t ret;
>  
> +	if (!fattr->show)
> +		return -EIO;
> +
>  	down_read(&policy->rwsem);
>  	ret = fattr->show(policy, buf);
>  	up_read(&policy->rwsem);
> @@ -947,6 +950,9 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
>  	struct freq_attr *fattr = to_attr(attr);
>  	ssize_t ret = -EINVAL;
>  
> +	if (!fattr->store)
> +		return -EIO;
> +
>  	/*
>  	 * cpus_read_trylock() is used here to work around a circular lock
>  	 * dependency problem with respect to the cpufreq_register_driver().

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v3]cpufreq: add NULL check to the store method of cpufreq

[Rafael J. Wysocki]

On Thursday, November 7, 2019 6:08:17 AM CET Shen Kai wrote:
> From: Kai Shen <shenkai8@huawei.com>
> 
> Add NULL check in the store function here to avoid NULL callback invoking.
> Though some interfaces of cpufreq are set as read-only, user can still get 
> write permission using chmod which can lead to a kernel crash.
> 
> The following operations can lead to a kernel crash.
> 
> chmod +w /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
> echo 1 >  /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
> 
> This bug was found on linux 4.19
> 
> Signed-off-by: Kai Shen <shenkai8@huawei.com>
> Reported-by: Feilong Lin <linfeilong@huawei.com>
> Reviewed-by: Feilong Lin <linfeilong@huawei.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V2->V3
> - use return value -EIO instead.
> - do NULL check in show method too.
> 
>  drivers/cpufreq/cpufreq.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 48a224a..bc19d6c 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -933,6 +933,9 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
>  	struct freq_attr *fattr = to_attr(attr);
>  	ssize_t ret;
>  
> +	if (!fattr->show)
> +		return -EIO;
> +
>  	down_read(&policy->rwsem);
>  	ret = fattr->show(policy, buf);
>  	up_read(&policy->rwsem);
> @@ -947,6 +950,9 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
>  	struct freq_attr *fattr = to_attr(attr);
>  	ssize_t ret = -EINVAL;
>  
> +	if (!fattr->store)
> +		return -EIO;
> +
>  	/*
>  	 * cpus_read_trylock() is used here to work around a circular lock
>  	 * dependency problem with respect to the cpufreq_register_driver().
> 

Applying as 5.5 material with updated subject and changelog (the original ones
did not mention the show() function).

Thanks!