[Qemu-devel] [PATCH] cirrus: fix oob access issue (CVE-2017-TODO)

[Qemu-devel] [PATCH] cirrus: fix oob access issue (CVE-2017-TODO)

[Gerd Hoffmann]

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) {
             return true;
         }
     } else {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue (CVE-2017-TODO)

[Wolfgang Bumiller]

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

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue (CVE-2017-TODO)

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue (CVE-2017-TODO)

[Laszlo Ersek]

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

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue (CVE-2017-TODO)

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue (CVE-2017-TODO)

[Wolfgang Bumiller]

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.

Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue (CVE-2017-TODO)

[Laszlo Ersek]

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 :(