[PATCH] cirrus: handle wraparound in cirrus_invalidate_region

[PATCH] cirrus: handle wraparound in cirrus_invalidate_region

[Gerd Hoffmann]

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.

Buglink: https://bugs.launchpad.net/qemu/+bug/1880189
Cc: Li Qiang <liq3ea@163.com>
Reported-by: Alexander Bulekov <alxndr@bu.edu>
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;
     }
 }
-- 
2.27.0

Re: [PATCH] cirrus: handle wraparound in cirrus_invalidate_region

[Li Qiang]

Gerd Hoffmann <kraxel@redhat.com> 于2020年8月21日周五 下午4:27写道：
>
> 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.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1880189
> Cc: Li Qiang <liq3ea@163.com>
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> 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);

Should here be 's->cirrus_addr_mask + 1 - off_cur'

> +            memory_region_set_dirty(&s->vga.vram, 0, off_cur_end);

And here be 'off_cur_end -1'

Thanks,
Li Qiang

> +        }
>          off_begin += off_pitch;
>      }
>  }
> --
> 2.27.0
>
>

Re: [PATCH] cirrus: handle wraparound in cirrus_invalidate_region

[Gerd Hoffmann]

> >      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);
> 
> Should here be 's->cirrus_addr_mask + 1 - off_cur'

Yes (mask != size).

> > +            memory_region_set_dirty(&s->vga.vram, 0, off_cur_end);
> 
> And here be 'off_cur_end -1'

--verbose please.  I think this one is correct.

take care,
  Gerd

Re: [PATCH] cirrus: handle wraparound in cirrus_invalidate_region

[Li Qiang]

Gerd Hoffmann <kraxel@redhat.com> 于2020年8月31日周一 下午7:23写道：
>
> > >      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);
> >
> > Should here be 's->cirrus_addr_mask + 1 - off_cur'
>
> Yes (mask != size).


Say if we have a range 0~0x2000 then the mask is '0x1fff' and the
off_cur is 0x1000.

0.    0x1000.  0x2000
        off_cur

Then the wrap occurs.

In the first set.
We just sets 0x1fff-0x1000= 0xfff bytes.
In fact we need to set 0x1000 bytes.




>
> > > +            memory_region_set_dirty(&s->vga.vram, 0, off_cur_end);
> >
> > And here be 'off_cur_end -1'
>
> --verbose please.  I think this one is correct.

Here the 'off_cur_end' is size.
In this second set we actually sets 'off_cur_end+1' size bytes.

In a word, I think the first lost a byte and the second added a more byte .

Thank,s
Li Qiang

>
> take care,
>   Gerd
>

Re: [PATCH] cirrus: handle wraparound in cirrus_invalidate_region

[Gerd Hoffmann]

  Hi,

> > > >          off_cur_end = ((off_cur + bytesperline - 1) & s->cirrus_addr_mask) + 1;
[ ... ]
> > > > +            memory_region_set_dirty(&s->vga.vram, 0, off_cur_end);
> > >
> > > And here be 'off_cur_end -1'
> >
> > --verbose please.  I think this one is correct.
> 
> Here the 'off_cur_end' is size.

Exactly.  And memory_region_set_dirty wants the size.  So everything is
fine, right?

take care,
  Gerd

Re: [PATCH] cirrus: handle wraparound in cirrus_invalidate_region

[Li Qiang]

Gerd Hoffmann <kraxel@redhat.com> 于2020年9月1日周二 下午1:15写道：
>
>   Hi,
>
> > > > >          off_cur_end = ((off_cur + bytesperline - 1) & s->cirrus_addr_mask) + 1;
> [ ... ]
> > > > > +            memory_region_set_dirty(&s->vga.vram, 0, off_cur_end);
> > > >
> > > > And here be 'off_cur_end -1'
> > >
> > > --verbose please.  I think this one is correct.
> >
> > Here the 'off_cur_end' is size.
>
> Exactly.  And memory_region_set_dirty wants the size.  So everything is
> fine, right?



