[Qemu-devel] [PATCH 0/6] virtio-gpu: scanout limit fixes

[Qemu-devel] [PATCH 0/6] virtio-gpu: scanout limit fixes

[marcandre.lureau]

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

Hi,

Here is a small series improving a bit the scanout limit handling. The
first patch was sent previously but is also included in this series
for convenience.

Marc-André Lureau (6):
  virtio-gpu: check early scanout id
  virtio-vga: propagate on gpu realized error
  virtio-gpu: check max_outputs value
  virtio-gpu: check max_outputs only
  virtio-gpu: use VIRTIO_GPU_MAX_SCANOUTS
  virtio-gpu: fix ui idx check

 hw/display/virtio-gpu-3d.c     |  5 +++--
 hw/display/virtio-gpu.c        | 28 ++++++++++++++++------------
 hw/display/virtio-vga.c        |  8 +++++++-
 include/hw/virtio/virtio-gpu.h |  6 ++----
 4 files changed, 28 insertions(+), 19 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/6] virtio-gpu: check early scanout id

[marcandre.lureau]

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

Before accessing the g->scanout array, in order to avoid potential
out-of-bounds access.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/virtio-gpu.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index c181fb3..1193838 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -507,6 +507,14 @@ 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 >= VIRTIO_GPU_MAX_SCANOUT ||
+        ss.scanout_id >= g->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;
+    }
+
     g->enable = 1;
     if (ss.resource_id == 0) {
         scanout = &g->scanout[ss.scanout_id];
@@ -516,8 +524,7 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
                 res->scanout_bitmask &= ~(1 << ss.scanout_id);
             }
         }
