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