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] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <PH7PR12MB81781DAABA5BF502A4CEAD72C0A42@PH7PR12MB8178.namprd12.prod.outlook.com>
Date: Mon, 24 Mar 2025 04:07:50 +0000
From: Akhil R <akhilrajeev@...dia.com>
To: Thierry Reding <thierry.reding@...il.com>
CC: Laxman Dewangan <ldewangan@...dia.com>, "digetx@...il.com"
	<digetx@...il.com>, "andi.shyti@...nel.org" <andi.shyti@...nel.org>, Jon
 Hunter <jonathanh@...dia.com>, "wsa@...nel.org" <wsa@...nel.org>,
	"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

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

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