arch/arm/mach-imx/pm-imx6.c | 25 ++++++++++++++++++++++--- arch/arm/mach-imx/suspend-imx6.S | 14 ++++++++++++-- 2 files changed, 34 insertions(+), 5 deletions(-)
The suspend code that runs from OCRAM is copied there with fncpy(),
which does not copy the kCFI type hash preceding the function. With
CONFIG_CFI=y the indirect call through imx6_suspend_in_ocram_fn
therefore panics.
Keep the call covered by CFI instead of exempting it:
- Declare imx6_suspend() with SYM_TYPED_FUNC_START() so a type hash
is emitted for it. fncpy() requires the entry point to be 8-byte
aligned, and the macro's alignment applies to the hash rather than
the entry that follows it, so pad manually before the macro.
- Add a cfi_type member at the end of struct imx6_cpu_pm_info, which
directly precedes the OCRAM copy of the function. It fits in the
struct's tail padding, so no sizes or offsets change. Fill it using
cfi_get_func_hash(), putting the hash where the caller's CFI check
expects it: four bytes before the function entry.
Also mark ccm_base, suspend_ocram_base and imx6_suspend_in_ocram_fn
as __ro_after_init: they are only written during __init, and the
function pointer in particular should not be writable afterwards.
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Yo'av Moshe <linux@yoavmoshe.com>
---
Tested on a Kobo Clara HD (i.MX6SLL) running postmarketOS
(clang/LLVM, CONFIG_CFI=y): suspend and resume work.
Changes in v4, all following Nick's suggestions:
- Drop the v3 __nocfi wrapper; keep the indirect call CFI-checked.
- Store the hash in a cfi_type member in the struct's tail padding
instead of open-coded pointer arithmetic.
- Use cfi_get_func_hash() instead of reading the hash manually.
- Use SYM_TYPED_FUNC_START()/SYM_FUNC_END() instead of ENTRY()/
ENDPROC() with a hand-rolled __CFI_TYPE.
Note: linux/uaccess.h is included before linux/cfi.h because
cfi_get_func_hash() uses get_kernel_nofault() and cfi.h does not
include uaccess.h itself.
arch/arm/mach-imx/pm-imx6.c | 25 ++++++++++++++++++++++---
arch/arm/mach-imx/suspend-imx6.S | 14 ++++++++++++--
2 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
index a671ca4..2d3d3cd 100644
--- a/arch/arm/mach-imx/pm-imx6.c
+++ b/arch/arm/mach-imx/pm-imx6.c
@@ -4,6 +4,8 @@
* Copyright 2011 Linaro Ltd.
*/
+#include <linux/uaccess.h>
+#include <linux/cfi.h>
#include <linux/clk/imx.h>
#include <linux/delay.h>
#include <linux/init.h>
@@ -61,9 +63,9 @@
#define MX6Q_SUSPEND_OCRAM_SIZE 0x1000
#define MX6_MAX_MMDC_IO_NUM 33
-static void __iomem *ccm_base;
-static void __iomem *suspend_ocram_base;
-static void (*imx6_suspend_in_ocram_fn)(void __iomem *ocram_vbase);
+static void __iomem *ccm_base __ro_after_init;
+static void __iomem *suspend_ocram_base __ro_after_init;
+static void (*imx6_suspend_in_ocram_fn)(void __iomem *ocram_vbase) __ro_after_init;
/*
* suspend ocram space layout:
@@ -229,8 +231,18 @@ struct imx6_cpu_pm_info {
struct imx6_pm_base l2_base;
u32 mmdc_io_num; /* Number of MMDC IOs which need saved/restored. */
u32 mmdc_io_val[MX6_MAX_MMDC_IO_NUM][2]; /* To save offset and value */
+ u32 cfi_type; /* kCFI type hash of imx6_suspend() */
} __aligned(8);
+/*
+ * The ocram copy of imx6_suspend() starts right after struct imx6_cpu_pm_info,
+ * and the CFI check on the indirect call reads the kCFI type hash from the
+ * four bytes preceding the function entry, so cfi_type must occupy the last
+ * four bytes of the struct, i.e. fit into its tail padding.
+ */
+static_assert(offsetofend(struct imx6_cpu_pm_info, cfi_type) ==
+ sizeof(struct imx6_cpu_pm_info));
+
void imx6_set_int_mem_clk_lpm(bool enable)
{
u32 val = readl_relaxed(ccm_base + CGPR);
@@ -568,6 +580,13 @@ static int __init imx6q_suspend_init(const struct imx6_pm_socdata *socdata)
mmdc_offset_array[i]);
}
+ /*
+ * Mask out the Thumb bit, as cfi_get_func_hash() expects the
+ * function's actual start address. Returns 0 if CONFIG_CFI=n.
+ */
+ pm_info->cfi_type =
+ cfi_get_func_hash((void *)((uintptr_t)&imx6_suspend & ~1UL));
+
imx6_suspend_in_ocram_fn = fncpy(
suspend_ocram_base + sizeof(*pm_info),
&imx6_suspend,
diff --git a/arch/arm/mach-imx/suspend-imx6.S b/arch/arm/mach-imx/suspend-imx6.S
index 63ccc2d..f60c4e5 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,7 +149,16 @@
.endm
-ENTRY(imx6_suspend)
+#ifdef CONFIG_CFI
+ /*
+ * Pad the location counter so that the type hash emitted by
+ * SYM_TYPED_FUNC_START() below ends on an 8-byte boundary:
+ * fncpy() requires the function entry to be 8-byte aligned.
+ */
+ .align 3
+ .4byte 0
+#endif
+SYM_TYPED_FUNC_START(imx6_suspend)
ldr r1, [r0, #PM_INFO_PBASE_OFFSET]
ldr r2, [r0, #PM_INFO_RESUME_ADDR_OFFSET]
ldr r3, [r0, #PM_INFO_DDR_TYPE_OFFSET]
@@ -329,4 +339,4 @@ resume:
resume_mmdc
ret lr
-ENDPROC(imx6_suspend)
+SYM_FUNC_END(imx6_suspend)
--
2.55.0On Thu, Aug 27, 2026 at 12:46 PM Yo'av Moshe <linux@yoavmoshe.com> wrote:
>
> The suspend code that runs from OCRAM is copied there with fncpy(),
> which does not copy the kCFI type hash preceding the function. With
> CONFIG_CFI=y the indirect call through imx6_suspend_in_ocram_fn
> therefore panics.
>
> Keep the call covered by CFI instead of exempting it:
Nice! This is a pretty good progression.
>
> - Declare imx6_suspend() with SYM_TYPED_FUNC_START() so a type hash
> is emitted for it. fncpy() requires the entry point to be 8-byte
> aligned, and the macro's alignment applies to the hash rather than
> the entry that follows it, so pad manually before the macro.
>
> - Add a cfi_type member at the end of struct imx6_cpu_pm_info, which
> directly precedes the OCRAM copy of the function. It fits in the
> struct's tail padding, so no sizes or offsets change. Fill it using
> cfi_get_func_hash(), putting the hash where the caller's CFI check
> expects it: four bytes before the function entry.
>
> Also mark ccm_base, suspend_ocram_base and imx6_suspend_in_ocram_fn
> as __ro_after_init: they are only written during __init, and the
> function pointer in particular should not be writable afterwards.
>
> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Yo'av Moshe <linux@yoavmoshe.com>
> ---
> Tested on a Kobo Clara HD (i.MX6SLL) running postmarketOS
> (clang/LLVM, CONFIG_CFI=y): suspend and resume work.
>
> Changes in v4, all following Nick's suggestions:
> - Drop the v3 __nocfi wrapper; keep the indirect call CFI-checked.
> - Store the hash in a cfi_type member in the struct's tail padding
> instead of open-coded pointer arithmetic.
> - Use cfi_get_func_hash() instead of reading the hash manually.
> - Use SYM_TYPED_FUNC_START()/SYM_FUNC_END() instead of ENTRY()/
> ENDPROC() with a hand-rolled __CFI_TYPE.
>
> Note: linux/uaccess.h is included before linux/cfi.h because
> cfi_get_func_hash() uses get_kernel_nofault() and cfi.h does not
> include uaccess.h itself.
Ah, no, we (you) should fix that. include/linux/cfi.h should IWYU.
>
> arch/arm/mach-imx/pm-imx6.c | 25 ++++++++++++++++++++++---
> arch/arm/mach-imx/suspend-imx6.S | 14 ++++++++++++--
> 2 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
> index a671ca4..2d3d3cd 100644
> --- a/arch/arm/mach-imx/pm-imx6.c
> +++ b/arch/arm/mach-imx/pm-imx6.c
> @@ -4,6 +4,8 @@
> * Copyright 2011 Linaro Ltd.
> */
>
> +#include <linux/uaccess.h>
> +#include <linux/cfi.h>
This is a bug in include/linux/cfi.h; don't work around it. Fix it.
> #include <linux/clk/imx.h>
> #include <linux/delay.h>
> #include <linux/init.h>
> @@ -61,9 +63,9 @@
> #define MX6Q_SUSPEND_OCRAM_SIZE 0x1000
> #define MX6_MAX_MMDC_IO_NUM 33
>
> -static void __iomem *ccm_base;
> -static void __iomem *suspend_ocram_base;
> -static void (*imx6_suspend_in_ocram_fn)(void __iomem *ocram_vbase);
> +static void __iomem *ccm_base __ro_after_init;
> +static void __iomem *suspend_ocram_base __ro_after_init;
> +static void (*imx6_suspend_in_ocram_fn)(void __iomem *ocram_vbase) __ro_after_init;
>
> /*
> * suspend ocram space layout:
> @@ -229,8 +231,18 @@ struct imx6_cpu_pm_info {
> struct imx6_pm_base l2_base;
> u32 mmdc_io_num; /* Number of MMDC IOs which need saved/restored. */
> u32 mmdc_io_val[MX6_MAX_MMDC_IO_NUM][2]; /* To save offset and value */
> + u32 cfi_type; /* kCFI type hash of imx6_suspend() */
> } __aligned(8);
>
> +/*
> + * The ocram copy of imx6_suspend() starts right after struct imx6_cpu_pm_info,
> + * and the CFI check on the indirect call reads the kCFI type hash from the
> + * four bytes preceding the function entry, so cfi_type must occupy the last
> + * four bytes of the struct, i.e. fit into its tail padding.
> + */
> +static_assert(offsetofend(struct imx6_cpu_pm_info, cfi_type) ==
> + sizeof(struct imx6_cpu_pm_info));
on this IWYU thread, shall we also include linux/build_bug.h for
static_assert or linux/stddef.h for offsetofend?
> +
> void imx6_set_int_mem_clk_lpm(bool enable)
> {
> u32 val = readl_relaxed(ccm_base + CGPR);
> @@ -568,6 +580,13 @@ static int __init imx6q_suspend_init(const struct imx6_pm_socdata *socdata)
> mmdc_offset_array[i]);
> }
>
> + /*
> + * Mask out the Thumb bit, as cfi_get_func_hash() expects the
> + * function's actual start address. Returns 0 if CONFIG_CFI=n.
> + */
> + pm_info->cfi_type =
> + cfi_get_func_hash((void *)((uintptr_t)&imx6_suspend & ~1UL));
> +
> imx6_suspend_in_ocram_fn = fncpy(
> suspend_ocram_base + sizeof(*pm_info),
> &imx6_suspend,
> diff --git a/arch/arm/mach-imx/suspend-imx6.S b/arch/arm/mach-imx/suspend-imx6.S
> index 63ccc2d..f60c4e5 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,7 +149,16 @@
>
> .endm
>
> -ENTRY(imx6_suspend)
> +#ifdef CONFIG_CFI
> + /*
> + * Pad the location counter so that the type hash emitted by
> + * SYM_TYPED_FUNC_START() below ends on an 8-byte boundary:
> + * fncpy() requires the function entry to be 8-byte aligned.
> + */
> + .align 3
> + .4byte 0
I still don't like these assembler directives inline like this; this
feels like we should have perhaps a new macro in
include/linux/cfi_types.h. Thoughts, Sami?
One that garuntees the 8B alignment of the symbol for w/e that
function patching routine requires?
> +#endif
> +SYM_TYPED_FUNC_START(imx6_suspend)
> ldr r1, [r0, #PM_INFO_PBASE_OFFSET]
> ldr r2, [r0, #PM_INFO_RESUME_ADDR_OFFSET]
> ldr r3, [r0, #PM_INFO_DDR_TYPE_OFFSET]
> @@ -329,4 +339,4 @@ resume:
> resume_mmdc
>
> ret lr
> -ENDPROC(imx6_suspend)
> +SYM_FUNC_END(imx6_suspend)
> --
> 2.55.0
>
--
Thanks,
~Nick Desaulniers
On Fri, Aug 28, 2026 at 11:29 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Thu, Aug 27, 2026 at 12:46 PM Yo'av Moshe <linux@yoavmoshe.com> wrote: > > > > Note: linux/uaccess.h is included before linux/cfi.h because > > cfi_get_func_hash() uses get_kernel_nofault() and cfi.h does not > > include uaccess.h itself. > > Ah, no, we (you) should fix that. include/linux/cfi.h should IWYU. Nathan fixed this here: https://lore.kernel.org/lkml/20260604-tracing-fix-cfi-h-build-error-v1-1-b27015390901@kernel.org/ > > -ENTRY(imx6_suspend) > > +#ifdef CONFIG_CFI > > + /* > > + * Pad the location counter so that the type hash emitted by > > + * SYM_TYPED_FUNC_START() below ends on an 8-byte boundary: > > + * fncpy() requires the function entry to be 8-byte aligned. > > + */ > > + .align 3 > > + .4byte 0 > > I still don't like these assembler directives inline like this; this > feels like we should have perhaps a new macro in > include/linux/cfi_types.h. Thoughts, Sami? > > One that garuntees the 8B alignment of the symbol for w/e that > function patching routine requires? Sounds like arm should override SYM_TYPED_FUNC_START to add whatever alignment is needed? Sami
On Fri, Aug 28, 2026 at 11:45 AM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> On Fri, Aug 28, 2026 at 11:29 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Thu, Aug 27, 2026 at 12:46 PM Yo'av Moshe <linux@yoavmoshe.com> wrote:
> > >
> > > Note: linux/uaccess.h is included before linux/cfi.h because
> > > cfi_get_func_hash() uses get_kernel_nofault() and cfi.h does not
> > > include uaccess.h itself.
> >
> > Ah, no, we (you) should fix that. include/linux/cfi.h should IWYU.
>
> Nathan fixed this here:
>
> https://lore.kernel.org/lkml/20260604-tracing-fix-cfi-h-build-error-v1-1-b27015390901@kernel.org/
Ah! Then this series should probably be rebased or include that patch.
>
> > > -ENTRY(imx6_suspend)
> > > +#ifdef CONFIG_CFI
> > > + /*
> > > + * Pad the location counter so that the type hash emitted by
> > > + * SYM_TYPED_FUNC_START() below ends on an 8-byte boundary:
> > > + * fncpy() requires the function entry to be 8-byte aligned.
> > > + */
> > > + .align 3
> > > + .4byte 0
> >
> > I still don't like these assembler directives inline like this; this
> > feels like we should have perhaps a new macro in
> > include/linux/cfi_types.h. Thoughts, Sami?
> >
> > One that garuntees the 8B alignment of the symbol for w/e that
> > function patching routine requires?
>
> Sounds like arm should override SYM_TYPED_FUNC_START to add whatever
> alignment is needed?
The 8B alignment seems to be a requirement of fncopy() for 32b ARM.
But functions in ARM can have 4B or even 2B alignment w/ Thumb, I
think.
Doing such an override would add unnecessary padding for each use of
SYM_TYPED_FUNC_START where probably none of them have this fncopy()
constraint.
```
#ifdef CONFIG_CFI
#define SYM_TYPED_START_ALIGNED(name, linkage, align) \
linkage(name) ASM_NL \
.balign align ASM_NL \
.fill (align) - 4, 1, 0 ASM_NL \
__CFI_TYPE(name) ASM_NL \
name:
#else /* CONFIG_CFI */
#define SYM_TYPED_START_ALIGNED(name, linkage, align) \
SYM_START(name, linkage, .balign align)
#endif /* CONFIG_CFI */
#define SYM_TYPED_FUNC_START_ALIGNED(name, align) \
SYM_TYPED_START_ALIGNED(name, SYM_L_GLOBAL, align)
```
then this driver could do
```
// fncopy needs 8B alignment; see arch/arm/include/asm/fncpy.h.
SYM_TYPED_FUNC_START_ALIGNED(imx6_suspend, 8)
```
--
Thanks,
~Nick Desaulniers
On Fri, Aug 28, 2026 at 1:19 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > The 8B alignment seems to be a requirement of fncopy() for 32b ARM. > But functions in ARM can have 4B or even 2B alignment w/ Thumb, I > think. > > Doing such an override would add unnecessary padding for each use of > SYM_TYPED_FUNC_START where probably none of them have this fncopy() > constraint. > > ``` > #ifdef CONFIG_CFI > > #define SYM_TYPED_START_ALIGNED(name, linkage, align) \ > linkage(name) ASM_NL \ > .balign align ASM_NL \ > .fill (align) - 4, 1, 0 ASM_NL \ > __CFI_TYPE(name) ASM_NL \ > name: > > #else /* CONFIG_CFI */ > > #define SYM_TYPED_START_ALIGNED(name, linkage, align) \ > SYM_START(name, linkage, .balign align) > > #endif /* CONFIG_CFI */ > #define SYM_TYPED_FUNC_START_ALIGNED(name, align) \ > SYM_TYPED_START_ALIGNED(name, SYM_L_GLOBAL, align) > ``` > then this driver could do > ``` > // fncopy needs 8B alignment; see arch/arm/include/asm/fncpy.h. > SYM_TYPED_FUNC_START_ALIGNED(imx6_suspend, 8) Looks reasonable, but this still feels ARM-specific to me. How about adding the macro to arch/arm/include/asm/linkage.h unless you have other users in mind? Sami
On 2026-08-28 10:50 PM, Sami Tolvanen wrote: > Looks reasonable, but this still feels ARM-specific to me. How about > adding the macro to arch/arm/include/asm/linkage.h unless you have > other users in mind? I've now sent a v5 with Nick's macro added to arch/arm/include/asm/linkage.h as you suggested, plus the other fixes Nick asked for: https://lore.kernel.org/all/20260830155100.1020620-1-linux@yoavmoshe.com/ The sashiko-bot's comment on v5 indeed pointed out that imx5, at91 and omap1 might suffer from the same bug: their suspend routines are also fncpy()'d into SRAM without a type hash and called indirectly. They're all ARM, so asm/linkage.h still seems like the right place for the macro. Do you think these need to be fixed as part of this patch, or as follow-ups? I only have my one imx6 device (a Kobo Clara HD), so I'm not sure how to test the fixes for the others. Yo'av
On Sun, Aug 30, 2026 at 9:24 AM Yo'av Moshe <linux@yoavmoshe.com> wrote: > > The sashiko-bot's comment on v5 indeed pointed out that imx5, at91 and > omap1 might suffer from the same bug: their suspend routines are also > fncpy()'d into SRAM without a type hash and called indirectly. They're > all ARM, so asm/linkage.h still seems like the right place for the > macro. > > Do you think these need to be fixed as part of this patch, or as > follow-ups? I only have my one imx6 device (a Kobo Clara HD), so I'm not > sure how to test the fixes for the others. I think separate patches for the others should be fine. Sami
On Tue, Sep 1, 2026 at 12:46 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > On Sun, Aug 30, 2026 at 9:24 AM Yo'av Moshe <linux@yoavmoshe.com> wrote: > > > > The sashiko-bot's comment on v5 indeed pointed out that imx5, at91 and > > omap1 might suffer from the same bug: their suspend routines are also > > fncpy()'d into SRAM without a type hash and called indirectly. They're > > all ARM, so asm/linkage.h still seems like the right place for the > > macro. > > > > Do you think these need to be fixed as part of this patch, or as > > follow-ups? I only have my one imx6 device (a Kobo Clara HD), so I'm not > > sure how to test the fixes for the others. > > I think separate patches for the others should be fine. Without devices in hand to test on, I'd be hesitant to claim 100% confidence. Maybe reach out to the MAINTAINERS of those, and see if they have cycles to help test potential patches, before writing/sending patches that potentially don't get tested? -- Thanks, ~Nick Desaulniers
On Tue, Sep 01, 2026 at 01:36:15PM -0700, Nick Desaulniers wrote: > On Tue, Sep 1, 2026 at 12:46 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > On Sun, Aug 30, 2026 at 9:24 AM Yo'av Moshe <linux@yoavmoshe.com> wrote: > > > > > > The sashiko-bot's comment on v5 indeed pointed out that imx5, at91 and > > > omap1 might suffer from the same bug: their suspend routines are also > > > fncpy()'d into SRAM without a type hash and called indirectly. They're > > > all ARM, so asm/linkage.h still seems like the right place for the > > > macro. > > > > > > Do you think these need to be fixed as part of this patch, or as > > > follow-ups? I only have my one imx6 device (a Kobo Clara HD), so I'm not > > > sure how to test the fixes for the others. > > > > I think separate patches for the others should be fine. > > Without devices in hand to test on, I'd be hesitant to claim 100% > confidence. Maybe reach out to the MAINTAINERS of those, and see if > they have cycles to help test potential patches, before > writing/sending patches that potentially don't get tested? Nick Desaulniers: Is this patch good to pickup? Frank > > -- > Thanks, > ~Nick Desaulniers >
On Wed, Sep 2, 2026 at 9:44 AM Frank Li <Frank.li@oss.nxp.com> wrote: > > Nick Desaulniers: > > Is this patch good to pickup? Yes. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> It does depend on https://lore.kernel.org/lkml/20260604-tracing-fix-cfi-h-build-error-v1-1-b27015390901@kernel.org/ though, so you might need to coordinate your PRs to Linus with Nathan. Nathan, is https://lore.kernel.org/lkml/20260604-tracing-fix-cfi-h-build-error-v1-1-b27015390901@kernel.org/ going via kbuild tree or what? -- Thanks, ~Nick Desaulniers
On Wed, Sep 02, 2026 at 10:10:23AM -0700, Nick Desaulniers wrote:
> It does depend on
> https://lore.kernel.org/lkml/20260604-tracing-fix-cfi-h-build-error-v1-1-b27015390901@kernel.org/
> though, so you might need to coordinate your PRs to Linus with Nathan.
> Nathan, is https://lore.kernel.org/lkml/20260604-tracing-fix-cfi-h-build-error-v1-1-b27015390901@kernel.org/
> going via kbuild tree or what?
That change is already in mainline as commit 979c294509f9 ("cfi: Include
uaccess.h for get_kernel_nofault"), so there should be nothing for
upstream maintainers to worry about. If this patch needs to go back to
stable past 6.18, then that change will need to be included with it.
--
Cheers,
Nathan
On Wed, Sep 2, 2026 at 10:10 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Wed, Sep 2, 2026 at 9:44 AM Frank Li <Frank.li@oss.nxp.com> wrote: > > > > Nick Desaulniers: > > > > Is this patch good to pickup? > > Yes. > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > It does depend on > https://lore.kernel.org/lkml/20260604-tracing-fix-cfi-h-build-error-v1-1-b27015390901@kernel.org/ > though, so you might need to coordinate your PRs to Linus with Nathan. > Nathan, is https://lore.kernel.org/lkml/20260604-tracing-fix-cfi-h-build-error-v1-1-b27015390901@kernel.org/ > going via kbuild tree or what? And make sure to pick up v5, not v4. Was just looking at this thread on lore, and it seems like Frank you replied on v4, not v5. My RB tag is for v5, not v4. https://lore.kernel.org/llvm/ca86be7a-7439-49cb-9deb-8d565ca0b1dd@yoavmoshe.com/T/#mc0e4f1537fba074b962de6b4338f289dbb26c81e -- Thanks, ~Nick Desaulniers
The suspend code that runs from OCRAM is copied there with fncpy(),
which does not copy the kCFI type hash preceding the function. With
CONFIG_CFI=y the indirect call through imx6_suspend_in_ocram_fn
therefore panics.
Keep the call covered by CFI instead of exempting it:
- Add SYM_TYPED_FUNC_START_ALIGNED(), a variant of
SYM_TYPED_FUNC_START() that aligns the function entry rather than
the type hash preceding it, and use it to declare imx6_suspend():
fncpy() requires the entry point of the function it copies to be
8-byte aligned. The macro lives in arch/arm/include/asm/linkage.h
since the requirement comes from arm's fncpy().
- Add a cfi_type member at the end of struct imx6_cpu_pm_info, which
directly precedes the OCRAM copy of the function. It fits in the
struct's tail padding, so no sizes or offsets change. Fill it using
cfi_get_func_hash(), putting the hash where the caller's CFI check
expects it: four bytes before the function entry.
Also mark ccm_base, suspend_ocram_base and imx6_suspend_in_ocram_fn
as __ro_after_init: they are only written during __init, and the
function pointer in particular should not be writable afterwards.
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Yo'av Moshe <linux@yoavmoshe.com>
---
Tested on a Kobo Clara HD (i.MX6SLL) running postmarketOS
(clang/LLVM, CONFIG_CFI=y): suspend and resume work.
Note that with CONFIG_CFI=y this depends on commit 979c294509f9
("cfi: Include uaccess.h for get_kernel_nofault()"), already in
mainline, which stable backports would need as well.
Changes in v5:
- Drop the linux/uaccess.h include workaround, obsoleted by commit
979c294509f9 (Nick, Sami).
- Include linux/build_bug.h and linux/stddef.h for static_assert()
and offsetofend() (Nick).
- Replace the open-coded alignment pad with a new
SYM_TYPED_FUNC_START_ALIGNED() macro as sketched by Nick, placed in
arch/arm/include/asm/linkage.h as suggested by Sami.
Changes in v4:
- Drop the v3 __nocfi wrapper; keep the indirect call CFI-checked.
- Store the hash in a cfi_type member in the struct's tail padding
instead of open-coded pointer arithmetic.
- Use cfi_get_func_hash() instead of reading the hash manually.
- Use SYM_TYPED_FUNC_START()/SYM_FUNC_END() instead of ENTRY()/
ENDPROC() with a hand-rolled __CFI_TYPE.
arch/arm/include/asm/linkage.h | 29 +++++++++++++++++++++++++++++
arch/arm/mach-imx/pm-imx6.c | 26 +++++++++++++++++++++++---
arch/arm/mach-imx/suspend-imx6.S | 6 ++++--
3 files changed, 56 insertions(+), 5 deletions(-)
diff --git a/arch/arm/include/asm/linkage.h b/arch/arm/include/asm/linkage.h
index c467069..bba992f 100644
--- a/arch/arm/include/asm/linkage.h
+++ b/arch/arm/include/asm/linkage.h
@@ -9,4 +9,33 @@
.type name, %function; \
END(name)
+#ifdef __ASSEMBLY__
+
+/*
+ * Variants of SYM_TYPED_START/SYM_TYPED_FUNC_START that align the
+ * function entry itself instead of the kCFI type hash preceding it,
+ * for functions whose entry point must meet an alignment requirement,
+ * such as the 8-byte alignment fncpy() demands of its source.
+ */
+#ifdef CONFIG_CFI
+
+#define SYM_TYPED_START_ALIGNED(name, linkage, align) \
+ linkage(name) ASM_NL \
+ .balign align ASM_NL \
+ .fill (align) - 4, 1, 0 ASM_NL \
+ __CFI_TYPE(name) ASM_NL \
+ name:
+
+#else /* CONFIG_CFI */
+
+#define SYM_TYPED_START_ALIGNED(name, linkage, align) \
+ SYM_START(name, linkage, .balign align)
+
+#endif /* CONFIG_CFI */
+
+#define SYM_TYPED_FUNC_START_ALIGNED(name, align) \
+ SYM_TYPED_START_ALIGNED(name, SYM_L_GLOBAL, align)
+
+#endif /* __ASSEMBLY__ */
+
#endif
diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
index a671ca4..3c73e2c 100644
--- a/arch/arm/mach-imx/pm-imx6.c
+++ b/arch/arm/mach-imx/pm-imx6.c
@@ -4,6 +4,8 @@
* Copyright 2011 Linaro Ltd.
*/
+#include <linux/build_bug.h>
+#include <linux/cfi.h>
#include <linux/clk/imx.h>
#include <linux/delay.h>
#include <linux/init.h>
@@ -18,6 +20,7 @@
#include <linux/of_platform.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
+#include <linux/stddef.h>
#include <linux/suspend.h>
#include <asm/cacheflush.h>
#include <asm/fncpy.h>
@@ -61,9 +64,9 @@
#define MX6Q_SUSPEND_OCRAM_SIZE 0x1000
#define MX6_MAX_MMDC_IO_NUM 33
-static void __iomem *ccm_base;
-static void __iomem *suspend_ocram_base;
-static void (*imx6_suspend_in_ocram_fn)(void __iomem *ocram_vbase);
+static void __iomem *ccm_base __ro_after_init;
+static void __iomem *suspend_ocram_base __ro_after_init;
+static void (*imx6_suspend_in_ocram_fn)(void __iomem *ocram_vbase) __ro_after_init;
/*
* suspend ocram space layout:
@@ -229,8 +232,18 @@ struct imx6_cpu_pm_info {
struct imx6_pm_base l2_base;
u32 mmdc_io_num; /* Number of MMDC IOs which need saved/restored. */
u32 mmdc_io_val[MX6_MAX_MMDC_IO_NUM][2]; /* To save offset and value */
+ u32 cfi_type; /* kCFI type hash of imx6_suspend() */
} __aligned(8);
+/*
+ * The ocram copy of imx6_suspend() starts right after struct imx6_cpu_pm_info,
+ * and the CFI check on the indirect call reads the kCFI type hash from the
+ * four bytes preceding the function entry, so cfi_type must occupy the last
+ * four bytes of the struct, i.e. fit into its tail padding.
+ */
+static_assert(offsetofend(struct imx6_cpu_pm_info, cfi_type) ==
+ sizeof(struct imx6_cpu_pm_info));
+
void imx6_set_int_mem_clk_lpm(bool enable)
{
u32 val = readl_relaxed(ccm_base + CGPR);
@@ -568,6 +581,13 @@ static int __init imx6q_suspend_init(const struct imx6_pm_socdata *socdata)
mmdc_offset_array[i]);
}
+ /*
+ * Mask out the Thumb bit, as cfi_get_func_hash() expects the
+ * function's actual start address. Returns 0 if CONFIG_CFI=n.
+ */
+ pm_info->cfi_type =
+ cfi_get_func_hash((void *)((uintptr_t)&imx6_suspend & ~1UL));
+
imx6_suspend_in_ocram_fn = fncpy(
suspend_ocram_base + sizeof(*pm_info),
&imx6_suspend,
diff --git a/arch/arm/mach-imx/suspend-imx6.S b/arch/arm/mach-imx/suspend-imx6.S
index 63ccc2d..95d5f72 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,7 +149,8 @@
.endm
-ENTRY(imx6_suspend)
+/* fncpy() requires an 8-byte-aligned entry; see arch/arm/include/asm/fncpy.h */
+SYM_TYPED_FUNC_START_ALIGNED(imx6_suspend, 8)
ldr r1, [r0, #PM_INFO_PBASE_OFFSET]
ldr r2, [r0, #PM_INFO_RESUME_ADDR_OFFSET]
ldr r3, [r0, #PM_INFO_DDR_TYPE_OFFSET]
@@ -329,4 +331,4 @@ resume:
resume_mmdc
ret lr
-ENDPROC(imx6_suspend)
+SYM_FUNC_END(imx6_suspend)
--
2.55.0From: Frank Li <Frank.Li@nxp.com>
On Sun, 30 Aug 2026 17:51:00 +0200, Yo'av Moshe wrote:
> The suspend code that runs from OCRAM is copied there with fncpy(),
> which does not copy the kCFI type hash preceding the function. With
> CONFIG_CFI=y the indirect call through imx6_suspend_in_ocram_fn
> therefore panics.
>
> Keep the call covered by CFI instead of exempting it:
>
> [...]
Applied, thanks!
[1/1] ARM: imx: Fix suspend/resume crash with Clang CFI
commit: 242757d109bd5c9b8fb7194fb4d44625be0b859a
Best regards,
--
Frank Li <Frank.Li@nxp.com>
On Sun, Aug 30, 2026 at 8:51 AM Yo'av Moshe <linux@yoavmoshe.com> wrote: > > The suspend code that runs from OCRAM is copied there with fncpy(), > which does not copy the kCFI type hash preceding the function. With > CONFIG_CFI=y the indirect call through imx6_suspend_in_ocram_fn > therefore panics. > > Keep the call covered by CFI instead of exempting it: > > - Add SYM_TYPED_FUNC_START_ALIGNED(), a variant of > SYM_TYPED_FUNC_START() that aligns the function entry rather than > the type hash preceding it, and use it to declare imx6_suspend(): > fncpy() requires the entry point of the function it copies to be > 8-byte aligned. The macro lives in arch/arm/include/asm/linkage.h > since the requirement comes from arm's fncpy(). > > - Add a cfi_type member at the end of struct imx6_cpu_pm_info, which > directly precedes the OCRAM copy of the function. It fits in the > struct's tail padding, so no sizes or offsets change. Fill it using > cfi_get_func_hash(), putting the hash where the caller's CFI check > expects it: four bytes before the function entry. > > Also mark ccm_base, suspend_ocram_base and imx6_suspend_in_ocram_fn > as __ro_after_init: they are only written during __init, and the > function pointer in particular should not be writable afterwards. > > Suggested-by: Nick Desaulniers <ndesaulniers@google.com> > Cc: stable@vger.kernel.org > Signed-off-by: Yo'av Moshe <linux@yoavmoshe.com> Reviewed-by: Sami Tolvanen <samitolvanen@google.com> Sami
© 2016 - 2026 Red Hat, Inc.