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