All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jean Delvare <jdelvare@suse.de>
To: Wolfram Sang <wsa+renesas@sang-engineering.com>,
	 linux-renesas-soc@vger.kernel.org
Cc: 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
Date: Thu, 30 May 2024 15:24:58 +0200	[thread overview]
Message-ID: <1e626d93f4220cc348300bbc61089de32300122d.camel@suse.de> (raw)
In-Reply-To: <20240426064408.7372-1-wsa+renesas@sang-engineering.com>

Hi Wolfram, Baruch, Peter,

On Fri, 2024-04-26 at 08:44 +0200, Wolfram Sang wrote:
> Brauch reported an OOPS when using the designware controller as target
> only. Target-only modes break the assumption of one transfer function
> always being available. Fix this by always checking the pointer in
> __i2c_transfer.

I was asked to backport this fix to our oldest kernel branches, so I
looked into it and I have comments and a question.

> Reported-by: Baruch Siach <baruch@tkos.co.il>
> Closes: https://lore.kernel.org/r/4269631780e5ba789cf1ae391eec1b959def7d99.1712761976.git.baruch@tkos.co.il
> Fixes: 4b1acc43331d ("i2c: core changes for slave support")

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.

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.

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?

> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c  | 12 ++++++------
>  drivers/i2c/i2c-core-smbus.c |  2 +-
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index ff5c486a1dbb..db0d1ac82910 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -2200,13 +2200,18 @@ static int i2c_check_for_quirks(struct i2c_adapter *adap, struct i2c_msg *msgs,
>   * Returns negative errno, else the number of messages executed.
>   *
>   * Adapter lock must be held when calling this function. No debug logging
> - * takes place. adap->algo->master_xfer existence isn't checked.
> + * takes place.
>   */
>  int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  {
>         unsigned long orig_jiffies;
>         int ret, try;
>  
> +       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.
Note that i2c-dev currently lacks this check, I think it should be
added.
* __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.

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.

Or is this optimization not worth it?

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


-- 
Jean Delvare
SUSE L3 Support

  parent reply	other threads:[~2024-05-30 13:25 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-26  6:44 [PATCH] i2c: smbus: fix NULL function pointer dereference Wolfram Sang
2024-04-26  7:28 ` Sergei Shtylyov
2024-04-26  8:39   ` Wolfram Sang
2024-04-26  8:32 ` Baruch Siach
2024-04-26  9:52 ` Wolfram Sang
2024-05-30 13:24 ` Jean Delvare [this message]
2024-06-04  8:50   ` Wolfram Sang
2024-06-04 15:11     ` Jean Delvare
2024-06-04 20:08       ` Wolfram Sang
2024-06-05  9:20         ` Jean Delvare
  -- strict thread matches above, loose matches on Subject: below --
2024-04-10 15:12 Baruch Siach
2024-04-26  6:43 ` Wolfram Sang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1e626d93f4220cc348300bbc61089de32300122d.camel@suse.de \
    --to=jdelvare@suse.de \
    --cc=baruch@tkos.co.il \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=peda@axentia.se \
    --cc=wsa+renesas@sang-engineering.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.