| Message ID | 1442400070-23316-1-git-send-email-marc.zyngier@arm.com |
|---|---|
| State | New |
| Headers | show |
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>
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...
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>
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; > }
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.
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; }
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(-)