| Message ID | 20161213071439.32322-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> > From: Prasad J Pandit <pjp@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. > > Reported-by: Zhenhao Hong <zhenhaohong@gmail.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/display/virtio-gpu-3d.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > 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); > resp = g_malloc(sizeof(*resp) + max_size); > - > - resp->hdr.type = VIRTIO_GPU_RESP_OK_CAPSET; > - virgl_renderer_fill_caps(gc.capset_id, > - gc.capset_version, > - (void *)resp->capset_data); > + if (max_size) { > + resp->hdr.type = VIRTIO_GPU_RESP_OK_CAPSET; > + virgl_renderer_fill_caps(gc.capset_id, > + gc.capset_version, > + (void *)resp->capset_data); > + } > virtio_gpu_ctrl_response(g, cmd, &resp->hdr, sizeof(*resp) + max_size); Just one point here for 'max_size=0', do we don't require to set resp->hdr.type? If yes, other patch on list by 'Li Qiang' can also help to avoid sending garbage value. > g_free(resp); > } > -- > 2.9.3 > > >
On Di, 2016-12-13 at 12:44 +0530, P J P wrote: > From: Prasad J Pandit <pjp@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? > 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 ... > resp = g_malloc(sizeof(*resp) + max_size); > - > - resp->hdr.type = VIRTIO_GPU_RESP_OK_CAPSET; > - 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. It's not that we process untrusted data without checking. If a buffer overflow happens here this would clearly be a bug in the virglrenderer library, because the size advertised and the size actually needed mismatch. cheers, Gerd
+-- On Tue, 13 Dec 2016, Pankaj Gupta wrote --+
| > + if (max_size) {
| > + resp->hdr.type = VIRTIO_GPU_RESP_OK_CAPSET;
| > + virgl_renderer_fill_caps(gc.capset_id,
| > + gc.capset_version,
| > + (void *)resp->capset_data);
| > + }
| > virtio_gpu_ctrl_response(g, cmd, &resp->hdr, sizeof(*resp) + max_size);
|
| Just one point here for 'max_size=0', do we don't require to set resp->hdr.type?
I thought hdr.type is if capabilities data is set.
| If yes, other patch on list by 'Li Qiang' can also help to avoid sending garbage value.
g_malloc0 one? Yes.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
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 <pjp@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
> | 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. Ah, ok, that makes sense. I guess we want throw an error (VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER) in the error case then instead of leaving resp->hdr.type unset. cheers, Gerd
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); resp = g_malloc(sizeof(*resp) + max_size); - - resp->hdr.type = VIRTIO_GPU_RESP_OK_CAPSET; - virgl_renderer_fill_caps(gc.capset_id, - gc.capset_version, - (void *)resp->capset_data); + if (max_size) { + resp->hdr.type = VIRTIO_GPU_RESP_OK_CAPSET; + virgl_renderer_fill_caps(gc.capset_id, + gc.capset_version, + (void *)resp->capset_data); + } virtio_gpu_ctrl_response(g, cmd, &resp->hdr, sizeof(*resp) + max_size); g_free(resp); }