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