[Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value

[Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

In Cirrus CLGD 54xx VGA Emulator, if cirrus graphics mode is VGA,
'cirrus_get_bpp' returns zero(0), which could lead to a divide
by zero error in while copying pixel data. The same could occur
via blit pitch values. Add check to avoid it.

Reported-by: Huawei PSIRT <psirt@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/display/cirrus_vga.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 3d712d5..bdb092e 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -272,6 +272,9 @@ static void cirrus_update_memory_access(CirrusVGAState *s);
 static bool blit_region_is_unsafe(struct CirrusVGAState *s,
                                   int32_t pitch, int32_t addr)
 {
+    if (!pitch) {
+        return true;
+    }
     if (pitch < 0) {
         int64_t min = addr
             + ((int64_t)s->cirrus_blt_height-1) * pitch;
@@ -715,7 +718,7 @@ static int cirrus_bitblt_videotovideo_patterncopy(CirrusVGAState * s)
                                             s->cirrus_addr_mask));
 }
 
-static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
+static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
 {
     int sx = 0, sy = 0;
     int dx = 0, dy = 0;
@@ -729,6 +732,9 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
         int width, height;
 
         depth = s->vga.get_bpp(&s->vga) / 8;
+        if (!depth) {
+            return 0;
+        }
         s->vga.get_resolution(&s->vga, &width, &height);
 
         /* extra x, y */
@@ -783,6 +789,8 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
     cirrus_invalidate_region(s, s->cirrus_blt_dstaddr,
 				s->cirrus_blt_dstpitch, s->cirrus_blt_width,
 				s->cirrus_blt_height);
+
+    return 1;
 }
 
 static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)
@@ -790,11 +798,9 @@ static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)
     if (blit_is_unsafe(s))
         return 0;
 
-    cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr,
+    return cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr,
             s->cirrus_blt_srcaddr - s->vga.start_addr,
             s->cirrus_blt_width, s->cirrus_blt_height);
-
-    return 1;
 }
 
 /***************************************
-- 
2.7.4

Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value

[P J P]

+-- On Tue, 18 Oct 2016, P J P wrote --+
| From: Prasad J Pandit <pjp@fedoraproject.org>
| 
| In Cirrus CLGD 54xx VGA Emulator, if cirrus graphics mode is VGA,
| 'cirrus_get_bpp' returns zero(0), which could lead to a divide
| by zero error in while copying pixel data. The same could occur
| via blit pitch values. Add check to avoid it.
| 
| Reported-by: Huawei PSIRT <psirt@huawei.com>
| Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
| ---
|  hw/display/cirrus_vga.c | 14 ++++++++++----
|  1 file changed, 10 insertions(+), 4 deletions(-)
| 

Ping...!
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value

[Marc-André Lureau]

Hi

On Tue, Oct 18, 2016 at 11:46 AM P J P <ppandit@redhat.com> wrote:

> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> In Cirrus CLGD 54xx VGA Emulator, if cirrus graphics mode is VGA,
> 'cirrus_get_bpp' returns zero(0), which could lead to a divide
> by zero error in while copying pixel data. The same could occur
> via blit pitch values. Add check to avoid it.
>

For completeness, do you have a reproducer and/or a backtrace?


>
> Reported-by: Huawei PSIRT <psirt@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/display/cirrus_vga.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index 3d712d5..bdb092e 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -272,6 +272,9 @@ static void cirrus_update_memory_access(CirrusVGAState
> *s);
>  static bool blit_region_is_unsafe(struct CirrusVGAState *s,
>                                    int32_t pitch, int32_t addr)
>  {
> +    if (!pitch) {
> +        return true;
> +    }
>

That doesn't look directly related to 'cirrus_get_bpp', care to explain?

     if (pitch < 0) {
>          int64_t min = addr
>              + ((int64_t)s->cirrus_blt_height-1) * pitch;
> @@ -715,7 +718,7 @@ static int
> cirrus_bitblt_videotovideo_patterncopy(CirrusVGAState * s)
>                                              s->cirrus_addr_mask));
>  }
>
> -static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w,
> int h)
> +static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int
> h)
>  {
>      int sx = 0, sy = 0;
>      int dx = 0, dy = 0;
> @@ -729,6 +732,9 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst,
> int src, int w, int h)
>          int width, height;
>
>          depth = s->vga.get_bpp(&s->vga) / 8;
> +        if (!depth) {
> +            return 0;
> +        }
>

Makes sense, since 'cirrus_get_bpp' would return 0 in VGA mode. But isn't
this a cirrus operation (not VGA), how did it get there? Perhaps this
should be catched earlier (invalid VGA operations).

         s->vga.get_resolution(&s->vga, &width, &height);
>
>          /* extra x, y */
> @@ -783,6 +789,8 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst,
> int src, int w, int h)
>      cirrus_invalidate_region(s, s->cirrus_blt_dstaddr,
>                                 s->cirrus_blt_dstpitch,
> s->cirrus_blt_width,
>                                 s->cirrus_blt_height);
> +
> +    return 1;
>  }
>
>  static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)
> @@ -790,11 +798,9 @@ static int
> cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)
>      if (blit_is_unsafe(s))
>          return 0;
>
> -    cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr,
> +    return cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr,
>              s->cirrus_blt_srcaddr - s->vga.start_addr,
>              s->cirrus_blt_width, s->cirrus_blt_height);
> -
> -    return 1;
>

