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

[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@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)
+				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);
-- 
2.43.2

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1505 bytes --]

On Thu, Mar 20, 2025 at 06:51:44PM +0530, 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?

Thierry

> +				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);
> -- 
> 2.43.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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@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.

Regards,
Akhil

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@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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2746 bytes --]

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>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Akhil R]

> 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>

Thanks, Thierry, for the Ack.

Hi Wolfram and Andi,
Could you share your thoughts on this change? Does it look good or needs some update?

Regards,
Akhil