| Message ID | 20210505045824.33880-7-liq3ea@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | vhost-user-gpu: fix several security issues | expand |
+-- 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
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!
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! >
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! > > >
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
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(-)