btw, not directly related to your patch, but the code looks strange in
cirrus_bitblt_videotovideo(), cirrus_bitblt_reset() is called if(ret), and
later if (!ret) in cirrus_bitblt_start(), that looks a bit weird, but it
may be fine.

I hope someone more familiar with the code can help review your patch.

Thanks


 }
>
>  /***************************************
> --
> 2.7.4
>
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value

[P J P]

  Hello Marc, all

+-- On Wed, 16 Nov 2016, Marc-André Lureau wrote --+
| For completeness, do you have a reproducer and/or a backtrace?

Yes, there is.

===
Thread 4 "qemu-system-x86" received signal SIGFPE, Arithmetic exception.
[Switching to Thread 0x7ffff002c700 (LWP 10506)]
0x000055555599fe2e in cirrus_do_copy (s=0x55555758af60, dst=0, src=0, w=2048, 
h=4096) at hw/display/cirrus_vga.c:735
735             sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;

(gdb) bt
#0  0x000055555599fe2e in cirrus_do_copy (s=0x55555758af60, dst=0, src=0, w=2048, h=4096) at hw/display/cirrus_vga.c:735
#1  0x00005555559a0134 in cirrus_bitblt_videotovideo_copy (s=0x55555758af60) at hw/display/cirrus_vga.c:793
#2  0x00005555559a0609 in cirrus_bitblt_videotovideo (s=0x55555758af60) at hw/display/cirrus_vga.c:915
#3  0x00005555559a0d77 in cirrus_bitblt_start (s=0x55555758af60) at hw/display/cirrus_vga.c:1056
#4  0x00005555559a1ad3 in cirrus_vga_write_gr (s=0x55555758af60, reg_index=42, reg_value=0) at hw/display/cirrus_vga.c:1572
#5  0x00005555559a3ad8 in cirrus_vga_ioport_write (opaque=0x55555758af60, addr=975, val=0, size=1) at hw/display/cirrus_vga.c:2678
#6  0x00005555557a8df7 in memory_region_write_accessor (mr=0x55555759ba50, addr=31, ...
#7  0x00005555557a900f in access_with_adjusted_size (addr=31, value=0x7ffff002b8b8, ...
#8  0x00005555557ab74f in memory_region_dispatch_write (mr=0x55555759ba50, addr=31, ...
#9  0x0000555555757003 in address_space_write_continue (as=0x55555621b5a0 <address_space_io>, ...
#10 0x000055555575714b in address_space_write (as=0x55555621b5a0 <address_space_io>, ...
#11 0x00005555557574d7 in address_space_rw (as=0x55555621b5a0 <address_space_io>, ...
#12 0x00005555557a53d1 in kvm_handle_io (port=975, attrs=..., data=0x7ffff7ff0000, ...
#13 0x00005555557a58d7 in kvm_cpu_exec (cpu=0x555556746f90)
#14 0x000055555578c752 in qemu_kvm_cpu_thread_fn (arg=0x555556746f90)
#15 0x00007ffff5e8d5ca in start_thread () from /lib64/libpthread.so.0
#16 0x00007ffff5bc70ed in clone () from /lib64/libc.so.6
===

| > --- a/hw/display/cirrus_vga.c
| > +++ b/hw/display/cirrus_vga.c
| > @@ -272,6 +272,9 @@ static void cirrus_update_memory_access(CirrusVGAState
| > *s);
| >  static bool blit_region_is_unsafe(struct CirrusVGAState *s,
| >                                    int32_t pitch, int32_t addr)
| >  {
| > +    if (!pitch) {
| > +        return true;
| > +    }
| >
| 
| That doesn't look directly related to 'cirrus_get_bpp', care to explain?

  'blit_region_is_unsafe' is called from 'blit_is_unsafe' to check if blit 
parameters (cirrus_blt_srcpitch/cirrus_blt_dstpitch)  are safe for 
'cirrus_do_copy'. These too could lead to div by zero in cirrus_do_copy

    static int cirrus_do_copy(CirrusVGAState *s, ...)
    {
        ...
        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));
    }

| btw, not directly related to your patch, but the code looks strange in
| cirrus_bitblt_videotovideo(), cirrus_bitblt_reset() is called if(ret), and
| later if (!ret) in cirrus_bitblt_start(), that looks a bit weird, but it
| may be fine.

  I think that is to avoid calling 'cirrus_bitblt_reset' twice.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value

[P J P]

+-- On Mon, 28 Nov 2016, P J P wrote --+
| +-- On Wed, 16 Nov 2016, Marc-André Lureau wrote --+
| | For completeness, do you have a reproducer and/or a backtrace?
| 
| Yes, there is.
| 
| ===
| Thread 4 "qemu-system-x86" received signal SIGFPE, Arithmetic exception.
| [Switching to Thread 0x7ffff002c700 (LWP 10506)]
| 0x000055555599fe2e in cirrus_do_copy (s=0x55555758af60, dst=0, src=0, w=2048, 
| h=4096) at hw/display/cirrus_vga.c:735
| 735             sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;
| 
| (gdb) bt
| #0  0x000055555599fe2e in cirrus_do_copy (s=0x55555758af60, dst=0, src=0, w=2048, h=4096) at hw/display/cirrus_vga.c:735
| #1  0x00005555559a0134 in cirrus_bitblt_videotovideo_copy (s=0x55555758af60) at hw/display/cirrus_vga.c:793
| #2  0x00005555559a0609 in cirrus_bitblt_videotovideo (s=0x55555758af60) at hw/display/cirrus_vga.c:915
| #3  0x00005555559a0d77 in cirrus_bitblt_start (s=0x55555758af60) at hw/display/cirrus_vga.c:1056
| #4  0x00005555559a1ad3 in cirrus_vga_write_gr (s=0x55555758af60, reg_index=42, reg_value=0) at hw/display/cirrus_vga.c:1572
| #5  0x00005555559a3ad8 in cirrus_vga_ioport_write (opaque=0x55555758af60, addr=975, val=0, size=1) at hw/display/cirrus_vga.c:2678
| #6  0x00005555557a8df7 in memory_region_write_accessor (mr=0x55555759ba50, addr=31, ...
| #7  0x00005555557a900f in access_with_adjusted_size (addr=31, value=0x7ffff002b8b8, ...
| #8  0x00005555557ab74f in memory_region_dispatch_write (mr=0x55555759ba50, addr=31, ...
| #9  0x0000555555757003 in address_space_write_continue (as=0x55555621b5a0 <address_space_io>, ...
| #10 0x000055555575714b in address_space_write (as=0x55555621b5a0 <address_space_io>, ...
| #11 0x00005555557574d7 in address_space_rw (as=0x55555621b5a0 <address_space_io>, ...
| #12 0x00005555557a53d1 in kvm_handle_io (port=975, attrs=..., data=0x7ffff7ff0000, ...
| #13 0x00005555557a58d7 in kvm_cpu_exec (cpu=0x555556746f90)
| #14 0x000055555578c752 in qemu_kvm_cpu_thread_fn (arg=0x555556746f90)
| #15 0x00007ffff5e8d5ca in start_thread () from /lib64/libpthread.so.0
| #16 0x00007ffff5bc70ed in clone () from /lib64/libc.so.6
| ===
| 

Ping..!
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value

[Alberto Garcia]

On Mon, Nov 28, 2016 at 11:52:08AM +0530, P J P wrote:
> | > --- a/hw/display/cirrus_vga.c
> | > +++ b/hw/display/cirrus_vga.c
> | > @@ -272,6 +272,9 @@ static void cirrus_update_memory_access(CirrusVGAState
> | > *s);
> | >  static bool blit_region_is_unsafe(struct CirrusVGAState *s,
> | >                                    int32_t pitch, int32_t addr)
> | >  {
> | > +    if (!pitch) {
> | > +        return true;
> | > +    }
> | >
> | 
> | That doesn't look directly related to 'cirrus_get_bpp', care to explain?
> 
>   'blit_region_is_unsafe' is called from 'blit_is_unsafe' to check if blit 
> parameters (cirrus_blt_srcpitch/cirrus_blt_dstpitch)  are safe for 
> 'cirrus_do_copy'. These too could lead to div by zero in cirrus_do_copy

This change is causing display artifacts in QEMU 2.8.

What seems to happen is that blit_is_unsafe() is also called for
CIRRUS_BLTMODE_PATTERNCOPY, but in this case cirrus_blt_srcpitch is
not used. However, because of this new check if its value is 0 then
cirrus_bitblt_common_patterncopy() returns early and becomes a no-op.

Berto

Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value

[Gerd Hoffmann]

On Mi, 2017-01-11 at 16:59 +0200, Alberto Garcia wrote:
> On Mon, Nov 28, 2016 at 11:52:08AM +0530, P J P wrote:
> > | > --- a/hw/display/cirrus_vga.c
> > | > +++ b/hw/display/cirrus_vga.c
> > | > @@ -272,6 +272,9 @@ static void cirrus_update_memory_access(CirrusVGAState
> > | > *s);
> > | >  static bool blit_region_is_unsafe(struct CirrusVGAState *s,
> > | >                                    int32_t pitch, int32_t addr)
> > | >  {
> > | > +    if (!pitch) {
> > | > +        return true;
> > | > +    }
> > | >
> > | 
> > | That doesn't look directly related to 'cirrus_get_bpp', care to explain?
> > 
> >   'blit_region_is_unsafe' is called from 'blit_is_unsafe' to check if blit 
> > parameters (cirrus_blt_srcpitch/cirrus_blt_dstpitch)  are safe for 
> > 'cirrus_do_copy'. These too could lead to div by zero in cirrus_do_copy
> 
> This change is causing display artifacts in QEMU 2.8.
> 
> What seems to happen is that blit_is_unsafe() is also called for
> CIRRUS_BLTMODE_PATTERNCOPY, but in this case cirrus_blt_srcpitch is
> not used. However, because of this new check if its value is 0 then
> cirrus_bitblt_common_patterncopy() returns early and becomes a no-op.

inflight vga queue pull request has a fix for that.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value

[Wolfgang Bumiller]

On Wed, Jan 11, 2017 at 09:43:41PM +0100, Gerd Hoffmann wrote:
> On Mi, 2017-01-11 at 16:59 +0200, Alberto Garcia wrote:
> > On Mon, Nov 28, 2016 at 11:52:08AM +0530, P J P wrote:
> > > | > --- a/hw/display/cirrus_vga.c
> > > | > +++ b/hw/display/cirrus_vga.c
> > > | > @@ -272,6 +272,9 @@ static void cirrus_update_memory_access(CirrusVGAState
> > > | > *s);
> > > | >  static bool blit_region_is_unsafe(struct CirrusVGAState *s,
> > > | >                                    int32_t pitch, int32_t addr)
> > > | >  {
> > > | > +    if (!pitch) {
> > > | > +        return true;
> > > | > +    }
> > > | >
> > > | 
> > > | That doesn't look directly related to 'cirrus_get_bpp', care to explain?
> > > 
> > >   'blit_region_is_unsafe' is called from 'blit_is_unsafe' to check if blit 
> > > parameters (cirrus_blt_srcpitch/cirrus_blt_dstpitch)  are safe for 
> > > 'cirrus_do_copy'. These too could lead to div by zero in cirrus_do_copy
> > 
> > This change is causing display artifacts in QEMU 2.8.
> > 
> > What seems to happen is that blit_is_unsafe() is also called for
> > CIRRUS_BLTMODE_PATTERNCOPY, but in this case cirrus_blt_srcpitch is
> > not used. However, because of this new check if its value is 0 then
> > cirrus_bitblt_common_patterncopy() returns early and becomes a no-op.
> 
> inflight vga queue pull request has a fix for that.

Do you mean:
 [PATCH] display: cirrus: ignore source pitch value as needed in blit_is_unsafe
 (Message-Id: <20170109203520.5619-1-brogers@suse.com>)

Because I'm still seeing artifacts on some setups (eg. on win XP).
As far as I can tell the check is still too strong:
The rops used by cirrus_bitblt_common_patterncopy seem to only be using
the destination pitch as far as I can see (all functions in
cirrus_vga_rop2.h) and in my tests only the destination pitch got
filled in, the source pitch was left as zero. Adapting the check when
coming from cirrus_bitblt_common_patterncopy seems to fix the issue for
me.

Additionally (but this didn't have any visible effect in my test (and
shouldn't)) the cirrus_fill rops called from cirrus_bitblt_solidfill
don't actually divide by the pitch (as far as I can see) but just add
it to their destination offset (cirrus_vga_rop2.h around line 276?),
not sure if it makes sense to change how this is handled at all as a
zero pitch there would IMO produce artifacts with or without the check.
I just thought I'd point it out in case someone wanted to know.

What do you think of the patch below? (Applied on top of both other
patches)?

It could definitely use some auditing to see if I missed any of the
code paths, since it involves a bunch of function pointers fetched from
lists depending on parameters. Here's a debug print showing the
situtation in cirrus_bitblt_common_patterncopy() when the artifacts
occured:

    s->cirrus_blt_mode               = 0xc0,
    s->cirrus_blt_modeext            = 0x00,
      Inferred use of s->vga.gr[0x32] from above values:
    rop_to_index[s->vga.gr[0x32]]    = 5
      (should be ROP2(cirrus_colorexpand_pattern_src) ?)
    s->cirrus_blt_pixelwidth         = 2
    s->cirrus_blt_width              = 1242
    s->cirrus_blt_height             = 27
    s->cirrus_blt_srcpitch           = 0      <-- culprit
    s->cirrus_blt_dstpitch           = 2560


---- 8< ----

>From a3be50cc3e3bb0f5eb784d30048b88333366bdca Mon Sep 17 00:00:00 2001
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
Date: Fri, 20 Jan 2017 09:44:39 +0100
Subject: [PATCH] cirrus: allow zero source pitch in pattern fill rops

The rops used by cirrus_bitblt_common_patterncopy only use
the destination pitch, so the source pitch shoul allowed to
be zero.

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 hw/display/cirrus_vga.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 379910d..c2fce8c 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -272,9 +272,6 @@ static void cirrus_update_memory_access(CirrusVGAState *s);
 static bool blit_region_is_unsafe(struct CirrusVGAState *s,
                                   int32_t pitch, int32_t addr)
 {
-    if (!pitch) {
-        return true;
-    }
     if (pitch < 0) {
         int64_t min = addr
             + ((int64_t)s->cirrus_blt_height-1) * pitch;
@@ -294,7 +291,7 @@ static bool blit_region_is_unsafe(struct CirrusVGAState *s,
     return false;
 }
 
-static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only)
+static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only, bool zero_src_pitch_ok)
 {
     /* should be the case, see cirrus_bitblt_start */
     assert(s->cirrus_blt_width > 0);
