| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3f4ee683-4fdb-4183-8f99-f931f853d9ae@kernel.org>
Date: Tue, 14 Oct 2025 16:58:56 -0500
From: "Mario Limonciello (AMD) (kernel.org)" <superm1@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>, Nathan Chancellor <nathan@...nel.org>
Cc: Mika Westerberg <mika.westerberg@...ux.intel.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Jan Dabros <jsd@...ihalf.com>, Andi Shyti <andi.shyti@...nel.org>,
Nick Desaulniers <nick.desaulniers+lkml@...il.com>,
Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>,
Kees Cook <kees@...nel.org>, Sami Tolvanen <samitolvanen@...gle.com>,
linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH] i2c: designware: Remove i2c_dw_remove_lock_support()
On 10/14/2025 3:33 PM, Bjorn Helgaas wrote:
> [+cc Mario, author of 440da737cf8d ("i2c: designware: Use PCI PSP
> driver for communication")]
>
> On Mon, Oct 13, 2025 at 06:05:03PM -0700, Nathan Chancellor wrote:
>> When building certain configurations with CONFIG_FINEIBT=y after
>> commit 894af4a1cde6 ("objtool: Validate kCFI calls"), there is a
>> warning due to an indirect call in dw_i2c_plat_remove():
>>
>> $ cat allno.config
>> CONFIG_ACPI=y
>> CONFIG_CFI=y
>> CONFIG_COMMON_CLK=y
>> CONFIG_CPU_MITIGATIONS=y
>> CONFIG_I2C=y
>> CONFIG_I2C_DESIGNWARE_BAYTRAIL=y
>> CONFIG_I2C_DESIGNWARE_CORE=y
>> CONFIG_I2C_DESIGNWARE_PLATFORM=y
>> CONFIG_IOSF_MBI=y
>> CONFIG_MITIGATION_RETPOLINE=y
>> CONFIG_MODULES=y
>> CONFIG_PCI=y
>> CONFIG_X86_KERNEL_IBT=y
>>
>> $ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 clean allnoconfig vmlinux
>> vmlinux.o: warning: objtool: dw_i2c_plat_remove+0x3c: no-cfi indirect call!
>>
>> With this configuration, i2c_dw_semaphore_cb_table has the BAYTRAIL
>> member and the sentinel (i.e., 2 members), both of which have an
>> implicit
>>
>> .remove = NULL,
>>
>> so Clang effectively turns i2c_dw_remove_lock_support(), which is later
>> inlined into dw_i2c_plat_remove(), into:
>>
>> static void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
>> {
>> if (dev->semaphore_idx > 2)
>> (*NULL)(dev):
>> }
>>
>> which is not necessarily problematic from a logic perspective (as the
>> code was not bounds checking semaphore_idx so an out of bounds index
>> could already crash) but objtool's new __nocfi indirect call checking
>> trips over Clang dropping the kCFI setup from a known NULL indirect
>> call.
>>
>> While it would be possible to fix this by transforming the initial check
>> into
>>
>> if (dev->semaphore_idx < 0 || dev->semaphore_idx >= ARRAY_SIZE(i2c_dw_semaphore_cb_table))
>>
>> the remove member is unused after commit 440da737cf8d ("i2c: designware:
>> Use PCI PSP driver for communication"), so i2c_dw_remove_lock_support()
>> can be removed altogether, as it will never actually do anything.
>>
>> Closes: https://github.com/ClangBuiltLinux/linux/issues/2133
>> Signed-off-by: Nathan Chancellor <nathan@...nel.org>
>
> I'm totally fine with the patch itself, but I think the commit log
> could be trimmed to something like the following with no loss:
>
> Remove struct i2c_dw_semaphore_callbacks.remove() and
> i2c_dw_remove_lock_support().
>
> 440da737cf8d ("i2c: designware: Use PCI PSP driver for
> communication") removed the last place that set
> i2c_dw_semaphore_callbacks.remove(), which made
> i2c_dw_remove_lock_support() a no-op.
>
> This has the side effect of avoiding this kCFI warning (see Link):
>
> dw_i2c_plat_remove+0x3c: no-cfi indirect call!
>
> Link: https://lore.kernel.org/r/20251013-dw_i2c_plat_remove-avoid-objtool-no-cfi-warning-v1-1-8cc4842967bf@kernel.org
>
> FWIW,
> Reviewed-by: Bjorn Helgaas <bhelgaas@...gle.com>
I echo Bjorn's comments on the lengthy commit message.
Code change looks fine.
Reviewed-by: Mario Limonciello (AMD) <superm1@...nel.org>
>
>> ---
>> drivers/i2c/busses/i2c-designware-core.h | 1 -
>> drivers/i2c/busses/i2c-designware-platdrv.c | 11 -----------
>> 2 files changed, 12 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
>> index 347843b4f5dd..d50664377c6b 100644
>> --- a/drivers/i2c/busses/i2c-designware-core.h
>> +++ b/drivers/i2c/busses/i2c-designware-core.h
>> @@ -330,7 +330,6 @@ struct dw_i2c_dev {
>>
>> struct i2c_dw_semaphore_callbacks {
>> int (*probe)(struct dw_i2c_dev *dev);
>> - void (*remove)(struct dw_i2c_dev *dev);
>> };
>>
>> int i2c_dw_init_regmap(struct dw_i2c_dev *dev);
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index 34d881572351..cff7e03dea7b 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -197,15 +197,6 @@ static int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
>> return 0;
>> }
>>
>> -static void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
>> -{
>> - if (dev->semaphore_idx < 0)
>> - return;
>> -
>> - if (i2c_dw_semaphore_cb_table[dev->semaphore_idx].remove)
>> - i2c_dw_semaphore_cb_table[dev->semaphore_idx].remove(dev);
>> -}
>> -
>> static int dw_i2c_plat_probe(struct platform_device *pdev)
>> {
>> u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
>> @@ -339,8 +330,6 @@ static void dw_i2c_plat_remove(struct platform_device *pdev)
>>
>> i2c_dw_prepare_clk(dev, false);
>>
>> - i2c_dw_remove_lock_support(dev);
>> -
>> reset_control_assert(dev->rst);
>> }
>>
>>
>> ---
>> base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
>> change-id: 20251013-dw_i2c_plat_remove-avoid-objtool-no-cfi-warning-5f2040eaadc2
>>
>> Best regards,
>> --
>> Nathan Chancellor <nathan@...nel.org>
>>
Powered by blists - more mailing lists