Re: [PATCH 1/2] KVM: x86: emulator: Fix illegal LEA handling

[Sean Christopherson <seanjc@google.com>]

On Fri, Jul 29, 2022, Michal Luczaj wrote:
> The emulator mishandles LEA with register source operand. Even though such
> LEA is illegal, it can be encoded and fed to CPU. In which case real
> hardware throws #UD. The emulator, instead, returns address of
> x86_emulate_ctxt._regs. This info leak hurts host's kASLR.
> 
> Tell the decoder that illegal LEA is not to be emulated.
> 
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
> What the emulator does for LEA is simply:
> 	
> 	case 0x8d: /* lea r16/r32, m */
> 		ctxt->dst.val = ctxt->src.addr.mem.ea;
> 		break;
> 
> And it makes sense if you assume that LEA's source operand is always
> memory. But because there is a race window between VM-exit and the decoder
> instruction fetch, emulator can be force fed an arbitrary opcode of choice.
> Including some that are simply illegal and would cause #UD in normal
> circumstances. Such as a LEA with a register-direct source operand -- for
> which the emulator sets `op->addr.reg`, but reads `op->addr.mem.ea`.
> 
> 		union {
> 			unsigned long *reg;
> 			struct segmented_address {
> 				ulong ea;
> 				unsigned seg;
> 			} mem;
> 			...
> 		} addr;
> 
> Because `reg` and `mem` are in union, emulator reveals address in host's
> memory.
> 
> I hope this patch is not considered an `instr_dual` abuse?

Nope, definitely not abuse.  This is very similar to how both SVM and VMX usurped
"reserved" Mod=3 (register) encodings from SGDT, SIDT, LGDT, LIDT, and INVLPG
to implement virtualization instructions.  I.e. if the Mod=3 encoding is ever
repurposed, then using instr_dual will become necessary.  I'm actually somewhat
surprised that Mod=3 hasn't already been repurposed.

>  arch/x86/kvm/emulate.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index f8382abe22ff..7c14706372d0 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -4566,6 +4566,10 @@ static const struct mode_dual mode_dual_63 = {
>  	N, I(DstReg | SrcMem32 | ModRM | Mov, em_movsxd)
>  };
>  
> +static const struct instr_dual instr_dual_8d = {
> +	D(DstReg | SrcMem | ModRM | NoAccess), N
> +};
> +
>  static const struct opcode opcode_table[256] = {
>  	/* 0x00 - 0x07 */
>  	F6ALU(Lock, em_add),
> @@ -4622,7 +4626,7 @@ static const struct opcode opcode_table[256] = {
>  	I2bv(DstMem | SrcReg | ModRM | Mov | PageTable, em_mov),
>  	I2bv(DstReg | SrcMem | ModRM | Mov, em_mov),
>  	I(DstMem | SrcNone | ModRM | Mov | PageTable, em_mov_rm_sreg),
> -	D(ModRM | SrcMem | NoAccess | DstReg),
> +	ID(0, &instr_dual_8d),

Somewhat tentatively because I'm terrible at reading the emulator's decoder, but
this looks correct...

Reviewed-by: Sean Christopherson <seanjc@google.com>

>  	I(ImplicitOps | SrcMem16 | ModRM, em_mov_sreg_rm),
>  	G(0, group1A),
>  	/* 0x90 - 0x97 */
> -- 
> 2.32.0
>