[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue (CVE-2017-TODO)
From: |
Gerd Hoffmann |
Subject: |
Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue (CVE-2017-TODO) |
Date: |
Wed, 25 Jan 2017 10:50:15 +0100 |
On Mi, 2017-01-25 at 09:30 +0100, Wolfgang Bumiller wrote:
> On Wed, Jan 25, 2017 at 08:07:05AM +0100, Gerd Hoffmann wrote:
> > From: Li Qiang <address@hidden>
> >
> > When doing bitblt copy in backward mode, we should minus the
> > blt width first just like the adding in the forward mode. This
> > can avoid the oob access of the front of vga's vram.
> >
> > Signed-off-by: Li Qiang <address@hidden>
> > Message-id: address@hidden
> >
> > { kraxel: with backward blits (negative pitch) addr is the topmost
> > address, so check it as-is against vram size ]
> >
> > Cc: address@hidden
> > Cc: P J P <address@hidden>
> > Cc: Laszlo Ersek <address@hidden>
> > Cc: Paolo Bonzini <address@hidden>
> > Cc: Wolfgang Bumiller <address@hidden>
> > Fixes: d3532a0db02296e687711b8cdc7791924efccea0 (CVE-2014-8106)
> > Signed-off-by: Gerd Hoffmann <address@hidden>
> > ---
> > hw/display/cirrus_vga.c | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> > index 379910d..b8c29a6 100644
> > --- a/hw/display/cirrus_vga.c
> > +++ b/hw/display/cirrus_vga.c
> > @@ -277,10 +277,9 @@ static bool blit_region_is_unsafe(struct
> > CirrusVGAState *s,
> > }
> > if (pitch < 0) {
> > int64_t min = addr
> > - + ((int64_t)s->cirrus_blt_height-1) * pitch;
> > - int32_t max = addr
> > - + s->cirrus_blt_width;
> > - if (min < 0 || max > s->vga.vram_size) {
> > + + ((int64_t)s->cirrus_blt_height - 1) * pitch
> > + - s->cirrus_blt_width;
> > + if (min < 0 || addr > s->vga.vram_size) {
>
> Call me paranoid, but shouldn't this be '>='? Missed this yesterday
> apparently, correct me if I'm wrong:
> If VRAM goes from 0..7 it has a size of 8, and this would accept
> address 8 as it's not > size.
I think you are right. The bkwd ops first execute the op, then
decrement, so addr is inclusive and the check is off by one.
> Note that for the blit functions themselves as well as
> blit_region_is_unsafe() the addresses are masked with cirrus_addr_mask.
[ ... ]
> But it seems this is not always the case?
[ ... ]
Looks like a bug indeed.
> Would it make sense to apply the masks in cirrus_bitblt_start() right
> after extracting them from s->vga.gr? Or to not mask the address inside
> blit_is_unafe()?
Applying the mask right after extracting sounds best to me, but I think
I'll better have a close look at the code first ...
cheers,
Gerd
Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue (CVE-2017-TODO), Gerd Hoffmann, 2017/01/25