| Message ID | 02051e0a-09d8-49a2-917f-7c2f278a1ba1@moroto.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: fix an error code in kvm_create_vm() | expand |
On Thu, Jun 13, 2024, Dan Carpenter wrote: > This error path used to return -ENOMEM from the where r is initialized > at the top of the function. But a new "r = kvm_init_irq_routing(kvm);" > was introduced in the middle of the function so now the error code is > not set and it eventually leads to a NULL dereference. Set the error > code back to -ENOMEM. > > Fixes: fbe4a7e881d4 ("KVM: Setup empty IRQ routing when creating a VM") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > virt/kvm/kvm_main.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 07ec9b67a202..ea7e32d722c9 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -1212,8 +1212,10 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname) > for (i = 0; i < KVM_NR_BUSES; i++) { > rcu_assign_pointer(kvm->buses[i], > kzalloc(sizeof(struct kvm_io_bus), GFP_KERNEL_ACCOUNT)); > - if (!kvm->buses[i]) > + if (!kvm->buses[i]) { > + r = -ENOMEM; > goto out_err_no_arch_destroy_vm; > + } > } Drat. Any objection to tweaking this slightly to guard against similar bugs in the future? If not, I'll apply+push the below. Thanks! -- From: Dan Carpenter <dan.carpenter@linaro.org> Date: Thu, 13 Jun 2024 17:33:16 +0300 Subject: [PATCH] KVM: fix an error code in kvm_create_vm() This error path used to return -ENOMEM from the where r is initialized at the top of the function. But a new "r = kvm_init_irq_routing(kvm);" was introduced in the middle of the function so now the error code is not set and it eventually leads to a NULL dereference. Set the error code back to -ENOMEM. Opportunistically tweak the logic to pre-set "r = -ENOMEM" immediately before the flows that can fail due to memory allocation failure to make it less likely that the bug recurs in the future. Fixes: fbe4a7e881d4 ("KVM: Setup empty IRQ routing when creating a VM") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Link: https://lore.kernel.org/r/02051e0a-09d8-49a2-917f-7c2f278a1ba1@moroto.mountain [sean: tweak all of the "r = -ENOMEM" sites] Signed-off-by: Sean Christopherson <seanjc@google.com> --- virt/kvm/kvm_main.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index b60186b9c1d3..436ca41f61e5 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1143,8 +1143,7 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname) { struct kvm *kvm = kvm_arch_alloc_vm(); struct kvm_memslots *slots; - int r = -ENOMEM; - int i, j; + int r, i, j; if (!kvm) return ERR_PTR(-ENOMEM); @@ -1181,6 +1180,7 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname) snprintf(kvm->stats_id, sizeof(kvm->stats_id), "kvm-%d", task_pid_nr(current)); + r = -ENOMEM; if (init_srcu_struct(&kvm->srcu)) goto out_err_no_srcu; if (init_srcu_struct(&kvm->irq_srcu)) @@ -1209,6 +1209,7 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname) rcu_assign_pointer(kvm->memslots[i], &kvm->__memslots[i][0]); } + r = -ENOMEM; for (i = 0; i < KVM_NR_BUSES; i++) { rcu_assign_pointer(kvm->buses[i], kzalloc(sizeof(struct kvm_io_bus), GFP_KERNEL_ACCOUNT)); base-commit: 3dee3b187499b317a6587e2b8e9bf3d5050e5288 --
On Thu, Jun 13, 2024 at 04:50:48PM -0700, Sean Christopherson wrote: > On Thu, Jun 13, 2024, Dan Carpenter wrote: > > This error path used to return -ENOMEM from the where r is initialized > > at the top of the function. But a new "r = kvm_init_irq_routing(kvm);" > > was introduced in the middle of the function so now the error code is > > not set and it eventually leads to a NULL dereference. Set the error > > code back to -ENOMEM. > > > > Fixes: fbe4a7e881d4 ("KVM: Setup empty IRQ routing when creating a VM") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > virt/kvm/kvm_main.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 07ec9b67a202..ea7e32d722c9 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -1212,8 +1212,10 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname) > > for (i = 0; i < KVM_NR_BUSES; i++) { > > rcu_assign_pointer(kvm->buses[i], > > kzalloc(sizeof(struct kvm_io_bus), GFP_KERNEL_ACCOUNT)); > > - if (!kvm->buses[i]) > > + if (!kvm->buses[i]) { > > + r = -ENOMEM; > > goto out_err_no_arch_destroy_vm; > > + } > > } > > Drat. Any objection to tweaking this slightly to guard against similar bugs in > the future? If not, I'll apply+push the below. No objections from me. :) regards, dan carpenter
> This error path used to return -ENOMEM from the where r is initialized
…
place where the local variable “r” …?
Regards,
Markus
On Thu, 13 Jun 2024 17:33:16 +0300, Dan Carpenter wrote: > This error path used to return -ENOMEM from the where r is initialized > at the top of the function. But a new "r = kvm_init_irq_routing(kvm);" > was introduced in the middle of the function so now the error code is > not set and it eventually leads to a NULL dereference. Set the error > code back to -ENOMEM. > > > [...] Applied to kvm-x86 generic, with the fixup I suggested and a massaged changelog to address Markus' feedback. Thanks again! [1/1] KVM: fix an error code in kvm_create_vm() https://github.com/kvm-x86/linux/commit/5c1f50ab7fcb -- https://github.com/kvm-x86/linux/tree/next
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 07ec9b67a202..ea7e32d722c9 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1212,8 +1212,10 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname) for (i = 0; i < KVM_NR_BUSES; i++) { rcu_assign_pointer(kvm->buses[i], kzalloc(sizeof(struct kvm_io_bus), GFP_KERNEL_ACCOUNT)); - if (!kvm->buses[i]) + if (!kvm->buses[i]) { + r = -ENOMEM; goto out_err_no_arch_destroy_vm; + } } r = kvm_arch_init_vm(kvm, type);
This error path used to return -ENOMEM from the where r is initialized at the top of the function. But a new "r = kvm_init_irq_routing(kvm);" was introduced in the middle of the function so now the error code is not set and it eventually leads to a NULL dereference. Set the error code back to -ENOMEM. Fixes: fbe4a7e881d4 ("KVM: Setup empty IRQ routing when creating a VM") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- virt/kvm/kvm_main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)