[PATCH] kvm: initialize all of the kvm_debugregs structure before sending it to userspace

[PATCH] kvm: initialize all of the kvm_debugregs structure before sending it to userspace

[Greg Kroah-Hartman]

When calling the KVM_GET_DEBUGREGS ioctl, on some configurations, there
might be some unitialized portions of the kvm_debugregs structure that
could be copied to userspace.  Prevent this as is done in the other kvm
ioctls, by setting the whole structure to 0 before copying anything into
it.

Bonus is that this reduces the lines of code as the explicit flag
setting and reserved space zeroing out can be removed.

Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: <x86@kernel.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: stable <stable@kernel.org>
Reported-by: Xingyuan Mo <hdthky0@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 arch/x86/kvm/x86.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index da4bbd043a7b..50a95c8082fa 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5254,12 +5254,11 @@ static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
 {
 	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->flags = 0;
-	memset(&dbgregs->reserved, 0, sizeof(dbgregs->reserved));
 }
 
 static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
-- 
2.39.1

Re: [PATCH] kvm: initialize all of the kvm_debugregs structure before sending it to userspace

[Xingyuan Mo]

On Tue, Feb 14, 2023 at 6:33 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> When calling the KVM_GET_DEBUGREGS ioctl, on some configurations, there
> might be some unitialized portions of the kvm_debugregs structure that
> could be copied to userspace.  Prevent this as is done in the other kvm
> ioctls, by setting the whole structure to 0 before copying anything into
> it.
>
> Bonus is that this reduces the lines of code as the explicit flag
> setting and reserved space zeroing out can be removed.
>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: <x86@kernel.org>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: stable <stable@kernel.org>
> Reported-by: Xingyuan Mo <hdthky0@gmail.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  arch/x86/kvm/x86.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index da4bbd043a7b..50a95c8082fa 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5254,12 +5254,11 @@ static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
>  {
>         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->flags = 0;
> -       memset(&dbgregs->reserved, 0, sizeof(dbgregs->reserved));
>  }
>
>  static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
> --
> 2.39.1
>

Tested-by: Xingyuan Mo <hdthky0@gmail.com>

Re: [PATCH] kvm: initialize all of the kvm_debugregs structure before sending it to userspace

[Paolo Bonzini]

On 2/14/23 11:33, Greg Kroah-Hartman wrote:
> When calling the KVM_GET_DEBUGREGS ioctl, on some configurations, there
> might be some unitialized portions of the kvm_debugregs structure that
> could be copied to userspace.  Prevent this as is done in the other kvm
> ioctls, by setting the whole structure to 0 before copying anything into
> it.
> 
> Bonus is that this reduces the lines of code as the explicit flag
> setting and reserved space zeroing out can be removed.

Queued, thanks!

Paolo

> 
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: <x86@kernel.org>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: stable <stable@kernel.org>
> Reported-by: Xingyuan Mo <hdthky0@gmail.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>   arch/x86/kvm/x86.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index da4bbd043a7b..50a95c8082fa 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5254,12 +5254,11 @@ static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
>   {
>   	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->flags = 0;
> -	memset(&dbgregs->reserved, 0, sizeof(dbgregs->reserved));
>   }
>   
>   static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,

Re: [PATCH] kvm: initialize all of the kvm_debugregs structure before sending it to userspace

[Mathias Krause]

On 14.02.23 11:33, Greg Kroah-Hartman wrote:
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index da4bbd043a7b..50a95c8082fa 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5254,12 +5254,11 @@ static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
>  {
>  	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->flags = 0;
> -	memset(&dbgregs->reserved, 0, sizeof(dbgregs->reserved));
>  }
>  
>  static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,

While this change handles the info leak for 32 bit kernels just fine, it
completely ignores that the ABI is broken for such kernels. The bug
(existing since the introduction of the API) effectively makes using
DR1..3 impossible. The memcpy() will only copy half of dbgregs->db and
effectively only allows setting DR0 to its intended value. The remaining
registers get shuffled around (lower half of db[1] will end up in DR2,
not DR1) or completely ignored (db[2..3] which should end up in DR3 and
DR4). Now, this broken ABI might be considdered "API," so I gave it a
look...

A Debian code search gave only three real users of these ioctl()s:
- VirtualBox ([1], lines 1735 ff.),
- QEMU ([2], in kvm_put_debugregs(): lines 4491 ff. and
  kvm_get_debugregs(): lines 4515 ff.) and
