Re: [PATCH] i2c: smbus: fix NULL function pointer dereference

[Wolfram Sang]

Hi Jean,

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

I agree. We definitely want I2C_FUNC_I2C to be set and make use of it as
much as possible. We should just not completely rely on it.

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

OK, can be argued.

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

Agreed.

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

Which makes handling this gracefully even more important.

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

Ack. Sorry! :)

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

So, are you okay with keeping the check where it is now and turning the
dev_dbg into WARN_ON_ONCE? I am.

All the best,

   Wolfram

Attachment: signature.asc
Description: PGP signature