Re: [PATCH] drivers: thermal: Add NULL pointer check before using cooling device stats

[manafm]

On 2020-12-05 16:42, Daniel Lezcano wrote:
> On 04/12/2020 20:09, Manaf Meethalavalappu Pallikunhi wrote:
> > There is a possible chance that some cooling device stats buffer
> > allocation fails due to very high cooling device max state value.
> > Later cooling device update or cooling stats sysfs will try to
> > access stats data for the same cooling device. It will lead to
> > NULL pointer dereference issue.
> >
> > Add a NULL pointer check before accessing thermal cooling device
> > stats data. It fixes the following bug
> >
> > [ 26.812833] Unable to handle kernel NULL pointer dereference at 
> > virtual address 0000000000000004
> > [ 27.122960] Call trace:
> > [ 27.122963] do_raw_spin_lock+0x18/0xe8
> > [ 27.122966] _raw_spin_lock+0x24/0x30
> > [ 27.128157] thermal_cooling_device_stats_update+0x24/0x98
> > [ 27.128162] cur_state_store+0x88/0xb8
> > [ 27.128166] dev_attr_store+0x40/0x58
> > [ 27.128169] sysfs_kf_write+0x50/0x68
> > [ 27.133358] kernfs_fop_write+0x12c/0x1c8
> > [ 27.133362] __vfs_write+0x54/0x160
> > [ 27.152297] vfs_write+0xcc/0x188
> > [ 27.157132] ksys_write+0x78/0x108
> > [ 27.162050] ksys_write+0xf8/0x108
> > [ 27.166968] __arm_smccc_hvc+0x158/0x4b0
> > [ 27.166973] __arm_smccc_hvc+0x9c/0x4b0
> > [ 27.186005] el0_svc+0x8/0xc
> >
> > Signed-off-by: Manaf Meethalavalappu Pallikunhi 
> > <manafm@xxxxxxxxxxxxxx>
>
> The only place where it can crash is when the
> thermal_cooling_device_stats_update() function is called.
>
> The other places in show*/store* in the stats directory are 
> inaccessible
> as the sysfs entry is not showed up due to the
> thermal_cooling_device_stats_setup() failing.
>
> It would have been nice if the thermal_cooling_device_stats_update() 
> was
> not called at all but I don't see how we can do that without static 
> keys
> which is overkill for a degraded mode.
>
> I guess having the kzallocation warning in the console output is enough
> to warn the user the system is working without the stats for the 
> cooling
> device. That should not prevent the system functioning.
>
> Can you resend with the check in thermal_cooling_device_stats_update() 
> only?
>
> Thanks
>
>   -- Daniel
Thanks for your review, I double checked it, yes, no need to add NULL 
check in stats sysfs show*/store* functions.
I will update this in V2

Thanks
Manaf
> > ---
> >  drivers/thermal/thermal_sysfs.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/thermal/thermal_sysfs.c 
> > b/drivers/thermal/thermal_sysfs.c
> > index 473449b..a5e4855 100644
> > --- a/drivers/thermal/thermal_sysfs.c
> > +++ b/drivers/thermal/thermal_sysfs.c
> > @@ -827,6 +827,9 @@ void thermal_cooling_device_stats_update(struct 
> > thermal_cooling_device *cdev,
> >  {
> >  	struct cooling_dev_stats *stats = cdev->stats;
> >
> > +	if (!stats)
> > +		return;
> > +
> >  	spin_lock(&stats->lock);
> >
> >  	if (stats->state == new_state)
> > @@ -848,6 +851,9 @@ static ssize_t total_trans_show(struct device 
> > *dev,
> >  	struct cooling_dev_stats *stats = cdev->stats;
> >  	int ret;
> >
> > +	if (!stats)
> > +		return -ENODEV;
> > +
> >  	spin_lock(&stats->lock);
> >  	ret = sprintf(buf, "%u\n", stats->total_trans);
> >  	spin_unlock(&stats->lock);
> > @@ -864,6 +870,9 @@ time_in_state_ms_show(struct device *dev, struct 
> > device_attribute *attr,
> >  	ssize_t len = 0;
> >  	int i;
> >
> > +	if (!stats)
> > +		return -ENODEV;
> > +
> >  	spin_lock(&stats->lock);
> >  	update_time_in_state(stats);
> >
> > @@ -882,8 +891,12 @@ reset_store(struct device *dev, struct 
> > device_attribute *attr, const char *buf,
> >  {
> >  	struct thermal_cooling_device *cdev = to_cooling_device(dev);
> >  	struct cooling_dev_stats *stats = cdev->stats;
> > -	int i, states = stats->max_states;
> > +	int i, states;
> > +
> > +	if (!stats)
> > +		return -ENODEV;
> >
> > +	states = stats->max_states;
> >  	spin_lock(&stats->lock);
> >
> >  	stats->total_trans = 0;
> > @@ -907,6 +920,9 @@ static ssize_t trans_table_show(struct device 
> > *dev,
> >  	ssize_t len = 0;
> >  	int i, j;
> >
> > +	if (!stats)
> > +		return -ENODEV;
> > +
> >  	len += snprintf(buf + len, PAGE_SIZE - len, " From  :    To\n");
> >  	len += snprintf(buf + len, PAGE_SIZE - len, "       : ");
> >  	for (i = 0; i < stats->max_states; i++) {