Re: [PATCH v3 7/8] hw/display/artist: Refactor artist_rop8() to avoid buffer over-run

[Alexander Bulekov]

On 200804 2320, Helge Deller wrote:
> Hi Alexander,
> 
> * Alexander Bulekov <alxndr@bu.edu>:
> > I applied this series and it fixes most of the problems I saw before.
> > I still see a few crashes - I made issues for them on launchpad:
> > https://bugs.launchpad.net/qemu/+bug/1890310
> > https://bugs.launchpad.net/qemu/+bug/1890311

> > https://bugs.launchpad.net/qemu/+bug/1890312
Hi Helge, I can still reproduce this one  ^^^
I'll fuzz it some more, but so far I am not finding anything else.
Thanks!
-Alex

> 
> Thanks for testing!
> Below is the updated patch which fixes all of the issues you reported so
> far. Can you please test again?
> If you like you can pull all changes from
> https://github.com/hdeller/qemu-hppa/commits/target-hppa
> 
> Thanks!
> Helge
> 
> ---------------
> 
> From ee31d3aa9dd91cde3a4df8fce97239a0ff26f7cc Mon Sep 17 00:00:00 2001
> From: Helge Deller <deller@gmx.de>
> Date: Tue, 4 Aug 2020 15:35:38 +0200
> Subject: [PATCH] hw/display/artist: Prevent out of VRAM buffer accesses
> 
> Simplify bounds checks by changing some parameters like row or column
> numbers to become unsigned instead of signed.
> With that we can check if the calculated offset is bigger than the size
> of the VRAM region and bail out if not.
> 
> Reported-by: LLVM libFuzzer
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1880326
> Buglink: https://bugs.launchpad.net/qemu/+bug/1890310
> Buglink: https://bugs.launchpad.net/qemu/+bug/1890311
> Buglink: https://bugs.launchpad.net/qemu/+bug/1890312
> Signed-off-by: Helge Deller <deller@gmx.de>
> 
> diff --git a/hw/display/artist.c b/hw/display/artist.c
> index 47de17b9e9..66a230bbd5 100644
> --- a/hw/display/artist.c
> +++ b/hw/display/artist.c
> @@ -35,9 +35,9 @@
>  struct vram_buffer {
>      MemoryRegion mr;
>      uint8_t *data;
> -    int size;
> -    int width;
> -    int height;
> +    unsigned int size;
> +    unsigned int width;
> +    unsigned int height;
>  };
> 
>  typedef struct ARTISTState {
> @@ -203,14 +203,18 @@ static int16_t artist_get_y(uint32_t reg)
>  }
> 
>  static void artist_invalidate_lines(struct vram_buffer *buf,
> -                                    int starty, int height)
> +                                    unsigned int starty, unsigned int height)
>  {
> -    int start = starty * buf->width;
> -    int size = height * buf->width;
> +    unsigned int start, size;
> 
> -    if (start + size <= buf->size) {
> -        memory_region_set_dirty(&buf->mr, start, size);
> +    if (starty >= buf->height) {
> +        return;
>      }
> +
> +    start = starty * buf->width;
> +    size = MIN(height * buf->width, buf->size - start);
> +
> +    memory_region_set_dirty(&buf->mr, start, size);
>  }
> 
>  static int vram_write_pix_per_transfer(ARTISTState *s)
> @@ -274,15 +278,15 @@ static artist_rop_t artist_get_op(ARTISTState *s)
>  }
> 
>  static void artist_rop8(ARTISTState *s, struct vram_buffer *buf,
> -                        int offset, uint8_t val)
> +                        unsigned int offset, uint8_t val)
>  {
>      const artist_rop_t op = artist_get_op(s);
>      uint8_t plane_mask;
>      uint8_t *dst;
> 
> -    if (offset < 0 || offset >= buf->size) {
> +    if (offset >= buf->size) {
>          qemu_log_mask(LOG_GUEST_ERROR,
> -                      "rop8 offset:%d bufsize:%u\n", offset, buf->size);
> +                      "rop8 offset:%u bufsize:%u\n", offset, buf->size);
>          return;
>      }
>      dst = buf->data + offset;
> @@ -294,8 +298,7 @@ static void artist_rop8(ARTISTState *s, struct 
> vram_buffer *buf,
>          break;
> 
>      case ARTIST_ROP_COPY:
> -        *dst &= ~plane_mask;
> -        *dst |= val & plane_mask;
> +        *dst = (*dst & ~plane_mask) | (val & plane_mask);
>          break;
> 
>      case ARTIST_ROP_XOR:
> @@ -349,7 +352,8 @@ static void vram_bit_write(ARTISTState *s, int posx, int 
> posy, bool incr_x,
>  {
>      struct vram_buffer *buf;
>      uint32_t vram_bitmask = s->vram_bitmask;
> -    int mask, i, pix_count, pix_length, offset, width;
> +    int mask, i, pix_count, pix_length;
> +    unsigned int offset, width;
>      uint8_t *data8, *p;
> 
>      pix_count = vram_write_pix_per_transfer(s);
> @@ -394,7 +398,9 @@ static void vram_bit_write(ARTISTState *s, int posx, int 
> posy, bool incr_x,
> 
>      case 3:
>          if (s->cmap_bm_access) {
> -            *(uint32_t *)(p + offset) = data;
> +            if (offset + 3 < buf->size) {
> +                *(uint32_t *)(p + offset) = data;
> +            }
>              break;
>          }
>          data8 = (uint8_t *)&data;
> @@ -464,12 +470,14 @@ static void vram_bit_write(ARTISTState *s, int posx, 
> int posy, bool incr_x,
>      }
>  }
> 
> -static void block_move(ARTISTState *s, int source_x, int source_y, int 
> dest_x,
> -                       int dest_y, int width, int height)
> +static void block_move(ARTISTState *s,
> +                       unsigned int source_x, unsigned int source_y,
> +                       unsigned int dest_x,   unsigned int dest_y,
> +                       unsigned int width,    unsigned int height)
>  {
>      struct vram_buffer *buf;
>      int line, endline, lineincr, startcolumn, endcolumn, columnincr, column;
> -    uint32_t dst, src;
> +    unsigned int dst, src;
> 
>      trace_artist_block_move(source_x, source_y, dest_x, dest_y, width, 
> height);
> 
> @@ -481,6 +489,12 @@ static void block_move(ARTISTState *s, int source_x, int 
> source_y, int dest_x,
>      }
> 
>      buf = &s->vram_buffer[ARTIST_BUFFER_AP];
> +    if (height > buf->height) {
> +        height = buf->height;
> +    }
> +    if (width > buf->width) {
> +        width = buf->width;
> +    }
> 
>      if (dest_y > source_y) {
>          /* move down */
> @@ -507,24 +521,27 @@ static void block_move(ARTISTState *s, int source_x, 
> int source_y, int dest_x,
>      }
> 
>      for ( ; line != endline; line += lineincr) {
> -        src = source_x + ((line + source_y) * buf->width);
> -        dst = dest_x + ((line + dest_y) * buf->width);
> +        src = source_x + ((line + source_y) * buf->width) + startcolumn;
> +        dst = dest_x + ((line + dest_y) * buf->width) + startcolumn;
> 
>          for (column = startcolumn; column != endcolumn; column += 
> columnincr) {
> -            if (dst + column > buf->size || src + column > buf->size) {
> +            if (dst >= buf->size || src >= buf->size) {
>                  continue;
>              }
> -            artist_rop8(s, buf, dst + column, buf->data[src + column]);
> +            artist_rop8(s, buf, dst, buf->data[src]);
> +            src += columnincr;
> +            dst += columnincr;
>          }
>      }
> 
>      artist_invalidate_lines(buf, dest_y, height);
>  }
> 
> -static void fill_window(ARTISTState *s, int startx, int starty,
> -                        int width, int height)
> +static void fill_window(ARTISTState *s,
> +                        unsigned int startx, unsigned int starty,
> +                        unsigned int width,  unsigned int height)
>  {
> -    uint32_t offset;
> +    unsigned int offset;
>      uint8_t color = artist_get_color(s);
>      struct vram_buffer *buf;
>      int x, y;
> @@ -561,7 +578,9 @@ static void fill_window(ARTISTState *s, int startx, int 
> starty,
>      artist_invalidate_lines(buf, starty, height);
>  }
> 
> -static void draw_line(ARTISTState *s, int x1, int y1, int x2, int y2,
> +static void draw_line(ARTISTState *s,
> +                      unsigned int x1, unsigned int y1,
> +                      unsigned int x2, unsigned int y2,
>                        bool update_start, int skip_pix, int max_pix)
>  {
>      struct vram_buffer *buf = &s->vram_buffer[ARTIST_BUFFER_AP];
> @@ -636,7 +655,7 @@ static void draw_line(ARTISTState *s, int x1, int y1, int 
> x2, int y2,
>      color = artist_get_color(s);
> 
>      do {
> -        int ofs;
> +        unsigned int ofs;
> 
>          if (c1) {
>              ofs = x * s->width + y;
> @@ -768,13 +787,13 @@ static void font_write16(ARTISTState *s, uint16_t val)
>      uint16_t mask;
>      int i;
> 
> -    int startx = artist_get_x(s->vram_start);
> -    int starty = artist_get_y(s->vram_start) + s->font_write_pos_y;
> -    int offset = starty * s->width + startx;
> +    unsigned int startx = artist_get_x(s->vram_start);
> +    unsigned int starty = artist_get_y(s->vram_start) + s->font_write_pos_y;
> +    unsigned int offset = starty * s->width + startx;
> 
>      buf = &s->vram_buffer[ARTIST_BUFFER_AP];
> 
> -    if (offset + 16 > buf->size) {
> +    if (offset + 16 >= buf->size) {
>          return;
>      }
> 
> @@ -1138,7 +1157,7 @@ static void artist_vram_write(void *opaque, hwaddr 
> addr, uint64_t val,
>      struct vram_buffer *buf;
>      int posy = (addr >> 11) & 0x3ff;
>      int posx = addr & 0x7ff;
> -    uint32_t offset;
> +    unsigned int offset;
>      trace_artist_vram_write(size, addr, val);
> 
>      if (s->cmap_bm_access) {
> @@ -1161,16 +1180,22 @@ static void artist_vram_write(void *opaque, hwaddr 
> addr, uint64_t val,
>      offset = posy * buf->width + posx;
>      switch (size) {
>      case 4:
> -        *(uint32_t *)(buf->data + offset) = be32_to_cpu(val);
> -        memory_region_set_dirty(&buf->mr, offset, 4);
> +        if (offset + 3 < buf->size) {
> +            *(uint32_t *)(buf->data + offset) = be32_to_cpu(val);
> +            memory_region_set_dirty(&buf->mr, offset, 4);
> +        }
>          break;
>      case 2:
> -        *(uint16_t *)(buf->data + offset) = be16_to_cpu(val);
> -        memory_region_set_dirty(&buf->mr, offset, 2);
> +        if (offset + 1 < buf->size) {
> +            *(uint16_t *)(buf->data + offset) = be16_to_cpu(val);
> +            memory_region_set_dirty(&buf->mr, offset, 2);
> +        }
>          break;
>      case 1:
> -        *(uint8_t *)(buf->data + offset) = val;
> -        memory_region_set_dirty(&buf->mr, offset, 1);
> +        if (offset < buf->size) {
> +            *(uint8_t *)(buf->data + offset) = val;
> +            memory_region_set_dirty(&buf->mr, offset, 1);
> +        }
>          break;
>      default:
>          break;