[Spice-devel] [PATCH 12/16] worker: remove useless QXL_CMD_MESSAGE

[Frediano Ziglio]

> 
> From: Marc-André Lureau <marcandre.lureau at gmail.com>
> 
> Unsecure code shouldn't be compiled in even in debug mode.
> Doesn't seem to be used, and probably should be handled at qemu qxl
> driver level instead.
> ---
>  server/red_parse_qxl.c | 26 --------------------------
>  server/red_parse_qxl.h |  4 ----
>  server/red_worker.c    | 18 ------------------
>  3 files changed, 48 deletions(-)
> 
> diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
> index 9464c7a..f9b3dc1 100644
> --- a/server/red_parse_qxl.c
> +++ b/server/red_parse_qxl.c
> @@ -1261,32 +1261,6 @@ void red_put_update_cmd(RedUpdateCmd *red)
>      /* nothing yet */
>  }
>  
> -int red_get_message(RedMemSlotInfo *slots, int group_id,
> -                    RedMessage *red, QXLPHYSICAL addr)
> -{
> -    QXLMessage *qxl;
> -    int error;
> -
> -    /*
> -     * security alert:
> -     *   qxl->data[0] size isn't specified anywhere -> can't verify
> -     *   luckily this is for debug logging only,
> -     *   so we can just ignore it by default.
> -     */
> -    qxl = (QXLMessage *)memslot_get_virt(slots, addr, sizeof(*qxl),
> group_id, &error);
> -    if (error) {
> -        return 1;
> -    }
> -    red->release_info  = &qxl->release_info;
> -    red->data          = qxl->data;
> -    return 0;
> -}
> -
> -void red_put_message(RedMessage *red)
> -{
> -    /* nothing yet */
> -}
> -
>  static unsigned int surface_format_to_bpp(uint32_t format)
>  {
>      switch (format) {
> diff --git a/server/red_parse_qxl.h b/server/red_parse_qxl.h
> index 09059f5..26abd88 100644
> --- a/server/red_parse_qxl.h
> +++ b/server/red_parse_qxl.h
> @@ -130,10 +130,6 @@ int red_get_update_cmd(RedMemSlotInfo *slots, int
> group_id,
>                         RedUpdateCmd *red, QXLPHYSICAL addr);
>  void red_put_update_cmd(RedUpdateCmd *red);
>  
> -int red_get_message(RedMemSlotInfo *slots, int group_id,
> -                    RedMessage *red, QXLPHYSICAL addr);
> -void red_put_message(RedMessage *red);
> -
>  int red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
>                          RedSurfaceCmd *red, QXLPHYSICAL addr);
>  void red_put_surface_cmd(RedSurfaceCmd *red);
> diff --git a/server/red_worker.c b/server/red_worker.c
> index cf20ccd..c65f1b2 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -313,24 +313,6 @@ static int red_process_commands(RedWorker *worker,
> uint32_t max_pipe_size, int *
>              red_put_update_cmd(&update);
>              break;
>          }
> -        case QXL_CMD_MESSAGE: {
> -            RedMessage message;
> -            QXLReleaseInfoExt release_info_ext;
> -
> -            if (red_get_message(&worker->mem_slots, ext_cmd.group_id,
> -                                &message, ext_cmd.cmd.data)) {
> -                break;
> -            }
> -#ifdef DEBUG
> -            /* alert: accessing message.data is insecure */
> -            spice_warning("MESSAGE: %s", message.data);
> -#endif
> -            release_info_ext.group_id = ext_cmd.group_id;
> -            release_info_ext.info = message.release_info;
> -            worker->qxl->st->qif->release_resource(worker->qxl,
> release_info_ext);
> -            red_put_message(&message);
> -            break;
> -        }
>          case QXL_CMD_SURFACE: {
>              RedSurfaceCmd *surface = spice_new0(RedSurfaceCmd, 1);
>  

I strongly against this patch.
Main reason is we should at least release the resource to guest.
Actually all code is removed so we will also have an "bad command type" error for free

Frediano