diff mbox

[03/10] vga: add vga_scanline_invalidated helper

Message ID 20170404102315.24923-4-kraxel@redhat.com
State New, archived
Headers show

Commit Message

Gerd Hoffmann April 4, 2017, 10:23 a.m. UTC
Add vga_scanline_invalidated helper to check whenever a scanline was
invalidated.  Add a sanity check to fix OOB read access for display
heights larger than 2048.

Only cirrus uses this, for hardware cursor rendering, so having this
work properly for the first 2048 scanlines only shouldn't be a problem
as the cirrus can't handle large resolutions anyway.  Also changing the
invalidated_y_table size would break live migration.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Richard Henderson April 15, 2017, 10:39 a.m. UTC | #1
On 04/04/2017 03:23 AM, Gerd Hoffmann wrote:
> +static bool vga_scanline_invalidated(VGACommonState *s, int y)
> +{
> +    if (y >= VGA_MAX_HEIGHT) {
> +        return false;
> +    }
> +    return s->invalidated_y_table[y >> 5] |= 1 << (y & 0x1f);
> +}
> +
>  void vga_sync_dirty_bitmap(VGACommonState *s)
>  {
>      memory_region_sync_dirty_bitmap(&s->vram);
> @@ -1638,8 +1646,8 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
>          page1 = addr + bwidth - 1;
>          update |= memory_region_get_dirty(&s->vram, page0, page1 - page0,
>                                            DIRTY_MEMORY_VGA);
> -        /* explicit invalidation for the hardware cursor */
> -        update |= (s->invalidated_y_table[y >> 5] >> (y & 0x1f)) & 1;
> +        /* explicit invalidation for the hardware cursor (cirrus only) */
> +        update |= vga_scanline_invalidated(s, y);

The return stmt doesn't match what you're replacing.  Are you really intending 
to modify invalidated_y_table here?


r~
Gerd Hoffmann April 18, 2017, 9:23 a.m. UTC | #2
> > +static bool vga_scanline_invalidated(VGACommonState *s, int y)
> > +{
> > +    if (y >= VGA_MAX_HEIGHT) {
> > +        return false;
> > +    }
> > +    return s->invalidated_y_table[y >> 5] |= 1 << (y & 0x1f);
> > +}

> The return stmt doesn't match what you're replacing.  Are you really intending 
> to modify invalidated_y_table here?

No.  Really stupid cut&paste bug.  Fixed.

thanks,
  Gerd
diff mbox

Patch

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 69c3e1d..f320946 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1434,6 +1434,14 @@  void vga_invalidate_scanlines(VGACommonState *s, int y1, int y2)
     }
 }
 
+static bool vga_scanline_invalidated(VGACommonState *s, int y)
+{
+    if (y >= VGA_MAX_HEIGHT) {
+        return false;
+    }
+    return s->invalidated_y_table[y >> 5] |= 1 << (y & 0x1f);
+}
+
 void vga_sync_dirty_bitmap(VGACommonState *s)
 {
     memory_region_sync_dirty_bitmap(&s->vram);
@@ -1638,8 +1646,8 @@  static void vga_draw_graphic(VGACommonState *s, int full_update)
         page1 = addr + bwidth - 1;
         update |= memory_region_get_dirty(&s->vram, page0, page1 - page0,
                                           DIRTY_MEMORY_VGA);
-        /* explicit invalidation for the hardware cursor */
-        update |= (s->invalidated_y_table[y >> 5] >> (y & 0x1f)) & 1;
+        /* explicit invalidation for the hardware cursor (cirrus only) */
+        update |= vga_scanline_invalidated(s, y);
         if (update) {
             if (y_start < 0)
                 y_start = y;
@@ -1686,7 +1694,7 @@  static void vga_draw_graphic(VGACommonState *s, int full_update)
                                   page_max - page_min,
                                   DIRTY_MEMORY_VGA);
     }
-    memset(s->invalidated_y_table, 0, ((height + 31) >> 5) * 4);
+    memset(s->invalidated_y_table, 0, sizeof(s->invalidated_y_table));
 }
 
 static void vga_draw_blank(VGACommonState *s, int full_update)