Re: [PATCH v3 1/8] hwmon: (pmbus/adm1266) cap PDIO scan in get_multiple at ADM1266_PDIO_NR

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Overwriting previously set GPIO state in `*bits`.
- [High] Uninitialized stack memory use due to insufficient validation of `i2c_smbus_read_block_data`.
- [High] Heap buffer overflow in `adm1266_pmbus_block_xfer`.
- [High] Heap buffer overflow in `adm1266_nvmem_read_blackbox` due to a missing bounds check.
- [High] Data race and unprotected shared state modification in `adm1266_nvmem_read`.
--

commit 850ac6b65baa3669e7d4f1e67c46fb676ed9a96c
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

hwmon: (pmbus/adm1266) cap PDIO scan in get_multiple at ADM1266_PDIO_NR

This commit modifies the GPIO get_multiple accessor to stop the PDIO scan
at the correct upper bound. While reviewing this change, a few other issues
were noticed in the surrounding code, some of which are addressed by later
commits in the series.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index d90f8f80be8e0..11f9a44f4361a 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -211,7 +211,7 @@ static int adm1266_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask
>  	status = read_buf[0] + (read_buf[1] << 8);
>  
>  	*bits = 0;

This isn't a bug introduced by this patch, but does this second assignment
to *bits unconditionally wipe out the GPIO states successfully collected in
the first loop for indices 0-8? 

I notice this is fixed later in the series by the commit "hwmon:
(pmbus/adm1266) don't clobber GPIO bits before PDIO read in get_multiple".

Also, is it possible for i2c_smbus_read_block_data() to return a short read
here?

drivers/hwmon/pmbus/adm1266.c:adm1266_gpio_get_multiple() {
    ...
	ret = i2c_smbus_read_block_data(data->client, ADM1266_PDIO_STATUS, read_buf);
	if (ret < 0)
		return ret;

	status = read_buf[0] + (read_buf[1] << 8);
    ...
}

If a short read occurs (returning 0 or 1 byte), does the status calculation
process uninitialized stack memory from read_buf? I see this is addressed
later in the series by "hwmon: (pmbus/adm1266) reject short block-read
responses in the GPIO accessors".

> -	for_each_set_bit_from(gpio_nr, mask, ADM1266_GPIO_NR + ADM1266_PDIO_STATUS) {
> +	for_each_set_bit_from(gpio_nr, mask, ADM1266_GPIO_NR + ADM1266_PDIO_NR) {
>  		if (test_bit(gpio_nr - ADM1266_GPIO_NR, &status))
>  			set_bit(gpio_nr, bits);
>  	}

This isn't a bug introduced by this patch, but does this loop lack bounds
checking against the destination buffer size?

drivers/hwmon/pmbus/adm1266.c:adm1266_nvmem_read_blackbox() {
    ...
	record_count = buf[3];

	for (index = 0; index < record_count; index++) {
		ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, read_buff);
		if (ret < 0)
			return ret;

		if (ret != ADM1266_BLACKBOX_SIZE)
			return -EIO;

		read_buff += ADM1266_BLACKBOX_SIZE;
	}
    ...
}

Since data->dev_mem is only 2048 bytes, if the hardware returns a
record_count greater than 32, does this overflow data->dev_mem?

Looking at the block transfer implementation, is there a potential heap buffer
overflow?

drivers/hwmon/pmbus/adm1266.c:adm1266_pmbus_block_xfer() {
    ...
	struct i2c_msg msgs[2] = {
        ...
		{
			.addr = client->addr,
			.flags = I2C_M_RD | I2C_M_DMA_SAFE,
			.buf = data->read_buf,
			.len = ADM1266_PMBUS_BLOCK_MAX + 2,
		}
	};
    ...
	ret = i2c_transfer(client->adapter, msgs, 2);
    ...
}

Since data->read_buf is allocated as ADM1266_PMBUS_BLOCK_MAX + 1 (256 bytes),
if a full block is read (or PEC is enabled and max data is received), could
i2c_transfer() overflow data->read_buf by writing 257 bytes into it?

Lastly, is there a missing lock protecting the shared data->dev_mem buffer
during reads?

drivers/hwmon/pmbus/adm1266.c:adm1266_nvmem_read() {
    ...
	if (offset == 0) {
		memset(data->dev_mem, 0, data->nvmem_config.size);

		ret = adm1266_nvmem_read_blackbox(data, data->dev_mem);
        ...
	}

	memcpy(val, data->dev_mem + offset, bytes);
    ...
}

Can a concurrent read at offset > 0 bypass initialization and read partially
updated or zeroed data while another thread executes memset and reads the
blackbox at offset == 0? 

I notice this is fixed later in the series by the commit "hwmon:
(pmbus/adm1266) serialize NVMEM blackbox read with pmbus_lock".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-adm1266-gpio-fixes-v3-0-e425e4f88139@nexthop.ai?part=1