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

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

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