[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PULL v5 09/12] hw/display/artist: Prevent out of VRAM buffer accesses
From: |
Helge Deller |
Subject: |
[PULL v5 09/12] hw/display/artist: Prevent out of VRAM buffer accesses |
Date: |
Wed, 26 Aug 2020 23:13:42 +0200 |
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>
---
hw/display/artist.c | 110 +++++++++++++++++++++++++++-----------------
1 file changed, 69 insertions(+), 41 deletions(-)
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);
--
2.21.3
- [PULL v5 00/12] The following changes since commit 3461487523b897d324e8d91f3fd20ed55f849544:, Helge Deller, 2020/08/26
- [PULL v5 01/12] hw/hppa: Sync hppa_hardware.h file with SeaBIOS sources, Helge Deller, 2020/08/26
- [PULL v5 05/12] hw/hppa/lasi: Don't abort on invalid IMR value, Helge Deller, 2020/08/26
- [PULL v5 08/12] Revert "hw/display/artist: Avoid drawing line when nothing to display", Helge Deller, 2020/08/26
- [PULL v5 07/12] hw/display/artist: Refactor artist_rop8() to avoid buffer over-run, Helge Deller, 2020/08/26
- [PULL v5 09/12] hw/display/artist: Prevent out of VRAM buffer accesses,
Helge Deller <=
- [PULL v5 10/12] hw/display/artist: Unbreak size mismatch memory accesses, Helge Deller, 2020/08/26
- [PULL v5 11/12] hw/display/artist: Fix invalidation of lines in artist_draw_line(), Helge Deller, 2020/08/26
- [PULL v5 12/12] hw/display/artist: Fix invalidation of lines near screen border, Helge Deller, 2020/08/26
- [PULL v5 03/12] hw/hppa: Implement proper SeaBIOS version check, Helge Deller, 2020/08/26
- [PULL v5 06/12] hw/display/artist: Check offset in draw_line to avoid buffer over-run, Helge Deller, 2020/08/26
- [PULL v5 02/12] seabios-hppa: Update to SeaBIOS hppa version 1, Helge Deller, 2020/08/26
- [PULL v5 04/12] hw/display/artist.c: fix out of bounds check, Helge Deller, 2020/08/26
- Re: [PULL v5 00/12] The following changes since commit 3461487523b897d324e8d91f3fd20ed55f849544:, Peter Maydell, 2020/08/27