[PATCH 0/3] virtio-gpu: security, logging and UB fixes

[PATCH 0/3] virtio-gpu: security, logging and UB fixes

[Bin Guo]

Three small fixes for virtio-gpu, each independent:

1. Add resource validation to CTX_ATTACH/DETACH_RESOURCE handlers,
   resolving the long-standing "TODO add security" comments.

2. Replace three fprintf(stderr) calls with qemu_log_mask(LOG_GUEST_ERROR)
   and trace events, consistent with the rest of the virtio-gpu code.

3. Use unsigned shift (1U <<) for scanout_bitmask operations to avoid
   undefined behavior when the shift count could theoretically reach 31.

Bin Guo (3):
  virtio-gpu-virgl: validate resource existence on ctx attach/detach
  virtio-gpu-virgl: replace fprintf(stderr) with qemu_log_mask and trace
    events
  virtio-gpu: use unsigned shift for scanout bitmask operations

 hw/display/trace-events       |  2 ++
 hw/display/virtio-gpu-virgl.c | 33 +++++++++++++++++++++++----------
 hw/display/virtio-gpu.c       | 12 ++++++------
 3 files changed, 31 insertions(+), 16 deletions(-)

-- 
2.50.1 (Apple Git-155)

[PATCH 1/3] virtio-gpu-virgl: validate resource existence on ctx attach/detach

[Bin Guo]

The CTX_ATTACH_RESOURCE and CTX_DETACH_RESOURCE command handlers
forwarded the guest-supplied resource_id directly to virglrenderer
without verifying that the resource actually exists.  A malicious or
buggy guest could reference an invalid resource_id, potentially causing
undefined behavior inside virglrenderer.

Resolve the long-standing "TODO add security" comments by checking the
resource against QEMU's own resource table (virtio_gpu_find_resource)
before forwarding the request.  This is consistent with how other
command handlers in virtio-gpu.c validate resource references.

Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
---
 hw/display/virtio-gpu-virgl.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 60c78af06a..b992ffc44e 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -743,6 +743,14 @@ static void virgl_cmd_ctx_attach_resource(VirtIOGPU *g,
     trace_virtio_gpu_cmd_ctx_res_attach(att_res.hdr.ctx_id,
                                         att_res.resource_id);
 
+    if (!virtio_gpu_find_resource(g, att_res.resource_id)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid resource %d for ctx %d\n",
+                      __func__, att_res.resource_id, att_res.hdr.ctx_id);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+
     virgl_renderer_ctx_attach_resource(att_res.hdr.ctx_id, att_res.resource_id);
 }
 
@@ -755,6 +763,14 @@ static void virgl_cmd_ctx_detach_resource(VirtIOGPU *g,
     trace_virtio_gpu_cmd_ctx_res_detach(det_res.hdr.ctx_id,
                                         det_res.resource_id);
 
+    if (!virtio_gpu_find_resource(g, det_res.resource_id)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: invalid resource %d for ctx %d\n",
+                      __func__, det_res.resource_id, det_res.hdr.ctx_id);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+
     virgl_renderer_ctx_detach_resource(det_res.hdr.ctx_id, det_res.resource_id);
 }
 
@@ -1073,11 +1089,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
         virgl_cmd_resource_unref(g, cmd, &cmd_suspended);
         break;
     case VIRTIO_GPU_CMD_CTX_ATTACH_RESOURCE:
-        /* TODO add security */
         virgl_cmd_ctx_attach_resource(g, cmd);
         break;
     case VIRTIO_GPU_CMD_CTX_DETACH_RESOURCE:
-        /* TODO add security */
         virgl_cmd_ctx_detach_resource(g, cmd);
         break;
     case VIRTIO_GPU_CMD_GET_CAPSET_INFO:
-- 
2.50.1 (Apple Git-155)

[PATCH 2/3] virtio-gpu-virgl: replace fprintf(stderr) with qemu_log_mask and trace events

[Bin Guo]

