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

[Alexander Bulekov]

On 200805 2244, Helge Deller wrote:
> Hello Alexander,
> 
> * Alexander Bulekov <alxndr@bu.edu>:
> > On 200804 2320, Helge Deller wrote:
> > > * 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.
> 
> I've now updated the patch which does address all issues you found
> so far. It's attached below.
> 
> If you like you can pull the full series from
> https://github.com/hdeller/qemu-hppa/commits/target-hppa

Hi Helge,
I pulled from your repo, but I can still reproduce LP1890312
(I build with --enable-sanitizers). Maybe I am missing something? With
this cleared up, I'm happy to leave an Acked-by for the artist patches
in this series.

git show --summary
commit 1657a7a95adc15552138c2b4d310a06128093892 (HEAD, hppa/target-hppa, 
target-hppa)
Author: Helge Deller <deller@gmx.de>
Date:   Tue Aug 4 15:35:38 2020 +0200

    hw/display/artist: Prevent out of VRAM buffer accesses
...

cat << EOF | ./hppa-softmmu/qemu-system-hppa -m 64 -display none -qtest stdio 
-accel qtest
writew 0xf8118001 0x105a
readq 0xf900f8ff
EOF
[I 1596728668.114076] OPENED
[R +0.048943] writew 0xf8118001 0x105a
OK
[S +0.048977] OK
[R +0.048994] readq 0xf900f8ff
/home/alxndr/Development/qemu/general-fuzz/hw/display/artist.c:1218:15: runtime 
error: load of misaligned address 0x7fdbcd40f8ff for type 'uint32_t' (aka 
'unsigned int'), which
requires 4 byte alignment
0x7fdbcd40f8ff: note: pointer points here
<memory cannot be printed>
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior 
/home/alxndr/Development/qemu/general-fuzz/hw/display/artist.c:1218:15 in
AddressSanitizer:DEADLYSIGNAL
=================================================================
==8496==ERROR: AddressSanitizer: SEGV on unknown address 0x7fdbcd40f8ff (pc 
0x55de9a75bf5f bp 0x7ffe26482770 sp 0x7ffe26482510 T0)
==8496==The signal is caused by a READ memory access.
    #0 in artist_vram_read /hw/display/artist.c:1218:15
    #1 in memory_region_read_accessor /softmmu/memory.c:434:11
    #2 in access_with_adjusted_size /softmmu/memory.c:539:18
    #3 in memory_region_dispatch_read1 /softmmu/memory.c:1385:16
    #4 in memory_region_dispatch_read /softmmu/memory.c:1413:9
    #5 in flatview_read_continue /exec.c:3239:23
    #6 in flatview_read /exec.c:3278:12
    #7 in address_space_read_full /exec.c:3291:18
    #8 in address_space_read /include/exec/memory.h:2414:18
    #9 in qtest_process_command /softmmu/qtest.c:485:13
    #10 in qtest_process_inbuf /softmmu/qtest.c:710:9
    #11 in qtest_read /softmmu/qtest.c:722:5
    #12 in qemu_chr_be_write_impl /chardev/char.c:188:9
    #13 in qemu_chr_be_write /chardev/char.c:200:9
    #14 in fd_chr_read /chardev/char-fd.c:68:9
    #15 in qio_channel_fd_source_dispatch /io/channel-watch.c:84:12
    #16 in g_main_context_dispatch 
    #17 in glib_pollfds_poll /util/main-loop.c:217:9
    #18 in os_host_main_loop_wait /util/main-loop.c:240:5
    #19 in main_loop_wait /util/main-loop.c:516:11
    #20 in qemu_main_loop /softmmu/vl.c:1676:9
    #21 in main /softmmu/main.c:49:5

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV 
/home/alxndr/Development/qemu/general-fuzz/hw/display/artist.c:1218:15 in 
artist_vram_read
==8496==ABORTING

-Alex

> I'd be happy if you could recheck if you find anything else.
> 
> In the next step I need to check if everything still works on HP-UX. If
> that's Ok, I'd like to send out a new pull request.  If you could give
> your Acked-by or Reviewed-by it would be great.
> 
> Thanks!
> Helge
> 
> ---
> From 1657a7a95adc15552138c2b4d310a06128093892 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 various bounds checks by changing parameters like row and 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
> Buglink: https://bugs.launchpad.net/qemu/+bug/1890370
> Signed-off-by: Helge Deller <deller@gmx.de>
> 
> diff --git a/hw/display/artist.c b/hw/display/artist.c
> index 47de17b9e9..570811030f 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);
> @@ -364,8 +368,7 @@ static void vram_bit_write(ARTISTState *s, int posx, int 
> posy, bool incr_x,
>          offset = posy * width + posx;
>      }
> 
> -    if (!buf->size) {
> -        qemu_log("write to non-existent buffer\n");
> +    if (!buf->size || offset >= buf->size) {
>          return;
>      }
> 
> @@ -394,7 +397,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 +469,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 +488,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 +520,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 +577,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 +654,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 +786,14 @@ 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 (startx >= buf->width || starty >= buf->height ||
> +        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) {
> @@ -1159,18 +1178,28 @@ static void artist_vram_write(void *opaque, hwaddr 
> addr, uint64_t val,
>      }
> 
>      offset = posy * buf->width + posx;
> +    if (offset >= buf->size) {
> +        return;
> +    }
> +
>      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;