From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Igor spotted that there's a race, where a region that's unref'd
in a _del callback might be free'd before the set_mem_table call in
the _commit callback, and thus the vhost might end up using free memory.
Fix this by building a complete temporary sections list, ref'ing every
section (during add and nop) and then unref'ing the whole list right
at the end of commit.
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
hw/virtio/vhost.c | 73 ++++++++++++++++++++++++++++++-----------------
include/hw/virtio/vhost.h | 2 ++
2 files changed, 49 insertions(+), 26 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index e4290ce93d..610bfe6945 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -621,6 +621,8 @@ static void vhost_begin(MemoryListener *listener)
memory_listener);
dev->mem_changed_end_addr = 0;
dev->mem_changed_start_addr = -1;
+ dev->tmp_sections = NULL;
+ dev->n_tmp_sections = 0;
}
static void vhost_commit(MemoryListener *listener)
@@ -629,17 +631,25 @@ static void vhost_commit(MemoryListener *listener)
memory_listener);
hwaddr start_addr = 0;
ram_addr_t size = 0;
+ MemoryRegionSection *old_sections;
+ int n_old_sections;
+
uint64_t log_size;
int r;
+ old_sections = dev->mem_sections;
+ n_old_sections = dev->n_mem_sections;
+ dev->mem_sections = dev->tmp_sections;
+ dev->n_mem_sections = dev->n_tmp_sections;
+
if (!dev->memory_changed) {
- return;
+ goto out;
}
if (!dev->started) {
- return;
+ goto out;
}
if (dev->mem_changed_start_addr > dev->mem_changed_end_addr) {
- return;
+ goto out;
}
if (dev->started) {
@@ -656,7 +666,7 @@ static void vhost_commit(MemoryListener *listener)
VHOST_OPS_DEBUG("vhost_set_mem_table failed");
}
dev->memory_changed = false;
- return;
+ goto out;
}
log_size = vhost_get_log_size(dev);
/* We allocate an extra 4K bytes to log,
@@ -675,6 +685,27 @@ static void vhost_commit(MemoryListener *listener)
vhost_dev_log_resize(dev, log_size);
}
dev->memory_changed = false;
+
+out:
+ /* Deref the old list of sections, this must happen _after_ the
+ * vhost_set_mem_table to ensure the client isn't still using the
+ * section we're about to unref.
+ */
+ while (n_old_sections--) {
+ memory_region_unref(old_sections[n_old_sections].mr);
+ }
+ g_free(old_sections);
+ return;
+}
+
+static void vhost_add_section(struct vhost_dev *dev,
+ MemoryRegionSection *section)
+{
+ ++dev->n_tmp_sections;
+ dev->tmp_sections = g_renew(MemoryRegionSection, dev->tmp_sections,
+ dev->n_tmp_sections);
+ dev->tmp_sections[dev->n_tmp_sections - 1] = *section;
+ memory_region_ref(section->mr);
}
static void vhost_region_add(MemoryListener *listener,
@@ -687,36 +718,31 @@ static void vhost_region_add(MemoryListener *listener,
return;
}
- ++dev->n_mem_sections;
- dev->mem_sections = g_renew(MemoryRegionSection, dev->mem_sections,
- dev->n_mem_sections);
- dev->mem_sections[dev->n_mem_sections - 1] = *section;
- memory_region_ref(section->mr);
+ vhost_add_section(dev, section);
vhost_set_memory(listener, section, true);
}
-static void vhost_region_del(MemoryListener *listener,
+static void vhost_region_nop(MemoryListener *listener,
MemoryRegionSection *section)
{
struct vhost_dev *dev = container_of(listener, struct vhost_dev,
memory_listener);
- int i;
if (!vhost_section(section)) {
return;
}
- vhost_set_memory(listener, section, false);
- memory_region_unref(section->mr);
- for (i = 0; i < dev->n_mem_sections; ++i) {
- if (dev->mem_sections[i].offset_within_address_space
- == section->offset_within_address_space) {
- --dev->n_mem_sections;
- memmove(&dev->mem_sections[i], &dev->mem_sections[i+1],
- (dev->n_mem_sections - i) * sizeof(*dev->mem_sections));
- break;
- }
+ vhost_add_section(dev, section);
+}
+
+static void vhost_region_del(MemoryListener *listener,
+ MemoryRegionSection *section)
+{
+ if (!vhost_section(section)) {
+ return;
}
+
+ vhost_set_memory(listener, section, false);
}
static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
@@ -783,11 +809,6 @@ static void vhost_iommu_region_del(MemoryListener *listener,
}
}
-static void vhost_region_nop(MemoryListener *listener,
- MemoryRegionSection *section)
-{
-}
-
static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
struct vhost_virtqueue *vq,
unsigned idx, bool enable_log)
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 467dc7794b..21f3022499 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -54,6 +54,8 @@ struct vhost_dev {
struct vhost_memory *mem;
int n_mem_sections;
MemoryRegionSection *mem_sections;
+ int n_tmp_sections;
+ MemoryRegionSection *tmp_sections;
struct vhost_virtqueue *vqs;
int nvqs;
/* the first virtqueue which would be used by this vhost dev */
--
2.14.3
On Mon, 18 Dec 2017 20:13:34 +0000
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Igor spotted that there's a race, where a region that's unref'd
> in a _del callback might be free'd before the set_mem_table call in
> the _commit callback, and thus the vhost might end up using free memory.
>
> Fix this by building a complete temporary sections list, ref'ing every
> section (during add and nop) and then unref'ing the whole list right
> at the end of commit.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
As Michael've suggested it should go into stable (CCed)
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/virtio/vhost.c | 73 ++++++++++++++++++++++++++++++-----------------
> include/hw/virtio/vhost.h | 2 ++
> 2 files changed, 49 insertions(+), 26 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index e4290ce93d..610bfe6945 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -621,6 +621,8 @@ static void vhost_begin(MemoryListener *listener)
> memory_listener);
> dev->mem_changed_end_addr = 0;
> dev->mem_changed_start_addr = -1;
> + dev->tmp_sections = NULL;
> + dev->n_tmp_sections = 0;
> }
>
> static void vhost_commit(MemoryListener *listener)
> @@ -629,17 +631,25 @@ static void vhost_commit(MemoryListener *listener)
> memory_listener);
> hwaddr start_addr = 0;
> ram_addr_t size = 0;
> + MemoryRegionSection *old_sections;
> + int n_old_sections;
> +
> uint64_t log_size;
> int r;
>
> + old_sections = dev->mem_sections;
> + n_old_sections = dev->n_mem_sections;
> + dev->mem_sections = dev->tmp_sections;
> + dev->n_mem_sections = dev->n_tmp_sections;
> +
> if (!dev->memory_changed) {
> - return;
> + goto out;
> }
> if (!dev->started) {
> - return;
> + goto out;
> }
> if (dev->mem_changed_start_addr > dev->mem_changed_end_addr) {
> - return;
> + goto out;
> }
>
> if (dev->started) {
> @@ -656,7 +666,7 @@ static void vhost_commit(MemoryListener *listener)
> VHOST_OPS_DEBUG("vhost_set_mem_table failed");
> }
> dev->memory_changed = false;
> - return;
> + goto out;
> }
> log_size = vhost_get_log_size(dev);
> /* We allocate an extra 4K bytes to log,
> @@ -675,6 +685,27 @@ static void vhost_commit(MemoryListener *listener)
> vhost_dev_log_resize(dev, log_size);
> }
> dev->memory_changed = false;
> +
> +out:
> + /* Deref the old list of sections, this must happen _after_ the
> + * vhost_set_mem_table to ensure the client isn't still using the
> + * section we're about to unref.
> + */
> + while (n_old_sections--) {
> + memory_region_unref(old_sections[n_old_sections].mr);
> + }
> + g_free(old_sections);
> + return;
> +}
> +
> +static void vhost_add_section(struct vhost_dev *dev,
> + MemoryRegionSection *section)
> +{
> + ++dev->n_tmp_sections;
> + dev->tmp_sections = g_renew(MemoryRegionSection, dev->tmp_sections,
> + dev->n_tmp_sections);
> + dev->tmp_sections[dev->n_tmp_sections - 1] = *section;
> + memory_region_ref(section->mr);
> }
>
> static void vhost_region_add(MemoryListener *listener,
> @@ -687,36 +718,31 @@ static void vhost_region_add(MemoryListener *listener,
> return;
> }
>
> - ++dev->n_mem_sections;
> - dev->mem_sections = g_renew(MemoryRegionSection, dev->mem_sections,
> - dev->n_mem_sections);
> - dev->mem_sections[dev->n_mem_sections - 1] = *section;
> - memory_region_ref(section->mr);
> + vhost_add_section(dev, section);
> vhost_set_memory(listener, section, true);
> }
>
> -static void vhost_region_del(MemoryListener *listener,
> +static void vhost_region_nop(MemoryListener *listener,
> MemoryRegionSection *section)
> {
> struct vhost_dev *dev = container_of(listener, struct vhost_dev,
> memory_listener);
> - int i;
>
> if (!vhost_section(section)) {
> return;
> }
>
> - vhost_set_memory(listener, section, false);
> - memory_region_unref(section->mr);
> - for (i = 0; i < dev->n_mem_sections; ++i) {
> - if (dev->mem_sections[i].offset_within_address_space
> - == section->offset_within_address_space) {
> - --dev->n_mem_sections;
> - memmove(&dev->mem_sections[i], &dev->mem_sections[i+1],
> - (dev->n_mem_sections - i) * sizeof(*dev->mem_sections));
> - break;
> - }
> + vhost_add_section(dev, section);
> +}
> +
> +static void vhost_region_del(MemoryListener *listener,
> + MemoryRegionSection *section)
> +{
> + if (!vhost_section(section)) {
> + return;
> }
> +
> + vhost_set_memory(listener, section, false);
> }
>
> static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> @@ -783,11 +809,6 @@ static void vhost_iommu_region_del(MemoryListener *listener,
> }
> }
>
> -static void vhost_region_nop(MemoryListener *listener,
> - MemoryRegionSection *section)
> -{
> -}
> -
> static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
> struct vhost_virtqueue *vq,
> unsigned idx, bool enable_log)
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 467dc7794b..21f3022499 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -54,6 +54,8 @@ struct vhost_dev {
> struct vhost_memory *mem;
> int n_mem_sections;
> MemoryRegionSection *mem_sections;
> + int n_tmp_sections;
> + MemoryRegionSection *tmp_sections;
> struct vhost_virtqueue *vqs;
> int nvqs;
> /* the first virtqueue which would be used by this vhost dev */
* Igor Mammedov (imammedo@redhat.com) wrote:
> On Mon, 18 Dec 2017 20:13:34 +0000
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
>
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Igor spotted that there's a race, where a region that's unref'd
> > in a _del callback might be free'd before the set_mem_table call in
> > the _commit callback, and thus the vhost might end up using free memory.
> >
> > Fix this by building a complete temporary sections list, ref'ing every
> > section (during add and nop) and then unref'ing the whole list right
> > at the end of commit.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> As Michael've suggested it should go into stable (CCed)
OK,
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Thanks!
Dave
>
> > ---
> > hw/virtio/vhost.c | 73 ++++++++++++++++++++++++++++++-----------------
> > include/hw/virtio/vhost.h | 2 ++
> > 2 files changed, 49 insertions(+), 26 deletions(-)
> >
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index e4290ce93d..610bfe6945 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -621,6 +621,8 @@ static void vhost_begin(MemoryListener *listener)
> > memory_listener);
> > dev->mem_changed_end_addr = 0;
> > dev->mem_changed_start_addr = -1;
> > + dev->tmp_sections = NULL;
> > + dev->n_tmp_sections = 0;
> > }
> >
> > static void vhost_commit(MemoryListener *listener)
> > @@ -629,17 +631,25 @@ static void vhost_commit(MemoryListener *listener)
> > memory_listener);
> > hwaddr start_addr = 0;
> > ram_addr_t size = 0;
> > + MemoryRegionSection *old_sections;
> > + int n_old_sections;
> > +
> > uint64_t log_size;
> > int r;
> >
> > + old_sections = dev->mem_sections;
> > + n_old_sections = dev->n_mem_sections;
> > + dev->mem_sections = dev->tmp_sections;
> > + dev->n_mem_sections = dev->n_tmp_sections;
> > +
> > if (!dev->memory_changed) {
> > - return;
> > + goto out;
> > }
> > if (!dev->started) {
> > - return;
> > + goto out;
> > }
> > if (dev->mem_changed_start_addr > dev->mem_changed_end_addr) {
> > - return;
> > + goto out;
> > }
> >
> > if (dev->started) {
> > @@ -656,7 +666,7 @@ static void vhost_commit(MemoryListener *listener)
> > VHOST_OPS_DEBUG("vhost_set_mem_table failed");
> > }
> > dev->memory_changed = false;
> > - return;
> > + goto out;
> > }
> > log_size = vhost_get_log_size(dev);
> > /* We allocate an extra 4K bytes to log,
> > @@ -675,6 +685,27 @@ static void vhost_commit(MemoryListener *listener)
> > vhost_dev_log_resize(dev, log_size);
> > }
> > dev->memory_changed = false;
> > +
> > +out:
> > + /* Deref the old list of sections, this must happen _after_ the
> > + * vhost_set_mem_table to ensure the client isn't still using the
> > + * section we're about to unref.
> > + */
> > + while (n_old_sections--) {
> > + memory_region_unref(old_sections[n_old_sections].mr);
> > + }
> > + g_free(old_sections);
> > + return;
> > +}
> > +
> > +static void vhost_add_section(struct vhost_dev *dev,
> > + MemoryRegionSection *section)
> > +{
> > + ++dev->n_tmp_sections;
> > + dev->tmp_sections = g_renew(MemoryRegionSection, dev->tmp_sections,
> > + dev->n_tmp_sections);
> > + dev->tmp_sections[dev->n_tmp_sections - 1] = *section;
> > + memory_region_ref(section->mr);
> > }
> >
> > static void vhost_region_add(MemoryListener *listener,
> > @@ -687,36 +718,31 @@ static void vhost_region_add(MemoryListener *listener,
> > return;
> > }
> >
> > - ++dev->n_mem_sections;
> > - dev->mem_sections = g_renew(MemoryRegionSection, dev->mem_sections,
> > - dev->n_mem_sections);
> > - dev->mem_sections[dev->n_mem_sections - 1] = *section;
> > - memory_region_ref(section->mr);
> > + vhost_add_section(dev, section);
> > vhost_set_memory(listener, section, true);
> > }
> >
> > -static void vhost_region_del(MemoryListener *listener,
> > +static void vhost_region_nop(MemoryListener *listener,
> > MemoryRegionSection *section)
> > {
> > struct vhost_dev *dev = container_of(listener, struct vhost_dev,
> > memory_listener);
> > - int i;
> >
> > if (!vhost_section(section)) {
> > return;
> > }
> >
> > - vhost_set_memory(listener, section, false);
> > - memory_region_unref(section->mr);
> > - for (i = 0; i < dev->n_mem_sections; ++i) {
> > - if (dev->mem_sections[i].offset_within_address_space
> > - == section->offset_within_address_space) {
> > - --dev->n_mem_sections;
> > - memmove(&dev->mem_sections[i], &dev->mem_sections[i+1],
> > - (dev->n_mem_sections - i) * sizeof(*dev->mem_sections));
> > - break;
> > - }
> > + vhost_add_section(dev, section);
> > +}
> > +
> > +static void vhost_region_del(MemoryListener *listener,
> > + MemoryRegionSection *section)
> > +{
> > + if (!vhost_section(section)) {
> > + return;
> > }
> > +
> > + vhost_set_memory(listener, section, false);
> > }
> >
> > static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > @@ -783,11 +809,6 @@ static void vhost_iommu_region_del(MemoryListener *listener,
> > }
> > }
> >
> > -static void vhost_region_nop(MemoryListener *listener,
> > - MemoryRegionSection *section)
> > -{
> > -}
> > -
> > static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
> > struct vhost_virtqueue *vq,
> > unsigned idx, bool enable_log)
> > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > index 467dc7794b..21f3022499 100644
> > --- a/include/hw/virtio/vhost.h
> > +++ b/include/hw/virtio/vhost.h
> > @@ -54,6 +54,8 @@ struct vhost_dev {
> > struct vhost_memory *mem;
> > int n_mem_sections;
> > MemoryRegionSection *mem_sections;
> > + int n_tmp_sections;
> > + MemoryRegionSection *tmp_sections;
> > struct vhost_virtqueue *vqs;
> > int nvqs;
> > /* the first virtqueue which would be used by this vhost dev */
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
© 2016 - 2025 Red Hat, Inc.