[Qemu-devel] [PATCH] fix :cirrus_vga fix OOB read case qemu Segmentation fault

[Qemu-devel] [PATCH] fix :cirrus_vga fix OOB read case qemu Segmentation fault

[hangaohuai]

check the validity of parameters in cirrus_bitblt_rop_fwd_transp_xxx
and cirrus_bitblt_rop_fwd_xxx to avoid the OOB read which causes qemu Segmentation fault.

After the fix, we will touch the assert in
cirrus_invalidate_region:
assert(off_cur_end >= off_cur);

Signed-off-by: fangying <fangying1@huawei.com>
Signed-off-by: hangaohuai <hangaohuai@huawei.com>
---
 hw/display/cirrus_vga_rop.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/display/cirrus_vga_rop.h b/hw/display/cirrus_vga_rop.h
index 0925a00..12a15e0 100644
--- a/hw/display/cirrus_vga_rop.h
+++ b/hw/display/cirrus_vga_rop.h
@@ -97,6 +97,11 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_8)(CirrusVGAState *s,
     uint8_t p;
     dstpitch -= bltwidth;
     srcpitch -= bltwidth;
+
+    if (dstpitch < 0 || srcpitch < 0) {
+        return;
+    }
+
     for (y = 0; y < bltheight; y++) {
         for (x = 0; x < bltwidth; x++) {
 	    p = *dst;
@@ -143,6 +148,11 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_16)(CirrusVGAState *s,
     uint8_t p1, p2;
     dstpitch -= bltwidth;
     srcpitch -= bltwidth;
+
+    if (srcpitch < 0 || dstpitch < 0) {
+        return;
+    }
+
     for (y = 0; y < bltheight; y++) {
         for (x = 0; x < bltwidth; x+=2) {
 	    p1 = *dst;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] fix :cirrus_vga fix OOB read case qemu Segmentation fault

[Gerd Hoffmann]

> @@ -97,6 +97,11 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_, ROP_NAME),_8)(CirrusVGAState *s,
>      uint8_t p;
>      dstpitch -= bltwidth;
>      srcpitch -= bltwidth;
> +
> +    if (dstpitch < 0 || srcpitch < 0) {
> +        return;
> +    }

Shouldn't that be ...

    if (bltheight > 1 && (dstpitch < 0 || srcpitch < 0)) {


... matching the check of the non-transparent version a few lines up in
the same source file?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] fix :cirrus_vga fix OOB read case qemu Segmentation fault

[Gonglei (Arei)]

Hi Gerd,

Thanks for rapid reply :)


> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Monday, March 13, 2017 9:55 PM
> To: Hangaohuai
> Cc: qemu-devel@nongnu.org; fangying; Gonglei (Arei)
> Subject: Re: [Qemu-devel] [PATCH] fix :cirrus_vga fix OOB read case qemu
> Segmentation fault
> 
> > @@ -97,6 +97,11 @@ glue(glue(cirrus_bitblt_rop_fwd_transp_,
> ROP_NAME),_8)(CirrusVGAState *s,
> >      uint8_t p;
> >      dstpitch -= bltwidth;
> >      srcpitch -= bltwidth;
> > +
> > +    if (dstpitch < 0 || srcpitch < 0) {
> > +        return;
> > +    }
> 
> Shouldn't that be ...
> 
>     if (bltheight > 1 && (dstpitch < 0 || srcpitch < 0)) {
> 
> 
> ... matching the check of the non-transparent version a few lines up in
> the same source file?
> 
Maybe yes, we check this patch after getting your clue:

commit d16136d22af0fcf0d651de04c9e3cbc7137cc6f9
Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Mon Jul 7 10:32:34 2014 +1000

cirrus: Fix host CPU blits

Commit b2eb849d4b1fdb6f35d5c46958c7f703cf64cfef
"CVE-2007-1320 - Cirrus LGD-54XX "bitblt" heap overflow" broke
cpu to video blits.

When the ROP function is called from cirrus_bitblt_cputovideo_next(),
we pass 0 for the pitch but only operate on one line at a time. The
added test was tripping because after the initial substraction, the
pitch becomes negative. Make the test only trip when the height is
larger than one (ie. the pitch is actually used).

This fixes HW cursor support in Windows NT4.0 (which otherwise was
a white rectangle) and general display of icons in that OS when using
8bpp mode.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

diff --git a/hw/display/cirrus_vga_rop.h b/hw/display/cirrus_vga_rop.h
index 9c7bb09..0925a00 100644
--- a/hw/display/cirrus_vga_rop.h
+++ b/hw/display/cirrus_vga_rop.h
@@ -52,8 +52,7 @@ glue(cirrus_bitblt_rop_fwd_, ROP_NAME)(CirrusVGAState *s,
dstpitch -= bltwidth;
srcpitch -= bltwidth;

- if (dstpitch < 0 || srcpitch < 0) {
- /* is 0 valid? srcpitch == 0 could be useful */
+ if (bltheight > 1 && (dstpitch < 0 || srcpitch < 0)) {
return;
}

So does v2 is needed?

Thanks,
-Gonglei

Re: [Qemu-devel] [PATCH] fix :cirrus_vga fix OOB read case qemu Segmentation fault

[Gerd Hoffmann]

  Hi,

> commit d16136d22af0fcf0d651de04c9e3cbc7137cc6f9
> Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date: Mon Jul 7 10:32:34 2014 +1000
> 
> cirrus: Fix host CPU blits

> So does v2 is needed?

Yes, otherwise we will end up with a regression
similar to the one fixed by ben.

cheers,
  Gerd