[bug report] thermal: Add cooling device's statistics in sysfs

[bug report] thermal: Add cooling device's statistics in sysfs

[Dan Carpenter]

Hello Viresh Kumar,

The patch 8ea229511e06: "thermal: Add cooling device's statistics in
sysfs" from Apr 2, 2018, leads to the following Smatch static checker
warning:

	drivers/thermal/thermal_sysfs.c:656 thermal_cooling_device_stats_update()
	warn: potential integer overflow from user 'stats->state * stats->max_states + new_state'

drivers/thermal/thermal_sysfs.c
    642 void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
    643                                          unsigned long new_state)
    644 {
    645         struct cooling_dev_stats *stats = cdev->stats;
    646 
    647         if (!stats)
    648                 return;
    649 
    650         spin_lock(&stats->lock);
    651 
    652         if (stats->state == new_state)
    653                 goto unlock;
    654 
    655         update_time_in_state(stats);
--> 656         stats->trans_table[stats->state * stats->max_states + new_state]++;
                                                                      ^^^^^^^^^
The new state value comes from the user via sysfs.  It is <= LONG_MAX
but otherwise there is no limit on its value.  Presumably only the
admin can write to this file so the security impact of this buffer
overflow is not as bad as it could have been.

    657         stats->state = new_state;
    658         stats->total_trans++;
    659 
    660 unlock:
    661         spin_unlock(&stats->lock);
    662 }

regards,
dan carpenter

Re: [bug report] thermal: Add cooling device's statistics in sysfs

[Dan Carpenter]