@@ -304,6 +301,10 @@ static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only)
         return true;
     }
 
+    if (!s->cirrus_blt_dstpitch) {
+        return true;
+    }
+
     if (blit_region_is_unsafe(s, s->cirrus_blt_dstpitch,
                               s->cirrus_blt_dstaddr & s->cirrus_addr_mask)) {
         return true;
@@ -311,6 +312,11 @@ static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only)
     if (dst_only) {
         return false;
     }
+
+    if (!zero_src_pitch_ok && !s->cirrus_blt_srcpitch) {
+        return true;
+    }
+
     if (blit_region_is_unsafe(s, s->cirrus_blt_srcpitch,
                               s->cirrus_blt_srcaddr & s->cirrus_addr_mask)) {
         return true;
@@ -676,8 +682,9 @@ static int cirrus_bitblt_common_patterncopy(CirrusVGAState * s,
 
     dst = s->vga.vram_ptr + (s->cirrus_blt_dstaddr & s->cirrus_addr_mask);
 
-    if (blit_is_unsafe(s, false))
+    if (blit_is_unsafe(s, false, true)) {
         return 0;
+    }
 
     (*s->cirrus_rop) (s, dst, src,
                       s->cirrus_blt_dstpitch, 0,
@@ -694,7 +701,7 @@ static int cirrus_bitblt_solidfill(CirrusVGAState *s, int blt_rop)
 {
     cirrus_fill_t rop_func;
 
-    if (blit_is_unsafe(s, true)) {
+    if (blit_is_unsafe(s, true, true)) {
         return 0;
     }
     rop_func = cirrus_fill[rop_to_index[blt_rop]][s->cirrus_blt_pixelwidth - 1];
@@ -798,7 +805,7 @@ static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
 
 static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)
 {
-    if (blit_is_unsafe(s, false))
+    if (blit_is_unsafe(s, false, false))
         return 0;
 
     return cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr,
-- 
2.1.4

Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value

[Gerd Hoffmann]

  Hi,

> > > What seems to happen is that blit_is_unsafe() is also called for
> > > CIRRUS_BLTMODE_PATTERNCOPY, but in this case cirrus_blt_srcpitch is
> > > not used. However, because of this new check if its value is 0 then
> > > cirrus_bitblt_common_patterncopy() returns early and becomes a no-op.
> > 
> > inflight vga queue pull request has a fix for that.
> 
> Do you mean:
>  [PATCH] display: cirrus: ignore source pitch value as needed in blit_is_unsafe
>  (Message-Id: <20170109203520.5619-1-brogers@suse.com>)

Yes.

> Because I'm still seeing artifacts on some setups (eg. on win XP).
> As far as I can tell the check is still too strong:
> The rops used by cirrus_bitblt_common_patterncopy seem to only be using
> the destination pitch as far as I can see (all functions in
> cirrus_vga_rop2.h) and in my tests only the destination pitch got
> filled in, the source pitch was left as zero. Adapting the check when
> coming from cirrus_bitblt_common_patterncopy seems to fix the issue for
> me.

> From a3be50cc3e3bb0f5eb784d30048b88333366bdca Mon Sep 17 00:00:00 2001
> From: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Date: Fri, 20 Jan 2017 09:44:39 +0100
> Subject: [PATCH] cirrus: allow zero source pitch in pattern fill rops
> 
> The rops used by cirrus_bitblt_common_patterncopy only use
> the destination pitch, so the source pitch shoul allowed to
> be zero.
> 
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>

Looks good to me, can you 'git send-email' it to the list?

thanks,
  Gerd