1. Patchew
  2. QEMU
  3. View series

[RFC PATCH] hw/display/virtio-gpu: Fix memory leak (CID 1453811)

Philippe Mathieu-Daudé posted 1 patch 2 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/next-importer-push tags/patchew/20210531101928.1662732-1-philmd@redhat.com
hw/display/virtio-gpu.c | 28 +++++++++++-----------------
1 file changed, 11 insertions(+), 17 deletions(-)
[RFC PATCH] hw/display/virtio-gpu: Fix memory leak (CID 1453811)
Posted by Philippe Mathieu-Daudé 2 years, 11 months ago 
To avoid leaking memory on the error path, reorder the
code as:
- check the parameters first
- check resource already existing
- finally allocate memory

Reported-by: Coverity (CID 1453811: RESOURCE_LEAK)
Fixes: e0933d91b1c ("virtio-gpu: Add virtio_gpu_resource_create_blob")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
RFC because the s->iov check is dubious.
---
 hw/display/virtio-gpu.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 4d549377cbc..8d047007bbb 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -340,8 +340,15 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
         return;
     }
 
-    res = virtio_gpu_find_resource(g, cblob.resource_id);
-    if (res) {
+    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
+        cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
+                      __func__);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+        return;
+    }
+
+    if (virtio_gpu_find_resource(g, cblob.resource_id)) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
                       __func__, cblob.resource_id);
         cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
@@ -352,25 +359,12 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
     res->resource_id = cblob.resource_id;
     res->blob_size = cblob.size;
 
-    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
-        cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
-                      __func__);
-        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
-        g_free(res);
-        return;
-    }
-
-    if (res->iov) {
-        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
-        return;
-    }
-
     ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
                                         cmd, &res->addrs, &res->iov,
                                         &res->iov_cnt);
