[RESEND,4/4] thermal: check for invalid trip setup when registering thermal device
diff mbox

Message ID 1357140582-8151-5-git-send-email-eduardo.valentin@ti.com
State Rejected
Delegated to: Zhang Rui
Headers show

Commit Message

Eduardo Valentin Jan. 2, 2013, 3:29 p.m. UTC
This patch adds an extra check in the data structure while registering
a thermal device. The check is to avoid registering zones with a number
of trips greater than zero, but with no .get_trip_temp nor .get_trip_type
callbacks. Receiving such data structure may end in wrong data access.

Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
 drivers/thermal/thermal_sys.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

Comments

durgadoss.r@intel.com Jan. 2, 2013, 3:53 p.m. UTC | #1
> -----Original Message-----
> From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> owner@vger.kernel.org] On Behalf Of Eduardo Valentin
> Sent: Wednesday, January 02, 2013 9:00 PM
> To: Zhang, Rui
> Cc: R, Durgadoss; linux-pm@lists.linux-foundation.org; linux-
> pm@vger.kernel.org; Eduardo Valentin
> Subject: [PATCH RESEND 4/4] thermal: check for invalid trip setup when
> registering thermal device
> 
> This patch adds an extra check in the data structure while registering
> a thermal device. The check is to avoid registering zones with a number
> of trips greater than zero, but with no .get_trip_temp nor .get_trip_type
> callbacks. Receiving such data structure may end in wrong data access.

