diff mbox

display: virtio-gpu-3d: check virgl capabilities max_size

Message ID 20161214070156.23368-1-ppandit@redhat.com
State New
Headers show

Commit Message

Prasad Pandit Dec. 14, 2016, 7:01 a.m. UTC 
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(-)

Comments

Prasad Pandit Dec. 20, 2016, 11:04 a.m. UTC | #1 
+-- 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
Marc-André Lureau Dec. 20, 2016, 11:08 a.m. UTC | #2 
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
Prasad Pandit Dec. 20, 2016, 11:56 a.m. UTC | #3 
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
Marc-André Lureau Dec. 20, 2016, 1:52 p.m. UTC | #4 
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
Gerd Hoffmann Dec. 20, 2016, 2:36 p.m. UTC | #5 
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
diff mbox

Patch

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,