- Linux's KVM selftests ([3], lines 722 ff., used in vcpu_load_state()
  and vcpu_save_state()).

Linux's selftest uses the API only to read and bounce back the state --
doesn't do any sanity checks on it.

VirtualBox and QEMU, OTOH, assume that the array is properly filled,
i.e. indices 0..3 map to DR0..3. This means, these users are currently
(and *always* have been) broken when trying to set DR1..3. Time to get
them fixed before x86-32 vanishes into irrelevance.

[1] https://www.virtualbox.org/browser/vbox/trunk/src/VBox/VMM/VMMR3/NEMR3Native-linux.cpp?rev=98193#L1735
[2] https://gitlab.com/qemu-project/qemu/-/blob/v7.2.0/target/i386/kvm/kvm.c#L4480-4522
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/kvm/include/x86_64/processor.h?h=v6.2#n722

An ABI-breaking^Wfixing change like below might be worth to apply on top
to get that long standing bug fixed:

-- >8 --
Subject: [PATCH] KVM: x86: Fix broken debugregs ABI for 32 bit kernels

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
the 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.

This ABI breaking change actually fixes known users [1,2] that have been
broken since the API's introduction in commit a1efbe77c1fd ("KVM: x86:
Add support for saving&restoring debug registers").

Also take 'dr6' from the arch part directly, as we do for 'dr7'. There's
no need to take the clunky route via kvm_get_dr().

[1] https://www.virtualbox.org/browser/vbox/trunk/src/VBox/VMM/VMMR3/NEMR3Native-linux.cpp?rev=98193#L1735
[2] https://gitlab.com/qemu-project/qemu/-/blob/v7.2.0/target/i386/kvm/kvm.c#L4480-4522

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a2c299d47e69..db3967de7958 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5261,18 +5261,23 @@ 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;
+	unsigned int i;
 
 	memset(dbgregs, 0, sizeof(*dbgregs));
