| Message ID | 20200821082622.7169-1-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cirrus: handle wraparound in cirrus_invalidate_region | expand |
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 > >
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; > } > } >
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; > > } > > } > > >
> > 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
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 >
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
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 >
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
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 >
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
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; } }
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(-)