[Qemu-devel] [PATCH] cirrus: fix oob access issue

[Qemu-devel] [PATCH] cirrus: fix oob access issue

[Li Qiang]

From: Li Qiang <liqiang6-s@360.cn>

When doing bitblt copy in backward mode, minus the blt width first
to avoid an oob access issue.

Signed-off-by: Li Qiang <liqiang6-s@360.cn>
---
 hw/display/cirrus_vga.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 379910d..7ddd289 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -277,7 +277,8 @@ static bool blit_region_is_unsafe(struct CirrusVGAState *s,
     }
     if (pitch < 0) {
         int64_t min = addr
-            + ((int64_t)s->cirrus_blt_height-1) * pitch;
+            + ((int64_t)s->cirrus_blt_height-1) * pitch
+            - s->cirrus_blt_width;
         int32_t max = addr
             + s->cirrus_blt_width;
         if (min < 0 || max > s->vga.vram_size) {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH] cirrus: fix oob access issue
Message-id: 58871f9b.d635240a.4cda6.5322@mx.google.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/58871f9b.d635240a.4cda6.5322@mx.google.com -> patchew/58871f9b.d635240a.4cda6.5322@mx.google.com
Switched to a new branch 'test'
080333c cirrus: fix oob access issue

=== OUTPUT BEGIN ===
Checking PATCH 1/1: cirrus: fix oob access issue...
ERROR: spaces required around that '-' (ctx:VxV)
#21: FILE: hw/display/cirrus_vga.c:280:
+            + ((int64_t)s->cirrus_blt_height-1) * pitch
                                             ^

total: 1 errors, 0 warnings, 9 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue

[Laszlo Ersek]

On 01/24/17 10:34, Li Qiang wrote:
> From: Li Qiang <liqiang6-s@360.cn>
> 
> When doing bitblt copy in backward mode, minus the blt width first
> to avoid an oob access issue.
> 
> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> ---
>  hw/display/cirrus_vga.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index 379910d..7ddd289 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -277,7 +277,8 @@ static bool blit_region_is_unsafe(struct CirrusVGAState *s,
>      }
>      if (pitch < 0) {
>          int64_t min = addr
> -            + ((int64_t)s->cirrus_blt_height-1) * pitch;
> +            + ((int64_t)s->cirrus_blt_height-1) * pitch
> +            - s->cirrus_blt_width;
>          int32_t max = addr
>              + s->cirrus_blt_width;
>          if (min < 0 || max > s->vga.vram_size) {
> 

I believe this is incorrect. In this case (AFAIR), "addr" points to the
left-most pixel (= lowest address) of the bottom line (= highest
address). This is why "max" is calculated the way it is -- to get the
max address, just move to the right side of the same bottom line.

Which then also means, in order to get the top left corner, you just
need to subtract an integral multiple of the stride (you are already on
the left side). Since the pitch is negative here, that means adding an
integral multiple of the pitch.

Finally, for a single-line blt, the bottom line is the only line, in
which case we pitch multiplier should be (1 - 1) == 0.

I think the code is correct as-is.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue

[Gerd Hoffmann]

> > diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> > index 379910d..7ddd289 100644
> > --- a/hw/display/cirrus_vga.c
> > +++ b/hw/display/cirrus_vga.c
> > @@ -277,7 +277,8 @@ static bool blit_region_is_unsafe(struct CirrusVGAState *s,
> >      }
> >      if (pitch < 0) {
> >          int64_t min = addr
> > -            + ((int64_t)s->cirrus_blt_height-1) * pitch;
> > +            + ((int64_t)s->cirrus_blt_height-1) * pitch
> > +            - s->cirrus_blt_width;
> >          int32_t max = addr
> >              + s->cirrus_blt_width;
> >          if (min < 0 || max > s->vga.vram_size) {
> > 
> 
> I believe this is incorrect. In this case (AFAIR), "addr" points to the
> left-most pixel (= lowest address) of the bottom line (= highest
> address).

If I read the code correctly it is backwards *both* x and y axis, so
addr is the right-most pixel of the bottom line.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue

[Laszlo Ersek]

On 01/24/17 11:48, Gerd Hoffmann wrote:
>>> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
>>> index 379910d..7ddd289 100644
>>> --- a/hw/display/cirrus_vga.c
>>> +++ b/hw/display/cirrus_vga.c
>>> @@ -277,7 +277,8 @@ static bool blit_region_is_unsafe(struct CirrusVGAState *s,
>>>      }
>>>      if (pitch < 0) {
>>>          int64_t min = addr
>>> -            + ((int64_t)s->cirrus_blt_height-1) * pitch;
>>> +            + ((int64_t)s->cirrus_blt_height-1) * pitch
>>> +            - s->cirrus_blt_width;
>>>          int32_t max = addr
>>>              + s->cirrus_blt_width;
>>>          if (min < 0 || max > s->vga.vram_size) {
>>>
>>
>> I believe this is incorrect. In this case (AFAIR), "addr" points to the
>> left-most pixel (= lowest address) of the bottom line (= highest
>> address).
> 
> If I read the code correctly it is backwards *both* x and y axis, so
> addr is the right-most pixel of the bottom line.

What is "max" then? If "addr" is the right-most pixel of the bottom
line, then "max" has the highest address just past the rectangle, and
then adding anything non-negative to it makes no sense.

Adding a negative value to it (i.e., if s->cirrus_blt_width were
negative) might make sense, for getting the leftmost pixel on the bottom
line, but then that "max" value shouldn't be used for the final boundary
check (against RAM size).

... Really as I remember it from the downstream review, the pitch is
negative (bottom-up), but the horizontal direction remains left to right.

0-----------------------------------------------------------------> x
|
|   addr + (height - 1) * pitch, pitch < 0, height > 0
|   |
|   v
|   *--------------------* <-- addr + (height - 1) * pitch + width
|   |                    |
|   |                    |
|   |                    |
|   |                    |
|   |                    |
|   |                    |
|   *--------------------*
|   ^                    ^
|   |                    |
|   addr                 max = addr + width, width > 0
|
v
y


The rightmost column represented here is exclusive, the rest is inclusive.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue

[Laszlo Ersek]

On 01/24/17 12:17, Laszlo Ersek wrote:
> On 01/24/17 11:48, Gerd Hoffmann wrote:
>>>> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
>>>> index 379910d..7ddd289 100644
>>>> --- a/hw/display/cirrus_vga.c
>>>> +++ b/hw/display/cirrus_vga.c
>>>> @@ -277,7 +277,8 @@ static bool blit_region_is_unsafe(struct CirrusVGAState *s,
>>>>      }
>>>>      if (pitch < 0) {
>>>>          int64_t min = addr
>>>> -            + ((int64_t)s->cirrus_blt_height-1) * pitch;
>>>> +            + ((int64_t)s->cirrus_blt_height-1) * pitch
>>>> +            - s->cirrus_blt_width;
>>>>          int32_t max = addr
>>>>              + s->cirrus_blt_width;
>>>>          if (min < 0 || max > s->vga.vram_size) {
>>>>
>>>
>>> I believe this is incorrect. In this case (AFAIR), "addr" points to the
>>> left-most pixel (= lowest address) of the bottom line (= highest
>>> address).
>>
>> If I read the code correctly it is backwards *both* x and y axis, so
>> addr is the right-most pixel of the bottom line.
> 
> What is "max" then? If "addr" is the right-most pixel of the bottom
> line, then "max" has the highest address just past the rectangle, and
> then adding anything non-negative to it makes no sense.

sorry, over-editing caused a bug. I meant

  If "addr" is the right-most pixel of the bottom line (or just one
  past that), then *it* has the highest address, and then adding
  anything non-negative to it (i.e., how we end up with "max") makes no
  sense.

I should have started with the diagram, not with the text.

Thanks
Laszlo

> 
> Adding a negative value to it (i.e., if s->cirrus_blt_width were
> negative) might make sense, for getting the leftmost pixel on the bottom
> line, but then that "max" value shouldn't be used for the final boundary
> check (against RAM size).
> 
> ... Really as I remember it from the downstream review, the pitch is
> negative (bottom-up), but the horizontal direction remains left to right.
> 
> 0-----------------------------------------------------------------> x
> |
> |   addr + (height - 1) * pitch, pitch < 0, height > 0
> |   |
> |   v
> |   *--------------------* <-- addr + (height - 1) * pitch + width
> |   |                    |
> |   |                    |
> |   |                    |
> |   |                    |
> |   |                    |
> |   |                    |
> |   *--------------------*
> |   ^                    ^
> |   |                    |
> |   addr                 max = addr + width, width > 0
> |
> v
> y
> 
> 
> The rightmost column represented here is exclusive, the rest is inclusive.
> 
> Thanks
> Laszlo
> 

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue

[Gerd Hoffmann]

> >>>      if (pitch < 0) {
> >>>          int64_t min = addr
> >>> -            + ((int64_t)s->cirrus_blt_height-1) * pitch;
> >>> +            + ((int64_t)s->cirrus_blt_height-1) * pitch
> >>> +            - s->cirrus_blt_width;
> >>>          int32_t max = addr
> >>>              + s->cirrus_blt_width;
> >>>          if (min < 0 || max > s->vga.vram_size) {
> >>>
> >>
> >> I believe this is incorrect. In this case (AFAIR), "addr" points to the
> >> left-most pixel (= lowest address) of the bottom line (= highest
> >> address).
> > 
> > If I read the code correctly it is backwards *both* x and y axis, so
> > addr is the right-most pixel of the bottom line.
> 
> What is "max" then? If "addr" is the right-most pixel of the bottom
> line, then "max" has the highest address just past the rectangle, and
> then adding anything non-negative to it makes no sense.

That is (with the patch applied) inconsistent indeed.  We must either
subtract s->cirrus_blt_width from min (addr == right-most), or add it to
max (addr == left-most), but certainly not both.

> ... Really as I remember it from the downstream review, the pitch is
> negative (bottom-up), but the horizontal direction remains left to right.

Looking at cirrus_vga_rop.h I see:
 - cirrus_bitblt_rop_fwd_*() increment src and dst while walking the
   scanline, and
 - cirrus_bitblt_rop_bkwd_*() decrement src and dst ...

I still think x axis goes backwards too and therefore addr is the
right-most pixel.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue

[Wolfgang Bumiller]

On Tue, Jan 24, 2017 at 01:29:58PM +0100, Gerd Hoffmann wrote:
> > >>>      if (pitch < 0) {
> > >>>          int64_t min = addr
> > >>> -            + ((int64_t)s->cirrus_blt_height-1) * pitch;
> > >>> +            + ((int64_t)s->cirrus_blt_height-1) * pitch
> > >>> +            - s->cirrus_blt_width;
> > >>>          int32_t max = addr
> > >>>              + s->cirrus_blt_width;
> > >>>          if (min < 0 || max > s->vga.vram_size) {
> > >>>
> > >>
> > >> I believe this is incorrect. In this case (AFAIR), "addr" points to the
> > >> left-most pixel (= lowest address) of the bottom line (= highest
> > >> address).
> > > 
> > > If I read the code correctly it is backwards *both* x and y axis, so
> > > addr is the right-most pixel of the bottom line.
> > 
> > What is "max" then? If "addr" is the right-most pixel of the bottom
> > line, then "max" has the highest address just past the rectangle, and
> > then adding anything non-negative to it makes no sense.
> 
> That is (with the patch applied) inconsistent indeed.  We must either
> subtract s->cirrus_blt_width from min (addr == right-most), or add it to
> max (addr == left-most), but certainly not both.
> 
> > ... Really as I remember it from the downstream review, the pitch is
> > negative (bottom-up), but the horizontal direction remains left to right.
> 
> Looking at cirrus_vga_rop.h I see:
>  - cirrus_bitblt_rop_fwd_*() increment src and dst while walking the
>    scanline, and
>  - cirrus_bitblt_rop_bkwd_*() decrement src and dst ...
> 
> I still think x axis goes backwards too and therefore addr is the
> right-most pixel.

I agree.

Seeing how I've already been reading through that code I thought I'd
go over it again and too would say both min and max need to be adapted:

     if (pitch < 0) {
         int64_t min = addr
             + ((int64_t)s->cirrus_blt_height-1) * pitch;
+            - s->cirrus_blt_width;
         int32_t max = addr
-            + s->cirrus_blt_width;
         if (min < 0 || max > s->vga.vram_size) {


Here's the rest of my analysis in case anyone's interested (mostly to
justify the first part of this mail):

-) Sign of the blit width/height & pitch values at the beginning:

    |    s->cirrus_blt_width = (s->vga.gr[0x20] | (s->vga.gr[0x21] << 8)) + 1;
    |    s->cirrus_blt_height = (s->vga.gr[0x22] | (s->vga.gr[0x23] << 8)) + 1;
    |    s->cirrus_blt_dstpitch = (s->vga.gr[0x24] | (s->vga.gr[0x25] << 8));
    |    s->cirrus_blt_srcpitch = (s->vga.gr[0x26] | (s->vga.gr[0x27] << 8));

   vga.gr is an uint8_t[]

==> all values are positive at this point

-) Backward blits invert the sign:

    |    if (s->cirrus_blt_mode & CIRRUS_BLTMODE_BACKWARDS) {
    |        s->cirrus_blt_dstpitch = -s->cirrus_blt_dstpitch;
    |        s->cirrus_blt_srcpitch = -s->cirrus_blt_srcpitch;


Starting with the simple one:

-) Forward blits from cirrus_vga_rop.h:
   Width (which is positive) is subtracted from the pitch (which
   is also positive), turning srcpitch and dstpitch into values
   representing the remaining bytes after the current row.
   The pattern below repeats for all functions:

      |static void
      |glue(cirrus_bitblt_rop_fwd_, ROP_NAME)(CirrusVGAState *s,
      |                             uint8_t *dst,const uint8_t *src,
      |                             int dstpitch,int srcpitch,
      |                             int bltwidth,int bltheight)
      |{
      |    int x,y;
      |    dstpitch -= bltwidth;
      |    srcpitch -= bltwidth;
      |
      |    if (bltheight > 1 && (dstpitch < 0 || srcpitch < 0)) {
      |        return;
      |    }
      |
      |    for (y = 0; y < bltheight; y++) {
      |        for (x = 0; x < bltwidth; x++) {
      |            ROP_OP(dst, *src);
      |            dst++;
      |            src++;
      |        }
      |        dst += dstpitch;
      |        src += srcpitch;
      |    }
      |}

   The first access to src/dst is unmodified, so the lowest accessed
   address is the initial address.

   Some functions iterate through pairs:
      |   for (x = 0; x < bltwidth; x+=2) {
      |      p1 = *dst;
      |      p2 = *(dst+1);
   Since the loop uses `x += 2` this `+1` should not go out of bounds
   provided the width is even (which if not the case should at least
   have an even pitch value).

Conclusion for forward blits:
   We use (start + pitch * (height-1) + width) which seems obvious for
   the main pattern but we also need to assert that the 2nd example
   above cannot access an additional byte with *(dst+1) after this
   value. This seems to be okay: for an odd width eg. 5, the highest
   x value the loop body is executed with will be 4, and we access
   4+1=5 at most.

-) Backward blits form cirrus_vga_rop.h:
   (Pitch is negative, width is positive.)
   They "add" the width to the pitch (essentially reducing its length),
   turning the 'dstpitch' and 'srcpitch' variables - as with forward
   blits - into values representing only the *remaining* bytes "in
   front of" the data in the current row.
   The pattern:

      |glue(cirrus_bitblt_rop_bkwd_, ROP_NAME)(CirrusVGAState *s,
      |                                        uint8_t *dst,const uint8_t *src,
      |                                        int dstpitch,int srcpitch,
      |                                        int bltwidth,int bltheight)
      |{
      |    int x,y;
      |    dstpitch += bltwidth;
      |    srcpitch += bltwidth;
      |    for (y = 0; y < bltheight; y++) {
      |        for (x = 0; x < bltwidth; x++) {
      |            ROP_OP(dst, *src);
      |            dst--;
      |            src--;
      |        }
      |        dst += dstpitch;
      |        src += srcpitch;
      |    }
      |}

   Pitch is negative, first value touched is 'dst' and 'src'
   unmodified. The same pattern is used throughout the rest of the
   header.

   As far as I can see the 'bkwd' ops only ever subtract from the
   addresses (note that in `X += Xpitch`, Xpitch is negative), and
   like with forward blits we have at most a 'src-1' (not +1 this
   time) access in a loop body where we iterate over pairs, so the
   same reasoning holds here.

Conclusion for backward blits:
   1) We should use: (addr + (height-1)*pitch - width) but originally
      missed the `-with` part there.

   2) The maximum value should actually be `addr` itself as far as I
      can tell, so the `+ s->cirrus_blt_width` for max should be
      dropped.


The functions in cirrus_vga_rop2.h are a bitmore complex. The safety
checks *may* even be too strict in some cases but I haven't seen any
aftefacts coming from too strong *range* checks (only the zero pitch
check for which I sent a patch yesterday which I need to make a v2 for
now :|).
(Too strict in the pattern functions which clamp the source offsets
with bitands, but I really don't feel like looking deep enough into
this to make the checks even lighter :p)

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue

[Laszlo Ersek]

On 01/24/17 16:31, Wolfgang Bumiller wrote:
> On Tue, Jan 24, 2017 at 01:29:58PM +0100, Gerd Hoffmann wrote:
>>>>>>      if (pitch < 0) {
>>>>>>          int64_t min = addr
>>>>>> -            + ((int64_t)s->cirrus_blt_height-1) * pitch;
>>>>>> +            + ((int64_t)s->cirrus_blt_height-1) * pitch
>>>>>> +            - s->cirrus_blt_width;
>>>>>>          int32_t max = addr
>>>>>>              + s->cirrus_blt_width;
>>>>>>          if (min < 0 || max > s->vga.vram_size) {
>>>>>>
>>>>>
>>>>> I believe this is incorrect. In this case (AFAIR), "addr" points to the
>>>>> left-most pixel (= lowest address) of the bottom line (= highest
>>>>> address).
>>>>
>>>> If I read the code correctly it is backwards *both* x and y axis, so
>>>> addr is the right-most pixel of the bottom line.
>>>
>>> What is "max" then? If "addr" is the right-most pixel of the bottom
>>> line, then "max" has the highest address just past the rectangle, and
>>> then adding anything non-negative to it makes no sense.
>>
>> That is (with the patch applied) inconsistent indeed.  We must either
>> subtract s->cirrus_blt_width from min (addr == right-most), or add it to
>> max (addr == left-most), but certainly not both.
>>
>>> ... Really as I remember it from the downstream review, the pitch is
>>> negative (bottom-up), but the horizontal direction remains left to right.
>>
>> Looking at cirrus_vga_rop.h I see:
>>  - cirrus_bitblt_rop_fwd_*() increment src and dst while walking the
>>    scanline, and
>>  - cirrus_bitblt_rop_bkwd_*() decrement src and dst ...
>>
>> I still think x axis goes backwards too and therefore addr is the
>> right-most pixel.
> 
> I agree.
> 
> Seeing how I've already been reading through that code I thought I'd
> go over it again and too would say both min and max need to be adapted:
> 
>      if (pitch < 0) {
>          int64_t min = addr
>              + ((int64_t)s->cirrus_blt_height-1) * pitch;
> +            - s->cirrus_blt_width;
>          int32_t max = addr
> -            + s->cirrus_blt_width;
>          if (min < 0 || max > s->vga.vram_size) {
> 
> 
> Here's the rest of my analysis in case anyone's interested (mostly to
> justify the first part of this mail):
> 
> -) Sign of the blit width/height & pitch values at the beginning:
> 
>     |    s->cirrus_blt_width = (s->vga.gr[0x20] | (s->vga.gr[0x21] << 8)) + 1;
>     |    s->cirrus_blt_height = (s->vga.gr[0x22] | (s->vga.gr[0x23] << 8)) + 1;
>     |    s->cirrus_blt_dstpitch = (s->vga.gr[0x24] | (s->vga.gr[0x25] << 8));
>     |    s->cirrus_blt_srcpitch = (s->vga.gr[0x26] | (s->vga.gr[0x27] << 8));
> 
>    vga.gr is an uint8_t[]
> 
> ==> all values are positive at this point
> 
> -) Backward blits invert the sign:
> 
>     |    if (s->cirrus_blt_mode & CIRRUS_BLTMODE_BACKWARDS) {
>     |        s->cirrus_blt_dstpitch = -s->cirrus_blt_dstpitch;
>     |        s->cirrus_blt_srcpitch = -s->cirrus_blt_srcpitch;
> 
> 
> Starting with the simple one:
> 
> -) Forward blits from cirrus_vga_rop.h:
>    Width (which is positive) is subtracted from the pitch (which
>    is also positive), turning srcpitch and dstpitch into values
>    representing the remaining bytes after the current row.
>    The pattern below repeats for all functions:
> 
>       |static void
>       |glue(cirrus_bitblt_rop_fwd_, ROP_NAME)(CirrusVGAState *s,
>       |                             uint8_t *dst,const uint8_t *src,
>       |                             int dstpitch,int srcpitch,
>       |                             int bltwidth,int bltheight)
>       |{
>       |    int x,y;
>       |    dstpitch -= bltwidth;
>       |    srcpitch -= bltwidth;
>       |
>       |    if (bltheight > 1 && (dstpitch < 0 || srcpitch < 0)) {
>       |        return;
>       |    }
>       |
>       |    for (y = 0; y < bltheight; y++) {
>       |        for (x = 0; x < bltwidth; x++) {
>       |            ROP_OP(dst, *src);
>       |            dst++;
>       |            src++;
>       |        }
>       |        dst += dstpitch;
>       |        src += srcpitch;
>       |    }
>       |}
> 
>    The first access to src/dst is unmodified, so the lowest accessed
>    address is the initial address.
> 
>    Some functions iterate through pairs:
>       |   for (x = 0; x < bltwidth; x+=2) {
>       |      p1 = *dst;
>       |      p2 = *(dst+1);
>    Since the loop uses `x += 2` this `+1` should not go out of bounds
>    provided the width is even (which if not the case should at least
>    have an even pitch value).
> 
> Conclusion for forward blits:
>    We use (start + pitch * (height-1) + width) which seems obvious for
>    the main pattern but we also need to assert that the 2nd example
>    above cannot access an additional byte with *(dst+1) after this
>    value. This seems to be okay: for an odd width eg. 5, the highest
>    x value the loop body is executed with will be 4, and we access
>    4+1=5 at most.
> 
> -) Backward blits form cirrus_vga_rop.h:
>    (Pitch is negative, width is positive.)
>    They "add" the width to the pitch (essentially reducing its length),
>    turning the 'dstpitch' and 'srcpitch' variables - as with forward
>    blits - into values representing only the *remaining* bytes "in
>    front of" the data in the current row.
>    The pattern:
> 
>       |glue(cirrus_bitblt_rop_bkwd_, ROP_NAME)(CirrusVGAState *s,
>       |                                        uint8_t *dst,const uint8_t *src,
>       |                                        int dstpitch,int srcpitch,
>       |                                        int bltwidth,int bltheight)
>       |{
>       |    int x,y;
>       |    dstpitch += bltwidth;
>       |    srcpitch += bltwidth;
>       |    for (y = 0; y < bltheight; y++) {
>       |        for (x = 0; x < bltwidth; x++) {
>       |            ROP_OP(dst, *src);
>       |            dst--;
>       |            src--;
>       |        }
>       |        dst += dstpitch;
>       |        src += srcpitch;
>       |    }
>       |}
> 
>    Pitch is negative, first value touched is 'dst' and 'src'
>    unmodified. The same pattern is used throughout the rest of the
>    header.
> 
>    As far as I can see the 'bkwd' ops only ever subtract from the
>    addresses (note that in `X += Xpitch`, Xpitch is negative), and
>    like with forward blits we have at most a 'src-1' (not +1 this
>    time) access in a loop body where we iterate over pairs, so the
>    same reasoning holds here.
> 
> Conclusion for backward blits:
>    1) We should use: (addr + (height-1)*pitch - width) but originally
>       missed the `-with` part there.
> 
>    2) The maximum value should actually be `addr` itself as far as I
>       can tell, so the `+ s->cirrus_blt_width` for max should be
>       dropped.

You (and Gerd) are correct.

I've just looked up my original downstream review of this patch
("cirrus: fix blit region check", now commit d3532a0db022 in upstream).
While at that time I range-checked everything and their grandma, and
found (with Gerd's feedback) those aspects safe, I only *assumed* that
for the negative pitch case, "addr" would point to the bottom left
corner of the rectangle:

On 01/15/15 12:21, Laszlo Ersek wrote:

> The negative pitch means (I think) that "addr" points to the lower
> left corner of the rectangle.
>
> The second part guarantees that the last blitted byte fits (lower
> right corner).

To which Gerd responded "upper left". In retrospect I don't understand
why we didn't discuss that question further, as it now seems that we
were both wrong -- "addr" stands for bottom right, in the negative pitch
case.

So, I agree that we should subtract width from both min and max here.

The current discussion has proved that the blit_region_is_unsafe()
function is incorrect ATM -- in a backwards blit, the guest can advance
to lower addresses than the "min" value we calculate and enforce to be
non-negative.

Unfortunately, the original patch was meant to address the
then-embargoed CVE-2014-8106. Since we have a bug in that code (= a
security fix), this issue should have been reported privately as well,
and another embargo would have been justified. We need to fix this ASAP,
and a new CVE should be requested; the cat is now out of the bag.

Thanks
Laszlo

> 
> 
> The functions in cirrus_vga_rop2.h are a bitmore complex. The safety
> checks *may* even be too strict in some cases but I haven't seen any
> aftefacts coming from too strong *range* checks (only the zero pitch
> check for which I sent a patch yesterday which I need to make a v2 for
> now :|).
> (Too strict in the pattern functions which clamp the source offsets
> with bitands, but I really don't feel like looking deep enough into
> this to make the checks even lighter :p)
> 

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue

[Li Qiang]

2017-01-25 0:12 GMT+08:00 Laszlo Ersek <lersek@redhat.com>:

> On 01/24/17 16:31, Wolfgang Bumiller wrote:
> > On Tue, Jan 24, 2017 at 01:29:58PM +0100, Gerd Hoffmann wrote:
> >>>>>>      if (pitch < 0) {
> >>>>>>          int64_t min = addr
> >>>>>> -            + ((int64_t)s->cirrus_blt_height-1) * pitch;
> >>>>>> +            + ((int64_t)s->cirrus_blt_height-1) * pitch
> >>>>>> +            - s->cirrus_blt_width;
> >>>>>>          int32_t max = addr
> >>>>>>              + s->cirrus_blt_width;
> >>>>>>          if (min < 0 || max > s->vga.vram_size) {
> >>>>>>
> >>>>>
> >>>>> I believe this is incorrect. In this case (AFAIR), "addr" points to
> the
> >>>>> left-most pixel (= lowest address) of the bottom line (= highest
> >>>>> address).
> >>>>
> >>>> If I read the code correctly it is backwards *both* x and y axis, so
> >>>> addr is the right-most pixel of the bottom line.
> >>>
> >>> What is "max" then? If "addr" is the right-most pixel of the bottom
> >>> line, then "max" has the highest address just past the rectangle, and
> >>> then adding anything non-negative to it makes no sense.
> >>
> >> That is (with the patch applied) inconsistent indeed.  We must either
> >> subtract s->cirrus_blt_width from min (addr == right-most), or add it to
> >> max (addr == left-most), but certainly not both.
> >>
> >>> ... Really as I remember it from the downstream review, the pitch is
> >>> negative (bottom-up), but the horizontal direction remains left to
> right.
> >>
> >> Looking at cirrus_vga_rop.h I see:
> >>  - cirrus_bitblt_rop_fwd_*() increment src and dst while walking the
> >>    scanline, and
> >>  - cirrus_bitblt_rop_bkwd_*() decrement src and dst ...
> >>
> >> I still think x axis goes backwards too and therefore addr is the
> >> right-most pixel.
> >
> > I agree.
> >
> > Seeing how I've already been reading through that code I thought I'd
> > go over it again and too would say both min and max need to be adapted:
> >
> >      if (pitch < 0) {
> >          int64_t min = addr
> >              + ((int64_t)s->cirrus_blt_height-1) * pitch;
> > +            - s->cirrus_blt_width;
> >          int32_t max = addr
> > -            + s->cirrus_blt_width;
> >          if (min < 0 || max > s->vga.vram_size) {
> >
> >
> > Here's the rest of my analysis in case anyone's interested (mostly to
> > justify the first part of this mail):
> >
> > -) Sign of the blit width/height & pitch values at the beginning:
> >
> >     |    s->cirrus_blt_width = (s->vga.gr[0x20] | (s->vga.gr[0x21] <<
> 8)) + 1;
> >     |    s->cirrus_blt_height = (s->vga.gr[0x22] | (s->vga.gr[0x23] <<
> 8)) + 1;
> >     |    s->cirrus_blt_dstpitch = (s->vga.gr[0x24] | (s->vga.gr[0x25]
> << 8));
> >     |    s->cirrus_blt_srcpitch = (s->vga.gr[0x26] | (s->vga.gr[0x27]
> << 8));
> >
> >    vga.gr is an uint8_t[]
> >
> > ==> all values are positive at this point
> >
> > -) Backward blits invert the sign:
> >
> >     |    if (s->cirrus_blt_mode & CIRRUS_BLTMODE_BACKWARDS) {
> >     |        s->cirrus_blt_dstpitch = -s->cirrus_blt_dstpitch;
> >     |        s->cirrus_blt_srcpitch = -s->cirrus_blt_srcpitch;
> >
> >
> > Starting with the simple one:
> >
> > -) Forward blits from cirrus_vga_rop.h:
> >    Width (which is positive) is subtracted from the pitch (which
> >    is also positive), turning srcpitch and dstpitch into values
> >    representing the remaining bytes after the current row.
> >    The pattern below repeats for all functions:
> >
> >       |static void
> >       |glue(cirrus_bitblt_rop_fwd_, ROP_NAME)(CirrusVGAState *s,
> >       |                             uint8_t *dst,const uint8_t *src,
> >       |                             int dstpitch,int srcpitch,
> >       |                             int bltwidth,int bltheight)
> >       |{
> >       |    int x,y;
> >       |    dstpitch -= bltwidth;
> >       |    srcpitch -= bltwidth;
> >       |
> >       |    if (bltheight > 1 && (dstpitch < 0 || srcpitch < 0)) {
> >       |        return;
> >       |    }
> >       |
> >       |    for (y = 0; y < bltheight; y++) {
> >       |        for (x = 0; x < bltwidth; x++) {
> >       |            ROP_OP(dst, *src);
> >       |            dst++;
> >       |            src++;
> >       |        }
> >       |        dst += dstpitch;
> >       |        src += srcpitch;
> >       |    }
> >       |}
> >
> >    The first access to src/dst is unmodified, so the lowest accessed
> >    address is the initial address.
> >
> >    Some functions iterate through pairs:
> >       |   for (x = 0; x < bltwidth; x+=2) {
> >       |      p1 = *dst;
> >       |      p2 = *(dst+1);
> >    Since the loop uses `x += 2` this `+1` should not go out of bounds
> >    provided the width is even (which if not the case should at least
> >    have an even pitch value).
> >
> > Conclusion for forward blits:
> >    We use (start + pitch * (height-1) + width) which seems obvious for
> >    the main pattern but we also need to assert that the 2nd example
> >    above cannot access an additional byte with *(dst+1) after this
> >    value. This seems to be okay: for an odd width eg. 5, the highest
> >    x value the loop body is executed with will be 4, and we access
> >    4+1=5 at most.
> >
> > -) Backward blits form cirrus_vga_rop.h:
> >    (Pitch is negative, width is positive.)
> >    They "add" the width to the pitch (essentially reducing its length),
> >    turning the 'dstpitch' and 'srcpitch' variables - as with forward
> >    blits - into values representing only the *remaining* bytes "in
> >    front of" the data in the current row.
> >    The pattern:
> >
> >       |glue(cirrus_bitblt_rop_bkwd_, ROP_NAME)(CirrusVGAState *s,
> >       |                                        uint8_t *dst,const
> uint8_t *src,
> >       |                                        int dstpitch,int srcpitch,
> >       |                                        int bltwidth,int
> bltheight)
> >       |{
> >       |    int x,y;
> >       |    dstpitch += bltwidth;
> >       |    srcpitch += bltwidth;
> >       |    for (y = 0; y < bltheight; y++) {
> >       |        for (x = 0; x < bltwidth; x++) {
> >       |            ROP_OP(dst, *src);
> >       |            dst--;
> >       |            src--;
> >       |        }
> >       |        dst += dstpitch;
> >       |        src += srcpitch;
> >       |    }
> >       |}
> >
> >    Pitch is negative, first value touched is 'dst' and 'src'
> >    unmodified. The same pattern is used throughout the rest of the
> >    header.
> >
> >    As far as I can see the 'bkwd' ops only ever subtract from the
> >    addresses (note that in `X += Xpitch`, Xpitch is negative), and
> >    like with forward blits we have at most a 'src-1' (not +1 this
> >    time) access in a loop body where we iterate over pairs, so the
> >    same reasoning holds here.
> >
> > Conclusion for backward blits:
> >    1) We should use: (addr + (height-1)*pitch - width) but originally
> >       missed the `-with` part there.
> >
> >    2) The maximum value should actually be `addr` itself as far as I
> >       can tell, so the `+ s->cirrus_blt_width` for max should be
> >       dropped.
>
> You (and Gerd) are correct.
>
> I've just looked up my original downstream review of this patch
> ("cirrus: fix blit region check", now commit d3532a0db022 in upstream).
> While at that time I range-checked everything and their grandma, and
> found (with Gerd's feedback) those aspects safe, I only *assumed* that
> for the negative pitch case, "addr" would point to the bottom left
> corner of the rectangle:
>
> On 01/15/15 12:21, Laszlo Ersek wrote:
>
> > The negative pitch means (I think) that "addr" points to the lower
> > left corner of the rectangle.
> >
> > The second part guarantees that the last blitted byte fits (lower
> > right corner).
>
> To which Gerd responded "upper left". In retrospect I don't understand
> why we didn't discuss that question further, as it now seems that we
> were both wrong -- "addr" stands for bottom right, in the negative pitch
> case.
>
> So, I agree that we should subtract width from both min and max here.
>
>
I have read all the discuss, very long and useful, but I think I still need
some
time to get a full understand. So I think one of you can provide the formal
patch to
describe the issue in more detail.



> The current discussion has proved that the blit_region_is_unsafe()
> function is incorrect ATM -- in a backwards blit, the guest can advance
> to lower addresses than the "min" value we calculate and enforce to be
> non-negative.
>
> Unfortunately, the original patch was meant to address the
> then-embargoed CVE-2014-8106. Since we have a bug in that code (= a
> security fix), this issue should have been reported privately as well,
> and another embargo would have been justified. We need to fix this ASAP,
> and a new CVE should be requested; the cat is now out of the bag.
>
>
Actually, I have reported this issue several months ago to redhat. But it
seems
this is not get the focus, so I tried myself to address it. As I'm not a
vga specialist I think
this issue is a little difficult.

Thanks!

Li Qiang / Cloud Security Team, Qihoo 360 Inc


> Thanks
> Laszlo
>
> >
> >
> > The functions in cirrus_vga_rop2.h are a bitmore complex. The safety
> > checks *may* even be too strict in some cases but I haven't seen any
> > aftefacts coming from too strong *range* checks (only the zero pitch
> > check for which I sent a patch yesterday which I need to make a v2 for
> > now :|).
> > (Too strict in the pattern functions which clamp the source offsets
> > with bitands, but I really don't feel like looking deep enough into
> > this to make the checks even lighter :p)
> >
>
>

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue

[Laszlo Ersek]

On 01/25/17 02:18, Li Qiang wrote:
> 
> 
> 2017-01-25 0:12 GMT+08:00 Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com>>:
> 
>     On 01/24/17 16:31, Wolfgang Bumiller wrote:
>     > On Tue, Jan 24, 2017 at 01:29:58PM +0100, Gerd Hoffmann wrote:
>     >>>>>>      if (pitch < 0) {
>     >>>>>>          int64_t min = addr
>     >>>>>> -            + ((int64_t)s->cirrus_blt_height-1) * pitch;
>     >>>>>> +            + ((int64_t)s->cirrus_blt_height-1) * pitch
>     >>>>>> +            - s->cirrus_blt_width;
>     >>>>>>          int32_t max = addr
>     >>>>>>              + s->cirrus_blt_width;
>     >>>>>>          if (min < 0 || max > s->vga.vram_size) {
>     >>>>>>
>     >>>>>
>     >>>>> I believe this is incorrect. In this case (AFAIR), "addr"
>     points to the
>     >>>>> left-most pixel (= lowest address) of the bottom line (= highest
>     >>>>> address).
>     >>>>
>     >>>> If I read the code correctly it is backwards *both* x and y
>     axis, so
>     >>>> addr is the right-most pixel of the bottom line.
>     >>>
>     >>> What is "max" then? If "addr" is the right-most pixel of the bottom
>     >>> line, then "max" has the highest address just past the
>     rectangle, and
>     >>> then adding anything non-negative to it makes no sense.
>     >>
>     >> That is (with the patch applied) inconsistent indeed.  We must either
>     >> subtract s->cirrus_blt_width from min (addr == right-most), or
>     add it to
>     >> max (addr == left-most), but certainly not both.
>     >>
>     >>> ... Really as I remember it from the downstream review, the pitch is
>     >>> negative (bottom-up), but the horizontal direction remains left
>     to right.
>     >>
>     >> Looking at cirrus_vga_rop.h I see:
>     >>  - cirrus_bitblt_rop_fwd_*() increment src and dst while walking the
>     >>    scanline, and
>     >>  - cirrus_bitblt_rop_bkwd_*() decrement src and dst ...
>     >>
>     >> I still think x axis goes backwards too and therefore addr is the
>     >> right-most pixel.
>     >
>     > I agree.
>     >
>     > Seeing how I've already been reading through that code I thought I'd
>     > go over it again and too would say both min and max need to be
>     adapted:
>     >
>     >      if (pitch < 0) {
>     >          int64_t min = addr
>     >              + ((int64_t)s->cirrus_blt_height-1) * pitch;
>     > +            - s->cirrus_blt_width;
>     >          int32_t max = addr
>     > -            + s->cirrus_blt_width;
>     >          if (min < 0 || max > s->vga.vram_size) {
>     >
>     >
>     > Here's the rest of my analysis in case anyone's interested (mostly to
>     > justify the first part of this mail):
>     >
>     > -) Sign of the blit width/height & pitch values at the beginning:
>     >
>     >     |    s->cirrus_blt_width = (s->vga.gr <http://vga.gr>[0x20] |
>     (s->vga.gr <http://vga.gr>[0x21] << 8)) + 1;
>     >     |    s->cirrus_blt_height = (s->vga.gr <http://vga.gr>[0x22] |
>     (s->vga.gr <http://vga.gr>[0x23] << 8)) + 1;
>     >     |    s->cirrus_blt_dstpitch = (s->vga.gr <http://vga.gr>[0x24]
>     | (s->vga.gr <http://vga.gr>[0x25] << 8));
>     >     |    s->cirrus_blt_srcpitch = (s->vga.gr <http://vga.gr>[0x26]
>     | (s->vga.gr <http://vga.gr>[0x27] << 8));
>     >
>     >    vga.gr <http://vga.gr> is an uint8_t[]
>     >
>     > ==> all values are positive at this point
>     >
>     > -) Backward blits invert the sign:
>     >
>     >     |    if (s->cirrus_blt_mode & CIRRUS_BLTMODE_BACKWARDS) {
>     >     |        s->cirrus_blt_dstpitch = -s->cirrus_blt_dstpitch;
>     >     |        s->cirrus_blt_srcpitch = -s->cirrus_blt_srcpitch;
>     >
>     >
>     > Starting with the simple one:
>     >
>     > -) Forward blits from cirrus_vga_rop.h:
>     >    Width (which is positive) is subtracted from the pitch (which
>     >    is also positive), turning srcpitch and dstpitch into values
>     >    representing the remaining bytes after the current row.
>     >    The pattern below repeats for all functions:
>     >
>     >       |static void
>     >       |glue(cirrus_bitblt_rop_fwd_, ROP_NAME)(CirrusVGAState *s,
>     >       |                             uint8_t *dst,const uint8_t *src,
>     >       |                             int dstpitch,int srcpitch,
>     >       |                             int bltwidth,int bltheight)
>     >       |{
>     >       |    int x,y;
>     >       |    dstpitch -= bltwidth;
>     >       |    srcpitch -= bltwidth;
>     >       |
>     >       |    if (bltheight > 1 && (dstpitch < 0 || srcpitch < 0)) {
>     >       |        return;
>     >       |    }
>     >       |
>     >       |    for (y = 0; y < bltheight; y++) {
>     >       |        for (x = 0; x < bltwidth; x++) {
>     >       |            ROP_OP(dst, *src);
>     >       |            dst++;
>     >       |            src++;
>     >       |        }
>     >       |        dst += dstpitch;
>     >       |        src += srcpitch;
>     >       |    }
>     >       |}
>     >
>     >    The first access to src/dst is unmodified, so the lowest accessed
>     >    address is the initial address.
>     >
>     >    Some functions iterate through pairs:
>     >       |   for (x = 0; x < bltwidth; x+=2) {
>     >       |      p1 = *dst;
>     >       |      p2 = *(dst+1);
>     >    Since the loop uses `x += 2` this `+1` should not go out of bounds
>     >    provided the width is even (which if not the case should at least
>     >    have an even pitch value).
>     >
>     > Conclusion for forward blits:
>     >    We use (start + pitch * (height-1) + width) which seems obvious for
>     >    the main pattern but we also need to assert that the 2nd example
>     >    above cannot access an additional byte with *(dst+1) after this
>     >    value. This seems to be okay: for an odd width eg. 5, the highest
>     >    x value the loop body is executed with will be 4, and we access
>     >    4+1=5 at most.
>     >
>     > -) Backward blits form cirrus_vga_rop.h:
>     >    (Pitch is negative, width is positive.)
>     >    They "add" the width to the pitch (essentially reducing its
>     length),
>     >    turning the 'dstpitch' and 'srcpitch' variables - as with forward
>     >    blits - into values representing only the *remaining* bytes "in
>     >    front of" the data in the current row.
>     >    The pattern:
>     >
>     >       |glue(cirrus_bitblt_rop_bkwd_, ROP_NAME)(CirrusVGAState *s,
>     >       |                                        uint8_t *dst,const
>     uint8_t *src,
>     >       |                                        int dstpitch,int
>     srcpitch,
>     >       |                                        int bltwidth,int
>     bltheight)
>     >       |{
>     >       |    int x,y;
>     >       |    dstpitch += bltwidth;
>     >       |    srcpitch += bltwidth;
>     >       |    for (y = 0; y < bltheight; y++) {
>     >       |        for (x = 0; x < bltwidth; x++) {
>     >       |            ROP_OP(dst, *src);
>     >       |            dst--;
>     >       |            src--;
>     >       |        }
>     >       |        dst += dstpitch;
>     >       |        src += srcpitch;
>     >       |    }
>     >       |}
>     >
>     >    Pitch is negative, first value touched is 'dst' and 'src'
>     >    unmodified. The same pattern is used throughout the rest of the
>     >    header.
>     >
>     >    As far as I can see the 'bkwd' ops only ever subtract from the
>     >    addresses (note that in `X += Xpitch`, Xpitch is negative), and
>     >    like with forward blits we have at most a 'src-1' (not +1 this
>     >    time) access in a loop body where we iterate over pairs, so the
>     >    same reasoning holds here.
>     >
>     > Conclusion for backward blits:
>     >    1) We should use: (addr + (height-1)*pitch - width) but originally
>     >       missed the `-with` part there.
>     >
>     >    2) The maximum value should actually be `addr` itself as far as I
>     >       can tell, so the `+ s->cirrus_blt_width` for max should be
>     >       dropped.
> 
>     You (and Gerd) are correct.
> 
>     I've just looked up my original downstream review of this patch
>     ("cirrus: fix blit region check", now commit d3532a0db022 in upstream).
>     While at that time I range-checked everything and their grandma, and
>     found (with Gerd's feedback) those aspects safe, I only *assumed* that
>     for the negative pitch case, "addr" would point to the bottom left
>     corner of the rectangle:
> 
>     On 01/15/15 12:21, Laszlo Ersek wrote:
> 
>     > The negative pitch means (I think) that "addr" points to the lower
>     > left corner of the rectangle.
>     >
>     > The second part guarantees that the last blitted byte fits (lower
>     > right corner).
> 
>     To which Gerd responded "upper left". In retrospect I don't understand
>     why we didn't discuss that question further, as it now seems that we
>     were both wrong -- "addr" stands for bottom right, in the negative pitch
>     case.
> 
>     So, I agree that we should subtract width from both min and max here.
> 
> 
> I have read all the discuss, very long and useful, but I think I still
> need some
> time to get a full understand. So I think one of you can provide the
> formal patch to 
> describe the issue in more detail.

Your patch is almost correct.

The blit_region_is_unsafe() function determines, in advance, the address
range that a blit operation intends to access (read or overwrite), as
configured by the guest. If the address range is not a subset of the VGA
RAM (i.e., the guest tries to access QEMU memory before the VGA RAM, or
after it), then the operation is rejected.

There are forward and backward advancing variants of the blit
operations. They translate to positive and negative pitches,
respectively. On the negative pitch branch, the address range that
blit_region_is_unsafe() calculates and verifies against the VGA RAM is
incorrect; it does not actually match the address range that the actual,
later execution of the blit operation will scan over. Therefore it is
possible for the guest to construct a blit request that passes the
(incorrect) check, but then the actual address range scan gets outside
of the restricted area.

The issue is that the starting address for the backwards advancing blit
ops, as interpreted by the actual address range scan, is the bottom
right corner of the rectangle. But the range check in
blit_region_is_unsafe() considers the starting address to identify the
bottom left corner of the rectangle. Your change corrects the
interpretation of "addr" in blit_region_is_unsafe() for "min", which
stands for "top left", but in parallel, the "max" value has to be
corrected too; otherwise "max" will be too large (it will point past the
bottom right corner of the rectangle).

> 
>  
> 
>     The current discussion has proved that the blit_region_is_unsafe()
>     function is incorrect ATM -- in a backwards blit, the guest can advance
>     to lower addresses than the "min" value we calculate and enforce to be
>     non-negative.
> 
>     Unfortunately, the original patch was meant to address the
>     then-embargoed CVE-2014-8106. Since we have a bug in that code (= a
>     security fix), this issue should have been reported privately as well,
>     and another embargo would have been justified. We need to fix this ASAP,
>     and a new CVE should be requested; the cat is now out of the bag.
> 
> 
> Actually, I have reported this issue several months ago to redhat. But
> it seems
> this is not get the focus,

That's a cold shower. :(

> so I tried myself to address it. As I'm not a
> vga specialist I think
> this issue is a little difficult.

It is.

Gerd, do you think you can raise this with the SRT? (I mean, if you
agree that this is a security issue.)

Thanks
Laszlo


> 
> Thanks!
> 
> Li Qiang / Cloud Security Team, Qihoo 360 Inc
>  
> 
>     Thanks
>     Laszlo
> 
>     >
>     >
>     > The functions in cirrus_vga_rop2.h are a bitmore complex. The safety
>     > checks *may* even be too strict in some cases but I haven't seen any
>     > aftefacts coming from too strong *range* checks (only the zero pitch
>     > check for which I sent a patch yesterday which I need to make a v2 for
>     > now :|).
>     > (Too strict in the pattern functions which clamp the source offsets
>     > with bitands, but I really don't feel like looking deep enough into
>     > this to make the checks even lighter :p)
>     >
> 
> 

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue

[Gerd Hoffmann]

  Hi,

> > I have read all the discuss, very long and useful, but I think I still
> > need some
> > time to get a full understand. So I think one of you can provide the
> > formal patch to 
> > describe the issue in more detail.
> 
> Your patch is almost correct.

New version out for review.

> Gerd, do you think you can raise this with the SRT? (I mean, if you
> agree that this is a security issue.)

Yes, it is.  Getting a CVE number for it is wip.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue

[Gerd Hoffmann]

  Hi,

> > The negative pitch means (I think) that "addr" points to the lower
> > left corner of the rectangle.
> >
> > The second part guarantees that the last blitted byte fits (lower
> > right corner).
> 
> To which Gerd responded "upper left". In retrospect I don't understand
> why we didn't discuss that question further, as it now seems that we
> were both wrong -- "addr" stands for bottom right, in the negative pitch
> case.

/me looks at d3532a0db02296e687711b8cdc7791924efccea0 and I can't
remember I wrote that code :-o

And I can't remember the discussion either.

The good thing is I probably looked more careful at the code because of
that ...

> Unfortunately, the original patch was meant to address the
> then-embargoed CVE-2014-8106. Since we have a bug in that code (= a
> security fix), this issue should have been reported privately as well,

It has been reported privately first.  I've actually suggested to send
it to the public list without embargo, given that we are moving away
from cirrus so this is less critical than it used to be two years ago.
Cirrus isn't the default display adapter any more in qemu, since years,
and management apps (virt-manager, ovirt, ...) are following.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue

[Laszlo Ersek]

On 01/25/17 08:18, Gerd Hoffmann wrote:
>   Hi,
> 
>>> The negative pitch means (I think) that "addr" points to the lower
>>> left corner of the rectangle.
>>>
>>> The second part guarantees that the last blitted byte fits (lower
>>> right corner).
>>
>> To which Gerd responded "upper left". In retrospect I don't understand
>> why we didn't discuss that question further, as it now seems that we
>> were both wrong -- "addr" stands for bottom right, in the negative pitch
>> case.
> 
> /me looks at d3532a0db02296e687711b8cdc7791924efccea0 and I can't
> remember I wrote that code :-o

Haha, happens to me too :)

> And I can't remember the discussion either.
> 
> The good thing is I probably looked more careful at the code because of
> that ...
> 
>> Unfortunately, the original patch was meant to address the
>> then-embargoed CVE-2014-8106. Since we have a bug in that code (= a
>> security fix), this issue should have been reported privately as well,
> 
> It has been reported privately first.  I've actually suggested to send
> it to the public list without embargo, given that we are moving away
> from cirrus so this is less critical than it used to be two years ago.
> Cirrus isn't the default display adapter any more in qemu, since years,
> and management apps (virt-manager, ovirt, ...) are following.

Ah, I see -- a CVE is justified, but an embargo: likely not. Makes sense.

Thanks!
Laszlo

[Qemu-devel] [PATCH] cirrus: fix oob access issue

[Li Qiang]

From: Li Qiang <liqiang6-s@360.cn>

When doing bitblt copy in backward mode, we should minus the
blt width first just like the adding in the forward mode. This
can avoid the oob access of the front of vga's vram.

Signed-off-by: Li Qiang <liqiang6-s@360.cn>
---
 hw/display/cirrus_vga.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 379910d..fa56730 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -277,7 +277,8 @@ static bool blit_region_is_unsafe(struct CirrusVGAState *s,
     }
     if (pitch < 0) {
         int64_t min = addr
-            + ((int64_t)s->cirrus_blt_height-1) * pitch;
+            + ((int64_t)s->cirrus_blt_height - 1) * pitch
+            - s->cirrus_blt_width;
         int32_t max = addr
             + s->cirrus_blt_width;
         if (min < 0 || max > s->vga.vram_size) {
-- 
1.8.3.1