+        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);

The s->cirrus_addr_mask can be reached. I mean we can do following:
s->vga.vram[s->cirrus_addr_mask].

If I understand correctly, the 'off_cur' and 's->cirrus_addr_mask' is both index
[off_cur, s->cirrus_addr_mask].

So the len is 's->cirrus_addr_mask->off_cur+1'.


+            memory_region_set_dirty(&s->vga.vram, 0, off_cur_end);


For the 'off_cur_end' here, why we add 1 at the first?:

"off_cur_end = ((off_cur + bytesperline - 1) & s->cirrus_addr_mask) + 1;"
This addition '1' is what I think should be substracted in wrapped cases.

Thanks,
Li Qiang

+        }
         off_begin += off_pitch;
     }
 }



>
> take care,
>   Gerd
>

Re: [PATCH] cirrus: handle wraparound in cirrus_invalidate_region

[Gerd Hoffmann]

  Hi,

> +            /* wraparound */
> +            memory_region_set_dirty(&s->vga.vram, off_cur,
> s->cirrus_addr_mask - off_cur);

> So the len is 's->cirrus_addr_mask->off_cur+1'.

Correct.

> +            memory_region_set_dirty(&s->vga.vram, 0, off_cur_end);
> 
> For the 'off_cur_end' here, why we add 1 at the first?:
> 
> "off_cur_end = ((off_cur + bytesperline - 1) & s->cirrus_addr_mask) + 1;"
                                         ^^^^
> This addition '1' is what I think should be substracted in wrapped cases.

The +1 balances the -1 done before ...

take care,
  Gerd

Re: [PATCH] cirrus: handle wraparound in cirrus_invalidate_region

[Li Qiang]

Gerd Hoffmann <kraxel@redhat.com> 于2020年9月1日周二 下午3:16写道：
>
>   Hi,
>
> > +            /* wraparound */
> > +            memory_region_set_dirty(&s->vga.vram, off_cur,
> > s->cirrus_addr_mask - off_cur);
>
> > So the len is 's->cirrus_addr_mask->off_cur+1'.
>
> Correct.

So do you agree me the first set size should be 's->cirrus_addr_mask -
off_cur+1'?



>
> > +            memory_region_set_dirty(&s->vga.vram, 0, off_cur_end);
> >
> > For the 'off_cur_end' here, why we add 1 at the first?:
> >
> > "off_cur_end = ((off_cur + bytesperline - 1) & s->cirrus_addr_mask) + 1;"
>                                          ^^^^
> > This addition '1' is what I think should be substracted in wrapped cases.
>
> The +1 balances the -1 done before ...

Then the second set size is ok.

Thanks,
Li Qiang

>
> take care,
>   Gerd
>

Re: [PATCH] cirrus: handle wraparound in cirrus_invalidate_region

[Gerd Hoffmann]

On Tue, Sep 01, 2020 at 03:37:17PM +0800, Li Qiang wrote:
> Gerd Hoffmann <kraxel@redhat.com> 于2020年9月1日周二 下午3:16写道：
> >
> >   Hi,
> >
> > > +            /* wraparound */
> > > +            memory_region_set_dirty(&s->vga.vram, off_cur,
> > > s->cirrus_addr_mask - off_cur);
> >
> > > So the len is 's->cirrus_addr_mask->off_cur+1'.
> >
> > Correct.
> 
> So do you agree me the first set size should be 's->cirrus_addr_mask -
> off_cur+1'?

Yes.

> > The +1 balances the -1 done before ...
> 
> Then the second set size is ok.

Ok, good.  v2 sent now.

thanks,
  Gerd

Re: [PATCH] cirrus: handle wraparound in cirrus_invalidate_region

[Philippe Mathieu-Daudé]

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.

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;
>      }
>  }
> 

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;
> >      }
> >  }
> > 
>