Re: [PATCH] ARM: cfi: Fix compilation corner case

[Nathan Chancellor <nathan@kernel.org>]

On Fri, Nov 08, 2024 at 08:37:36AM +0100, Linus Walleij wrote:
> When enabling expert mode CONFIG_EXPERT and using that power
> user mode to disable the branch prediction hardening
> !CONFIG_HARDEN_BRANCH_PREDICTOR, the picky assembly linker
> in CLANG notices that some assembly in proc-v7.S does
> not have corresponding C call sites, i.e. the prototypes
> in proc-v7-bugs.c are enclosed in ifdef
> CONFIG_HARDEN_BRANCH_PREDICTOR so this assembly:
> 
> SYM_TYPED_FUNC_START(cpu_v7_smc_switch_mm)
> SYM_TYPED_FUNC_START(cpu_v7_hvc_switch_mm)
> 
> Results in:
> 
> ld.lld: error: undefined symbol: __kcfi_typeid_cpu_v7_smc_switch_mm
> >>> referenced by proc-v7.S:94 (.../arch/arm/mm/proc-v7.S:94)
> >>> arch/arm/mm/proc-v7.o:(.text+0x108) in archive vmlinux.a
> 
> ld.lld: error: undefined symbol: __kcfi_typeid_cpu_v7_hvc_switch_mm
> >>> referenced by proc-v7.S:105 (.../arch/arm/mm/proc-v7.S:105)
> >>> arch/arm/mm/proc-v7.o:(.text+0x124) in archive vmlinux.a
> 
> Fix this by adding an additional requirement that
> CONFIG_HARDEN_BRANCH_PREDICTOR has to be enabled to compile
> these assembly calls.
> 
> I suppose it wasn't a problem before because the linker is not
> so picky that other assembly symbols are actually being
> used.

I think this has been a problem since the original CFI change, so I
think it is worth dropping this block; it is more likely that
CONFIG_HARDEN_BRANCH_PREDICTOR is not often disabled outside of
randconfig, so it is hard to hit this issue in practice. As far as I can
tell, this is totally expected with the given configuration.
SYM_TYPED_FUNC_START makes use of __kcfi_typeid_<func>, which the
compiler generates for address-taken function declarations, but in this
configuration, cpu_v7_smc_switch_mm() and cpu_v7_hvc_switch_mm() have no
actual uses aside from their definition, so the compiler does not
generate these symbols, resulting in the link time error above. Perhaps
some of this could be encorporated into the beginning of the commit
message to make the issue a little more clear and less like the compiler
is at fault here ("the picky assembly linker" stands out a bit to me
there).

> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202411041456.ZsoEiD7T-lkp@intel.com/
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Regardless of the commit message nits, this seems like the right thing
to do.

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  arch/arm/mm/proc-v7.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
> index 5fb9a6aecb00..2cd933342679 100644
> --- a/arch/arm/mm/proc-v7.S
> +++ b/arch/arm/mm/proc-v7.S
> @@ -94,7 +94,7 @@ SYM_TYPED_FUNC_START(cpu_v7_dcache_clean_area)
>  	ret	lr
>  SYM_FUNC_END(cpu_v7_dcache_clean_area)
>  
> -#ifdef CONFIG_ARM_PSCI
> +#if defined(CONFIG_ARM_PSCI) && defined(CONFIG_HARDEN_BRANCH_PREDICTOR)
>  	.arch_extension sec
>  SYM_TYPED_FUNC_START(cpu_v7_smc_switch_mm)
>  	stmfd	sp!, {r0 - r3}
> 
> ---
> base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
> change-id: 20241107-fix-kcfi-bug-ae3b08cbc167
> 
> Best regards,
> -- 
> Linus Walleij <linus.walleij@linaro.org>
>