[PATCH] virtio-gpu: move scanout_id sanity check

Gerd Hoffmann posted 1 patch 2 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/next-importer-push tags/patchew/20210604075029.1201478-1-kraxel@redhat.com
Maintainers: Gerd Hoffmann <kraxel@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>
hw/display/virtio-gpu.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
[PATCH] virtio-gpu: move scanout_id sanity check
Posted by Gerd Hoffmann 2 years, 9 months ago
Checking scanout_id in virtio_gpu_do_set_scanout() is too late, for the
"resource_id == 0" case (aka disable scanout) the scanout_id is used
unchecked.  Move the check into the callers to fix that.

Fixes: e64d4b6a9bc3 ("virtio-gpu: Refactor virtio_gpu_set_scanout")
Fixes: 32db3c63ae11 ("virtio-gpu: Add virtio_gpu_set_scanout_blob")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/383
Reported-by: Alexander Bulekov <alxndr@bu.edu>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/virtio-gpu.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 4d549377cbc1..e183f4ecdaa5 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -610,12 +610,6 @@ static void virtio_gpu_do_set_scanout(VirtIOGPU *g,
     struct virtio_gpu_scanout *scanout;
     uint8_t *data;
 
-    if (scanout_id >= g->parent_obj.conf.max_outputs) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
-                      __func__, scanout_id);
-        *error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
-        return;
-    }
     scanout = &g->parent_obj.scanout[scanout_id];
 
     if (r->x > fb->width ||
@@ -694,6 +688,13 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
     trace_virtio_gpu_cmd_set_scanout(ss.scanout_id, ss.resource_id,
                                      ss.r.width, ss.r.height, ss.r.x, ss.r.y);
 
+    if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
+                      __func__, ss.scanout_id);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
+        return;
+    }
+
     if (ss.resource_id == 0) {
         virtio_gpu_disable_scanout(g, ss.scanout_id);
         return;
@@ -730,6 +731,13 @@ static void virtio_gpu_set_scanout_blob(VirtIOGPU *g,
                                           ss.r.width, ss.r.height, ss.r.x,
                                           ss.r.y);
 
+    if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
+                      __func__, ss.scanout_id);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
+        return;
+    }
+
     if (ss.resource_id == 0) {
         virtio_gpu_disable_scanout(g, ss.scanout_id);
         return;
-- 
2.31.1


RE: [PATCH] virtio-gpu: move scanout_id sanity check
Posted by Kasireddy, Vivek 2 years, 9 months ago
Reviewed-by: Vivek Kasireddy <vivek.kasireddy@intel.com>


> -----Original Message-----
> From: Qemu-devel <qemu-devel-bounces+vivek.kasireddy=intel.com@nongnu.org> On
> Behalf Of Gerd Hoffmann
> Sent: Friday, June 04, 2021 12:50 AM
> To: qemu-devel@nongnu.org
> Cc: Alexander Bulekov <alxndr@bu.edu>; Gerd Hoffmann <kraxel@redhat.com>;
> Michael S. Tsirkin <mst@redhat.com>
> Subject: [PATCH] virtio-gpu: move scanout_id sanity check
> 
> Checking scanout_id in virtio_gpu_do_set_scanout() is too late, for the
> "resource_id == 0" case (aka disable scanout) the scanout_id is used
> unchecked.  Move the check into the callers to fix that.
> 
> Fixes: e64d4b6a9bc3 ("virtio-gpu: Refactor virtio_gpu_set_scanout")
> Fixes: 32db3c63ae11 ("virtio-gpu: Add virtio_gpu_set_scanout_blob")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/383
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/virtio-gpu.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 4d549377cbc1..e183f4ecdaa5 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -610,12 +610,6 @@ static void virtio_gpu_do_set_scanout(VirtIOGPU *g,
>      struct virtio_gpu_scanout *scanout;
>      uint8_t *data;
> 
> -    if (scanout_id >= g->parent_obj.conf.max_outputs) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
> -                      __func__, scanout_id);
> -        *error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
> -        return;
> -    }
>      scanout = &g->parent_obj.scanout[scanout_id];
> 
>      if (r->x > fb->width ||
> @@ -694,6 +688,13 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
>      trace_virtio_gpu_cmd_set_scanout(ss.scanout_id, ss.resource_id,
>                                       ss.r.width, ss.r.height, ss.r.x, ss.r.y);
> 
> +    if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
> +                      __func__, ss.scanout_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
> +        return;
> +    }
> +
>      if (ss.resource_id == 0) {
>          virtio_gpu_disable_scanout(g, ss.scanout_id);
>          return;
> @@ -730,6 +731,13 @@ static void virtio_gpu_set_scanout_blob(VirtIOGPU *g,
>                                            ss.r.width, ss.r.height, ss.r.x,
>                                            ss.r.y);
> 
> +    if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
> +                      __func__, ss.scanout_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
> +        return;
> +    }
> +
>      if (ss.resource_id == 0) {
>          virtio_gpu_disable_scanout(g, ss.scanout_id);
>          return;
> --
> 2.31.1
> 


Re: [PATCH] virtio-gpu: move scanout_id sanity check
Posted by Li Qiang 2 years, 9 months ago
Gerd Hoffmann <kraxel@redhat.com> 于2021年6月4日周五 下午3:50写道:
>
> Checking scanout_id in virtio_gpu_do_set_scanout() is too late, for the
> "resource_id == 0" case (aka disable scanout) the scanout_id is used
> unchecked.  Move the check into the callers to fix that.
>
> Fixes: e64d4b6a9bc3 ("virtio-gpu: Refactor virtio_gpu_set_scanout")
> Fixes: 32db3c63ae11 ("virtio-gpu: Add virtio_gpu_set_scanout_blob")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/383
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
>  hw/display/virtio-gpu.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 4d549377cbc1..e183f4ecdaa5 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -610,12 +610,6 @@ static void virtio_gpu_do_set_scanout(VirtIOGPU *g,
>      struct virtio_gpu_scanout *scanout;
>      uint8_t *data;
>
> -    if (scanout_id >= g->parent_obj.conf.max_outputs) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
> -                      __func__, scanout_id);
> -        *error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
> -        return;
> -    }
>      scanout = &g->parent_obj.scanout[scanout_id];
>
>      if (r->x > fb->width ||
> @@ -694,6 +688,13 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
>      trace_virtio_gpu_cmd_set_scanout(ss.scanout_id, ss.resource_id,
>                                       ss.r.width, ss.r.height, ss.r.x, ss.r.y);
>
> +    if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
> +                      __func__, ss.scanout_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
> +        return;
> +    }
> +
>      if (ss.resource_id == 0) {
>          virtio_gpu_disable_scanout(g, ss.scanout_id);
>          return;
> @@ -730,6 +731,13 @@ static void virtio_gpu_set_scanout_blob(VirtIOGPU *g,
>                                            ss.r.width, ss.r.height, ss.r.x,
>                                            ss.r.y);
>
> +    if (ss.scanout_id >= g->parent_obj.conf.max_outputs) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout id specified %d",
> +                      __func__, ss.scanout_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID;
> +        return;
> +    }
> +
>      if (ss.resource_id == 0) {
>          virtio_gpu_disable_scanout(g, ss.scanout_id);
>          return;
> --
> 2.31.1
>
>