cadence_uart: bounds check write offset

PATCH Applied Michael S. Tsirkin 4 participants 5 messages Apr 18, 2016 – Apr 29, 2016 8 messages not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Michael S. Tsirkin

author

2016-04-18 10:07 UTC #2 lore raw

PATCH 1/1 cadence_uart: bounds check write offset

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 李强 <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---

hw/char/cadence_uart.c +3 -0

index 486591b..7977878 100644
@@ -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;

Peter Maydell

2016-04-18 10:10 UTC #3 lore raw

CCing the maintainers for this device...

Quoted 22 lines · 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 李强 <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>
---
hw/char/cadence_uart.c +0 -0
index 486591b..7977878 100644

hw/char/cadence_uart.c static void uart_write(void *opaque, hwaddr offset, · line 383

Quoted 9 lines

hw/char/cadence_uart.c +3 -0

@@ -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

Alistair Francis

Reviewed-by

2016-04-18 20:50 UTC #4 lore raw

Quoted 17 lines · Peter Maydell

CCing the maintainers for this device...
Quoted 14 lines · 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 李强 <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>

Good catch.

Reviewed-by Alistair Francis <[email protected]>

Thanks,

Alistair

Quoted 21 lines

Quoted 8 lines

---
hw/char/cadence_uart.c +0 -0
index 486591b..7977878 100644

hw/char/cadence_uart.c static void uart_write(void *opaque, hwaddr offset, · line 383

Quoted 9 lines

hw/char/cadence_uart.c +3 -0

@@ -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

Peter Maydell

Applied

2016-04-19 10:15 UTC #5 lore raw

Quoted 14 lines · Alistair Francis

Quoted 13 lines · Peter Maydell
CCing the maintainers for this device...
Quoted 10 lines · 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.

Quoted 8 lines

Quoted 4 lines
Quoted 4 lines
Fix by checking the offset against the array size.
Reported-by 李强 <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>
Good catch.
Reviewed-by Alistair Francis <[email protected]>

Applied to master, thanks (so this will be in rc3). I added a Cc: [email protected] tag too.

-- PMM

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): CAFEAcA9zNW-hdTW4wK9vjCcmAjDKAcfQA+DNKra-jS2d=TLxLQ@mail.gmail.com

A message that is not in the archive (sent off-list or not yet received): [email protected]

Red Hat Product Security

2016-04-29 13:09 UTC #12 lore raw

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

Quoted 10 lines · On Sun Apr 24 22:05:43 2016, [email protected] wrote:

Hi, Is there any progress in requesting CVE for this issue ?
-----邮件原件-----
发件人: Michael S. Tsirkin [mailto:[email protected]]
发送时间: 2016年4月18日 17:29
收件人: 李强
抄送: Peter Maydell; [email protected]; [email protected]; [email protected]; [email protected]
主题: Re: 答复: [QEMU-SECURITY] Out-of-bands write in uart_write()

Quoted 8 lines

Quoted 1 line · 李强
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