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@xxxxxxx>
> ---
> 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@xxxxxxxxxx>
> I(ImplicitOps | SrcMem16 | ModRM, em_mov_sreg_rm),
> G(0, group1A),
> /* 0x90 - 0x97 */
> --
> 2.32.0
>
