| Message ID | 20160418100735.GA517@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
CCing the maintainers for this device... On 18 April 2016 at 11:07, Michael S. Tsirkin <mst@redhat.com> wrote: > cadence_uart_init() initializes an I/O memory region of size 0x1000 > bytes. However in uart_write(), the 'offset' parameter (offset within > region) is divided by 4 and then used to index the array 'r' of size > CADENCE_UART_R_MAX which is much smaller: (0x48/4). If 'offset>>=2' > exceeds CADENCE_UART_R_MAX, this will cause an out-of-bounds memory > write where the offset and the value are controlled by guest. > > This will corrupt QEMU memory, in most situations this causes the vm to > crash. > > Fix by checking the offset against the array size. > > Reported-by: ?? <liqiang6-s@360.cn> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c > index 486591b..7977878 100644 > --- a/hw/char/cadence_uart.c > +++ b/hw/char/cadence_uart.c > @@ -375,6 +375,9 @@ static void uart_write(void *opaque, hwaddr offset, > > DB_PRINT(" offset:%x data:%08x\n", (unsigned)offset, (unsigned)value); > offset >>= 2; > + if (offset >= CADENCE_UART_R_MAX) { > + return; > + } > switch (offset) { > case R_IER: /* ier (wts imr) */ > s->r[R_IMR] |= value; thanks -- PMM
On Mon, Apr 18, 2016 at 3:10 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > CCing the maintainers for this device... > > On 18 April 2016 at 11:07, Michael S. Tsirkin <mst@redhat.com> wrote: >> cadence_uart_init() initializes an I/O memory region of size 0x1000 >> bytes. However in uart_write(), the 'offset' parameter (offset within >> region) is divided by 4 and then used to index the array 'r' of size >> CADENCE_UART_R_MAX which is much smaller: (0x48/4). If 'offset>>=2' >> exceeds CADENCE_UART_R_MAX, this will cause an out-of-bounds memory >> write where the offset and the value are controlled by guest. >> >> This will corrupt QEMU memory, in most situations this causes the vm to >> crash. >> >> Fix by checking the offset against the array size. >> >> Reported-by: ?? <liqiang6-s@360.cn> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Good catch. Reviewed-by: Alistair Francis <alistair.francis@xilinx.com> Thanks, Alistair >> >> --- >> >> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c >> index 486591b..7977878 100644 >> --- a/hw/char/cadence_uart.c >> +++ b/hw/char/cadence_uart.c >> @@ -375,6 +375,9 @@ static void uart_write(void *opaque, hwaddr offset, >> >> DB_PRINT(" offset:%x data:%08x\n", (unsigned)offset, (unsigned)value); >> offset >>= 2; >> + if (offset >= CADENCE_UART_R_MAX) { >> + return; >> + } >> switch (offset) { >> case R_IER: /* ier (wts imr) */ >> s->r[R_IMR] |= value; > > thanks > -- PMM >
On 18 April 2016 at 21:50, Alistair Francis <alistair.francis@xilinx.com> wrote: > On Mon, Apr 18, 2016 at 3:10 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> CCing the maintainers for this device... >> >> On 18 April 2016 at 11:07, Michael S. Tsirkin <mst@redhat.com> wrote: >>> cadence_uart_init() initializes an I/O memory region of size 0x1000 >>> bytes. However in uart_write(), the 'offset' parameter (offset within >>> region) is divided by 4 and then used to index the array 'r' of size >>> CADENCE_UART_R_MAX which is much smaller: (0x48/4). If 'offset>>=2' >>> exceeds CADENCE_UART_R_MAX, this will cause an out-of-bounds memory >>> write where the offset and the value are controlled by guest. >>> >>> This will corrupt QEMU memory, in most situations this causes the vm to >>> crash. >>> >>> Fix by checking the offset against the array size. >>> >>> Reported-by: ?? <liqiang6-s@360.cn> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Good catch. > > Reviewed-by: Alistair Francis <alistair.francis@xilinx.com> Applied to master, thanks (so this will be in rc3). I added a Cc: qemu-stable@nongnu.org tag too. -- PMM
diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c index 486591b..7977878 100644 --- a/hw/char/cadence_uart.c +++ b/hw/char/cadence_uart.c @@ -375,6 +375,9 @@ static void uart_write(void *opaque, hwaddr offset, DB_PRINT(" offset:%x data:%08x\n", (unsigned)offset, (unsigned)value); offset >>= 2; + if (offset >= CADENCE_UART_R_MAX) { + return; + } switch (offset) { case R_IER: /* ier (wts imr) */ s->r[R_IMR] |= value;
cadence_uart_init() initializes an I/O memory region of size 0x1000 bytes. However in uart_write(), the 'offset' parameter (offset within region) is divided by 4 and then used to index the array 'r' of size CADENCE_UART_R_MAX which is much smaller: (0x48/4). If 'offset>>=2' exceeds CADENCE_UART_R_MAX, this will cause an out-of-bounds memory write where the offset and the value are controlled by guest. This will corrupt QEMU memory, in most situations this causes the vm to crash. Fix by checking the offset against the array size. Reported-by: ?? <liqiang6-s@360.cn> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> ---