diff mbox series

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

Message ID 20220729134801.1120-1-mhal@rbox.co
State Accepted
Commit 4ac5b4237793a6db791999edd53f0396c04053cd
Headers show
Series [1/2] KVM: x86: emulator: Fix illegal LEA handling | expand

Commit Message

Michal Luczaj July 29, 2022, 1:48 p.m. UTC
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(-)

Comments

Sean Christopherson July 29, 2022, 4:41 p.m. UTC | #1
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
>
Sean Christopherson July 29, 2022, 4:53 p.m. UTC | #2
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");
Michal Luczaj July 31, 2022, 8:43 p.m. UTC | #3
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
diff mbox series

Patch

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