linux-kernel - Re: [PATCH v1 1/4] hwmon: (acpi_power_meter) Fix using 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 for Android: free password hash cracker in your pocket

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

Message-ID: <aa6e1c02-b8bf-4d25-ad21-2018af72e16f@roeck-us.net>
Date: Mon, 25 Nov 2024 08:03:50 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Huisong Li <lihuisong@...wei.com>, linux-hwmon@...r.kernel.org,
 linux-kernel@...r.kernel.org
Cc: jdelvare@...e.com, liuyonglong@...wei.com, zhanjie9@...ilicon.com,
 zhenglifeng1@...wei.com
Subject: Re: [PATCH v1 1/4] hwmon: (acpi_power_meter) Fix using uninitialized
 variables

On 11/25/24 01:34, Huisong Li wrote:
> The 'power1_alarm' attribute uses the 'power' and 'cap' in the
> acpi_power_meter_resource structure. However, these two fields are just
> updated when user query 'power' and 'cap' attribute, or hardware enforced
> limit. If user directly query the 'power1_alarm' attribute without queryng
> above two attributes, driver will use the uninitialized variables to judge.
> In addition, the 'power1_alarm' attribute needs to update power and cap to
> show the real state.
> 
> Signed-off-by: Huisong Li <lihuisong@...wei.com>
> ---
>   drivers/hwmon/acpi_power_meter.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> index 2f1c9d97ad21..4c3314e35d30 100644
> --- a/drivers/hwmon/acpi_power_meter.c
> +++ b/drivers/hwmon/acpi_power_meter.c
> @@ -396,6 +396,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,6 +426,13 @@ static ssize_t show_val(struct device *dev,
>   			val = 0;
>   		break;
>   	case 6:
> +		ret = update_meter(resource);
> +		if (ret)
> +			return ret;
> +		ret = update_cap(resource);
> +		if (ret)
> +			return ret;
> +
>   		if (resource->power > resource->cap)
>   			val = 1;
>   		else


While technically correct, the implementation of this attribute defeats its
purpose. It is supposed to reflect the current status as reported by the
hardware. A real fix would be to use the associated notification to set or
reset a status flag, and to report the current value of that flag as reported
by the hardware.

If there is no notification support, the attribute should not even exist,
unless there is a means to retrieve its value from ACPI (the status itself,
not by comparing temperature values).

Guenter