thermal: Add cooling device's statistics in sysfs

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).

Quoted 3 lines · Dan Carpenter

(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));

Quoted 16 lines · Viresh Kumar

Quoted 3 lines · Dan Carpenter

(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);

Quoted 1 line

	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.

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.

Quoted 26 lines · 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]++;
                                                                      ^^^^^^^^^

Quoted 2 lines

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.