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

[Nick Desaulniers]

On Mon, Aug 24, 2026 at 7:10 AM Yo'av Moshe <linux@xxxxxxxxxxxxx> wrote:
>
> Sorry it took so long, I finally got it to work using your method!

Great! (It's somewhat funny to me the level of effort going into this;
I love it. Keep up the good work).  It's also good to hear that we can
keep this working _with_ CFI still enabled.

>
> On 2026-08-17 11:58 PM, Nick Desaulniers wrote:
> > Can you share that diff? I would have expected that to work. Perhaps a
> > minor mistake in your implementation?
>
> I started with something very simple: using SYM_TYPED_FUNC_START and
> copying the 4-byte hash into OCRAM before fncpy(). But that didn't boot
> on my hardware (Kobo Clara HD, i.MX6SLL).
>
> I think the issues were:
> 1. My first attempt wrote the hash to suspend_ocram_base +
> sizeof(*pm_info) - 4, which I suspect overwrote the last member of
> struct imx6_cpu_pm_info, corrupting the memory controller setup.
> 2. Moving the hash after pm_info and offsetting fncpy by +4 still failed
> — I think because fncpy requires 8-byte aligned source and destination
> addresses (FNCPY_ALIGN, with a BUG_ON check).
> 3. Offsetting by +8 for alignment also failed. I suspect it is because
> SYM_TYPED_FUNC_START emits the 4-byte hash after the .align directive,
> which shifts the imx6_suspend label out of 8-byte alignment, again
> triggering fncpy's source alignment BUG_ON.
>
> The version that finally boots on hardware emits the hash manually with
> explicit alignment padding, instead of using SYM_TYPED_FUNC_START:
>
> diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
> index a671ca4..b500922 100644
> --- a/arch/arm/mach-imx/pm-imx6.c
> +++ b/arch/arm/mach-imx/pm-imx6.c
> @@ -515,7 +515,7 @@ static int __init imx6q_suspend_init(const struct imx6_pm_socdata *socdata)
>         pm_info = suspend_ocram_base;
>         pm_info->pbase = ocram_pbase;
>         pm_info->resume_addr = __pa_symbol(v7_cpu_resume);
> -       pm_info->pm_info_size = sizeof(*pm_info);
> +       pm_info->pm_info_size = sizeof(*pm_info) + 8;

I wonder if it would be better to modify the `struct imx6_cpu_pm_info`
struct definition instead to add explicit padding there?  The comment
below about reserving padding seems to make more sense for this hunk
here than down there.

Like add a `u32 cfi_type;` member last, maybe?  That's not 8 bytes,
but I think it fits in the padding, then you might not need +/- 8 in
your pointer arithmetic anymore? I _think_.

>
>         /*
>          * ccm physical address is not used by asm code currently,
> @@ -568,10 +568,16 @@ static int __init imx6q_suspend_init(const struct imx6_pm_socdata *socdata)
>                         mmdc_offset_array[i]);
>         }
>
> +       /* Reserve 8 bytes between pm_info and the function copy in OCRAM:
> +        * 4 bytes padding + 4 bytes kCFI type hash, so that the hash sits
> +        * at fncpy_dest - 4 and fncpy_dest remains 8-byte aligned. */
> +       *(u32 *)(suspend_ocram_base + sizeof(*pm_info) + 4) =
> +               *(((u32 *)&imx6_suspend) - 1);

This is incredibly hard to follow.  This is copying the hash, not
reserving anything?

Is cfi_get_func_hash() something we can use here?

```
#include <linux/cfi.h>

#ifdef CONFIG_CFI
    // Make sure to clear the thumb bit!
    pm_info->cfi_type = cfi_get_func_hash((void
*)((uintptr_t)&imx6_suspend & ~1));
#endif
```

then remove the below hunk?

> +
>         imx6_suspend_in_ocram_fn = fncpy(
> -               suspend_ocram_base + sizeof(*pm_info),
> +               suspend_ocram_base + sizeof(*pm_info) + 8,
>                 &imx6_suspend,
> -               MX6Q_SUSPEND_OCRAM_SIZE - sizeof(*pm_info));
> +               MX6Q_SUSPEND_OCRAM_SIZE - sizeof(*pm_info) - 8);
>
>         __arm_iomem_set_ro(suspend_ocram_base, MX6Q_SUSPEND_OCRAM_SIZE);
>
> diff --git a/arch/arm/mach-imx/suspend-imx6.S b/arch/arm/mach-imx/suspend-imx6.S
> index 63ccc2d..c06e474 100644
> --- a/arch/arm/mach-imx/suspend-imx6.S
> +++ b/arch/arm/mach-imx/suspend-imx6.S
> @@ -3,6 +3,7 @@
>   * Copyright 2014 Freescale Semiconductor, Inc.
>   */
>
> +#include <linux/cfi_types.h>
>  #include <linux/linkage.h>
>  #include <asm/assembler.h>
>  #include <asm/asm-offsets.h>
> @@ -148,6 +149,15 @@
>
>         .endm
>
> +#ifdef CONFIG_CFI
> +       /*
> +        * Emit kCFI type hash before imx6_suspend with padding to preserve
> +        * the 8-byte alignment that fncpy requires for the source address.
> +        */
> +       .align  3
> +       .4byte  0
> +       __CFI_TYPE(imx6_suspend)
> +#endif
>  ENTRY(imx6_suspend)

Should you replace ENTRY (and everything you added to this file) with
SYM_TYPED_START?  Or perhaps we should add a new macro to cfi_types.h
for this?  ENTRY_TYPED or something?

>         ldr     r1, [r0, #PM_INFO_PBASE_OFFSET]
>         ldr     r2, [r0, #PM_INFO_RESUME_ADDR_OFFSET]
>
>
> > How did you verify this? Can you share the command line invocations and output?
> I tested each iteration on postmarketOS by building the kernel with
> pmbootstrap (Clang/LLVM, CONFIG_CFI=y), replacing the vmlinuz on the SD
> card, and booting the Kobo Clara HD. The earlier attempts all failed to
> boot (though they worked fine in QEMU, which doesn't emulate the i.MX6
> MMDC hardware I guess?).
>
> I'm not really sure which approach is better now. The version above
> preserves CFI on the indirect call, but it's quite involved compared to
> the v3 __nocfi wrapper. If you think this is the better way to go, I'm
> happy to clean it up and resubmit as v4.

I do think it's better, but probably still needs revision.

FWIW, I still have a bunch of i.mx8 family boards in a box under my
desk.  I have a wafer gifted to me from my best man that we think is
some i.mx chip from his time at NXP.  Out of nostalgia for that line,
let's get this wrapped up neatly with a bow.

>
> Yo'av



-- 
Thanks,
~Nick Desaulniers