[PATCH 0/3] KVM: x86 - misc fixes

[PATCH 0/3] KVM: x86 - misc fixes

[Mathias Krause]

This is v2 of an old patch which gained an info leak fix to make it a
series.

v1 -> v2:
- drop the stable cc, shorten commit log
- split out dr6 change
- add KVM_GET_MSRS stack info leak fix

v1: https://lore.kernel.org/kvm/20230220104050.419438-1-minipli@grsecurity.net/

Please apply!

Thanks,
Mathias

Mathias Krause (3):
  KVM: x86: Fix KVM_GET_MSRS stack info leak
  KVM: x86: Simplify kvm_vcpu_ioctl_x86_get_debugregs()
  KVM: x86: Fix broken debugregs ABI for 32 bit kernels

 arch/x86/kvm/x86.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

-- 
2.39.2

[PATCH 1/3] KVM: x86: Fix KVM_GET_MSRS stack info leak

[Mathias Krause]

Commit 6abe9c1386e5 ("KVM: X86: Move ignore_msrs handling upper the
stack") changed the 'ignore_msrs' handling, including sanitizing return
values to the caller. This was fine until commit 12bc2132b15e ("KVM:
X86: Do the same ignore_msrs check for feature msrs") which allowed
non-existing feature MSRs to be ignored, i.e. to not generate an error
on the ioctl() level. It even tried to preserve the sanitization of the
return value. However, the logic is flawed, as '*data' will be
overwritten again with the uninitialized stack value of msr.data.

Fix this by simplifying the logic and always initializing msr.data,
vanishing the need for an additional error exit path.

Fixes: 12bc2132b15e ("KVM: X86: Do the same ignore_msrs check for feature msrs")
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 arch/x86/kvm/x86.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 363b1c080205..13ec948f3241 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1704,22 +1704,17 @@ static int do_get_msr_feature(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
 	struct kvm_msr_entry msr;
 	int r;
 
+	/* Unconditionally clear the output for simplicity */
+	msr.data = 0;
 	msr.index = index;
 	r = kvm_get_msr_feature(&msr);
 
-	if (r == KVM_MSR_RET_INVALID) {
-		/* Unconditionally clear the output for simplicity */
-		*data = 0;
-		if (kvm_msr_ignored_check(index, 0, false))
-			r = 0;
-	}
-
-	if (r)
-		return r;
+	if (r == KVM_MSR_RET_INVALID && kvm_msr_ignored_check(index, 0, false))
+		r = 0;
 
 	*data = msr.data;
 
-	return 0;
+	return r;
 }
 
 static bool __kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
-- 
2.39.2

Re: [PATCH 1/3] KVM: x86: Fix KVM_GET_MSRS stack info leak

[Xiaoyao Li]

On 2/3/2024 8:45 PM, Mathias Krause wrote:
> Commit 6abe9c1386e5 ("KVM: X86: Move ignore_msrs handling upper the
> stack") changed the 'ignore_msrs' handling, including sanitizing return
> values to the caller. This was fine until commit 12bc2132b15e ("KVM:
> X86: Do the same ignore_msrs check for feature msrs") which allowed
> non-existing feature MSRs to be ignored, i.e. to not generate an error
> on the ioctl() level. It even tried to preserve the sanitization of the
> return value. However, the logic is flawed, as '*data' will be
> overwritten again with the uninitialized stack value of msr.data.
> 
> Fix this by simplifying the logic and always initializing msr.data,
> vanishing the need for an additional error exit path.
> 
> Fixes: 12bc2132b15e ("KVM: X86: Do the same ignore_msrs check for feature msrs")
> Signed-off-by: Mathias Krause <minipli@grsecurity.net>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   arch/x86/kvm/x86.c | 15 +++++----------
>   1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 363b1c080205..13ec948f3241 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1704,22 +1704,17 @@ static int do_get_msr_feature(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
>   	struct kvm_msr_entry msr;
>   	int r;
>   
> +	/* Unconditionally clear the output for simplicity */
> +	msr.data = 0;
>   	msr.index = index;
>   	r = kvm_get_msr_feature(&msr);
>   
> -	if (r == KVM_MSR_RET_INVALID) {
> -		/* Unconditionally clear the output for simplicity */
> -		*data = 0;
> -		if (kvm_msr_ignored_check(index, 0, false))
> -			r = 0;
> -	}
> -
> -	if (r)
> -		return r;
> +	if (r == KVM_MSR_RET_INVALID && kvm_msr_ignored_check(index, 0, false))
> +		r = 0;
>   
>   	*data = msr.data;
>   
> -	return 0;
> +	return r;
>   }
>   
>   static bool __kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)

Re: [PATCH 1/3] KVM: x86: Fix KVM_GET_MSRS stack info leak

[Sean Christopherson]

On Sat, Feb 03, 2024, Mathias Krause wrote:
> Commit 6abe9c1386e5 ("KVM: X86: Move ignore_msrs handling upper the
> stack") changed the 'ignore_msrs' handling, including sanitizing return
> values to the caller. This was fine until commit 12bc2132b15e ("KVM:
> X86: Do the same ignore_msrs check for feature msrs") which allowed
> non-existing feature MSRs to be ignored, i.e. to not generate an error
> on the ioctl() level. It even tried to preserve the sanitization of the
> return value. However, the logic is flawed, as '*data' will be
> overwritten again with the uninitialized stack value of msr.data.

Ugh, what a terrible commit.  This makes no sense:

    Logically the ignore_msrs and report_ignored_msrs should also apply to feature
    MSRs.  Add them in.

The whole point of ignore_msrs was so that KVM could run _guest_ code that isn't
aware it's running in a VM, and so attempts to access MSRs that the _guest_ thinks
are always available.

The feature MSRs API is used only by userspace which obviously should know that
it's dealing with KVM.  Ignoring bad access from the host is just asinine.

At this point, it's not worth trying to revert that commit, but oof.

> Fix this by simplifying the logic and always initializing msr.data,
> vanishing the need for an additional error exit path.

Out of curiosity, was this found by inspection, or by some other means?  I'm quite
surprised none of the sanitizers stumbled across this.

> Fixes: 12bc2132b15e ("KVM: X86: Do the same ignore_msrs check for feature msrs")

I'll apply this for 6.8.  I think I'll also throw together a follow-up series to
clean up some of this mess.  There's no good reason this code has to be so grossly
fragile.

Re: [PATCH 1/3] KVM: x86: Fix KVM_GET_MSRS stack info leak

[Mathias Krause]

On 05.02.24 19:42, Sean Christopherson wrote:
> On Sat, Feb 03, 2024, Mathias Krause wrote:
>> Commit 6abe9c1386e5 ("KVM: X86: Move ignore_msrs handling upper the
>> stack") changed the 'ignore_msrs' handling, including sanitizing return
>> values to the caller. This was fine until commit 12bc2132b15e ("KVM:
>> X86: Do the same ignore_msrs check for feature msrs") which allowed
>> non-existing feature MSRs to be ignored, i.e. to not generate an error
>> on the ioctl() level. It even tried to preserve the sanitization of the
>> return value. However, the logic is flawed, as '*data' will be
>> overwritten again with the uninitialized stack value of msr.data.
> 
> Ugh, what a terrible commit.  This makes no sense:
> 
>     Logically the ignore_msrs and report_ignored_msrs should also apply to feature
>     MSRs.  Add them in.
> 
> The whole point of ignore_msrs was so that KVM could run _guest_ code that isn't
> aware it's running in a VM, and so attempts to access MSRs that the _guest_ thinks
> are always available.

Yeah, I was wondering that myself too. But I thought, maybe there's
buggy QEMU versions out there and it's because of that?

> 
> The feature MSRs API is used only by userspace which obviously should know that
> it's dealing with KVM.  Ignoring bad access from the host is just asinine.

From a quick google search I found, enabling kvm.ignore_msrs is a common
suggestion to work around Windows bluescreens. I'm not a Windows user,
less so in VMs, so dunno if that's just snake oil or sometimes works by
chance because of returning "random" MSR values.

> 
> At this point, it's not worth trying to revert that commit, but oof.
> 
>> Fix this by simplifying the logic and always initializing msr.data,
>> vanishing the need for an additional error exit path.
> 
> Out of curiosity, was this found by inspection, or by some other means?  I'm quite
> surprised none of the sanitizers stumbled across this.

Manual inspection, yes. I was looking how MSRs are handled in general to
answer a different question for myself (related to FSGSBASE handling
resp. the lack thereof, but completely unrelated to this change) and
found this code just a little bit too ugly and looked a little closer.

> 
>> Fixes: 12bc2132b15e ("KVM: X86: Do the same ignore_msrs check for feature msrs")
> 
> I'll apply this for 6.8.  I think I'll also throw together a follow-up series to
> clean up some of this mess.  There's no good reason this code has to be so grossly
> fragile.

Thanks,
Mathias

[PATCH 2/3] KVM: x86: Simplify kvm_vcpu_ioctl_x86_get_debugregs()

[Mathias Krause]

Take 'dr6' from the arch part directly as already done for 'dr7'.
There's no need to take the clunky route via kvm_get_dr().

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 arch/x86/kvm/x86.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 13ec948f3241..0f958dcf8458 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5504,12 +5504,9 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
 					     struct kvm_debugregs *dbgregs)
 {
-	unsigned long val;
-
 	memset(dbgregs, 0, sizeof(*dbgregs));
 	memcpy(dbgregs->db, vcpu->arch.db, sizeof(vcpu->arch.db));
-	kvm_get_dr(vcpu, 6, &val);
-	dbgregs->dr6 = val;
+	dbgregs->dr6 = vcpu->arch.dr6;
 	dbgregs->dr7 = vcpu->arch.dr7;
 }
 
-- 
2.39.2

Re: [PATCH 2/3] KVM: x86: Simplify kvm_vcpu_ioctl_x86_get_debugregs()

[Xiaoyao Li]

On 2/3/2024 8:45 PM, Mathias Krause wrote:
> Take 'dr6' from the arch part directly as already done for 'dr7'.
> There's no need to take the clunky route via kvm_get_dr().
> 
> Signed-off-by: Mathias Krause <minipli@grsecurity.net>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   arch/x86/kvm/x86.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 13ec948f3241..0f958dcf8458 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5504,12 +5504,9 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
>   static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
>   					     struct kvm_debugregs *dbgregs)
>   {
> -	unsigned long val;
> -
>   	memset(dbgregs, 0, sizeof(*dbgregs));
>   	memcpy(dbgregs->db, vcpu->arch.db, sizeof(vcpu->arch.db));
> -	kvm_get_dr(vcpu, 6, &val);
> -	dbgregs->dr6 = val;
> +	dbgregs->dr6 = vcpu->arch.dr6;
>   	dbgregs->dr7 = vcpu->arch.dr7;
>   }
>   

Re: [PATCH 2/3] KVM: x86: Simplify kvm_vcpu_ioctl_x86_get_debugregs()

[Sean Christopherson]

On Sat, Feb 03, 2024, Mathias Krause wrote:
> Take 'dr6' from the arch part directly as already done for 'dr7'.
> There's no need to take the clunky route via kvm_get_dr().
> 
> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
> ---
>  arch/x86/kvm/x86.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 13ec948f3241..0f958dcf8458 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5504,12 +5504,9 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
>  static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
>  					     struct kvm_debugregs *dbgregs)
>  {
> -	unsigned long val;
> -
>  	memset(dbgregs, 0, sizeof(*dbgregs));
>  	memcpy(dbgregs->db, vcpu->arch.db, sizeof(vcpu->arch.db));
> -	kvm_get_dr(vcpu, 6, &val);
> -	dbgregs->dr6 = val;
> +	dbgregs->dr6 = vcpu->arch.dr6;

Blech, kvm_get_dr() is so dumb, it takes an out parameter despite have a void
return.  I would rather fix that wart and go the other direction, i.e. make dr7
go through kvm_get_dr().  This obviously isn't a fast path, so the extra CALL+RET
is a non-issue.  And if we wanted to fix that, e.g. for other paths that are
slightly less slow, we should do so for all reads (and writes) to dr6 and dr7,
e.g. provide dedicated APIs like we do for GPRs.

Alternatively, I would probably be ok just open coding all direct reads and writes
to dr6 and dr7.  IIRC, at one point KVM was doing something meaningful in kvm_get_dr()
for DR7 (which probably contributed to the weird API), but that's no longer the
case.

Actually, it probably makes sense to do both, i.e. do the below, and then open
code all direct accesses.  I think the odds of us needing wrappers around reading
and writing guest DR6 and DR7 are quite low and there are enough existing open coded
accesses that forcing them to convert would be awkward.

I'll send a small two patch series.

---
Subject: [PATCH] KVM: x86: Make kvm_get_dr() return a value, not use an out
 parameter

TODO: writeme

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/emulate.c          | 17 ++++-------------
 arch/x86/kvm/kvm_emulate.h      |  2 +-
 arch/x86/kvm/smm.c              | 15 ++++-----------
 arch/x86/kvm/svm/svm.c          |  7 ++-----
 arch/x86/kvm/vmx/nested.c       |  2 +-
 arch/x86/kvm/vmx/vmx.c          |  5 +----
 arch/x86/kvm/x86.c              | 20 ++++++++------------
 8 files changed, 22 insertions(+), 48 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index ad5319a503f0..464fa2197748 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -2046,7 +2046,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3);
 int kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4);
 int kvm_set_cr8(struct kvm_vcpu *vcpu, unsigned long cr8);
 int kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val);
-void kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val);
+unsigned long kvm_get_dr(struct kvm_vcpu *vcpu, int dr);
 unsigned long kvm_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lmsw(struct kvm_vcpu *vcpu, unsigned long msw);
 int kvm_emulate_xsetbv(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 695ab5b6055c..33444627fcf4 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3011,7 +3011,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
 		ret = em_push(ctxt);
 	}
 
-	ops->get_dr(ctxt, 7, &dr7);
+	dr7 = ops->get_dr(ctxt, 7);
 	ops->set_dr(ctxt, 7, dr7 & ~(DR_LOCAL_ENABLE_MASK | DR_LOCAL_SLOWDOWN));
 
 	return ret;
@@ -3866,15 +3866,6 @@ static int check_cr_access(struct x86_emulate_ctxt *ctxt)
 	return X86EMUL_CONTINUE;
 }
 
