Re: [PATCH net-next v7 2/2] net: sfp: extend SMBus support

[Jonas Jelonek]

Hi,

On 10.05.26 18:47, Simon Horman wrote:
> From: 'Simon Horman' <horms@xxxxxxxxxx>
>
> [ ... ]
>> @@ -756,50 +757,110 @@ static int sfp_i2c_write(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
> [ ... ]
>> +static int sfp_smbus_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
>> +			  size_t len)
>> +{
>> +	union i2c_smbus_data smbus_data;
>> +	u8 bus_addr = a2 ? 0x51 : 0x50;
>> +	size_t this_len, transferred;
>> +	u32 functionality;
>> +	u8 *data = buf;
>> +	int ret;
>> +
>> +	functionality = i2c_get_functionality(sfp->i2c);
>> +
>> +	while (len) {
>> +		this_len = min(len, sfp->i2c_block_size);
>> +
>> +		if (functionality & I2C_FUNC_SMBUS_READ_I2C_BLOCK) {
>> +			smbus_data.block[0] = this_len;
>> +			ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0,
>> +					     I2C_SMBUS_READ, dev_addr,
>> +					     I2C_SMBUS_I2C_BLOCK_DATA, &smbus_data);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			memcpy(data, &smbus_data.block[1], this_len);
> Can this sequence leak uninitialized kernel stack memory to the output buffer?
>
> When performing an I2C_SMBUS_I2C_BLOCK_DATA transfer, i2c_smbus_xfer()
> updates smbus_data.block[0] with the actual number of bytes read from the
> bus upon success.
>
> If the adapter or device returns fewer bytes than requested (a short read),
> smbus_data.block[0] will be less than this_len. Because memcpy()
> unconditionally copies this_len bytes, it appears it would overrun the valid
> data and copy uninitialized memory from the smbus_data stack variable.
>
> Since the SFP EEPROM data is accessible to userspace via ethtool, could this
> expose uninitialized kernel stack memory to userspace?
>
> Should the code verify that smbus_data.block[0] == this_len, or use
> smbus_data.block[0] as the actual transferred length?

Valid point, indeed uninitialized memory might be leaked here. I wouldn't
use the proposed fix though. Maybe I missed it but It doesn't seem to be
the contract that I2C/SMBus drivers need to update that field after read.
So using that for a check seems wrong. As a fix I will just make sure no
uninitialized memory is leaked.

If there are other suggestions please raise your hand, otherwise I'll
sned the follow-up soon.

Regards,
Jonas Jelonek