Re: [PATCH] KVM: x86: restore IP after all far jump failures

[Radim KrÄmÃÅ]

2016-11-22 15:18-0800, Nadav Amit:
>> On Nov 22, 2016, at 12:56 PM, Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx> wrote:
>> 
>> 2016-11-22 11:43-0800, Nadav Amit:
>>> I admit my wrongdoings, but I still think the fix should have been to
>>> remove the entire recovery logic and just return X86EMUL_UNHANDLEABLE if
>>> something goes wrong (exception). This will kill the misbehaving process
>>> but keep the VM running.
>> 
>> X86EMUL_UNHANDLEABLE will kill the whole VM (on QEMU, other userspaces
>> might handle the instruction and resume KVM).
> 
> I donât think so. If CPL is not 0, handle_emulation_failure() will be called
> and will inject #UD.

This part of the emulator should only be used on Intels without
unrestricted guest, for real and unpaged protected mode.

It is unsafe as we don't make a clear distinction which instructions are
emulated for MMIO and which for old CPUs, but anyway, we should not be
emulating it only any OS that considers security.

(x86 can use pretty much any instruction for MMIO, but I think that KVM
 should let a userspace emulator handle those exotic OSes.)

>> The recovery path is in the spec, which means that nothing goes wrong.
>> I think we implement the spec quite well now, so keeping the #GP and CS
>> recovery is slightly better, although not safer.
>> 
>>> Otherwise, a malicious VM process, which can somehow control descriptors
>>> (LDT?) may modify the descriptor during the emulation and get the system
>>> to inconsistent state and prevent the VM-entry.
>> 
>> We restore the original CS -- malicious guest would get killed on a
>> failed VM entry anyway, so the difference is only in KVM internal error
>> code (assuming there are no other bugs).
>> 
>> Am I misunderstanding something?
> 
> Most likely you are right, but after doing one mistake, I donât want
> to be accountable for another.
> 
> Note there is another problematic case in em_ret_far(). If an exception
> occurs when RIP is assigned, the RSP updates (of emulate_pop() ) are not 
> going to be reverted. Can it be used for anything malicious? I donât know.

True, we are not emulating it right ... security is just side-effect.
Benefits of emulation are not worth the work so I'll prepare a patch
that just returns X86EMUL_UNHANDLEABLE.  It's not going to fix other
bugs with stack handling, but at least we won't be making it crazier.

Thanks.