Message-ID: <b2tnimag62ty6wndyjsy7u5fay6y52zn47vvifw6rh5abeqzpu@pqyyczutxcwu>
Date: Tue, 4 Jun 2024 10:50:30 +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,

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

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

> > +       if (!adap->algo->master_xfer) {
> > +               dev_dbg(&adap->dev, "I2C level transfers not supported\n");
> > +               return -EOPNOTSUPP;
> > +       }
> > +
> 
> Not related specifically to this commit, as it is only moving a check
> which already existed before, but this looks inefficient to me.
> 
> We end up performing the check with every I2C-level transfer, while the
> availability of such support can almost always be checked once and for
> all in the I2C device driver (i2c-dev and i2c_smbus_xfer_emulated being
> the exceptions).
> 
> I see two ways for us to reach this check:
> * __i2c_transfer() or i2c_transfer() gets called directly by a device
> driver. This driver should have checked for the I2C_FUNC_I2C
> functionality flag before calling either function. If they did not,
> it's a driver bug, which should be fixed in the driver in question.

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?

> Note that i2c-dev currently lacks this check, I think it should be
> added.

True, it should be a role-model of a good citizen :)

> * __i2c_transfer() gets called by i2c_smbus_xfer_emulated(). We should
> add a check for I2C_FUNC_I2C in __i2c_smbus_xfer() before calling this
> function. This is more or less what Baruch proposed initially, and I
> think it would have been a more efficient fix.

As I said above, more efficient but not thorough. One driver not
checking I2C_FUNC_I2C and boom...

> And if you are concerned about functionality flags not being set
> properly (specifically I2C_FUNC_I2C being set while .master_xfer isn't
> set [1]) then we should add a consistency check at i2c_adapter
> registration time, so again it's done once and for all and we don't
> have to check again and again at transfer time.

I think this check is worth to have. It is not complete because drivers
may still not check the flag, but it is one step to be more robust.

> Or is this optimization not worth it?

I think so. It is one pointer check against a kernel oopsing somewhere
somewhen.

> [1] BTW, looking at the only two in-tree slave-only I2C adapter
> drivers, i2c-at91-slave and i2c-designware-slave, both are setting
> functionality flags other than I2C_FUNC_SLAVE. Unless I don't
> understand how the slave functionality works, this is a bug. I'll
> prepare and post patches later today.

Thanks for doing that!

Happy hacking,

   Wolfram


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

Powered by blists - more mailing lists