| Message ID | 20200804140056.7690-8-deller@gmx.de |
|---|---|
| State | New |
| Headers | show |
| Series | target-hppa fixes v3 | expand |
Hi Helge, 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 Thanks! -Alex On 200804 1600, Helge Deller wrote: > From: Philippe Mathieu-Daudé <f4bug@amsat.org> > > Invalid I/O writes can craft an offset out of the vram_buffer range. > Instead of passing an unsafe pointer to artist_rop8(), pass the vram_buffer and > the offset. We can now check if the offset is in range before accessing it. > > We avoid: > > Program terminated with signal SIGSEGV, Segmentation fault. > 284 *dst &= ~plane_mask; > (gdb) bt > #0 0x000056367b2085c0 in artist_rop8 (s=0x56367d38b510, dst=0x7f9f972fffff <error: Cannot access memory at address 0x7f9f972fffff>, val=0 '\000') at hw/display/artist.c:284 > #1 0x000056367b209325 in draw_line (s=0x56367d38b510, x1=-20480, y1=-1, x2=0, y2=17920, update_start=true, skip_pix=-1, max_pix=-1) at hw/display/artist.c:646 > > Reported-by: LLVM libFuzzer > Buglink: https://bugs.launchpad.net/qemu/+bug/1880326 > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > Signed-off-by: Helge Deller <deller@gmx.de> > --- > hw/display/artist.c | 40 +++++++++++++++++++++++++--------------- > 1 file changed, 25 insertions(+), 15 deletions(-) > > diff --git a/hw/display/artist.c b/hw/display/artist.c > index a206afe641..47de17b9e9 100644 > --- a/hw/display/artist.c > +++ b/hw/display/artist.c > @@ -273,11 +273,20 @@ static artist_rop_t artist_get_op(ARTISTState *s) > return (s->image_bitmap_op >> 8) & 0xf; > } > > -static void artist_rop8(ARTISTState *s, uint8_t *dst, uint8_t val) > +static void artist_rop8(ARTISTState *s, struct vram_buffer *buf, > + int offset, uint8_t val) > { > - > const artist_rop_t op = artist_get_op(s); > - uint8_t plane_mask = s->plane_mask & 0xff; > + uint8_t plane_mask; > + uint8_t *dst; > + > + if (offset < 0 || offset >= buf->size) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "rop8 offset:%d bufsize:%u\n", offset, buf->size); > + return; > + } > + dst = buf->data + offset; > + plane_mask = s->plane_mask & 0xff; > > switch (op) { > case ARTIST_ROP_CLEAR: > @@ -375,7 +384,7 @@ static void vram_bit_write(ARTISTState *s, int posx, int posy, bool incr_x, > for (i = 0; i < pix_count; i++) { > uint32_t off = offset + pix_count - 1 - i; > if (off < buf->size) { > - artist_rop8(s, p + off, > + artist_rop8(s, buf, off, > (data & 1) ? (s->plane_mask >> 24) : 0); > } > data >>= 1; > @@ -395,7 +404,7 @@ static void vram_bit_write(ARTISTState *s, int posx, int posy, bool incr_x, > s->vram_bitmask & (1 << (28 + i))) { > uint32_t off = offset + 3 - i; > if (off < buf->size) { > - artist_rop8(s, p + off, data8[ROP8OFF(i)]); > + artist_rop8(s, buf, off, data8[ROP8OFF(i)]); > } > } > } > @@ -424,10 +433,10 @@ static void vram_bit_write(ARTISTState *s, int posx, int posy, bool incr_x, > if (!(s->image_bitmap_op & 0x20000000) || > (vram_bitmask & mask)) { > if (data & mask) { > - artist_rop8(s, p + offset + i, s->fg_color); > + artist_rop8(s, buf, offset + i, s->fg_color); > } else { > if (!(s->image_bitmap_op & 0x10000002)) { > - artist_rop8(s, p + offset + i, s->bg_color); > + artist_rop8(s, buf, offset + i, s->bg_color); > } > } > } > @@ -505,7 +514,7 @@ static void block_move(ARTISTState *s, int source_x, int source_y, int dest_x, > if (dst + column > buf->size || src + column > buf->size) { > continue; > } > - artist_rop8(s, buf->data + dst + column, buf->data[src + column]); > + artist_rop8(s, buf, dst + column, buf->data[src + column]); > } > } > > @@ -546,7 +555,7 @@ static void fill_window(ARTISTState *s, int startx, int starty, > offset = y * s->width; > > for (x = startx; x < startx + width; x++) { > - artist_rop8(s, buf->data + offset + x, color); > + artist_rop8(s, buf, offset + x, color); > } > } > artist_invalidate_lines(buf, starty, height); > @@ -559,7 +568,6 @@ static void draw_line(ARTISTState *s, int x1, int y1, int x2, int y2, > uint8_t color; > int dx, dy, t, e, x, y, incy, diago, horiz; > bool c1; > - uint8_t *p; > > trace_artist_draw_line(x1, y1, x2, y2); > > @@ -628,16 +636,18 @@ static void draw_line(ARTISTState *s, int x1, int y1, int x2, int y2, > color = artist_get_color(s); > > do { > + int ofs; > + > if (c1) { > - p = buf->data + x * s->width + y; > + ofs = x * s->width + y; > } else { > - p = buf->data + y * s->width + x; > + ofs = y * s->width + x; > } > > if (skip_pix > 0) { > skip_pix--; > } else { > - artist_rop8(s, p, color); > + artist_rop8(s, buf, ofs, color); > } > > if (e > 0) { > @@ -771,10 +781,10 @@ static void font_write16(ARTISTState *s, uint16_t val) > for (i = 0; i < 16; i++) { > mask = 1 << (15 - i); > if (val & mask) { > - artist_rop8(s, buf->data + offset + i, color); > + artist_rop8(s, buf, offset + i, color); > } else { > if (!(s->image_bitmap_op & 0x20000000)) { > - artist_rop8(s, buf->data + offset + i, s->bg_color); > + artist_rop8(s, buf, offset + i, s->bg_color); > } > } > } > -- > 2.21.3 > >
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 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;
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;
On 200804 1801, Alexander Bulekov wrote: > 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 Found one more. It doesn't occur without these patches, but I forgot to check and submitted a bug: https://bugs.launchpad.net/qemu/+bug/1890370 Oops.. Closed the report, but the details about the segfault are there. I think I hit the limits of how much code I will be able to cover, so I'll probably stop testing now. 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;
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 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;
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;
Hello Alexander, On 06.08.20 17:46, Alexander Bulekov wrote: > On 200805 2244, Helge Deller wrote: >> * 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. >> > 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 The current tree at https://github.com/hdeller/qemu-hppa/commits/target-hppa does survive all your tests and in addition fixes an artist bug which made the dtwm window manager rendering wrong on HP-UX. Please completely revert your tree before pulling again. I'll send out a complete patch series shortly. Thanks! Helge
On 200809 0717, Helge Deller wrote: > Hello Alexander, > > On 06.08.20 17:46, Alexander Bulekov wrote: > > On 200805 2244, Helge Deller wrote: > >> * 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. > >> > > 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 > > The current tree at > https://github.com/hdeller/qemu-hppa/commits/target-hppa > does survive all your tests and in addition fixes an artist bug which > made the dtwm window manager rendering wrong on HP-UX. > Please completely revert your tree before pulling again. > I'll send out a complete patch series shortly. > Hi Helge, This still causes a segfault for me: cat << EOF | ./hppa-softmmu/qemu-system-hppa -m 64 -display none \ -qtest stdio -accel qtest writew 0xf8118001 0x105a readq 0xf900f8ff EOF I think the problem is that there is a bounds check missing on the artist_vram_read with s->cmap_bm_access path. The bounds check is present for artist_vram_write, so I just copied it over, and that fixed the issue (applied on top of the current qemu-hppa/target-hppa): diff --git a/hw/display/artist.c b/hw/display/artist.c index 720db179ab..678023bb3a 100644 --- a/hw/display/artist.c +++ b/hw/display/artist.c @@ -1216,8 +1216,10 @@ static uint64_t artist_vram_read(void *opaque, hwaddr addr, unsigned size) if (s->cmap_bm_access) { buf = &s->vram_buffer[ARTIST_BUFFER_CMAP]; - val = *(uint32_t *)(buf->data + addr); - trace_artist_vram_read(size, addr, 0, 0, val); + if (addr + 3 < buf->size) { + val = *(uint32_t *)(buf->data + addr); + trace_artist_vram_read(size, addr, 0, 0, val); + } return 0; } -Alex > Thanks! > Helge
On 09.08.20 19:17, Alexander Bulekov wrote: > On 200809 0717, Helge Deller wrote: >> Hello Alexander, >> >> On 06.08.20 17:46, Alexander Bulekov wrote: >>> On 200805 2244, Helge Deller wrote: >>>> * 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. >>>> >>> 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 >> >> The current tree at >> https://github.com/hdeller/qemu-hppa/commits/target-hppa >> does survive all your tests and in addition fixes an artist bug which >> made the dtwm window manager rendering wrong on HP-UX. >> Please completely revert your tree before pulling again. >> I'll send out a complete patch series shortly. >> > > Hi Helge, > This still causes a segfault for me: > > cat << EOF | ./hppa-softmmu/qemu-system-hppa -m 64 -display none \ > -qtest stdio -accel qtest > writew 0xf8118001 0x105a > readq 0xf900f8ff > EOF It's strange, I don't know why, but I can't reproduce it: [deller@ls3530 qemu-helge-system]$ cat << EOF | ./hppa-softmmu/qemu-system-hppa -m 64 -display none -qtest stdio -accel qtest writew 0xf8118001 0x105a readq 0xf900f8ff EOF [I 1596999589.838131] OPENED [R +0.006113] writew 0xf8118001 0x105a OK [S +0.006128] OK [R +0.006137] readq 0xf900f8ff OK 0x0000000000000000 [S +0.006141] OK 0x0000000000000000 [I +0.006163] CLOSED Nevertheless, you are correct that there is a bounds check missing! > I think the problem is that there is a bounds check missing on the > artist_vram_read with s->cmap_bm_access path. The bounds check is > present for artist_vram_write, so I just copied it over, and that fixed > the issue (applied on top of the current qemu-hppa/target-hppa): I'll apply it with minor modifications... > > diff --git a/hw/display/artist.c b/hw/display/artist.c > index 720db179ab..678023bb3a 100644 > --- a/hw/display/artist.c > +++ b/hw/display/artist.c > @@ -1216,8 +1216,10 @@ static uint64_t artist_vram_read(void *opaque, hwaddr addr, unsigned size) > > if (s->cmap_bm_access) { > buf = &s->vram_buffer[ARTIST_BUFFER_CMAP]; > - val = *(uint32_t *)(buf->data + addr); > - trace_artist_vram_read(size, addr, 0, 0, val); > + if (addr + 3 < buf->size) { We need to check addr < buf->size too, otherwise with addr = 0xffffffff would succeed too: if (addr < buf->size && addr + 3 < buf->size) { > + val = *(uint32_t *)(buf->data + addr); > + trace_artist_vram_read(size, addr, 0, 0, val); > + } > return 0; This seems wrong... should be "return val;", otherwise the whole function doesn't make sense. Thank you! Helge
On 09.08.20 21:38, Helge Deller wrote: > On 09.08.20 19:17, Alexander Bulekov wrote: >> On 200809 0717, Helge Deller wrote: >>> The current tree at >>> https://github.com/hdeller/qemu-hppa/commits/target-hppa >>> does survive all your tests and in addition fixes an artist bug which >>> made the dtwm window manager rendering wrong on HP-UX. >>> Please completely revert your tree before pulling again. >>> I'll send out a complete patch series shortly. >>> >> This still causes a segfault for me: >> >> cat << EOF | ./hppa-softmmu/qemu-system-hppa -m 64 -display none \ >> -qtest stdio -accel qtest >> writew 0xf8118001 0x105a >> readq 0xf900f8ff >> EOF Alexander, I've committed the patch below to the git tree at: https://github.com/hdeller/qemu-hppa/tree/target-hppa Ok? Helge From 0725d38381330138052e282bb61feeacc0c1e50b Mon Sep 17 00:00:00 2001 From: Helge Deller <deller@gmx.de> Date: Wed, 9 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 Acked-by: Alexander Bulekov <alxndr@bu.edu> Signed-off-by: Helge Deller <deller@gmx.de> diff --git a/hw/display/artist.c b/hw/display/artist.c index f37aa9eb49..46eaa10dae 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 { @@ -274,15 +274,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 +294,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 +348,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 +364,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 +393,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 +465,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 +484,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 +516,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 +573,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]; @@ -571,12 +585,12 @@ static void draw_line(ARTISTState *s, int x1, int y1, int x2, int y2, trace_artist_draw_line(x1, y1, x2, y2); - if (x1 * y1 >= buf->size || x2 * y2 >= buf->size) { - qemu_log_mask(LOG_GUEST_ERROR, - "draw_line (%d,%d) (%d,%d)\n", x1, y1, x2, y2); - return; + if ((x1 >= buf->width && x2 >= buf->width) || + (y1 >= buf->height && y2 >= buf->height)) { + return; } + if (update_start) { s->vram_start = (x2 << 16) | y2; } @@ -633,7 +647,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; @@ -765,13 +779,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; } @@ -1135,7 +1150,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) { @@ -1156,18 +1171,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; @@ -1183,9 +1208,12 @@ static uint64_t artist_vram_read(void *opaque, hwaddr addr, unsigned size) if (s->cmap_bm_access) { buf = &s->vram_buffer[ARTIST_BUFFER_CMAP]; - val = *(uint32_t *)(buf->data + addr); + val = 0; + if (addr < buf->size && addr + 3 < buf->size) { + val = *(uint32_t *)(buf->data + addr); + } trace_artist_vram_read(size, addr, 0, 0, val); - return 0; + return val; } buf = vram_read_buffer(s);
On 200809 2151, Helge Deller wrote: > On 09.08.20 21:38, Helge Deller wrote: > > On 09.08.20 19:17, Alexander Bulekov wrote: > >> On 200809 0717, Helge Deller wrote: > >>> The current tree at > >>> https://github.com/hdeller/qemu-hppa/commits/target-hppa > >>> does survive all your tests and in addition fixes an artist bug which > >>> made the dtwm window manager rendering wrong on HP-UX. > >>> Please completely revert your tree before pulling again. > >>> I'll send out a complete patch series shortly. > >>> > >> This still causes a segfault for me: > >> > >> cat << EOF | ./hppa-softmmu/qemu-system-hppa -m 64 -display none \ > >> -qtest stdio -accel qtest > >> writew 0xf8118001 0x105a > >> readq 0xf900f8ff > >> EOF > > Alexander, I've committed the patch below to the git tree at: > https://github.com/hdeller/qemu-hppa/tree/target-hppa > Ok? > Hi Helge, Yes - I tried this out and it looks like all the test cases are fixed. Its strange that LP1890370 doesn't reproduce for you. Did you build with --enable-sanitizers? For this commit: https://github.com/hdeller/qemu-hppa/commit/2db0481e69cf8ca764bc0e41c86ac9a037ae4de5 Acked-by: Alexander Bulekov <alxndr@bu.edu> Thanks! -Alex > Helge > From 0725d38381330138052e282bb61feeacc0c1e50b Mon Sep 17 00:00:00 2001 > From: Helge Deller <deller@gmx.de> > Date: Wed, 9 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 > Acked-by: Alexander Bulekov <alxndr@bu.edu> > Signed-off-by: Helge Deller <deller@gmx.de> > > diff --git a/hw/display/artist.c b/hw/display/artist.c > index f37aa9eb49..46eaa10dae 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 { > @@ -274,15 +274,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 +294,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 +348,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 +364,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 +393,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 +465,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 +484,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 +516,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 +573,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]; > @@ -571,12 +585,12 @@ static void draw_line(ARTISTState *s, int x1, int y1, int x2, int y2, > > trace_artist_draw_line(x1, y1, x2, y2); > > - if (x1 * y1 >= buf->size || x2 * y2 >= buf->size) { > - qemu_log_mask(LOG_GUEST_ERROR, > - "draw_line (%d,%d) (%d,%d)\n", x1, y1, x2, y2); > - return; > + if ((x1 >= buf->width && x2 >= buf->width) || > + (y1 >= buf->height && y2 >= buf->height)) { > + return; > } > > + > if (update_start) { > s->vram_start = (x2 << 16) | y2; > } > @@ -633,7 +647,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; > @@ -765,13 +779,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; > } > > @@ -1135,7 +1150,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) { > @@ -1156,18 +1171,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; > @@ -1183,9 +1208,12 @@ static uint64_t artist_vram_read(void *opaque, hwaddr addr, unsigned size) > > if (s->cmap_bm_access) { > buf = &s->vram_buffer[ARTIST_BUFFER_CMAP]; > - val = *(uint32_t *)(buf->data + addr); > + val = 0; > + if (addr < buf->size && addr + 3 < buf->size) { > + val = *(uint32_t *)(buf->data + addr); > + } > trace_artist_vram_read(size, addr, 0, 0, val); > - return 0; > + return val; > } > > buf = vram_read_buffer(s);
diff --git a/hw/display/artist.c b/hw/display/artist.c index a206afe641..47de17b9e9 100644 --- a/hw/display/artist.c +++ b/hw/display/artist.c @@ -273,11 +273,20 @@ static artist_rop_t artist_get_op(ARTISTState *s) return (s->image_bitmap_op >> 8) & 0xf; } -static void artist_rop8(ARTISTState *s, uint8_t *dst, uint8_t val) +static void artist_rop8(ARTISTState *s, struct vram_buffer *buf, + int offset, uint8_t val) { - const artist_rop_t op = artist_get_op(s); - uint8_t plane_mask = s->plane_mask & 0xff; + uint8_t plane_mask; + uint8_t *dst; + + if (offset < 0 || offset >= buf->size) { + qemu_log_mask(LOG_GUEST_ERROR, + "rop8 offset:%d bufsize:%u\n", offset, buf->size); + return; + } + dst = buf->data + offset; + plane_mask = s->plane_mask & 0xff; switch (op) { case ARTIST_ROP_CLEAR: @@ -375,7 +384,7 @@ static void vram_bit_write(ARTISTState *s, int posx, int posy, bool incr_x, for (i = 0; i < pix_count; i++) { uint32_t off = offset + pix_count - 1 - i; if (off < buf->size) { - artist_rop8(s, p + off, + artist_rop8(s, buf, off, (data & 1) ? (s->plane_mask >> 24) : 0); } data >>= 1; @@ -395,7 +404,7 @@ static void vram_bit_write(ARTISTState *s, int posx, int posy, bool incr_x, s->vram_bitmask & (1 << (28 + i))) { uint32_t off = offset + 3 - i; if (off < buf->size) { - artist_rop8(s, p + off, data8[ROP8OFF(i)]); + artist_rop8(s, buf, off, data8[ROP8OFF(i)]); } } } @@ -424,10 +433,10 @@ static void vram_bit_write(ARTISTState *s, int posx, int posy, bool incr_x, if (!(s->image_bitmap_op & 0x20000000) || (vram_bitmask & mask)) { if (data & mask) { - artist_rop8(s, p + offset + i, s->fg_color); + artist_rop8(s, buf, offset + i, s->fg_color); } else { if (!(s->image_bitmap_op & 0x10000002)) { - artist_rop8(s, p + offset + i, s->bg_color); + artist_rop8(s, buf, offset + i, s->bg_color); } } } @@ -505,7 +514,7 @@ static void block_move(ARTISTState *s, int source_x, int source_y, int dest_x, if (dst + column > buf->size || src + column > buf->size) { continue; } - artist_rop8(s, buf->data + dst + column, buf->data[src + column]); + artist_rop8(s, buf, dst + column, buf->data[src + column]); } } @@ -546,7 +555,7 @@ static void fill_window(ARTISTState *s, int startx, int starty, offset = y * s->width; for (x = startx; x < startx + width; x++) { - artist_rop8(s, buf->data + offset + x, color); + artist_rop8(s, buf, offset + x, color); } } artist_invalidate_lines(buf, starty, height); @@ -559,7 +568,6 @@ static void draw_line(ARTISTState *s, int x1, int y1, int x2, int y2, uint8_t color; int dx, dy, t, e, x, y, incy, diago, horiz; bool c1; - uint8_t *p; trace_artist_draw_line(x1, y1, x2, y2); @@ -628,16 +636,18 @@ static void draw_line(ARTISTState *s, int x1, int y1, int x2, int y2, color = artist_get_color(s); do { + int ofs; + if (c1) { - p = buf->data + x * s->width + y; + ofs = x * s->width + y; } else { - p = buf->data + y * s->width + x; + ofs = y * s->width + x; } if (skip_pix > 0) { skip_pix--; } else { - artist_rop8(s, p, color); + artist_rop8(s, buf, ofs, color); } if (e > 0) { @@ -771,10 +781,10 @@ static void font_write16(ARTISTState *s, uint16_t val) for (i = 0; i < 16; i++) { mask = 1 << (15 - i); if (val & mask) { - artist_rop8(s, buf->data + offset + i, color); + artist_rop8(s, buf, offset + i, color); } else { if (!(s->image_bitmap_op & 0x20000000)) { - artist_rop8(s, buf->data + offset + i, s->bg_color); + artist_rop8(s, buf, offset + i, s->bg_color); } } }