| Message ID | 20180830005454.142075-1-pshier@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] KVM: Remove CREATE_IRQCHIP/SET_PIT2 race | expand |
2018-08-29 17:54-0700, Peter Shier: > From: Steve Rutherford <srutherford@google.com> > > Fixes a NULL pointer dereference, caused by the PIT firing an interrupt > before the interrupt table has been initialized. > > SET_PIT2 can race with the creation of the IRQchip. In particular, > if SET_PIT2 is called with a low PIT timer period (after the creation of > the IOAPIC, but before the instantiation of the irq routes), the PIT can > fire an interrupt at an uninitialized table. From what you describe, it seems that the problems is the timer interrupt started in kvm_vm_ioctl_set_pit2() -> kvm_pit_load_count() -> pit_load_count() -> create_pit_timer() -> hrtimer_start() that immediately fires pit_timer_fn() -> pit_do_work() -> kvm_set_irq() and the irq routes there are not set up properly. I don't see what we race with, though: create_pit_timer() checks ioapic_in_kernel() before creating the timer, so the whole KVM_CREATE_IRQCHIP must have finished and routing is kvm_setup_default_irq_routing() if the timer fires. The only other way to change the routing is through KVM_SET_GSI_ROUTING, but that doesn't have kvm->lock protection, so it shouldn't matter. Isn't the bug somewhere else? Thanks. > Signed-off-by: Steve Rutherford <srutherford@google.com> > Signed-off-by: Peter Shier <pshier@google.com> > Signed-off-by: Jim Mattson <jmattson@google.com> > --- > arch/x86/kvm/x86.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 506bd2b4b8bb7..188bc3fc907f1 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -4518,10 +4518,11 @@ long kvm_arch_vm_ioctl(struct file *filp, > r = -EFAULT; > if (copy_from_user(&u.ps, argp, sizeof u.ps)) > goto out; > + mutex_lock(&kvm->lock); > r = -ENXIO; > - if (!kvm->arch.vpit) > - goto out; > - r = kvm_vm_ioctl_set_pit(kvm, &u.ps); > + if (kvm->arch.vpit) > + r = kvm_vm_ioctl_set_pit(kvm, &u.ps); > + mutex_unlock(&kvm->lock); > break; > } > case KVM_GET_PIT2: { > @@ -4541,10 +4542,11 @@ long kvm_arch_vm_ioctl(struct file *filp, > r = -EFAULT; > if (copy_from_user(&u.ps2, argp, sizeof(u.ps2))) > goto out; > + mutex_lock(&kvm->lock); > r = -ENXIO; > - if (!kvm->arch.vpit) > - goto out; > - r = kvm_vm_ioctl_set_pit2(kvm, &u.ps2); > + if (kvm->arch.vpit) > + r = kvm_vm_ioctl_set_pit2(kvm, &u.ps2); > + mutex_unlock(&kvm->lock); > break; > } > case KVM_REINJECT_CONTROL: { > -- > 2.19.0.rc0.228.g281dcd1b4d0-goog
KVM_CREATE_IRQCHIP has been refactored since I wrote this patch, but the issue is still there. The IOAPIC is initialized (underneath kvm->lock) before the call to kvm_setup_default_irq_routing (which sets up the routing tables). If you are setting up the PIT without kvm->lock, there is a window where ioapic_in_kernel can return true, but default routing tables have not been configured. On Thu, Aug 30, 2018 at 9:35 AM Radim Krčmář <rkrcmar@redhat.com> wrote: > > 2018-08-29 17:54-0700, Peter Shier: > > From: Steve Rutherford <srutherford@google.com> > > > > Fixes a NULL pointer dereference, caused by the PIT firing an interrupt > > before the interrupt table has been initialized. > > > > SET_PIT2 can race with the creation of the IRQchip. In particular, > > if SET_PIT2 is called with a low PIT timer period (after the creation of > > the IOAPIC, but before the instantiation of the irq routes), the PIT can > > fire an interrupt at an uninitialized table. > > From what you describe, it seems that the problems is the timer > interrupt started in > > kvm_vm_ioctl_set_pit2() -> > kvm_pit_load_count() -> > pit_load_count() -> > create_pit_timer() -> > hrtimer_start() > > that immediately fires > > pit_timer_fn() -> > pit_do_work() -> > kvm_set_irq() > > and the irq routes there are not set up properly. > I don't see what we race with, though: > > create_pit_timer() checks ioapic_in_kernel() before creating the timer, > so the whole KVM_CREATE_IRQCHIP must have finished and routing is > kvm_setup_default_irq_routing() if the timer fires. > > The only other way to change the routing is through KVM_SET_GSI_ROUTING, > but that doesn't have kvm->lock protection, so it shouldn't matter. > > Isn't the bug somewhere else? > > Thanks. > > > Signed-off-by: Steve Rutherford <srutherford@google.com> > > Signed-off-by: Peter Shier <pshier@google.com> > > Signed-off-by: Jim Mattson <jmattson@google.com> > > --- > > arch/x86/kvm/x86.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 506bd2b4b8bb7..188bc3fc907f1 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -4518,10 +4518,11 @@ long kvm_arch_vm_ioctl(struct file *filp, > > r = -EFAULT; > > if (copy_from_user(&u.ps, argp, sizeof u.ps)) > > goto out; > > + mutex_lock(&kvm->lock); > > r = -ENXIO; > > - if (!kvm->arch.vpit) > > - goto out; > > - r = kvm_vm_ioctl_set_pit(kvm, &u.ps); > > + if (kvm->arch.vpit) > > + r = kvm_vm_ioctl_set_pit(kvm, &u.ps); > > + mutex_unlock(&kvm->lock); > > break; > > } > > case KVM_GET_PIT2: { > > @@ -4541,10 +4542,11 @@ long kvm_arch_vm_ioctl(struct file *filp, > > r = -EFAULT; > > if (copy_from_user(&u.ps2, argp, sizeof(u.ps2))) > > goto out; > > + mutex_lock(&kvm->lock); > > r = -ENXIO; > > - if (!kvm->arch.vpit) > > - goto out; > > - r = kvm_vm_ioctl_set_pit2(kvm, &u.ps2); > > + if (kvm->arch.vpit) > > + r = kvm_vm_ioctl_set_pit2(kvm, &u.ps2); > > + mutex_unlock(&kvm->lock); > > break; > > } > > case KVM_REINJECT_CONTROL: { > > -- > > 2.19.0.rc0.228.g281dcd1b4d0-goog
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 506bd2b4b8bb7..188bc3fc907f1 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -4518,10 +4518,11 @@ long kvm_arch_vm_ioctl(struct file *filp, r = -EFAULT; if (copy_from_user(&u.ps, argp, sizeof u.ps)) goto out; + mutex_lock(&kvm->lock); r = -ENXIO; - if (!kvm->arch.vpit) - goto out; - r = kvm_vm_ioctl_set_pit(kvm, &u.ps); + if (kvm->arch.vpit) + r = kvm_vm_ioctl_set_pit(kvm, &u.ps); + mutex_unlock(&kvm->lock); break; } case KVM_GET_PIT2: { @@ -4541,10 +4542,11 @@ long kvm_arch_vm_ioctl(struct file *filp, r = -EFAULT; if (copy_from_user(&u.ps2, argp, sizeof(u.ps2))) goto out; + mutex_lock(&kvm->lock); r = -ENXIO; - if (!kvm->arch.vpit) - goto out; - r = kvm_vm_ioctl_set_pit2(kvm, &u.ps2); + if (kvm->arch.vpit) + r = kvm_vm_ioctl_set_pit2(kvm, &u.ps2); + mutex_unlock(&kvm->lock); break; } case KVM_REINJECT_CONTROL: {