-    if (ret != 0) {
+    if (ret != 0 || res->iov) {
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+        g_free(res);
         return;
     }
 
-- 
2.26.3
Re: [RFC PATCH] hw/display/virtio-gpu: Fix memory leak (CID 1453811)
Posted by Philippe Mathieu-Daudé 2 years, 10 months ago 
ping?

On 5/31/21 12:19 PM, Philippe Mathieu-Daudé wrote:
> To avoid leaking memory on the error path, reorder the
> code as:
> - check the parameters first
> - check resource already existing
> - finally allocate memory
> 
> Reported-by: Coverity (CID 1453811: RESOURCE_LEAK)
> Fixes: e0933d91b1c ("virtio-gpu: Add virtio_gpu_resource_create_blob")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> RFC because the s->iov check is dubious.
> ---
>  hw/display/virtio-gpu.c | 28 +++++++++++-----------------
>  1 file changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 4d549377cbc..8d047007bbb 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -340,8 +340,15 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
>          return;
>      }
>  
> -    res = virtio_gpu_find_resource(g, cblob.resource_id);
> -    if (res) {
> +    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
> +        cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
> +                      __func__);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
> +        return;
> +    }
> +
> +    if (virtio_gpu_find_resource(g, cblob.resource_id)) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
>                        __func__, cblob.resource_id);
>          cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> @@ -352,25 +359,12 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
>      res->resource_id = cblob.resource_id;
>      res->blob_size = cblob.size;
>  
> -    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
> -        cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
> -                      __func__);
> -        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
> -        g_free(res);
> -        return;
> -    }
> -
> -    if (res->iov) {
> -        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> -        return;
> -    }
> -
>      ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
>                                          cmd, &res->addrs, &res->iov,
>                                          &res->iov_cnt);
> -    if (ret != 0) {
> +    if (ret != 0 || res->iov) {
>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> +        g_free(res);
>          return;
>      }
>  
>
Re: [RFC PATCH] hw/display/virtio-gpu: Fix memory leak (CID 1453811)
Posted by Philippe Mathieu-Daudé 2 years, 10 months ago 
ping?

On 6/14/21 12:52 PM, Philippe Mathieu-Daudé wrote:
> ping?
> 
> On 5/31/21 12:19 PM, Philippe Mathieu-Daudé wrote:
>> To avoid leaking memory on the error path, reorder the
>> code as:
>> - check the parameters first
>> - check resource already existing
>> - finally allocate memory
>>
>> Reported-by: Coverity (CID 1453811: RESOURCE_LEAK)
>> Fixes: e0933d91b1c ("virtio-gpu: Add virtio_gpu_resource_create_blob")
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> RFC because the s->iov check is dubious.
>> ---
>>  hw/display/virtio-gpu.c | 28 +++++++++++-----------------
>>  1 file changed, 11 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>> index 4d549377cbc..8d047007bbb 100644
>> --- a/hw/display/virtio-gpu.c
>> +++ b/hw/display/virtio-gpu.c
>> @@ -340,8 +340,15 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
>>          return;
>>      }
>>  
>> -    res = virtio_gpu_find_resource(g, cblob.resource_id);
>> -    if (res) {
>> +    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
>> +        cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
>> +                      __func__);
>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>> +        return;
>> +    }
>> +
>> +    if (virtio_gpu_find_resource(g, cblob.resource_id)) {
>>          qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
>>                        __func__, cblob.resource_id);
>>          cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>> @@ -352,25 +359,12 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
>>      res->resource_id = cblob.resource_id;
>>      res->blob_size = cblob.size;
>>  
>> -    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
>> -        cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
>> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
>> -                      __func__);
>> -        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>> -        g_free(res);
>> -        return;
>> -    }
>> -
>> -    if (res->iov) {
>> -        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>> -        return;
>> -    }
>> -
>>      ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
>>                                          cmd, &res->addrs, &res->iov,
>>                                          &res->iov_cnt);
>> -    if (ret != 0) {
>> +    if (ret != 0 || res->iov) {
>>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>> +        g_free(res);
>>          return;
>>      }
>>  
>>
>
Re: [RFC PATCH] hw/display/virtio-gpu: Fix memory leak (CID 1453811)
Posted by Philippe Mathieu-Daudé 2 years, 9 months ago 
If this is not a proper fix, what would be the correct one?

On 6/21/21 11:59 AM, Philippe Mathieu-Daudé wrote:
> ping?
> 
> On 6/14/21 12:52 PM, Philippe Mathieu-Daudé wrote:
>> ping?
>>
>> On 5/31/21 12:19 PM, Philippe Mathieu-Daudé wrote:
>>> To avoid leaking memory on the error path, reorder the
>>> code as:
>>> - check the parameters first
>>> - check resource already existing
>>> - finally allocate memory
>>>
>>> Reported-by: Coverity (CID 1453811: RESOURCE_LEAK)
>>> Fixes: e0933d91b1c ("virtio-gpu: Add virtio_gpu_resource_create_blob")
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>> RFC because the s->iov check is dubious.
>>> ---
>>>  hw/display/virtio-gpu.c | 28 +++++++++++-----------------
>>>  1 file changed, 11 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>>> index 4d549377cbc..8d047007bbb 100644
>>> --- a/hw/display/virtio-gpu.c
>>> +++ b/hw/display/virtio-gpu.c
>>> @@ -340,8 +340,15 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
>>>          return;
>>>      }
>>>  
>>> -    res = virtio_gpu_find_resource(g, cblob.resource_id);
>>> -    if (res) {
>>> +    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
>>> +        cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
>>> +                      __func__);
>>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>>> +        return;
>>> +    }
>>> +
>>> +    if (virtio_gpu_find_resource(g, cblob.resource_id)) {
>>>          qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
>>>                        __func__, cblob.resource_id);
>>>          cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>>> @@ -352,25 +359,12 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
>>>      res->resource_id = cblob.resource_id;
>>>      res->blob_size = cblob.size;
>>>  
>>> -    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
>>> -        cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
>>> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
>>> -                      __func__);
>>> -        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>>> -        g_free(res);
>>> -        return;
>>> -    }
>>> -
>>> -    if (res->iov) {
>>> -        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>>> -        return;
>>> -    }
>>> -
>>>      ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
>>>                                          cmd, &res->addrs, &res->iov,
>>>                                          &res->iov_cnt);
>>> -    if (ret != 0) {
>>> +    if (ret != 0 || res->iov) {
>>>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>>> +        g_free(res);
>>>          return;
>>>      }
>>>  
>>>
>>
>
Re: [RFC PATCH] hw/display/virtio-gpu: Fix memory leak (CID 1453811)
Posted by Marc-André Lureau 2 years, 9 months ago 
Hi

On Mon, May 31, 2021 at 2:20 PM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> To avoid leaking memory on the error path, reorder the
> code as:
> - check the parameters first
> - check resource already existing
> - finally allocate memory
>
> Reported-by: Coverity (CID 1453811: RESOURCE_LEAK)
> Fixes: e0933d91b1c ("virtio-gpu: Add virtio_gpu_resource_create_blob")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> RFC because the s->iov check is dubious.
>

Yes, that looks wrong. Before the patch, the condition is always false /
dead code.

Furthermore, the init_udmabuf seems to really make sense when iov != NULL
and remapping takes place.

Vivek, please review

thanks

---
>  hw/display/virtio-gpu.c | 28 +++++++++++-----------------
>  1 file changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 4d549377cbc..8d047007bbb 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -340,8 +340,15 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU
> *g,
>          return;
>      }
>
> -    res = virtio_gpu_find_resource(g, cblob.resource_id);
> -    if (res) {
> +    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
> +        cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
> +                      __func__);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
> +        return;
> +    }
> +
> +    if (virtio_gpu_find_resource(g, cblob.resource_id)) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
>                        __func__, cblob.resource_id);
>          cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> @@ -352,25 +359,12 @@ static void
> virtio_gpu_resource_create_blob(VirtIOGPU *g,
>      res->resource_id = cblob.resource_id;
>      res->blob_size = cblob.size;
>
> -    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
> -        cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
> -                      __func__);
> -        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
> -        g_free(res);
> -        return;
> -    }
> -
> -    if (res->iov) {
> -        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> -        return;
> -    }
> -
>      ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries,
> sizeof(cblob),
>                                          cmd, &res->addrs, &res->iov,
>                                          &res->iov_cnt);
> -    if (ret != 0) {
> +    if (ret != 0 || res->iov) {
>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> +        g_free(res);
>          return;
>      }
>
> --
> 2.26.3
>
>
>

-- 
Marc-André Lureau
RE: [RFC PATCH] hw/display/virtio-gpu: Fix memory leak (CID 1453811)
Posted by Kasireddy, Vivek 2 years, 9 months ago 
Hi Philippe,
I am really sorry for the late review; I totally missed it until now.
Please find my comments inline.

From: Marc-André Lureau <marcandre.lureau@gmail.com>
Sent: Wednesday, July 07, 2021 4:05 AM
To: Philippe Mathieu-Daudé <philmd@redhat.com>; Kasireddy, Vivek <vivek.kasireddy@intel.com>
Cc: QEMU <qemu-devel@nongnu.org>; Gerd Hoffmann <kraxel@redhat.com>; Michael S. Tsirkin <mst@redhat.com>
Subject: Re: [RFC PATCH] hw/display/virtio-gpu: Fix memory leak (CID 1453811)

Hi

On Mon, May 31, 2021 at 2:20 PM Philippe Mathieu-Daudé <philmd@redhat.com<mailto:philmd@redhat.com>> wrote:
To avoid leaking memory on the error path, reorder the
code as:
- check the parameters first
- check resource already existing
- finally allocate memory

Reported-by: Coverity (CID 1453811: RESOURCE_LEAK)
Fixes: e0933d91b1c ("virtio-gpu: Add virtio_gpu_resource_create_blob")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com<mailto:philmd@redhat.com>>
---
RFC because the s->iov check is dubious.

Yes, that looks wrong. Before the patch, the condition is always false / dead code.

Furthermore, the init_udmabuf seems to really make sense when iov != NULL and remapping takes place.

Vivek, please review

thanks

---
 hw/display/virtio-gpu.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 4d549377cbc..8d047007bbb 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -340,8 +340,15 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
         return;
     }

