Re: [Qemu-devel] [PATCH] display: virtio-gpu-3d: check virgl capabilities max_size

[P J P <ppandit@redhat.com>]

   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