| Message ID | 20220729134801.1120-1-mhal@rbox.co (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4ac5b4237793a6db791999edd53f0396c04053cd |
| Headers | show |
| Series | [1/2] KVM: x86: emulator: Fix illegal LEA handling | expand |
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 >
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 */
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(-)