Re: [PATCH v8 11/20] KVM: x86: Handle EGPR index and REX2-incompatible opcodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] x86/kvm: Missing NoRex2 flag on legacy instructions allows REX2-prefixed execution without #UD
- [High] x86/kvm: NoRex2 check precedes opcode group expansion making it impossible to #UD nested instructions
--

commit a27108ff09a58dbc37193fd34a249d4d8837decb
Author: Chang S. Bae <chang.seok.bae@intel.com>

KVM: x86: Handle EGPR index and REX2-incompatible opcodes

Prepare the emulator for REX2 handling by introducing the NoRex2 opcode
flag and supporting extended register indices. For the latter, factor out
common logic for calculating register IDs.

Link: https://lore.kernel.org/1ebf3a23-5671-41c1-8daa-c83f2f105936@redhat.com

> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index c1b21282187f3..a1a567fd03780 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
[ ... ]
> @@ -4273,7 +4284,7 @@ static const struct opcode opcode_table[256] = {
>  	I2bvIP(DstDI | SrcDX | Mov | String | Unaligned, em_in, ins, check_perm_in), /* insb, insw/insd */
>  	I2bvIP(SrcSI | DstDX | String, em_out, outs, check_perm_out), /* outsb, outsw/outsd */

[Severity: High]
Should these string instructions (ins/outs) include the NoRex2 flag?
Without it, could KVM incorrectly emulate them instead of injecting a #UD
when prefixed with REX2?

>  	/* 0x70 - 0x7F */
> -	X16(D(SrcImmByte | NearBranch | IsBranch)),
> +	X16(D(SrcImmByte | NearBranch | IsBranch | NoRex2)),
>  	/* 0x80 - 0x87 */
[ ... ]
> @@ -4297,15 +4308,15 @@ static const struct opcode opcode_table[256] = {
>  	II(ImplicitOps | Stack, em_pushf, pushf),

[Severity: High]
Does pushf also need the NoRex2 flag since it takes no operands and
should #UD with REX2? Legacy return instructions (ret) seem to be missing
this flag as well.

>  	II(ImplicitOps | Stack, em_popf, popf),
>  	I(ImplicitOps, em_sahf), I(ImplicitOps, em_lahf),
>  	/* 0xA0 - 0xA7 */
[ ... ]
> @@ -4335,17 +4346,17 @@ static const struct opcode opcode_table[256] = {
>  	/* 0xD8 - 0xDF */
>  	N, E(0, &escape_d9), N, E(0, &escape_db), N, E(0, &escape_dd), N, N,

[Severity: High]
Since x87 FPU instructions do not support REX2 prefixes, should they have
the NoRex2 flag here to ensure they raise #UD?

>  	/* 0xE0 - 0xE7 */
> -	X3(I(SrcImmByte | NearBranch | IsBranch, em_loop)),
[ ... ]
> -	I2bvIP(SrcDX | DstAcc, em_in,  in,  check_perm_in),
> -	I2bvIP(SrcAcc | DstDX, em_out, out, check_perm_out),
> +	I2bvIP(SrcDX | DstAcc | NoRex2, em_in,  in,  check_perm_in),
> +	I2bvIP(SrcAcc | DstDX | NoRex2, em_out, out, check_perm_out),
>  	/* 0xF0 - 0xF7 */
>  	N, DI(ImplicitOps, icebp), N, N,
>  	DI(ImplicitOps | Priv, hlt), D(ImplicitOps),

[Severity: High]
Does hlt need the NoRex2 flag since it takes no operands? (Similarly,
syscall in twobyte_table appears to be missing it too).

>  	G(ByteOp, group3), G(0, group3),
[ ... ]
> @@ -5001,6 +5012,13 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len, int
>  		opcode = opcode_table[ctxt->b];
>  	}
>  
> +	/*
> +	 * Instructions marked with NoRex2 ignore a legacy REX prefix, but #UD
> +	 * should be raised when prefixed with REX2.
> +	 */
> +	if (opcode.flags & NoRex2 && ctxt->rex_prefix == REX2_PREFIX)
> +		opcode.flags = Undefined;
> +

[Severity: High]
Does evaluating opcode.flags & NoRex2 here occur too early? 

Since this check precedes the while (ctxt->d & GroupMask) loop that
expands opcode groups, will nested group instructions (like monitor,
mwait, or vmcall) bypass this check and fail to raise #UD when given
a REX2 prefix?

>  	if (opcode.flags & ModRM)
>  		ctxt->modrm = insn_fetch(u8, ctxt);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930210750.1487547-1-chang.seok.bae@intel.com?part=11