Patchwork cirrus_vga: fix division by 0 for color expansion rop

login
register
mail settings
Submitter Aurelien Jarno
Date Dec. 31, 2010, 7:11 p.m.
Message ID <1293822678-4100-1-git-send-email-aurelien@aurel32.net>
Download mbox | patch
Permalink /patch/77105/
State New
Headers show

Comments

Aurelien Jarno - Dec. 31, 2010, 7:11 p.m.
Commit d85d0d3883f5a567fa2969a0396e42e0a662b3fa introduces a regression
with Windows ME that leads to a division by 0 and a crash.

It uses the color expansion rop with the source pitch set to 0. This is
something allowed, as the manual explicitely says "When the source of
color-expand data is display memory, the source pitch is ignored.".

This patch fixes this regression by computing sx, sy and others
variables only if they are going to be used later, that is for a plain
copy ROP. It basically consists in moving code.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 hw/cirrus_vga.c |   65 ++++++++++++++++++++++++++++---------------------------
 1 files changed, 33 insertions(+), 32 deletions(-)
Andreas Färber - Dec. 31, 2010, 11:21 p.m.
Am 31.12.2010 um 20:11 schrieb Aurelien Jarno:

> Commit d85d0d3883f5a567fa2969a0396e42e0a662b3fa introduces a  
> regression
> with Windows ME that leads to a division by 0 and a crash.
>
> It uses the color expansion rop with the source pitch set to 0. This  
> is
> something allowed, as the manual explicitely says "When the source of
> color-expand data is display memory, the source pitch is ignored.".
>
> This patch fixes this regression by computing sx, sy and others
> variables only if they are going to be used later, that is for a plain
> copy ROP. It basically consists in moving code.
>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
> hw/cirrus_vga.c |   65 +++++++++++++++++++++++++++ 
> +---------------------------
> 1 files changed, 33 insertions(+), 32 deletions(-)
>
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index 4f5040c..d4f2556 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -675,44 +675,45 @@ static void cirrus_do_copy(CirrusVGAState *s,  
> int dst, int src, int w, int h)
> {
>   int sx, sy;
>   int dx, dy;
> -    int width, height;
>   int depth;
>   int notify = 0;
>
> -    depth = s->vga.get_bpp(&s->vga) / 8;
> -    s->vga.get_resolution(&s->vga, &width, &height);
> -
> -    /* extra x, y */
> -    sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;
> -    sy = (src / ABS(s->cirrus_blt_srcpitch));
> -    dx = (dst % ABS(s->cirrus_blt_dstpitch)) / depth;
> -    dy = (dst / ABS(s->cirrus_blt_dstpitch));
> -
> -    /* normalize width */
> -    w /= depth;
> -
> -    /* if we're doing a backward copy, we have to adjust
> -       our x/y to be the upper left corner (instead of the lower
> -       right corner) */
> -    if (s->cirrus_blt_dstpitch < 0) {
> -	sx -= (s->cirrus_blt_width / depth) - 1;
> -	dx -= (s->cirrus_blt_width / depth) - 1;
> -	sy -= s->cirrus_blt_height - 1;
> -	dy -= s->cirrus_blt_height - 1;
> -    }
> +    /* make to sure only copy if it's a plain copy ROP */

"make sure to ..." while you're at it.

> +    if (*s->cirrus_rop == cirrus_bitblt_rop_fwd_src ||
> +        *s->cirrus_rop == cirrus_bitblt_rop_bkwd_src) {
> +
> +        int width, height;
> +
> +        depth = s->vga.get_bpp(&s->vga) / 8;
> +        s->vga.get_resolution(&s->vga, &width, &height);
> +
> +        /* extra x, y */
> +        sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;
> +        sy = (src / ABS(s->cirrus_blt_srcpitch));
> +        dx = (dst % ABS(s->cirrus_blt_dstpitch)) / depth;
> +        dy = (dst / ABS(s->cirrus_blt_dstpitch));
> +
> +        /* normalize width */
> +        w /= depth;
> +
> +        /* if we're doing a backward copy, we have to adjust
> +           our x/y to be the upper left corner (instead of the lower
> +           right corner) */
> +        if (s->cirrus_blt_dstpitch < 0) {
> +            sx -= (s->cirrus_blt_width / depth) - 1;
> +            dx -= (s->cirrus_blt_width / depth) - 1;
> +            sy -= s->cirrus_blt_height - 1;
> +            dy -= s->cirrus_blt_height - 1;
> +        }
>
> -    /* are we in the visible portion of memory? */
> -    if (sx >= 0 && sy >= 0 && dx >= 0 && dy >= 0 &&
> -	(sx + w) <= width && (sy + h) <= height &&
> -	(dx + w) <= width && (dy + h) <= height) {
> -	notify = 1;
> +        /* are we in the visible portion of memory? */
> +        if (sx >= 0 && sy >= 0 && dx >= 0 && dy >= 0 &&
> +            (sx + w) <= width && (sy + h) <= height &&
> +            (dx + w) <= width && (dy + h) <= height) {
> +            notify = 1;
> +        }
>   }
>
> -    /* make to sure only copy if it's a plain copy ROP */
> -    if (*s->cirrus_rop != cirrus_bitblt_rop_fwd_src &&
> -	*s->cirrus_rop != cirrus_bitblt_rop_bkwd_src)
> -	notify = 0;
> -
>   /* we have to flush all pending changes so that the copy
>      is generated at the appropriate moment in time */
>   if (notify)
> -- 
> 1.7.2.3
>
>
Aurelien Jarno - Jan. 1, 2011, 2:16 p.m.
On Fri, Dec 31, 2010 at 08:11:18PM +0100, Aurelien Jarno wrote:
> Commit d85d0d3883f5a567fa2969a0396e42e0a662b3fa introduces a regression
> with Windows ME that leads to a division by 0 and a crash.
> 
> It uses the color expansion rop with the source pitch set to 0. This is
> something allowed, as the manual explicitely says "When the source of
> color-expand data is display memory, the source pitch is ignored.".
> 
> This patch fixes this regression by computing sx, sy and others
> variables only if they are going to be used later, that is for a plain
> copy ROP. It basically consists in moving code.
> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  hw/cirrus_vga.c |   65 ++++++++++++++++++++++++++++---------------------------
>  1 files changed, 33 insertions(+), 32 deletions(-)

I have just discovered that this patch fixes bug #604166:

https://bugs.launchpad.net/qemu/+bug/604166

Patch

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 4f5040c..d4f2556 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -675,44 +675,45 @@  static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
 {
     int sx, sy;
     int dx, dy;
-    int width, height;
     int depth;
     int notify = 0;
 
-    depth = s->vga.get_bpp(&s->vga) / 8;
-    s->vga.get_resolution(&s->vga, &width, &height);
-
-    /* extra x, y */
-    sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;
-    sy = (src / ABS(s->cirrus_blt_srcpitch));
-    dx = (dst % ABS(s->cirrus_blt_dstpitch)) / depth;
-    dy = (dst / ABS(s->cirrus_blt_dstpitch));
-
-    /* normalize width */
-    w /= depth;
-
-    /* if we're doing a backward copy, we have to adjust
-       our x/y to be the upper left corner (instead of the lower
-       right corner) */
-    if (s->cirrus_blt_dstpitch < 0) {
-	sx -= (s->cirrus_blt_width / depth) - 1;
-	dx -= (s->cirrus_blt_width / depth) - 1;
-	sy -= s->cirrus_blt_height - 1;
-	dy -= s->cirrus_blt_height - 1;
-    }
+    /* make to sure only copy if it's a plain copy ROP */
+    if (*s->cirrus_rop == cirrus_bitblt_rop_fwd_src ||
+        *s->cirrus_rop == cirrus_bitblt_rop_bkwd_src) {
+
+        int width, height;
+
+        depth = s->vga.get_bpp(&s->vga) / 8;
+        s->vga.get_resolution(&s->vga, &width, &height);
+
+        /* extra x, y */
+        sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;
+        sy = (src / ABS(s->cirrus_blt_srcpitch));
+        dx = (dst % ABS(s->cirrus_blt_dstpitch)) / depth;
+        dy = (dst / ABS(s->cirrus_blt_dstpitch));
+
+        /* normalize width */
+        w /= depth;
+
+        /* if we're doing a backward copy, we have to adjust
+           our x/y to be the upper left corner (instead of the lower
+           right corner) */
+        if (s->cirrus_blt_dstpitch < 0) {
+            sx -= (s->cirrus_blt_width / depth) - 1;
+            dx -= (s->cirrus_blt_width / depth) - 1;
+            sy -= s->cirrus_blt_height - 1;
+            dy -= s->cirrus_blt_height - 1;
+        }
 
-    /* are we in the visible portion of memory? */
-    if (sx >= 0 && sy >= 0 && dx >= 0 && dy >= 0 &&
-	(sx + w) <= width && (sy + h) <= height &&
-	(dx + w) <= width && (dy + h) <= height) {
-	notify = 1;
+        /* are we in the visible portion of memory? */
+        if (sx >= 0 && sy >= 0 && dx >= 0 && dy >= 0 &&
+            (sx + w) <= width && (sy + h) <= height &&
+            (dx + w) <= width && (dy + h) <= height) {
+            notify = 1;
+        }
     }
 
-    /* make to sure only copy if it's a plain copy ROP */
-    if (*s->cirrus_rop != cirrus_bitblt_rop_fwd_src &&
-	*s->cirrus_rop != cirrus_bitblt_rop_bkwd_src)
-	notify = 0;
-
     /* we have to flush all pending changes so that the copy
        is generated at the appropriate moment in time */
     if (notify)