diff mbox

display: cirrus: check vga bits per pixel(bpp) value

Message ID 1476776717-24807-1-git-send-email-ppandit@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Prasad Pandit Oct. 18, 2016, 7:45 a.m. UTC 
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(-)

Comments

Prasad Pandit Nov. 15, 2016, 4:43 p.m. UTC | #1 
+-- 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
Marc-André Lureau Nov. 16, 2016, 2:13 p.m. UTC | #2 
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
Prasad Pandit Nov. 28, 2016, 6:22 a.m. UTC | #3 
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
Prasad Pandit Dec. 5, 2016, 7:33 a.m. UTC | #4 
+-- 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
Alberto Garcia Jan. 11, 2017, 2:59 p.m. UTC | #5 
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
Gerd Hoffmann Jan. 11, 2017, 8:43 p.m. UTC | #6 
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
diff mbox

Patch

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;
 }
 
 /***************************************