[PATCH] i2c: core: Disable i2c_generic_scl_recovery callback checks with CFI

[PATCH] i2c: core: Disable i2c_generic_scl_recovery callback checks with CFI

[Zhipeng Wang]

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.

Trigger I2C recovery through SCL connection LOW.

[    2.452853] Kernel panic - not syncing: CFI failure (target: 0x0)
[    2.476964] Call trace:
[    2.480104] dump_backtrace.cfi_jt+0x0/0x8
[    2.484909] dump_stack_lvl+0x80/0xb8
[    2.489273] panic+0x180/0x444
[    2.489282] find_check_fn+0x0/0x218
[    2.500079] i2c_generic_scl_recovery+0x390/0x400
[    2.509573] i2c_recover_bus+0x3c/0x84
[    2.509582] i2c_imx_xfer_common+0x60/0xd50 [i2c_imx]
[    2.521005] i2c_imx_xfer+0x1ac/0x3ec [i2c_imx]
[    2.531196] __i2c_transfer+0x2b8/0x910
[    2.531208] i2c_transfer+0xd8/0x1cc
[    2.531216] regmap_i2c_write+0x54/0xa4

Signed-off-by: Zhipeng Wang <zhipeng.wang_1@nxp.com>
---
 drivers/i2c/i2c-core-base.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index d43db2c3876e..87bca6d8514b 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -334,7 +334,8 @@ static int i2c_gpio_init_generic_recovery(struct i2c_adapter *adap)
 	 * don't touch the recovery information if the driver is not using
 	 * generic SCL recovery
 	 */
-	if (bri->recover_bus && bri->recover_bus != i2c_generic_scl_recovery)
+	if (bri->recover_bus && bri->recover_bus != i2c_generic_scl_recovery &&
+	    !(IS_ENABLED(CONFIG_CFI_CLANG) && IS_ENABLED(CONFIG_MODULES)))
 		return 0;
 
 	/*
@@ -415,7 +416,8 @@ static int i2c_init_recovery(struct i2c_adapter *adap)
 		goto err;
 	}
 
-	if (bri->scl_gpiod && bri->recover_bus == i2c_generic_scl_recovery) {
+	if (bri->scl_gpiod && (bri->recover_bus == i2c_generic_scl_recovery ||
+	    (IS_ENABLED(CONFIG_CFI_CLANG) && IS_ENABLED(CONFIG_MODULES)))) {
 		bri->get_scl = get_scl_gpio_value;
 		bri->set_scl = set_scl_gpio_value;
 		if (bri->sda_gpiod) {
@@ -424,7 +426,8 @@ static int i2c_init_recovery(struct i2c_adapter *adap)
 			if (gpiod_get_direction(bri->sda_gpiod) == 0)
 				bri->set_sda = set_sda_gpio_value;
 		}
-	} else if (bri->recover_bus == i2c_generic_scl_recovery) {
+	} else if (bri->recover_bus == i2c_generic_scl_recovery ||
+	    (IS_ENABLED(CONFIG_CFI_CLANG) && IS_ENABLED(CONFIG_MODULES))) {
 		/* Generic SCL recovery */
 		if (!bri->set_scl || !bri->get_scl) {
 			err_str = "no {get|set}_scl() found";
-- 
2.25.1

Re: [PATCH] i2c: core: Disable i2c_generic_scl_recovery callback checks with CFI

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 342 bytes --]

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?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [EXT] Re: [PATCH] i2c: core: Disable i2c_generic_scl_recovery callback checks with CFI

[Zhipeng Wang]

CONFIG_CFI_CLANG breaks cross-module function address equality because the jump tables are built at link time. That works for the monolithic kernel, but loadable modules are linked separately. CFI in loadable modules works, but each module gets its own jump tables. That function pointers are replaced by pointers into the jump tables; since modules have different jump tables, they will get different pointers as well. In other words, the values of two pointers to the same function will differ if one of them is in a loadable module.

In summary. info->recover_bus = i2c_generic_scl_recovery in I2C adapter driver, and in the I2C core to judge whether bri->recover_bus and i2c_generic_scl_recovery are equal, the case cannot work, after enabling CFI.

From a) to fix this issue, we can only avoid the case mentioned above. I didn't find a better way, so I submitted this patch.
From b) CFI to fix this issue, does Sami @samitolvanen@google.com have a solution? According to my understanding, this is a side effect of Control Flow Integrity (CFI).

BRs
Zhipeng

-----Original Message-----
From: Wolfram Sang <wsa@kernel.org> 
Sent: 2022年6月30日 3:29
To: Zhipeng Wang <zhipeng.wang_1@nxp.com>
Cc: linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: [EXT] Re: [PATCH] i2c: core: Disable i2c_generic_scl_recovery callback checks with CFI

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?

Re: [PATCH] i2c: core: Disable i2c_generic_scl_recovery callback checks with CFI

[Sami Tolvanen]

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

RE: [EXT] Re: [PATCH] i2c: core: Disable i2c_generic_scl_recovery callback checks with CFI

[Zhipeng Wang]

Hi,

I tested this change on kernel 5.15 and it works. Thanks.

BRs,
Zhipeng

-----Original Message-----
From: Sami Tolvanen <samitolvanen@google.com> 
Sent: 2022年6月30日 23:55
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>
Subject: [EXT] Re: [PATCH] i2c: core: Disable i2c_generic_scl_recovery callback checks with CFI

Caution: EXT Email

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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20220610233513.1798771-1-samitolvanen%40google.com%2F&amp;data=05%7C01%7Czhipeng.wang_1%40nxp.com%7Cc9599657c9d64ae43bd108da5ab0eb0a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637922013151312330%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=djDZ8gNPGnIKz7DfX4w3psZf44zHnph7FflOLa%2F4Qnk%3D&amp;reserved=0

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