[Qemu-devel] [PATCH] cadence_uart: bounds check write offset

[Qemu-devel] [PATCH] cadence_uart: bounds check write offset

[Michael S. Tsirkin]

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;

Re: [Qemu-devel] [PATCH] cadence_uart: bounds check write offset

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] cadence_uart: bounds check write offset

[Alistair Francis]

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
>

Re: [Qemu-devel] [PATCH] cadence_uart: bounds check write offset

[Peter Maydell]

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

[Qemu-devel] [engineering.redhat.com #398672] [QEMU-SECURITY] Out-of-bands write in uart_write()

[Red Hat Product Security]

On Sun Apr 24 22:05:43 2016, liqiang6-s@360.cn wrote:
> Hi, Is there any progress in requesting CVE for this issue ?
>
> -----邮件原件-----
> 发件人: Michael S. Tsirkin [mailto:mst@redhat.com]
> 发送时间: 2016年4月18日 17:29
> 收件人: 李强
> 抄送: Peter Maydell; pmatouse@redhat.com; sstabellini@kernel.org;
> secalert@redhat.com; mdroth@linux.vnet.ibm.com
> 主题: Re: 答复: [QEMU-SECURITY] Out-of-bands write in uart_write()
>
> On Mon, Apr 18, 2016 at 09:26:22AM +0000, 李强 wrote:
> > Sure, it is not related to KVM. But I think it is a security issue
> of qemu, can this issue be assigned a CVE number ?
>
> We'll get back to you on this.
> Meanwhile, could you pls confirm you are okay with posting the patch
> on the public mailing list?
>

Michael,

have you already requested CVE or can we assign it internally? I just want to
make sure we avoid conflicts.

Thanks!

Regards,

--
Adam Mariš / Red Hat Product Security