* [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value @ 2016-10-18 7:45 P J P 2016-11-15 16:43 ` P J P 2016-11-16 14:13 ` Marc-André Lureau 0 siblings, 2 replies; 9+ messages in thread From: P J P @ 2016-10-18 7:45 UTC (permalink / raw) To: Qemu Developers; +Cc: Peter Maydell, Huawei PSIRT, Prasad J Pandit 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value 2016-10-18 7:45 [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value P J P @ 2016-11-15 16:43 ` P J P 2016-11-16 14:13 ` Marc-André Lureau 1 sibling, 0 replies; 9+ messages in thread From: P J P @ 2016-11-15 16:43 UTC (permalink / raw) To: Qemu Developers; +Cc: Peter Maydell, Huawei PSIRT +-- 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value 2016-10-18 7:45 [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value P J P 2016-11-15 16:43 ` P J P @ 2016-11-16 14:13 ` Marc-André Lureau 2016-11-28 6:22 ` P J P 1 sibling, 1 reply; 9+ messages in thread From: Marc-André Lureau @ 2016-11-16 14:13 UTC (permalink / raw) To: P J P, Qemu Developers; +Cc: Peter Maydell, Huawei PSIRT, Prasad J Pandit 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value 2016-11-16 14:13 ` Marc-André Lureau @ 2016-11-28 6:22 ` P J P 2016-12-05 7:33 ` P J P 2017-01-11 14:59 ` Alberto Garcia 0 siblings, 2 replies; 9+ messages in thread From: P J P @ 2016-11-28 6:22 UTC (permalink / raw) To: Marc-André Lureau; +Cc: Qemu Developers, Peter Maydell, Huawei PSIRT 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value 2016-11-28 6:22 ` P J P @ 2016-12-05 7:33 ` P J P 2017-01-11 14:59 ` Alberto Garcia 1 sibling, 0 replies; 9+ messages in thread From: P J P @ 2016-12-05 7:33 UTC (permalink / raw) To: Qemu Developers; +Cc: Marc-André Lureau, Peter Maydell, Huawei PSIRT +-- 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value 2016-11-28 6:22 ` P J P 2016-12-05 7:33 ` P J P @ 2017-01-11 14:59 ` Alberto Garcia 2017-01-11 20:43 ` Gerd Hoffmann 1 sibling, 1 reply; 9+ messages in thread From: Alberto Garcia @ 2017-01-11 14:59 UTC (permalink / raw) To: P J P Cc: Marc-André Lureau, Qemu Developers, Peter Maydell, Huawei PSIRT, Gerd Hoffmann 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value 2017-01-11 14:59 ` Alberto Garcia @ 2017-01-11 20:43 ` Gerd Hoffmann 2017-01-20 9:46 ` Wolfgang Bumiller 0 siblings, 1 reply; 9+ messages in thread From: Gerd Hoffmann @ 2017-01-11 20:43 UTC (permalink / raw) To: Alberto Garcia Cc: P J P, Marc-André Lureau, Qemu Developers, Peter Maydell, Huawei PSIRT 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value 2017-01-11 20:43 ` Gerd Hoffmann @ 2017-01-20 9:46 ` Wolfgang Bumiller 2017-01-23 11:56 ` Gerd Hoffmann 0 siblings, 1 reply; 9+ messages in thread From: Wolfgang Bumiller @ 2017-01-20 9:46 UTC (permalink / raw) To: Gerd Hoffmann Cc: Alberto Garcia, Huawei PSIRT, Peter Maydell, Marc-André Lureau, Qemu Developers, P J P 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value 2017-01-20 9:46 ` Wolfgang Bumiller @ 2017-01-23 11:56 ` Gerd Hoffmann 0 siblings, 0 replies; 9+ messages in thread From: Gerd Hoffmann @ 2017-01-23 11:56 UTC (permalink / raw) To: Wolfgang Bumiller Cc: Alberto Garcia, Huawei PSIRT, Peter Maydell, Marc-André Lureau, Qemu Developers, P J P 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-01-23 11:57 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-10-18 7:45 [Qemu-devel] [PATCH] display: cirrus: check vga bits per pixel(bpp) value P J P 2016-11-15 16:43 ` P J P 2016-11-16 14:13 ` Marc-André Lureau 2016-11-28 6:22 ` P J P 2016-12-05 7:33 ` P J P 2017-01-11 14:59 ` Alberto Garcia 2017-01-11 20:43 ` Gerd Hoffmann 2017-01-20 9:46 ` Wolfgang Bumiller 2017-01-23 11:56 ` Gerd Hoffmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).