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

[Jonathon Jongsma]

On Thu, 2016-10-20 at 06:08 -0400, Frediano Ziglio wrote:
> > 
> > 
> > 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


OK, can you add a bit of this back story to the commit log?

Acked-by: Jonathon Jongsma <jjongsma at redhat.com>



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