Re: [PATCH v4 2/3] x86/insn-eval: Add insn_assign_reg() helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unaligned `unsigned long` pointer dereference leading to Undefined Behavior and adjacent memory overwrite during 8-bit high-byte register emulation.
--

commit a80417d075cbd125216356389da2d074bd0a780e
Author: Kiryl Shutsemau (Meta) <kas@kernel.org>

x86/insn-eval: Add insn_assign_reg() helper

This commit lifts KVM's assign_register() into <asm/insn-eval.h> as
insn_assign_reg() so it can be used by both KVM and the TDX guest #VE
handler. It rewrites the helper to use arithmetic operations instead of
pointer punning to avoid relying on specific aliasing or endianness.

> diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h
> index 4733e9064ee55..85251e718a775 100644
> --- a/arch/x86/include/asm/insn-eval.h
> +++ b/arch/x86/include/asm/insn-eval.h

[ ... ]

> @@ -46,4 +47,28 @@ enum insn_mmio_type insn_decode_mmio(struct insn *insn, int *bytes);
>  
>  bool insn_is_nop(struct insn *insn);
>  
> +/*
> + * Write @val into *@reg with x86 partial-register-write semantics: a 1-
> + * or 2-byte write leaves the upper bits of the destination untouched; a
> + * 4-byte write zero-extends to 64 bits (matching IN[BWL], MOV[BWL]
> + * etc.); an 8-byte write overwrites the full register.
> + */
> +static inline void insn_assign_reg(unsigned long *reg, u64 val, int bytes)
> +{
> +	switch (bytes) {
> +	case 1:
> +		*reg = (*reg & ~0xfful)   | (val & 0xff);
> +		break;

[Severity: High]
Does this 64-bit read-modify-write cause an unaligned access and memory
corruption when emulating high-byte registers?

In the KVM emulator, decode_register() generates a pointer offset by 1 byte
for high-byte registers like AH, CH, DH, and BH:

arch/x86/kvm/emulate.c:decode_register() {
    ...
    if (highbyte_regs && modrm_reg >= 4 && modrm_reg < 8)
        p = (unsigned char *)reg_rmw(ctxt, modrm_reg & 3) + 1;
    ...
}

When this unaligned pointer is passed to write_register_operand() and then
to insn_assign_reg(), treating it as an unsigned long pointer and performing
a 64-bit operation will result in an unaligned access.

Additionally, since the 8-byte operation starts at a +1 byte offset from
the base register in the _regs array, won't this overwrite the lowest
byte of the adjacent register in the array?

The previous implementation avoided this by casting to an 8-bit pointer
before writing:

> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index c8e292e9a24df..c80752e224ee0 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c

[ ... ]

> @@ -439,25 +440,6 @@ static void assign_masked(ulong *dest, ulong src, ulong mask)
>  	*dest = (*dest & ~mask) | (src & mask);
>  }
>  
> -static void assign_register(unsigned long *reg, u64 val, int bytes)
> -{
> -	/* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
> -	switch (bytes) {
> -	case 1:
> -		*(u8 *)reg = (u8)val;
> -		break;
> -	case 2:
> -		*(u16 *)reg = (u16)val;
> -		break;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780584300.git.kas@kernel.org?part=2