| 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
| ||
|
Date: Wed, 9 Nov 2022 20:26:59 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Guenter Roeck <linux@...ck-us.net>
Cc: "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,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 7/9] thermal/core: Protect sysfs accesses to thermal
operations with thermal zone mutex
On Mon, Oct 17, 2022 at 3:09 PM Guenter Roeck <linux@...ck-us.net> wrote:
>
> Protect access to thermal operations against thermal zone removal by
> acquiring the thermal zone device mutex. After acquiring the mutex, check
> if the thermal zone device is registered and abort the operation if not.
>
> With this change, we can call __thermal_zone_device_update() instead of
> thermal_zone_device_update() from trip_point_temp_store() and from
> emul_temp_store(). Similar, we can call __thermal_zone_set_trips() instead
> of thermal_zone_set_trips() from trip_point_hyst_store().
>
> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
> ---
> drivers/thermal/thermal_core.c | 4 +-
> drivers/thermal/thermal_core.h | 2 +
> drivers/thermal/thermal_sysfs.c | 77 ++++++++++++++++++++++++++++-----
> 3 files changed, 69 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 9facd9c5b70f..b8e3b262b2bd 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -403,8 +403,8 @@ static void thermal_zone_device_init(struct thermal_zone_device *tz)
> pos->initialized = false;
> }
>
> -static void __thermal_zone_device_update(struct thermal_zone_device *tz,
> - enum thermal_notify_event event)
> +void __thermal_zone_device_update(struct thermal_zone_device *tz,
> + enum thermal_notify_event event)
> {
> int count;
>
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 1571917bd3c8..7b51b9a22e7e 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -109,6 +109,8 @@ int thermal_register_governor(struct thermal_governor *);
> void thermal_unregister_governor(struct thermal_governor *);
> int thermal_zone_device_set_policy(struct thermal_zone_device *, char *);
> int thermal_build_list_of_policies(char *buf);
> +void __thermal_zone_device_update(struct thermal_zone_device *tz,
> + enum thermal_notify_event event);
>
> /* Helpers */
> void thermal_zone_set_trips(struct thermal_zone_device *tz);
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index ec495c7dff03..68607e6070e8 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -92,7 +92,15 @@ trip_point_type_show(struct device *dev, struct device_attribute *attr,
> if (sscanf(attr->attr.name, "trip_point_%d_type", &trip) != 1)
> return -EINVAL;
>
> + mutex_lock(&tz->lock);
> +
> + if (!device_is_registered(dev)) {
> + mutex_unlock(&tz->lock);
> + return -ENODEV;
> + }
> +
> result = tz->ops->get_trip_type(tz, trip, &type);
> + mutex_unlock(&tz->lock);
> if (result)
> return result;
>
> @@ -128,10 +136,17 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
> if (kstrtoint(buf, 10, &temperature))
> return -EINVAL;
>
> + mutex_lock(&tz->lock);
> +
> + if (!device_is_registered(dev)) {
> + ret = -ENODEV;
> + goto unlock;
> + }
> +
> if (tz->ops->set_trip_temp) {
> ret = tz->ops->set_trip_temp(tz, trip, temperature);
> if (ret)
> - return ret;
> + goto unlock;
> }
>
> if (tz->trips)
> @@ -140,18 +155,21 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr,
> if (tz->ops->get_trip_hyst) {
> ret = tz->ops->get_trip_hyst(tz, trip, &hyst);
> if (ret)
> - return ret;
> + goto unlock;
> }
>
> ret = tz->ops->get_trip_type(tz, trip, &type);
> if (ret)
> - return ret;
> + goto unlock;
>
> thermal_notify_tz_trip_change(tz->id, trip, type, temperature, hyst);
>
> - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> + __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
>
> - return count;
> +unlock:
> + mutex_unlock(&tz->lock);
> +
> + return ret ? ret : count;
> }
>
> static ssize_t
> @@ -168,12 +186,19 @@ trip_point_temp_show(struct device *dev, struct device_attribute *attr,
> if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip) != 1)
> return -EINVAL;
>
> + mutex_lock(&tz->lock);
> +
> + if (!device_is_registered(dev)) {
> + ret = -ENODEV;
> + goto unlock;
> + }
> +
> ret = tz->ops->get_trip_temp(tz, trip, &temperature);
>
Well, this is a pattern I've already talked about twice, so let me
just mention it here.
> - if (ret)
> - return ret;
> +unlock:
> + mutex_unlock(&tz->lock);
>
> - return sprintf(buf, "%d\n", temperature);
> + return ret ? ret : sprintf(buf, "%d\n", temperature);
And I wouldn't change this (like in the other patch I've commented).
> }
>
> static ssize_t
> @@ -193,6 +218,13 @@ trip_point_hyst_store(struct device *dev, struct device_attribute *attr,
> if (kstrtoint(buf, 10, &temperature))
> return -EINVAL;
>
> + mutex_lock(&tz->lock);
> +
> + if (!device_is_registered(dev)) {
> + ret = -ENODEV;
> + goto unlock;
> + }
> +
> /*
> * We are not doing any check on the 'temperature' value
> * here. The driver implementing 'set_trip_hyst' has to
> @@ -201,7 +233,10 @@ trip_point_hyst_store(struct device *dev, struct device_attribute *attr,
> ret = tz->ops->set_trip_hyst(tz, trip, temperature);
>
> if (!ret)
> - thermal_zone_set_trips(tz);
> + __thermal_zone_set_trips(tz);
> +
> +unlock:
> + mutex_unlock(&tz->lock);
>
> return ret ? ret : count;
> }
> @@ -220,8 +255,18 @@ trip_point_hyst_show(struct device *dev, struct device_attribute *attr,
> if (sscanf(attr->attr.name, "trip_point_%d_hyst", &trip) != 1)
> return -EINVAL;
>
> + mutex_lock(&tz->lock);
> +
> + if (!device_is_registered(dev)) {
> + ret = -ENODEV;
> + goto unlock;
> + }
> +
> ret = tz->ops->get_trip_hyst(tz, trip, &temperature);
Same pattern again.
> +unlock:
> + mutex_unlock(&tz->lock);
> +
> return ret ? ret : sprintf(buf, "%d\n", temperature);
> }
>
> @@ -269,16 +314,24 @@ emul_temp_store(struct device *dev, struct device_attribute *attr,
> if (kstrtoint(buf, 10, &temperature))
> return -EINVAL;
>
> + mutex_lock(&tz->lock);
> +
> + if (!device_is_registered(dev)) {
> + ret = -ENODEV;
> + goto unlock;
> + }
> +
> if (!tz->ops->set_emul_temp) {
> - mutex_lock(&tz->lock);
> tz->emul_temperature = temperature;
> - mutex_unlock(&tz->lock);
> } else {
> ret = tz->ops->set_emul_temp(tz, temperature);
> }
Drop the leftover braces that are not necessary now?
>
> if (!ret)
> - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> + __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> +
> +unlock:
> + mutex_unlock(&tz->lock);
>
> return ret ? ret : count;
> }
> --
Powered by blists - more mailing lists