| Message ID | 20240203124522.592778-4-minipli@grsecurity.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86 - misc fixes | expand |
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.
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!
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;
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(-)