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

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?

 arch/x86/kvm/emulate.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

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
>

On Fri, Jul 29, 2022, Michal Luczaj wrote:
> + * To trigger the emulator and feed it with LEA, we VM-exit on IO (with a
> + * single OUTS), then race decoder's instruction fetch - hoping to replace the
> + * initial IO op with an illegal LEA.

Rather than play games with memory, can't we just require and use force_emulation_prefix
to force KVM to emulate a bogus LEA encoding?  emulator.c in KVM-unit-tests already has
most of what you need, e.g. I believe it's just a matter of implementing
test_illegal_lea().  That test already has test_smsw_reg(), which is darn near the
same thing, it just expects a different result (success instead of #UD).

diff --git a/x86/emulator.c b/x86/emulator.c
index cd78e3cb..dd50578d 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -1193,6 +1193,7 @@ int main(void)
                test_smsw_reg(mem);
                test_nop(mem);
                test_mov_dr(mem);
+               test_illegal_lea();
        } else {
                report_skip("skipping register-only tests, "
                            "use kvm.force_emulation_prefix=1 to enable");

On 7/29/22 18:53, Sean Christopherson wrote:
> On Fri, Jul 29, 2022, Michal Luczaj wrote:
>> + * To trigger the emulator and feed it with LEA, we VM-exit on IO (with a
>> + * single OUTS), then race decoder's instruction fetch - hoping to replace the
>> + * initial IO op with an illegal LEA.
> 
> Rather than play games with memory, can't we just require and use force_emulation_prefix
> to force KVM to emulate a bogus LEA encoding?  emulator.c in KVM-unit-tests already has
> most of what you need, e.g. I believe it's just a matter of implementing
> test_illegal_lea().  That test already has test_smsw_reg(), which is darn near the
> same thing, it just expects a different result (success instead of #UD).
> 
> diff --git a/x86/emulator.c b/x86/emulator.c
> index cd78e3cb..dd50578d 100644
> --- a/x86/emulator.c
> +++ b/x86/emulator.c
> @@ -1193,6 +1193,7 @@ int main(void)
>                 test_smsw_reg(mem);
>                 test_nop(mem);
>                 test_mov_dr(mem);
> +               test_illegal_lea();
>         } else {
>                 report_skip("skipping register-only tests, "
>                             "use kvm.force_emulation_prefix=1 to enable");
> 

Ahh, right. Using force_emulation_prefix seems way better. Thanks!

I'll add "kvm-unit-tests" in the subject and send as v2 PATCH.

Michal