Yes, agreed. we are moving away from this API too :-(
But Nothing hurts to have this fix. So,

Reviewed-by: Durgadoss R <durgadoss.r@intel.com>

Thanks,
Durga

> 
> Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
> ---
>  drivers/thermal/thermal_sys.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index fba27c3..0a1bf6b 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -1530,6 +1530,9 @@ struct thermal_zone_device
> *thermal_zone_device_register(const char *type,
>  	if (!ops || !ops->get_temp)
>  		return ERR_PTR(-EINVAL);
> 
> +	if (trips > 0 && !ops->get_trip_type)
> +		return ERR_PTR(-EINVAL);
> +
>  	tz = kzalloc(sizeof(struct thermal_zone_device), GFP_KERNEL);
>  	if (!tz)
>  		return ERR_PTR(-ENOMEM);
> --
> 1.7.7.1.488.ge8e1c
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhang Rui Jan. 16, 2013, 2:45 a.m. UTC | #2
On Wed, 2013-01-02 at 17:29 +0200, Eduardo Valentin wrote:
> This patch adds an extra check in the data structure while registering
> a thermal device. The check is to avoid registering zones with a number
> of trips greater than zero, but with no .get_trip_temp nor .get_trip_type
> callbacks. Receiving such data structure may end in wrong data access.
> 
> Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
> ---
>  drivers/thermal/thermal_sys.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> index fba27c3..0a1bf6b 100644
> --- a/drivers/thermal/thermal_sys.c
> +++ b/drivers/thermal/thermal_sys.c
> @@ -1530,6 +1530,9 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>  	if (!ops || !ops->get_temp)
>  		return ERR_PTR(-EINVAL);
>  
> +	if (trips > 0 && !ops->get_trip_type)
> +		return ERR_PTR(-EINVAL);
> +
Hmmm, I do not think we need this.

the sysfs I/F trip_point_X_type already does this check.

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eduardo Valentin Jan. 16, 2013, 2:22 p.m. UTC | #3
On 16-01-2013 04:45, Zhang Rui wrote:
> On Wed, 2013-01-02 at 17:29 +0200, Eduardo Valentin wrote:
>> This patch adds an extra check in the data structure while registering
>> a thermal device. The check is to avoid registering zones with a number
>> of trips greater than zero, but with no .get_trip_temp nor .get_trip_type
>> callbacks. Receiving such data structure may end in wrong data access.
>>
>> Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
>> ---
>>   drivers/thermal/thermal_sys.c |    3 +++
>>   1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
>> index fba27c3..0a1bf6b 100644
>> --- a/drivers/thermal/thermal_sys.c
>> +++ b/drivers/thermal/thermal_sys.c
>> @@ -1530,6 +1530,9 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
>>   	if (!ops || !ops->get_temp)
>>   		return ERR_PTR(-EINVAL);
>>
>> +	if (trips > 0 && !ops->get_trip_type)
>> +		return ERR_PTR(-EINVAL);
>> +
> Hmmm, I do not think we need this.
>
> the sysfs I/F trip_point_X_type already does this check.

Well that's not the point. If you trust only the sysfs handling, fine. 
The fix is very punctual. If you pass wrong parameters to 
thermal_zone_device_register, it will crash.

Of course, in either case, with or without his patch, it won't register 
the device, so you won't get to the sysfs handling part. At least with 
this fix, the crash won't happen and the driver writer will receive an 
error code to treat.

Ok. being specific, what happens is that there are accesses to 
ops->get_trip_type, even if trips > 0 and if .get_trip_type is NULL, 
right below inside the register function. Instead of checking for 
.get_trip_type every time before using it, this patch adds a single 
constraint, so that, if you have trips, you must specify their types.. 
To me sounds reasonable enough.



>
> thanks,
> rui
>

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhang Rui Jan. 17, 2013, 7:10 a.m. UTC | #4
On Wed, 2013-01-16 at 16:22 +0200, Eduardo Valentin wrote:
> On 16-01-2013 04:45, Zhang Rui wrote:
> > On Wed, 2013-01-02 at 17:29 +0200, Eduardo Valentin wrote:
> >> This patch adds an extra check in the data structure while registering
> >> a thermal device. The check is to avoid registering zones with a number
> >> of trips greater than zero, but with no .get_trip_temp nor .get_trip_type
> >> callbacks. Receiving such data structure may end in wrong data access.
> >>
> >> Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
> >> ---
> >>   drivers/thermal/thermal_sys.c |    3 +++
> >>   1 files changed, 3 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
> >> index fba27c3..0a1bf6b 100644
> >> --- a/drivers/thermal/thermal_sys.c
> >> +++ b/drivers/thermal/thermal_sys.c
> >> @@ -1530,6 +1530,9 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type,
> >>   	if (!ops || !ops->get_temp)
> >>   		return ERR_PTR(-EINVAL);
> >>
> >> +	if (trips > 0 && !ops->get_trip_type)
> >> +		return ERR_PTR(-EINVAL);
> >> +
> > Hmmm, I do not think we need this.
> >
> > the sysfs I/F trip_point_X_type already does this check.
> 
> Well that's not the point. If you trust only the sysfs handling, fine. 
> The fix is very punctual. If you pass wrong parameters to 
> thermal_zone_device_register, it will crash.
> 
> Of course, in either case, with or without his patch, it won't register 
> the device, so you won't get to the sysfs handling part. At least with 
> this fix, the crash won't happen and the driver writer will receive an 
> error code to treat.
> 
> Ok. being specific, what happens is that there are accesses to 
> ops->get_trip_type, even if trips > 0 and if .get_trip_type is NULL, 
> right below inside the register function. Instead of checking for 
> .get_trip_type every time before using it, this patch adds a single 
> constraint, so that, if you have trips, you must specify their types.. 
> To me sounds reasonable enough.
> 
agreed.

applied to thermal -next.

thanks,
rui


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index fba27c3..0a1bf6b 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -1530,6 +1530,9 @@  struct thermal_zone_device *thermal_zone_device_register(const char *type,
 	if (!ops || !ops->get_temp)
 		return ERR_PTR(-EINVAL);
 
+	if (trips > 0 && !ops->get_trip_type)
+		return ERR_PTR(-EINVAL);
+
 	tz = kzalloc(sizeof(struct thermal_zone_device), GFP_KERNEL);
 	if (!tz)
 		return ERR_PTR(-ENOMEM);