* [PATCH] ARM: KVM: prevent NULL pointer dereference with KVM ioctl
@ 2013-05-07 12:55 Andre Przywara
2013-05-08 9:53 ` Marc Zyngier
0 siblings, 1 reply; 4+ messages in thread
From: Andre Przywara @ 2013-05-07 12:55 UTC (permalink / raw)
To: linux-arm-kernel
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.
Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
arch/arm/kvm/arm.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index c1fe498..0c571ff 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -893,6 +893,11 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
case KVM_SET_ONE_REG:
case KVM_GET_ONE_REG: {
struct kvm_one_reg reg;
+
+ /* Make sure they initialize the vcpu with KVM_ARM_VCPU_INIT */
+ if (unlikely(vcpu->arch.target < 0))
+ return -ENOEXEC;
+
if (copy_from_user(®, argp, sizeof(reg)))
return -EFAULT;
if (ioctl == KVM_SET_ONE_REG)
@@ -905,6 +910,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
struct kvm_reg_list reg_list;
unsigned n;
+ /* Make sure they initialize the vcpu with KVM_ARM_VCPU_INIT */
+ if (unlikely(vcpu->arch.target < 0))
+ return -ENOEXEC;
+
if (copy_from_user(®_list, user_list, sizeof(reg_list)))
return -EFAULT;
n = reg_list.n;
--
1.7.12.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH] ARM: KVM: prevent NULL pointer dereference with KVM ioctl
2013-05-07 12:55 [PATCH] ARM: KVM: prevent NULL pointer dereference with KVM ioctl Andre Przywara
@ 2013-05-08 9:53 ` Marc Zyngier
2013-05-13 5:13 ` Christoffer Dall
0 siblings, 1 reply; 4+ messages in thread
From: Marc Zyngier @ 2013-05-08 9:53 UTC (permalink / raw)
To: linux-arm-kernel
On 07/05/13 13:55, 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.
>
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
> arch/arm/kvm/arm.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index c1fe498..0c571ff 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -893,6 +893,11 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> case KVM_SET_ONE_REG:
> case KVM_GET_ONE_REG: {
> struct kvm_one_reg reg;
> +
> + /* Make sure they initialize the vcpu with KVM_ARM_VCPU_INIT */
> + if (unlikely(vcpu->arch.target < 0))
> + return -ENOEXEC;
> +
> if (copy_from_user(®, argp, sizeof(reg)))
> return -EFAULT;
> if (ioctl == KVM_SET_ONE_REG)
> @@ -905,6 +910,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> struct kvm_reg_list reg_list;
> unsigned n;
>
> + /* Make sure they initialize the vcpu with KVM_ARM_VCPU_INIT */
> + if (unlikely(vcpu->arch.target < 0))
> + return -ENOEXEC;
> +
> if (copy_from_user(®_list, user_list, sizeof(reg_list)))
> return -EFAULT;
> n = reg_list.n;
>
So we also have the same snippet in kvm_arch_vcpu_ioctl_run().
Can you create a static function (kvm_vcpu_initialized?) containing that
code and make all three sites use it?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH] ARM: KVM: prevent NULL pointer dereference with KVM ioctl
2013-05-08 9:53 ` Marc Zyngier
@ 2013-05-13 5:13 ` Christoffer Dall
2013-05-13 5:53 ` Christoffer Dall
0 siblings, 1 reply; 4+ messages in thread
From: Christoffer Dall @ 2013-05-13 5:13 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, May 08, 2013 at 10:53:28AM +0100, Marc Zyngier wrote:
> On 07/05/13 13:55, 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.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> > ---
> > arch/arm/kvm/arm.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> > index c1fe498..0c571ff 100644
> > --- a/arch/arm/kvm/arm.c
> > +++ b/arch/arm/kvm/arm.c
> > @@ -893,6 +893,11 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> > case KVM_SET_ONE_REG:
> > case KVM_GET_ONE_REG: {
> > struct kvm_one_reg reg;
> > +
> > + /* Make sure they initialize the vcpu with KVM_ARM_VCPU_INIT */
> > + if (unlikely(vcpu->arch.target < 0))
> > + return -ENOEXEC;
> > +
> > if (copy_from_user(®, argp, sizeof(reg)))
> > return -EFAULT;
> > if (ioctl == KVM_SET_ONE_REG)
> > @@ -905,6 +910,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> > struct kvm_reg_list reg_list;
> > unsigned n;
> >
> > + /* Make sure they initialize the vcpu with KVM_ARM_VCPU_INIT */
> > + if (unlikely(vcpu->arch.target < 0))
> > + return -ENOEXEC;
> > +
> > if (copy_from_user(®_list, user_list, sizeof(reg_list)))
> > return -EFAULT;
> > n = reg_list.n;
> >
>
> So we also have the same snippet in kvm_arch_vcpu_ioctl_run().
>
> Can you create a static function (kvm_vcpu_initialized?) containing that
> code and make all three sites use it?
>
Andre, will you re-spin your patch as per Marc's comments?
Thanks,
-Christoffer
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH] ARM: KVM: prevent NULL pointer dereference with KVM ioctl
2013-05-13 5:13 ` Christoffer Dall
@ 2013-05-13 5:53 ` Christoffer Dall
0 siblings, 0 replies; 4+ messages in thread
From: Christoffer Dall @ 2013-05-13 5:53 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, May 12, 2013 at 10:13 PM, Christoffer Dall
<cdall@cs.columbia.edu> wrote:
> On Wed, May 08, 2013 at 10:53:28AM +0100, Marc Zyngier wrote:
>> On 07/05/13 13:55, 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.
>> >
>> > Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> > ---
>> > arch/arm/kvm/arm.c | 9 +++++++++
>> > 1 file changed, 9 insertions(+)
>> >
>> > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> > index c1fe498..0c571ff 100644
>> > --- a/arch/arm/kvm/arm.c
>> > +++ b/arch/arm/kvm/arm.c
>> > @@ -893,6 +893,11 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>> > case KVM_SET_ONE_REG:
>> > case KVM_GET_ONE_REG: {
>> > struct kvm_one_reg reg;
>> > +
>> > + /* Make sure they initialize the vcpu with KVM_ARM_VCPU_INIT */
>> > + if (unlikely(vcpu->arch.target < 0))
>> > + return -ENOEXEC;
>> > +
>> > if (copy_from_user(®, argp, sizeof(reg)))
>> > return -EFAULT;
>> > if (ioctl == KVM_SET_ONE_REG)
>> > @@ -905,6 +910,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>> > struct kvm_reg_list reg_list;
>> > unsigned n;
>> >
>> > + /* Make sure they initialize the vcpu with KVM_ARM_VCPU_INIT */
>> > + if (unlikely(vcpu->arch.target < 0))
>> > + return -ENOEXEC;
>> > +
>> > if (copy_from_user(®_list, user_list, sizeof(reg_list)))
>> > return -EFAULT;
>> > n = reg_list.n;
>> >
>>
>> So we also have the same snippet in kvm_arch_vcpu_ioctl_run().
>>
>> Can you create a static function (kvm_vcpu_initialized?) containing that
>> code and make all three sites use it?
>>
> Andre, will you re-spin your patch as per Marc's comments?
>
oh, never mind, missed it when I looked over the mails.
-Christoffer
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-05-13 5:53 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-07 12:55 [PATCH] ARM: KVM: prevent NULL pointer dereference with KVM ioctl Andre Przywara
2013-05-08 9:53 ` Marc Zyngier
2013-05-13 5:13 ` Christoffer Dall
2013-05-13 5:53 ` Christoffer Dall
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).