Re: [PATCH RFC 2/6] arm64/efi: Add and parse custom PSCI EFI configuration table

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unclamped `num_features` leads to out-of-bounds read of `efi_psci.features` array.
- [High] PSCI power-down calls (CPU_SUSPEND/CPU_OFF) bypass the `arm64_efi_psci_call` epilogue, causing a permanent `preempt_count` and `TTBR0` leak.
- [High] `arm64_efi_psci_call()` lacks validation of EFI runtime services state, leading to a translation fault and panic if booted with `efi=noruntime`.
--

--- Patch [2]: [PATCH RFC 2/6] arm64/efi: Add and parse custom PSCI EFI configuration table ---
commit fe3c6fc95ebeb940962943f65e4f442c468d4718
Author: Sven Peter <sven@kernel.org>

arm64/efi: Add and parse custom PSCI EFI configuration table

This commit adds a new custom EFI configuration table and extracts a
pointer to a PSCI handler. It invokes the handler under the EFI virtual
mapping as a plain call rather than through the runtime services dispatch.
This approach relies on the firmware to not use lazy state and to be
reentrant.

> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index 30cd7f8043986..e40885567f9e9 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
[ ... ]
> @@ -242,6 +243,63 @@ bool efi_runtime_fixup_exception(struct pt_regs *regs, const char *msg)
>  /* EFI requires 8 KiB of stack space for runtime services */
>  static_assert(THREAD_SIZE >= SZ_8K);
>  
> +struct efi_psci_table efi_psci __ro_after_init;
> +static unsigned long efi_psci_handler_table __initdata = EFI_INVALID_TABLE_ADDR;
> +const efi_config_table_type_t efi_arch_tables[] __initconst = {
> +	{LINUX_EFI_ARM_PSCI_HANDLER_TABLE_GUID, &efi_psci_handler_table},
> +	{}
> +};
> +
> +static void __init arm64_efi_init_psci(void)
> +{
> +	struct efi_psci_table *psci;
> +
> +	if (efi_psci_handler_table == EFI_INVALID_TABLE_ADDR)
> +		return;
> +
> +	psci = early_memremap_ro(efi_psci_handler_table, sizeof(*psci));
> +	if (psci == NULL) {
> +		pr_warn("Unable to map PSCI table.\n");
> +		return;
> +	}
> +
> +	memcpy(&efi_psci, psci, sizeof(*psci));

[Severity: High]
Is it safe to copy the num_features field from untrusted firmware directly
into efi_psci without clamping it to EFI_PSCI_MAX_FN?

The destination structure defines features as a fixed-size array of size
EFI_PSCI_MAX_FN. If firmware provides a num_features value larger than
this, subsequent driver code relying on num_features to indicate the number
of valid entries could read past the end of the efi_psci object in .rodata.

Could we validate and clamp num_features here?

> +	early_memunmap(psci, sizeof(*psci));
> +}
> +
> +void __init arm64_efi_init(void)
> +{
> +	efi_init();
> +	arm64_efi_init_psci();
> +}
> +
> +unsigned long arm64_efi_psci_call(unsigned long function_id, unsigned long arg0,
> +				  unsigned long arg1, unsigned long arg2)
> +{
> +	unsigned long ret, flags;
> +
> +	/*
> +	 * Note that unlike for regular EFI runtime calls we don't have to save
> +	 * FP/SIMD state here because the handler ABI forbids using those.
> +	 * Likewise, we do not take any lock here because the handler has to be
> +	 * re-entrant. We couldn't take the sleeping efi_runtime_lock here
> +	 * anyway because we may be called from atomic context for cpuidle
> +	 * and CPU bring-up.
> +	 */
> +	local_irq_save(flags);
> +	efi_virtmap_load();
> +	uaccess_ttbr0_enable();
> +	post_ttbr_update_workaround();

[Severity: High]
What happens here if the system is booted with efi=noruntime?

If EFI runtime services are disabled, arm_enable_runtime_services() returns
early, and efi_mm.pgd is left as NULL. However, arm64_efi_init_psci()
parses the configuration table and registers the handler regardless of whether
efi_enabled(EFI_RUNTIME_SERVICES) is true.

When arm64_efi_psci_call() unconditionally invokes efi_virtmap_load(), it
would pass the NULL efi_mm.pgd to cpu_switch_mm(), programming a garbage
physical address into TTBR0_EL1. This could crash the system with a
translation fault when attempting to execute the unmapped EFI virtual address.

Should we check efi_enabled(EFI_RUNTIME_SERVICES) before registering the
handler or attempting to load the map?

> +
> +	ret = efi_psci.psci_handler(function_id, arg0, arg1, arg2);
> +
> +	uaccess_ttbr0_disable();
> +	efi_virtmap_unload();
> +	local_irq_restore(flags);

[Severity: High]
Can PSCI power-down calls (like CPU_SUSPEND or CPU_OFF) bypass this
epilogue?

efi_virtmap_load() calls preempt_disable(), and efi_virtmap_unload()
calls preempt_enable().

Successful CPU power-down calls do not return to the caller. For example, for
CPU_SUSPEND, execution resumes via the kernel's hardware reset vector
(cpu_resume), which bypasses this return path and jumps straight to the
caller of __cpu_suspend_enter.

As a result, the epilogue of arm64_efi_psci_call() is skipped entirely,
permanently leaking the +1 preempt_count and leaving TTBR0_EL1 incorrectly
mapped to efi_mm on the idle thread.

Is there a way to handle this bypass for suspend/off calls?

> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-efi-psci-v1-0-9efb3abf0e4c@kernel.org?part=2