[PATCH 0/7] vhost-user-gpu: fix several security issues

[PATCH 0/7] vhost-user-gpu: fix several security issues

[Li Qiang]

These security issue is low severity and is similar with the
virtio-vga/virtio-gpu device. All of them can be triggered by
the guest user.

Li Qiang (7):
  vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info
  vhost-user-gpu: fix resource leak in 'vg_resource_create_2d'
  vhost-user-gpu: fix memory leak in vg_resource_attach_backing
  vhost-user-gpu: fix memory link while calling 'vg_resource_unref'
  vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'
  vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
  vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset'

 contrib/vhost-user-gpu/vhost-user-gpu.c |  7 +++++++
 contrib/vhost-user-gpu/virgl.c          | 17 ++++++++++++++++-
 2 files changed, 23 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info

[Li Qiang]

Otherwise some of the 'resp' will be leaked to guest.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 contrib/vhost-user-gpu/virgl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
index 9e6660c7ab..6a332d601f 100644
--- a/contrib/vhost-user-gpu/virgl.c
+++ b/contrib/vhost-user-gpu/virgl.c
@@ -128,6 +128,7 @@ virgl_cmd_get_capset_info(VuGpu *g,
 
     VUGPU_FILL_CMD(info);
 
+    memset(&resp, 0, sizeof(resp));
     if (info.capset_index == 0) {
         resp.capset_id = VIRTIO_GPU_CAPSET_VIRGL;
         virgl_renderer_get_cap_set(resp.capset_id,
-- 
2.25.1

Re: [PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info

[P J P]

+-- On Tue, 4 May 2021, Li Qiang wrote --+
| Otherwise some of the 'resp' will be leaked to guest.
| 
| diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
| index 9e6660c7ab..6a332d601f 100644
|  
| +    memset(&resp, 0, sizeof(resp));
|      if (info.capset_index == 0) {
|          resp.capset_id = VIRTIO_GPU_CAPSET_VIRGL;
|          virgl_renderer_get_cap_set(resp.capset_id,

-   vg_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
+   vg_ctrl_response(g, cmd, &resp.hdr, sizeof(resp.hdr));

* While memset(3) is okay, should it also send header(hdr) size as 'resp_len'?


Thank you.
--
 - P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info

[Li Qiang]

P J P <ppandit@redhat.com> 于2021年5月5日周三 下午3:24写道：
>
> +-- On Tue, 4 May 2021, Li Qiang wrote --+
> | Otherwise some of the 'resp' will be leaked to guest.
> |
> | diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
> | index 9e6660c7ab..6a332d601f 100644
> |
> | +    memset(&resp, 0, sizeof(resp));
> |      if (info.capset_index == 0) {
> |          resp.capset_id = VIRTIO_GPU_CAPSET_VIRGL;
> |          virgl_renderer_get_cap_set(resp.capset_id,
>
> -   vg_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
> +   vg_ctrl_response(g, cmd, &resp.hdr, sizeof(resp.hdr));
>
> * While memset(3) is okay, should it also send header(hdr) size as 'resp_len'?
>

I don't think so. This function also set fields other than header such
as 'resp.capset_id', 'resp.capset_max_version' and so on.


Thanks,
Li Qiang

>
> Thank you.
> --
>  - P J P
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
>

Re: [PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info

[P J P]

[-- Attachment #1: Type: text/plain, Size: 736 bytes --]

+-- On Wed, 5 May 2021, Li Qiang wrote --+
| P J P <ppandit@redhat.com> 于2021年5月5日周三 下午3:24写道：
| > -   vg_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
| > +   vg_ctrl_response(g, cmd, &resp.hdr, sizeof(resp.hdr));
| >
| > * While memset(3) is okay, should it also send header(hdr) size as 'resp_len'?
| 
| I don't think so. This function also set fields other than header such
| as 'resp.capset_id', 'resp.capset_max_version' and so on.

But it is passing 'resp.hdr' reference as parameter and size of 'resp' as 
length.

  sizeof(struct virtio_gpu_ctrl_hdr): 24
  sizeof(struct virtio_gpu_resp_capset_info): 40

It may cause OOB access.

Thank you.
--
 - P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info

[Li Qiang]

P J P <ppandit@redhat.com> 于2021年5月6日周四 下午1:53写道：
>
> +-- On Wed, 5 May 2021, Li Qiang wrote --+
> | P J P <ppandit@redhat.com> 于2021年5月5日周三 下午3:24写道：
> | > -   vg_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
> | > +   vg_ctrl_response(g, cmd, &resp.hdr, sizeof(resp.hdr));
> | >
> | > * While memset(3) is okay, should it also send header(hdr) size as 'resp_len'?
> |
> | I don't think so. This function also set fields other than header such
> | as 'resp.capset_id', 'resp.capset_max_version' and so on.
>
> But it is passing 'resp.hdr' reference as parameter and size of 'resp' as
> length.
>
>   sizeof(struct virtio_gpu_ctrl_hdr): 24
>   sizeof(struct virtio_gpu_resp_capset_info): 40
>
> It may cause OOB access.

Where is the OOB access? I don't see this.
vg_ctrl_response is a general function so it accepts 'struct
virtio_gpu_ctrl_hdr' pointer and will
just set the 'hdr' field. The 'resp_len' is just used in
'iov_from_buf' to copy data to the vring.
The 'hdr' is the first field of 'virtio_gpu_resp_capset_info'.

struct virtio_gpu_resp_capset_info {
    struct virtio_gpu_ctrl_hdr hdr;
    uint32_t capset_id;
    uint32_t capset_max_version;
    uint32_t capset_max_size;
    uint32_t padding;
};

So here:
vg_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
&resp.hdr is the same as &resp.


Thanks,
Li Qiang


>
> Thank you.
> --
>  - P J P
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

[PATCH 2/7] vhost-user-gpu: fix resource leak in 'vg_resource_create_2d'

[Li Qiang]

Call 'vugbm_buffer_destroy' in error path to avoid resource leak.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 contrib/vhost-user-gpu/vhost-user-gpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c b/contrib/vhost-user-gpu/vhost-user-gpu.c
index f73f292c9f..b5e153d0d6 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -349,6 +349,7 @@ vg_resource_create_2d(VuGpu *g,
         g_critical("%s: resource creation failed %d %d %d",
                    __func__, c2d.resource_id, c2d.width, c2d.height);
         g_free(res);
+        vugbm_buffer_destroy(&res->buffer);
         cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
         return;
     }
-- 
2.25.1

Re: [PATCH 2/7] vhost-user-gpu: fix resource leak in 'vg_resource_create_2d'

[P J P]

+-- On Tue, 4 May 2021, Li Qiang wrote --+
| Call 'vugbm_buffer_destroy' in error path to avoid resource leak.
| 
| Signed-off-by: Li Qiang <liq3ea@163.com>
| ---
|  contrib/vhost-user-gpu/vhost-user-gpu.c | 1 +
|  1 file changed, 1 insertion(+)
| 
| diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c b/contrib/vhost-user-gpu/vhost-user-gpu.c
| index f73f292c9f..b5e153d0d6 100644
| --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
| +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
| @@ -349,6 +349,7 @@ vg_resource_create_2d(VuGpu *g,
|          g_critical("%s: resource creation failed %d %d %d",
|                     __func__, c2d.resource_id, c2d.width, c2d.height);
|          g_free(res);
| +        vugbm_buffer_destroy(&res->buffer);
|          cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
|          return;
|      }

* Looks good.

Reviewed-by: Prasad J Pandit <pjp@fedoraproject.org>

Thank you.
--
 - P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

[PATCH 3/7] vhost-user-gpu: fix memory leak in vg_resource_attach_backing

[Li Qiang]

Check whether the 'res' has already been attach_backing to avoid
memory leak.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 contrib/vhost-user-gpu/vhost-user-gpu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c b/contrib/vhost-user-gpu/vhost-user-gpu.c
index b5e153d0d6..0437e52b64 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -489,6 +489,11 @@ vg_resource_attach_backing(VuGpu *g,
         return;
     }
 
+    if (res->iov) {
+        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+        return;
+    }
+
     ret = vg_create_mapping_iov(g, &ab, cmd, &res->iov);
     if (ret != 0) {
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
-- 
2.25.1

Re: [PATCH 3/7] vhost-user-gpu: fix memory leak in vg_resource_attach_backing

[P J P]

+-- On Tue, 4 May 2021, Li Qiang wrote --+
| Check whether the 'res' has already been attach_backing to avoid
| memory leak.
| 
| Signed-off-by: Li Qiang <liq3ea@163.com>
| ---
|  contrib/vhost-user-gpu/vhost-user-gpu.c | 5 +++++
|  1 file changed, 5 insertions(+)
| 
| diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c b/contrib/vhost-user-gpu/vhost-user-gpu.c
| index b5e153d0d6..0437e52b64 100644
| --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
| +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
| @@ -489,6 +489,11 @@ vg_resource_attach_backing(VuGpu *g,
|          return;
|      }
|  
| +    if (res->iov) {
| +        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
| +        return;
| +    }
| +
|      ret = vg_create_mapping_iov(g, &ab, cmd, &res->iov);
|      if (ret != 0) {
|          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;


* While it'll work, should the check be done by 'virtio_gpu_find_resource()' 
  before returning 'res' ? ie. if 'res->iov' is being used, then it's similar 
  case as 'illegal resource specified %d'.


Thank you.
--
 - P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH 3/7] vhost-user-gpu: fix memory leak in vg_resource_attach_backing

[Li Qiang]

P J P <ppandit@redhat.com> 于2021年5月5日周三 下午3:39写道：
>
> +-- On Tue, 4 May 2021, Li Qiang wrote --+
> | Check whether the 'res' has already been attach_backing to avoid
> | memory leak.
> |
> | Signed-off-by: Li Qiang <liq3ea@163.com>
> | ---
> |  contrib/vhost-user-gpu/vhost-user-gpu.c | 5 +++++
> |  1 file changed, 5 insertions(+)
> |
> | diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c b/contrib/vhost-user-gpu/vhost-user-gpu.c
> | index b5e153d0d6..0437e52b64 100644
> | --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
> | +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
> | @@ -489,6 +489,11 @@ vg_resource_attach_backing(VuGpu *g,
> |          return;
> |      }
> |
> | +    if (res->iov) {
> | +        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> | +        return;
> | +    }
> | +
> |      ret = vg_create_mapping_iov(g, &ab, cmd, &res->iov);
> |      if (ret != 0) {
> |          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>
>
> * While it'll work, should the check be done by 'virtio_gpu_find_resource()'
>   before returning 'res' ? ie. if 'res->iov' is being used, then it's similar
>   case as 'illegal resource specified %d'.

No, 'virtio_gpu_find_resource' is a general function and is used in a lot place
and 'res->iov' is specified to 'vg_resource_[de]attach_backing' so we should do
this check in 'vg_resource_attach_backing'.

Thanks,
Li Qiang

>
>
> Thank you.
> --
>  - P J P
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
>

[PATCH 4/7] vhost-user-gpu: fix memory link while calling 'vg_resource_unref'

[Li Qiang]

If the guest trigger following sequences, the attach_backing will be leaked:

	vg_resource_create_2d
	vg_resource_attach_backing
	vg_resource_unref

This patch fix this by freeing 'res->iov' in vg_resource_destroy.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 contrib/vhost-user-gpu/vhost-user-gpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c b/contrib/vhost-user-gpu/vhost-user-gpu.c
index 0437e52b64..770dfad529 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -400,6 +400,7 @@ vg_resource_destroy(VuGpu *g,
     }
 
     vugbm_buffer_destroy(&res->buffer);
+    g_free(res->iov);
     pixman_image_unref(res->image);
     QTAILQ_REMOVE(&g->reslist, res, next);
     g_free(res);
-- 
2.25.1

Re: [PATCH 4/7] vhost-user-gpu: fix memory link while calling 'vg_resource_unref'

[P J P]

+-- On Tue, 4 May 2021, Li Qiang wrote --+
| If the guest trigger following sequences, the attach_backing will be leaked:
| 
| 	vg_resource_create_2d
| 	vg_resource_attach_backing
| 	vg_resource_unref
| 
| This patch fix this by freeing 'res->iov' in vg_resource_destroy.
| 
| Signed-off-by: Li Qiang <liq3ea@163.com>
| ---
|  contrib/vhost-user-gpu/vhost-user-gpu.c | 1 +
|  1 file changed, 1 insertion(+)
| 
| diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c b/contrib/vhost-user-gpu/vhost-user-gpu.c
| index 0437e52b64..770dfad529 100644
| --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
| +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
| @@ -400,6 +400,7 @@ vg_resource_destroy(VuGpu *g,
|      }
|  
|      vugbm_buffer_destroy(&res->buffer);
| +    g_free(res->iov);
|      pixman_image_unref(res->image);
|      QTAILQ_REMOVE(&g->reslist, res, next);
|      g_free(res);
| 

* Looks good.

Reviewed-by: Prasad J Pandit <pjp@fedoraproject.org>


Thank you.
--
 - P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

[PATCH 5/7] vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'

[Li Qiang]

The 'res->iov' will be leaked if the guest trigger following sequences:

	virgl_cmd_create_resource_2d
	virgl_resource_attach_backing
	virgl_cmd_resource_unref

This patch fixes this.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 contrib/vhost-user-gpu/virgl.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
index 6a332d601f..c669d73a1d 100644
--- a/contrib/vhost-user-gpu/virgl.c
+++ b/contrib/vhost-user-gpu/virgl.c
@@ -108,9 +108,16 @@ virgl_cmd_resource_unref(VuGpu *g,
                          struct virtio_gpu_ctrl_command *cmd)
 {
     struct virtio_gpu_resource_unref unref;
+    struct iovec *res_iovs = NULL;
+    int num_iovs = 0;
 
     VUGPU_FILL_CMD(unref);
 
+    virgl_renderer_resource_detach_iov(unref.resource_id,
+                                       &res_iovs,
+                                       &num_iovs);
+    g_free(res_iovs);
+
     virgl_renderer_resource_unref(unref.resource_id);
 }
 
-- 
2.25.1

Re: [PATCH 5/7] vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'

[P J P]

+-- On Tue, 4 May 2021, Li Qiang wrote --+
| diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
| index 6a332d601f..c669d73a1d 100644
| --- a/contrib/vhost-user-gpu/virgl.c
| +++ b/contrib/vhost-user-gpu/virgl.c
| @@ -108,9 +108,16 @@ virgl_cmd_resource_unref(VuGpu *g,
|                           struct virtio_gpu_ctrl_command *cmd)
|  {
|      struct virtio_gpu_resource_unref unref;
| +    struct iovec *res_iovs = NULL;
| +    int num_iovs = 0;
|  
|      VUGPU_FILL_CMD(unref);
|  
| +    virgl_renderer_resource_detach_iov(unref.resource_id,
| +                                       &res_iovs,
| +                                       &num_iovs);
| +    g_free(res_iovs);
| +
|      virgl_renderer_resource_unref(unref.resource_id);

* Should this also call 'virtio_gpu_cleanup_mapping_iov' similar to 
  'hw/display/virtio-gpu-3d.c:virgl_cmd_resource_unref'?

    if (res_iovs != NULL && num_iovs != 0) {                                    
        virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);                  
    }


Thank you.
--
 - P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH 5/7] vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'

[Li Qiang]

P J P <ppandit@redhat.com> 于2021年5月5日周三 下午3:48写道：
>
> +-- On Tue, 4 May 2021, Li Qiang wrote --+
> | diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
> | index 6a332d601f..c669d73a1d 100644
> | --- a/contrib/vhost-user-gpu/virgl.c
> | +++ b/contrib/vhost-user-gpu/virgl.c
> | @@ -108,9 +108,16 @@ virgl_cmd_resource_unref(VuGpu *g,
> |                           struct virtio_gpu_ctrl_command *cmd)
> |  {
> |      struct virtio_gpu_resource_unref unref;
> | +    struct iovec *res_iovs = NULL;
> | +    int num_iovs = 0;
> |
> |      VUGPU_FILL_CMD(unref);
> |
> | +    virgl_renderer_resource_detach_iov(unref.resource_id,
> | +                                       &res_iovs,
> | +                                       &num_iovs);
> | +    g_free(res_iovs);
> | +
> |      virgl_renderer_resource_unref(unref.resource_id);
>
> * Should this also call 'virtio_gpu_cleanup_mapping_iov' similar to
>   'hw/display/virtio-gpu-3d.c:virgl_cmd_resource_unref'?
>
>     if (res_iovs != NULL && num_iovs != 0) {
>         virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
>     }
>
>

No because the resource here contains only 'res->iov' no memory mapping
like 'hw/display/virtio-gpu-3d.c:virgl_cmd_resource_unref'.

Thanks,
Li Qiang

> Thank you.
> --
>  - P J P
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
>

[PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'

[Li Qiang]

If 'virgl_renderer_resource_attach_iov' failed, the 'res_iovs' will
be leaked.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 contrib/vhost-user-gpu/virgl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
index c669d73a1d..a16a311d80 100644
--- a/contrib/vhost-user-gpu/virgl.c
+++ b/contrib/vhost-user-gpu/virgl.c
@@ -287,8 +287,11 @@ virgl_resource_attach_backing(VuGpu *g,
         return;
     }
 
-    virgl_renderer_resource_attach_iov(att_rb.resource_id,
+    ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
                                        res_iovs, att_rb.nr_entries);
+    if (ret != 0) {
+        g_free(res_iovs);
+    }
 }
 
 static void
-- 
2.25.1

Re: [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'

[P J P]

+-- On Tue, 4 May 2021, Li Qiang wrote --+
| diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
| index c669d73a1d..a16a311d80 100644
| --- a/contrib/vhost-user-gpu/virgl.c
| +++ b/contrib/vhost-user-gpu/virgl.c
| @@ -287,8 +287,11 @@ virgl_resource_attach_backing(VuGpu *g,
|          return;
|      }
|  
| -    virgl_renderer_resource_attach_iov(att_rb.resource_id,
| +    ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
|                                         res_iovs, att_rb.nr_entries);
| +    if (ret != 0) {
| +        g_free(res_iovs);
| +    }
|  }

* Similar to earlier, 
  hw/display/virtio-gpu-3d.c:virgl_resource_attach_backing() calls 
  'virtio_gpu_cleanup_mapping_iov'

* should it be same for vhost-user-gpu?


Thank you.
--
 - P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

Hi

On Wed, May 5, 2021 at 12:03 PM P J P <ppandit@redhat.com> wrote:

> +-- On Tue, 4 May 2021, Li Qiang wrote --+
> | diff --git a/contrib/vhost-user-gpu/virgl.c
> b/contrib/vhost-user-gpu/virgl.c
> | index c669d73a1d..a16a311d80 100644
> | --- a/contrib/vhost-user-gpu/virgl.c
> | +++ b/contrib/vhost-user-gpu/virgl.c
> | @@ -287,8 +287,11 @@ virgl_resource_attach_backing(VuGpu *g,
> |          return;
> |      }
> |
> | -    virgl_renderer_resource_attach_iov(att_rb.resource_id,
> | +    ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
> |                                         res_iovs, att_rb.nr_entries);
> | +    if (ret != 0) {
> | +        g_free(res_iovs);
> | +    }
> |  }
>
> * Similar to earlier,
>   hw/display/virtio-gpu-3d.c:virgl_resource_attach_backing() calls
>   'virtio_gpu_cleanup_mapping_iov'
>
> * should it be same for vhost-user-gpu?
>
>
>
Good question, that's also what you did when you fixed it for virtio-gpu in:

commit 33243031dad02d161225ba99d782616da133f689
Author: Li Qiang <liq3ea@gmail.com>
Date:   Thu Dec 29 03:11:26 2016 -0500

    virtio-gpu-3d: fix memory leak in resource attach backing


Btw, for each patch, it would be nice to have a reference to the original
fix in virtio-gpu.

Thanks!

[-- Attachment #2: Type: text/html, Size: 1879 bytes --]

Re: [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'

[Li Qiang]

Marc-André Lureau <marcandre.lureau@gmail.com> 于2021年5月5日周三 下午5:08写道：
>
> Hi
>
> On Wed, May 5, 2021 at 12:03 PM P J P <ppandit@redhat.com> wrote:
>>
>> +-- On Tue, 4 May 2021, Li Qiang wrote --+
>> | diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
>> | index c669d73a1d..a16a311d80 100644
>> | --- a/contrib/vhost-user-gpu/virgl.c
>> | +++ b/contrib/vhost-user-gpu/virgl.c
>> | @@ -287,8 +287,11 @@ virgl_resource_attach_backing(VuGpu *g,
>> |          return;
>> |      }
>> |
>> | -    virgl_renderer_resource_attach_iov(att_rb.resource_id,
>> | +    ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
>> |                                         res_iovs, att_rb.nr_entries);
>> | +    if (ret != 0) {
>> | +        g_free(res_iovs);
>> | +    }
>> |  }
>>
>> * Similar to earlier,
>>   hw/display/virtio-gpu-3d.c:virgl_resource_attach_backing() calls
>>   'virtio_gpu_cleanup_mapping_iov'
>>
>> * should it be same for vhost-user-gpu?
>>
>>
>
> Good question, that's also what you did when you fixed it for virtio-gpu in:
>
> commit 33243031dad02d161225ba99d782616da133f689
> Author: Li Qiang <liq3ea@gmail.com>
> Date:   Thu Dec 29 03:11:26 2016 -0500
>
>     virtio-gpu-3d: fix memory leak in resource attach backing
>

Do you mean this;
-->https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01156.html

I think there is no need for this.

The virtio_gpu_cleanup_mapping_iov is needed in virtio-gpu is because
it need map guest memory.
But in vhost-user-gpu case, the 'vg_create_mapping_iov' calls
'vu_gpa_to_va' and this function don't need
do map memory.

But for the beauty of symmetry we can abstract a function called
'vg_destroy_mapping_iov' and it just calls g_free(res->iov).
Like the pair 'virtio_gpu_create_mapping_iov' and 'virtio_gpu_cleanup_mapping'.

Thanks,
Li Qiang

>
> Btw, for each patch, it would be nice to have a reference to the original fix in virtio-gpu.
>
> Thanks!
>

Re: [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2457 bytes --]

Hi

On Wed, May 5, 2021 at 1:24 PM Li Qiang <liq3ea@gmail.com> wrote:

> Marc-André Lureau <marcandre.lureau@gmail.com> 于2021年5月5日周三 下午5:08写道：
> >
> > Hi
> >
> > On Wed, May 5, 2021 at 12:03 PM P J P <ppandit@redhat.com> wrote:
> >>
> >> +-- On Tue, 4 May 2021, Li Qiang wrote --+
> >> | diff --git a/contrib/vhost-user-gpu/virgl.c
> b/contrib/vhost-user-gpu/virgl.c
> >> | index c669d73a1d..a16a311d80 100644
> >> | --- a/contrib/vhost-user-gpu/virgl.c
> >> | +++ b/contrib/vhost-user-gpu/virgl.c
> >> | @@ -287,8 +287,11 @@ virgl_resource_attach_backing(VuGpu *g,
> >> |          return;
> >> |      }
> >> |
> >> | -    virgl_renderer_resource_attach_iov(att_rb.resource_id,
> >> | +    ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
> >> |                                         res_iovs, att_rb.nr_entries);
> >> | +    if (ret != 0) {
> >> | +        g_free(res_iovs);
> >> | +    }
> >> |  }
> >>
> >> * Similar to earlier,
> >>   hw/display/virtio-gpu-3d.c:virgl_resource_attach_backing() calls
> >>   'virtio_gpu_cleanup_mapping_iov'
> >>
> >> * should it be same for vhost-user-gpu?
> >>
> >>
> >
> > Good question, that's also what you did when you fixed it for virtio-gpu
> in:
> >
> > commit 33243031dad02d161225ba99d782616da133f689
> > Author: Li Qiang <liq3ea@gmail.com>
> > Date:   Thu Dec 29 03:11:26 2016 -0500
> >
> >     virtio-gpu-3d: fix memory leak in resource attach backing
> >
>
> Do you mean this;
> -->https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg01156.html
>
> I think there is no need for this.
>
> The virtio_gpu_cleanup_mapping_iov is needed in virtio-gpu is because
> it need map guest memory.
> But in vhost-user-gpu case, the 'vg_create_mapping_iov' calls
> 'vu_gpa_to_va' and this function don't need
> do map memory.
>
> But for the beauty of symmetry we can abstract a function called
> 'vg_destroy_mapping_iov' and it just calls g_free(res->iov).
> Like the pair 'virtio_gpu_create_mapping_iov' and
> 'virtio_gpu_cleanup_mapping'.
>
>
Right. I think I like the suggestion to add a 'virtio_gpu_cleanup_mapping'
(with a comment to explain it) to avoid this kind of question again. Feel
free to add a patch on top if you want.

thanks


>
> >
> > Btw, for each patch, it would be nice to have a reference to the
> original fix in virtio-gpu.
> >
> > Thanks!
> >
>


-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3752 bytes --]

[PATCH 7/7] vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset'

[Li Qiang]

If 'virgl_cmd_get_capset' set 'max_size' to 0,
the 'virgl_renderer_fill_caps' will write the data after the 'resp'.
This patch avoid this by checking the returned 'max_size'.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 contrib/vhost-user-gpu/virgl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
index a16a311d80..7172104b19 100644
--- a/contrib/vhost-user-gpu/virgl.c
+++ b/contrib/vhost-user-gpu/virgl.c
@@ -177,6 +177,10 @@ virgl_cmd_get_capset(VuGpu *g,
 
     virgl_renderer_get_cap_set(gc.capset_id, &max_ver,
                                &max_size);
+    if (!max_size) {
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+        return;
+    }
     resp = g_malloc0(sizeof(*resp) + max_size);
 
     resp->hdr.type = VIRTIO_GPU_RESP_OK_CAPSET;
-- 
2.25.1

Re: [PATCH 7/7] vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset'

[P J P]

+-- On Tue, 4 May 2021, Li Qiang wrote --+
| If 'virgl_cmd_get_capset' set 'max_size' to 0,
| the 'virgl_renderer_fill_caps' will write the data after the 'resp'.
| This patch avoid this by checking the returned 'max_size'.
| 
| Signed-off-by: Li Qiang <liq3ea@163.com>
| ---
|  contrib/vhost-user-gpu/virgl.c | 4 ++++
|  1 file changed, 4 insertions(+)
| 
| diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
| index a16a311d80..7172104b19 100644
| --- a/contrib/vhost-user-gpu/virgl.c
| +++ b/contrib/vhost-user-gpu/virgl.c
| @@ -177,6 +177,10 @@ virgl_cmd_get_capset(VuGpu *g,
|  
|      virgl_renderer_get_cap_set(gc.capset_id, &max_ver,
|                                 &max_size);
| +    if (!max_size) {
| +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
| +        return;
| +    }
|      resp = g_malloc0(sizeof(*resp) + max_size);
|  
|      resp->hdr.type = VIRTIO_GPU_RESP_OK_CAPSET;

* Looks okay.

Reviewed-by: Prasad J Pandit <pjp@fedoraproject.org>

Thank you.
--
 - P J P
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH 0/7] vhost-user-gpu: fix several security issues

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1043 bytes --]

Hi

On Wed, May 5, 2021 at 9:21 AM Li Qiang <liq3ea@163.com> wrote:

> These security issue is low severity and is similar with the
> virtio-vga/virtio-gpu device. All of them can be triggered by
> the guest user.
>
> Li Qiang (7):
>   vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info
>   vhost-user-gpu: fix resource leak in 'vg_resource_create_2d'
>   vhost-user-gpu: fix memory leak in vg_resource_attach_backing
>   vhost-user-gpu: fix memory link while calling 'vg_resource_unref'
>   vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'
>   vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
>   vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset'
>
>  contrib/vhost-user-gpu/vhost-user-gpu.c |  7 +++++++
>  contrib/vhost-user-gpu/virgl.c          | 17 ++++++++++++++++-
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
> --
>

The whole series looks good to me, and applies fixes that were done earlier
in virtio-gpu.

Thanks


-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 1545 bytes --]

Re: [PATCH 0/7] vhost-user-gpu: fix several security issues

[Li Qiang]

Marc-André Lureau <marcandre.lureau@gmail.com> 于2021年5月5日周三 下午5:10写道：
>
> Hi
>
> On Wed, May 5, 2021 at 9:21 AM Li Qiang <liq3ea@163.com> wrote:
>>
>> These security issue is low severity and is similar with the
>> virtio-vga/virtio-gpu device. All of them can be triggered by
>> the guest user.
>>
>> Li Qiang (7):
>>   vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info
>>   vhost-user-gpu: fix resource leak in 'vg_resource_create_2d'
>>   vhost-user-gpu: fix memory leak in vg_resource_attach_backing
>>   vhost-user-gpu: fix memory link while calling 'vg_resource_unref'
>>   vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'
>>   vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
>>   vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset'
>>
>>  contrib/vhost-user-gpu/vhost-user-gpu.c |  7 +++++++
>>  contrib/vhost-user-gpu/virgl.c          | 17 ++++++++++++++++-
>>  2 files changed, 23 insertions(+), 1 deletion(-)
>>
>> --
>
>
> The whole series looks good to me, and applies fixes that were done earlier in virtio-gpu.

Do you mean you have merged this series?
Should I tweak something such as "adding the original fix in
virtio-gpu"/"better mapping iov cleanup"?

Thanks,
Li Qiang

>
> Thanks
>
>
> --
> Marc-André Lureau

Re: [PATCH 0/7] vhost-user-gpu: fix several security issues

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1581 bytes --]

Hi

On Wed, May 5, 2021 at 1:28 PM Li Qiang <liq3ea@gmail.com> wrote:

> Marc-André Lureau <marcandre.lureau@gmail.com> 于2021年5月5日周三 下午5:10写道：
> >
> > Hi
> >
> > On Wed, May 5, 2021 at 9:21 AM Li Qiang <liq3ea@163.com> wrote:
> >>
> >> These security issue is low severity and is similar with the
> >> virtio-vga/virtio-gpu device. All of them can be triggered by
> >> the guest user.
> >>
> >> Li Qiang (7):
> >>   vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info
> >>   vhost-user-gpu: fix resource leak in 'vg_resource_create_2d'
> >>   vhost-user-gpu: fix memory leak in vg_resource_attach_backing
> >>   vhost-user-gpu: fix memory link while calling 'vg_resource_unref'
> >>   vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'
> >>   vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
> >>   vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset'
> >>
> >>  contrib/vhost-user-gpu/vhost-user-gpu.c |  7 +++++++
> >>  contrib/vhost-user-gpu/virgl.c          | 17 ++++++++++++++++-
> >>  2 files changed, 23 insertions(+), 1 deletion(-)
> >>
> >> --
> >
> >
> > The whole series looks good to me, and applies fixes that were done
> earlier in virtio-gpu.
>
> Do you mean you have merged this series?
> Should I tweak something such as "adding the original fix in
> virtio-gpu"/"better mapping iov cleanup"?
>
>
No I didn't. I was waiting for the answers to Prasad questions, and
eventually v2.

Then either Gerd or me can queue this imho.

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2393 bytes --]

Re: [PATCH 0/7] vhost-user-gpu: fix several security issues

[Philippe Mathieu-Daudé]

On 5/5/21 11:35 AM, Marc-André Lureau wrote:
> Hi
> 
> On Wed, May 5, 2021 at 1:28 PM Li Qiang <liq3ea@gmail.com
> <mailto:liq3ea@gmail.com>> wrote:
> 
>     Marc-André Lureau <marcandre.lureau@gmail.com
>     <mailto:marcandre.lureau@gmail.com>> 于2021年5月5日周三 下午5:10写道：
>     >
>     > Hi
>     >
>     > On Wed, May 5, 2021 at 9:21 AM Li Qiang <liq3ea@163.com
>     <mailto:liq3ea@163.com>> wrote:
>     >>
>     >> These security issue is low severity and is similar with the
>     >> virtio-vga/virtio-gpu device. All of them can be triggered by
>     >> the guest user.
>     >>
>     >> Li Qiang (7):
>     >>   vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info
>     >>   vhost-user-gpu: fix resource leak in 'vg_resource_create_2d'
>     >>   vhost-user-gpu: fix memory leak in vg_resource_attach_backing
>     >>   vhost-user-gpu: fix memory link while calling 'vg_resource_unref'
>     >>   vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'
>     >>   vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
>     >>   vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset'
>     >>
>     >>  contrib/vhost-user-gpu/vhost-user-gpu.c |  7 +++++++
>     >>  contrib/vhost-user-gpu/virgl.c          | 17 ++++++++++++++++-
>     >>  2 files changed, 23 insertions(+), 1 deletion(-)
>     >>
>     >> --
>     >
>     >
>     > The whole series looks good to me, and applies fixes that were
>     done earlier in virtio-gpu.
> 
>     Do you mean you have merged this series?
>     Should I tweak something such as "adding the original fix in
>     virtio-gpu"/"better mapping iov cleanup"?

Yes, and please also mention the corresponding CVE (CVE-2021-3544,
CVE-2021-3545, CVE-2021-3546).

> 
> 
> No I didn't. I was waiting for the answers to Prasad questions, and
> eventually v2.
> 
> Then either Gerd or me can queue this imho.
>  
> -- 
> Marc-André Lureau

Re: [PATCH 0/7] vhost-user-gpu: fix several security issues

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2021年5月11日周二 上午3:25写道：
>
> On 5/5/21 11:35 AM, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, May 5, 2021 at 1:28 PM Li Qiang <liq3ea@gmail.com
> > <mailto:liq3ea@gmail.com>> wrote:
> >
> >     Marc-André Lureau <marcandre.lureau@gmail.com
> >     <mailto:marcandre.lureau@gmail.com>> 于2021年5月5日周三 下午5:10写道：
> >     >
> >     > Hi
> >     >
> >     > On Wed, May 5, 2021 at 9:21 AM Li Qiang <liq3ea@163.com
> >     <mailto:liq3ea@163.com>> wrote:
> >     >>
> >     >> These security issue is low severity and is similar with the
> >     >> virtio-vga/virtio-gpu device. All of them can be triggered by
> >     >> the guest user.
> >     >>
> >     >> Li Qiang (7):
> >     >>   vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info
> >     >>   vhost-user-gpu: fix resource leak in 'vg_resource_create_2d'
> >     >>   vhost-user-gpu: fix memory leak in vg_resource_attach_backing
> >     >>   vhost-user-gpu: fix memory link while calling 'vg_resource_unref'
> >     >>   vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'
> >     >>   vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
> >     >>   vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset'
> >     >>
> >     >>  contrib/vhost-user-gpu/vhost-user-gpu.c |  7 +++++++
> >     >>  contrib/vhost-user-gpu/virgl.c          | 17 ++++++++++++++++-
> >     >>  2 files changed, 23 insertions(+), 1 deletion(-)
> >     >>
> >     >> --
> >     >
> >     >
> >     > The whole series looks good to me, and applies fixes that were
> >     done earlier in virtio-gpu.
> >
> >     Do you mean you have merged this series?
> >     Should I tweak something such as "adding the original fix in
> >     virtio-gpu"/"better mapping iov cleanup"?
>
> Yes, and please also mention the corresponding CVE (CVE-2021-3544,
> CVE-2021-3545, CVE-2021-3546).
>

OK, I'm still waiting for the some of the patch's response from
Prasad. Kindly ping @Prasad


Thanks,
Li Qiang

> >
> >
> > No I didn't. I was waiting for the answers to Prasad questions, and
> > eventually v2.
> >
> > Then either Gerd or me can queue this imho.
> >
> > --
> > Marc-André Lureau
>