-static int check_dr7_gd(struct x86_emulate_ctxt *ctxt)
-{
-	unsigned long dr7;
-
-	ctxt->ops->get_dr(ctxt, 7, &dr7);
-
-	return dr7 & DR7_GD;
-}
-
 static int check_dr_read(struct x86_emulate_ctxt *ctxt)
 {
 	int dr = ctxt->modrm_reg;
@@ -3887,10 +3878,10 @@ static int check_dr_read(struct x86_emulate_ctxt *ctxt)
 	if ((cr4 & X86_CR4_DE) && (dr == 4 || dr == 5))
 		return emulate_ud(ctxt);
 
-	if (check_dr7_gd(ctxt)) {
+	if (ctxt->ops->get_dr(ctxt, 7) & DR7_GD) {
 		ulong dr6;
 
-		ctxt->ops->get_dr(ctxt, 6, &dr6);
+		dr6 = ctxt->ops->get_dr(ctxt, 6);
 		dr6 &= ~DR_TRAP_BITS;
 		dr6 |= DR6_BD | DR6_ACTIVE_LOW;
 		ctxt->ops->set_dr(ctxt, 6, dr6);
@@ -5449,7 +5440,7 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
 		ctxt->dst.val = ops->get_cr(ctxt, ctxt->modrm_reg);
 		break;
 	case 0x21: /* mov from dr to reg */
-		ops->get_dr(ctxt, ctxt->modrm_reg, &ctxt->dst.val);
+		ctxt->dst.val = ops->get_dr(ctxt, ctxt->modrm_reg);
 		break;
 	case 0x40 ... 0x4f:	/* cmov */
 		if (test_cc(ctxt->b, ctxt->eflags))
diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
index 4351149484fb..5382646162a3 100644
--- a/arch/x86/kvm/kvm_emulate.h
+++ b/arch/x86/kvm/kvm_emulate.h
@@ -203,7 +203,7 @@ struct x86_emulate_ops {
 	ulong (*get_cr)(struct x86_emulate_ctxt *ctxt, int cr);
 	int (*set_cr)(struct x86_emulate_ctxt *ctxt, int cr, ulong val);
 	int (*cpl)(struct x86_emulate_ctxt *ctxt);
-	void (*get_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong *dest);
+	ulong (*get_dr)(struct x86_emulate_ctxt *ctxt, int dr);
 	int (*set_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong value);
 	int (*set_msr_with_filter)(struct x86_emulate_ctxt *ctxt, u32 msr_index, u64 data);
 	int (*get_msr_with_filter)(struct x86_emulate_ctxt *ctxt, u32 msr_index, u64 *pdata);
diff --git a/arch/x86/kvm/smm.c b/arch/x86/kvm/smm.c
index dc3d95fdca7d..f5a30d3a44a1 100644
--- a/arch/x86/kvm/smm.c
+++ b/arch/x86/kvm/smm.c
@@ -184,7 +184,6 @@ static void enter_smm_save_state_32(struct kvm_vcpu *vcpu,
 				    struct kvm_smram_state_32 *smram)
 {
 	struct desc_ptr dt;
-	unsigned long val;
 	int i;
 
 	smram->cr0     = kvm_read_cr0(vcpu);
@@ -195,10 +194,8 @@ static void enter_smm_save_state_32(struct kvm_vcpu *vcpu,
 	for (i = 0; i < 8; i++)
 		smram->gprs[i] = kvm_register_read_raw(vcpu, i);
 
-	kvm_get_dr(vcpu, 6, &val);
-	smram->dr6     = (u32)val;
-	kvm_get_dr(vcpu, 7, &val);
-	smram->dr7     = (u32)val;
+	smram->dr6     = (u32)kvm_get_dr(vcpu, 6);
+	smram->dr7     = (u32)kvm_get_dr(vcpu, 7);
 
 	enter_smm_save_seg_32(vcpu, &smram->tr, &smram->tr_sel, VCPU_SREG_TR);
 	enter_smm_save_seg_32(vcpu, &smram->ldtr, &smram->ldtr_sel, VCPU_SREG_LDTR);
@@ -231,7 +228,6 @@ static void enter_smm_save_state_64(struct kvm_vcpu *vcpu,
 				    struct kvm_smram_state_64 *smram)
 {
 	struct desc_ptr dt;
-	unsigned long val;
 	int i;
 
 	for (i = 0; i < 16; i++)
@@ -240,11 +236,8 @@ static void enter_smm_save_state_64(struct kvm_vcpu *vcpu,
 	smram->rip    = kvm_rip_read(vcpu);
 	smram->rflags = kvm_get_rflags(vcpu);
 
-
-	kvm_get_dr(vcpu, 6, &val);
-	smram->dr6 = val;
-	kvm_get_dr(vcpu, 7, &val);
-	smram->dr7 = val;
+	smram->dr6 = kvm_get_dr(vcpu, 6);
+	smram->dr7 = kvm_get_dr(vcpu, 7);;
 
 	smram->cr0 = kvm_read_cr0(vcpu);
 	smram->cr3 = kvm_read_cr3(vcpu);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index e90b429c84f1..dda91f7cd71b 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2735,7 +2735,6 @@ static int dr_interception(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 	int reg, dr;
-	unsigned long val;
 	int err = 0;
 
 	/*
@@ -2763,11 +2762,9 @@ static int dr_interception(struct kvm_vcpu *vcpu)
 	dr = svm->vmcb->control.exit_code - SVM_EXIT_READ_DR0;
 	if (dr >= 16) { /* mov to DRn  */
 		dr -= 16;
-		val = kvm_register_read(vcpu, reg);
-		err = kvm_set_dr(vcpu, dr, val);
+		err = kvm_set_dr(vcpu, dr, kvm_register_read(vcpu, reg));
 	} else {
-		kvm_get_dr(vcpu, dr, &val);
-		kvm_register_write(vcpu, reg, val);
+		kvm_register_write(vcpu, reg, kvm_get_dr(vcpu, dr));
 	}
 
 	return kvm_complete_insn_gp(vcpu, err);
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 994e014f8a50..28d1088a1770 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -4433,7 +4433,7 @@ static void sync_vmcs02_to_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 		(vm_entry_controls_get(to_vmx(vcpu)) & VM_ENTRY_IA32E_MODE);
 
 	if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_DEBUG_CONTROLS)
-		kvm_get_dr(vcpu, 7, (unsigned long *)&vmcs12->guest_dr7);
+		vmcs12->guest_dr7 = kvm_get_dr(vcpu, 7);
 
 	if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_IA32_EFER)
 		vmcs12->guest_ia32_efer = vcpu->arch.efer;
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index e262bc2ba4e5..aa47433d0c9b 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5566,10 +5566,7 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 
 	reg = DEBUG_REG_ACCESS_REG(exit_qualification);
 	if (exit_qualification & TYPE_MOV_FROM_DR) {
-		unsigned long val;
-
-		kvm_get_dr(vcpu, dr, &val);
-		kvm_register_write(vcpu, reg, val);
+		kvm_register_write(vcpu, reg, kvm_get_dr(vcpu, dr));
 		err = 0;
 	} else {
 		err = kvm_set_dr(vcpu, dr, kvm_register_read(vcpu, reg));
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c339d9f95b4b..b2357009bbbe 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1399,21 +1399,21 @@ int kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
 }
 EXPORT_SYMBOL_GPL(kvm_set_dr);
 
-void kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val)
+unsigned long kvm_get_dr(struct kvm_vcpu *vcpu, int dr)
 {
 	size_t size = ARRAY_SIZE(vcpu->arch.db);
 
 	switch (dr) {
 	case 0 ... 3:
-		*val = vcpu->arch.db[array_index_nospec(dr, size)];
+		return vcpu->arch.db[array_index_nospec(dr, size)];
 		break;
 	case 4:
 	case 6:
-		*val = vcpu->arch.dr6;
+		return vcpu->arch.dr6;
 		break;
 	case 5:
 	default: /* 7 */
-		*val = vcpu->arch.dr7;
+		return vcpu->arch.dr7;
 		break;
 	}
 }
@@ -5510,13 +5510,10 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
 					     struct kvm_debugregs *dbgregs)
 {
-	unsigned long val;
-
 	memset(dbgregs, 0, sizeof(*dbgregs));
 	memcpy(dbgregs->db, vcpu->arch.db, sizeof(vcpu->arch.db));
-	kvm_get_dr(vcpu, 6, &val);
-	dbgregs->dr6 = val;
-	dbgregs->dr7 = vcpu->arch.dr7;
+	dbgregs->dr6 = kvm_get_dr(vcpu, 6);
+	dbgregs->dr7 = kvm_get_dr(vcpu, 7);
 }
 
 static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
@@ -8165,10 +8162,9 @@ static void emulator_wbinvd(struct x86_emulate_ctxt *ctxt)
 	kvm_emulate_wbinvd_noskip(emul_to_vcpu(ctxt));
 }
 
-static void emulator_get_dr(struct x86_emulate_ctxt *ctxt, int dr,
-			    unsigned long *dest)
+static unsigned long emulator_get_dr(struct x86_emulate_ctxt *ctxt, int dr)
 {
-	kvm_get_dr(emul_to_vcpu(ctxt), dr, dest);
+	return kvm_get_dr(emul_to_vcpu(ctxt), dr);
 }
 
 static int emulator_set_dr(struct x86_emulate_ctxt *ctxt, int dr,

base-commit: 60eedcfceda9db46f1b333e5e1aa9359793f04fb
-- 

Re: [PATCH 2/3] KVM: x86: Simplify kvm_vcpu_ioctl_x86_get_debugregs()

[Mathias Krause]

On 05.02.24 20:53, Sean Christopherson wrote:
> On Sat, Feb 03, 2024, Mathias Krause wrote:
>> Take 'dr6' from the arch part directly as already done for 'dr7'.
>> There's no need to take the clunky route via kvm_get_dr().
>>
>> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
>> ---
>>  arch/x86/kvm/x86.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 13ec948f3241..0f958dcf8458 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -5504,12 +5504,9 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
>>  static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
>>  					     struct kvm_debugregs *dbgregs)
>>  {
>> -	unsigned long val;
>> -
>>  	memset(dbgregs, 0, sizeof(*dbgregs));
>>  	memcpy(dbgregs->db, vcpu->arch.db, sizeof(vcpu->arch.db));
>> -	kvm_get_dr(vcpu, 6, &val);
>> -	dbgregs->dr6 = val;
>> +	dbgregs->dr6 = vcpu->arch.dr6;
> 
> Blech, kvm_get_dr() is so dumb, it takes an out parameter despite have a void
> return.

Jepp, that's why I tried to get rid of it.

>          I would rather fix that wart and go the other direction, i.e. make dr7
> go through kvm_get_dr().  This obviously isn't a fast path, so the extra CALL+RET
> is a non-issue.

Okay. I thought, as this is an indirect call which is slightly more
expensive under RETPOLINE, I'd go the other way and simply open-code the
access, as done a few lines below in kvm_vcpu_ioctl_x86_set_debugregs().

But I don't mind that hard. You just mentioned last year[1], this part
shouldn't be squashed into what became patch 3 in this series.

[1] https://lore.kernel.org/kvm/ZCxarzBknX6o7dcb@google.com/

>                  And if we wanted to fix that, e.g. for other paths that are
> slightly less slow, we should do so for all reads (and writes) to dr6 and dr7,
> e.g. provide dedicated APIs like we do for GPRs.
> 
> Alternatively, I would probably be ok just open coding all direct reads and writes
> to dr6 and dr7.  IIRC, at one point KVM was doing something meaningful in kvm_get_dr()
> for DR7 (which probably contributed to the weird API), but that's no longer the
> case.

Yeah, that special handling got simplified in commit 5679b803e44e ("KVM:
SVM: keep DR6 synchronized with vcpu->arch.dr6"). And yes, open-coding
the accesses would be my preferred option, as it's easier to read and
generates even less code. No need to have this indirection for a simple
member access.

> 
> Actually, it probably makes sense to do both, i.e. do the below, and then open
> code all direct accesses.  I think the odds of us needing wrappers around reading
> and writing guest DR6 and DR7 are quite low and there are enough existing open coded
> accesses that forcing them to convert would be awkward.
> 
> I'll send a small two patch series.
> 
> ---
> Subject: [PATCH] KVM: x86: Make kvm_get_dr() return a value, not use an out
>  parameter
> 
> TODO: writeme
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  2 +-
>  arch/x86/kvm/emulate.c          | 17 ++++-------------
>  arch/x86/kvm/kvm_emulate.h      |  2 +-
>  arch/x86/kvm/smm.c              | 15 ++++-----------
>  arch/x86/kvm/svm/svm.c          |  7 ++-----
>  arch/x86/kvm/vmx/nested.c       |  2 +-
>  arch/x86/kvm/vmx/vmx.c          |  5 +----
>  arch/x86/kvm/x86.c              | 20 ++++++++------------
>  8 files changed, 22 insertions(+), 48 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index ad5319a503f0..464fa2197748 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -2046,7 +2046,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3);
>  int kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4);
>  int kvm_set_cr8(struct kvm_vcpu *vcpu, unsigned long cr8);
>  int kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val);
> -void kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val);
> +unsigned long kvm_get_dr(struct kvm_vcpu *vcpu, int dr);
>  unsigned long kvm_get_cr8(struct kvm_vcpu *vcpu);
>  void kvm_lmsw(struct kvm_vcpu *vcpu, unsigned long msw);
>  int kvm_emulate_xsetbv(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 695ab5b6055c..33444627fcf4 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -3011,7 +3011,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
>  		ret = em_push(ctxt);
>  	}
>  
> -	ops->get_dr(ctxt, 7, &dr7);
> +	dr7 = ops->get_dr(ctxt, 7);
>  	ops->set_dr(ctxt, 7, dr7 & ~(DR_LOCAL_ENABLE_MASK | DR_LOCAL_SLOWDOWN));
>  
>  	return ret;
> @@ -3866,15 +3866,6 @@ static int check_cr_access(struct x86_emulate_ctxt *ctxt)
>  	return X86EMUL_CONTINUE;
>  }
>  
> -static int check_dr7_gd(struct x86_emulate_ctxt *ctxt)
> -{
> -	unsigned long dr7;
> -
> -	ctxt->ops->get_dr(ctxt, 7, &dr7);
> -
> -	return dr7 & DR7_GD;
> -}
> -
>  static int check_dr_read(struct x86_emulate_ctxt *ctxt)
>  {
>  	int dr = ctxt->modrm_reg;
> @@ -3887,10 +3878,10 @@ static int check_dr_read(struct x86_emulate_ctxt *ctxt)
>  	if ((cr4 & X86_CR4_DE) && (dr == 4 || dr == 5))
>  		return emulate_ud(ctxt);
>  
> -	if (check_dr7_gd(ctxt)) {
> +	if (ctxt->ops->get_dr(ctxt, 7) & DR7_GD) {
>  		ulong dr6;
>  
> -		ctxt->ops->get_dr(ctxt, 6, &dr6);
> +		dr6 = ctxt->ops->get_dr(ctxt, 6);
>  		dr6 &= ~DR_TRAP_BITS;
>  		dr6 |= DR6_BD | DR6_ACTIVE_LOW;
>  		ctxt->ops->set_dr(ctxt, 6, dr6);
> @@ -5449,7 +5440,7 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
>  		ctxt->dst.val = ops->get_cr(ctxt, ctxt->modrm_reg);
>  		break;
>  	case 0x21: /* mov from dr to reg */
> -		ops->get_dr(ctxt, ctxt->modrm_reg, &ctxt->dst.val);
> +		ctxt->dst.val = ops->get_dr(ctxt, ctxt->modrm_reg);
>  		break;
>  	case 0x40 ... 0x4f:	/* cmov */
>  		if (test_cc(ctxt->b, ctxt->eflags))
> diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
> index 4351149484fb..5382646162a3 100644
> --- a/arch/x86/kvm/kvm_emulate.h
> +++ b/arch/x86/kvm/kvm_emulate.h
> @@ -203,7 +203,7 @@ struct x86_emulate_ops {
>  	ulong (*get_cr)(struct x86_emulate_ctxt *ctxt, int cr);
>  	int (*set_cr)(struct x86_emulate_ctxt *ctxt, int cr, ulong val);
>  	int (*cpl)(struct x86_emulate_ctxt *ctxt);
> -	void (*get_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong *dest);
> +	ulong (*get_dr)(struct x86_emulate_ctxt *ctxt, int dr);
>  	int (*set_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong value);
>  	int (*set_msr_with_filter)(struct x86_emulate_ctxt *ctxt, u32 msr_index, u64 data);
>  	int (*get_msr_with_filter)(struct x86_emulate_ctxt *ctxt, u32 msr_index, u64 *pdata);
> diff --git a/arch/x86/kvm/smm.c b/arch/x86/kvm/smm.c
> index dc3d95fdca7d..f5a30d3a44a1 100644
> --- a/arch/x86/kvm/smm.c
> +++ b/arch/x86/kvm/smm.c
> @@ -184,7 +184,6 @@ static void enter_smm_save_state_32(struct kvm_vcpu *vcpu,
>  				    struct kvm_smram_state_32 *smram)
>  {
>  	struct desc_ptr dt;
> -	unsigned long val;
>  	int i;
>  
>  	smram->cr0     = kvm_read_cr0(vcpu);
> @@ -195,10 +194,8 @@ static void enter_smm_save_state_32(struct kvm_vcpu *vcpu,
>  	for (i = 0; i < 8; i++)
>  		smram->gprs[i] = kvm_register_read_raw(vcpu, i);
>  
> -	kvm_get_dr(vcpu, 6, &val);
> -	smram->dr6     = (u32)val;
> -	kvm_get_dr(vcpu, 7, &val);
> -	smram->dr7     = (u32)val;
> +	smram->dr6     = (u32)kvm_get_dr(vcpu, 6);
> +	smram->dr7     = (u32)kvm_get_dr(vcpu, 7);
>  
>  	enter_smm_save_seg_32(vcpu, &smram->tr, &smram->tr_sel, VCPU_SREG_TR);
>  	enter_smm_save_seg_32(vcpu, &smram->ldtr, &smram->ldtr_sel, VCPU_SREG_LDTR);
> @@ -231,7 +228,6 @@ static void enter_smm_save_state_64(struct kvm_vcpu *vcpu,
>  				    struct kvm_smram_state_64 *smram)
>  {
>  	struct desc_ptr dt;
> -	unsigned long val;
>  	int i;
>  
>  	for (i = 0; i < 16; i++)
> @@ -240,11 +236,8 @@ static void enter_smm_save_state_64(struct kvm_vcpu *vcpu,
>  	smram->rip    = kvm_rip_read(vcpu);
>  	smram->rflags = kvm_get_rflags(vcpu);
>  
> -
> -	kvm_get_dr(vcpu, 6, &val);
> -	smram->dr6 = val;
> -	kvm_get_dr(vcpu, 7, &val);
> -	smram->dr7 = val;
> +	smram->dr6 = kvm_get_dr(vcpu, 6);
> +	smram->dr7 = kvm_get_dr(vcpu, 7);;
>  
>  	smram->cr0 = kvm_read_cr0(vcpu);
>  	smram->cr3 = kvm_read_cr3(vcpu);
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index e90b429c84f1..dda91f7cd71b 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2735,7 +2735,6 @@ static int dr_interception(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	int reg, dr;
> -	unsigned long val;
>  	int err = 0;
>  
>  	/*
> @@ -2763,11 +2762,9 @@ static int dr_interception(struct kvm_vcpu *vcpu)
>  	dr = svm->vmcb->control.exit_code - SVM_EXIT_READ_DR0;
>  	if (dr >= 16) { /* mov to DRn  */
>  		dr -= 16;
> -		val = kvm_register_read(vcpu, reg);
> -		err = kvm_set_dr(vcpu, dr, val);
> +		err = kvm_set_dr(vcpu, dr, kvm_register_read(vcpu, reg));
>  	} else {
> -		kvm_get_dr(vcpu, dr, &val);
> -		kvm_register_write(vcpu, reg, val);
> +		kvm_register_write(vcpu, reg, kvm_get_dr(vcpu, dr));
>  	}
>  
>  	return kvm_complete_insn_gp(vcpu, err);
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 994e014f8a50..28d1088a1770 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -4433,7 +4433,7 @@ static void sync_vmcs02_to_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>  		(vm_entry_controls_get(to_vmx(vcpu)) & VM_ENTRY_IA32E_MODE);
>  
>  	if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_DEBUG_CONTROLS)
> -		kvm_get_dr(vcpu, 7, (unsigned long *)&vmcs12->guest_dr7);
> +		vmcs12->guest_dr7 = kvm_get_dr(vcpu, 7);
>  
>  	if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_IA32_EFER)
>  		vmcs12->guest_ia32_efer = vcpu->arch.efer;
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index e262bc2ba4e5..aa47433d0c9b 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5566,10 +5566,7 @@ static int handle_dr(struct kvm_vcpu *vcpu)
>  
>  	reg = DEBUG_REG_ACCESS_REG(exit_qualification);
>  	if (exit_qualification & TYPE_MOV_FROM_DR) {
> -		unsigned long val;
> -
> -		kvm_get_dr(vcpu, dr, &val);
> -		kvm_register_write(vcpu, reg, val);
> +		kvm_register_write(vcpu, reg, kvm_get_dr(vcpu, dr));
>  		err = 0;
>  	} else {
>  		err = kvm_set_dr(vcpu, dr, kvm_register_read(vcpu, reg));
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c339d9f95b4b..b2357009bbbe 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1399,21 +1399,21 @@ int kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
>  }
>  EXPORT_SYMBOL_GPL(kvm_set_dr);
>  
> -void kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val)
> +unsigned long kvm_get_dr(struct kvm_vcpu *vcpu, int dr)
>  {
>  	size_t size = ARRAY_SIZE(vcpu->arch.db);
>  
>  	switch (dr) {
>  	case 0 ... 3:
> -		*val = vcpu->arch.db[array_index_nospec(dr, size)];
> +		return vcpu->arch.db[array_index_nospec(dr, size)];
>  		break;
>  	case 4:
>  	case 6:
> -		*val = vcpu->arch.dr6;
> +		return vcpu->arch.dr6;
>  		break;
>  	case 5:
>  	default: /* 7 */
> -		*val = vcpu->arch.dr7;
> +		return vcpu->arch.dr7;
>  		break;
>  	}
>  }
> @@ -5510,13 +5510,10 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
>  static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
>  					     struct kvm_debugregs *dbgregs)
>  {
> -	unsigned long val;
> -
>  	memset(dbgregs, 0, sizeof(*dbgregs));
>  	memcpy(dbgregs->db, vcpu->arch.db, sizeof(vcpu->arch.db));
> -	kvm_get_dr(vcpu, 6, &val);
> -	dbgregs->dr6 = val;
> -	dbgregs->dr7 = vcpu->arch.dr7;
> +	dbgregs->dr6 = kvm_get_dr(vcpu, 6);
> +	dbgregs->dr7 = kvm_get_dr(vcpu, 7);
>  }
>  
>  static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
> @@ -8165,10 +8162,9 @@ static void emulator_wbinvd(struct x86_emulate_ctxt *ctxt)
>  	kvm_emulate_wbinvd_noskip(emul_to_vcpu(ctxt));
>  }
>  
> -static void emulator_get_dr(struct x86_emulate_ctxt *ctxt, int dr,
> -			    unsigned long *dest)
> +static unsigned long emulator_get_dr(struct x86_emulate_ctxt *ctxt, int dr)
>  {
> -	kvm_get_dr(emul_to_vcpu(ctxt), dr, dest);
> +	return kvm_get_dr(emul_to_vcpu(ctxt), dr);
>  }
>  
>  static int emulator_set_dr(struct x86_emulate_ctxt *ctxt, int dr,
> 
> base-commit: 60eedcfceda9db46f1b333e5e1aa9359793f04fb

As this provides a saner API to kvm_set_dr(),
Acked-by: Mathias Krause <minipli@grsecurity.net>

Re: [PATCH 2/3] KVM: x86: Simplify kvm_vcpu_ioctl_x86_get_debugregs()

[Sean Christopherson]

On Tue, Feb 06, 2024, Mathias Krause wrote:
> On 05.02.24 20:53, Sean Christopherson wrote:
> > On Sat, Feb 03, 2024, Mathias Krause wrote:
> >> Take 'dr6' from the arch part directly as already done for 'dr7'.
> >> There's no need to take the clunky route via kvm_get_dr().
> >>
> >> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
> >> ---
> >>  arch/x86/kvm/x86.c | 5 +----
> >>  1 file changed, 1 insertion(+), 4 deletions(-)
> >>
> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >> index 13ec948f3241..0f958dcf8458 100644
> >> --- a/arch/x86/kvm/x86.c
> >> +++ b/arch/x86/kvm/x86.c
> >> @@ -5504,12 +5504,9 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
> >>  static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
> >>  					     struct kvm_debugregs *dbgregs)
> >>  {
> >> -	unsigned long val;
> >> -
> >>  	memset(dbgregs, 0, sizeof(*dbgregs));
> >>  	memcpy(dbgregs->db, vcpu->arch.db, sizeof(vcpu->arch.db));
> >> -	kvm_get_dr(vcpu, 6, &val);
> >> -	dbgregs->dr6 = val;
> >> +	dbgregs->dr6 = vcpu->arch.dr6;
> > 
> > Blech, kvm_get_dr() is so dumb, it takes an out parameter despite have a void
> > return.
> 
> Jepp, that's why I tried to get rid of it.
> 
> >          I would rather fix that wart and go the other direction, i.e. make dr7
> > go through kvm_get_dr().  This obviously isn't a fast path, so the extra CALL+RET
> > is a non-issue.
> 
> Okay. I thought, as this is an indirect call which is slightly more
> expensive under RETPOLINE, I'd go the other way and simply open-code the
> access, as done a few lines below in kvm_vcpu_ioctl_x86_set_debugregs().

It's not an indirect call.  It's not even strictly guaranteed to be a function
call, e.g. within x86.c, kvm_get_dr() is in scope of kvm_vcpu_ioctl_x86_get_debugregs()
and so a very smart compiler could fully inline and optimize it to just
"xxx = vcpu->arch.dr6" through dead code elimination (I doubt gcc or clang actually
does this, but it's possible).

Re: [PATCH 2/3] KVM: x86: Simplify kvm_vcpu_ioctl_x86_get_debugregs()

[Mathias Krause]

On 06.02.24 19:24, Sean Christopherson wrote:
> On Tue, Feb 06, 2024, Mathias Krause wrote:
>> On 05.02.24 20:53, Sean Christopherson wrote:
>>> On Sat, Feb 03, 2024, Mathias Krause wrote:
>>>> Take 'dr6' from the arch part directly as already done for 'dr7'.
>>>> There's no need to take the clunky route via kvm_get_dr().
>>>>
>>>> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
>>>> ---
>>>>  arch/x86/kvm/x86.c | 5 +----
>>>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>> index 13ec948f3241..0f958dcf8458 100644
>>>> --- a/arch/x86/kvm/x86.c
>>>> +++ b/arch/x86/kvm/x86.c
>>>> @@ -5504,12 +5504,9 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
>>>>  static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
>>>>  					     struct kvm_debugregs *dbgregs)
>>>>  {
>>>> -	unsigned long val;
>>>> -
>>>>  	memset(dbgregs, 0, sizeof(*dbgregs));
>>>>  	memcpy(dbgregs->db, vcpu->arch.db, sizeof(vcpu->arch.db));
>>>> -	kvm_get_dr(vcpu, 6, &val);
>>>> -	dbgregs->dr6 = val;
>>>> +	dbgregs->dr6 = vcpu->arch.dr6;
>>>
>>> Blech, kvm_get_dr() is so dumb, it takes an out parameter despite have a void
>>> return.
>>
>> Jepp, that's why I tried to get rid of it.
>>
>>>          I would rather fix that wart and go the other direction, i.e. make dr7
>>> go through kvm_get_dr().  This obviously isn't a fast path, so the extra CALL+RET
>>> is a non-issue.
>>
>> Okay. I thought, as this is an indirect call which is slightly more
>> expensive under RETPOLINE, I'd go the other way and simply open-code the
>> access, as done a few lines below in kvm_vcpu_ioctl_x86_set_debugregs().
> 
> It's not an indirect call.  It's not even strictly guaranteed to be a function
> call, e.g. within x86.c, kvm_get_dr() is in scope of kvm_vcpu_ioctl_x86_get_debugregs()
> and so a very smart compiler could fully inline and optimize it to just
> "xxx = vcpu->arch.dr6" through dead code elimination (I doubt gcc or clang actually
> does this, but it's possible).

Oh, snap! I got confused by your later patch which had all the indirect
calls. But yes, for arch/x86/kvm/x86.c you're right, gcc is smart enough
to inline it. I guess, clang can do that as well.

Thanks,
Mathias

Re: [PATCH 2/3] KVM: x86: Simplify kvm_vcpu_ioctl_x86_get_debugregs()

[Mathias Krause]

On 05.02.24 20:53, Sean Christopherson wrote:
> Subject: [PATCH] KVM: x86: Make kvm_get_dr() return a value, not use an out
>  parameter
> [...]  
> @@ -240,11 +236,8 @@ static void enter_smm_save_state_64(struct kvm_vcpu *vcpu,
>  	smram->rip    = kvm_rip_read(vcpu);
>  	smram->rflags = kvm_get_rflags(vcpu);
>  
> -
> -	kvm_get_dr(vcpu, 6, &val);
> -	smram->dr6 = val;
> -	kvm_get_dr(vcpu, 7, &val);
> -	smram->dr7 = val;
> +	smram->dr6 = kvm_get_dr(vcpu, 6);
> +	smram->dr7 = kvm_get_dr(vcpu, 7);;
                                        ^^ nit: double semicolon

>  
>  	smram->cr0 = kvm_read_cr0(vcpu);
>  	smram->cr3 = kvm_read_cr3(vcpu);
> 

Thanks,
Mathias

[PATCH 3/3] KVM: x86: Fix broken debugregs ABI for 32 bit kernels

[Mathias Krause]

The ioctl()s to get and set KVM's debug registers are broken for 32 bit
kernels as they'd only copy half of the user register state because of a
UAPI and in-kernel type mismatch (__u64 vs. unsigned long; 8 vs. 4
bytes).

This makes it impossible for userland to set anything but DR0 without
resorting to bit folding tricks.

Switch to a loop for copying debug registers that'll implicitly do the
type conversion for us, if needed.

There are likely no users (left) for 32bit KVM, fix the bug nonetheless.

Fixes: a1efbe77c1fd ("KVM: x86: Add support for saving&restoring debug registers")
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 arch/x86/kvm/x86.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0f958dcf8458..34ea934b499b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5504,8 +5504,14 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
 					     struct kvm_debugregs *dbgregs)
 {
+	unsigned int i;
+
 	memset(dbgregs, 0, sizeof(*dbgregs));
-	memcpy(dbgregs->db, vcpu->arch.db, sizeof(vcpu->arch.db));
+
+	BUILD_BUG_ON(ARRAY_SIZE(vcpu->arch.db) != ARRAY_SIZE(dbgregs->db));
+	for (i = 0; i < ARRAY_SIZE(vcpu->arch.db); i++)
+		dbgregs->db[i] = vcpu->arch.db[i];
+
 	dbgregs->dr6 = vcpu->arch.dr6;
 	dbgregs->dr7 = vcpu->arch.dr7;
 }
@@ -5513,6 +5519,8 @@ static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
 static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
 					    struct kvm_debugregs *dbgregs)
 {
+	unsigned int i;
+
 	if (dbgregs->flags)
 		return -EINVAL;
 
@@ -5521,7 +5529,9 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
 	if (!kvm_dr7_valid(dbgregs->dr7))
 		return -EINVAL;
 
-	memcpy(vcpu->arch.db, dbgregs->db, sizeof(vcpu->arch.db));
+	for (i = 0; i < ARRAY_SIZE(vcpu->arch.db); i++)
+		vcpu->arch.db[i] = dbgregs->db[i];
+
 	kvm_update_dr0123(vcpu);
 	vcpu->arch.dr6 = dbgregs->dr6;
 	vcpu->arch.dr7 = dbgregs->dr7;
-- 
2.39.2

Re: [PATCH 3/3] KVM: x86: Fix broken debugregs ABI for 32 bit kernels

[Sean Christopherson]

On Sat, Feb 03, 2024, Mathias Krause wrote:
> The ioctl()s to get and set KVM's debug registers are broken for 32 bit
> kernels as they'd only copy half of the user register state because of a
> UAPI and in-kernel type mismatch (__u64 vs. unsigned long; 8 vs. 4
> bytes).
> 
> This makes it impossible for userland to set anything but DR0 without
> resorting to bit folding tricks.
> 
> Switch to a loop for copying debug registers that'll implicitly do the
> type conversion for us, if needed.
> 
> There are likely no users (left) for 32bit KVM, fix the bug nonetheless.

And this has always been broken, so if there were ever users of 32-bit KVM, they
obviously didn't use this API :-)

If the code weren't also a cleanup for 64-bit, I would vote to change the APIs
to just fail for 32-bit.  But there's just no good reason to assume that the
layouts of KVM's internal storage and "struct kvm_debugregs" are identical.

Re: [PATCH 3/3] KVM: x86: Fix broken debugregs ABI for 32 bit kernels

[Mathias Krause]

On 05.02.24 19:46, Sean Christopherson wrote:
> On Sat, Feb 03, 2024, Mathias Krause wrote:
>> The ioctl()s to get and set KVM's debug registers are broken for 32 bit
>> kernels as they'd only copy half of the user register state because of a
>> UAPI and in-kernel type mismatch (__u64 vs. unsigned long; 8 vs. 4
>> bytes).
>>
>> This makes it impossible for userland to set anything but DR0 without
>> resorting to bit folding tricks.
>>
>> Switch to a loop for copying debug registers that'll implicitly do the
>> type conversion for us, if needed.
>>
>> There are likely no users (left) for 32bit KVM, fix the bug nonetheless.
> 
> And this has always been broken,

Jepp, that's why the fixes tag mentions the commit introducing the API.
I also mentioned it already last year, tho[1]:
"... The bug (existing since the introduction of the API) effectively
makes using DR1..3 impossible."

[1]
https://lore.kernel.org/kvm/20230220104050.419438-1-minipli@grsecurity.net/

>                                  so if there were ever users of 32-bit KVM, they
> obviously didn't use this API :-)

Well, I do remember having issues with hardware breakpoints in
combination with 32 bit guests. But that was *years* ago -- maybe even
decades. Man, I'm old!

> 
> If the code weren't also a cleanup for 64-bit, I would vote to change the APIs
> to just fail for 32-bit.  But there's just no good reason to assume that the
> layouts of KVM's internal storage and "struct kvm_debugregs" are identical.

Thanks!

Re: [PATCH 0/3] KVM: x86 - misc fixes

[Sean Christopherson]

On Sat, Feb 03, 2024, Mathias Krause wrote:
> This is v2 of an old patch which gained an info leak fix to make it a
> series.
> 
> v1 -> v2:
> - drop the stable cc, shorten commit log
> - split out dr6 change
> - add KVM_GET_MSRS stack info leak fix

In the future, please post unrelated patches separately.  Bundling things into a
"misc fixes" series might seem like it's less work for maintainers, but for me at
least, it ends up being more work, e.g. to route patches into different branches.
It often ends up being more work for the contributor too, e.g. if only one patch
needs a new version.

Re: [PATCH 0/3] KVM: x86 - misc fixes

[Mathias Krause]

On 05.02.24 20:56, Sean Christopherson wrote:
> On Sat, Feb 03, 2024, Mathias Krause wrote:
>> This is v2 of an old patch which gained an info leak fix to make it a
>> series.
>>
>> v1 -> v2:
>> - drop the stable cc, shorten commit log
>> - split out dr6 change
>> - add KVM_GET_MSRS stack info leak fix
> 
> In the future, please post unrelated patches separately.  Bundling things into a
> "misc fixes" series might seem like it's less work for maintainers, but for me at
> least, it ends up being more work, e.g. to route patches into different branches.
> It often ends up being more work for the contributor too, e.g. if only one patch
> needs a new version.

That was a little sloppy, agreed. Will do better next time.

Thanks,
Mathias

Re: [PATCH 0/3] KVM: x86 - misc fixes

[Sean Christopherson]

On Sat, 03 Feb 2024 13:45:19 +0100, Mathias Krause wrote:
> This is v2 of an old patch which gained an info leak fix to make it a
> series.
> 
> v1 -> v2:
> - drop the stable cc, shorten commit log
> - split out dr6 change
> - add KVM_GET_MSRS stack info leak fix
> 
> [...]

Applied patch 1 to kvm-x86 fixes (for 6.8), and patch 3 to misc (for 6.9).  As
promised/threatened, I'll send a small series to cleanup the kvm_get_dr() mess.

[1/3] KVM: x86: Fix KVM_GET_MSRS stack info leak
      https://github.com/kvm-x86/linux/commit/3376ca3f1a20
[2/3] KVM: x86: Simplify kvm_vcpu_ioctl_x86_get_debugregs()
      (not applied)
[3/3] KVM: x86: Fix broken debugregs ABI for 32 bit kernels
      https://github.com/kvm-x86/linux/commit/e1dda3afe2a9

--
https://github.com/kvm-x86/linux/tree/next