From: "Daniel P. Berrangé" <berrange@redhat.com> To: qemu-devel@nongnu.org Cc: "Laurent Vivier" <laurent@vivier.eu>, "Riku Voipio" <riku.voipio@iki.fi>, "Gerd Hoffmann" <kraxel@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com> Subject: [Qemu-devel] [PATCH v2 5/5] qxl: avoid unaligned pointer reads/writes Date: Fri, 12 Apr 2019 13:16:26 +0100 [thread overview] Message-ID: <20190412121626.19829-6-berrange@redhat.com> (raw) In-Reply-To: <20190412121626.19829-1-berrange@redhat.com> The SPICE_RING_PROD_ITEM() macro is initializing a local 'uint64_t *' variable to point to the 'el' field inside the QXLReleaseRing struct. This uint64_t field is not guaranteed aligned as the struct is packed. Code should not take the address of fields within a packed struct. Changing the SPICE_RING_PROD_ITEM() macro to avoid taking the address of the field is impractical. It is clearer to just remove the macro and inline its functionality in the three call sites that need it. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- hw/display/qxl.c | 55 +++++++++++++++++++++--------------------------- 1 file changed, 24 insertions(+), 31 deletions(-) diff --git a/hw/display/qxl.c b/hw/display/qxl.c index c8ce5781e0..5c38e6e906 100644 --- a/hw/display/qxl.c +++ b/hw/display/qxl.c @@ -33,24 +33,6 @@ #include "qxl.h" -/* - * NOTE: SPICE_RING_PROD_ITEM accesses memory on the pci bar and as - * such can be changed by the guest, so to avoid a guest trigerrable - * abort we just qxl_set_guest_bug and set the return to NULL. Still - * it may happen as a result of emulator bug as well. - */ -#undef SPICE_RING_PROD_ITEM -#define SPICE_RING_PROD_ITEM(qxl, r, ret) { \ - uint32_t prod = (r)->prod & SPICE_RING_INDEX_MASK(r); \ - if (prod >= ARRAY_SIZE((r)->items)) { \ - qxl_set_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch " \ - "%u >= %zu", prod, ARRAY_SIZE((r)->items)); \ - ret = NULL; \ - } else { \ - ret = &(r)->items[prod].el; \ - } \ - } - #undef SPICE_RING_CONS_ITEM #define SPICE_RING_CONS_ITEM(qxl, r, ret) { \ uint32_t cons = (r)->cons & SPICE_RING_INDEX_MASK(r); \ @@ -414,7 +396,8 @@ static void init_qxl_rom(PCIQXLDevice *d) static void init_qxl_ram(PCIQXLDevice *d) { uint8_t *buf; - uint64_t *item; + uint32_t prod; + QXLReleaseRing *ring; buf = d->vga.vram_ptr; d->ram = (QXLRam *)(buf + le32_to_cpu(d->shadow_rom.ram_header_offset)); @@ -426,9 +409,12 @@ static void init_qxl_ram(PCIQXLDevice *d) SPICE_RING_INIT(&d->ram->cmd_ring); SPICE_RING_INIT(&d->ram->cursor_ring); SPICE_RING_INIT(&d->ram->release_ring); - SPICE_RING_PROD_ITEM(d, &d->ram->release_ring, item); - assert(item); - *item = 0; + + ring = &d->ram->release_ring; + prod = ring->prod & SPICE_RING_INDEX_MASK(ring); + assert(prod < ARRAY_SIZE(ring->items)); + ring->items[prod].el = 0; + qxl_ring_set_dirty(d); } @@ -732,7 +718,7 @@ static int interface_req_cmd_notification(QXLInstance *sin) static inline void qxl_push_free_res(PCIQXLDevice *d, int flush) { QXLReleaseRing *ring = &d->ram->release_ring; - uint64_t *item; + uint32_t prod; int notify; #define QXL_FREE_BUNCH_SIZE 32 @@ -759,11 +745,15 @@ static inline void qxl_push_free_res(PCIQXLDevice *d, int flush) if (notify) { qxl_send_events(d, QXL_INTERRUPT_DISPLAY); } - SPICE_RING_PROD_ITEM(d, ring, item); - if (!item) { + + ring = &d->ram->release_ring; + prod = ring->prod & SPICE_RING_INDEX_MASK(ring); + if (prod >= ARRAY_SIZE(ring->items)) { + qxl_set_guest_bug(d, "SPICE_RING_PROD_ITEM indices mismatch " + "%u >= %zu", prod, ARRAY_SIZE(ring->items)); return; } - *item = 0; + ring->items[prod].el = 0; d->num_free_res = 0; d->last_release = NULL; qxl_ring_set_dirty(d); @@ -775,7 +765,8 @@ static void interface_release_resource(QXLInstance *sin, { PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); QXLReleaseRing *ring; - uint64_t *item, id; + uint32_t prod; + uint64_t id; if (ext.group_id == MEMSLOT_GROUP_HOST) { /* host group -> vga mode update request */ @@ -792,16 +783,18 @@ static void interface_release_resource(QXLInstance *sin, * pci bar 0, $command.release_info */ ring = &qxl->ram->release_ring; - SPICE_RING_PROD_ITEM(qxl, ring, item); - if (!item) { + prod = ring->prod & SPICE_RING_INDEX_MASK(ring); + if (prod >= ARRAY_SIZE(ring->items)) { + qxl_set_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch " + "%u >= %zu", prod, ARRAY_SIZE(ring->items)); return; } - if (*item == 0) { + if (ring->items[prod].el == 0) { /* stick head into the ring */ id = ext.info->id; ext.info->next = 0; qxl_ram_set_dirty(qxl, &ext.info->next); - *item = id; + ring->items[prod].el = id; qxl_ring_set_dirty(qxl); } else { /* append item to the list */ -- 2.20.1
WARNING: multiple messages have this Message-ID (diff)
From: "Daniel P. Berrangé" <berrange@redhat.com> To: qemu-devel@nongnu.org Cc: Riku Voipio <riku.voipio@iki.fi>, Laurent Vivier <laurent@vivier.eu>, Gerd Hoffmann <kraxel@redhat.com> Subject: [Qemu-devel] [PATCH v2 5/5] qxl: avoid unaligned pointer reads/writes Date: Fri, 12 Apr 2019 13:16:26 +0100 [thread overview] Message-ID: <20190412121626.19829-6-berrange@redhat.com> (raw) Message-ID: <20190412121626.DiojtgoQrwN0aM590uLu0lo4pfzGoMyKjULP-CkzQTw@z> (raw) In-Reply-To: <20190412121626.19829-1-berrange@redhat.com> The SPICE_RING_PROD_ITEM() macro is initializing a local 'uint64_t *' variable to point to the 'el' field inside the QXLReleaseRing struct. This uint64_t field is not guaranteed aligned as the struct is packed. Code should not take the address of fields within a packed struct. Changing the SPICE_RING_PROD_ITEM() macro to avoid taking the address of the field is impractical. It is clearer to just remove the macro and inline its functionality in the three call sites that need it. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- hw/display/qxl.c | 55 +++++++++++++++++++++--------------------------- 1 file changed, 24 insertions(+), 31 deletions(-) diff --git a/hw/display/qxl.c b/hw/display/qxl.c index c8ce5781e0..5c38e6e906 100644 --- a/hw/display/qxl.c +++ b/hw/display/qxl.c @@ -33,24 +33,6 @@ #include "qxl.h" -/* - * NOTE: SPICE_RING_PROD_ITEM accesses memory on the pci bar and as - * such can be changed by the guest, so to avoid a guest trigerrable - * abort we just qxl_set_guest_bug and set the return to NULL. Still - * it may happen as a result of emulator bug as well. - */ -#undef SPICE_RING_PROD_ITEM -#define SPICE_RING_PROD_ITEM(qxl, r, ret) { \ - uint32_t prod = (r)->prod & SPICE_RING_INDEX_MASK(r); \ - if (prod >= ARRAY_SIZE((r)->items)) { \ - qxl_set_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch " \ - "%u >= %zu", prod, ARRAY_SIZE((r)->items)); \ - ret = NULL; \ - } else { \ - ret = &(r)->items[prod].el; \ - } \ - } - #undef SPICE_RING_CONS_ITEM #define SPICE_RING_CONS_ITEM(qxl, r, ret) { \ uint32_t cons = (r)->cons & SPICE_RING_INDEX_MASK(r); \ @@ -414,7 +396,8 @@ static void init_qxl_rom(PCIQXLDevice *d) static void init_qxl_ram(PCIQXLDevice *d) { uint8_t *buf; - uint64_t *item; + uint32_t prod; + QXLReleaseRing *ring; buf = d->vga.vram_ptr; d->ram = (QXLRam *)(buf + le32_to_cpu(d->shadow_rom.ram_header_offset)); @@ -426,9 +409,12 @@ static void init_qxl_ram(PCIQXLDevice *d) SPICE_RING_INIT(&d->ram->cmd_ring); SPICE_RING_INIT(&d->ram->cursor_ring); SPICE_RING_INIT(&d->ram->release_ring); - SPICE_RING_PROD_ITEM(d, &d->ram->release_ring, item); - assert(item); - *item = 0; + + ring = &d->ram->release_ring; + prod = ring->prod & SPICE_RING_INDEX_MASK(ring); + assert(prod < ARRAY_SIZE(ring->items)); + ring->items[prod].el = 0; + qxl_ring_set_dirty(d); } @@ -732,7 +718,7 @@ static int interface_req_cmd_notification(QXLInstance *sin) static inline void qxl_push_free_res(PCIQXLDevice *d, int flush) { QXLReleaseRing *ring = &d->ram->release_ring; - uint64_t *item; + uint32_t prod; int notify; #define QXL_FREE_BUNCH_SIZE 32 @@ -759,11 +745,15 @@ static inline void qxl_push_free_res(PCIQXLDevice *d, int flush) if (notify) { qxl_send_events(d, QXL_INTERRUPT_DISPLAY); } - SPICE_RING_PROD_ITEM(d, ring, item); - if (!item) { + + ring = &d->ram->release_ring; + prod = ring->prod & SPICE_RING_INDEX_MASK(ring); + if (prod >= ARRAY_SIZE(ring->items)) { + qxl_set_guest_bug(d, "SPICE_RING_PROD_ITEM indices mismatch " + "%u >= %zu", prod, ARRAY_SIZE(ring->items)); return; } - *item = 0; + ring->items[prod].el = 0; d->num_free_res = 0; d->last_release = NULL; qxl_ring_set_dirty(d); @@ -775,7 +765,8 @@ static void interface_release_resource(QXLInstance *sin, { PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); QXLReleaseRing *ring; - uint64_t *item, id; + uint32_t prod; + uint64_t id; if (ext.group_id == MEMSLOT_GROUP_HOST) { /* host group -> vga mode update request */ @@ -792,16 +783,18 @@ static void interface_release_resource(QXLInstance *sin, * pci bar 0, $command.release_info */ ring = &qxl->ram->release_ring; - SPICE_RING_PROD_ITEM(qxl, ring, item); - if (!item) { + prod = ring->prod & SPICE_RING_INDEX_MASK(ring); + if (prod >= ARRAY_SIZE(ring->items)) { + qxl_set_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch " + "%u >= %zu", prod, ARRAY_SIZE(ring->items)); return; } - if (*item == 0) { + if (ring->items[prod].el == 0) { /* stick head into the ring */ id = ext.info->id; ext.info->next = 0; qxl_ram_set_dirty(qxl, &ext.info->next); - *item = id; + ring->items[prod].el = id; qxl_ring_set_dirty(qxl); } else { /* append item to the list */ -- 2.20.1
next prev parent reply other threads:[~2019-04-12 12:16 UTC|newest] Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-04-12 12:16 [Qemu-devel] [PATCH v2 0/5] misc set of fixes for warnings under GCC 9 Daniel P. Berrangé 2019-04-12 12:16 ` Daniel P. Berrangé 2019-04-12 12:16 ` [Qemu-devel] [PATCH v2 1/5] linux-user: avoid string truncation warnings in uname field copying Daniel P. Berrangé 2019-04-12 12:16 ` Daniel P. Berrangé 2019-04-12 12:28 ` Laurent Vivier 2019-04-12 12:16 ` [Qemu-devel] [PATCH v2 2/5] linux-user: avoid string truncation warnings in elf " Daniel P. Berrangé 2019-04-12 12:16 ` Daniel P. Berrangé 2019-04-12 12:32 ` Laurent Vivier 2019-04-12 12:16 ` [Qemu-devel] [PATCH v2 3/5] sockets: avoid string truncation warnings when copying UNIX path Daniel P. Berrangé 2019-04-12 12:16 ` Daniel P. Berrangé 2019-05-02 15:45 ` Laurent Vivier 2019-05-02 15:48 ` Daniel P. Berrangé 2019-05-02 15:48 ` Daniel P. Berrangé 2019-05-02 16:18 ` Laurent Vivier 2019-04-12 12:16 ` [Qemu-devel] [PATCH v2 4/5] hw/usb: avoid format truncation warning when formatting port name Daniel P. Berrangé 2019-04-12 12:16 ` Daniel P. Berrangé 2019-05-02 6:44 ` Gerd Hoffmann 2019-05-02 6:44 ` Gerd Hoffmann 2019-04-12 12:16 ` Daniel P. Berrangé [this message] 2019-04-12 12:16 ` [Qemu-devel] [PATCH v2 5/5] qxl: avoid unaligned pointer reads/writes Daniel P. Berrangé 2019-05-07 7:54 ` Gerd Hoffmann 2019-05-07 8:11 ` Philippe Mathieu-Daudé 2019-05-07 8:53 ` Gerd Hoffmann
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190412121626.19829-6-berrange@redhat.com \ --to=berrange@redhat.com \ --cc=kraxel@redhat.com \ --cc=laurent@vivier.eu \ --cc=qemu-devel@nongnu.org \ --cc=riku.voipio@iki.fi \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.