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

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

[P J P]

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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c
index 758d33a..6ceeba3 100644
--- a/hw/display/virtio-gpu-3d.c
+++ b/hw/display/virtio-gpu-3d.c
@@ -370,8 +370,12 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
 
     virgl_renderer_get_cap_set(gc.capset_id, &max_ver,
                                &max_size);
+    if (!max_size) {
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+        return;
+    }
+
     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,
-- 
2.9.3

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

[P J P]

+-- On Wed, 14 Dec 2016, 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.
| 
| Reported-by: Zhenhao Hong <zhenhaohong@gmail.com>
| Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
| ---
|  hw/display/virtio-gpu-3d.c | 6 +++++-
|  1 file changed, 5 insertions(+), 1 deletion(-)
| 
| diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c
| index 758d33a..6ceeba3 100644
| --- a/hw/display/virtio-gpu-3d.c
| +++ b/hw/display/virtio-gpu-3d.c
| @@ -370,8 +370,12 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
|  
|      virgl_renderer_get_cap_set(gc.capset_id, &max_ver,
|                                 &max_size);
| +    if (!max_size) {
| +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
| +        return;
| +    }
| +
|      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,
| 

Ping..!
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

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

[Marc-André Lureau]

Hi

On Wed, Dec 14, 2016 at 8:02 AM P J P <ppandit@redhat.com> 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.
>
> Reported-by: Zhenhao Hong <zhenhaohong@gmail.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/display/virtio-gpu-3d.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c
> index 758d33a..6ceeba3 100644
> --- a/hw/display/virtio-gpu-3d.c
> +++ b/hw/display/virtio-gpu-3d.c
> @@ -370,8 +370,12 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
>
>      virgl_renderer_get_cap_set(gc.capset_id, &max_ver,
>                                 &max_size);
> +    if (!max_size) {
>

Shouldn't it check for >= sizeof(union virgl_caps) ? (since that's what
virglrenderer vrend_renderer_fill_caps() expects)


+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
> +        return;
> +    }
> +
>      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,
> --
> 2.9.3
>
> --
Marc-André Lureau

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

[P J P]

  Hello Marc,

+-- On Tue, 20 Dec 2016, Marc-André Lureau wrote --+
| > +    if (!max_size) {
| 
| Shouldn't it check for >= sizeof(union virgl_caps) ? (since that's what
| virglrenderer vrend_renderer_fill_caps() expects)

  No, 'max_size' isn't set by a user, it's set by the library function 
'vrend_renderer_get_cap_set'.

  -> https://cgit.freedesktop.org/~airlied/virglrenderer/tree/src/vrend_renderer.c#n6280

And if not zero, it'll be set to sizeof(union virgl_caps).

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

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

[Marc-André Lureau]

Hi

On Tue, Dec 20, 2016 at 12:56 PM P J P <ppandit@redhat.com> wrote:

>   Hello Marc,
>
> +-- On Tue, 20 Dec 2016, Marc-André Lureau wrote --+
> | > +    if (!max_size) {
> |
> | Shouldn't it check for >= sizeof(union virgl_caps) ? (since that's what
> | virglrenderer vrend_renderer_fill_caps() expects)
>
>   No, 'max_size' isn't set by a user, it's set by the library function
> 'vrend_renderer_get_cap_set'.
>
>   ->
> https://cgit.freedesktop.org/~airlied/virglrenderer/tree/src/vrend_renderer.c#n6280
>
> And if not zero, it'll be set to sizeof(union virgl_caps).
>

ok

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

-- 
Marc-André Lureau

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

[Gerd Hoffmann]

On Di, 2016-12-20 at 16:34 +0530, P J P wrote:
> 
> Ping..!

Queued up now.  But I don't feel like sending a pull request hours
before disappearing into the xmas holidays, so that'll happen in
january.

cheers,
  Gerd

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

[P J P]

+-- On Tue, 13 Dec 2016, Marc-André Lureau wrote --+
| > -    resp = g_malloc(sizeof(*resp) + max_size);
| > +    if (!max_size) {
| > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
| > +        return;
| > +    }
| >
| > +    resp = g_malloc(sizeof(*resp) + max_size);
| >      resp->hdr.type = VIRTIO_GPU_RESP_OK_CAPSET;
|
| That looks good to me, waiting for the proper patch.

Okay, sent. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

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

[Marc-André Lureau]

Hi

On Tue, Dec 13, 2016 at 5:27 PM P J P <ppandit@redhat.com> wrote:

> +-- On Tue, 13 Dec 2016, Gerd Hoffmann wrote --+
> | 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.
>
>   I see, okay. Does the patch below look okay?
>
> ===
> diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c
> index 758d33a..6ceeba3 100644
> --- a/hw/display/virtio-gpu-3d.c
> +++ b/hw/display/virtio-gpu-3d.c
> @@ -370,8 +370,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);
> +    if (!max_size) {
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
> +        return;
> +    }
>
> +    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,
> ===
>
>
That looks good to me, waiting for the proper patch.


> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>
> --
Marc-André Lureau

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

[P J P]

+-- On Tue, 13 Dec 2016, Gerd Hoffmann wrote --+
| 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.

  I see, okay. Does the patch below look okay?

===
diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c
index 758d33a..6ceeba3 100644
--- a/hw/display/virtio-gpu-3d.c
+++ b/hw/display/virtio-gpu-3d.c
@@ -370,8 +370,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);
+    if (!max_size) {
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
+        return;
+    }
 
+    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,
===

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

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

[Gerd Hoffmann]

> | 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

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

[P J P]

   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

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

[P J P]

+-- 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

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

[Gerd Hoffmann]

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

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

[Pankaj Gupta]

> 
> 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
> 
> 
> 

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

[P J P]

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);
     g_free(resp);
 }
-- 
2.9.3