RE: [PATCH v2] i2c: tegra: check msg length in SMBUS block read

[Akhil R]

> > > For SMBUS block read, do not continue to read if the message length
> > > passed from the device is '0' or greater than the maximum allowed bytes.
> > >
> > > Signed-off-by: Akhil R <akhilrajeev@xxxxxxxxxx>
> > > ---
> > > v1->v2: Add check for the maximum data as well.
> > >
> > >  drivers/i2c/busses/i2c-tegra.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> > > index 87976e99e6d0..049b4d154c23 100644
> > > --- a/drivers/i2c/busses/i2c-tegra.c
> > > +++ b/drivers/i2c/busses/i2c-tegra.c
> > > @@ -1395,6 +1395,11 @@ static int tegra_i2c_xfer(struct i2c_adapter
> *adap,
> > struct i2c_msg msgs[],
> > >  			ret = tegra_i2c_xfer_msg(i2c_dev, &msgs[i],
> > MSG_END_CONTINUE);
> > >  			if (ret)
> > >  				break;
> > > +
> > > +			/* Validate message length before proceeding */
> > > +			if (msgs[i].buf[0] == 0 || msgs[i].buf[0] >
> > I2C_SMBUS_BLOCK_MAX)
> >
> > I wonder if this can ever happen. Looking at the implementation of the
> > i2c_smbus_{read,write}_i2c_block_data() functions, they already cap the
> > length at I2C_SMBUS_BLOCK_MAX.
> >
> > I suppose some user could be explicitly sending off messages with bad
> > lengths, but wouldn't it be better to return an error in that case
> > instead of just aborting silently?
> 
> For SMBUS read, if I understood it correctly, the check happens after the whole
> data is read. So, I believe it makes sense to abort the operation before an erroneous
> read. I have not verified this violation, but I think the error for I2C_SMBUS_BLOCK_MAX
> will also be printed at i2c_smbus_read_i2c_block_data() functions even though we
> return silently from the driver.
> 
> The check for '0' is not printed anywhere, but it is probably, okay? There is no data
> to be read anyway. Please let me know your thoughts.

For context, I was referring to the check in the function i2c_smbus_xfer_emulated() at the
line 502. This gets called for i2c_smbus_read_block_data() as well.

Regards,
Akhil