[PATCH v3 0/8] KVM: x86: Fix emulated MOV DR{4,5} #GP bugs

[PATCH v3 0/8] KVM: x86: Fix emulated MOV DR{4,5} #GP bugs

[Sean Christopherson]

Carlos' fix for a bug where KVM fails to signal #GP on emulated MOV DR to
DR4 or DR5, plus a pile of follow-up fixes priority goofs (#UD versus #GP,
#GP vs #DB, and #GP versus #VMEXIT on SVM).  The rest are opportunistic
cleanups.

v3:
 - Fix a #UD vs #GP due to CPL>0 bug.
 - Prioritize DR7.GD=1 #DB over CPL>0 #GP on Intel.

v2: https://lore.kernel.org/all/20260603230718.1733483-1-seanjc@google.com

v1: https://lore.kernel.org/all/20260601133320.91479-2-clopez@suse.de

Carlos López (1):
  KVM: x86: Treat any non-zero return from set_dr() as a faulting
    condition

Sean Christopherson (7):
  KVM: x86: Prioritize DR7.GD #DB over #GP due to illegal DR6/7 value
  KVM: x86: Manually check DR4/5 write values to fix SVM intercept
    priority
  KVM: x86: Prioritize #UD on MOV DR over #GP due to non-zero CPL
  KVM: VMX: Prioritize DR7.GD=1 #DB over CPL>0 #GP on Intel
  KVM: x86: Use kvm_dr{6,7}_valid() to check DR{4,5,6,7} write values in
    emulator
  KVM: x86: WARN if MOV DR emulation hits a "too late" #GP
  KVM: x86: Read CR4.DE in emulator if and only if accessing DR4 or DR5

 arch/x86/kvm/emulate.c | 49 +++++++++++++++++++++++++++++++-----------
 arch/x86/kvm/vmx/vmx.c |  6 +++---
 2 files changed, 40 insertions(+), 15 deletions(-)


base-commit: c1f7303302927f9cbf4efedf70f0512cde168c65
-- 
2.54.0.1136.gdb2ca164c4-goog

[PATCH v3 1/8] KVM: x86: Treat any non-zero return from set_dr() as a faulting condition

[Sean Christopherson]

From: Carlos López <clopez@suse.de>

When emulating a MOV to a debug register, em_dr_write() calls
@ctxt->ops->set_dr(), which is forwarded to emulator_set_dr() and
then kvm_set_dr(). The latter checks that the written value is valid,
otherwise returning an error, in which case the emulator is supposed to
inject a #GP fault into the guest.

Commit 996ff5429e98 ("KVM: x86: move kvm_inject_gp up from kvm_set_dr
to callers") changed the contract of kvm_set_dr() (and thus
emulator_set_dr()), returning 1 as an error instead of -1, but the
caller in em_dr_write() was never updated, checking only if the returned
value is negative. The end result is that em_dr_write() does not detect
the error, so an invalid write does not generate a #GP, but at the same
time the register value is not updated.

The practical impact is limited, as check_dr_write() already checks DR6
and DR7 manually. However, it misses DR4/DR5, which alias DR6/DR7 when
CR4.DE=0.

Fix the bug by treating any non-zero return from set_dr() as a reason to
inject #GP.

Note, the manual checks on DR6 and DR7 are flawed, as they incorrectly
prioritize the #GP over a DR7.GD=1 #DB (the General Detect #DB has
priority on both Intel and AMD).

Note #2, relying on ->set_dr() to detect #GP is also flawed as all
exceptions have higher priority than the instruction intercept on SVM,
i.e. the manual checks need to be extended to DR4 and DR5 (after the
priority bug is fixed).

Fixes: 996ff5429e98 ("KVM: x86: move kvm_inject_gp up from kvm_set_dr to callers")
Signed-off-by: Carlos López <clopez@suse.de>
Link: https://patch.msgid.link/20260601133320.91479-2-clopez@suse.de
[sean: drop explicit "!= 0", massage changelog]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b566ab5c7515..75cd8b6136aa 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3299,7 +3299,7 @@ static int em_dr_write(struct x86_emulate_ctxt *ctxt)
 		val = ctxt->src.val & ~0U;
 
 	/* #UD condition is already handled. */
