[FYI PATCH 0/3] CVE-2020-2732

[FYI PATCH 0/3] CVE-2020-2732

[Paolo Bonzini]

vmx_check_intercept is not yet fully implemented by KVM on Intel processors,
causing e.g. the I/O or MSR interception bitmaps not to be checked.
In general we can just disallow instruction emulation on behalf of L1,
but this series also implements I/O port checks.

Paolo

Oliver Upton (2):
  KVM: nVMX: Refactor IO bitmap checks into helper function
  KVM: nVMX: Check IO instruction VM-exit conditions

Paolo Bonzini (1):
  KVM: nVMX: Don't emulate instructions in guest mode

 arch/x86/kvm/vmx/nested.c | 39 ++++++++++++++++++++-----------
 arch/x86/kvm/vmx/nested.h |  2 ++
 arch/x86/kvm/vmx/vmx.c    | 59 +++++++++++++++++++++++++++++++++++++++++------
 3 files changed, 79 insertions(+), 21 deletions(-)

-- 
1.8.3.1

[FYI PATCH 1/3] KVM: nVMX: Don't emulate instructions in guest mode

[Paolo Bonzini]

vmx_check_intercept is not yet fully implemented. To avoid emulating
instructions disallowed by the L1 hypervisor, refuse to emulate
instructions by default.

Cc: stable@vger.kernel.org
[Made commit, added commit msg - Oliver]
Signed-off-by: Oliver Upton <oupton@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/vmx/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index dcca514ffd42..5801a86f9c24 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7164,7 +7164,7 @@ static int vmx_check_intercept(struct kvm_vcpu *vcpu,
 	}
 
 	/* TODO: check more intercepts... */
-	return X86EMUL_CONTINUE;
+	return X86EMUL_UNHANDLEABLE;
 }
 
 #ifdef CONFIG_X86_64
-- 
1.8.3.1

[FYI PATCH 2/3] KVM: nVMX: Refactor IO bitmap checks into helper function

[Paolo Bonzini]

From: Oliver Upton <oupton@google.com>

Checks against the IO bitmap are useful for both instruction emulation
and VM-exit reflection. Refactor the IO bitmap checks into a helper
function.

