Re: [PATCH] HID: wacom: validate battery range before scaling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hid: wacom: battery updates and initialization skipped on out-of-range values
--

commit 2de6d44ab126bd3e2386ea0695c8fb70e0ec0e11
Author: Aldo Ariel Panzardo <qwe.aldo@gmail.com>

    HID: wacom: validate battery range before scaling
    
    Battery events divide by the difference between the logical maximum
    and minimum without validating the range.  A malformed HID descriptor
    with equal limits can therefore trigger a divide-by-zero exception.
    
    Move percentage conversion to a helper that widens the arithmetic,
    rejects non-positive ranges and out-of-range results, and accounts for
    a nonzero logical minimum.

> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 8feb8027be95e..1d6a72320a6e1 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c

[ ... ]

> @@ -1952,6 +1953,24 @@ static void wacom_wac_battery_usage_mapping(struct hid_device *hdev,
>  	return;
>  }
>  
> +static bool wacom_wac_battery_percentage(struct hid_field *field, __s32 value,
> +					 __s32 *percentage)
> +{
> +	s64 range = (s64)field->logical_maximum - field->logical_minimum;
> +	s64 result;
> +
> +	if (range <= 0)
> +		return false;
> +
> +	result = div64_s64(((s64)value - field->logical_minimum) * 100,
> +			   range);
> +	if (result < 0 || result > 100)
> +		return false;

[Severity: Medium]
Will this aggressively drop events when a hardware ADC reports a raw value
slightly out of bounds (such as when fully charged or discharged)? 

Should this clamp the percentage to [0, 100] instead of returning false so
we don't drop the event entirely?

> +
> +	*percentage = result;
> +	return true;
> +}
> +
>  static void wacom_wac_battery_event(struct hid_device *hdev, struct hid_field *field,
>  		struct hid_usage *usage, __s32 value)
>  {
> @@ -1965,7 +1984,8 @@ static void wacom_wac_battery_event(struct hid_device *hdev, struct hid_field *f
>  			wacom_wac->hid_data.bat_status = POWER_SUPPLY_STATUS_UNKNOWN;
>  		}
>  		else {
> -			value = value * 100 / (field->logical_maximum - field->logical_minimum);
> +			if (!wacom_wac_battery_percentage(field, value, &value))
> +				return;

[Severity: Medium]
If the very first report is slightly out of bounds, could this early return
in wacom_wac_battery_event() skip setting the WACOM_QUIRK_BATTERY quirk below,
preventing the battery power supply from being registered at all?

>  			wacom_wac->hid_data.battery_capacity = value;
>  			wacom_wac->hid_data.bat_connected = 1;
>  			wacom_wac->hid_data.bat_status = WACOM_POWER_SUPPLY_STATUS_AUTO;
> @@ -1973,7 +1993,8 @@ static void wacom_wac_battery_event(struct hid_device *hdev, struct hid_field *f
>  		wacom_wac->features.quirks |= WACOM_QUIRK_BATTERY;
>  		break;
>  	case WACOM_HID_WD_BATTERY_LEVEL:
> -		value = value * 100 / (field->logical_maximum - field->logical_minimum);
> +		if (!wacom_wac_battery_percentage(field, value, &value))
> +			return;
>  		wacom_wac->hid_data.battery_capacity = value;
>  		wacom_wac->hid_data.bat_connected = 1;
>  		wacom_wac->hid_data.bat_status = WACOM_POWER_SUPPLY_STATUS_AUTO;

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