qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Li Qiang
Subject: Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue
Date: Wed, 25 Jan 2017 09:18:35 +0800

2017-01-25 0:12 GMT+08:00 Laszlo Ersek <address@hidden>:

> 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)
> >
>
>


reply via email to

[Prev in Thread] Current Thread [Next in Thread]