Signed-off-by: Oliver Upton <oupton@google.com>
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/vmx/nested.c | 39 +++++++++++++++++++++++++--------------
 arch/x86/kvm/vmx/nested.h |  2 ++
 2 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 50d8dbb3616d..f979832c394d 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5312,24 +5312,17 @@ static int handle_vmfunc(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
-
-static bool nested_vmx_exit_handled_io(struct kvm_vcpu *vcpu,
-				       struct vmcs12 *vmcs12)
+/*
+ * Return true if an IO instruction with the specified port and size should cause
+ * a VM-exit into L1.
+ */
+bool nested_vmx_check_io_bitmaps(struct kvm_vcpu *vcpu, unsigned int port,
+				 int size)
 {
-	unsigned long exit_qualification;
+	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
 	gpa_t bitmap, last_bitmap;
-	unsigned int port;
-	int size;
 	u8 b;
 
-	if (!nested_cpu_has(vmcs12, CPU_BASED_USE_IO_BITMAPS))
-		return nested_cpu_has(vmcs12, CPU_BASED_UNCOND_IO_EXITING);
-
-	exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
-
-	port = exit_qualification >> 16;
-	size = (exit_qualification & 7) + 1;
-
 	last_bitmap = (gpa_t)-1;
 	b = -1;
 
@@ -5356,6 +5349,24 @@ static bool nested_vmx_exit_handled_io(struct kvm_vcpu *vcpu,
 	return false;
 }
 
+static bool nested_vmx_exit_handled_io(struct kvm_vcpu *vcpu,
+				       struct vmcs12 *vmcs12)
+{
+	unsigned long exit_qualification;
+	unsigned int port;
+	int size;
+
+	if (!nested_cpu_has(vmcs12, CPU_BASED_USE_IO_BITMAPS))
+		return nested_cpu_has(vmcs12, CPU_BASED_UNCOND_IO_EXITING);
+
+	exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
+
+	port = exit_qualification >> 16;
+	size = (exit_qualification & 7) + 1;
+
+	return nested_vmx_check_io_bitmaps(vcpu, port, size);
+}
+
 /*
  * Return 1 if we should exit from L2 to L1 to handle an MSR access,
  * rather than handle it ourselves in L0. I.e., check whether L1 expressed
diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
index 1db388f2a444..9aeda46f473e 100644
--- a/arch/x86/kvm/vmx/nested.h
+++ b/arch/x86/kvm/vmx/nested.h
@@ -33,6 +33,8 @@ void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
 int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification,
 			u32 vmx_instruction_info, bool wr, int len, gva_t *ret);
 void nested_vmx_pmu_entry_exit_ctls_update(struct kvm_vcpu *vcpu);
+bool nested_vmx_check_io_bitmaps(struct kvm_vcpu *vcpu, unsigned int port,
+				 int size);
 
 static inline struct vmcs12 *get_vmcs12(struct kvm_vcpu *vcpu)
 {
-- 
1.8.3.1

[FYI PATCH 3/3] KVM: nVMX: Check IO instruction VM-exit conditions

[Paolo Bonzini]

From: Oliver Upton <oupton@google.com>

Consult the 'unconditional IO exiting' and 'use IO bitmaps' VM-execution
controls when checking instruction interception. If the 'use IO bitmaps'
VM-execution control is 1, check the instruction access against the IO
bitmaps to determine if the instruction causes a VM-exit.

Signed-off-by: Oliver Upton <oupton@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/vmx/nested.c |  2 +-
 arch/x86/kvm/vmx/vmx.c    | 57 ++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index f979832c394d..e920d7834d73 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5353,7 +5353,7 @@ static bool nested_vmx_exit_handled_io(struct kvm_vcpu *vcpu,
 				       struct vmcs12 *vmcs12)
 {
 	unsigned long exit_qualification;
-	unsigned int port;
+	unsigned short port;
 	int size;
 
 	if (!nested_cpu_has(vmcs12, CPU_BASED_USE_IO_BITMAPS))
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 5801a86f9c24..63aaf44edd1f 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7145,6 +7145,39 @@ static void vmx_request_immediate_exit(struct kvm_vcpu *vcpu)
 	to_vmx(vcpu)->req_immediate_exit = true;
 }
 
+static int vmx_check_intercept_io(struct kvm_vcpu *vcpu,
+				  struct x86_instruction_info *info)
+{
+	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
+	unsigned short port;
+	bool intercept;
+	int size;
+
+	if (info->intercept == x86_intercept_in ||
+	    info->intercept == x86_intercept_ins) {
+		port = info->src_val;
+		size = info->dst_bytes;
+	} else {
+		port = info->dst_val;
+		size = info->src_bytes;
+	}
+
+	/*
+	 * If the 'use IO bitmaps' VM-execution control is 0, IO instruction
+	 * VM-exits depend on the 'unconditional IO exiting' VM-execution
+	 * control.
+	 *
+	 * Otherwise, IO instruction VM-exits are controlled by the IO bitmaps.
+	 */
+	if (!nested_cpu_has(vmcs12, CPU_BASED_USE_IO_BITMAPS))
+		intercept = nested_cpu_has(vmcs12,
+					   CPU_BASED_UNCOND_IO_EXITING);
+	else
+		intercept = nested_vmx_check_io_bitmaps(vcpu, port, size);
+
+	return intercept ? X86EMUL_UNHANDLEABLE : X86EMUL_CONTINUE;
+}
+
 static int vmx_check_intercept(struct kvm_vcpu *vcpu,
 			       struct x86_instruction_info *info,
 			       enum x86_intercept_stage stage)
@@ -7152,18 +7185,30 @@ static int vmx_check_intercept(struct kvm_vcpu *vcpu,
 	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
 	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
 
+	switch (info->intercept) {
 	/*
 	 * RDPID causes #UD if disabled through secondary execution controls.
 	 * Because it is marked as EmulateOnUD, we need to intercept it here.
 	 */
-	if (info->intercept == x86_intercept_rdtscp &&
-	    !nested_cpu_has2(vmcs12, SECONDARY_EXEC_RDTSCP)) {
-		ctxt->exception.vector = UD_VECTOR;
-		ctxt->exception.error_code_valid = false;
-		return X86EMUL_PROPAGATE_FAULT;
-	}
+	case x86_intercept_rdtscp:
+		if (!nested_cpu_has2(vmcs12, SECONDARY_EXEC_RDTSCP)) {
+			ctxt->exception.vector = UD_VECTOR;
+			ctxt->exception.error_code_valid = false;
+			return X86EMUL_PROPAGATE_FAULT;
+		}
+		break;
+
+	case x86_intercept_in:
+	case x86_intercept_ins:
+	case x86_intercept_out:
+	case x86_intercept_outs:
+		return vmx_check_intercept_io(vcpu, info);
 
 	/* TODO: check more intercepts... */
+	default:
+		break;
+	}
+
 	return X86EMUL_UNHANDLEABLE;
 }
 
-- 
1.8.3.1

Re: [FYI PATCH 1/3] KVM: nVMX: Don't emulate instructions in guest mode

[Jan Kiszka]

On 24.02.20 19:56, Paolo Bonzini wrote:
> vmx_check_intercept is not yet fully implemented. To avoid emulating
> instructions disallowed by the L1 hypervisor, refuse to emulate
> instructions by default.
>
> Cc: stable@vger.kernel.org
> [Made commit, added commit msg - Oliver]
> Signed-off-by: Oliver Upton <oupton@google.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   arch/x86/kvm/vmx/vmx.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index dcca514ffd42..5801a86f9c24 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7164,7 +7164,7 @@ static int vmx_check_intercept(struct kvm_vcpu *vcpu,
>   	}
>
>   	/* TODO: check more intercepts... */
> -	return X86EMUL_CONTINUE;
> +	return X86EMUL_UNHANDLEABLE;
>   }
>
>   #ifdef CONFIG_X86_64
>

