2017-01-25 0:12 GMT+08:00 Laszlo Ersek <ler...@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)
> >
>
>
