* [PATCH] arm64: KVM: Fix user access for debug registers
@ 2015-09-16 10:41 Marc Zyngier
2015-09-16 13:41 ` Christoffer Dall
2015-09-16 14:42 ` Alex Bennée
0 siblings, 2 replies; 6+ messages in thread
From: Marc Zyngier @ 2015-09-16 10:41 UTC (permalink / raw)
To: linux-arm-kernel
When setting the debug register from userspace, make sure that
copy_from_user() is called with its parameters in the expected
order. It otherwise doesn't do what you think.
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Cc: Alex Benn?e <alex.bennee@linaro.org>
Fixes: 84e690bfbed1 ("KVM: arm64: introduce vcpu->arch.debug_ptr")
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
arch/arm64/kvm/sys_regs.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index b41607d..1d0463e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -272,7 +272,7 @@ static int set_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
{
__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg];
- if (copy_from_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
+ if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
return -EFAULT;
return 0;
}
@@ -314,7 +314,7 @@ static int set_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
{
__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg];
- if (copy_from_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
+ if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
return -EFAULT;
return 0;
@@ -358,7 +358,7 @@ static int set_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
{
__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg];
- if (copy_from_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
+ if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
return -EFAULT;
return 0;
}
@@ -400,7 +400,7 @@ static int set_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
{
__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg];
- if (copy_from_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
+ if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
return -EFAULT;
return 0;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] arm64: KVM: Fix user access for debug registers
2015-09-16 10:41 [PATCH] arm64: KVM: Fix user access for debug registers Marc Zyngier
@ 2015-09-16 13:41 ` Christoffer Dall
2015-09-16 14:06 ` Marc Zyngier
2015-09-16 14:35 ` Alex Bennée
2015-09-16 14:42 ` Alex Bennée
1 sibling, 2 replies; 6+ messages in thread
From: Christoffer Dall @ 2015-09-16 13:41 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Sep 16, 2015 at 11:41:10AM +0100, Marc Zyngier wrote:
> When setting the debug register from userspace, make sure that
> copy_from_user() is called with its parameters in the expected
> order. It otherwise doesn't do what you think.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: Alex Benn?e <alex.bennee@linaro.org>
> Fixes: 84e690bfbed1 ("KVM: arm64: introduce vcpu->arch.debug_ptr")
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
yikes!
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] arm64: KVM: Fix user access for debug registers
2015-09-16 13:41 ` Christoffer Dall
@ 2015-09-16 14:06 ` Marc Zyngier
2015-09-16 14:35 ` Alex Bennée
1 sibling, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2015-09-16 14:06 UTC (permalink / raw)
To: linux-arm-kernel
On 16/09/15 14:41, Christoffer Dall wrote:
> On Wed, Sep 16, 2015 at 11:41:10AM +0100, Marc Zyngier wrote:
>> When setting the debug register from userspace, make sure that
>> copy_from_user() is called with its parameters in the expected
>> order. It otherwise doesn't do what you think.
>>
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Alex Benn?e <alex.bennee@linaro.org>
>> Fixes: 84e690bfbed1 ("KVM: arm64: introduce vcpu->arch.debug_ptr")
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>
> yikes!
>
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
>
Thanks. Merged and pushed out to -next, together with the physaddr_t patch.
M.
--
Jazz is not dead. It just smells funny...
________________________________
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] arm64: KVM: Fix user access for debug registers
2015-09-16 13:41 ` Christoffer Dall
2015-09-16 14:06 ` Marc Zyngier
@ 2015-09-16 14:35 ` Alex Bennée
2015-09-16 14:46 ` Marc Zyngier
1 sibling, 1 reply; 6+ messages in thread
From: Alex Bennée @ 2015-09-16 14:35 UTC (permalink / raw)
To: linux-arm-kernel
Christoffer Dall <christoffer.dall@linaro.org> writes:
> On Wed, Sep 16, 2015 at 11:41:10AM +0100, Marc Zyngier wrote:
>> When setting the debug register from userspace, make sure that
>> copy_from_user() is called with its parameters in the expected
>> order. It otherwise doesn't do what you think.
>>
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Alex Benn?e <alex.bennee@linaro.org>
>> Fixes: 84e690bfbed1 ("KVM: arm64: introduce vcpu->arch.debug_ptr")
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>
> yikes!
OK I'm now muchly confused as to how it could have worked...
>
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
--
Alex Benn?e
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] arm64: KVM: Fix user access for debug registers
2015-09-16 14:35 ` Alex Bennée
@ 2015-09-16 14:46 ` Marc Zyngier
0 siblings, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2015-09-16 14:46 UTC (permalink / raw)
To: linux-arm-kernel
On 16/09/15 15:35, Alex Benn?e wrote:
>
> Christoffer Dall <christoffer.dall@linaro.org> writes:
>
>> On Wed, Sep 16, 2015 at 11:41:10AM +0100, Marc Zyngier wrote:
>>> When setting the debug register from userspace, make sure that
>>> copy_from_user() is called with its parameters in the expected
>>> order. It otherwise doesn't do what you think.
>>>
>>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>>> Cc: Alex Benn?e <alex.bennee@linaro.org>
>>> Fixes: 84e690bfbed1 ("KVM: arm64: introduce vcpu->arch.debug_ptr")
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>
>> yikes!
>
> OK I'm now muchly confused as to how it could have worked...
Well, we only write the registers at boot time, and corrupting userspace
did go unnoticed. I was only able to reproduce this on a model with PAN
enabled.
Copy-paste bug.
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] arm64: KVM: Fix user access for debug registers
2015-09-16 10:41 [PATCH] arm64: KVM: Fix user access for debug registers Marc Zyngier
2015-09-16 13:41 ` Christoffer Dall
@ 2015-09-16 14:42 ` Alex Bennée
1 sibling, 0 replies; 6+ messages in thread
From: Alex Bennée @ 2015-09-16 14:42 UTC (permalink / raw)
To: linux-arm-kernel
Marc Zyngier <marc.zyngier@arm.com> writes:
> When setting the debug register from userspace, make sure that
> copy_from_user() is called with its parameters in the expected
> order. It otherwise doesn't do what you think.
Oops. Well that exposes a big hole in my testing. While I tested
debugging inside the guest worked before and after being guest debugged
I think GDBs tendency to reload all the debug registers between each
step may have masked this.
Debugging GDB in action or some sort of migration event would of course
screw this up but I'm afraid my testing wasn't evil enough.
Anyway have a:
Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: Alex Benn?e <alex.bennee@linaro.org>
> Fixes: 84e690bfbed1 ("KVM: arm64: introduce vcpu->arch.debug_ptr")
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> arch/arm64/kvm/sys_regs.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index b41607d..1d0463e 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -272,7 +272,7 @@ static int set_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> {
> __u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg];
>
> - if (copy_from_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
> + if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
> return -EFAULT;
> return 0;
> }
> @@ -314,7 +314,7 @@ static int set_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> {
> __u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg];
>
> - if (copy_from_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
> + if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
> return -EFAULT;
>
> return 0;
> @@ -358,7 +358,7 @@ static int set_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> {
> __u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg];
>
> - if (copy_from_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
> + if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
> return -EFAULT;
> return 0;
> }
> @@ -400,7 +400,7 @@ static int set_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> {
> __u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg];
>
> - if (copy_from_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
> + if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
> return -EFAULT;
> return 0;
> }
--
Alex Benn?e
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-09-16 14:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-16 10:41 [PATCH] arm64: KVM: Fix user access for debug registers Marc Zyngier
2015-09-16 13:41 ` Christoffer Dall
2015-09-16 14:06 ` Marc Zyngier
2015-09-16 14:35 ` Alex Bennée
2015-09-16 14:46 ` Marc Zyngier
2015-09-16 14:42 ` Alex Bennée
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).