* [PATCH 0/7] vhost-user-gpu: fix several security issues
@ 2021-05-05 4:58 Li Qiang
2021-05-05 4:58 ` [PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info Li Qiang
` (7 more replies)
0 siblings, 8 replies; 28+ messages in thread
From: Li Qiang @ 2021-05-05 4:58 UTC (permalink / raw)
To: marcandre.lureau, kraxel, qemu-devel; +Cc: Li Qiang, liq3ea
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info
2021-05-05 4:58 [PATCH 0/7] vhost-user-gpu: fix several security issues Li Qiang
@ 2021-05-05 4:58 ` Li Qiang
2021-05-05 7:23 ` P J P
2021-05-05 4:58 ` [PATCH 2/7] vhost-user-gpu: fix resource leak in 'vg_resource_create_2d' Li Qiang
` (6 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Li Qiang @ 2021-05-05 4:58 UTC (permalink / raw)
To: marcandre.lureau, kraxel, qemu-devel; +Cc: Li Qiang, liq3ea
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/7] vhost-user-gpu: fix resource leak in 'vg_resource_create_2d'
2021-05-05 4:58 [PATCH 0/7] vhost-user-gpu: fix several security issues Li Qiang
2021-05-05 4:58 ` [PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info Li Qiang
@ 2021-05-05 4:58 ` Li Qiang
2021-05-05 7:28 ` P J P
2021-05-05 4:58 ` [PATCH 3/7] vhost-user-gpu: fix memory leak in vg_resource_attach_backing Li Qiang
` (5 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Li Qiang @ 2021-05-05 4:58 UTC (permalink / raw)
To: marcandre.lureau, kraxel, qemu-devel; +Cc: Li Qiang, liq3ea
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 3/7] vhost-user-gpu: fix memory leak in vg_resource_attach_backing
2021-05-05 4:58 [PATCH 0/7] vhost-user-gpu: fix several security issues Li Qiang
2021-05-05 4:58 ` [PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info Li Qiang
2021-05-05 4:58 ` [PATCH 2/7] vhost-user-gpu: fix resource leak in 'vg_resource_create_2d' Li Qiang
@ 2021-05-05 4:58 ` Li Qiang
2021-05-05 7:39 ` P J P
2021-05-05 4:58 ` [PATCH 4/7] vhost-user-gpu: fix memory link while calling 'vg_resource_unref' Li Qiang
` (4 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Li Qiang @ 2021-05-05 4:58 UTC (permalink / raw)
To: marcandre.lureau, kraxel, qemu-devel; +Cc: Li Qiang, liq3ea
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 4/7] vhost-user-gpu: fix memory link while calling 'vg_resource_unref'
2021-05-05 4:58 [PATCH 0/7] vhost-user-gpu: fix several security issues Li Qiang
` (2 preceding siblings ...)
2021-05-05 4:58 ` [PATCH 3/7] vhost-user-gpu: fix memory leak in vg_resource_attach_backing Li Qiang
@ 2021-05-05 4:58 ` Li Qiang
2021-05-05 7:42 ` P J P
2021-05-05 4:58 ` [PATCH 5/7] vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref' Li Qiang
` (3 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Li Qiang @ 2021-05-05 4:58 UTC (permalink / raw)
To: marcandre.lureau, kraxel, qemu-devel; +Cc: Li Qiang, liq3ea
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 5/7] vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'
2021-05-05 4:58 [PATCH 0/7] vhost-user-gpu: fix several security issues Li Qiang
` (3 preceding siblings ...)
2021-05-05 4:58 ` [PATCH 4/7] vhost-user-gpu: fix memory link while calling 'vg_resource_unref' Li Qiang
@ 2021-05-05 4:58 ` Li Qiang
2021-05-05 7:48 ` P J P
2021-05-05 4:58 ` [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing' Li Qiang
` (2 subsequent siblings)
7 siblings, 1 reply; 28+ messages in thread
From: Li Qiang @ 2021-05-05 4:58 UTC (permalink / raw)
To: marcandre.lureau, kraxel, qemu-devel; +Cc: Li Qiang, liq3ea
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
2021-05-05 4:58 [PATCH 0/7] vhost-user-gpu: fix several security issues Li Qiang
` (4 preceding siblings ...)
2021-05-05 4:58 ` [PATCH 5/7] vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref' Li Qiang
@ 2021-05-05 4:58 ` Li Qiang
2021-05-05 7:55 ` P J P
2021-05-05 4:58 ` [PATCH 7/7] vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset' Li Qiang
2021-05-05 9:10 ` [PATCH 0/7] vhost-user-gpu: fix several security issues Marc-André Lureau
7 siblings, 1 reply; 28+ messages in thread
From: Li Qiang @ 2021-05-05 4:58 UTC (permalink / raw)
To: marcandre.lureau, kraxel, qemu-devel; +Cc: Li Qiang, liq3ea
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 7/7] vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset'
2021-05-05 4:58 [PATCH 0/7] vhost-user-gpu: fix several security issues Li Qiang
` (5 preceding siblings ...)
2021-05-05 4:58 ` [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing' Li Qiang
@ 2021-05-05 4:58 ` Li Qiang
2021-05-05 7:58 ` P J P
2021-05-05 9:10 ` [PATCH 0/7] vhost-user-gpu: fix several security issues Marc-André Lureau
7 siblings, 1 reply; 28+ messages in thread
From: Li Qiang @ 2021-05-05 4:58 UTC (permalink / raw)
To: marcandre.lureau, kraxel, qemu-devel; +Cc: Li Qiang, liq3ea
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info
2021-05-05 4:58 ` [PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info Li Qiang
@ 2021-05-05 7:23 ` P J P
2021-05-05 9:07 ` Li Qiang
0 siblings, 1 reply; 28+ messages in thread
From: P J P @ 2021-05-05 7:23 UTC (permalink / raw)
To: Li Qiang; +Cc: marcandre.lureau, liq3ea, kraxel, qemu-devel
+-- 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/7] vhost-user-gpu: fix resource leak in 'vg_resource_create_2d'
2021-05-05 4:58 ` [PATCH 2/7] vhost-user-gpu: fix resource leak in 'vg_resource_create_2d' Li Qiang
@ 2021-05-05 7:28 ` P J P
0 siblings, 0 replies; 28+ messages in thread
From: P J P @ 2021-05-05 7:28 UTC (permalink / raw)
To: Li Qiang; +Cc: marcandre.lureau, liq3ea, kraxel, qemu-devel
+-- 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/7] vhost-user-gpu: fix memory leak in vg_resource_attach_backing
2021-05-05 4:58 ` [PATCH 3/7] vhost-user-gpu: fix memory leak in vg_resource_attach_backing Li Qiang
@ 2021-05-05 7:39 ` P J P
2021-05-05 9:11 ` Li Qiang
0 siblings, 1 reply; 28+ messages in thread
From: P J P @ 2021-05-05 7:39 UTC (permalink / raw)
To: Li Qiang; +Cc: marcandre.lureau, liq3ea, kraxel, qemu-devel
+-- 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/7] vhost-user-gpu: fix memory link while calling 'vg_resource_unref'
2021-05-05 4:58 ` [PATCH 4/7] vhost-user-gpu: fix memory link while calling 'vg_resource_unref' Li Qiang
@ 2021-05-05 7:42 ` P J P
0 siblings, 0 replies; 28+ messages in thread
From: P J P @ 2021-05-05 7:42 UTC (permalink / raw)
To: Li Qiang; +Cc: marcandre.lureau, liq3ea, kraxel, qemu-devel
+-- 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/7] vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'
2021-05-05 4:58 ` [PATCH 5/7] vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref' Li Qiang
@ 2021-05-05 7:48 ` P J P
2021-05-05 9:14 ` Li Qiang
0 siblings, 1 reply; 28+ messages in thread
From: P J P @ 2021-05-05 7:48 UTC (permalink / raw)
To: Li Qiang; +Cc: marcandre.lureau, liq3ea, kraxel, qemu-devel
+-- 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
2021-05-05 4:58 ` [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing' Li Qiang
@ 2021-05-05 7:55 ` P J P
2021-05-05 9:07 ` Marc-André Lureau
0 siblings, 1 reply; 28+ messages in thread
From: P J P @ 2021-05-05 7:55 UTC (permalink / raw)
To: Li Qiang; +Cc: marcandre.lureau, liq3ea, kraxel, qemu-devel
+-- 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 7/7] vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset'
2021-05-05 4:58 ` [PATCH 7/7] vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset' Li Qiang
@ 2021-05-05 7:58 ` P J P
0 siblings, 0 replies; 28+ messages in thread
From: P J P @ 2021-05-05 7:58 UTC (permalink / raw)
To: Li Qiang; +Cc: marcandre.lureau, liq3ea, kraxel, qemu-devel
+-- 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info
2021-05-05 7:23 ` P J P
@ 2021-05-05 9:07 ` Li Qiang
2021-05-06 5:53 ` P J P
0 siblings, 1 reply; 28+ messages in thread
From: Li Qiang @ 2021-05-05 9:07 UTC (permalink / raw)
To: P J P; +Cc: marcandre lureau, Li Qiang, Gerd Hoffmann, Qemu Developers
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
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
2021-05-05 7:55 ` P J P
@ 2021-05-05 9:07 ` Marc-André Lureau
2021-05-05 9:23 ` Li Qiang
0 siblings, 1 reply; 28+ messages in thread
From: Marc-André Lureau @ 2021-05-05 9:07 UTC (permalink / raw)
To: P J P; +Cc: Li Qiang, Li Qiang, Gerd Hoffmann, QEMU
[-- 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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/7] vhost-user-gpu: fix several security issues
2021-05-05 4:58 [PATCH 0/7] vhost-user-gpu: fix several security issues Li Qiang
` (6 preceding siblings ...)
2021-05-05 4:58 ` [PATCH 7/7] vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset' Li Qiang
@ 2021-05-05 9:10 ` Marc-André Lureau
2021-05-05 9:27 ` Li Qiang
7 siblings, 1 reply; 28+ messages in thread
From: Marc-André Lureau @ 2021-05-05 9:10 UTC (permalink / raw)
To: Li Qiang; +Cc: Li Qiang, Gerd Hoffmann, QEMU
[-- 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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/7] vhost-user-gpu: fix memory leak in vg_resource_attach_backing
2021-05-05 7:39 ` P J P
@ 2021-05-05 9:11 ` Li Qiang
0 siblings, 0 replies; 28+ messages in thread
From: Li Qiang @ 2021-05-05 9:11 UTC (permalink / raw)
To: P J P; +Cc: marcandre lureau, Li Qiang, Gerd Hoffmann, Qemu Developers
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
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/7] vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref'
2021-05-05 7:48 ` P J P
@ 2021-05-05 9:14 ` Li Qiang
0 siblings, 0 replies; 28+ messages in thread
From: Li Qiang @ 2021-05-05 9:14 UTC (permalink / raw)
To: P J P; +Cc: marcandre lureau, Li Qiang, Gerd Hoffmann, Qemu Developers
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
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
2021-05-05 9:07 ` Marc-André Lureau
@ 2021-05-05 9:23 ` Li Qiang
2021-05-05 9:33 ` Marc-André Lureau
0 siblings, 1 reply; 28+ messages in thread
From: Li Qiang @ 2021-05-05 9:23 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: QEMU, Li Qiang, Gerd Hoffmann, P J P
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!
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/7] vhost-user-gpu: fix several security issues
2021-05-05 9:10 ` [PATCH 0/7] vhost-user-gpu: fix several security issues Marc-André Lureau
@ 2021-05-05 9:27 ` Li Qiang
2021-05-05 9:35 ` Marc-André Lureau
0 siblings, 1 reply; 28+ messages in thread
From: Li Qiang @ 2021-05-05 9:27 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Li Qiang, Gerd Hoffmann, QEMU
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing'
2021-05-05 9:23 ` Li Qiang
@ 2021-05-05 9:33 ` Marc-André Lureau
0 siblings, 0 replies; 28+ messages in thread
From: Marc-André Lureau @ 2021-05-05 9:33 UTC (permalink / raw)
To: Li Qiang; +Cc: QEMU, Li Qiang, Gerd Hoffmann, P J P
[-- 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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/7] vhost-user-gpu: fix several security issues
2021-05-05 9:27 ` Li Qiang
@ 2021-05-05 9:35 ` Marc-André Lureau
2021-05-10 19:25 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 28+ messages in thread
From: Marc-André Lureau @ 2021-05-05 9:35 UTC (permalink / raw)
To: Li Qiang; +Cc: Li Qiang, Gerd Hoffmann, QEMU
[-- 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 --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info
2021-05-05 9:07 ` Li Qiang
@ 2021-05-06 5:53 ` P J P
2021-05-06 7:49 ` Li Qiang
0 siblings, 1 reply; 28+ messages in thread
From: P J P @ 2021-05-06 5:53 UTC (permalink / raw)
To: Li Qiang; +Cc: marcandre lureau, Li Qiang, Gerd Hoffmann, Qemu Developers
[-- 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info
2021-05-06 5:53 ` P J P
@ 2021-05-06 7:49 ` Li Qiang
0 siblings, 0 replies; 28+ messages in thread
From: Li Qiang @ 2021-05-06 7:49 UTC (permalink / raw)
To: P J P; +Cc: marcandre lureau, Li Qiang, Gerd Hoffmann, Qemu Developers
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/7] vhost-user-gpu: fix several security issues
2021-05-05 9:35 ` Marc-André Lureau
@ 2021-05-10 19:25 ` Philippe Mathieu-Daudé
2021-05-11 2:49 ` Li Qiang
0 siblings, 1 reply; 28+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-10 19:25 UTC (permalink / raw)
To: Marc-André Lureau, Li Qiang; +Cc: Li Qiang, Gerd Hoffmann, QEMU
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/7] vhost-user-gpu: fix several security issues
2021-05-10 19:25 ` Philippe Mathieu-Daudé
@ 2021-05-11 2:49 ` Li Qiang
0 siblings, 0 replies; 28+ messages in thread
From: Li Qiang @ 2021-05-11 2:49 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Prasad J Pandit
Cc: Li Qiang, Marc-André Lureau, Gerd Hoffmann, QEMU
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
>
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2021-05-11 2:51 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-05 4:58 [PATCH 0/7] vhost-user-gpu: fix several security issues Li Qiang
2021-05-05 4:58 ` [PATCH 1/7] vhost-user-gpu: fix memory disclosure in virgl_cmd_get_capset_info Li Qiang
2021-05-05 7:23 ` P J P
2021-05-05 9:07 ` Li Qiang
2021-05-06 5:53 ` P J P
2021-05-06 7:49 ` Li Qiang
2021-05-05 4:58 ` [PATCH 2/7] vhost-user-gpu: fix resource leak in 'vg_resource_create_2d' Li Qiang
2021-05-05 7:28 ` P J P
2021-05-05 4:58 ` [PATCH 3/7] vhost-user-gpu: fix memory leak in vg_resource_attach_backing Li Qiang
2021-05-05 7:39 ` P J P
2021-05-05 9:11 ` Li Qiang
2021-05-05 4:58 ` [PATCH 4/7] vhost-user-gpu: fix memory link while calling 'vg_resource_unref' Li Qiang
2021-05-05 7:42 ` P J P
2021-05-05 4:58 ` [PATCH 5/7] vhost-user-gpu: fix memory leak in 'virgl_cmd_resource_unref' Li Qiang
2021-05-05 7:48 ` P J P
2021-05-05 9:14 ` Li Qiang
2021-05-05 4:58 ` [PATCH 6/7] vhost-user-gpu: fix memory leak in 'virgl_resource_attach_backing' Li Qiang
2021-05-05 7:55 ` P J P
2021-05-05 9:07 ` Marc-André Lureau
2021-05-05 9:23 ` Li Qiang
2021-05-05 9:33 ` Marc-André Lureau
2021-05-05 4:58 ` [PATCH 7/7] vhost-user-gpu: fix OOB write in 'virgl_cmd_get_capset' Li Qiang
2021-05-05 7:58 ` P J P
2021-05-05 9:10 ` [PATCH 0/7] vhost-user-gpu: fix several security issues Marc-André Lureau
2021-05-05 9:27 ` Li Qiang
2021-05-05 9:35 ` Marc-André Lureau
2021-05-10 19:25 ` Philippe Mathieu-Daudé
2021-05-11 2:49 ` Li Qiang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.