Re: [PATCH v10 6/7] power: supply: Add driver for ASUS Transformer battery

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The bounds check for battery property reading fails to account for the 2-byte read size, allowing the second byte to be read from logically invalid padding data.
- [Medium] Battery data cache is not invalidated on system resume, causing user-space to receive potentially stale battery readings.
--

--- Patch [6]: [PATCH v10 6/7] power: supply: Add driver for ASUS Transformer battery ---
commit 576ecafdda8cdf8e2dbf1964e8a1e9097bcefa55
Author: Michał Mirosław <[email protected]>

power: supply: Add driver for ASUS Transformer battery

This commit adds a new battery driver for Asus Transformer tablets and
mobile docks. It implements one battery cell per EC controller and
supports reading battery status via I2C.

> diff --git a/drivers/power/supply/asus-transformer-ec-battery.c b/drivers/power/supply/asus-transformer-ec-battery.c
> new file mode 100644
> index 0000000000000..4c0c6d4b09e2c
> --- /dev/null
> +++ b/drivers/power/supply/asus-transformer-ec-battery.c

[ ... ]

> +static int asus_ec_battery_get_value(struct asus_ec_battery_data *priv,
> +				     enum power_supply_property psp)
> +{
> +	int ret, offs;
> +
> +	guard(mutex)(&priv->battery_lock);
> +
> +	if (psp >= ARRAY_SIZE(asus_ec_battery_prop_offs))
> +		return -EINVAL;
> +
> +	offs = asus_ec_battery_prop_offs[psp];
> +	if (!offs)
> +		return -EINVAL;
> +
> +	ret = asus_ec_battery_refresh(priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (offs >= priv->batt_data[0])
> +		return -ENODATA;
> +
> +	return get_unaligned_le16(priv->batt_data + offs);

[Severity: Medium]
Should the bounds check account for the 2-byte read size?

If the EC returns a length where offs == priv->batt_data[0] - 1, the bounds
check offs >= priv->batt_data[0] will pass. 

Since get_unaligned_le16() reads two bytes, wouldn't it read a byte past the
logical data length and return corrupted data instead of -ENODATA?

> +}

[ ... ]

> +static int __maybe_unused asus_ec_battery_resume(struct device *dev)
> +{
> +	struct asus_ec_battery_data *priv = dev_get_drvdata(dev);
> +
> +	schedule_delayed_work(&priv->poll_work,
> +			      msecs_to_jiffies(ASUSEC_BATTERY_DATA_FRESH_MSEC));
> +
> +	return 0;
> +}

[Severity: Medium]
Should the data cache be explicitly invalidated here?

If priv->batt_data_ts is not reset on resume, and jiffies has not
sufficiently advanced past the expiration timestamp during suspend, could the
driver serve stale battery data to user-space from before the sleep?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6