From 510952d4c33ee69574167ce30829b21c815a165b Mon Sep 17 00:00:00 2001

From: Kevin Wolf <kwolf@redhat.com>

Date: Wed, 3 Jun 2015 14:13:31 +0200

Subject: [PATCH 1/2] 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 <kwolf@redhat.com>

---

 hw/ide.c | 16 ++++++++++++++++

 1 file changed, 16 insertions(+)

diff --git a/hw/ide.c b/hw/ide.c

index 791666b..211ec88 100644

--- a/hw/ide.c

+++ b/hw/ide.c

@@ -3002,6 +3002,10 @@ static void ide_data_writew(void *opaque, uint32_t addr, uint32_t val)

     buffered_pio_write(s, addr, 2);

     p = s->data_ptr;

+    if (p + 2 > s->data_end) {

+        return;

+    }

+

     *(uint16_t *)p = le16_to_cpu(val);

     p += 2;

     s->data_ptr = p;

@@ -3021,6 +3025,10 @@ static uint32_t ide_data_readw(void *opaque, uint32_t addr)

     buffered_pio_read(s, addr, 2);

     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;

@@ -3040,6 +3048,10 @@ static void ide_data_writel(void *opaque, uint32_t addr, uint32_t val)

     buffered_pio_write(s, addr, 4);

     p = s->data_ptr;

+    if (p + 4 > s->data_end) {

+        return;

+    }

+

     *(uint32_t *)p = le32_to_cpu(val);

     p += 4;

     s->data_ptr = p;

@@ -3059,6 +3071,10 @@ static uint32_t ide_data_readl(void *opaque, uint32_t addr)

     buffered_pio_read(s, addr, 4);

     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;

-- 

2.1.4

From 1ac0f60d558b7fca55c69a61ab4c4538af1f02f9 Mon Sep 17 00:00:00 2001

From: Kevin Wolf <kwolf@redhat.com>

Date: Wed, 3 Jun 2015 14:41:27 +0200

Subject: [PATCH 2/2] 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 <kwolf@redhat.com>

---

 hw/ide.c | 16 ++++++++++++----

 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/ide.c b/hw/ide.c

index 211ec88..7b84d1b 100644

--- a/hw/ide.c

+++ b/hw/ide.c

@@ -3009,8 +3009,10 @@ static 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);

+    }

 }

 static uint32_t ide_data_readw(void *opaque, uint32_t addr)

@@ -3032,8 +3034,10 @@ static 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;

 }

@@ -3055,8 +3059,10 @@ static 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);

+    }

 }

 static uint32_t ide_data_readl(void *opaque, uint32_t addr)

@@ -3078,8 +3084,10 @@ static 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;

 }

-- 

2.1.4

From a9de14175548c04e0f8be7fae219246509ba46a9 Mon Sep 17 00:00:00 2001

From: Kevin Wolf <kwolf@redhat.com>

Date: Wed, 3 Jun 2015 14:13:31 +0200

Subject: [PATCH 1/3] 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 <kwolf@redhat.com>

---

 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;

-- 

1.8.3.1

From aa851d30acfbb9580098ac1dc82885530cb8b3c1 Mon Sep 17 00:00:00 2001

From: Kevin Wolf <kwolf@redhat.com>

Date: Wed, 3 Jun 2015 14:17:46 +0200

Subject: [PATCH 2/3] 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 <kwolf@redhat.com>

---

 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;

     }

--

1.8.3.1

From 1d3c2268f8708126a34064c2e0c1000b40e6f3e5 Mon Sep 17 00:00:00 2001

From: Kevin Wolf <kwolf@redhat.com>

Date: Wed, 3 Jun 2015 14:41:27 +0200

Subject: [PATCH 3/3] 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 <kwolf@redhat.com>

---

 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;

 }

--

1.8.3.1