| Message ID | 20230214103304.3689213-1-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm: initialize all of the kvm_debugregs structure before sending it to userspace | expand |
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>
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,
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;
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
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.
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,
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(-)