lkml.org 
[lkml]   [2020]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [PATCH] thermal: Add a sanity check for invalid state at stats update
On Mon, Mar 30, 2020 at 7:39 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> The thermal sysfs handler keeps the statistics table with the fixed
> size that was determined from the initial max_states() call, and the
> table entry is updated at each sysfs cur_state write call. And, when
> the driver's set_cur_state() ops accepts the value given from
> user-space, the thermal sysfs core blindly applies it to the
> statistics table entry, which may overflow and cause an Oops.
> Although it's rather a bug in the driver's ops implementations, we
> shouldn't crash but rather give a proper warning instead.
>
> This patch adds a sanity check for avoiding such an OOB access and
> warns with a stack trace to show the suspicious device in question.

Hi Takashi,

Instead of this warning, I think we should reject such input when
writing to cur_state.

See attached patch. If you think this OK, I'll submit it.

Regards,
Amit

> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>
> We've hit some crash by stress tests, and this patch at least works
> around the crash itself. While the actual bug fix of the buggy driver
> is still being investigated, I submit the hardening in the core side
> at first.
>
> drivers/thermal/thermal_sysfs.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index aa99edb4dff7..a23c4e701d63 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -772,6 +772,11 @@ void thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev,
>
> spin_lock(&stats->lock);
>
> + if (dev_WARN_ONCE(&cdev->device, new_state >= stats->max_states,
> + "new state %ld exceeds max_state %ld",
> + new_state, stats->max_states))
> + goto unlock;
> +
> if (stats->state == new_state)
> goto unlock;
>
> --
> 2.16.4
>
From 54266260d483ab4476510dd4461a1cafc611e17d Mon Sep 17 00:00:00 2001
Message-Id: <54266260d483ab4476510dd4461a1cafc611e17d.1586266224.git.amit.kucheria@linaro.org>
From: Amit Kucheria <amit.kucheria@linaro.org>
Date: Tue, 7 Apr 2020 18:48:14 +0530
Subject: [PATCH] thermal: Reject invalid cur_state input from userspace

We don't check if the cur_state value input in sysfs is greater than the
maximum cooling state that the cooling device supports. This can cause
access to unallocated memory in case THERMAL_STATISTICS in enabled and
could also crash cooling devices that don't check for an invalid state in
their set_cur_state() callback.

Return an error if the state being requested in greater than the maximum
cooling state the device supports.

Reported-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
drivers/thermal/thermal_sysfs.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 7e1d11bdd258..8033e5a9386a 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -703,7 +703,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)
@@ -712,6 +712,13 @@ cur_state_store(struct device *dev, struct device_attribute *attr,
if ((long)state < 0)
return -EINVAL;

+ result = cdev->ops->get_max_state(cdev, &max_state);
+ if (result)
+ return result;
+
+ if (state >= max_state)
+ return -EINVAL;
+
mutex_lock(&cdev->lock);

result = cdev->ops->set_cur_state(cdev, state);
--
2.20.1
\
 
 \ /
  Last update: 2020-04-07 15:31    [W:0.072 / U:7.620 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site