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

[Frediano Ziglio]

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

The QXLMessage has no size so potentially a guest could give an
address that cause the string to overflow out of the video memory.
The current solution is to parse the message, release the resources
associated without printing the message from the client.
This also considering that the QXLMessage usage was deprecated
a while ago (I don't know exactly when).
This patches limit the string to 100000 characters (guest can feed
so much logs in other way) and limit to video memory.
Marc-Andre time ago sent a patch to remove entirely the handling
of this message however I rejected pointing out that this would
cause a leak in any guest using it.
Another solution would be to just release the guest resource
and mark as definitively obsolete this message.

Frediano

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