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

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: <bk6rgqfcn5op5iuojoisogvtrp24ldblgkq4g62ffr4z7wnzug@xlp3ce5bx7bs>
Date: Tue, 4 Jun 2024 22:08:41 +0200
From: Wolfram Sang <wsa+renesas@...g-engineering.com>
To: Jean Delvare <jdelvare@...e.de>
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 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


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)