| Message ID | 20220601014328.1444271-1-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
| Series | [kernel,v2] KVM: Don't null dereference ops->destroy | expand |
On 6/1/22 03:43, Alexey Kardashevskiy wrote: > A KVM device cleanup happens in either of two callbacks: > 1) destroy() which is called when the VM is being destroyed; > 2) release() which is called when a device fd is closed. > > Most KVM devices use 1) but Book3s's interrupt controller KVM devices > (XICS, XIVE, XIVE-native) use 2) as they need to close and reopen during > the machine execution. The error handling in kvm_ioctl_create_device() > assumes destroy() is always defined which leads to NULL dereference as > discovered by Syzkaller. > > This adds a checks for destroy!=NULL and adds a missing release(). > > This is not changing kvm_destroy_devices() as devices with defined > release() should have been removed from the KVM devices list by then. > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Queued, thanks. Paolo > --- > Changes: > v2: > * do not touch kvm_destroy_devices > * call release() in the error path > --- > virt/kvm/kvm_main.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index f30bb8c16f26..e1c4bca95040 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -4299,8 +4299,11 @@ static int kvm_ioctl_create_device(struct kvm *kvm, > kvm_put_kvm_no_destroy(kvm); > mutex_lock(&kvm->lock); > list_del(&dev->vm_node); > + if (ops->release) > + ops->release(dev); > mutex_unlock(&kvm->lock); > - ops->destroy(dev); > + if (ops->destroy) > + ops->destroy(dev); > return ret; > } > Queued, thanks. Paolo
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index f30bb8c16f26..e1c4bca95040 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -4299,8 +4299,11 @@ static int kvm_ioctl_create_device(struct kvm *kvm, kvm_put_kvm_no_destroy(kvm); mutex_lock(&kvm->lock); list_del(&dev->vm_node); + if (ops->release) + ops->release(dev); mutex_unlock(&kvm->lock); - ops->destroy(dev); + if (ops->destroy) + ops->destroy(dev); return ret; }
A KVM device cleanup happens in either of two callbacks: 1) destroy() which is called when the VM is being destroyed; 2) release() which is called when a device fd is closed. Most KVM devices use 1) but Book3s's interrupt controller KVM devices (XICS, XIVE, XIVE-native) use 2) as they need to close and reopen during the machine execution. The error handling in kvm_ioctl_create_device() assumes destroy() is always defined which leads to NULL dereference as discovered by Syzkaller. This adds a checks for destroy!=NULL and adds a missing release(). This is not changing kvm_destroy_devices() as devices with defined release() should have been removed from the KVM devices list by then. Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- Changes: v2: * do not touch kvm_destroy_devices * call release() in the error path --- virt/kvm/kvm_main.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)