Is this expected to cause regressions on less common workloads?
Jailhouse as L1 now fails when Linux as L2 tries to boot a CPU: L2-Linux
gets a triple fault on load_current_idt() in start_secondary(). Only
bisected so far, didn't debug further.

Jan

Re: [FYI PATCH 1/3] KVM: nVMX: Don't emulate instructions in guest mode

[Oliver Upton]

Hi Jan,

On Sat, Feb 29, 2020 at 10:00 AM Jan Kiszka <jan.kiszka@web.de> wrote:
> Is this expected to cause regressions on less common workloads?
> Jailhouse as L1 now fails when Linux as L2 tries to boot a CPU: L2-Linux
> gets a triple fault on load_current_idt() in start_secondary(). Only
> bisected so far, didn't debug further.

I'm guessing that Jailhouse doesn't use 'descriptor table exiting', so
when KVM gets the corresponding exit from L2 the emulation burden is
on L0. We now refuse the emulation, which kicks a #UD back to L2. I
can get a patch out quickly to address this case (like the PIO exiting
one that came in this series) but the eventual solution is to map
emulator intercept checks into VM-exits + call into the
nested_vmx_exit_reflected() plumbing.

--
Thanks,
Oliver

Re: [FYI PATCH 1/3] KVM: nVMX: Don't emulate instructions in guest mode

[Jim Mattson]

On Sat, Feb 29, 2020 at 10:33 AM Oliver Upton <oupton@google.com> wrote:
>
> Hi Jan,
>
> On Sat, Feb 29, 2020 at 10:00 AM Jan Kiszka <jan.kiszka@web.de> wrote:
> > Is this expected to cause regressions on less common workloads?
> > Jailhouse as L1 now fails when Linux as L2 tries to boot a CPU: L2-Linux
> > gets a triple fault on load_current_idt() in start_secondary(). Only
> > bisected so far, didn't debug further.
>
> I'm guessing that Jailhouse doesn't use 'descriptor table exiting', so
> when KVM gets the corresponding exit from L2 the emulation burden is
> on L0. We now refuse the emulation, which kicks a #UD back to L2. I
> can get a patch out quickly to address this case (like the PIO exiting
> one that came in this series) but the eventual solution is to map
> emulator intercept checks into VM-exits + call into the
> nested_vmx_exit_reflected() plumbing.

If Jailhouse doesn't use descriptor table exiting, why is L0
intercepting descriptor table instructions? Is this just so that L0
can partially emulate UMIP on hardware that doesn't support it?

Re: [FYI PATCH 1/3] KVM: nVMX: Don't emulate instructions in guest mode

[Jan Kiszka]

On 29.02.20 20:00, Jim Mattson wrote:
> On Sat, Feb 29, 2020 at 10:33 AM Oliver Upton <oupton@google.com> wrote:
>>
>> Hi Jan,
>>
>> On Sat, Feb 29, 2020 at 10:00 AM Jan Kiszka <jan.kiszka@web.de> wrote:
>>> Is this expected to cause regressions on less common workloads?
>>> Jailhouse as L1 now fails when Linux as L2 tries to boot a CPU: L2-Linux
>>> gets a triple fault on load_current_idt() in start_secondary(). Only
>>> bisected so far, didn't debug further.
>>
>> I'm guessing that Jailhouse doesn't use 'descriptor table exiting', so
>> when KVM gets the corresponding exit from L2 the emulation burden is
>> on L0. We now refuse the emulation, which kicks a #UD back to L2. I
>> can get a patch out quickly to address this case (like the PIO exiting
>> one that came in this series) but the eventual solution is to map
>> emulator intercept checks into VM-exits + call into the
>> nested_vmx_exit_reflected() plumbing.
>
> If Jailhouse doesn't use descriptor table exiting, why is L0
> intercepting descriptor table instructions? Is this just so that L0
> can partially emulate UMIP on hardware that doesn't support it?
>

