From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33984) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WG5O2-0008RV-IN for qemu-devel@nongnu.org; Wed, 19 Feb 2014 06:32:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WG5Nw-00088k-JO for qemu-devel@nongnu.org; Wed, 19 Feb 2014 06:32:22 -0500 Received: from mx1.redhat.com ([209.132.183.28]:16349) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WG5Nw-00088g-As for qemu-devel@nongnu.org; Wed, 19 Feb 2014 06:32:16 -0500 Message-ID: <5304963D.9000401@redhat.com> Date: Wed, 19 Feb 2014 12:32:13 +0100 From: Laszlo Ersek MIME-Version: 1.0 References: <1392806450-3452-1-git-send-email-kraxel@redhat.com> In-Reply-To: <1392806450-3452-1-git-send-email-kraxel@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qxl: add sanity check List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: spice-devel@lists.freedesktop.org, qemu-devel@nongnu.org On 02/19/14 11:40, Gerd Hoffmann wrote: > Signed-off-by: Gerd Hoffmann > --- > 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