| Message ID | 1396275242-10810-22-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 31 March 2014 15:17, Michael S. Tsirkin <mst@redhat.com> wrote: > CVE-2013-4538 > > s->cmd_len used as index in ssd0323_transfer() to store 32-bit field. > Possible this field might then be supplied by guest to overwrite a > return addr somewhere. Same for row/col fields, which are indicies into > framebuffer array. > > To fix validate after load. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/display/ssd0323.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/hw/display/ssd0323.c b/hw/display/ssd0323.c > index 971152e..b520c69 100644 > --- a/hw/display/ssd0323.c > +++ b/hw/display/ssd0323.c > @@ -312,13 +312,22 @@ static int ssd0323_load(QEMUFile *f, void *opaque, int version_id) > return -EINVAL; > > s->cmd_len = qemu_get_be32(f); > + if (s->cmd_len < 0 || s->cmd_len > ARRAY_SIZE(s->cmd_data)) { > + return -EINVAL; > + } > s->cmd = qemu_get_be32(f); > for (i = 0; i < 8; i++) > s->cmd_data[i] = qemu_get_be32(f); > s->row = qemu_get_be32(f); > + if (s->row < 0 || s->row >= 80 ) { > + return -EINVAL; > + } > s->row_start = qemu_get_be32(f); > s->row_end = qemu_get_be32(f); > s->col = qemu_get_be32(f); > + if (s->col < 0 || s->col >= 64 ) { > + return -EINVAL; > + } > s->col_start = qemu_get_be32(f); > s->col_end = qemu_get_be32(f); > s->redraw = qemu_get_be32(f); This isn't sufficient. You also need to validate that the row/col_start/end are within bounds; otherwise the guest can provoke an overrun by either setting the _end field so large that the row++ increments just walk off the end of the array, or by setting the _start value to something bogus and then letting the "we hit end of row" logic reset row to row_start. thanks -- PMM
diff --git a/hw/display/ssd0323.c b/hw/display/ssd0323.c index 971152e..b520c69 100644 --- a/hw/display/ssd0323.c +++ b/hw/display/ssd0323.c @@ -312,13 +312,22 @@ static int ssd0323_load(QEMUFile *f, void *opaque, int version_id) return -EINVAL; s->cmd_len = qemu_get_be32(f); + if (s->cmd_len < 0 || s->cmd_len > ARRAY_SIZE(s->cmd_data)) { + return -EINVAL; + } s->cmd = qemu_get_be32(f); for (i = 0; i < 8; i++) s->cmd_data[i] = qemu_get_be32(f); s->row = qemu_get_be32(f); + if (s->row < 0 || s->row >= 80 ) { + return -EINVAL; + } s->row_start = qemu_get_be32(f); s->row_end = qemu_get_be32(f); s->col = qemu_get_be32(f); + if (s->col < 0 || s->col >= 64 ) { + return -EINVAL; + } s->col_start = qemu_get_be32(f); s->col_end = qemu_get_be32(f); s->redraw = qemu_get_be32(f);
CVE-2013-4538 s->cmd_len used as index in ssd0323_transfer() to store 32-bit field. Possible this field might then be supplied by guest to overwrite a return addr somewhere. Same for row/col fields, which are indicies into framebuffer array. To fix validate after load. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/display/ssd0323.c | 9 +++++++++ 1 file changed, 9 insertions(+)