From d2ff85854512574e7209f295e87b0835d5b032c6 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Sun, 26 Jul 2015 23:42:53 -0400 Subject: ide: Check array bounds before writing to io_buffer (CVE-2015-5154) If the end_transfer_func of a command is called because enough data has been read or written for the current PIO transfer, and it fails to correctly call the command completion functions, the DRQ bit in the status register and s->end_transfer_func may remain set. This allows the guest to access further bytes in s->io_buffer beyond s->data_end, and eventually overflowing the io_buffer. One case where this currently happens is emulation of the ATAPI command START STOP UNIT. This patch fixes the problem by adding explicit array bounds checks before accessing the buffer instead of relying on end_transfer_func to function correctly. Cc: qemu-stable@nongnu.org Signed-off-by: Kevin Wolf Reviewed-by: John Snow --- hw/ide/core.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/hw/ide/core.c b/hw/ide/core.c index 122e955..44fcc23 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -2021,6 +2021,10 @@ void ide_data_writew(void *opaque, uint32_t addr, uint32_t val) } p = s->data_ptr; + if (p + 2 > s->data_end) { + return; + } + *(uint16_t *)p = le16_to_cpu(val); p += 2; s->data_ptr = p; @@ -2042,6 +2046,10 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr) } p = s->data_ptr; + if (p + 2 > s->data_end) { + return 0; + } + ret = cpu_to_le16(*(uint16_t *)p); p += 2; s->data_ptr = p; @@ -2063,6 +2071,10 @@ void ide_data_writel(void *opaque, uint32_t addr, uint32_t val) } p = s->data_ptr; + if (p + 4 > s->data_end) { + return; + } + *(uint32_t *)p = le32_to_cpu(val); p += 4; s->data_ptr = p; @@ -2084,6 +2096,10 @@ uint32_t ide_data_readl(void *opaque, uint32_t addr) } p = s->data_ptr; + if (p + 4 > s->data_end) { + return 0; + } + ret = cpu_to_le32(*(uint32_t *)p); p += 4; s->data_ptr = p; -- cgit v1.1 From 03441c3a4a42beb25460dd11592539030337d0f8 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Sun, 26 Jul 2015 23:42:53 -0400 Subject: ide/atapi: Fix START STOP UNIT command completion The command must be completed on all code paths. START STOP UNIT with pwrcnd set should succeed without doing anything. Signed-off-by: Kevin Wolf Reviewed-by: John Snow --- hw/ide/atapi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c index 950e311..79dd167 100644 --- a/hw/ide/atapi.c +++ b/hw/ide/atapi.c @@ -983,6 +983,7 @@ static void cmd_start_stop_unit(IDEState *s, uint8_t* buf) if (pwrcnd) { /* eject/load only happens for power condition == 0 */ + ide_atapi_cmd_ok(s); return; } -- cgit v1.1 From cb72cba83021fa42719e73a5249c12096a4d1cfc Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Sun, 26 Jul 2015 23:42:53 -0400 Subject: ide: Clear DRQ after handling all expected accesses This is additional hardening against an end_transfer_func that fails to clear the DRQ status bit. The bit must be unset as soon as the PIO transfer has completed, so it's better to do this in a central place instead of duplicating the code in all commands (and forgetting it in some). Signed-off-by: Kevin Wolf Reviewed-by: John Snow --- hw/ide/core.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/hw/ide/core.c b/hw/ide/core.c index 44fcc23..50449ca 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -2028,8 +2028,10 @@ void ide_data_writew(void *opaque, uint32_t addr, uint32_t val) *(uint16_t *)p = le16_to_cpu(val); p += 2; s->data_ptr = p; - if (p >= s->data_end) + if (p >= s->data_end) { + s->status &= ~DRQ_STAT; s->end_transfer_func(s); + } } uint32_t ide_data_readw(void *opaque, uint32_t addr) @@ -2053,8 +2055,10 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr) ret = cpu_to_le16(*(uint16_t *)p); p += 2; s->data_ptr = p; - if (p >= s->data_end) + if (p >= s->data_end) { + s->status &= ~DRQ_STAT; s->end_transfer_func(s); + } return ret; } @@ -2078,8 +2082,10 @@ void ide_data_writel(void *opaque, uint32_t addr, uint32_t val) *(uint32_t *)p = le32_to_cpu(val); p += 4; s->data_ptr = p; - if (p >= s->data_end) + if (p >= s->data_end) { + s->status &= ~DRQ_STAT; s->end_transfer_func(s); + } } uint32_t ide_data_readl(void *opaque, uint32_t addr) @@ -2103,8 +2109,10 @@ uint32_t ide_data_readl(void *opaque, uint32_t addr) ret = cpu_to_le32(*(uint32_t *)p); p += 4; s->data_ptr = p; - if (p >= s->data_end) + if (p >= s->data_end) { + s->status &= ~DRQ_STAT; s->end_transfer_func(s); + } return ret; } -- cgit v1.1