Received-SPF: pass (google.com: domain of linux-kernel+bounces-201336-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 4 Jun 2024 22:08:41 +0200
From: Wolfram Sang <wsa+renesas@sang-engineering.com>
To: Jean Delvare <jdelvare@suse.de>
Cc: linux-renesas-soc@vger.kernel.org, Baruch Siach <baruch@tkos.co.il>, 
	linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, Peter Rosin <peda@axentia.se>
Subject: Re: [PATCH] i2c: smbus: fix NULL function pointer dereference
Message-ID: <bk6rgqfcn5op5iuojoisogvtrp24ldblgkq4g62ffr4z7wnzug@xlp3ce5bx7bs>
Mail-Followup-To: Wolfram Sang <wsa+renesas@sang-engineering.com>, 
	Jean Delvare <jdelvare@suse.de>, linux-renesas-soc@vger.kernel.org, 
	Baruch Siach <baruch@tkos.co.il>, linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Peter Rosin <peda@axentia.se>
References: <20240426064408.7372-1-wsa+renesas@sang-engineering.com>
 <1e626d93f4220cc348300bbc61089de32300122d.camel@suse.de>
 <b2tnimag62ty6wndyjsy7u5fay6y52zn47vvifw6rh5abeqzpu@pqyyczutxcwu>
 <20240604171113.232628f9@endymion.delvare>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="4bhsazqdizvel7kz"
Content-Disposition: inline
In-Reply-To: <20240604171113.232628f9@endymion.delvare>


--4bhsazqdizvel7kz
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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?
>=20
> 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?
>=20
> 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


--4bhsazqdizvel7kz
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEOZGx6rniZ1Gk92RdFA3kzBSgKbYFAmZfdEUACgkQFA3kzBSg
Kba6Tw//ZXtwgs1Z64LmhwEfpaTqY+W5nFX6jknwWjmZovfxNwQw8d/YqCuQKKfP
07P18mNfynJxN/H/QQZi6QW4XSyW0vEv18co4QNmW9YRRWRcyT18U0Onhf4NzwH2
uZBta6T2zoPiCAAiALKUCXCg1KTG0rm2usWZzuBG6dtlNnTQtmbNj6GyqTg9+NkB
i5Il7D9Vq2OvryNAe0dBgJYbd58rESLNeTYk99+4vqzR+bzTt3DPWvwuFa8JBEaH
RvQHHEMxYol3Jdp60WwRaWDIvMvFdVT6qmxkrSZDTLGSw3VwZdJ+YMY1B2dPAXPr
JFPwNEQftVaf7uRFj7lsBqgtzHkDtvAWbJtZP4DCdaMPOZKSV4oc0qm4iF8MV/Oj
jtBWFEJ68H4DCd89l2Y4I5EbA8eTpDRHQNLsjI7cMw5+bg8/z71rqSLa5nLNetLY
roHH/3+hMvOACaxKxv7EVGGZXoSfTid/hp7CpSciiwcI5h8uFkEnw3QHWgyS7b7v
EORXybfUyv/CCPJNybhoJdCsupmtFohn48NiWr1fVcvpCimvpiTatXuBP7uuOspN
EwqUW31YYobHybDmaIhD7DetIWPwGMZItcztkUfM3T7LbWFxGh44mbmGdfEobscT
AW0o7GioHbwSeU78vAUrjje8hVmmQwJE1DQ8gda/RtmFmjQEmoY=
=8vSS
-----END PGP SIGNATURE-----

--4bhsazqdizvel7kz--