| Message ID | 20230913074657.523530-1-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vhost: Add a defensive check in vhost_commit against wrong deallocation | expand |
On Wed, Sep 13, 2023 at 3:47 PM Eric Auger <eric.auger@redhat.com> wrote: > > In vhost_commit(), it may happen that dev->mem_sections and > dev->tmp_sections are equal, in which case, unconditionally > freeing old_sections at the end of the function will also free > dev->mem_sections used on subsequent call leading to a segmentation > fault. > > Check this situation before deallocating memory. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Fixes: c44317efecb2 ("vhost: Build temporary section list and deref > after commit") > CC: QEMU Stable <qemu-stable@nongnu.org> > > --- > > This SIGSEV condition can be reproduced with > https://lore.kernel.org/all/20230904080451.424731-1-eric.auger@redhat.com/#r > This is most probably happening in a situation where the memory API is > used in a wrong manner but well. Any chance to move this to the memory API or we may end up with things like this in another listener? Thanks > --- > hw/virtio/vhost.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index e2f6ffb446..c02c599ef0 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -545,6 +545,11 @@ static void vhost_commit(MemoryListener *listener) > dev->mem_sections = dev->tmp_sections; > dev->n_mem_sections = dev->n_tmp_sections; > > + if (old_sections == dev->mem_sections) { > + assert(n_old_sections == dev->n_mem_sections); > + return; > + } > + > if (dev->n_mem_sections != n_old_sections) { > changed = true; > } else { > -- > 2.41.0 >
Hi Jason, On 9/14/23 05:46, Jason Wang wrote: > On Wed, Sep 13, 2023 at 3:47 PM Eric Auger <eric.auger@redhat.com> wrote: >> In vhost_commit(), it may happen that dev->mem_sections and >> dev->tmp_sections are equal, in which case, unconditionally >> freeing old_sections at the end of the function will also free >> dev->mem_sections used on subsequent call leading to a segmentation >> fault. >> >> Check this situation before deallocating memory. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Fixes: c44317efecb2 ("vhost: Build temporary section list and deref >> after commit") >> CC: QEMU Stable <qemu-stable@nongnu.org> >> >> --- >> >> This SIGSEV condition can be reproduced with >> https://lore.kernel.org/all/20230904080451.424731-1-eric.auger@redhat.com/#r >> This is most probably happening in a situation where the memory API is >> used in a wrong manner but well. > Any chance to move this to the memory API or we may end up with things > like this in another listener? I am not very familiar with the vhost code but aren't those tmp_sections and mem_sections really specific to the vhost device? I am not sure we can easily generalize. Thanks Eric > > Thanks > >> --- >> hw/virtio/vhost.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >> index e2f6ffb446..c02c599ef0 100644 >> --- a/hw/virtio/vhost.c >> +++ b/hw/virtio/vhost.c >> @@ -545,6 +545,11 @@ static void vhost_commit(MemoryListener *listener) >> dev->mem_sections = dev->tmp_sections; >> dev->n_mem_sections = dev->n_tmp_sections; >> >> + if (old_sections == dev->mem_sections) { >> + assert(n_old_sections == dev->n_mem_sections); >> + return; >> + } >> + >> if (dev->n_mem_sections != n_old_sections) { >> changed = true; >> } else { >> -- >> 2.41.0 >>
On Wed, Sep 13, 2023 at 09:46:57AM +0200, Eric Auger wrote: > In vhost_commit(), it may happen that dev->mem_sections and > dev->tmp_sections are equal, Could you please explain a bit more how this can happen? I don't see how. > in which case, unconditionally > freeing old_sections at the end of the function will also free > dev->mem_sections used on subsequent call leading to a segmentation > fault. > > Check this situation before deallocating memory. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Fixes: c44317efecb2 ("vhost: Build temporary section list and deref > after commit") > CC: QEMU Stable <qemu-stable@nongnu.org> > > --- > > This SIGSEV condition can be reproduced with > https://lore.kernel.org/all/20230904080451.424731-1-eric.auger@redhat.com/#r > This is most probably happening in a situation where the memory API is > used in a wrong manner but well. sounds like misusing the memory API can lead to all kind of mischief. > --- > hw/virtio/vhost.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index e2f6ffb446..c02c599ef0 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -545,6 +545,11 @@ static void vhost_commit(MemoryListener *listener) > dev->mem_sections = dev->tmp_sections; > dev->n_mem_sections = dev->n_tmp_sections; > > + if (old_sections == dev->mem_sections) { > + assert(n_old_sections == dev->n_mem_sections); > + return; > + } > + > if (dev->n_mem_sections != n_old_sections) { > changed = true; > } else { > -- > 2.41.0
Hi Michael, On 10/3/23 14:43, Michael S. Tsirkin wrote: > On Wed, Sep 13, 2023 at 09:46:57AM +0200, Eric Auger wrote: >> In vhost_commit(), it may happen that dev->mem_sections and >> dev->tmp_sections are equal, > Could you please explain a bit more how this can happen? > I don't see how. > >> in which case, unconditionally >> freeing old_sections at the end of the function will also free >> dev->mem_sections used on subsequent call leading to a segmentation >> fault. >> >> Check this situation before deallocating memory. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Fixes: c44317efecb2 ("vhost: Build temporary section list and deref >> after commit") >> CC: QEMU Stable <qemu-stable@nongnu.org> >> >> --- >> >> This SIGSEV condition can be reproduced with >> https://lore.kernel.org/all/20230904080451.424731-1-eric.auger@redhat.com/#r >> This is most probably happening in a situation where the memory API is >> used in a wrong manner but well. > sounds like misusing the memory API can lead to all kind of mischief. This happened in a situation where I resized an [IOMMU] MR within the VFIO vfio_listener_region_add leading to recursive calls ot region_add callbacks. The issue is it was not straightforward to find the link with vhost. Thanks Eric > >> --- >> hw/virtio/vhost.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >> index e2f6ffb446..c02c599ef0 100644 >> --- a/hw/virtio/vhost.c >> +++ b/hw/virtio/vhost.c >> @@ -545,6 +545,11 @@ static void vhost_commit(MemoryListener *listener) >> dev->mem_sections = dev->tmp_sections; >> dev->n_mem_sections = dev->n_tmp_sections; >> >> + if (old_sections == dev->mem_sections) { >> + assert(n_old_sections == dev->n_mem_sections); >> + return; >> + } >> + >> if (dev->n_mem_sections != n_old_sections) { >> changed = true; >> } else { >> -- >> 2.41.0
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index e2f6ffb446..c02c599ef0 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -545,6 +545,11 @@ static void vhost_commit(MemoryListener *listener) dev->mem_sections = dev->tmp_sections; dev->n_mem_sections = dev->n_tmp_sections; + if (old_sections == dev->mem_sections) { + assert(n_old_sections == dev->n_mem_sections); + return; + } + if (dev->n_mem_sections != n_old_sections) { changed = true; } else {
In vhost_commit(), it may happen that dev->mem_sections and dev->tmp_sections are equal, in which case, unconditionally freeing old_sections at the end of the function will also free dev->mem_sections used on subsequent call leading to a segmentation fault. Check this situation before deallocating memory. Signed-off-by: Eric Auger <eric.auger@redhat.com> Fixes: c44317efecb2 ("vhost: Build temporary section list and deref after commit") CC: QEMU Stable <qemu-stable@nongnu.org> --- This SIGSEV condition can be reproduced with https://lore.kernel.org/all/20230904080451.424731-1-eric.auger@redhat.com/#r This is most probably happening in a situation where the memory API is used in a wrong manner but well. --- hw/virtio/vhost.c | 5 +++++ 1 file changed, 5 insertions(+)