* [PATCH v2] KVM: fix memory leak in kvm_io_bus_unregister_dev() @ 2020-09-02 22:57 Rustam Kovhaev 2020-09-02 23:34 ` Gustavo A. R. Silva 0 siblings, 1 reply; 5+ messages in thread From: Rustam Kovhaev @ 2020-09-02 22:57 UTC (permalink / raw) To: pbonzini, vkuznets, kvm; +Cc: linux-kernel, gregkh, Rustam Kovhaev when kmalloc() fails in kvm_io_bus_unregister_dev(), before removing the bus, we should iterate over all other devices linked to it and call kvm_iodevice_destructor() for them Reported-and-tested-by: syzbot+f196caa45793d6374707@syzkaller.appspotmail.com Link: https://syzkaller.appspot.com/bug?extid=f196caa45793d6374707 Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- v2: - remove redundant whitespace - remove goto statement and use if/else --- virt/kvm/kvm_main.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 67cd0b88a6b6..cf88233b819a 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -4332,7 +4332,7 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, struct kvm_io_device *dev) { - int i; + int i, j; struct kvm_io_bus *new_bus, *bus; bus = kvm_get_bus(kvm, bus_idx); @@ -4349,17 +4349,20 @@ void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, new_bus = kmalloc(struct_size(bus, range, bus->dev_count - 1), GFP_KERNEL_ACCOUNT); - if (!new_bus) { + if (new_bus) { + memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); + new_bus->dev_count--; + memcpy(new_bus->range + i, bus->range + i + 1, + (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); + } else { pr_err("kvm: failed to shrink bus, removing it completely\n"); - goto broken; + for (j = 0; j < bus->dev_count; j++) { + if (j == i) + continue; + kvm_iodevice_destructor(bus->range[j].dev); + } } - memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); - new_bus->dev_count--; - memcpy(new_bus->range + i, bus->range + i + 1, - (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); - -broken: rcu_assign_pointer(kvm->buses[bus_idx], new_bus); synchronize_srcu_expedited(&kvm->srcu); kfree(bus); -- 2.28.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] KVM: fix memory leak in kvm_io_bus_unregister_dev() 2020-09-02 22:57 [PATCH v2] KVM: fix memory leak in kvm_io_bus_unregister_dev() Rustam Kovhaev @ 2020-09-02 23:34 ` Gustavo A. R. Silva 2020-09-03 17:22 ` Rustam Kovhaev 0 siblings, 1 reply; 5+ messages in thread From: Gustavo A. R. Silva @ 2020-09-02 23:34 UTC (permalink / raw) To: Rustam Kovhaev, pbonzini, vkuznets, kvm; +Cc: linux-kernel, gregkh Hi, On 9/2/20 17:57, Rustam Kovhaev wrote: > when kmalloc() fails in kvm_io_bus_unregister_dev(), before removing > the bus, we should iterate over all other devices linked to it and call > kvm_iodevice_destructor() for them > > Reported-and-tested-by: syzbot+f196caa45793d6374707@syzkaller.appspotmail.com > Link: https://syzkaller.appspot.com/bug?extid=f196caa45793d6374707 > Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com> > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> I think it's worthwhile to add a Fixes tag for this, too. Please, see more comments below... > --- > v2: > - remove redundant whitespace > - remove goto statement and use if/else > --- > virt/kvm/kvm_main.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 67cd0b88a6b6..cf88233b819a 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -4332,7 +4332,7 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, > void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > struct kvm_io_device *dev) > { > - int i; > + int i, j; > struct kvm_io_bus *new_bus, *bus; > > bus = kvm_get_bus(kvm, bus_idx); > @@ -4349,17 +4349,20 @@ void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > > new_bus = kmalloc(struct_size(bus, range, bus->dev_count - 1), > GFP_KERNEL_ACCOUNT); > - if (!new_bus) { > + if (new_bus) { > + memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); ^^^ It seems that you can use struct_size() here (see the allocation code above)... > + new_bus->dev_count--; > + memcpy(new_bus->range + i, bus->range + i + 1, > + (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); ^^^ ...and, if possible, you can also use flex_array_size() here. Thanks -- Gustavo > + } else { > pr_err("kvm: failed to shrink bus, removing it completely\n"); > - goto broken; > + for (j = 0; j < bus->dev_count; j++) { > + if (j == i) > + continue; > + kvm_iodevice_destructor(bus->range[j].dev); > + } > } > > - memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); > - new_bus->dev_count--; > - memcpy(new_bus->range + i, bus->range + i + 1, > - (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); > - > -broken: > rcu_assign_pointer(kvm->buses[bus_idx], new_bus); > synchronize_srcu_expedited(&kvm->srcu); > kfree(bus); > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] KVM: fix memory leak in kvm_io_bus_unregister_dev() 2020-09-02 23:34 ` Gustavo A. R. Silva @ 2020-09-03 17:22 ` Rustam Kovhaev 2020-09-04 12:04 ` Vitaly Kuznetsov 0 siblings, 1 reply; 5+ messages in thread From: Rustam Kovhaev @ 2020-09-03 17:22 UTC (permalink / raw) To: Gustavo A. R. Silva; +Cc: pbonzini, vkuznets, kvm, linux-kernel, gregkh On Wed, Sep 02, 2020 at 06:34:11PM -0500, Gustavo A. R. Silva wrote: > Hi, > > On 9/2/20 17:57, Rustam Kovhaev wrote: > > when kmalloc() fails in kvm_io_bus_unregister_dev(), before removing > > the bus, we should iterate over all other devices linked to it and call > > kvm_iodevice_destructor() for them > > > > Reported-and-tested-by: syzbot+f196caa45793d6374707@syzkaller.appspotmail.com > > Link: https://syzkaller.appspot.com/bug?extid=f196caa45793d6374707 > > Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com> > > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > I think it's worthwhile to add a Fixes tag for this, too. > > Please, see more comments below... > > > --- > > v2: > > - remove redundant whitespace > > - remove goto statement and use if/else > > --- > > virt/kvm/kvm_main.c | 21 ++++++++++++--------- > > 1 file changed, 12 insertions(+), 9 deletions(-) > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 67cd0b88a6b6..cf88233b819a 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -4332,7 +4332,7 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, > > void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > > struct kvm_io_device *dev) > > { > > - int i; > > + int i, j; > > struct kvm_io_bus *new_bus, *bus; > > > > bus = kvm_get_bus(kvm, bus_idx); > > @@ -4349,17 +4349,20 @@ void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > > > > new_bus = kmalloc(struct_size(bus, range, bus->dev_count - 1), > > GFP_KERNEL_ACCOUNT); > > - if (!new_bus) { > > + if (new_bus) { > > + memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); > > ^^^ > It seems that you can use struct_size() here (see the allocation code above)... > > > + new_bus->dev_count--; > > + memcpy(new_bus->range + i, bus->range + i + 1, > > + (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); > > ^^^ > ...and, if possible, you can also use flex_array_size() here. > > Thanks > -- > Gustavo > > > + } else { > > pr_err("kvm: failed to shrink bus, removing it completely\n"); > > - goto broken; > > + for (j = 0; j < bus->dev_count; j++) { > > + if (j == i) > > + continue; > > + kvm_iodevice_destructor(bus->range[j].dev); > > + } > > } > > > > - memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); > > - new_bus->dev_count--; > > - memcpy(new_bus->range + i, bus->range + i + 1, > > - (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); > > - > > -broken: > > rcu_assign_pointer(kvm->buses[bus_idx], new_bus); > > synchronize_srcu_expedited(&kvm->srcu); > > kfree(bus); > > hi Gustavo, thank you for the review, i'll send the new patch. Vitaly, i think i will need to drop your "Reviewed-by", because there is going to be a bit more changes ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] KVM: fix memory leak in kvm_io_bus_unregister_dev() 2020-09-03 17:22 ` Rustam Kovhaev @ 2020-09-04 12:04 ` Vitaly Kuznetsov 2020-09-04 14:37 ` Gustavo A. R. Silva 0 siblings, 1 reply; 5+ messages in thread From: Vitaly Kuznetsov @ 2020-09-04 12:04 UTC (permalink / raw) To: Rustam Kovhaev, Gustavo A. R. Silva; +Cc: pbonzini, kvm, linux-kernel, gregkh Rustam Kovhaev <rkovhaev@gmail.com> writes: > On Wed, Sep 02, 2020 at 06:34:11PM -0500, Gustavo A. R. Silva wrote: >> Hi, >> >> On 9/2/20 17:57, Rustam Kovhaev wrote: >> > when kmalloc() fails in kvm_io_bus_unregister_dev(), before removing >> > the bus, we should iterate over all other devices linked to it and call >> > kvm_iodevice_destructor() for them >> > >> > Reported-and-tested-by: syzbot+f196caa45793d6374707@syzkaller.appspotmail.com >> > Link: https://syzkaller.appspot.com/bug?extid=f196caa45793d6374707 >> > Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com> >> > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> >> I think it's worthwhile to add a Fixes tag for this, too. >> >> Please, see more comments below... >> >> > --- >> > v2: >> > - remove redundant whitespace >> > - remove goto statement and use if/else >> > --- >> > virt/kvm/kvm_main.c | 21 ++++++++++++--------- >> > 1 file changed, 12 insertions(+), 9 deletions(-) >> > >> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >> > index 67cd0b88a6b6..cf88233b819a 100644 >> > --- a/virt/kvm/kvm_main.c >> > +++ b/virt/kvm/kvm_main.c >> > @@ -4332,7 +4332,7 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, >> > void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, >> > struct kvm_io_device *dev) >> > { >> > - int i; >> > + int i, j; >> > struct kvm_io_bus *new_bus, *bus; >> > >> > bus = kvm_get_bus(kvm, bus_idx); >> > @@ -4349,17 +4349,20 @@ void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, >> > >> > new_bus = kmalloc(struct_size(bus, range, bus->dev_count - 1), >> > GFP_KERNEL_ACCOUNT); >> > - if (!new_bus) { >> > + if (new_bus) { >> > + memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); >> >> ^^^ >> It seems that you can use struct_size() here (see the allocation code above)... >> >> > + new_bus->dev_count--; >> > + memcpy(new_bus->range + i, bus->range + i + 1, >> > + (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); >> >> ^^^ >> ...and, if possible, you can also use flex_array_size() here. >> >> Thanks >> -- >> Gustavo >> >> > + } else { >> > pr_err("kvm: failed to shrink bus, removing it completely\n"); >> > - goto broken; >> > + for (j = 0; j < bus->dev_count; j++) { >> > + if (j == i) >> > + continue; >> > + kvm_iodevice_destructor(bus->range[j].dev); >> > + } >> > } >> > >> > - memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); >> > - new_bus->dev_count--; >> > - memcpy(new_bus->range + i, bus->range + i + 1, >> > - (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); >> > - >> > -broken: >> > rcu_assign_pointer(kvm->buses[bus_idx], new_bus); >> > synchronize_srcu_expedited(&kvm->srcu); >> > kfree(bus); >> > > > hi Gustavo, thank you for the review, i'll send the new patch. > Vitaly, i think i will need to drop your "Reviewed-by", because there is > going to be a bit more changes > Personally, I'd prefer to make struct_size()/flex_array_size() a separate preparatory patch so the real fix is small but I don't have a strong opinion. I'll take look at v3 so feel free to drop R-b if you decide to make a combined patch and feel free to keep it if you make the preparatory changes separate :-) -- Vitaly ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] KVM: fix memory leak in kvm_io_bus_unregister_dev() 2020-09-04 12:04 ` Vitaly Kuznetsov @ 2020-09-04 14:37 ` Gustavo A. R. Silva 0 siblings, 0 replies; 5+ messages in thread From: Gustavo A. R. Silva @ 2020-09-04 14:37 UTC (permalink / raw) To: Vitaly Kuznetsov Cc: Rustam Kovhaev, Gustavo A. R. Silva, pbonzini, kvm, linux-kernel, gregkh On Fri, Sep 04, 2020 at 02:04:23PM +0200, Vitaly Kuznetsov wrote: > Rustam Kovhaev <rkovhaev@gmail.com> writes: > > > On Wed, Sep 02, 2020 at 06:34:11PM -0500, Gustavo A. R. Silva wrote: > >> Hi, > >> > >> On 9/2/20 17:57, Rustam Kovhaev wrote: > >> > when kmalloc() fails in kvm_io_bus_unregister_dev(), before removing > >> > the bus, we should iterate over all other devices linked to it and call > >> > kvm_iodevice_destructor() for them > >> > > >> > Reported-and-tested-by: syzbot+f196caa45793d6374707@syzkaller.appspotmail.com > >> > Link: https://syzkaller.appspot.com/bug?extid=f196caa45793d6374707 > >> > Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.com> > >> > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> > >> > >> I think it's worthwhile to add a Fixes tag for this, too. > >> > >> Please, see more comments below... [..] > > > > hi Gustavo, thank you for the review, i'll send the new patch. > > Vitaly, i think i will need to drop your "Reviewed-by", because there is > > going to be a bit more changes > > > > Personally, I'd prefer to make struct_size()/flex_array_size() a > separate preparatory patch so the real fix is small but I don't have a > strong opinion. I'll take look at v3 so feel free to drop R-b if you > decide to make a combined patch and feel free to keep it if you make the > preparatory changes separate :-) > I agree. A two-patch series is much better in this case. Rustam - please add a Fixes tag to the first patch and see if it can be applied to -stable. If so, you should Cc stable@vger.kernel.org, too. Thanks -- Gustavo ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-04 14:31 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-09-02 22:57 [PATCH v2] KVM: fix memory leak in kvm_io_bus_unregister_dev() Rustam Kovhaev 2020-09-02 23:34 ` Gustavo A. R. Silva 2020-09-03 17:22 ` Rustam Kovhaev 2020-09-04 12:04 ` Vitaly Kuznetsov 2020-09-04 14:37 ` Gustavo A. R. Silva
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.