[Qemu-devel] [PATCH] do boundary check based on absolute value

[Qemu-devel] [PATCH] do boundary check based on absolute value

[Glauber Costa]

For backward operations, dstpitch and srcpitch can
be negative. This leads BLTUNSAFE macro into an
overflow, and as a result, it avoids performing
operations that are perfectly valid.

The visible effect that led to that patch was the gnome-panel
bar in Fedora10. Before this patch, you could see garbage
clobbering a big portion of the bar.

After this patch, this garbage is gone.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/cirrus_vga.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index e0cf458..5690719 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -221,15 +221,17 @@
 #define CIRRUS_HOOK_NOT_HANDLED 0
 #define CIRRUS_HOOK_HANDLED 1
 
+#define ABS(a) ((signed)(a) > 0 ? a : -a)
+
 #define BLTUNSAFE(s) \
     ( \
         ( /* check dst is within bounds */ \
-            (s)->cirrus_blt_height * (s)->cirrus_blt_dstpitch \
+            (s)->cirrus_blt_height * ABS((s)->cirrus_blt_dstpitch) \
                 + ((s)->cirrus_blt_dstaddr & (s)->cirrus_addr_mask) > \
                     (s)->vram_size \
         ) || \
         ( /* check src is within bounds */ \
-            (s)->cirrus_blt_height * (s)->cirrus_blt_srcpitch \
+            (s)->cirrus_blt_height * ABS((s)->cirrus_blt_srcpitch) \
                 + ((s)->cirrus_blt_srcaddr & (s)->cirrus_addr_mask) > \
                     (s)->vram_size \
         ) \
-- 
1.5.6.5

[Qemu-devel] Re: [PATCH] do boundary check based on absolute value

[Anthony Liguori]

Glauber Costa wrote:
> For backward operations, dstpitch and srcpitch can
> be negative. This leads BLTUNSAFE macro into an
> overflow, and as a result, it avoids performing
> operations that are perfectly valid.
>
> The visible effect that led to that patch was the gnome-panel
> bar in Fedora10. Before this patch, you could see garbage
> clobbering a big portion of the bar.
>
> After this patch, this garbage is gone.
>
>   

Applied.  Thanks.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] do boundary check based on absolute value

[René Rebe]

Hi,

Glauber Costa wrote:
> For backward operations, dstpitch and srcpitch can
> be negative. This leads BLTUNSAFE macro into an
> overflow, and as a result, it avoids performing
> operations that are perfectly valid.
>
> The visible effect that led to that patch was the gnome-panel
> bar in Fedora10. Before this patch, you could see garbage
> clobbering a big portion of the bar.
>
> After this patch, this garbage is gone.
>   
Confirmed to fix corruption with simple window mangers like
blackbox/fluxbox when moving windows out of the screen
bounds, which had on my TODO for review.
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  hw/cirrus_vga.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index e0cf458..5690719 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -221,15 +221,17 @@
>  #define CIRRUS_HOOK_NOT_HANDLED 0
>  #define CIRRUS_HOOK_HANDLED 1
>  
> +#define ABS(a) ((signed)(a) > 0 ? a : -a)
> +
>  #define BLTUNSAFE(s) \
>      ( \
>          ( /* check dst is within bounds */ \
> -            (s)->cirrus_blt_height * (s)->cirrus_blt_dstpitch \
> +            (s)->cirrus_blt_height * ABS((s)->cirrus_blt_dstpitch) \
>                  + ((s)->cirrus_blt_dstaddr & (s)->cirrus_addr_mask) > \
>                      (s)->vram_size \
>          ) || \
>          ( /* check src is within bounds */ \
> -            (s)->cirrus_blt_height * (s)->cirrus_blt_srcpitch \
> +            (s)->cirrus_blt_height * ABS((s)->cirrus_blt_srcpitch) \
>                  + ((s)->cirrus_blt_srcaddr & (s)->cirrus_addr_mask) > \
>                      (s)->vram_size \
>          ) \
>   

-- 
  René Rebe - ExactCODE GmbH - Europe, Germany, Berlin
  http://exactcode.de | http://t2-project.org | http://rene.rebe.name

Re: [Qemu-devel] [PATCH] do boundary check based on absolute value

[Glauber Costa]

>
> Confirmed to fix corruption with simple window mangers like
> blackbox/fluxbox when moving windows out of the screen
> bounds, which had on my TODO for review.

Great!

-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."