That seems to be the case: My host lacks umip, L1 has it. So, KVM is
intercepting descriptor table load instructions to emulate umip.
Jailhouse never activates that interception.

Jan

Re: [FYI PATCH 1/3] KVM: nVMX: Don't emulate instructions in guest mode

[Jim Mattson]

Since UMIP emulation is broken, I'm not sure why anyone would use it.
(Sorry, Paolo.)

On Sat, Feb 29, 2020 at 11:21 AM Jan Kiszka <jan.kiszka@web.de> wrote:
>
> On 29.02.20 20:00, Jim Mattson wrote:
> > On Sat, Feb 29, 2020 at 10:33 AM Oliver Upton <oupton@google.com> wrote:
> >>
> >> Hi Jan,
> >>
> >> On Sat, Feb 29, 2020 at 10:00 AM Jan Kiszka <jan.kiszka@web.de> wrote:
> >>> Is this expected to cause regressions on less common workloads?
> >>> Jailhouse as L1 now fails when Linux as L2 tries to boot a CPU: L2-Linux
> >>> gets a triple fault on load_current_idt() in start_secondary(). Only
> >>> bisected so far, didn't debug further.
> >>
> >> I'm guessing that Jailhouse doesn't use 'descriptor table exiting', so
> >> when KVM gets the corresponding exit from L2 the emulation burden is
> >> on L0. We now refuse the emulation, which kicks a #UD back to L2. I
> >> can get a patch out quickly to address this case (like the PIO exiting
> >> one that came in this series) but the eventual solution is to map
> >> emulator intercept checks into VM-exits + call into the
> >> nested_vmx_exit_reflected() plumbing.
> >
> > If Jailhouse doesn't use descriptor table exiting, why is L0
> > intercepting descriptor table instructions? Is this just so that L0
> > can partially emulate UMIP on hardware that doesn't support it?
> >
>
> That seems to be the case: My host lacks umip, L1 has it. So, KVM is
> intercepting descriptor table load instructions to emulate umip.
> Jailhouse never activates that interception.
>
> Jan

Re: [FYI PATCH 1/3] KVM: nVMX: Don't emulate instructions in guest mode

[Jan Kiszka]

On 29.02.20 21:17, Jim Mattson wrote:
> Since UMIP emulation is broken, I'm not sure why anyone would use it.
> (Sorry, Paolo.)

FWIW, adding "-umip" to the "-cpu host" in my qemu setup works around
the bug.

Jan

>
> On Sat, Feb 29, 2020 at 11:21 AM Jan Kiszka <jan.kiszka@web.de> wrote:
>>
>> On 29.02.20 20:00, Jim Mattson wrote:
>>> On Sat, Feb 29, 2020 at 10:33 AM Oliver Upton <oupton@google.com> wrote:
>>>>
>>>> Hi Jan,
>>>>
>>>> On Sat, Feb 29, 2020 at 10:00 AM Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>> Is this expected to cause regressions on less common workloads?
>>>>> Jailhouse as L1 now fails when Linux as L2 tries to boot a CPU: L2-Linux
>>>>> gets a triple fault on load_current_idt() in start_secondary(). Only
>>>>> bisected so far, didn't debug further.
>>>>
>>>> I'm guessing that Jailhouse doesn't use 'descriptor table exiting', so
>>>> when KVM gets the corresponding exit from L2 the emulation burden is
>>>> on L0. We now refuse the emulation, which kicks a #UD back to L2. I
>>>> can get a patch out quickly to address this case (like the PIO exiting
>>>> one that came in this series) but the eventual solution is to map
>>>> emulator intercept checks into VM-exits + call into the
>>>> nested_vmx_exit_reflected() plumbing.
>>>
>>> If Jailhouse doesn't use descriptor table exiting, why is L0
>>> intercepting descriptor table instructions? Is this just so that L0
>>> can partially emulate UMIP on hardware that doesn't support it?
>>>
>>
>> That seems to be the case: My host lacks umip, L1 has it. So, KVM is
>> intercepting descriptor table load instructions to emulate umip.
>> Jailhouse never activates that interception.
>>
>> Jan