On Fri, Oct 14, 2022 at 05:08:04PM +0300, Dan Carpenter wrote:
> Hello Viresh Kumar,
> 
> The patch 8ea229511e06: "thermal: Add cooling device's statistics in
> sysfs" from Apr 2, 2018, leads to the following Smatch static checker
> warning:
> 
> 	drivers/thermal/thermal_sysfs.c:656 thermal_cooling_device_stats_update()
> 	warn: potential integer overflow from user 'stats->state * stats->max_states + new_state'
> 
> drivers/thermal/thermal_sysfs.c
>     642 void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
>     643                                          unsigned long new_state)
>     644 {
>     645         struct cooling_dev_stats *stats = cdev->stats;
>     646 
>     647         if (!stats)
>     648                 return;
>     649 
>     650         spin_lock(&stats->lock);
>     651 
>     652         if (stats->state == new_state)
>     653                 goto unlock;
>     654 
>     655         update_time_in_state(stats);
> --> 656         stats->trans_table[stats->state * stats->max_states + new_state]++;
>                                                                       ^^^^^^^^^
> The new state value comes from the user via sysfs.  It is <= LONG_MAX
> but otherwise there is no limit on its value.  Presumably only the
> admin can write to this file so the security impact of this buffer
> overflow is not as bad as it could have been.
> 

(There are some drivers which might cap new_state.  I only looked at
tc654_set_cur_state() and it doesn't cap the value.  It's always going
to be safer to do the bounds checking in a central place).

regards,
dan carpenter

Re: [bug report] thermal: Add cooling device's statistics in sysfs

[Viresh Kumar]

On 14-10-22, 17:08, Dan Carpenter wrote:
> Hello Viresh Kumar,
> 
> The patch 8ea229511e06: "thermal: Add cooling device's statistics in
> sysfs" from Apr 2, 2018, leads to the following Smatch static checker
> warning:
> 
> 	drivers/thermal/thermal_sysfs.c:656 thermal_cooling_device_stats_update()
> 	warn: potential integer overflow from user 'stats->state * stats->max_states + new_state'
> 
> drivers/thermal/thermal_sysfs.c
>     642 void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
>     643                                          unsigned long new_state)
>     644 {
>     645         struct cooling_dev_stats *stats = cdev->stats;
>     646 
>     647         if (!stats)
>     648                 return;
>     649 
>     650         spin_lock(&stats->lock);
>     651 
>     652         if (stats->state == new_state)
>     653                 goto unlock;
>     654 
>     655         update_time_in_state(stats);
> --> 656         stats->trans_table[stats->state * stats->max_states + new_state]++;
>                                                                       ^^^^^^^^^
> The new state value comes from the user via sysfs.  It is <= LONG_MAX
> but otherwise there is no limit on its value.

This routine gets called after cdev->ops->set_cur_state() has returned
successfully. That callback does the verification of this value, based
on what's the maximum value allowed and hence the size of the array
here.

I don't think we will have any issue here unless the cooling driver is
buggy and isn't checking the state properly.

> Presumably only the
> admin can write to this file so the security impact of this buffer
> overflow is not as bad as it could have been.

-- 
viresh

Re: [bug report] thermal: Add cooling device's statistics in sysfs

[Dan Carpenter]

On Mon, Oct 17, 2022 at 11:27:35AM +0530, Viresh Kumar wrote:
> On 14-10-22, 17:08, Dan Carpenter wrote:
> > Hello Viresh Kumar,
> > 
> > The patch 8ea229511e06: "thermal: Add cooling device's statistics in
> > sysfs" from Apr 2, 2018, leads to the following Smatch static checker
> > warning:
> > 
> > 	drivers/thermal/thermal_sysfs.c:656 thermal_cooling_device_stats_update()
> > 	warn: potential integer overflow from user 'stats->state * stats->max_states + new_state'
> > 
> > drivers/thermal/thermal_sysfs.c
> >     642 void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
> >     643                                          unsigned long new_state)
> >     644 {
> >     645         struct cooling_dev_stats *stats = cdev->stats;
> >     646 
> >     647         if (!stats)
> >     648                 return;
> >     649 
> >     650         spin_lock(&stats->lock);
> >     651 
> >     652         if (stats->state == new_state)
> >     653                 goto unlock;
> >     654 
> >     655         update_time_in_state(stats);
> > --> 656         stats->trans_table[stats->state * stats->max_states + new_state]++;
> >                                                                       ^^^^^^^^^
> > The new state value comes from the user via sysfs.  It is <= LONG_MAX
> > but otherwise there is no limit on its value.
> 
> This routine gets called after cdev->ops->set_cur_state() has returned
> successfully. That callback does the verification of this value, based
> on what's the maximum value allowed and hence the size of the array
> here.
> 
> I don't think we will have any issue here unless the cooling driver is
> buggy and isn't checking the state properly.
> 

I only looked at the 1 example I posted, but so far as I can see from a
sample size of 1 they are all buggy.  100% buggy.

This strategy of relying on the drivers to do it correctly was doomed
from the get go.

regards,
dan carpenter

Re: [bug report] thermal: Add cooling device's statistics in sysfs

[Viresh Kumar]

On 17-10-22, 10:09, Dan Carpenter wrote:
> I only looked at the 1 example I posted, but so far as I can see from a
> sample size of 1 they are all buggy.  100% buggy.
> 
> This strategy of relying on the drivers to do it correctly was doomed
> from the get go.

Yeah, I realized that too after sending you the email. I am working on
a patch to fix it.

-- 
viresh

Re: [bug report] thermal: Add cooling device's statistics in sysfs

[Viresh Kumar]

On 14-10-22, 17:30, Dan Carpenter wrote:
> (There are some drivers which might cap new_state.  I only looked at
> tc654_set_cur_state() and it doesn't cap the value.  It's always going
> to be safer to do the bounds checking in a central place).

I agree that it is better to do it at central place.

But, tc654_set_cur_state() seems to be doing it just fine:

static int tc654_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
{
	struct tc654_data *data = tc654_update_client(cdev->devdata);

	if (IS_ERR(data))
		return PTR_ERR(data);

	return _set_pwm(data, clamp_val(state, 0, TC654_MAX_COOLING_STATE));
}

-- 
viresh

Re: [bug report] thermal: Add cooling device's statistics in sysfs

[Dan Carpenter]

On Mon, Oct 17, 2022 at 03:08:29PM +0530, Viresh Kumar wrote:
> On 14-10-22, 17:30, Dan Carpenter wrote:
> > (There are some drivers which might cap new_state.  I only looked at
> > tc654_set_cur_state() and it doesn't cap the value.  It's always going
> > to be safer to do the bounds checking in a central place).
> 
> I agree that it is better to do it at central place.
> 
> But, tc654_set_cur_state() seems to be doing it just fine:
> 
> static int tc654_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
> {
> 	struct tc654_data *data = tc654_update_client(cdev->devdata);
> 
> 	if (IS_ERR(data))
> 		return PTR_ERR(data);
> 
> 	return _set_pwm(data, clamp_val(state, 0, TC654_MAX_COOLING_STATE));

Ideally it would return -EINVAL for invalid state values instead of
clamping it to a valid range.

I feel like -EINVAL is generally a better API.  But also if we're
relying it to check and return an error code then obviously that's not
what it does.

regards,
dan carpenter

Re: [bug report] thermal: Add cooling device's statistics in sysfs

[Viresh Kumar]

On 17-10-22, 16:19, Dan Carpenter wrote:
> On Mon, Oct 17, 2022 at 03:08:29PM +0530, Viresh Kumar wrote:
> > On 14-10-22, 17:30, Dan Carpenter wrote:
> > > (There are some drivers which might cap new_state.  I only looked at
> > > tc654_set_cur_state() and it doesn't cap the value.  It's always going
> > > to be safer to do the bounds checking in a central place).
> > 
> > I agree that it is better to do it at central place.
> > 
> > But, tc654_set_cur_state() seems to be doing it just fine:
> > 
> > static int tc654_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
> > {
> > 	struct tc654_data *data = tc654_update_client(cdev->devdata);
> > 
> > 	if (IS_ERR(data))
> > 		return PTR_ERR(data);
> > 
> > 	return _set_pwm(data, clamp_val(state, 0, TC654_MAX_COOLING_STATE));
> 
> Ideally it would return -EINVAL for invalid state values instead of
> clamping it to a valid range.
> 
> I feel like -EINVAL is generally a better API.  But also if we're
> relying it to check and return an error code then obviously that's not
> what it does.

Right. Hope the patches I sent fixes this, can you have a look at them
too ?

-- 
viresh