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@xxxxxxxxxx>
>
> >
> >
> > On Tue, 2016-10-18 at 10:09 +0100, Frediano Ziglio wrote:
> > >
> > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > > ---
> > > 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);
> >
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel