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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YsRkPUcrMj+JU0Om@kroah.com>
Date:   Tue, 5 Jul 2022 18:18:05 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Varad Gautam <varadgautam@...gle.com>
Cc:     linux-kernel@...r.kernel.org,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Amit Kucheria <amitk@...nel.org>,
        Zhang Rui <rui.zhang@...el.com>, linux-pm@...r.kernel.org,
        stable@...r.kernel.org
Subject: Re: [PATCH] thermal: sysfs: Perform bounds check when storing
 thermal states

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@...gle.com>
> Cc: stable@...r.kernel.org
> ---
>  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