-        if (ss.scanout_id == 0 ||
-            ss.scanout_id >= g->conf.max_outputs) {
+        if (ss.scanout_id == 0) {
             qemu_log_mask(LOG_GUEST_ERROR,
                           "%s: illegal scanout id specified %d",
                           __func__, ss.scanout_id);
@@ -532,14 +539,6 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
     }
 
     /* create a surface for this scanout */
-    if (ss.scanout_id >= VIRTIO_GPU_MAX_SCANOUT ||
-        ss.scanout_id >= g->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;
-    }
-
     res = virtio_gpu_find_resource(g, ss.resource_id);
     if (!res) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal resource specified %d\n",
-- 
2.7.4

[Qemu-devel] [PATCH 2/6] virtio-vga: propagate on gpu realized error

[marcandre.lureau]

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/virtio-gpu-3d.c | 1 +
 hw/display/virtio-vga.c    | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c
index fa19294..20e8865 100644
--- a/hw/display/virtio-gpu-3d.c
+++ b/hw/display/virtio-gpu-3d.c
@@ -17,6 +17,7 @@
 #include "trace.h"
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/virtio-gpu.h"
+#include "qapi/error.h"
 
 #ifdef CONFIG_VIRGL
 
diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index e58b165..f49f8de 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -4,6 +4,7 @@
 #include "ui/console.h"
 #include "vga_int.h"
 #include "hw/virtio/virtio-pci.h"
+#include "qapi/error.h"
 
 /*
  * virtio-vga: This extends VirtioPCIProxy.
@@ -89,6 +90,7 @@ static void virtio_vga_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     VirtIOVGA *vvga = VIRTIO_VGA(vpci_dev);
     VirtIOGPU *g = &vvga->vdev;
     VGACommonState *vga = &vvga->vga;
+    Error *err = NULL;
     uint32_t offset;
     int i;
 
@@ -124,7 +126,11 @@ static void virtio_vga_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     /* force virtio-1.0 */
     vpci_dev->flags &= ~VIRTIO_PCI_FLAG_DISABLE_MODERN;
     vpci_dev->flags |= VIRTIO_PCI_FLAG_DISABLE_LEGACY;
-    object_property_set_bool(OBJECT(g), true, "realized", errp);
+    object_property_set_bool(OBJECT(g), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
 
     /* add stdvga mmio regions */
     pci_std_vga_mmio_region_init(vga, &vpci_dev->modern_bar,
-- 
2.7.4

[Qemu-devel] [PATCH 3/6] virtio-gpu: check max_outputs value

[marcandre.lureau]

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

The value must be less than VIRTIO_GPU_MAX_SCANOUT.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/virtio-gpu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 1193838..77e542f 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -19,6 +19,7 @@
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/virtio-gpu.h"
 #include "hw/virtio/virtio-bus.h"
+#include "qapi/error.h"
 
 static struct virtio_gpu_simple_resource*
 virtio_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id);
@@ -928,6 +929,11 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
     bool have_virgl;
     int i;
 
+    if (g->conf.max_outputs > VIRTIO_GPU_MAX_SCANOUT) {
+        error_setg(errp, "invalid max_outputs > %d", VIRTIO_GPU_MAX_SCANOUT);
+        return;
+    }
+
     g->config_size = sizeof(struct virtio_gpu_config);
     g->virtio_config.num_scanouts = g->conf.max_outputs;
     virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU,
-- 
2.7.4

[Qemu-devel] [PATCH 4/6] virtio-gpu: check max_outputs only

[marcandre.lureau]

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

The scanout id should not be above the configured num_scanouts.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/virtio-gpu-3d.c | 4 ++--
 hw/display/virtio-gpu.c    | 5 ++---
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c
index 20e8865..433bf93 100644
--- a/hw/display/virtio-gpu-3d.c
+++ b/hw/display/virtio-gpu-3d.c
@@ -128,7 +128,7 @@ static void virgl_cmd_resource_flush(VirtIOGPU *g,
     trace_virtio_gpu_cmd_res_flush(rf.resource_id,
                                    rf.r.width, rf.r.height, rf.r.x, rf.r.y);
 
-    for (i = 0; i < VIRTIO_GPU_MAX_SCANOUT; i++) {
+    for (i = 0; i < g->conf.max_outputs; i++) {
         if (g->scanout[i].resource_id != rf.resource_id) {
             continue;
         }
@@ -147,7 +147,7 @@ static void virgl_cmd_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 >= VIRTIO_GPU_MAX_SCANOUT) {
+    if (ss.scanout_id >= g->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;
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 77e542f..17933cb 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -465,7 +465,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
 
     pixman_region_init_rect(&flush_region,
                             rf.r.x, rf.r.y, rf.r.width, rf.r.height);
-    for (i = 0; i < VIRTIO_GPU_MAX_SCANOUT; i++) {
+    for (i = 0; i < g->conf.max_outputs; i++) {
         struct virtio_gpu_scanout *scanout;
         pixman_region16_t region, finalregion;
         pixman_box16_t *extents;
@@ -508,8 +508,7 @@ 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 >= VIRTIO_GPU_MAX_SCANOUT ||
-        ss.scanout_id >= g->conf.max_outputs) {
+    if (ss.scanout_id >= g->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;
-- 
2.7.4

[Qemu-devel] [PATCH 5/6] virtio-gpu: use VIRTIO_GPU_MAX_SCANOUTS

[marcandre.lureau]

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

The value is defined in virtio_gpu.h already (changing from 4 to 16).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/virtio-gpu.c        | 4 ++--
 include/hw/virtio/virtio-gpu.h | 6 ++----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 17933cb..5811a2d 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -928,8 +928,8 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
     bool have_virgl;
     int i;
 
-    if (g->conf.max_outputs > VIRTIO_GPU_MAX_SCANOUT) {
-        error_setg(errp, "invalid max_outputs > %d", VIRTIO_GPU_MAX_SCANOUT);
+    if (g->conf.max_outputs > VIRTIO_GPU_MAX_SCANOUTS) {
+        error_setg(errp, "invalid max_outputs > %d", VIRTIO_GPU_MAX_SCANOUTS);
         return;
     }
 
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 13b0ab0..1602a13 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -27,8 +27,6 @@
 
 #define VIRTIO_ID_GPU 16
 
-#define VIRTIO_GPU_MAX_SCANOUT 4
-
 struct virtio_gpu_simple_resource {
     uint32_t resource_id;
     uint32_t width;
@@ -98,8 +96,8 @@ typedef struct VirtIOGPU {
     QTAILQ_HEAD(, virtio_gpu_ctrl_command) cmdq;
     QTAILQ_HEAD(, virtio_gpu_ctrl_command) fenceq;
 
-    struct virtio_gpu_scanout scanout[VIRTIO_GPU_MAX_SCANOUT];
-    struct virtio_gpu_requested_state req_state[VIRTIO_GPU_MAX_SCANOUT];
+    struct virtio_gpu_scanout scanout[VIRTIO_GPU_MAX_SCANOUTS];
+    struct virtio_gpu_requested_state req_state[VIRTIO_GPU_MAX_SCANOUTS];
 
     struct virtio_gpu_conf conf;
     int enabled_output_bitmask;
-- 
2.7.4

[Qemu-devel] [PATCH 6/6] virtio-gpu: fix ui idx check

[marcandre.lureau]

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

Fix off-by-one value check (0 is the first scanout).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/virtio-gpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 5811a2d..d1f25d5 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -878,7 +878,7 @@ static int virtio_gpu_ui_info(void *opaque, uint32_t idx, QemuUIInfo *info)
 {
     VirtIOGPU *g = opaque;
 
-    if (idx > g->conf.max_outputs) {
+    if (idx >= g->conf.max_outputs) {
         return -1;
     }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/6] virtio-gpu: scanout limit fixes

[Gerd Hoffmann]

On Do, 2016-05-19 at 12:25 +0200, marcandre.lureau@redhat.com wrote:
> Here is a small series improving a bit the scanout limit handling. The
> first patch was sent previously but is also included in this series
> for convenience.

Doesn't apply cleanly to master, does this depend on unmerged patches?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 0/6] virtio-gpu: scanout limit fixes

[Marc-André Lureau]

Hi

On Fri, May 20, 2016 at 7:44 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Do, 2016-05-19 at 12:25 +0200, marcandre.lureau@redhat.com wrote:
>> Here is a small series improving a bit the scanout limit handling. The
>> first patch was sent previously but is also included in this series
>> for convenience.
>
> Doesn't apply cleanly to master, does this depend on unmerged patches?
>

No, it was based on 8ec4fe0a4b. There is a small header conflict in
the third patch that can be trivially solved.


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 0/6] virtio-gpu: scanout limit fixes

[Gerd Hoffmann]

On Fr, 2016-05-20 at 11:52 +0200, Marc-André Lureau wrote:
> Hi
> 
> On Fri, May 20, 2016 at 7:44 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > On Do, 2016-05-19 at 12:25 +0200, marcandre.lureau@redhat.com wrote:
> >> Here is a small series improving a bit the scanout limit handling. The
> >> first patch was sent previously but is also included in this series
> >> for convenience.
> >
> > Doesn't apply cleanly to master, does this depend on unmerged patches?
> >
> 
> No, it was based on 8ec4fe0a4b. There is a small header conflict in
> the third patch that can be trivially solved.

Thanks.  Applied against 8ec4fe0a4b & rebased -- in ui queue now.

cheers,
  Gerd