[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] display: virtio-gpu-3d: check virgl capabilitie
From: |
P J P |
Subject: |
Re: [Qemu-devel] [PATCH] display: virtio-gpu-3d: check virgl capabilities max_size |
Date: |
Tue, 13 Dec 2016 17:20:51 +0530 (IST) |
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 <address@hidden>
| >
| > 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