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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Short I2C reads return a positive value that bypasses the caller's error check, leading to uninitialized data access and cache bypass.
- [Medium] Off-by-one error in length bounds check allows reading outside the valid EC payload.
--

commit 04954fc98984d98f0039fee1c123ecdf16dc2ff7
Author: Michał Mirosław <mirq-linux@rere.qmqm.pl>

power: supply: Add driver for ASUS Transformer battery

This commit adds a new driver for ASUS Transformer batteries, which implements
one battery cell per EC controller and supports reading battery status for the
pad and mobile dock.

> 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..db6331352f7d4
> --- /dev/null
> +++ b/drivers/power/supply/asus-transformer-ec-battery.c
[ ... ]
> +static int asus_ec_battery_refresh(struct asus_ec_battery_data *priv)
> +{
> +	struct i2c_client *client = priv->ec->dockram;
> +	struct device *dev = &client->dev;
> +	int ret = 0;
> +
> +	if (time_before(jiffies, priv->batt_data_ts))
> +		return ret;
> +
> +	memset(priv->batt_data, 0, ASUSEC_ENTRY_BUFSIZE);
> +	ret = i2c_smbus_read_i2c_block_data(client, ASUSEC_DOCKRAM_BATT_CTL,
> +					    ASUSEC_ENTRY_SIZE, priv->batt_data);
> +	if (ret < ASUSEC_ENTRY_SIZE)
> +		return ret;

[Severity: Medium]
Since i2c_smbus_read_i2c_block_data() returns the number of bytes read on
success, could this return a positive value on a short read?

If this returns a positive value less than ASUSEC_ENTRY_SIZE, it bypasses
updating the batt_data_ts timestamp, which breaks the 5-second caching
mechanism and triggers an I2C transaction on every subsequent property read.

Additionally, asus_ec_battery_get_value() below only checks for errors with
if (ret < 0), which would fail to catch this positive short read value as
an error, potentially leading to the use of uninitialized data.

[ ... ]
> +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]
Does this bounds check properly account for the 16-bit read?

Since get_unaligned_le16() reads two bytes, if offs exactly equals
priv->batt_data[0] - 1, the bounds check passes. However, the second byte
read (offs + 1) would fall outside the valid payload length.

Could this silently combine one byte of valid data with a zero-padded byte
from the buffer, returning a corrupted value?

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