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

[PATCH v2 0/8] 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.

Change since v1:
Add the CVE information:
Several memory leak issues is combined assigned CVE-2021-3544.
Information disclosure is assigned CVE-2021-3545.
OOB memory write access is assigned CVE-2021-3545.

Abstract 'vg_cleanup_mapping_iov' to make code more clearly.

Li Qiang (8):
  vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info
    (CVE-2021-3545)
  vhost-user-gpu: fix resource leak in 'vg_resource_create_2d'
    (CVE-2021-3544)
  vhost-user-gpu: fix memory leak in vg_resource_attach_backing
    (CVE-2021-3544)
  vhost-user-gpu: fix memory leak while calling 'vg_resource_unref'
    (CVE-2021-3544)
  vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'
    (CVE-2021-3544)
  vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
    (CVE-2021-3544)
  vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset'
    (CVE-2021-3546)
  vhost-user-gpu: abstract vg_cleanup_mapping_iov

 contrib/vhost-user-gpu/vhost-user-gpu.c | 29 ++++++++++++++++++++++---
 contrib/vhost-user-gpu/virgl.c          | 20 +++++++++++++++--
 contrib/vhost-user-gpu/vugpu.h          |  2 +-
 3 files changed, 45 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH v2 1/8] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info (CVE-2021-3545)

[Li Qiang]

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

