| Message ID | 20250418042421.1393-1-chenyufeng@iie.ac.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kvm: potential NULL pointer dereference in kvm_vm_ioctl_create_vcpu() | expand |
On 4/18/25 06:24, Chen Yufeng wrote: > A patch similar to commit 5593473a1e6c ("KVM: avoid NULL pointer > dereference in kvm_dirty_ring_push"). > > If kvm_get_vcpu_by_id() or xa_insert() failed, kvm_vm_ioctl_create_vcpu() > will call kvm_dirty_ring_free(), freeing ring->dirty_gfns and setting it > to NULL. Then, it calls kvm_arch_vcpu_destroy(), which may call > kvm_dirty_ring_push() in specific call stack under the same conditions as > previous commit said. Finally, kvm_dirty_ring_push() will use > ring->dirty_gfns, leading to a NULL pointer dereference. > > Signed-off-by: Chen Yufeng <chenyufeng@iie.ac.cn> > --- > virt/kvm/kvm_main.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index e85b33a92624..3c97e598d866 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -4178,7 +4178,9 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id) > xa_erase(&kvm->vcpu_array, vcpu->vcpu_idx); > unlock_vcpu_destroy: > mutex_unlock(&kvm->lock); > + kvm_arch_vcpu_destroy(vcpu); > kvm_dirty_ring_free(&vcpu->dirty_ring); > + goto vcpu_free_run_page; Makes sense, but the goto is not needed. Just move kvm_dirty_ring_free() above "vcpu_free_run_page:", in the same style as kvm_vcpu_destroy(). Paolo > arch_vcpu_destroy: > kvm_arch_vcpu_destroy(vcpu); > vcpu_free_run_page:
> On 4/18/25 06:24, Chen Yufeng wrote: > > A patch similar to commit 5593473a1e6c ("KVM: avoid NULL pointer > > dereference in kvm_dirty_ring_push"). > > > > If kvm_get_vcpu_by_id() or xa_insert() failed, kvm_vm_ioctl_create_vcpu() > > will call kvm_dirty_ring_free(), freeing ring->dirty_gfns and setting it > > to NULL. Then, it calls kvm_arch_vcpu_destroy(), which may call > > kvm_dirty_ring_push() in specific call stack under the same conditions as > > previous commit said. Finally, kvm_dirty_ring_push() will use > > ring->dirty_gfns, leading to a NULL pointer dereference. > > > > Signed-off-by: Chen Yufeng <chenyufeng@iie.ac.cn> > > --- > > virt/kvm/kvm_main.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index e85b33a92624..3c97e598d866 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -4178,7 +4178,9 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id) > > xa_erase(&kvm->vcpu_array, vcpu->vcpu_idx); > > unlock_vcpu_destroy: > > mutex_unlock(&kvm->lock); > > + kvm_arch_vcpu_destroy(vcpu); > > kvm_dirty_ring_free(&vcpu->dirty_ring); > > + goto vcpu_free_run_page; > > Makes sense, but the goto is not needed. Just move > kvm_dirty_ring_free() above "vcpu_free_run_page:", in the same style as > kvm_vcpu_destroy(). > > Paolo You're right, and I'll take your advice and send v2 patch. > > arch_vcpu_destroy: > > kvm_arch_vcpu_destroy(vcpu); > > vcpu_free_run_page: -- Thanks, Chen Yufeng
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index e85b33a92624..3c97e598d866 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -4178,7 +4178,9 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id) xa_erase(&kvm->vcpu_array, vcpu->vcpu_idx); unlock_vcpu_destroy: mutex_unlock(&kvm->lock); + kvm_arch_vcpu_destroy(vcpu); kvm_dirty_ring_free(&vcpu->dirty_ring); + goto vcpu_free_run_page; arch_vcpu_destroy: kvm_arch_vcpu_destroy(vcpu); vcpu_free_run_page:
A patch similar to commit 5593473a1e6c ("KVM: avoid NULL pointer dereference in kvm_dirty_ring_push"). If kvm_get_vcpu_by_id() or xa_insert() failed, kvm_vm_ioctl_create_vcpu() will call kvm_dirty_ring_free(), freeing ring->dirty_gfns and setting it to NULL. Then, it calls kvm_arch_vcpu_destroy(), which may call kvm_dirty_ring_push() in specific call stack under the same conditions as previous commit said. Finally, kvm_dirty_ring_push() will use ring->dirty_gfns, leading to a NULL pointer dereference. Signed-off-by: Chen Yufeng <chenyufeng@iie.ac.cn> --- virt/kvm/kvm_main.c | 2 ++ 1 file changed, 2 insertions(+)