Skip to contentSkip to conversation

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

Michael S. Tsirkin

author
#2 lore raw

PATCH 1/1 cadence_uart: bounds check write offset

In reply to a message not in the archive

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;
#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

Reviewed-by
#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

Applied
#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

#12 lore raw

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

In reply to a message not in the archive
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