Re: [Qemu-devel] [PATCH] qxl: add sanity check

[Laszlo Ersek <lersek@redhat.com>]

On 02/19/14 11:40, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/qxl.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 1471cc0..2a559eb 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -1429,7 +1429,7 @@ static int qxl_destroy_primary(PCIQXLDevice *d, qxl_async_io async)
>      return 1;
>  }
>  
> -static void qxl_set_mode(PCIQXLDevice *d, int modenr, int loadvm)
> +static void qxl_set_mode(PCIQXLDevice *d, unsigned int modenr, int loadvm)
>  {
>      pcibus_t start = d->pci.io_regions[QXL_RAM_RANGE_INDEX].addr;
>      pcibus_t end   = d->pci.io_regions[QXL_RAM_RANGE_INDEX].size + start;
> @@ -1439,6 +1439,12 @@ static void qxl_set_mode(PCIQXLDevice *d, int modenr, int loadvm)
>          .mem_start = start,
>          .mem_end = end
>      };
> +
> +    if (modenr >= d->modes->n_modes) {
> +        qxl_set_guest_bug(d, "mode number out of range");
> +        return;
> +    }
> +
>      QXLSurfaceCreate surface = {
>          .width      = mode->x_res,
>          .height     = mode->y_res,
> 

Well, if I want to obsess about standards conformance, this is too late,
because the initialization of the "mode" pointer a bit higher up:

    QXLMode *mode = d->modes->modes + modenr;

already invokes undefined behavior, when modenr is out of range.

In practice, meh -- the check is done early enough to prevent
dereferencing the (already undefined) pointer.

I also guess gcc is *not* smart enough to derive the undefined-ness as
soon as we do the wrong initialization. (Because if it were smart enough
to see that, then it would compile the check you're adding into
"constant false".)

Also, isn't this CVE material?...

Reviewed-by: Laszlo Ersek <lersek@redhat.com>