[PATCH] vhost: Add a defensive check in vhost_commit against wrong deallocation

[PATCH] vhost: Add a defensive check in vhost_commit against wrong deallocation

[Eric Auger]

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(+)

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

Re: [PATCH] vhost: Add a defensive check in vhost_commit against wrong deallocation

[Jason Wang]

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
>

Re: [PATCH] vhost: Add a defensive check in vhost_commit against wrong deallocation

[Eric Auger]

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
>>

Re: [PATCH] vhost: Add a defensive check in vhost_commit against wrong deallocation

[Michael S. Tsirkin]

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

Re: [PATCH] vhost: Add a defensive check in vhost_commit against wrong deallocation

[Eric Auger]

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