Re: [PATCH v2] ARM: imx: Fix suspend/resume crash with Clang CFI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Applying `__nocfi` to `imx6q_suspend_finish` unintentionally disables KCFI checks for other indirect branches.
- [High] The CFI bypass mitigation is incomplete: `suspend_ocram_base` remains writable, enabling a Write-What-Where (Data-Oriented Programming) exploit.

Pre-existing issues:
- [High] The kCFI panic with `fncpy` relocated code affects multiple platforms, but is only fixed for i.MX6.
- [High] The `ccm_base` global variable is written only during initialization but lacks `__ro_after_init` protection, exposing it to manipulation for Read-Modify-Write (RMW) gadgets.
--

--- Patch [1]: [PATCH v2] ARM: imx: Fix suspend/resume crash with Clang CFI ---
commit 66327a9e81a0b45dd7493d089dd60f3a912e1240
Author: Yo'av Moshe <linux@yoavmoshe.com>

ARM: imx: Fix suspend/resume crash with Clang CFI

Relocated suspend code in OCRAM lacks compiler-generated CFI type
signatures. This patch annotates imx6q_suspend_finish with __nocfi to
bypass CFI checking for the indirect call, and marks the function pointer
as __ro_after_init.

> diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
> index a671ca498f887..b425a47019896 100644
> --- a/arch/arm/mach-imx/pm-imx6.c
> +++ b/arch/arm/mach-imx/pm-imx6.c
> @@ -63,7 +63,7 @@
>  
>  static void __iomem *ccm_base;

[Severity: High]
This is a pre-existing issue, but does ccm_base also need __ro_after_init
protection?

Since ccm_base is assigned once in imx6_pm_common_init() and then
repeatedly dereferenced in power management functions like imx6_set_lpm(),
could an attacker overwrite it to misdirect read/write operations?

For example, readl_relaxed(ccm_base + CLPCR) could be redirected to
arbitrary kernel addresses. Adding __ro_after_init here might prevent
trivial data-oriented programming exploitation of power management paths.

>  static void __iomem *suspend_ocram_base;

[Severity: High]
Since suspend_ocram_base is passed as the first argument to the __nocfi
function imx6_suspend_in_ocram_fn, does leaving it mutable in the .bss
section defeat the intent of the __ro_after_init protection?

The target assembly routine imx6_suspend trusts this pointer (passed in
r0), using it to derive target addresses for memory writes:

arch/arm/mach-imx/suspend-imx6.S:imx6_suspend()
    ldr r11, [r0, #PM_INFO_MX6Q_SRC_V_OFFSET]
    /* store physical resume addr and pm_info address. */
    str r9, [r11, #MX6Q_SRC_GPR1]

Could an attacker with arbitrary memory write capabilities overwrite
suspend_ocram_base to point to a crafted payload, yielding a
write-what-where primitive during suspend that completely bypasses CFI?

> -static void (*imx6_suspend_in_ocram_fn)(void __iomem *ocram_vbase);
> +static void (*imx6_suspend_in_ocram_fn)(void __iomem *ocram_vbase) __ro_after_init;
>  
>  /*
>   * suspend ocram space layout:

[ ... ]

> @@ -360,7 +360,7 @@ int imx6_set_lpm(enum mxc_cpu_pwr_mode mode)
>  	return 0;
>  }
>  
> -static int imx6q_suspend_finish(unsigned long val)
> +static int __nocfi imx6q_suspend_finish(unsigned long val)

[Severity: High]
By applying __nocfi to the entire imx6q_suspend_finish() function, are we
unintentionally disabling CFI checks for other indirect branches?

Within this function, cpu_do_idle() and flush_cache_all() expand to
indirect calls (cpu_vtable[smp_processor_id()]->_do_idle() and
cpu_cache.flush_kern_all()).

Could an attacker with arbitrary memory write capabilities overwrite these
architecture-level function pointers and bypass kCFI entirely when a
suspend is triggered?

Would it be safer to restrict the __nocfi annotation to a minimal inline
or noinline wrapper function that only executes the single unverified
indirect call to imx6_suspend_in_ocram_fn?

>  {
>  	if (!imx6_suspend_in_ocram_fn) {
>  		cpu_do_idle();
>  	} else {
>  		/*
>  		 * call low level suspend function in ocram,
>  		 * as we need to float DDR IO.
>  		 */
>  		local_flush_tlb_all();
>  		/* check if need to flush internal L2 cache */
>  		if (!((struct imx6_cpu_pm_info *)
>  			suspend_ocram_base)->l2_base.vbase)
>  			flush_cache_all();
>  		imx6_suspend_in_ocram_fn(suspend_ocram_base);

[Severity: High]
This is a pre-existing issue, but does fixing this kCFI panic only for
i.MX6 leave other 32-bit ARM platforms broken?

The kCFI panic with fncpy() relocated code appears to affect multiple
platforms since fncpy() copies only the instruction body and omits the
compiler-generated SYM_TYPED_FUNC_START header.

For example, mx5_suspend_enter() in arch/arm/mach-imx/pm-imx5.c invokes
imx5_suspend_in_ocram_fn(suspend_ocram_base), and at91_suspend_finish()
in arch/arm/mach-at91/pm.c invokes at91_suspend_sram_fn(&soc_pm.data).

Since neither seem to have a __nocfi bypass, would they suffer
deterministic kernel panics on suspend when CONFIG_CFI_CLANG is active?

>  	}
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718091917.81738-1-linux@yoavmoshe.com?part=1