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

[Simon Horman]

From: 'Simon Horman' <horms@xxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: sfp: extend SMBus support

This commit extends SFP module SMBus support by adding word and I2C
block access capabilities. It implements single read and write helpers
that pick the largest supported transfer per chunk and fall back to smaller
chunks as needed.

> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index e58e29a1e8d27..16d41d7ee6328 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
[ ... ]
> @@ -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?