-    res = virtio_gpu_find_resource(g, cblob.resource_id);
-    if (res) {
+    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
+        cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
+                      __func__);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+        return;
+    }
+
+    if (virtio_gpu_find_resource(g, cblob.resource_id)) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
                       __func__, cblob.resource_id);
         cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
@@ -352,25 +359,12 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
     res->resource_id = cblob.resource_id;
     res->blob_size = cblob.size;

-    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
-        cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
-                      __func__);
-        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
-        g_free(res);
-        return;
-    }
-
-    if (res->iov) {
-        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
-        return;
-    }
[Kasireddy, Vivek] Yeah, removing this makes sense. Basically,
resource_create_blob = resource_create + attach_backing and I was trying to do what
attach_backing was doing but this conditional does not make sense for create_blob as
we just created the resource few lines above.

-
     ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
                                         cmd, &res->addrs, &res->iov,
                                         &res->iov_cnt);
-    if (ret != 0) {
+    if (ret != 0 || res->iov) {
[Kasireddy, Vivek] Would be redundant to check for res->iov as iov == NULL case will be covered by ret != 0.
With this change,

Reviewed-by: Vivek Kasireddy <vivek.kasireddy@intel.com<mailto:vivek.kasireddy@intel.com>>

Thanks,
Vivek

         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+        g_free(res);
         return;
     }

--
2.26.3



--
Marc-André Lureau

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.