Re: [PATCH] cirrus: handle wraparound in cirrus_invalidate_region

[Alexander Bulekov]

On 200821 1251, Philippe Mathieu-Daudé wrote:
> On 8/21/20 10:26 AM, Gerd Hoffmann wrote:
> > Code simply asserts that there is no wraparound instead of handling
> > it properly.  The assert() can be triggered by the guest (must be
> > privilidged inside the guest though).  Fix it.
> 
> Thanks for fixing this!
> 
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1880189
> > Cc: Li Qiang <liq3ea@163.com>
> > Reported-by: Alexander Bulekov <alxndr@bu.edu>
> 
> Well, I reported this one 3 months ago to secalert, not Alex.

Nice. In fact, I don't think I've come across this one yet, even with
fairly good coverage over the cirrus. Maybe its just a matter of
increasing the maximum input length. I think with my parser each out
instruction weighs in at 15 bytes + 4 byte separator, so it would take
900-1000 bytes to generate your reproducer, whereas I temporarily set
the limit at 400.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  hw/display/cirrus_vga.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> > index 212d6f5e6145..b91b64347473 100644
> > --- a/hw/display/cirrus_vga.c
> > +++ b/hw/display/cirrus_vga.c
> > @@ -640,10 +640,15 @@ static void cirrus_invalidate_region(CirrusVGAState * 
> > s, int off_begin,
> >      }
> >  
> >      for (y = 0; y < lines; y++) {
> > -        off_cur = off_begin;
> > +        off_cur = off_begin & s->cirrus_addr_mask;
> >          off_cur_end = ((off_cur + bytesperline - 1) & s->cirrus_addr_mask) 
> > + 1;
> > -        assert(off_cur_end >= off_cur);
> > -        memory_region_set_dirty(&s->vga.vram, off_cur, off_cur_end - 
> > off_cur);
> > +        if (off_cur_end >= off_cur) {
> > +            memory_region_set_dirty(&s->vga.vram, off_cur, off_cur_end - 
> > off_cur);
> > +        } else {
> > +            /* wraparound */
> > +            memory_region_set_dirty(&s->vga.vram, off_cur, 
> > s->cirrus_addr_mask - off_cur);
> > +            memory_region_set_dirty(&s->vga.vram, 0, off_cur_end);
> > +        }
> >          off_begin += off_pitch;
> >      }
> >  }
> > 
>