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 <liqiang...@360.cn> > > > > 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 <liqiang...@360.cn> > > Message-id: 5887254f.863a240a.2c122.5...@mx.google.com > > > > { kraxel: with backward blits (negative pitch) addr is the topmost > > address, so check it as-is against vram size ] > > > > Cc: qemu-sta...@nongnu.org > > Cc: P J P <ppan...@redhat.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Cc: Paolo Bonzini <pbonz...@redhat.com> > > Cc: Wolfgang Bumiller <w.bumil...@proxmox.com> > > Fixes: d3532a0db02296e687711b8cdc7791924efccea0 (CVE-2014-8106) > > Signed-off-by: Gerd Hoffmann <kra...@redhat.com> > > --- > > 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