From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>,
liqiang6-s@360.cn, ghoffman@redhat.com,
Li Qiang <liq3ea@gmail.com>,
qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue
Date: Tue, 24 Jan 2017 16:31:43 +0100 [thread overview]
Message-ID: <20170124153143.GA29823@olga.wb> (raw)
In-Reply-To: <1485260998.32716.45.camel@redhat.com>
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)
next prev parent reply other threads:[~2017-01-24 15:32 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-24 9:34 [Qemu-devel] [PATCH] cirrus: fix oob access issue Li Qiang
2017-01-24 9:50 ` no-reply
2017-01-24 10:23 ` Laszlo Ersek
2017-01-24 10:48 ` Gerd Hoffmann
2017-01-24 11:17 ` Laszlo Ersek
2017-01-24 11:24 ` Laszlo Ersek
2017-01-24 12:29 ` Gerd Hoffmann
2017-01-24 15:31 ` Wolfgang Bumiller [this message]
2017-01-24 16:12 ` Laszlo Ersek
2017-01-25 1:18 ` Li Qiang
2017-01-25 3:31 ` Laszlo Ersek
2017-01-25 7:26 ` Gerd Hoffmann
2017-01-25 7:18 ` Gerd Hoffmann
2017-01-25 10:13 ` Laszlo Ersek
2017-01-24 9:58 Li Qiang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170124153143.GA29823@olga.wb \
--to=w.bumiller@proxmox.com \
--cc=ghoffman@redhat.com \
--cc=kraxel@redhat.com \
--cc=lersek@redhat.com \
--cc=liq3ea@gmail.com \
--cc=liqiang6-s@360.cn \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.