Three places in virtio-gpu-virgl.c used fprintf(stderr) directly,
bypassing QEMU's logging and tracing infrastructure:

  1. Command error reporting in virgl_cmd_process() — replace with
     qemu_log_mask(LOG_GUEST_ERROR, ...) so the message is filtered
     via -d guest_errors, consistent with virtio-gpu.c.

  2. Statistics output in virtio_gpu_print_stats() — replace with
     trace_virtio_gpu_virgl_stats() so the data is captured by the
     tracing subsystem instead of unconditionally hitting stderr.

  3. Idle statistics message — replace with
     trace_virtio_gpu_virgl_stats_idle() to preserve the existing
     "idle" indication without stderr pollution.

Add the two new trace events to hw/display/trace-events.

Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
---
 hw/display/trace-events       |  2 ++
 hw/display/virtio-gpu-virgl.c | 15 +++++++--------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/display/trace-events b/hw/display/trace-events
index 4bfc457fba..cf1e852b45 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -58,6 +58,8 @@ virtio_gpu_fence_resp(uint64_t fence) "fence 0x%" PRIx64
 virtio_gpu_inc_inflight_fences(uint32_t inflight) "in-flight+ %u"
 virtio_gpu_dec_inflight_fences(uint32_t inflight) "in-flight- %u"
 virtio_gpu_cmd_suspended(uint32_t cmd) "cmd 0x%x"
+virtio_gpu_virgl_stats(int requests, int max_inflight, int req_3d, int bytes_3d) "stats: vq req %d, max_inflight %d -- 3D %d (%d bytes)"
+virtio_gpu_virgl_stats_idle(void) "stats: idle"
 
 # qxl.c
 disable qxl_io_write_vga(int qid, const char *mode, uint32_t addr, uint32_t val) "%d %s addr=%u val=%u"
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index b992ffc44e..afb0d0984a 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -1129,8 +1129,8 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
         return;
     }
     if (cmd->error) {
-        fprintf(stderr, "%s: ctrl 0x%x, error 0x%x\n", __func__,
-                cmd->cmd_hdr.type, cmd->error);
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: ctrl 0x%x, error 0x%x\n",
+                      __func__, cmd->cmd_hdr.type, cmd->error);
         virtio_gpu_ctrl_response_nodata(g, cmd, cmd->error);
         return;
     }
@@ -1370,17 +1370,16 @@ static void virtio_gpu_print_stats(void *opaque)
     VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
 
     if (g->stats.requests) {
-        fprintf(stderr, "stats: vq req %4d, %3d -- 3D %4d (%5d)\n",
-                g->stats.requests,
-                g->stats.max_inflight,
-                g->stats.req_3d,
-                g->stats.bytes_3d);
+        trace_virtio_gpu_virgl_stats(g->stats.requests,
+                                     g->stats.max_inflight,
+                                     g->stats.req_3d,
+                                     g->stats.bytes_3d);
         g->stats.requests     = 0;
         g->stats.max_inflight = 0;
         g->stats.req_3d       = 0;
         g->stats.bytes_3d     = 0;
     } else {
-        fprintf(stderr, "stats: idle\r");
+        trace_virtio_gpu_virgl_stats_idle();
     }
     timer_mod(gl->print_stats, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 1000);
 }
-- 
2.50.1 (Apple Git-155)

[PATCH 3/3] virtio-gpu: use unsigned shift for scanout bitmask operations

[Bin Guo]

The scanout_bitmask field is uint32_t, but all six bit operations used
the signed literal `1 << shift`.  When the shift count reaches 31 this
is undefined behavior (signed integer overflow).  Replace every
occurrence with `1U << shift` to perform an unsigned shift, which is
well-defined for all bit positions 0-31.

No functional change; the result is already stored in a uint32_t, so
the generated code is identical on most platforms.

Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
---
 hw/display/virtio-gpu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 88526051a9..9dfffa270a 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -387,7 +387,7 @@ void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
 
     res = virtio_gpu_find_resource(g, scanout->resource_id);
     if (res) {
-        res->scanout_bitmask &= ~(1 << scanout_id);
+        res->scanout_bitmask &= ~(1U << scanout_id);
     }
 
     qemu_console_set_surface(scanout->con, NULL);
