Date: Tue, 4 Jun 2024 17:11:13 +0200
From: Jean Delvare <jdelvare@...e.de>
To: Wolfram Sang <wsa+renesas@...g-engineering.com>
Cc: linux-renesas-soc@...r.kernel.org, Baruch Siach <baruch@...s.co.il>,
 linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org, Peter Rosin
 <peda@...ntia.se>
Subject: Re: [PATCH] i2c: smbus: fix NULL function pointer dereference

Hi Wolfram,

Thanks for your answer.

On Tue, 4 Jun 2024 10:50:30 +0200, Wolfram Sang wrote:
> Hi Jean,
> 
> > I have a hard time establishing a formal link between the reported bug
> > and the commit listed above. I do understand that it wouldn't make
> > sense to register an i2c_adapter with neither .master_xfer nor
> > .smbus_xfer set before .reg_slave was added to struct i2c_algorithm,
> > but there were no checks in i2c-core preventing it from happening.  
> 
> Well, yes, correct.
> 
> > It was also possible for any (broken) device driver to call
> > __i2c_transfer() without first checking if plain I2C transfers were
> > actually supported by the i2c_adapter. I would argue that such an issue
> > should have been fixed at the device driver level by checking for the
> > I2C_FUNC_I2C functionality flag before calling __i2c_transfer(). That's
> > a theoretical issue though as I'm not aware of any device driver having
> > this issue.  
> 
> In theory, checking against I2C_FUNC_I2C should happen. In practice,
> most I2C drivers do not do this. Being picky here could results in bad
> user experience because of OOPS. If we really want to enforce checking
> I2C_FUNC_I2C, then we should have this safety net while we convert all
> users. No, actually, I think we always should have some safety nets.

Point taken, makes sense.

Note that we still want I2C_FUNC_I2C to be set properly, because it
allows device drivers to optimize transfers (the at24 driver is a prime
example of that) or even just to bind to the I2C bus (for device
drivers which properly check for it).

> > The call stack in Baruch's report shows that the real issue is with
> > i2c_smbus_xfer_emulated() being called with the i2c bus lock already
> > held, and thus having to call __i2c_transfer() instead of
> > i2c_transfer(). This code path did not exist before commit 63453b59e411
> > ("i2c: smbus: add unlocked __i2c_smbus_xfer variant"), which was added
> > in kernel v4.19. Therefore I claim that CVE-2024-35984 only affects
> > kernel v4.19 and newer. Do we agree on that?  
> 
> (There is a CVE for it??) For Baruch's case, this is true. But there are
> __i2c_transfer users all over the tree, they are all potentially
> vulnerable, or?

Yes there are many, but I think we shall differentiate between 2 cases:
* Missing check in a specific kernel device driver. These are unlikely
  to be a problem in practice because (1) these devices are typically
  instantiated explicitly, and such explicit code or device tree
  description would not exist in the first place if said device was not
  compatible with said I2C bus, and (2) if such an incompatibility was
  really present then it would have been spotted and fixed very
  quickly. Arbitrary binding through sysfs attributes is still possible
  but would definitely require root access and evil intentions (at
  which point we are screwed no matter what). I'm honestly not worried
  about this scenario.
* The issue being triggered from user-space through i2c-dev, which is
  what Baruch reported. The user doing that can target any arbitrary
  I2C bus and thus cause the oops by accident or even on purpose. For
  me this is what CVE-2024-35984 is about. What limits the attack
  surface here is that slave-only I2C buses are rare and you typically
  need to be root to use i2c-dev. But this is still a serious issue.

Also note that the first case could happen ever since __i2c_transfer()
was introduced (kernel v3.6, commit b37d2a3a75cb) and is not limited to
slave-only adapters, as any SMBus-only i2c_adapter would also be
vulnerable.

So the "Fixes:" tag in commit 91811a31b68d is incorrect for both
scenarios.

> (...)
> I see the performance penalty, yet I prefer handling the buggy driver
> gracefully because kicking off I2C transfers is not a hot path. Maybe we
> could turn the dev_dbg into something louder to make people aware that
> there is a bug?

My previous message initially had a suggestion in that direction ;-)
but I first wanted your opinion on the check itself. dev_dbg() is
definitely not appropriate for a condition which should never happen
and implies there's a bug somewhere else. A WARN_ON_ONCE would probably
be better, so that the bug gets spotted and fixed quickly.

-- 
Jean Delvare
SUSE L3 Support

Powered by blists - more mailing lists