| Message ID | 20250320132144.34764-1-akhilrajeev@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] i2c: tegra: check msg length in SMBUS block read | expand |
> > > 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@nvidia.com> > > > --- > > > 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
On Fri, Mar 21, 2025 at 01:09:32PM +0000, Akhil R wrote: > > > 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@nvidia.com> > > > --- > > > 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. I don't feel strongly either way. I think it's ultimately up to Wolfram and Andi to decide how they want host drivers to handle this. Naïvely I would say that it's better for the core to check for validity, if possible, and refuse invalid messages or transfers, so that host drivers don't have to repeat these checks. It's also not clear to me how this should be handled if multiple messages are submitted and one of them ends up being invalid. Multiple messages in one means they are probably a logically atomic set, so any error should impact all of them. However, these are all issues that can be resolved at a later point or not, and this patch looks correct (worst case it's doing too much checking), so: Acked-by: Thierry Reding <treding@nvidia.com>
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) + break; + /* Set the msg length from first byte */ msgs[i].len += msgs[i].buf[0]; dev_dbg(i2c_dev->dev, "reading %d bytes\n", msgs[i].len);
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@nvidia.com> --- v1->v2: Add check for the maximum data as well. drivers/i2c/busses/i2c-tegra.c | 5 +++++ 1 file changed, 5 insertions(+)