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

[Dan Carpenter <dan.carpenter@oracle.com>]

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