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:
 <PH7PR12MB817882F6F4EEC820E22C092DC0DB2@PH7PR12MB8178.namprd12.prod.outlook.com>
Date: Fri, 21 Mar 2025 13:09:32 +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.

Regards,
Akhil