@@ -405,7 +405,7 @@ static void virtio_gpu_resource_destroy(VirtIOGPU *g,
 
     if (res->scanout_bitmask) {
         for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
-            if (res->scanout_bitmask & (1 << i)) {
+            if (res->scanout_bitmask & (1U << i)) {
                 virtio_gpu_disable_scanout(g, i);
             }
         }
@@ -571,7 +571,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
     for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
         QemuRect rect;
 
-        if (!(res->scanout_bitmask & (1 << i))) {
+        if (!(res->scanout_bitmask & (1U << i))) {
             continue;
         }
         scanout = &g->parent_obj.scanout[i];
@@ -605,10 +605,10 @@ void virtio_gpu_update_scanout(VirtIOGPU *g,
     scanout = &g->parent_obj.scanout[scanout_id];
     ores = virtio_gpu_find_resource(g, scanout->resource_id);
     if (ores) {
-        ores->scanout_bitmask &= ~(1 << scanout_id);
+        ores->scanout_bitmask &= ~(1U << scanout_id);
     }
 
-    res->scanout_bitmask |= (1 << scanout_id);
+    res->scanout_bitmask |= (1U << scanout_id);
     scanout->resource_id = res->resource_id;
     scanout->x = r->x;
     scanout->y = r->y;
@@ -1490,7 +1490,7 @@ static int virtio_gpu_post_load(void *opaque, int version_id)
         if (scanout->cursor.resource_id) {
             update_cursor(g, &scanout->cursor);
         }
-        res->scanout_bitmask |= (1 << i);
+        res->scanout_bitmask |= (1U << i);
     }
 
     return 0;
-- 
2.50.1 (Apple Git-155)

Re: [PATCH 3/3] virtio-gpu: use unsigned shift for scanout bitmask operations

[Akihiko Odaki]

On 2026/06/24 14:55, Bin Guo wrote:
> The scanout_bitmask field is uint32_t, but all six bit operations used
> the signed literal `1 << shift`.  When the shift count reaches 31 this
> is undefined behavior (signed integer overflow).  Replace every
> occurrence with `1U << shift` to perform an unsigned shift, which is
> well-defined for all bit positions 0-31.

I think we can make use of the BIT() macro.

Regards,
Akihiko Odaki

> 
> No functional change; the result is already stored in a uint32_t, so
> the generated code is identical on most platforms.
> 
> Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
> ---
>   hw/display/virtio-gpu.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 88526051a9..9dfffa270a 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -387,7 +387,7 @@ void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
>   
>       res = virtio_gpu_find_resource(g, scanout->resource_id);
>       if (res) {
> -        res->scanout_bitmask &= ~(1 << scanout_id);
> +        res->scanout_bitmask &= ~(1U << scanout_id);
>       }
>   
>       qemu_console_set_surface(scanout->con, NULL);
> @@ -405,7 +405,7 @@ static void virtio_gpu_resource_destroy(VirtIOGPU *g,
>   
>       if (res->scanout_bitmask) {
>           for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
> -            if (res->scanout_bitmask & (1 << i)) {
> +            if (res->scanout_bitmask & (1U << i)) {
>                   virtio_gpu_disable_scanout(g, i);
>               }
>           }
> @@ -571,7 +571,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
>       for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
>           QemuRect rect;
>   
> -        if (!(res->scanout_bitmask & (1 << i))) {
> +        if (!(res->scanout_bitmask & (1U << i))) {
>               continue;
>           }
>           scanout = &g->parent_obj.scanout[i];
> @@ -605,10 +605,10 @@ void virtio_gpu_update_scanout(VirtIOGPU *g,
>       scanout = &g->parent_obj.scanout[scanout_id];
>       ores = virtio_gpu_find_resource(g, scanout->resource_id);
>       if (ores) {
> -        ores->scanout_bitmask &= ~(1 << scanout_id);
> +        ores->scanout_bitmask &= ~(1U << scanout_id);
>       }
>   
> -    res->scanout_bitmask |= (1 << scanout_id);
> +    res->scanout_bitmask |= (1U << scanout_id);
>       scanout->resource_id = res->resource_id;
>       scanout->x = r->x;
>       scanout->y = r->y;
> @@ -1490,7 +1490,7 @@ static int virtio_gpu_post_load(void *opaque, int version_id)
>           if (scanout->cursor.resource_id) {
>               update_cursor(g, &scanout->cursor);
>           }
> -        res->scanout_bitmask |= (1 << i);
> +        res->scanout_bitmask |= (1U << i);
>       }
>   
>       return 0;

Re: [PATCH 2/3] virtio-gpu-virgl: replace fprintf(stderr) with qemu_log_mask and trace events

[Akihiko Odaki]

On 2026/06/24 14:55, Bin Guo wrote:
> Three places in virtio-gpu-virgl.c used fprintf(stderr) directly,
> bypassing QEMU's logging and tracing infrastructure:
> 
>    1. Command error reporting in virgl_cmd_process() — replace with
>       qemu_log_mask(LOG_GUEST_ERROR, ...) so the message is filtered
>       via -d guest_errors, consistent with virtio-gpu.c.
> 
>    2. Statistics output in virtio_gpu_print_stats() — replace with
>       trace_virtio_gpu_virgl_stats() so the data is captured by the
>       tracing subsystem instead of unconditionally hitting stderr.

Requiring both stats=on and the -trace option is inconvenient. Using 
trace_event_get_state() instead of keeping the stats property may be a 
good idea.

Regards,
Akihiko Odaki

> 
>    3. Idle statistics message — replace with
>       trace_virtio_gpu_virgl_stats_idle() to preserve the existing
>       "idle" indication without stderr pollution.
> 
> Add the two new trace events to hw/display/trace-events.
> 
> Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
> ---
>   hw/display/trace-events       |  2 ++
>   hw/display/virtio-gpu-virgl.c | 15 +++++++--------
>   2 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/display/trace-events b/hw/display/trace-events
> index 4bfc457fba..cf1e852b45 100644
> --- a/hw/display/trace-events
> +++ b/hw/display/trace-events
> @@ -58,6 +58,8 @@ virtio_gpu_fence_resp(uint64_t fence) "fence 0x%" PRIx64
>   virtio_gpu_inc_inflight_fences(uint32_t inflight) "in-flight+ %u"
>   virtio_gpu_dec_inflight_fences(uint32_t inflight) "in-flight- %u"
>   virtio_gpu_cmd_suspended(uint32_t cmd) "cmd 0x%x"
> +virtio_gpu_virgl_stats(int requests, int max_inflight, int req_3d, int bytes_3d) "stats: vq req %d, max_inflight %d -- 3D %d (%d bytes)"
> +virtio_gpu_virgl_stats_idle(void) "stats: idle"
>   
>   # qxl.c
>   disable qxl_io_write_vga(int qid, const char *mode, uint32_t addr, uint32_t val) "%d %s addr=%u val=%u"
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index b992ffc44e..afb0d0984a 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -1129,8 +1129,8 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>           return;
>       }
>       if (cmd->error) {
> -        fprintf(stderr, "%s: ctrl 0x%x, error 0x%x\n", __func__,
> -                cmd->cmd_hdr.type, cmd->error);
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: ctrl 0x%x, error 0x%x\n",
> +                      __func__, cmd->cmd_hdr.type, cmd->error);
>           virtio_gpu_ctrl_response_nodata(g, cmd, cmd->error);
>           return;
>       }
> @@ -1370,17 +1370,16 @@ static void virtio_gpu_print_stats(void *opaque)
>       VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>   
>       if (g->stats.requests) {
> -        fprintf(stderr, "stats: vq req %4d, %3d -- 3D %4d (%5d)\n",
> -                g->stats.requests,
> -                g->stats.max_inflight,
> -                g->stats.req_3d,
> -                g->stats.bytes_3d);
> +        trace_virtio_gpu_virgl_stats(g->stats.requests,
> +                                     g->stats.max_inflight,
> +                                     g->stats.req_3d,
> +                                     g->stats.bytes_3d);
>           g->stats.requests     = 0;
>           g->stats.max_inflight = 0;
>           g->stats.req_3d       = 0;
>           g->stats.bytes_3d     = 0;
>       } else {
> -        fprintf(stderr, "stats: idle\r");
> +        trace_virtio_gpu_virgl_stats_idle();
>       }
>       timer_mod(gl->print_stats, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 1000);
>   }

Re: [PATCH 2/3] virtio-gpu-virgl: replace fprintf(stderr) with qemu_log_mask and trace events

[marcandre.lureau]

On Wed, 24 Jun 2026 13:55:56 +0800, Bin Guo <guobin@linux.alibaba.com> wrote:
> Three places in virtio-gpu-virgl.c used fprintf(stderr) directly,
> bypassing QEMU's logging and tracing infrastructure:
> 
>   1. Command error reporting in virgl_cmd_process() — replace with
>      qemu_log_mask(LOG_GUEST_ERROR, ...) so the message is filtered
>      via -d guest_errors, consistent with virtio-gpu.c.
> 
> [...]

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

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

Re: [PATCH 3/3] virtio-gpu: use unsigned shift for scanout bitmask operations

[marcandre.lureau]

On Wed, 24 Jun 2026 13:55:57 +0800, Bin Guo <guobin@linux.alibaba.com> wrote:
> The scanout_bitmask field is uint32_t, but all six bit operations used
> the signed literal `1 << shift`.  When the shift count reaches 31 this
> is undefined behavior (signed integer overflow).  Replace every
> occurrence with `1U << shift` to perform an unsigned shift, which is
> well-defined for all bit positions 0-31.
> 
> No functional change; the result is already stored in a uint32_t, so
> the generated code is identical on most platforms.
> 
> [...]

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

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

Re: [PATCH 1/3] virtio-gpu-virgl: validate resource existence on ctx attach/detach

[marcandre.lureau]

> The CTX_ATTACH_RESOURCE and CTX_DETACH_RESOURCE command handlers
> forwarded the guest-supplied resource_id directly to virglrenderer
> without verifying that the resource actually exists.  A malicious or
> buggy guest could reference an invalid resource_id, potentially causing
> undefined behavior inside virglrenderer.
> 
> Resolve the long-standing "TODO add security" comments by checking the
> resource against QEMU's own resource table (virtio_gpu_find_resource)
> before forwarding the request.  This is consistent with how other
> command handlers in virtio-gpu.c validate resource references.
> 
> Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
> Message-ID: <32f6f23fb4880a4f1ec3056fde306332d54e442f.1782270919.git.guobin@linux.alibaba.com>
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 60c78af06a4..b992ffc44e1 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -743,6 +743,14 @@ static void virgl_cmd_ctx_attach_resource(VirtIOGPU *g,
>      trace_virtio_gpu_cmd_ctx_res_attach(att_res.hdr.ctx_id,
>                                          att_res.resource_id);
>  
> +    if (!virtio_gpu_find_resource(g, att_res.resource_id)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: invalid resource %d for ctx %d\n",
> +                      __func__, att_res.resource_id, att_res.hdr.ctx_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
>      virgl_renderer_ctx_attach_resource(att_res.hdr.ctx_id, att_res.resource_id);
>  }
>  
> @@ -755,6 +763,14 @@ static void virgl_cmd_ctx_detach_resource(VirtIOGPU *g,
>      trace_virtio_gpu_cmd_ctx_res_detach(det_res.hdr.ctx_id,
>                                          det_res.resource_id);
>  
> +    if (!virtio_gpu_find_resource(g, det_res.resource_id)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: invalid resource %d for ctx %d\n",
> +                      __func__, det_res.resource_id, det_res.hdr.ctx_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +

There is room for factorizing the checking of cmd resource ids. ok.

>      virgl_renderer_ctx_detach_resource(det_res.hdr.ctx_id, det_res.resource_id);
>  }
>  
> @@ -1073,11 +1089,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>          virgl_cmd_resource_unref(g, cmd, &cmd_suspended);
>          break;
>      case VIRTIO_GPU_CMD_CTX_ATTACH_RESOURCE:
> -        /* TODO add security */
>          virgl_cmd_ctx_attach_resource(g, cmd);
>          break;
>      case VIRTIO_GPU_CMD_CTX_DETACH_RESOURCE:
> -        /* TODO add security */

Tbh, I am not sure what Gerd had in mind when he wrote that. I doubt it was only about checking resource IDs.

>          virgl_cmd_ctx_detach_resource(g, cmd);
>          break;
>      case VIRTIO_GPU_CMD_GET_CAPSET_INFO:

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

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

Re: [PATCH 1/3] virtio-gpu-virgl: validate resource existence on ctx attach/detach

[Akihiko Odaki]

On 2026/06/24 14:55, Bin Guo wrote:
> The CTX_ATTACH_RESOURCE and CTX_DETACH_RESOURCE command handlers
> forwarded the guest-supplied resource_id directly to virglrenderer
> without verifying that the resource actually exists.  A malicious or
> buggy guest could reference an invalid resource_id, potentially causing
> undefined behavior inside virglrenderer.
> 
> Resolve the long-standing "TODO add security" comments by checking the
> resource against QEMU's own resource table (virtio_gpu_find_resource)
> before forwarding the request.  This is consistent with how other
> command handlers in virtio-gpu.c validate resource references.

I think it is better to be checked inside virglrenderer. In general, the 
timings/places of check and usage should be centralized to ensure that 
the invariant required by usage is properly covered by the check and to 
avoid time-of-check to time-of-use (ToCTToU) bugs. Since virglrenderer 
uses the value, it should also check it.

Furthermore, checking resource_id is insufficient as it does not cover 
another parameter, ctx_id, despite that the TODO comments are removed. 
QEMU currently has no knowledge what ctx_id is valid, so it also 
motivates having checks inside virglrenderer. The corresponding 
functions of virglrenderer may be extended to have return values for 
error reporting like other virglrenderer functions.

Regards,
Akihiko Odaki

> 
> Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
> ---
>   hw/display/virtio-gpu-virgl.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 60c78af06a..b992ffc44e 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -743,6 +743,14 @@ static void virgl_cmd_ctx_attach_resource(VirtIOGPU *g,
>       trace_virtio_gpu_cmd_ctx_res_attach(att_res.hdr.ctx_id,
>                                           att_res.resource_id);
>   
> +    if (!virtio_gpu_find_resource(g, att_res.resource_id)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: invalid resource %d for ctx %d\n",
> +                      __func__, att_res.resource_id, att_res.hdr.ctx_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
>       virgl_renderer_ctx_attach_resource(att_res.hdr.ctx_id, att_res.resource_id);
>   }
>   
> @@ -755,6 +763,14 @@ static void virgl_cmd_ctx_detach_resource(VirtIOGPU *g,
>       trace_virtio_gpu_cmd_ctx_res_detach(det_res.hdr.ctx_id,
>                                           det_res.resource_id);
>   
> +    if (!virtio_gpu_find_resource(g, det_res.resource_id)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: invalid resource %d for ctx %d\n",
> +                      __func__, det_res.resource_id, det_res.hdr.ctx_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
>       virgl_renderer_ctx_detach_resource(det_res.hdr.ctx_id, det_res.resource_id);
>   }
>   
> @@ -1073,11 +1089,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>           virgl_cmd_resource_unref(g, cmd, &cmd_suspended);
>           break;
>       case VIRTIO_GPU_CMD_CTX_ATTACH_RESOURCE:
> -        /* TODO add security */
>           virgl_cmd_ctx_attach_resource(g, cmd);
>           break;
>       case VIRTIO_GPU_CMD_CTX_DETACH_RESOURCE:
> -        /* TODO add security */
>           virgl_cmd_ctx_detach_resource(g, cmd);
>           break;
>       case VIRTIO_GPU_CMD_GET_CAPSET_INFO: