[Spice-devel] [PATCH spice-server 05/12] Make QXLMessage handling safe

[Jonathon Jongsma]

I think that a commit saying "Make X safe" really deserves a bit longer
description about what was unsafe, whether it's easily reproducible,
what the impact of the bug is, etc.  


On Tue, 2016-10-18 at 10:09 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/memslot.c       | 14 ++++++++++++++
>  server/memslot.h       |  3 +++
>  server/red-parse-qxl.c | 11 +++++++++++
>  server/red-parse-qxl.h |  1 +
>  server/red-worker.c    |  3 +--
>  5 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/server/memslot.c b/server/memslot.c
> index 99c63e4..75cb75f 100644
> --- a/server/memslot.c
> +++ b/server/memslot.c
> @@ -71,6 +71,20 @@ int memslot_validate_virt(RedMemSlotInfo *info,
> unsigned long virt, int slot_id,
>      return 1;
>  }
>  
> +unsigned long memslot_max_size_virt(RedMemSlotInfo *info,
> +                                    unsigned long virt, int slot_id,
> +                                    uint32_t group_id)
> +{
> +    MemSlot *slot;
> +
> +    slot = &info->mem_slots[group_id][slot_id];
> +
> +    if (virt < slot->virt_start_addr || virt > slot->virt_end_addr)
> {
> +        return 0;
> +    }
> +    return slot->virt_end_addr - virt;
> +}
> +
>  /*
>   * return virtual address if successful, which may be 0.
>   * returns 0 and sets error to 1 if an error condition occurs.
> diff --git a/server/memslot.h b/server/memslot.h
> index a29837c..5046d0f 100644
> --- a/server/memslot.h
> +++ b/server/memslot.h
> @@ -55,6 +55,9 @@ static inline int
> memslot_get_generation(RedMemSlotInfo *info, uint64_t addr)
>  
>  int memslot_validate_virt(RedMemSlotInfo *info, unsigned long virt,
> int slot_id,
>                            uint32_t add_size, uint32_t group_id);
> +unsigned long memslot_max_size_virt(RedMemSlotInfo *info,
> +                                    unsigned long virt, int slot_id,
> +                                    uint32_t group_id);
>  unsigned long memslot_get_virt(RedMemSlotInfo *info, QXLPHYSICAL
> addr, uint32_t add_size,
>                         int group_id, int *error);
>  
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index d75e27e..77533ca 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -1300,6 +1300,9 @@ int red_get_message(RedMemSlotInfo *slots, int
> group_id,
>  {
>      QXLMessage *qxl;
>      int error;
> +    int memslot_id;
> +    unsigned long len;
> +    uint8_t *end;
>  
>      /*
>       * security alert:
> @@ -1314,6 +1317,14 @@ int red_get_message(RedMemSlotInfo *slots, int
> group_id,
>      red->release_info_ext.info      = &qxl->release_info;
>      red->release_info_ext.group_id  = group_id;
>      red->data                       = qxl->data;
> +    memslot_id = memslot_get_id(slots, addr+sizeof(*qxl));
> +    len = memslot_max_size_virt(slots, ((intptr_t)
> qxl)+sizeof(*qxl), memslot_id, group_id);
> +    len = MIN(len, 100000);
> +    end = (uint8_t *)memchr(qxl->data, 0, len);
> +    if (end == NULL) {
> +        return 1;
> +    }
> +    red->len = end - qxl->data;
>      return 0;
>  }
>  
> diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
> index 0da20ad..e174ccc 100644
> --- a/server/red-parse-qxl.h
> +++ b/server/red-parse-qxl.h
> @@ -74,6 +74,7 @@ typedef struct RedUpdateCmd {
>  
>  typedef struct RedMessage {
>      QXLReleaseInfoExt release_info_ext;
> +    int len;
>      uint8_t *data;
>  } RedMessage;
>  
> diff --git a/server/red-worker.c b/server/red-worker.c
> index bbc971c..3784261 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -238,8 +238,7 @@ static int red_process_display(RedWorker *worker,
> int *ring_is_empty)
>                  break;
>              }
>  #ifdef DEBUG
> -            /* alert: accessing message.data is insecure */
> -            spice_warning("MESSAGE: %s", message.data);
> +            spice_warning("MESSAGE: %.*s", message.len,
> message.data);
>  #endif
>              red_qxl_release_resource(worker->qxl,
> message.release_info_ext);
>              red_put_message(&message);