-	if (ctxt->ops->set_dr(ctxt, ctxt->modrm_reg, val) < 0)
+	if (ctxt->ops->set_dr(ctxt, ctxt->modrm_reg, val))
 		return emulate_gp(ctxt, 0);
 
 	/* Disable writeback. */
-- 
2.54.0.1136.gdb2ca164c4-goog

[PATCH v3 2/8] KVM: x86: Prioritize DR7.GD #DB over #GP due to illegal DR6/7 value

[Sean Christopherson]

When emulating a MOV DR, specifically a write to DR6 or DR7, treat a #DB
due to DR7.GD (General Detect) as higher priority than a #GP due to an
illegal value.  While neither Intel's SDM nor AMD's APM says anything
about the relative priority, empirical testing on Intel and AMD shows that
the #DB has higher priority.  And for VMX, where the instruction intercept
has priority over *all* exceptions, KVM already treats the #DB as having
higher priority.

Cc: Maciej W. Rozycki <macro@orcam.me.uk>
Fixes: 3b88e41a4134 ("KVM: SVM: Add intercept check for accessing dr registers")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 75cd8b6136aa..4484c5fa19e3 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3854,11 +3854,16 @@ static int check_dr_write(struct x86_emulate_ctxt *ctxt)
 {
 	u64 new_val = ctxt->src.val64;
 	int dr = ctxt->modrm_reg;
+	int rc;
+
+	rc = check_dr_read(ctxt);
+	if (rc != X86EMUL_CONTINUE)
+		return rc;
 
 	if ((dr == 6 || dr == 7) && (new_val & 0xffffffff00000000ULL))
 		return emulate_gp(ctxt, 0);
 
-	return check_dr_read(ctxt);
+	return X86EMUL_CONTINUE;
 }
 
 static int check_svme(struct x86_emulate_ctxt *ctxt)
-- 
2.54.0.1136.gdb2ca164c4-goog

[PATCH v3 3/8] KVM: x86: Manually check DR4/5 write values to fix SVM intercept priority

[Sean Christopherson]

Manually (pre)check the values being written to DR4/5, i.e. the DR6/DR7
aliases, instead of relying on ->set_dr() => kvm_set_dr() to signal a #GP.
SVM unfortunately prioritizes all exceptions over an instruction intercept,
i.e. nSVM is relying on the emulator to perform *all* exception checks
prior to attempting to execute the instruction.

Fixes: 3b88e41a4134 ("KVM: SVM: Add intercept check for accessing dr registers")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 4484c5fa19e3..a1bccab0eefe 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3853,15 +3853,23 @@ static int check_dr_read(struct x86_emulate_ctxt *ctxt)
 static int check_dr_write(struct x86_emulate_ctxt *ctxt)
 {
 	u64 new_val = ctxt->src.val64;
-	int dr = ctxt->modrm_reg;
 	int rc;
 
 	rc = check_dr_read(ctxt);
 	if (rc != X86EMUL_CONTINUE)
 		return rc;
 
-	if ((dr == 6 || dr == 7) && (new_val & 0xffffffff00000000ULL))
-		return emulate_gp(ctxt, 0);
+	switch (ctxt->modrm_reg) {
+	case 4:
+	case 5:
+	case 6:
+	case 7:
+		if (new_val & 0xffffffff00000000ULL)
+			return emulate_gp(ctxt, 0);
+		break;
+	default:
+		break;
+	}
 
 	return X86EMUL_CONTINUE;
 }
-- 
2.54.0.1136.gdb2ca164c4-goog

[PATCH v3 4/8] KVM: x86: Prioritize #UD on MOV DR over #GP due to non-zero CPL

[Sean Christopherson]

Manually handle the CPL check for MOV DR instructions instead of using the
Priv flag, *after* checking for #UD scenarios, as #GP due to CPL>0 has
lower priority than all #UDs.

