Hello Gerd, +-- On Tue, 13 Dec 2016, Gerd Hoffmann wrote --+ | On Di, 2016-12-13 at 12:44 +0530, P J P wrote: | > From: Prasad J Pandit <p...@fedoraproject.org> | > | > Virtio GPU device while processing 'VIRTIO_GPU_CMD_GET_CAPSET' | > command, retrieves the maximum capabilities size to fill in the | > response object. It continues to fill in capabilities even if | > retrieved 'max_size' is zero(0), thus resulting in OOB access. | > Add check to avoid it. | | Hmm? Did you see this happing in practice?
Yes, there is a reproducer from Zhenhao. | > diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c | > index 758d33a..fbfb39f 100644 | > --- a/hw/display/virtio-gpu-3d.c | > +++ b/hw/display/virtio-gpu-3d.c | > @@ -371,11 +371,12 @@ static void virgl_cmd_get_capset(VirtIOGPU *g, | > virgl_renderer_get_cap_set(gc.capset_id, &max_ver, | > &max_size); | | This is not the guest returning the size, it is the host renderer | library saying how much space it needs ... The library funcion checks 'capset_id' supplied via the 'cmd' object, as 'gc' is initialised from a given command 'cmd'. It sets 'max_size=0' if capset_id != VREND_CAP_SET. VIRTIO_GPU_FILL_CMD(gc) virgl_renderer_get_cap_set(gc.capset_id -> vrend_renderer_get_cap_set -> https://cgit.freedesktop.org/~airlied/virglrenderer/tree/src/vrend_renderer.c#n6280 if (cap_set != VREND_CAP_SET) { *max_ver = 0; *max_size = 0; return; } | > - virgl_renderer_fill_caps(gc.capset_id, | > - gc.capset_version, | > - (void *)resp->capset_data); | | ... and here the renderer fills the qemu-allocated space with the actual | data. | | Can't see anything wrong here. IIUC, when max_size=0, g_malloc allocates memory for the 'resp' object, with sizeof(capset_data)=0. But 'virgl_renderer_fill_caps' would try to fill in upto sizeof(union virgl_caps). -> https://cgit.freedesktop.org/~airlied/virglrenderer/tree/src/vrend_renderer.c#n5940 Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
