Re: [PATCH 2/2] i386: Add notify VM exit support

[Chenyi Qiang <chenyi.qiang@intel.com>]

On 3/10/2022 5:17 PM, Daniel P. Berrangé wrote:
> On Thu, Mar 10, 2022 at 05:02:05PM +0800, Chenyi Qiang wrote:
>> There are cases that malicious virtual machine can cause CPU stuck (due
>> to event windows don't open up), e.g., infinite loop in microcode when
>> nested #AC (CVE-2015-5307). No event window means no event (NMI, SMI and
>> IRQ) can be delivered. It leads the CPU to be unavailable to host or
>> other VMs. Notify VM exit is introduced to mitigate such kind of
>> attacks, which will generate a VM exit if no event window occurs in VM
>> non-root mode for a specified amount of time (notify window).
>>
>> A new KVM capability KVM_CAP_X86_NOTIFY_VMEXIT is exposed to user space
>> so that the user can query the capability and set the expected notify
>> window when creating VMs.
>>
>> If notify VM exit happens with VM_INVALID_CONTEXT, hypervisor should
>> exit to user space with the exit reason KVM_EXIT_NOTIFY to inform the
>> fatal case. Then user space can inject a SHUTDOWN event to the target
>> vcpu. This is implemented by defining a new bit in flags field of
>> kvm_vcpu_event in KVM_SET_VCPU_EVENTS ioctl.
>>
>> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
>> ---
>>   hw/i386/x86.c         | 24 ++++++++++++++++++
>>   include/hw/i386/x86.h |  3 +++
>>   target/i386/kvm/kvm.c | 58 ++++++++++++++++++++++++++++---------------
>>   3 files changed, 65 insertions(+), 20 deletions(-)
>>
>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>> index b84840a1bb..25e6c50b1e 100644
>> --- a/hw/i386/x86.c
>> +++ b/hw/i386/x86.c
>> @@ -1309,6 +1309,23 @@ static void machine_set_sgx_epc(Object *obj, Visitor *v, const char *name,
>>       qapi_free_SgxEPCList(list);
>>   }
>>   
>> +static void x86_machine_get_notify_window(Object *obj, Visitor *v,
>> +                                const char *name, void *opaque, Error **errp)
>> +{
>> +    X86MachineState *x86ms = X86_MACHINE(obj);
>> +    int32_t notify_window = x86ms->notify_window;
>> +
>> +    visit_type_int32(v, name, &notify_window, errp);
>> +}
>> +
>> +static void x86_machine_set_notify_window(Object *obj, Visitor *v,
>> +                               const char *name, void *opaque, Error **errp)
>> +{
>> +    X86MachineState *x86ms = X86_MACHINE(obj);
>> +
>> +    visit_type_int32(v, name, &x86ms->notify_window, errp);
>> +}
>> +
>>   static void x86_machine_initfn(Object *obj)
>>   {
>>       X86MachineState *x86ms = X86_MACHINE(obj);
>> @@ -1319,6 +1336,7 @@ static void x86_machine_initfn(Object *obj)
>>       x86ms->oem_id = g_strndup(ACPI_BUILD_APPNAME6, 6);
>>       x86ms->oem_table_id = g_strndup(ACPI_BUILD_APPNAME8, 8);
>>       x86ms->bus_lock_ratelimit = 0;
>> +    x86ms->notify_window = -1;
>>   }
> 
> IIUC from the kernel patch, this negative value leaves the protection
> disabled, and thus the host remains vulnerable to the CVE. I would
> expect this ought to set a suitable default value to fix the flaw.
> 

Hum, I missed some explanation in commit message.
We had some discussion about the default behavior of this feature. There 
are some concerns. e.g.
There's a possibility, however small, that a notify VM exit happens
with VM_CONTEXT_INVALID set in exit qualification, which means VM
context is corrupted. To avoid the false positive and a well-behaved
guest gets killed, we decide to make this feature opt-in.

> Regards,
> Daniel