Fixes: 1e470be5a108 ("KVM: x86 emulator: fix mov dr to inject #UD when needed.")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index a1bccab0eefe..127a21eeef66 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3844,6 +3844,9 @@ static int check_dr_read(struct x86_emulate_ctxt *ctxt)
 	if ((cr4 & X86_CR4_DE) && (dr == 4 || dr == 5))
 		return emulate_ud(ctxt);
 
+	if (ctxt->ops->cpl(ctxt))
+		return emulate_gp(ctxt, 0);
+
 	if (ctxt->ops->get_effective_dr7(ctxt) & DR7_GD)
 		return emulate_db(ctxt, DR6_BD);
 
@@ -4380,11 +4383,10 @@ static const struct opcode twobyte_table[256] = {
 	D(ImplicitOps | ModRM | SrcMem | NoAccess), /* NOP + 7 * reserved NOP */
 	/* 0x20 - 0x2F */
 	DIP(ModRM | DstMem | Priv | Op3264 | NoMod, cr_read, check_cr_access),
-	DIP(ModRM | DstMem | Priv | Op3264 | NoMod, dr_read, check_dr_read),
+	DIP(ModRM | DstMem | Op3264 | NoMod, dr_read, check_dr_read),
 	IIP(ModRM | SrcMem | Priv | Op3264 | NoMod, em_cr_write, cr_write,
 						check_cr_access),
-	IIP(ModRM | SrcMem | Priv | Op3264 | NoMod, em_dr_write, dr_write,
-						check_dr_write),
+	IIP(ModRM | SrcMem | Op3264 | NoMod, em_dr_write, dr_write, check_dr_write),
 	N, N, N, N,
 	GP(ModRM | DstReg | SrcMem | Mov | Sse | Avx, &pfx_0f_28_0f_29),
 	GP(ModRM | DstMem | SrcReg | Mov | Sse | Avx, &pfx_0f_28_0f_29),
-- 
2.54.0.1136.gdb2ca164c4-goog

[PATCH v3 5/8] KVM: VMX: Prioritize DR7.GD=1 #DB over CPL>0 #GP on Intel

[Sean Christopherson]

When emulating a MOV DR on Intel with DR7.GD=1 at CPL>0, prioritize the #DB
due to DR7.GD over the #GP due to CPL>0, as empirical testing shows that
Intel CPUs (Skylake, Icelake and Emerald Rapids) prioritize the DR7.GD #DB
over all #GPs, whereas AMD CPUs prioritize the CPL>0 #GP (but not illegal
value #GPs) over the #DB.

Outside of the emulator, don't bother trying to provide the "correct"
priority based on the virtual CPU model, as it's simply impossible to do
so without intercepting *all* MOV DR accesses, which would result in a
massive, unacceptable performance hit.  Note, getting the priority right
when advertising Intel on AMD would also require intercepting #GP, as SVM
prioritizes all exceptions over the instruction intercept.

Note, neither Intel's SDM nor AMD's APM says anything about the relative
priority, hence the empirical testing.  Arguably Intel's description of
DR7.GD:

  causes a debug exception to be generated prior to any MOV instruction
  that accesses a debug register.

implies that DR7.GD has higher priority.  But that's a fairly weak argument
as the statement would still hold true if the #GP due to CPL>0 had higher
priority, as the #GP would prevent any access to a DR.

Fixes: 3b88e41a4134 ("KVM: SVM: Add intercept check for accessing dr registers")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c | 7 ++++++-
 arch/x86/kvm/vmx/vmx.c | 6 +++---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 127a21eeef66..b4dc57fe0bc9 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3834,6 +3834,7 @@ static int check_cr_access(struct x86_emulate_ctxt *ctxt)
 
 static int check_dr_read(struct x86_emulate_ctxt *ctxt)
 {
+	bool is_intel = ctxt->ops->guest_cpuid_is_intel_compatible(ctxt);
 	int dr = ctxt->modrm_reg;
 	u64 cr4;
 
@@ -3844,12 +3845,16 @@ static int check_dr_read(struct x86_emulate_ctxt *ctxt)
 	if ((cr4 & X86_CR4_DE) && (dr == 4 || dr == 5))
 		return emulate_ud(ctxt);
 
-	if (ctxt->ops->cpl(ctxt))
+	/* Intel CPUs prioritize the DR7.GD=1 #DB over the CPL>0 #GP. */
+	if (!is_intel && ctxt->ops->cpl(ctxt))
 		return emulate_gp(ctxt, 0);
 
 	if (ctxt->ops->get_effective_dr7(ctxt) & DR7_GD)
 		return emulate_db(ctxt, DR6_BD);
 
+	if (is_intel && ctxt->ops->cpl(ctxt))
+		return emulate_gp(ctxt, 0);
+
 	return X86EMUL_CONTINUE;
 }
 
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index c548f22375ad..2f13d3163367 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5750,9 +5750,6 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 	if (!kvm_require_dr(vcpu, dr))
 		return 1;
 
-	if (vmx_get_cpl(vcpu) > 0)
-		goto out;
-
 	dr7 = vmcs_readl(GUEST_DR7);
 	if (dr7 & DR7_GD) {
 		/*
@@ -5773,6 +5770,9 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 		}
 	}
 
+	if (vmx_get_cpl(vcpu) > 0)
+		goto out;
+
 	if (vcpu->guest_debug == 0) {
 		exec_controls_clearbit(to_vmx(vcpu), CPU_BASED_MOV_DR_EXITING);
 
-- 
2.54.0.1136.gdb2ca164c4-goog

[PATCH v3 6/8] KVM: x86: Use kvm_dr{6,7}_valid() to check DR{4,5,6,7} write values in emulator

[Sean Christopherson]

Use kvm_dr{6,7}_valid() to validate the incoming DR{4,5,6,7} value in the
emulator instead of open coding an equivalent check.  In the unlikely event
that the behavior of DR6/7 (and their aliases) changes in the future, using
common helpers will hopefully make it less likely the emulator logic will
be overlooked.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b4dc57fe0bc9..b1799ed01939 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3869,10 +3869,13 @@ static int check_dr_write(struct x86_emulate_ctxt *ctxt)
 
 	switch (ctxt->modrm_reg) {
 	case 4:
-	case 5:
 	case 6:
+		if (!kvm_dr6_valid(new_val))
+			return emulate_gp(ctxt, 0);
+		break;
+	case 5:
 	case 7:
-		if (new_val & 0xffffffff00000000ULL)
+		if (!kvm_dr7_valid(new_val))
 			return emulate_gp(ctxt, 0);
 		break;
 	default:
-- 
2.54.0.1136.gdb2ca164c4-goog

[PATCH v3 7/8] KVM: x86: WARN if MOV DR emulation hits a "too late" #GP

[Sean Christopherson]

WARN if ->set_dr() => kvm_set_dr() fails when emulating a MOV DR write,
as the emulator _must_ pre-check for #GPs in order to get the event
priority right when emulating MOV DR for L2 on SVM (all exceptions have
higher priority than the instruction intercept).

Opportunistically update the comment as the blurb about "#UD" being
checked is incomplete and misleading.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b1799ed01939..e030ef9b9744 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3298,8 +3298,12 @@ static int em_dr_write(struct x86_emulate_ctxt *ctxt)
 	else
 		val = ctxt->src.val & ~0U;
 
-	/* #UD condition is already handled. */
-	if (ctxt->ops->set_dr(ctxt, ctxt->modrm_reg, val))
+	/*
+	 * A #GP due to an illegal value should be impossible at this point, as
+	 * such #GPs have priority over MOV DR intercepts on SVM, i.e. KVM must
+	 * manually check the value *before* emulating the write.
+	 */
+	if (WARN_ON_ONCE(ctxt->ops->set_dr(ctxt, ctxt->modrm_reg, val)))
 		return emulate_gp(ctxt, 0);
 
 	/* Disable writeback. */
-- 
2.54.0.1136.gdb2ca164c4-goog

[PATCH v3 8/8] KVM: x86: Read CR4.DE in emulator if and only if accessing DR4 or DR5

[Sean Christopherson]

Micro-optimize emulation of MOV DR instructions by checking CR4.DE if and
only if DR4 or DR5 is being accessed.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index e030ef9b9744..8071b372d233 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3840,13 +3840,11 @@ static int check_dr_read(struct x86_emulate_ctxt *ctxt)
 {
 	bool is_intel = ctxt->ops->guest_cpuid_is_intel_compatible(ctxt);
 	int dr = ctxt->modrm_reg;
-	u64 cr4;
 
 	if (dr > 7)
 		return emulate_ud(ctxt);
 
-	cr4 = ctxt->ops->get_cr(ctxt, 4);
-	if ((cr4 & X86_CR4_DE) && (dr == 4 || dr == 5))
+	if ((dr == 4 || dr == 5) && (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_DE))
 		return emulate_ud(ctxt);
 
 	/* Intel CPUs prioritize the DR7.GD=1 #DB over the CPL>0 #GP. */
-- 
2.54.0.1136.gdb2ca164c4-goog

Re: [PATCH v3 3/8] KVM: x86: Manually check DR4/5 write values to fix SVM intercept priority

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Lack of canonical address checks for DR0-DR3 writes allows malicious userspace or guests to crash the host kernel, and breaks nested virtualization exception priorities.
--

commit 85827e7c2e927fda473c503a8703cddac0182bb8
Author: Sean Christopherson <seanjc@google.com>

KVM: x86: Manually check DR4/5 write values to fix SVM intercept priority

This patch manually pre-checks values written to DR4/5 (DR6/DR7 aliases) to
avoid relying on ->set_dr() for #GP signaling. This fixes an SVM intercept
priority issue where nSVM relies on the emulator to perform exception checks.

> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 4484c5fa19e33..a1bccab0eefee 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -3853,15 +3853,23 @@ static int check_dr_read(struct x86_emulate_ctxt *ctxt)
>  static int check_dr_write(struct x86_emulate_ctxt *ctxt)
>  {
>  	u64 new_val = ctxt->src.val64;
> -	int dr = ctxt->modrm_reg;
>  	int rc;
>  
>  	rc = check_dr_read(ctxt);
>  	if (rc != X86EMUL_CONTINUE)
>  		return rc;
>  
> -	if ((dr == 6 || dr == 7) && (new_val & 0xffffffff00000000ULL))
> -		return emulate_gp(ctxt, 0);
> +	switch (ctxt->modrm_reg) {
> +	case 4:
> +	case 5:
> +	case 6:
> +	case 7:
> +		if (new_val & 0xffffffff00000000ULL)
> +			return emulate_gp(ctxt, 0);
> +		break;
> +	default:
> +		break;

[Severity: Critical]
This is a pre-existing issue, but does this code need to check for canonical
addresses on DR0-DR3?

If a malicious guest forces instruction emulation of a write to DR0-DR3 with a
non-canonical address, this default case allows it to return X86EMUL_CONTINUE.

Since kvm_set_dr() also doesn't check for canonical addresses on DR0-DR3,
could this allow a non-canonical value to be written to vcpu->arch.db[0..3]?

If hardware breakpoints are active, KVM restores these registers using
set_debugreg(), which eventually calls native_set_debugreg():

arch/x86/include/asm/debugreg.h:native_set_debugreg() {
    ...
    case 0:
        asm("mov %0, %%db0" ::"r" (value));
        break;
    ...
}

Because this inline assembly lacks an exception table entry (_ASM_EXTABLE),
could writing a non-canonical address generate an unhandled #GP in host
kernel mode and crash the host?

Additionally, by omitting the #GP check in the emulator, would this incorrectly
synthesize a VM-exit to L1 instead of injecting a #GP to L2, breaking nested
virtualization exception priorities on SVM?

> +	}
>  
>  	return X86EMUL_CONTINUE;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612230113.684301-1-seanjc@google.com?part=3