Re: [PATCH] hwmon: (asus_atk0110) Check package count before accessing element

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The logic to restore the ACPI Embedded Controller (EC) state on driver teardown is inverted, causing the EC to be left in the wrong state.
- [High] Missing sysfs cache synchronization in `atk_read_value()` violating hwmon subsystem rules and leading to data races.
--

commit 5431f3f8e381507b8a466013109b4060a3814b41
Author: HyeongJun An <sammiee5311@gmail.com>

hwmon: (asus_atk0110) Check package count before accessing element

atk_ec_present() walks the management group package returned by the GGRP
ACPI method and, for each sub-package, reads its first element without
checking that the sub-package is non-empty. This patch adds a package.count
check to prevent out-of-bounds reads on malformed firmware packages.

> diff --git a/drivers/hwmon/asus_atk0110.c b/drivers/hwmon/asus_atk0110.c
> index 109318b0434d9..92afb64c09df8 100644
> --- a/drivers/hwmon/asus_atk0110.c
> +++ b/drivers/hwmon/asus_atk0110.c
> @@ -1037,6 +1037,9 @@ static int atk_ec_present(struct atk_data *data)
>  		if (obj->type != ACPI_TYPE_PACKAGE)
>  			continue;
>  
> +		if (!obj->package.count)
> +			continue;
> +
>  		id = &obj->package.elements[0];
>  		if (id->type != ACPI_TYPE_INTEGER)
>  			continue;

[Severity: High]
This is a pre-existing issue, but the logic to restore the ACPI Embedded
Controller (EC) state on driver teardown appears inverted.

In drivers/hwmon/asus_atk0110.c:atk_enumerate_new_hwmon(), the disable flag
is set like this:

	err = atk_ec_enabled(data);
	if (err < 0)
		return err;
	/* If the EC was disabled we will disable it again on unload */
	data->disable_ec = err;

If the EC was already enabled, atk_ec_enabled() returns 1, setting
data->disable_ec to 1. During module unload in atk_remove():

	if (data->disable_ec) {
		if (atk_ec_ctl(data, 0))
			dev_err(&pdev->dev, "Failed to disable EC\n");
	}

Does this mean we end up disabling the EC on unload if it was already enabled
prior to driver load, and leaving it enabled if it was disabled?

Could this asymmetric lifecycle handling break critical ACPI thermal and
power management functions on the motherboard by turning off the EC when it
should remain active?

[Severity: High]
This is also a pre-existing issue, but it looks like atk_read_value() misses
sysfs cache synchronization.

In drivers/hwmon/asus_atk0110.c:atk_read_value(), we see unprotected cache
updates:

	if (!sensor->is_valid ||
	    time_after(jiffies, sensor->last_updated + CACHE_TIME)) {
		// ... acpi evaluation ...
		sensor->is_valid = true;
		sensor->last_updated = jiffies;
		sensor->cached_value = *value;
	} else {
		*value = sensor->cached_value;
	}

Since hwmon subsystem guidelines explicitly require drivers to implement
locking for attributes registered manually, and these attributes are exposed
via hwmon_device_register_with_groups(), can concurrent sysfs reads execute
the underlying ACPI methods simultaneously?

Furthermore, could updating and reading the 64-bit sensor->cached_value
without locks on 32-bit systems result in torn reads, exposing absurdly high
or low sensor values to userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619122746.721981-1-sammiee5311@gmail.com?part=1