Re: [Qemu-devel] [PATCH 08/14] qxl: avoid unaligned pointer reads/writes

[Marc-André Lureau]

Hi

On Fri, Mar 29, 2019 at 12:20 PM Daniel P. Berrangé <address@hidden> wrote:
>
> 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é <address@hidden>

I didn't notice your patch, I sent a different one which didn't inline
as much code:

https://patchew.org/QEMU/address@hidden/

> ---
>  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
>
>


-- 
Marc-André Lureau