linux-kernel - Re: [PATCH v2 1/3] hwmon: (acpi_power_meter) Fix uninitialized variables

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: <dfa0e28b-d25c-4170-958c-d5e8f5cbea39@roeck-us.net>
Date: Wed, 8 Jan 2025 06:45:49 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Huisong Li <lihuisong@...wei.com>, linux-hwmon@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, jdelvare@...e.com, liuyonglong@...wei.com,
 zhanjie9@...ilicon.com, zhenglifeng1@...wei.com
Subject: Re: [PATCH v2 1/3] hwmon: (acpi_power_meter) Fix uninitialized
 variables

On 1/7/25 03:33, Huisong Li wrote:
> The 'power1_alarm' attribute uses the 'power' and 'cap' in the
> acpi_power_meter_resource structure. Currently, these two fields are just
> updated when user query 'power' and 'cap' attribute. If user directly query
> the 'power1_alarm' attribute without queryng above two attributes, driver
> will use uninitialized variables to judge.
> 
> So this patch adds the setting of alarm state and update 'cap' in the
> notification callback and update 'power' and 'cap' if needed to show the
> real alarm state.
> 
> Signed-off-by: Huisong Li <lihuisong@...wei.com>
> ---
>   drivers/hwmon/acpi_power_meter.c | 27 +++++++++++++++++++++++----
>   1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> index 2f1c9d97ad21..0b337d4fb212 100644
> --- a/drivers/hwmon/acpi_power_meter.c
> +++ b/drivers/hwmon/acpi_power_meter.c
> @@ -84,6 +84,7 @@ struct acpi_power_meter_resource {
>   	u64		power;
>   	u64		cap;
>   	u64		avg_interval;
> +	bool		power_alarm;
>   	int			sensors_valid;
>   	unsigned long		sensors_last_updated;
>   	struct sensor_device_attribute	sensors[NUM_SENSORS];
> @@ -396,6 +397,9 @@ static ssize_t show_val(struct device *dev,
>   	struct acpi_device *acpi_dev = to_acpi_device(dev);
>   	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
>   	u64 val = 0;
> +	int ret;
> +
> +	guard(mutex)(&resource->lock);
>   
>   	switch (attr->index) {
>   	case 0:
> @@ -423,10 +427,17 @@ static ssize_t show_val(struct device *dev,
>   			val = 0;
>   		break;
>   	case 6:
> -		if (resource->power > resource->cap)
> -			val = 1;
> -		else
> -			val = 0;
> +		ret = update_meter(resource);
> +		if (ret)
> +			return ret;
> +		/* need to update cap if not to support the notification. */
> +		if (!(resource->caps.flags & POWER_METER_CAN_NOTIFY)) {
> +			ret = update_cap(resource);
> +			if (ret)
> +				return ret;
> +		}
> +		val = resource->power_alarm || resource->power > resource->cap;
> +		resource->power_alarm = resource->power > resource->cap ? true : false;

The result of the comparison is already a boolean. ? : is unnecessary.

>   		break;
>   	case 7:
>   	case 8:
> @@ -847,12 +858,20 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>   		sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
>   		break;
>   	case METER_NOTIFY_CAP:
> +		mutex_lock(&resource->lock);
> +		res = update_cap(resource);
> +		if (res)
> +			dev_err(&device->dev, "update cap failed when capping value is changed.\n");

Please make this dev_err_once() to avoid logging noise if the problem
is persistent.

> +		mutex_unlock(&resource->lock);
>   		sysfs_notify(&device->dev.kobj, NULL, POWER_CAP_NAME);
>   		break;
>   	case METER_NOTIFY_INTERVAL:
>   		sysfs_notify(&device->dev.kobj, NULL, POWER_AVG_INTERVAL_NAME);
>   		break;
>   	case METER_NOTIFY_CAPPING:
> +		mutex_lock(&resource->lock);
> +		resource->power_alarm = true;
> +		mutex_unlock(&resource->lock);
>   		sysfs_notify(&device->dev.kobj, NULL, POWER_ALARM_NAME);
>   		dev_info(&device->dev, "Capping in progress.\n");
>   		break;