Re: [PATCH v6 25/33] s390: Introduce Start Arm Execution instruction

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] __sae64a lacks integration with the host's machine check handler, causing guest-induced machine checks to panic the host kernel.
- [High] Missing branch prediction isolation (Spectre v2 mitigation) around the SAE instruction.
- [High] Missing host interrupt intercept/exit protocol for SAE, which can lead to a CPU lockup.
- [Medium] Potential machine check misattribution due to a missing NOP padding instruction immediately after the SAE instruction.
--

--- Patch [25]: [PATCH v6 25/33] s390: Introduce Start Arm Execution instruction ---
commit 06d16be51e15035979052bdd29d727897f102ac2
Author: Steffen Eiden <[email protected]>

s390: Introduce Start Arm Execution instruction

This commit introduces the Start Arm Execution (SAE) instruction to enable
accelerated execution of arm64 virtual machines on s390. It adds the __sae64a
assembly function to handle the execution context switch.

[ ... ]

> diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S
> index 10dd9bbdf9856..5d9817acab0b6 100644
> --- a/arch/s390/kernel/entry.S
> +++ b/arch/s390/kernel/entry.S
> @@ -245,6 +245,28 @@ EXPORT_SYMBOL(__sie64a)
>  EXPORT_SYMBOL(sie_exit)
>  #endif
>  
> +#if IS_ENABLED(CONFIG_KVM_ARM64)
> +/*
> + * __sae64a calling convention:
> + * %r2 pointer to sae control block physical address
> + */
> +SYM_FUNC_START(__sae64a)
> +	stmg	%r6,%r14,__SF_GPRS(%r15)	# store kernel registers
> +	STBEAR	__SF_SAE_BEAR(%r15)		# save breaking event address register

[Severity: High]
Are we missing branch predictor isolation (BPEXIT/BPOFF/BPENTER) around
the sae instruction?

Without these macros, the host branch predictor might be left exposed to
the guest. Could a malicious guest intentionally mistrain the branch predictor
to mount a Spectre v2 branch target injection attack against the host kernel?

[Severity: High]
Should this function set a thread flag before execution, similar to __TI_sie,
to allow host interrupts to break the execution loop?

Without a flag like __TI_sae, host interrupt handlers (like ext_int_handler
and io_int_handler) will return directly to the sae instruction instead of
rewriting the return address. Could this cause the host VCPU thread to enter
an infinite execution loop, preventing signal delivery and causing a CPU lockup?

> +	.insn	rre,0xb9a50000,%r2,0		# Start Arm Execution

[Severity: High]
Does this instruction need .Lsae_entry and .Lsae_leave bounds, similar
to __sie64a, so the machine check handler can detect guest context?

In mcck_int_handler(), it checks for .Lsie_entry bounds:

mcck_int_handler:
	larl	%r14,.Lsie_entry
	clgrjl	%r9,%r14, 4f
	larl	%r14,.Lsie_leave
	clgrjhe	%r9,%r14, 4f
	lg	%r10,__LC_PCPU(%r13)
	oi	__PCPU_FLAGS+7(%r10), _CIF_MCCK_GUEST

Without equivalent bounds for sae, a guest-induced machine check might be
incorrectly handled as a host kernel machine check, leaving _CIF_MCCK_GUEST
unset and panicking the host.

> +	LBEAR	__SF_SAE_BEAR(%r15)		# restore breaking event address register

[Severity: Medium]
Is a padding instruction needed immediately following the sae instruction?

In __sie64a, a nopr 7 instruction immediately follows the sie instruction
to safely absorb deferred boundary machine checks without attributing them to
the host context. Without padding, could a deferred machine check be incorrectly
attributed to the host?

> +	lmg	%r6,%r14,__SF_GPRS(%r15)	# restore kernel registers
> +	xgr	%r0,%r0				# clear guest registers to
> +	xgr	%r1,%r1				# prevent speculative use
> +	xgr	%r2,%r2
> +	xgr	%r3,%r3
> +	xgr	%r4,%r4
> +	xgr	%r5,%r5
> +	BR_EX	%r14
> +SYM_FUNC_END(__sae64a)
> +EXPORT_SYMBOL(__sae64a)
> +#endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=25