Re: [PATCH v2 09/20] s390: Introduce read/write ARM sysreg instructions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Casting the physical address 'sdo' to a virtual pointer in inline assembly constraints breaks compiler alias analysis.
- [Medium] Speculative: Inconsistent physical vs virtual address usage for SAE hardware structures.
- [Medium] Speculative uninitialized memory leak if `EASR` fails and sets a condition code instead of a hard trap.
- [Low] Trailing comma in SASR and EASR assembler macros introduces an invalid empty argument.
- [Low] Silent bitwise truncation of the `flags` parameter in the `RIE_H` assembler macro.
--

commit 60f79315be3a103c198fd6153c59e6ac32172d24
Author: Steffen Eiden <seiden@linux.ibm.com>

s390: Introduce read/write ARM sysreg instructions

Introduce Extract Arm System Register and Store Arm System Register to
enable s390 hosts to read and write system registers for arm64 guests.
The new instructions use the new RIE_H instruction format.

> diff --git a/arch/s390/include/asm/sae-asm.h b/arch/s390/include/asm/sae-asm.h
> index 0000000000000..d81ed89eb4ed7
> --- /dev/null
> +++ b/arch/s390/include/asm/sae-asm.h

[ ... ]

> +/*
> + * RIE_H - RIE-h instruction format
> + *
> + * RIE-h format: <insn> R1, R3, I2, M4
> + *    +--------+----+----+----+-----------------+----+--------+
> + *    | OpCode | R1 |////| R3 |        I2       | M4 | Opcode |
> + *    +--------+----+----+----+-----------------+----+--------+
> + *    0        8    12   16   20                36   40      47
> + */
> +.macro RIE_H	opc, gr1, gr3, imm2, m4
> +	GPR_NUM	r1, \gr1
> +	GPR_NUM	r3, \gr3
> +	.byte	(\opc & 0xff00) >> 8
> +	.byte	r1 << 4
> +	.byte	(r3 << 4) | ((\imm2 & 0xf000) >> 12)
> +	.byte	((\imm2 & 0x0ff0) >> 4)
> +	.byte	((\imm2 & 0x000f) << 4) | (\m4 & 0xf)

[Severity: Low]
Does this bitwise AND silently truncate flags exceeding 4 bits?

The inline functions sasr() and easr() use the "I" constraint for the flags
parameter, which permits up to 8-bit unsigned constants (0-255). If a flag
greater than or equal to 16 is passed, it will compile successfully but the
upper bits will be silently discarded here.

> +	.byte	\opc & 0xff
> +.endm
> +
> +.macro SASR r1, r3, i2, m4
> +	RIE_H 0xed99, \r1, \r3, \i2, \m4,

[Severity: Low]
Is this trailing comma intentional?

It passes 6 arguments to RIE_H, which only declares 5 parameters. In GNU
Assembler, this creates an empty 6th argument, which can trigger a 'too
many positional arguments' error.

> +.endm
> +
> +.macro EASR r1, r3, i2, m4
> +	RIE_H 0xed9b, \r1, \r3, \i2, \m4,

[Severity: Low]
Is this trailing comma intentional?

Like SASR above, this passes an empty 6th argument to RIE_H.

> +.endm

> diff --git a/arch/s390/include/asm/sae.h b/arch/s390/include/asm/sae.h
> index 389c5ac2a36c0..6580536544eef 100644
> --- a/arch/s390/include/asm/sae.h
> +++ b/arch/s390/include/asm/sae.h

[ ... ]

> +/**
> + * sasr() - Set Arm System Register
> + * @arm_reg: ARM system register identifier; compile-time constant
> + * @val: Value to set
> + * @save_area: Pointer to SAE save area
> + * @flags: Operation flags; compile-time constant
> + *
> + * Sets an ARM system register value.
> + */
> +static __always_inline void sasr(unsigned int arm_reg, u64 val,
> +				 struct kvm_sae_save_area *save_area,
> +				 u64 flags)
> +{
> +	struct kvm_sae_save_area *sdo = (void *)save_area->sdo;

[Severity: High]
Does casting the physical address sdo to a virtual pointer without
phys_to_virt() break compiler alias analysis?

The save_area->sdo field holds a physical address. Casting it directly to a
virtual pointer and passing it to the inline assembly memory constraints
below ("+m" (*sdo)) misinforms the compiler about which memory is accessed.

Since the compiler is given a pointer missing the identity base offset, it
does not register that the actual virtual memory of sae_block is being
modified, allowing it to incorrectly reorder C-level reads/writes or cache
stale values in registers.

> +	u16 reg = arm_reg >> _SAE_ASR_REG_SHIFT;
> +
> +	asm volatile (
> +		"	SASR	%[r1],%[r3],%[i2],%[m4]\n"
> +		: "+m" (*save_area), "+m" (*sdo)
> +		: [r1] "d" (val),
> +		  [r3] "a" (save_area), [i2] "K" (reg), [m4] "I" (flags)

[Severity: Medium]
Does the SASR instruction expect a physical address here?

The sae64a() function explicitly converts sae_block to a physical address
before passing it to the hardware instruction, and save_area->sdo stores a
physical address. However, this inline assembly passes the save_area pointer
directly as a virtual address.

If SASR requires a physical address, passing a virtual address will cause a
hardware addressing exception when KASLR is active.

> +	);
> +}
> +
> +/**
> + * easr() - Extract Arm System Register
> + * @arm_reg: ARM system register identifier; compile-time constant
> + * @save_area: Pointer to SAE save area
> + * @flags: Operation flags; compile-time constant
> + *
> + * Reads an ARM system register value.
> + *
> + * Return: Register value
> + */
> +static __always_inline u64 easr(unsigned int arm_reg,
> +				const struct kvm_sae_save_area *save_area,
> +				u64 flags)
> +{
> +	struct kvm_sae_save_area *sdo = (void *)save_area->sdo;
> +	u16 reg = arm_reg >> _SAE_ASR_REG_SHIFT;
> +	u64 val;
> +
> +	asm volatile(
> +		"	EASR	%[r1],%[r3],%[i2],%[m4]\n"
> +		: [r1] "=d"(val)

[Severity: Medium]
Can this leak uninitialized stack or register data if EASR fails?

If EASR sets a condition code to report failure instead of injecting a
program check, the target register remains unmodified. Because the code
unconditionally returns val without checking a condition code and does not
initialize it, uninitialized memory could be returned.

> +		: "m"(*save_area),
> +		  "m"(*sdo), [r3] "a"(save_area), [i2] "K"(reg), [m4] "I"(flags)
> +	);
> +	return val;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831145536.913567-1-seiden@linux.ibm.com?part=9