[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue |
Date: |
Tue, 24 Jan 2017 17:12:57 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 |
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)
>
- [Qemu-devel] [PATCH] cirrus: fix oob access issue, Li Qiang, 2017/01/24
- Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue, no-reply, 2017/01/24
- Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue, Laszlo Ersek, 2017/01/24
- Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue, Gerd Hoffmann, 2017/01/24
- Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue, Laszlo Ersek, 2017/01/24
- Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue, Laszlo Ersek, 2017/01/24
- Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue, Gerd Hoffmann, 2017/01/24
- Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue, Wolfgang Bumiller, 2017/01/24
- Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue,
Laszlo Ersek <=
- Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue, Li Qiang, 2017/01/24
- Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue, Laszlo Ersek, 2017/01/24
- Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue, Gerd Hoffmann, 2017/01/25
- Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue, Gerd Hoffmann, 2017/01/25
- Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue, Laszlo Ersek, 2017/01/25