[Qemu-devel] [PATCH 0/2] qxl fixes

[Qemu-devel] [PATCH 0/2] qxl fixes

[Markus Armbruster]

Markus Armbruster (2):
  qxl: Fix SPICE_RING_PROD_ITEM(), SPICE_RING_CONS_ITEM() sanity check
  qxl: Don't drop client capability bits

 hw/qxl.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH 1/2] qxl: Fix SPICE_RING_PROD_ITEM(), SPICE_RING_CONS_ITEM() sanity check

[Markus Armbruster]

From: Markus Armbruster <armbru@pond.sub.org>

The pointer arithmetic there is safe, but ugly.  Coverity grouses
about it.  However, the actual comparison is off by one: <= end
instead of < end.  Fix by rewriting the check in a cleaner way.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/qxl.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index d08b9bd..2c2b422 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -37,33 +37,25 @@
  */
 #undef SPICE_RING_PROD_ITEM
 #define SPICE_RING_PROD_ITEM(qxl, r, ret) {                             \
-        typeof(r) start = r;                                            \
-        typeof(r) end = r + 1;                                          \
         uint32_t prod = (r)->prod & SPICE_RING_INDEX_MASK(r);           \
-        typeof(&(r)->items[prod]) m_item = &(r)->items[prod];           \
-        if (!((uint8_t*)m_item >= (uint8_t*)(start) && (uint8_t*)(m_item + 1) <= (uint8_t*)(end))) { \
+        if (prod >= ARRAY_SIZE((r)->items)) {                           \
             qxl_set_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch " \
-                          "! %p <= %p < %p", (uint8_t *)start,          \
-                          (uint8_t *)m_item, (uint8_t *)end);           \
+                          "%u >= %zu", prod, ARRAY_SIZE((r)->items));   \
             ret = NULL;                                                 \
         } else {                                                        \
-            ret = &m_item->el;                                          \
+            ret = &(r)->items[prod].el;                                 \
         }                                                               \
     }
 
 #undef SPICE_RING_CONS_ITEM
 #define SPICE_RING_CONS_ITEM(qxl, r, ret) {                             \
-        typeof(r) start = r;                                            \
-        typeof(r) end = r + 1;                                          \
         uint32_t cons = (r)->cons & SPICE_RING_INDEX_MASK(r);           \
-        typeof(&(r)->items[cons]) m_item = &(r)->items[cons];           \
-        if (!((uint8_t*)m_item >= (uint8_t*)(start) && (uint8_t*)(m_item + 1) <= (uint8_t*)(end))) { \
+        if (cons >= ARRAY_SIZE((r)->items)) {                           \
             qxl_set_guest_bug(qxl, "SPICE_RING_CONS_ITEM indices mismatch " \
-                          "! %p <= %p < %p", (uint8_t *)start,          \
-                          (uint8_t *)m_item, (uint8_t *)end);           \
+                          "%u >= %zu", cons, ARRAY_SIZE((r)->items));   \
             ret = NULL;                                                 \
         } else {                                                        \
-            ret = &m_item->el;                                          \
+            ret = &(r)->items[cons].el;                                 \
         }                                                               \
     }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 2/2] qxl: Don't drop client capability bits

[Markus Armbruster]

interface_set_client_capabilities() copies only the first few bits,
because it falls into a Classic C trap: you can declare a parameter
uint8_t caps[58], but the resulting parameter type is uint8_t *, not
uint8_t[58].  In particular, sizeof(caps) is sizeof(uint8_t *), not
the intended sizeof(uint8_t[58]).

Harmless, because the bits aren't used, yet.  Broken in commit
c10018d6.  Spotted by Coverity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/qxl.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 2c2b422..874d56f 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -951,9 +951,11 @@ static void interface_set_client_capabilities(QXLInstance *sin,
     }
 
     qxl->shadow_rom.client_present = client_present;
-    memcpy(qxl->shadow_rom.client_capabilities, caps, sizeof(caps));
+    memcpy(qxl->shadow_rom.client_capabilities, caps,
+           sizeof(qxl->shadow_rom.client_capabilities));
     qxl->rom->client_present = client_present;
-    memcpy(qxl->rom->client_capabilities, caps, sizeof(caps));
+    memcpy(qxl->rom->client_capabilities, caps,
+           sizeof(qxl->rom->client_capabilities));
     qxl_rom_set_dirty(qxl);
 
     qxl_send_events(qxl, QXL_INTERRUPT_CLIENT);
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 0/2] qxl fixes

[Gerd Hoffmann]

On 01/10/13 14:24, Markus Armbruster wrote:
> Markus Armbruster (2):
>   qxl: Fix SPICE_RING_PROD_ITEM(), SPICE_RING_CONS_ITEM() sanity check
>   qxl: Don't drop client capability bits
> 
>  hw/qxl.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 

Patch added to spice patch queue.

thanks,
  Gerd

[Qemu-devel] [PULL 0/2] spice patch queue

[Gerd Hoffmann]

  Hi,

Here comes the spice patch queue, carrying two coverity fixes in qxl.

please pull,
  Gerd

The following changes since commit 63fb2590839162afdf14d7c0ee02d460766c0956:

  Merge branch 'target-arm.next' of git://git.linaro.org/people/pmaydell/qemu-arm (2013-01-12 12:47:07 +0000)

are available in the git repository at:


  git://anongit.freedesktop.org/spice/qemu spice.v67

for you to fetch changes up to 08688af04dc1137ac2f420b35c235183926b4a23:

  qxl: Don't drop client capability bits (2013-01-14 08:59:38 +0100)

----------------------------------------------------------------
Markus Armbruster (2):
      qxl: Fix SPICE_RING_PROD_ITEM(), SPICE_RING_CONS_ITEM() sanity check
      qxl: Don't drop client capability bits

 hw/qxl.c |   26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

[Qemu-devel] [PATCH 2/2] qxl: Don't drop client capability bits

[Gerd Hoffmann]

From: Markus Armbruster <armbru@redhat.com>

interface_set_client_capabilities() copies only the first few bits,
because it falls into a Classic C trap: you can declare a parameter
uint8_t caps[58], but the resulting parameter type is uint8_t *, not
uint8_t[58].  In particular, sizeof(caps) is sizeof(uint8_t *), not
the intended sizeof(uint8_t[58]).

Harmless, because the bits aren't used, yet.  Broken in commit
c10018d6.  Spotted by Coverity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qxl.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index e8f380b..9dc44b9 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -951,9 +951,11 @@ static void interface_set_client_capabilities(QXLInstance *sin,
     }
 
     qxl->shadow_rom.client_present = client_present;
-    memcpy(qxl->shadow_rom.client_capabilities, caps, sizeof(caps));
+    memcpy(qxl->shadow_rom.client_capabilities, caps,
+           sizeof(qxl->shadow_rom.client_capabilities));
     qxl->rom->client_present = client_present;
-    memcpy(qxl->rom->client_capabilities, caps, sizeof(caps));
+    memcpy(qxl->rom->client_capabilities, caps,
+           sizeof(qxl->rom->client_capabilities));
     qxl_rom_set_dirty(qxl);
 
     qxl_send_events(qxl, QXL_INTERRUPT_CLIENT);
-- 
1.7.9.7