| Message ID | 1485328025-3783-1-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jan 25, 2017 at 08:07:05AM +0100, Gerd Hoffmann wrote: > From: Li Qiang <liqiang6-s@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 <liqiang6-s@360.cn> > Message-id: 5887254f.863a240a.2c122.5500@mx.google.com > > { kraxel: with backward blits (negative pitch) addr is the topmost > address, so check it as-is against vram size ] > > Cc: qemu-stable@nongnu.org > Cc: P J P <ppandit@redhat.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Wolfgang Bumiller <w.bumiller@proxmox.com> > Fixes: d3532a0db02296e687711b8cdc7791924efccea0 (CVE-2014-8106) > Signed-off-by: Gerd Hoffmann <kraxel@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. Note that for the blit functions themselves as well as blit_region_is_unsafe() the addresses are masked with cirrus_addr_mask. Like: if (blit_region_is_unsafe(s, s->cirrus_blt_dstpitch, s->cirrus_blt_dstaddr & s->cirrus_addr_mask)) { And eg. cirrus_bitblt_common_patterncopy() does: dst = s->vga.vram_ptr + (s->cirrus_blt_dstaddr & s->cirrus_addr_mask); But it seems this is not always the case? (Also adding a negative pitch to this would then move in the wrong direction...) Also in cirrus_bitblt_common_patterncopy() there is a call: cirrus_invalidate_region(s, s->cirrus_blt_dstaddr, s->cirrus_blt_dstpitch, s->cirrus_blt_width, s->cirrus_blt_height); This in turn takes the dstaddr's beginning as is and only masks the dst+pitch address like this: off_cur = off_begin; off_cur_end = (off_cur + bytesperline) & s->cirrus_addr_mask; memory_region_set_dirty(&s->vga.vram, off_cur, off_cur_end - off_cur); off_begin += off_pitch; So the first memory_region_set_dirty() call would if I'm not mistaken get a bad address? 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()? > return true; > } > } else { > -- > 1.8.3.1
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 <liqiang6-s@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 <liqiang6-s@360.cn> > > Message-id: 5887254f.863a240a.2c122.5500@mx.google.com > > > > { kraxel: with backward blits (negative pitch) addr is the topmost > > address, so check it as-is against vram size ] > > > > Cc: qemu-stable@nongnu.org > > Cc: P J P <ppandit@redhat.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: Wolfgang Bumiller <w.bumiller@proxmox.com> > > Fixes: d3532a0db02296e687711b8cdc7791924efccea0 (CVE-2014-8106) > > Signed-off-by: Gerd Hoffmann <kraxel@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
On 01/25/17 10:50, Gerd Hoffmann wrote: > 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 <liqiang6-s@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 <liqiang6-s@360.cn> >>> Message-id: 5887254f.863a240a.2c122.5500@mx.google.com >>> >>> { kraxel: with backward blits (negative pitch) addr is the topmost >>> address, so check it as-is against vram size ] >>> >>> Cc: qemu-stable@nongnu.org >>> Cc: P J P <ppandit@redhat.com> >>> Cc: Laszlo Ersek <lersek@redhat.com> >>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>> Cc: Wolfgang Bumiller <w.bumiller@proxmox.com> >>> Fixes: d3532a0db02296e687711b8cdc7791924efccea0 (CVE-2014-8106) >>> Signed-off-by: Gerd Hoffmann <kraxel@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. That's right IMO; however, in that case we also have to posit that "min" is exclusive. Assume that we have 16 pixels in the VGA memory (4x4), and that we are massaging the bottom right quadrant: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 addr = 15 height = 2 width = 2 pitch = -4 Then min = addr + (height - 1) * pitch - width = 15 + ( 2 - 1) * (-4) - 2 = 9 Which is the address right before the top left pixel; that is, it marks the first pixel *not* accessed. If that value was (-1), then the operation would still be valid. So we should accept (min == -1) -- this is dictated by plain symmetry. If "max" -- here, "addr" -- is inclusive, then "min" becomes exclusive. Thanks Laszlo
On Mi, 2017-01-25 at 08:07 +0100, Gerd Hoffmann wrote: > From: Li Qiang <liqiang6-s@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 <liqiang6-s@360.cn> > Message-id: 5887254f.863a240a.2c122.5500@mx.google.com > > { kraxel: with backward blits (negative pitch) addr is the topmost > address, so check it as-is against vram size ] > > Cc: qemu-stable@nongnu.org > Cc: P J P <ppandit@redhat.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Wolfgang Bumiller <w.bumiller@proxmox.com> > Fixes: d3532a0db02296e687711b8cdc7791924efccea0 (CVE-2014-8106) > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> For testers: All pending cirrus fixes are now pushed to: git://git.kraxel.org/qemu queue/vga Gerd Hoffmann (1): cirrus: fix blit address mask handling Li Qiang (1): cirrus: fix oob access issue (CVE-2017-TODO) Wolfgang Bumiller (1): cirrus: allow zero source pitch in pattern fill rops cheers, Gerd
On Wed, Jan 25, 2017 at 11:35:44AM +0100, Laszlo Ersek wrote: > On 01/25/17 10:50, Gerd Hoffmann wrote: > > 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 <liqiang6-s@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 <liqiang6-s@360.cn> > >>> Message-id: 5887254f.863a240a.2c122.5500@mx.google.com > >>> > >>> { kraxel: with backward blits (negative pitch) addr is the topmost > >>> address, so check it as-is against vram size ] > >>> > >>> Cc: qemu-stable@nongnu.org > >>> Cc: P J P <ppandit@redhat.com> > >>> Cc: Laszlo Ersek <lersek@redhat.com> > >>> Cc: Paolo Bonzini <pbonzini@redhat.com> > >>> Cc: Wolfgang Bumiller <w.bumiller@proxmox.com> > >>> Fixes: d3532a0db02296e687711b8cdc7791924efccea0 (CVE-2014-8106) > >>> Signed-off-by: Gerd Hoffmann <kraxel@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. > > That's right IMO; however, in that case we also have to posit that "min" > is exclusive. Assume that we have 16 pixels in the VGA memory (4x4), and > that we are massaging the bottom right quadrant: > > 0 1 2 3 > 4 5 6 7 > 8 9 10 11 > 12 13 14 15 > > addr = 15 > height = 2 > width = 2 > pitch = -4 > > Then > > min = addr + (height - 1) * pitch - width > = 15 + ( 2 - 1) * (-4) - 2 > = 9 > > Which is the address right before the top left pixel; that is, it marks > the first pixel *not* accessed. > > If that value was (-1), then the operation would still be valid. > > So we should accept (min == -1) -- this is dictated by plain symmetry. > If "max" -- here, "addr" -- is inclusive, then "min" becomes exclusive. You're right. You'd think it wouldn't take so many different people to notice these things :(. It was right there, I should have noticed it.
On 01/25/17 11:50, Wolfgang Bumiller wrote: > On Wed, Jan 25, 2017 at 11:35:44AM +0100, Laszlo Ersek wrote: >> On 01/25/17 10:50, Gerd Hoffmann wrote: >>> 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 <liqiang6-s@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 <liqiang6-s@360.cn> >>>>> Message-id: 5887254f.863a240a.2c122.5500@mx.google.com >>>>> >>>>> { kraxel: with backward blits (negative pitch) addr is the topmost >>>>> address, so check it as-is against vram size ] >>>>> >>>>> Cc: qemu-stable@nongnu.org >>>>> Cc: P J P <ppandit@redhat.com> >>>>> Cc: Laszlo Ersek <lersek@redhat.com> >>>>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>>>> Cc: Wolfgang Bumiller <w.bumiller@proxmox.com> >>>>> Fixes: d3532a0db02296e687711b8cdc7791924efccea0 (CVE-2014-8106) >>>>> Signed-off-by: Gerd Hoffmann <kraxel@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. >> >> That's right IMO; however, in that case we also have to posit that "min" >> is exclusive. Assume that we have 16 pixels in the VGA memory (4x4), and >> that we are massaging the bottom right quadrant: >> >> 0 1 2 3 >> 4 5 6 7 >> 8 9 10 11 >> 12 13 14 15 >> >> addr = 15 >> height = 2 >> width = 2 >> pitch = -4 >> >> Then >> >> min = addr + (height - 1) * pitch - width >> = 15 + ( 2 - 1) * (-4) - 2 >> = 9 >> >> Which is the address right before the top left pixel; that is, it marks >> the first pixel *not* accessed. >> >> If that value was (-1), then the operation would still be valid. >> >> So we should accept (min == -1) -- this is dictated by plain symmetry. >> If "max" -- here, "addr" -- is inclusive, then "min" becomes exclusive. > > You're right. > > You'd think it wouldn't take so many different people to notice these > things :(. It was right there, I should have noticed it. > I could tell you the same about "addr" pointing to bottom-left vs. bottom-right, in the original patch :(
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) { return true; } } else {