Re: [PATCH] thermal: sysfs: Perform bounds check when storing thermal states

[Greg KH]

On Tue, Jul 05, 2022 at 03:00:02PM +0000, Varad Gautam wrote:
> Check that a user-provided thermal state is within the maximum
> thermal states supported by a given driver before attempting to
> apply it. This prevents a subsequent OOB access in
> thermal_cooling_device_stats_update() while performing
> state-transition accounting on drivers that do not have this check
> in their set_cur_state() handle.
> 
> Signed-off-by: Varad Gautam <varadgautam@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
>  drivers/thermal/thermal_sysfs.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index 1c4aac8464a7..0c6b0223b133 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -607,7 +607,7 @@ cur_state_store(struct device *dev, struct device_attribute *attr,
>  		const char *buf, size_t count)
>  {
>  	struct thermal_cooling_device *cdev = to_cooling_device(dev);
> -	unsigned long state;
> +	unsigned long state, max_state;
>  	int result;
>  
>  	if (sscanf(buf, "%ld\n", &state) != 1)
> @@ -618,10 +618,20 @@ cur_state_store(struct device *dev, struct device_attribute *attr,
>  
>  	mutex_lock(&cdev->lock);
>  
> +	result = cdev->ops->get_max_state(cdev, &max_state);
> +	if (result)
> +		goto unlock;
> +
> +	if (state > max_state) {
> +		result = -EINVAL;
> +		goto unlock;
> +	}
> +
>  	result = cdev->ops->set_cur_state(cdev, state);

Why doesn't set_cur_state() check the max state before setting it?  Why
are the callers forced to always check it before?  That feels wrong...

thanks,

greg k-h