Re: [PATCH 2/4] i2c: cgbc: Add virtual I2C bus support

On 8/4/26 5:43 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Uninitialized stack variable `data` used as a denominator, leading to a potential division by zero panic.
> --
> 
> --- Patch [2]: [PATCH 2/4] i2c: cgbc: Add virtual I2C bus support ---
> commit 68db5c3b940f7f50540127b0266b86e6b3d69b22
> Author: Thomas Richard <[email protected]>
> 
> i2c: cgbc: Add virtual I2C bus support
> 
> Internally the Board Controller has a virtual I2C bus with some virtual
> storage devices on it. Add support for it by defining a i2c_adapter entry.
> This virtual bus has a fixed frequency that cannot be changed.
> 
>> diff --git a/drivers/i2c/busses/i2c-cgbc.c b/drivers/i2c/busses/i2c-cgbc.c
>> index 25a74fa51aa00..c3f1c83b105a8 100644
>> --- a/drivers/i2c/busses/i2c-cgbc.c
>> +++ b/drivers/i2c/busses/i2c-cgbc.c
> 
> [ ... ]
> 
>> @@ -147,11 +148,14 @@ static int cgbc_i2c_set_frequency(struct i2c_adapter *adap,
>>  	cmd[0] = CGBC_I2C_CMD_SPEED | algo_data->bus_id;
>>  	cmd[1] = cgbc_i2c_freq_to_reg(bus_frequency);
>>  
>> -	ret = cgbc_command(cgbc, &cmd, sizeof(cmd), &data, 1, NULL);
>> -	if (ret)
>> -		return dev_err_probe(i2c->dev, ret,
>> -				     "Failed to initialize I2C bus %s",
>> -				     adap->name);
>> +	/* Virtual bus has a fixed frequency */
>> +	if (algo_data->bus_id != CGBC_I2C_VIRTUAL_BUS_ID) {
>> +		ret = cgbc_command(cgbc, &cmd, sizeof(cmd), &data, 1, NULL);
>> +		if (ret)
>> +			return dev_err_probe(i2c->dev, ret,
>> +					     "Failed to initialize I2C bus %s",
>> +					     adap->name);
>> +	}
>>  
>>  	cmd[1] = 0x00;
> 
> [Severity: High]
> This is a pre-existing issue, but I noticed a potential problem in how
> cgbc_i2c_set_frequency() handles the data variable.
> 
> Could this lead to a division by zero panic? The data variable is allocated on
> the stack uninitialized:
> 
> drivers/i2c/busses/i2c-cgbc.c:cgbc_i2c_set_frequency() {
>     ...
>     u8 cmd[2], data;
>     ...
> }
> 
> When passed to cgbc_command(), if the board controller firmware returns a
> successful status but provides a zero byte count, cgbc_command() skips reading
> into data and returns 0:
> 
> drivers/mfd/cgbc-core.c:cgbc_command() {
>     ...
>     case CGBC_STATUS_DATA_READY:
>         if (istatus > data_size)
>             istatus = data_size;
>         for (i = 0; i < istatus; i++) {
>             _data[i] = ioread8(cgbc->io_cmd + CGBC_CMD_DATA + ((i + 1) % 4));
>     ...
> }
> 
> If istatus is 0, the loop is skipped and data remains untouched.
> 
> When execution continues in cgbc_i2c_set_frequency(), this uninitialized
> value is converted to a frequency:
> 
> drivers/i2c/busses/i2c-cgbc.c:cgbc_i2c_set_frequency() {
>     ...
>     ret = cgbc_command(cgbc, &cmd, sizeof(cmd), &data, 1, NULL);
>     if (ret)
>         return dev_err_probe(...);
> 
>     bus_frequency = cgbc_i2c_reg_to_freq(data);
>     ...
>     algo_data->read_maxtime_us = (BITS_PER_BYTE + 1) * CGBC_I2C_READ_MAX_LEN
>         * USEC_PER_SEC / bus_frequency;
>     ...
> }
> 
> If the uninitialized stack memory evaluates to 0, or if the hardware
> explicitly returns a frequency of 0, would this result in a division by zero
> during the read_maxtime_us calculation?

The answer is still no. If CGBC_I2C_CMD_SPEED succeeds (and we check
cgbc_command's return code), it returns exactly 1 byte. We provided a
data buffer with size 1 to cgbc_command, so there is no way for
uninitialized stack memory to be read if the command succeeded.

To prevent buffer overflow, cgbc_command verifies that the buffer is
large enough to hold all received data.

Best Regards,
Thomas