-	memcpy(dbgregs->db, vcpu->arch.db, sizeof(vcpu->arch.db));
-	kvm_get_dr(vcpu, 6, &val);
-	dbgregs->dr6 = val;
+
+	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;
 }
 
 static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
 					    struct kvm_debugregs *dbgregs)
 {
+	unsigned int i;
+
 	if (dbgregs->flags)
 		return -EINVAL;
 
@@ -5281,7 +5286,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.30.2

Re: [PATCH] kvm: initialize all of the kvm_debugregs structure before sending it to userspace

[Mathias Krause]

[-- Attachment #1: Type: text/plain, Size: 5780 bytes --]

On 20.02.23 11:40, Mathias Krause wrote:
> On 14.02.23 11:33, Greg Kroah-Hartman wrote:
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index da4bbd043a7b..50a95c8082fa 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -5254,12 +5254,11 @@ static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu,
>>  {
>>  	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->flags = 0;
>> -	memset(&dbgregs->reserved, 0, sizeof(dbgregs->reserved));
>>  }
>>  
>>  static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
> 
> While this change handles the info leak for 32 bit kernels just fine, it
> completely ignores that the ABI is broken for such kernels. The bug
> (existing since the introduction of the API) effectively makes using
> DR1..3 impossible. The memcpy() will only copy half of dbgregs->db and
> effectively only allows setting DR0 to its intended value. The remaining
> registers get shuffled around (lower half of db[1] will end up in DR2,
> not DR1) or completely ignored (db[2..3] which should end up in DR3 and
> DR4). Now, this broken ABI might be considdered "API," so I gave it a
> look...
> 
> A Debian code search gave only three real users of these ioctl()s:
> - VirtualBox ([1], lines 1735 ff.),
> - QEMU ([2], in kvm_put_debugregs(): lines 4491 ff. and
>   kvm_get_debugregs(): lines 4515 ff.) and
> - Linux's KVM selftests ([3], lines 722 ff., used in vcpu_load_state()
>   and vcpu_save_state()).
> 
> Linux's selftest uses the API only to read and bounce back the state --
> doesn't do any sanity checks on it.
> 
> VirtualBox and QEMU, OTOH, assume that the array is properly filled,
> i.e. indices 0..3 map to DR0..3. This means, these users are currently
> (and *always* have been) broken when trying to set DR1..3. Time to get
> them fixed before x86-32 vanishes into irrelevance.
> 
> [1] https://www.virtualbox.org/browser/vbox/trunk/src/VBox/VMM/VMMR3/NEMR3Native-linux.cpp?rev=98193#L1735
> [2] https://gitlab.com/qemu-project/qemu/-/blob/v7.2.0/target/i386/kvm/kvm.c#L4480-4522
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/kvm/include/x86_64/processor.h?h=v6.2#n722
> 
> An ABI-breaking^Wfixing change like below might be worth to apply on top
> to get that long standing bug fixed:
> 
> -- >8 --
> Subject: [PATCH] KVM: x86: Fix broken debugregs ABI for 32 bit kernels
> 
> 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
> the 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.
> 
> This ABI breaking change actually fixes known users [1,2] that have been
> broken since the API's introduction in commit a1efbe77c1fd ("KVM: x86:
> Add support for saving&restoring debug registers").
> 
> Also take 'dr6' from the arch part directly, as we do for 'dr7'. There's
> no need to take the clunky route via kvm_get_dr().
> 
> [1] https://www.virtualbox.org/browser/vbox/trunk/src/VBox/VMM/VMMR3/NEMR3Native-linux.cpp?rev=98193#L1735
> [2] https://gitlab.com/qemu-project/qemu/-/blob/v7.2.0/target/i386/kvm/kvm.c#L4480-4522
> 
> Fixes: a1efbe77c1fd ("KVM: x86: Add support for saving&restoring debug registers")
> Cc: stable <stable@kernel.org>	# needs 2c10b61421a2
> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
> ---
>  arch/x86/kvm/x86.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a2c299d47e69..db3967de7958 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5261,18 +5261,23 @@ 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;
> +	unsigned int i;
>  
>  	memset(dbgregs, 0, sizeof(*dbgregs));
> -	memcpy(dbgregs->db, vcpu->arch.db, sizeof(vcpu->arch.db));
> -	kvm_get_dr(vcpu, 6, &val);
> -	dbgregs->dr6 = val;
> +
> +	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;
>  }
>  
>  static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
>  					    struct kvm_debugregs *dbgregs)
>  {
> +	unsigned int i;
> +
>  	if (dbgregs->flags)
>  		return -EINVAL;
>  
> @@ -5281,7 +5286,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;

Ping? No interest in fixing this for i386?

I've attached a PoC showing the bug. It needs to be run on a 32-bit x86
kernel to actually trigger it. It does, however, only show half of the
bug, as the in-VM state differs from KVM's state, too. However, I was
too lazy to implement a full guest code execution setup (and the in-tree
selftests don't support i386), so you have to believe me or try to
follow my argumentation along with KVM's code.

Thanks,
Mathias

[-- Attachment #2: dr_kvm.c --]
[-- Type: text/x-csrc, Size: 2772 bytes --]

/*
 * Test for KVM's KVM_{GET,SET}_DEBUGREGS ioctl() being broken for i386.
 * It only copies half of the registers and interleaves them even.
 *
 * This test only shows half of the bug, as there's no way to retrieve the
 * in-VM effective debug registers without actually running code in the VM
 * itself (I was too lazy to implement :P). But if you would do, you'd see
 * that on the first iteration already, where we want to set DR0 only, we
 * actually do set DR1 too -- DR0 to 0xc0ffee0, DR1 to 0xbadc0de0. For the
 * second iteration we fail to set DR1 to 0xc0ffee1 but instead set DR2 to
 * 0xc0ffee1 and DR3 to 0xbadc0de1.
 *
 * Compile as:
 *   $ gcc -o dr_kvm dr_kvm.c
 *
 * Run as:
 *   $ ./dr_kvm
 *
 * If you're runnining a buggy 32-bit x86 kernel, the output will look like
 * this:
 *   $ ./dr_kvm
 *   dr_kvm: unexpected value of DR0 (0xbadc0de00c0ffee0, should be 0xc0ffee0) after setting DR0
 *   dr_kvm: unexpected value of DR1 (0xbadc0de10c0ffee1, should be 0xc0ffee1) after setting DR1
 *   dr_kvm: unexpected value of DR2 (0x0, should be 0xc0ffee2) after setting DR2
 *   dr_kvm: unexpected value of DR3 (0x0, should be 0xc0ffee3) after setting DR3
 *
 * On a 64 bit kernel or a kernel shiping the fix[1], no output should be
 * generated.
 *
 * [1] https://lore.kernel.org/kvm/20230220104050.419438-1-minipli@grsecurity.net/
 *
 * - minipli
 */

#include <linux/kvm.h>
#include <sys/ioctl.h>
#include <sys/stat.h>
#include <unistd.h>
#include <string.h>
#include <fcntl.h>
#include <err.h>

int main(void) {
	static const char dev_kvm[] = "/dev/kvm";
	struct kvm_debugregs dbg_regs;
	int kvm, vm, cpu;
	int res = 0;
	int i, j;

	kvm = open(dev_kvm, O_RDWR);
	if (kvm < 0)
		err(1, "open(%s)", dev_kvm);

	vm = ioctl(kvm, KVM_CREATE_VM, NULL);
	if (vm < 0)
		err(1, "ioctl(KVM_CREATE_VM)");

	if (ioctl(kvm, KVM_CHECK_EXTENSION, KVM_CAP_DEBUGREGS) == 0)
		errx(1, "KVM_CAP_DEBUGREGS not supported!");

	cpu = ioctl(vm, KVM_CREATE_VCPU, 0);
	if (vm < 0)
		err(1, "ioctl(KVM_CREATE_VCPU)");

	for (i = 0; i < 4; i++) {
		unsigned long db_val;

		memset(&dbg_regs, 0, sizeof(dbg_regs));

		dbg_regs.db[i] = 0xbadc0de0 | i;
		dbg_regs.db[i] <<= 32;
		dbg_regs.db[i] |= 0xc0ffee0 | i;

		db_val = dbg_regs.db[i];

		if (ioctl(cpu, KVM_SET_DEBUGREGS, &dbg_regs) != 0)
			err(1, "ioctl(KVM_SET_DEBUGREGS)");

		memset(&dbg_regs, 0, sizeof(dbg_regs));
		if (ioctl(cpu, KVM_GET_DEBUGREGS, &dbg_regs) != 0)
			err(1, "ioctl(KVM_GET_DEBUGREGS)");

		for (j = 0; j < 4; j++) {
			unsigned long expect = j == i ? db_val : 0;

			if (dbg_regs.db[j] != expect) {
				warnx("unexpected value of DR%d (0x%llx, should be 0x%lx) after setting DR%d",
				      j, dbg_regs.db[j], expect, i);
				res++;
			}
		}
	}

	close(kvm);
	close(cpu);
	close(vm);

	return res;
}

Re: [PATCH] kvm: initialize all of the kvm_debugregs structure before sending it to userspace

[Sean Christopherson]

On Mon, Apr 03, 2023, Mathias Krause wrote:
> On 20.02.23 11:40, Mathias Krause wrote:
> > VirtualBox and QEMU, OTOH, assume that the array is properly filled,
> > i.e. indices 0..3 map to DR0..3. This means, these users are currently
> > (and *always* have been) broken when trying to set DR1..3. Time to get
> > them fixed before x86-32 vanishes into irrelevance.

Practically speaking, KVM support for 32-bit host kernels has been irrelevant for
years.

> > [1] https://www.virtualbox.org/browser/vbox/trunk/src/VBox/VMM/VMMR3/NEMR3Native-linux.cpp?rev=98193#L1735
> > [2] https://gitlab.com/qemu-project/qemu/-/blob/v7.2.0/target/i386/kvm/kvm.c#L4480-4522
> > [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/kvm/include/x86_64/processor.h?h=v6.2#n722
> > 
> > An ABI-breaking^Wfixing change like below might be worth to apply on top
> > to get that long standing bug fixed:
> > 
> > -- >8 --
> > Subject: [PATCH] KVM: x86: Fix broken debugregs ABI for 32 bit kernels
> > 
> > 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
> > the 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.
> > 
> > This ABI breaking change actually fixes known users [1,2] that have been
> > broken since the API's introduction in commit a1efbe77c1fd ("KVM: x86:
> > Add support for saving&restoring debug registers").

Are there actually real users?  VMMs that invoke the ioctls(), sure.  But I highly
doubt there are actual deployments/users that run VMs on top of 32-bit kernels.

I like the patch, but would prefer not to mark it for stable, and definitely don't
want the changelog to incorrectly assert that there actually users that would
benefit from the fix.

The only reason we haven't deprecated support for KVM on 32-bit kernels is because
we want to be able to test nested TDP with a 32-bit L1 hypervisor, but I'm starting
to think even that is a weak excuse.   The only potential problem with using an old
kernel in L1 is that we _might_ not be able to test newfangled features.

> > Also take 'dr6' from the arch part directly, as we do for 'dr7'. There's
> > no need to take the clunky route via kvm_get_dr().

This belongs in a separate patch.