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

[Simon Horman <horms@kernel.org>]

On Fri, May 15, 2026 at 03:55:22PM +0200, Jonas Jelonek wrote:
> Hi,
> 
> On 10.05.26 18:47, Simon Horman wrote:
> > From: 'Simon Horman' <horms@kernel.org>
> >
> > [ ... ]
> >> @@ -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.

Yes, that sounds good to me.

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