Fixes: CVE-2021-3545
Reported-by: Li Qiang <liq3ea@163.com>
virtio-gpu fix: 42a8dadc74 ("virtio-gpu: fix information leak
in getting capset info dispatch")

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

[PATCH v2 2/8] vhost-user-gpu: fix resource leak in 'vg_resource_create_2d' (CVE-2021-3544)

[Li Qiang]

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

Fixes: CVE-2021-3544
Reported-by: Li Qiang <liq3ea@163.com>
Reviewed-by: Prasad J Pandit <pjp@fedoraproject.org>
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

[PATCH v2 3/8] vhost-user-gpu: fix memory leak in vg_resource_attach_backing (CVE-2021-3544)

[Li Qiang]

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

Fixes: CVE-2021-3544
Reported-by: Li Qiang <liq3ea@163.com>
virtio-gpu fix: 204f01b309 ("virtio-gpu: fix memory leak
in resource attach backing")

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

[PATCH v2 4/8] vhost-user-gpu: fix memory leak while calling 'vg_resource_unref' (CVE-2021-3544)

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

Fixes: CVE-2021-3544
Reported-by: Li Qiang <liq3ea@163.com>
virtio-gpu fix: 5e8e3c4c75 ("virtio-gpu: fix resource leak
in virgl_cmd_resource_unref")

Reviewed-by: Prasad J Pandit <pjp@fedoraproject.org>
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

[PATCH v2 5/8] vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref' (CVE-2021-3544)

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

Fixes: CVE-2021-3544
Reported-by: Li Qiang <liq3ea@163.com>
virtio-gpu fix: 5e8e3c4c75 ("virtio-gpu: fix resource leak
in virgl_cmd_resource_unref"

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

[PATCH v2 6/8] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing' (CVE-2021-3544)

[Li Qiang]

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

Fixes: CVE-2021-3544
Reported-by: Li Qiang <liq3ea@163.com>
virtio-gpu fix: 33243031da ("virtio-gpu-3d: fix memory leak
in resource attach backing")

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

[PATCH v2 7/8] vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset' (CVE-2021-3546)

[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'.

virtio-gpu fix: abd7f08b23 ("display: virtio-gpu-3d: check
virgl capabilities max_size")

Fixes: CVE-2021-3546
Reported-by: Li Qiang <liq3ea@163.com>
Reviewed-by: Prasad J Pandit <pjp@fedoraproject.org>
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

[PATCH v2 8/8] vhost-user-gpu: abstract vg_cleanup_mapping_iov

[Li Qiang]

Currently in vhost-user-gpu, we free resource directly in
the cleanup case of resource. If we change the cleanup logic
we need to change several places, also abstruct a
'vg_create_mapping_iov' can be symmetry with the
'vg_create_mapping_iov'. This is like what virtio-gpu does,
no function changed.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 contrib/vhost-user-gpu/vhost-user-gpu.c | 24 ++++++++++++++++++++----
 contrib/vhost-user-gpu/virgl.c          |  9 +++++----
 contrib/vhost-user-gpu/vugpu.h          |  2 +-
 3 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c b/contrib/vhost-user-gpu/vhost-user-gpu.c
index 770dfad529..6dc6a44f4e 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -49,6 +49,8 @@ static char *opt_render_node;
 static gboolean opt_virgl;
 
 static void vg_handle_ctrl(VuDev *dev, int qidx);
+static void vg_cleanup_mapping(VuGpu *g,
+                               struct virtio_gpu_simple_resource *res);
 
 static const char *
 vg_cmd_to_string(int cmd)
@@ -400,7 +402,7 @@ vg_resource_destroy(VuGpu *g,
     }
 
     vugbm_buffer_destroy(&res->buffer);
-    g_free(res->iov);
+    vg_cleanup_mapping(g, res);
     pixman_image_unref(res->image);
     QTAILQ_REMOVE(&g->reslist, res, next);
     g_free(res);
@@ -504,6 +506,22 @@ vg_resource_attach_backing(VuGpu *g,
     res->iov_cnt = ab.nr_entries;
 }
 
+/* Though currently only free iov, maybe later will do more work. */
+void vg_cleanup_mapping_iov(VuGpu *g,
+                            struct iovec *iov, uint32_t count)
+{
+    g_free(iov);
+}
+
+static void
+vg_cleanup_mapping(VuGpu *g,
+                   struct virtio_gpu_simple_resource *res)
+{
+    vg_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
+    res->iov = NULL;
+    res->iov_cnt = 0;
+}
+
 static void
 vg_resource_detach_backing(VuGpu *g,
                            struct virtio_gpu_ctrl_command *cmd)
@@ -522,9 +540,7 @@ vg_resource_detach_backing(VuGpu *g,
         return;
     }
 
-    g_free(res->iov);
-    res->iov = NULL;
-    res->iov_cnt = 0;
+    vg_cleanup_mapping(g, res);
 }
 
 static void
diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
index 7172104b19..3e45e1bd33 100644
--- a/contrib/vhost-user-gpu/virgl.c
+++ b/contrib/vhost-user-gpu/virgl.c
@@ -116,8 +116,9 @@ virgl_cmd_resource_unref(VuGpu *g,
     virgl_renderer_resource_detach_iov(unref.resource_id,
                                        &res_iovs,
                                        &num_iovs);
-    g_free(res_iovs);
-
+    if (res_iovs != NULL && num_iovs != 0) {
+        vg_cleanup_mapping_iov(g, res_iovs, num_iovs);
+    }
     virgl_renderer_resource_unref(unref.resource_id);
 }
 
@@ -294,7 +295,7 @@ virgl_resource_attach_backing(VuGpu *g,
     ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
                                        res_iovs, att_rb.nr_entries);
     if (ret != 0) {
-        g_free(res_iovs);
+        vg_cleanup_mapping_iov(g, res_iovs, att_rb.nr_entries);
     }
 }
 
@@ -314,7 +315,7 @@ virgl_resource_detach_backing(VuGpu *g,
     if (res_iovs == NULL || num_iovs == 0) {
         return;
     }
-    g_free(res_iovs);
+    vg_cleanup_mapping_iov(g, res_iovs, num_iovs);
 }
 
 static void
diff --git a/contrib/vhost-user-gpu/vugpu.h b/contrib/vhost-user-gpu/vugpu.h
index 04d5615812..e2864bba68 100644
--- a/contrib/vhost-user-gpu/vugpu.h
+++ b/contrib/vhost-user-gpu/vugpu.h
@@ -169,7 +169,7 @@ int     vg_create_mapping_iov(VuGpu *g,
                               struct virtio_gpu_resource_attach_backing *ab,
                               struct virtio_gpu_ctrl_command *cmd,
                               struct iovec **iov);
-
+void    vg_cleanup_mapping_iov(VuGpu *g, struct iovec *iov, uint32_t count);
 void    vg_get_display_info(VuGpu *vg, struct virtio_gpu_ctrl_command *cmd);
 
 void    vg_wait_ok(VuGpu *g);
-- 
2.25.1

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

[Marc-André Lureau]

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

Hi

On Sun, May 16, 2021 at 7:04 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.
>
> Change since v1:
> Add the CVE information:
> Several memory leak issues is combined assigned CVE-2021-3544.
> Information disclosure is assigned CVE-2021-3545.
> OOB memory write access is assigned CVE-2021-3545.
>
> Abstract 'vg_cleanup_mapping_iov' to make code more clearly.
>
> Li Qiang (8):
>   vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info
>     (CVE-2021-3545)
>   vhost-user-gpu: fix resource leak in 'vg_resource_create_2d'
>     (CVE-2021-3544)
>   vhost-user-gpu: fix memory leak in vg_resource_attach_backing
>     (CVE-2021-3544)
>   vhost-user-gpu: fix memory leak while calling 'vg_resource_unref'
>     (CVE-2021-3544)
>   vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'
>     (CVE-2021-3544)
>   vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
>     (CVE-2021-3544)
>   vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset'
>     (CVE-2021-3546)
>   vhost-user-gpu: abstract vg_cleanup_mapping_iov
>
>  contrib/vhost-user-gpu/vhost-user-gpu.c | 29 ++++++++++++++++++++++---
>  contrib/vhost-user-gpu/virgl.c          | 20 +++++++++++++++--
>  contrib/vhost-user-gpu/vugpu.h          |  2 +-
>  3 files changed, 45 insertions(+), 6 deletions(-)
>
> --
> 2.25.1
>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

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

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

[Gerd Hoffmann]

On Sat, May 15, 2021 at 08:03:55PM -0700, Li Qiang 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.
> 
> Change since v1:
> Add the CVE information:
> Several memory leak issues is combined assigned CVE-2021-3544.
> Information disclosure is assigned CVE-2021-3545.
> OOB memory write access is assigned CVE-2021-3545.
> 
> Abstract 'vg_cleanup_mapping_iov' to make code more clearly.

Added all ti vga queue.

thanks,
  Gerd