| Message ID | 20220628024155.2135990-1-zhipeng.wang_1@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: core: Disable i2c_generic_scl_recovery callback checks with CFI | expand |
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&data=05%7C01%7Czhipeng.wang_1%40nxp.com%7Cc9599657c9d64ae43bd108da5ab0eb0a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637922013151312330%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=djDZ8gNPGnIKz7DfX4w3psZf44zHnph7FflOLa%2F4Qnk%3D&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
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";
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(-)