diff mbox

hw/qxl: vaildate surface->data

Message ID 1351168048-29190-1-git-send-email-alevy@redhat.com
State New
Headers show

Commit Message

Alon Levy Oct. 25, 2012, 12:27 p.m. UTC 
Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Gerd Hoffmann Nov. 1, 2012, 9:33 a.m. UTC | #1 
On 10/25/12 14:27, Alon Levy wrote:
> Signed-off-by: Alon Levy <alevy@redhat.com>

Looks sane at a quick glance.

But: how far we wanna take this?  Add checks to qxl for each and every
assert() guests can trigger in spice-server?  So we end up
sanity-checking everything twice long-term?

I think instead we'll need a way for spice-server to report back errors
to qxl.  So spice-server would just notify qxl and go on (or stop
processing until reset) instead of aborting.  qxl in turn will notify
the guest.

[ The alternative would be to basically move server/red_parse_qxl.c
  into the qemu codebase.  I don't think we want that because that
  would make a bunch of data structures which are spice-server internal
  today (for good reasons) a libspice-server ABI+API. ]

cheers,
  Gerd
Alon Levy Nov. 1, 2012, 9:43 a.m. UTC | #2 
> On 10/25/12 14:27, Alon Levy wrote:
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> 
> Looks sane at a quick glance.
> 
> But: how far we wanna take this?  Add checks to qxl for each and
> every
> assert() guests can trigger in spice-server?  So we end up
> sanity-checking everything twice long-term?
> 
> I think instead we'll need a way for spice-server to report back
> errors
> to qxl.  So spice-server would just notify qxl and go on (or stop
> processing until reset) instead of aborting.  qxl in turn will notify
> the guest.

Yes I totally agree but never got around to doing it.

> 
> [ The alternative would be to basically move server/red_parse_qxl.c
>   into the qemu codebase.  I don't think we want that because that
>   would make a bunch of data structures which are spice-server
>   internal
>   today (for good reasons) a libspice-server ABI+API. ]
> 
> cheers,
>   Gerd
>
diff mbox

Patch

diff --git a/hw/qxl.c b/hw/qxl.c
index 1b47ed3..620b476 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -453,6 +453,16 @@  static int qxl_track_command(PCIQXLDevice *qxl, struct QXLCommandExt *ext)
                               cmd->u.surface_create.stride);
             return 1;
         }
+        if (cmd->type == QXL_SURFACE_CMD_CREATE) {
+            intptr_t surface_offset = (intptr_t)qxl_phys2virt(qxl,
+                                                     cmd->u.surface_create.data,
+                                                     MEMSLOT_GROUP_GUEST);
+            if (!surface_offset) {
+                qxl_set_guest_bug(qxl, "QXL_CMD_SURFACE invalid data: %ld\n",
+                                  cmd->u.surface_create.data);
+                return 1;
+            }
+        }
         qemu_mutex_lock(&qxl->track_lock);
         if (cmd->type == QXL_SURFACE_CMD_CREATE) {
             qxl->guest_surfaces.cmds[id] = ext->cmd.data;