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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <PH7PR12MB8178EA71ED8DA6DF97749375C0AE2@PH7PR12MB8178.namprd12.prod.outlook.com>
Date: Thu, 3 Apr 2025 16:05:32 +0000
From: Akhil R <akhilrajeev@...dia.com>
To: Thierry Reding <thierry.reding@...il.com>, Wolfram Sang
	<wsa+renesas@...g-engineering.com>, Andi Shyti <andi.shyti@...nel.org>
CC: Laxman Dewangan <ldewangan@...dia.com>, "digetx@...il.com"
	<digetx@...il.com>, Jon Hunter <jonathanh@...dia.com>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] i2c: tegra: check msg length in SMBUS block read

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