Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220628024155.2135990-1-zhipeng.wang_1@nxp.com> <Yryn8PdQIH7RaUwO@shikoro>
In-Reply-To: <Yryn8PdQIH7RaUwO@shikoro>
From:   Sami Tolvanen <samitolvanen@google.com>
Date:   Thu, 30 Jun 2022 08:54:35 -0700
Message-ID: <CABCJKud+iQ=Mo_-78hh-KsPAe9gtjqR0kxCTc3GOhkQd_F0GHA@mail.gmail.com>
Subject: Re: [PATCH] i2c: core: Disable i2c_generic_scl_recovery callback
 checks with CFI
To:     Wolfram Sang <wsa@kernel.org>,
        Zhipeng Wang <zhipeng.wang_1@nxp.com>,
        Kees Cook <keescook@chromium.org>
Cc:     linux-i2c@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jun 29, 2022 at 12:29 PM Wolfram Sang <wsa@kernel.org> wrote:
>
> On Tue, Jun 28, 2022 at 10:41:55AM +0800, Zhipeng Wang wrote:
> > CONFIG_CFI_CLANG breaks cross-module function address equality, which
> > breaks i2c_generic_scl_recovery as it compares a locally taken function
> > address to a one passed from a different module. Remove these sanity
> > checks for now.
>
> Can't we better fix a) the code or b) CFI?

Yes, we're working on fixing CFI:

https://lore.kernel.org/lkml/20220610233513.1798771-1-samitolvanen@google.com/

In the meantime, the possible workarounds are all more or less hacky.
Perhaps a slightly less intrusive alternative would be to add a
__cficanonical attribute to i2c_generic_scl_recovery and use the
function_nocfi() macro when referencing it elsewhere?

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index d43db2c3876e..dda93c5471f0 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -197,6 +197,11 @@ static int i2c_generic_bus_free(struct i2c_adapter *adap)
 #define RECOVERY_NDELAY                5000
 #define RECOVERY_CLK_CNT       9

+#ifdef CONFIG_CFI_CLANG
+#undef i2c_generic_scl_recovery
+#endif
+
+__cficanonical
 int i2c_generic_scl_recovery(struct i2c_adapter *adap)
 {
        struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fbda5ada2afc..7310cbdbd940 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -663,6 +663,10 @@ int i2c_recover_bus(struct i2c_adapter *adap);
 /* Generic recovery routines */
 int i2c_generic_scl_recovery(struct i2c_adapter *adap);

+#ifdef CONFIG_CFI_CLANG
+#define i2c_generic_scl_recovery function_nocfi(i2c_generic_scl_recovery)
+#endif
+
 /**
  * struct i2c_adapter_quirks - describe flaws of an i2c adapter
  * @flags: see I2C_AQ_* for possible flags and read below

Kees, any thoughts on the least terrible path forward here?

Zhipeng, if you want to test this on an older kernel, please note that
you'll also need to cherry-pick commit
e6f3b3c9c109ed57230996cf4a4c1b8ae7e36a81.

Sami