linux-kernel - Re: [PATCH] thermal: Add a sanity check for invalid state at stats update

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]

Date:   Tue, 7 Apr 2020 19:00:51 +0530
From:   Amit Kucheria <amit.kucheria@...durent.com>
To:     Takashi Iwai <tiwai@...e.de>
Cc:     Zhang Rui <rui.zhang@...el.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Linux PM list <linux-pm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] thermal: Add a sanity check for invalid state at stats update

On Mon, Mar 30, 2020 at 7:39 PM Takashi Iwai <tiwai@...e.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@...e.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
>

View attachment "0001-thermal-Reject-invalid-cur_state-input-from-userspac.patch" of type "text/x-patch" (1808 bytes)