Re: [PATCH v1] hwmon: (acpi_power_meter) Check ACPI_COMPANION() against NULL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Subsystem Guideline Violation / Data Race: Missing and incorrect locking for extra attributes.
- [High] Subsystem Guideline Violation: Integer overflow in DIV_ROUND_CLOSEST bypasses boundary checks.
- [High] Subsystem Guideline Violation: Integer truncation during 64-bit power calculations.
--

commit 20d683b13caf3ad1f2da04d46b6d0bb7dff30a02
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

hwmon: (acpi_power_meter) Check ACPI_COMPANION() against NULL

Every platform driver can be forced to match a device that doesn't match
its list of device IDs because of device_match_driver_override(), so
platform drivers that rely on the existence of a device's ACPI companion
object need to verify its presence.

> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> index be7f702dcde9c..0c9b9f4180fb7 100644
> --- a/drivers/hwmon/acpi_power_meter.c
> +++ b/drivers/hwmon/acpi_power_meter.c
> @@ -884,10 +884,14 @@ static void acpi_power_meter_notify(acpi_handle handle, u32 event, void *data)
>  
>  static int acpi_power_meter_probe(struct platform_device *pdev)
>  {
> -	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
>  	struct acpi_power_meter_resource *resource;
> +	struct acpi_device *device;
>  	int res;
>  
> +	device = ACPI_COMPANION(&pdev->dev);
> +	if (!device)
> +		return -ENODEV;
> +
>  	resource = kzalloc_obj(*resource);
>  	if (!resource)
>  		return -ENOMEM;

These might not be regressions introduced by the new code, but the hwmon
subsystem guidelines explicitly mandate that drivers must implement locking
required for attributes registered by any other means, and should use
hwmon_lock() and hwmon_unlock() for this purpose.

Does this sysfs show function read the 64-bit res->trip array without
acquiring any locks?

drivers/hwmon/acpi_power_meter.c:power1_average_min_show() {
    ...
    if (res->trip[POWER_METER_TRIP_AVERAGE_MIN_IDX] < 0)
        return sysfs_emit(buf, "unknown\n");

    return sysfs_emit(buf, "%lld\n",
              res->trip[POWER_METER_TRIP_AVERAGE_MIN_IDX] * 1000);
}

On 32-bit architectures, can this lead to torn reads and expose corrupted
states to userspace if updated concurrently via store operations?

Also, is it incorrect to use the driver's own mutex_lock(&res->lock) instead
of the required hwmon_lock() API?

drivers/hwmon/acpi_power_meter.c:power1_average_min_store() {
    ...
    mutex_lock(&res->lock);
    ret = set_trip(res, POWER_METER_TRIP_AVERAGE_MIN_IDX, trip);
    mutex_unlock(&res->lock);
    ...
}

Can these DIV_ROUND_CLOSEST macros overflow the integer width and wrap around
to a very small number if a mistakenly large value is provided?

drivers/hwmon/acpi_power_meter.c:set_trip() {
    ...
    trip = DIV_ROUND_CLOSEST(trip, 1000);
    ...
}

drivers/hwmon/acpi_power_meter.c:set_cap() {
    ...
    cap = DIV_ROUND_CLOSEST(cap, 1000);
    ...
}

Since the macro evaluates using the expression x + 500, does this silently
bypass boundary checks like min_cap and incorrectly program the underlying
ACPI hardware limits?

Finally, could the multiplication of the 64-bit attribute res->power
(measured in milliwatts) by 1000 severely overflow the 32-bit bound on 32-bit
architectures?

drivers/hwmon/acpi_power_meter.c:power_meter_read() {
    ...
    case hwmon_power_average:
        ...
        *val = res->power * 1000;
        break;
    ...
}

Since *val is a long capable of representing up to ~2.14 billion on 32-bit
systems, can a power consumption of 3000 Watts result in returning
fundamentally incorrect, negative numbers to userspace due to the lack of
overflow clamping or checks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/5068745.GXAFRqVoOG@rafael.j.wysocki?part=1