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
