| Message ID | 1368052086-25059-1-git-send-email-andre.przywara@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 09, 2013 at 12:28:06AM +0200, Andre Przywara wrote: > Some ARM KVM VCPU ioctls require the vCPU to be properly initialized > with the KVM_ARM_VCPU_INIT ioctl before being used with further > requests. KVM_RUN checks whether this initialization has been > done, but other ioctls do not. > Namely KVM_GET_REG_LIST will dereference an array with index -1 > without initialization and thus leads to a kernel oops. > Fix this by adding checks before executing the ioctl handlers. > > Changes from v1: > * moved check into a static function with a meaningful name > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > arch/arm/kvm/arm.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > index c1fe498..b73b587 100644 > --- a/arch/arm/kvm/arm.c > +++ b/arch/arm/kvm/arm.c > @@ -676,6 +676,15 @@ static void vcpu_pause(struct kvm_vcpu *vcpu) > wait_event_interruptible(*wq, !vcpu->arch.pause); > } > > +/* > + * Some ioctls require initialization by KVM_ARM_VCPU_INIT first, check > + * this with this function > + */ > +static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu) > +{ > + return vcpu->arch.target >= 0; > +} > + this should probably be a static inline in a header file instead, it's likely that it could be called from another file. > /** > * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute guest code > * @vcpu: The VCPU pointer > @@ -692,8 +701,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > int ret; > sigset_t sigsaved; > > - /* Make sure they initialize the vcpu with KVM_ARM_VCPU_INIT */ > - if (unlikely(vcpu->arch.target < 0)) > + if (unlikely(!kvm_vcpu_initialized(vcpu))) > return -ENOEXEC; > > ret = kvm_vcpu_first_run_init(vcpu); > @@ -893,6 +901,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp, > case KVM_SET_ONE_REG: > case KVM_GET_ONE_REG: { > struct kvm_one_reg reg; > + > + if (unlikely(!kvm_vcpu_initialized(vcpu))) > + return -ENOEXEC; > + > if (copy_from_user(®, argp, sizeof(reg))) > return -EFAULT; > if (ioctl == KVM_SET_ONE_REG) > @@ -905,6 +917,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp, > struct kvm_reg_list reg_list; > unsigned n; > > + if (unlikely(!kvm_vcpu_initialized(vcpu))) > + return -ENOEXEC; > + > if (copy_from_user(®_list, user_list, sizeof(reg_list))) > return -EFAULT; > n = reg_list.n; > -- > 1.7.12.1 >
On 05/13/2013 07:52 AM, Christoffer Dall wrote: > On Thu, May 09, 2013 at 12:28:06AM +0200, Andre Przywara wrote: >> Some ARM KVM VCPU ioctls require the vCPU to be properly initialized >> with the KVM_ARM_VCPU_INIT ioctl before being used with further >> requests. KVM_RUN checks whether this initialization has been >> done, but other ioctls do not. >> Namely KVM_GET_REG_LIST will dereference an array with index -1 >> without initialization and thus leads to a kernel oops. >> Fix this by adding checks before executing the ioctl handlers. >> >> Changes from v1: >> * moved check into a static function with a meaningful name >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> arch/arm/kvm/arm.c | 19 +++++++++++++++++-- >> 1 file changed, 17 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c >> index c1fe498..b73b587 100644 >> --- a/arch/arm/kvm/arm.c >> +++ b/arch/arm/kvm/arm.c >> @@ -676,6 +676,15 @@ static void vcpu_pause(struct kvm_vcpu *vcpu) >> wait_event_interruptible(*wq, !vcpu->arch.pause); >> } >> >> +/* >> + * Some ioctls require initialization by KVM_ARM_VCPU_INIT first, check >> + * this with this function >> + */ >> +static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu) >> +{ >> + return vcpu->arch.target >= 0; >> +} >> + > > this should probably be a static inline in a header file instead, it's > likely that it could be called from another file. kvm_host.h looks like a natural candidate, but unfortunately struct kvm_vcpu is opaque here, so dereferencing it does not work without further changes which I do not deem to be justified. I used kvm_coproc.h instead, which is loosely related (KVM_[SG]ET_ONE_REG) and just simply works. If you don't think that's appropriate, just drop me a note. Patch follows in a separate mail. Regards, Andre.
On Mon, May 13, 2013 at 3:23 PM, Andre Przywara <andre.przywara@linaro.org> wrote: > ooks like a natural candidate, but unfortunately struct kvm_vcpu is opaque > here, so dereferencing it does not work without further changes which I do > not deem to be justified. I used kvm_coproc.h instead, which is loosely > related (KVM_[SG]ET_ONE_REG) and just simply works. If you don't think > that's appropriate, just drop me a note. Patch follows in a separate mail. eh, not crazy about the idea. x86 has arch/x86/include/asm/kvm_guest.h, which may be the most appropriate, or this could be generalized to other architectures as well and included in include/linux/kvm_host.h. we can just stick it in arch/arm/kvm/arm.c for now and move later if need be, no need to create a fuzz. -Christoffer
On 05/14/2013 12:42 AM, Christoffer Dall wrote: > On Mon, May 13, 2013 at 3:23 PM, Andre Przywara > <andre.przywara@linaro.org> wrote: >> ooks like a natural candidate, but unfortunately struct kvm_vcpu is opaque >> here, so dereferencing it does not work without further changes which I do >> not deem to be justified. I used kvm_coproc.h instead, which is loosely >> related (KVM_[SG]ET_ONE_REG) and just simply works. If you don't think >> that's appropriate, just drop me a note. Patch follows in a separate mail. > > eh, not crazy about the idea. x86 has > arch/x86/include/asm/kvm_guest.h, which may be the most appropriate, > or this could be generalized to other architectures as well and > included in include/linux/kvm_host.h. > > we can just stick it in arch/arm/kvm/arm.c for now and move later if > need be, no need to create a fuzz. Ok, would you mind to commit v2 and add the "inline" on the way? Thanks, Andre.
On 05/14/2013 02:07 AM, Andre Przywara wrote: > On 05/14/2013 12:42 AM, Christoffer Dall wrote: >> On Mon, May 13, 2013 at 3:23 PM, Andre Przywara >> <andre.przywara@linaro.org> wrote: >>> ooks like a natural candidate, but unfortunately struct kvm_vcpu is >>> opaque >>> here, so dereferencing it does not work without further changes which >>> I do >>> not deem to be justified. I used kvm_coproc.h instead, which is loosely >>> related (KVM_[SG]ET_ONE_REG) and just simply works. If you don't think >>> that's appropriate, just drop me a note. Patch follows in a separate >>> mail. >> >> eh, not crazy about the idea. x86 has >> arch/x86/include/asm/kvm_guest.h, which may be the most appropriate, >> or this could be generalized to other architectures as well and >> included in include/linux/kvm_host.h. >> >> we can just stick it in arch/arm/kvm/arm.c for now and move later if >> need be, no need to create a fuzz. > > Ok, would you mind to commit v2 and add the "inline" on the way? You don't really need the inline if it's not in a header. The compiler will inline it anyway. Rob
On Tue, May 14, 2013 at 12:07 AM, Andre Przywara <andre.przywara@linaro.org> wrote: > On 05/14/2013 12:42 AM, Christoffer Dall wrote: >> >> On Mon, May 13, 2013 at 3:23 PM, Andre Przywara >> <andre.przywara@linaro.org> wrote: >>> >>> ooks like a natural candidate, but unfortunately struct kvm_vcpu is >>> opaque >>> here, so dereferencing it does not work without further changes which I >>> do >>> not deem to be justified. I used kvm_coproc.h instead, which is loosely >>> related (KVM_[SG]ET_ONE_REG) and just simply works. If you don't think >>> that's appropriate, just drop me a note. Patch follows in a separate >>> mail. >> >> >> eh, not crazy about the idea. x86 has >> arch/x86/include/asm/kvm_guest.h, which may be the most appropriate, >> or this could be generalized to other architectures as well and >> included in include/linux/kvm_host.h. >> >> we can just stick it in arch/arm/kvm/arm.c for now and move later if >> need be, no need to create a fuzz. > > > Ok, would you mind to commit v2 and add the "inline" on the way? > If it stays in arm.c it shouldn't have an inline on there, I'll commit the patch, thanks. -Christoffer
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index c1fe498..b73b587 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -676,6 +676,15 @@ static void vcpu_pause(struct kvm_vcpu *vcpu) wait_event_interruptible(*wq, !vcpu->arch.pause); } +/* + * Some ioctls require initialization by KVM_ARM_VCPU_INIT first, check + * this with this function + */ +static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu) +{ + return vcpu->arch.target >= 0; +} + /** * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute guest code * @vcpu: The VCPU pointer @@ -692,8 +701,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) int ret; sigset_t sigsaved; - /* Make sure they initialize the vcpu with KVM_ARM_VCPU_INIT */ - if (unlikely(vcpu->arch.target < 0)) + if (unlikely(!kvm_vcpu_initialized(vcpu))) return -ENOEXEC; ret = kvm_vcpu_first_run_init(vcpu); @@ -893,6 +901,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp, case KVM_SET_ONE_REG: case KVM_GET_ONE_REG: { struct kvm_one_reg reg; + + if (unlikely(!kvm_vcpu_initialized(vcpu))) + return -ENOEXEC; + if (copy_from_user(®, argp, sizeof(reg))) return -EFAULT; if (ioctl == KVM_SET_ONE_REG) @@ -905,6 +917,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp, struct kvm_reg_list reg_list; unsigned n; + if (unlikely(!kvm_vcpu_initialized(vcpu))) + return -ENOEXEC; + if (copy_from_user(®_list, user_list, sizeof(reg_list))) return -EFAULT; n = reg_list.n;
Some ARM KVM VCPU ioctls require the vCPU to be properly initialized with the KVM_ARM_VCPU_INIT ioctl before being used with further requests. KVM_RUN checks whether this initialization has been done, but other ioctls do not. Namely KVM_GET_REG_LIST will dereference an array with index -1 without initialization and thus leads to a kernel oops. Fix this by adding checks before executing the ioctl handlers. Changes from v1: * moved check into a static function with a meaningful name Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- arch/arm/kvm/arm.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)