[Spice-devel] qxl: add sanity check

Submitted by Gerd Hoffmann on Feb. 19, 2014, 10:40 a.m.

Details

Message ID 1392806450-3452-1-git-send-email-kraxel@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Gerd Hoffmann Feb. 19, 2014, 10:40 a.m.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

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,

Comments

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>
Looks good, ACK.

Christophe

On Wed, Feb 19, 2014 at 11:40:50AM +0100, 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,
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel