[Qemu-devel] [PATCH 0/2] AHCI: avoid mapping stale guest memory

[Qemu-devel] [PATCH 0/2] AHCI: avoid mapping stale guest memory

[John Snow]

Currently, the AHCI device tries to re-map guest memory every time
the low or high address registers are written to, whether or not the
AHCI device is currently active. If the other register has stale
information in it, this may lead to runtime failures.

Reconfigure the AHCI device to ignore writes to these registers while
the device is active, and otherwise postpone the dma memory map until
the device becomes active.

John Snow (2):
  AHCI: Do not (re)map FB/CLB buffers while not running
  AHCI: Protect cmd register

 hw/ide/ahci.c | 61 ++++++++++++++++++++++++++++++++++++++++++++---------------
 hw/ide/ahci.h |  2 ++
 2 files changed, 48 insertions(+), 15 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/2] AHCI: Do not (re)map FB/CLB buffers while not running

[John Snow]

The FIS Receive Buffer and Command List Buffer pointers
should not be edited while the FIS receive engine or
Command Receive engines are running.

Currently, we attempt to re-map the buffers every time they
are adjusted, but while the AHCI engines are off, these registers
may contain stale values, so we should not attempt to re-map these
values until the engines are reactivated.

Reported-by: Jordan Hargrave <jharg93@gmail.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 5651372..42bbf7f 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -51,6 +51,8 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis);
 static void ahci_init_d2h(AHCIDevice *ad);
 static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write);
 static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes);
+static void ahci_map_clb_address(AHCIDevice *ad);
+static void ahci_map_fis_address(AHCIDevice *ad);
 
 
 static uint32_t  ahci_port_read(AHCIState *s, int port, int offset)
@@ -202,25 +204,15 @@ static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
     switch (offset) {
         case PORT_LST_ADDR:
             pr->lst_addr = val;
-            map_page(s->as, &s->dev[port].lst,
-                     ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
-            s->dev[port].cur_cmd = NULL;
             break;
         case PORT_LST_ADDR_HI:
             pr->lst_addr_hi = val;
-            map_page(s->as, &s->dev[port].lst,
-                     ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
-            s->dev[port].cur_cmd = NULL;
             break;
         case PORT_FIS_ADDR:
             pr->fis_addr = val;
-            map_page(s->as, &s->dev[port].res_fis,
-                     ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
             break;
         case PORT_FIS_ADDR_HI:
             pr->fis_addr_hi = val;
-            map_page(s->as, &s->dev[port].res_fis,
-                     ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
             break;
         case PORT_IRQ_STAT:
             pr->irq_stat &= ~val;
@@ -235,10 +227,12 @@ static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
 
             if (pr->cmd & PORT_CMD_START) {
                 pr->cmd |= PORT_CMD_LIST_ON;
+                ahci_map_clb_address(&s->dev[port]);
             }
 
             if (pr->cmd & PORT_CMD_FIS_RX) {
                 pr->cmd |= PORT_CMD_FIS_ON;
+                ahci_map_fis_address(&s->dev[port]);
             }
 
             /* XXX usually the FIS would be pending on the bus here and
@@ -565,6 +559,21 @@ static void debug_print_fis(uint8_t *fis, int cmd_len)
 #endif
 }
 
+static void ahci_map_fis_address(AHCIDevice *ad)
+{
+    AHCIPortRegs *pr = &ad->port_regs;
+    map_page(ad->hba->as, &ad->res_fis,
+             ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
+}
+
+static void ahci_map_clb_address(AHCIDevice *ad)
+{
+    AHCIPortRegs *pr = &ad->port_regs;
+    ad->cur_cmd = NULL;
+    map_page(ad->hba->as, &ad->lst,
+             ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
+}
+
 static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished)
 {
     AHCIDevice *ad = &s->dev[port];
@@ -1366,10 +1375,8 @@ static int ahci_state_post_load(void *opaque, int version_id)
         ad = &s->dev[i];
         AHCIPortRegs *pr = &ad->port_regs;
 
-        map_page(s->as, &ad->lst,
-                 ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
-        map_page(s->as, &ad->res_fis,
-                 ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
+        ahci_map_clb_address(ad);
+        ahci_map_fis_address(ad);
         /*
          * All pending i/o should be flushed out on a migrate. However,
          * we might not have cleared the busy_slot since this is done
-- 
1.9.3

[Qemu-devel] [PATCH 2/2] AHCI: Protect cmd register

[John Snow]

Many bits in the CMD register are supposed to be strictly read-only.
We should not be deleting them on every write.

As a side-effect: pay explicit attention to when a guest marks off
the FIS Receive or Start bits, and disable the status bits ourselves,
instead of letting them implicitly fall off.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 26 +++++++++++++++++++++++++-
 hw/ide/ahci.h |  2 ++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 42bbf7f..b386a25 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -52,7 +52,9 @@ static void ahci_init_d2h(AHCIDevice *ad);
 static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write);
 static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes);
 static void ahci_map_clb_address(AHCIDevice *ad);
+static void ahci_unmap_clb_address(AHCIDevice *ad);
 static void ahci_map_fis_address(AHCIDevice *ad);
+static void ahci_unmap_fis_address(AHCIDevice *ad);
 
 
 static uint32_t  ahci_port_read(AHCIState *s, int port, int offset)
@@ -223,16 +225,24 @@ static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
             ahci_check_irq(s);
             break;
         case PORT_CMD:
-            pr->cmd = val & ~(PORT_CMD_LIST_ON | PORT_CMD_FIS_ON);
+            /* Block any Read-only fields from being set;
+             * including LIST_ON and FIS_ON. */
+            pr->cmd = (pr->cmd & PORT_CMD_RO_MASK) | (val & ~PORT_CMD_RO_MASK);
 
             if (pr->cmd & PORT_CMD_START) {
                 pr->cmd |= PORT_CMD_LIST_ON;
                 ahci_map_clb_address(&s->dev[port]);
+            } else if (pr->cmd & PORT_CMD_LIST_ON) {
+                ahci_unmap_clb_address(&s->dev[port]);
+                pr->cmd = pr->cmd & ~(PORT_CMD_LIST_ON);
             }
 
             if (pr->cmd & PORT_CMD_FIS_RX) {
                 pr->cmd |= PORT_CMD_FIS_ON;
                 ahci_map_fis_address(&s->dev[port]);
+            } else if (pr->cmd & PORT_CMD_FIS_ON) {
+                ahci_unmap_fis_address(&s->dev[port]);
+                pr->cmd = pr->cmd & ~(PORT_CMD_FIS_ON);
             }
 
             /* XXX usually the FIS would be pending on the bus here and
@@ -566,6 +576,13 @@ static void ahci_map_fis_address(AHCIDevice *ad)
              ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
 }
 
+static void ahci_unmap_fis_address(AHCIDevice *ad)
+{
+    dma_memory_unmap(ad->hba->as, ad->res_fis, 256,
+                     DMA_DIRECTION_FROM_DEVICE, 256);
+    ad->res_fis = NULL;
+}
+
 static void ahci_map_clb_address(AHCIDevice *ad)
 {
     AHCIPortRegs *pr = &ad->port_regs;
@@ -574,6 +591,13 @@ static void ahci_map_clb_address(AHCIDevice *ad)
              ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
 }
 
+static void ahci_unmap_clb_address(AHCIDevice *ad)
+{
+    dma_memory_unmap(ad->hba->as, ad->lst, 1024,
+                     DMA_DIRECTION_FROM_DEVICE, 1024);
+    ad->lst = NULL;
+}
+
 static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished)
 {
     AHCIDevice *ad = &s->dev[port];
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 99aa0c9..501c002 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -132,6 +132,8 @@
 #define PORT_CMD_ICC_PARTIAL      (0x2 << 28) /* Put i/f in partial state */
 #define PORT_CMD_ICC_SLUMBER      (0x6 << 28) /* Put i/f in slumber state */
 
+#define PORT_CMD_RO_MASK          0x007dffe0 /* Which CMD bits are read only? */
+
 /* ap->flags bits */
 #define AHCI_FLAG_NO_NCQ                  (1 << 24)
 #define AHCI_FLAG_IGN_IRQ_IF_ERR          (1 << 25) /* ignore IRQ_IF_ERR */
-- 
1.9.3

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] AHCI: avoid mapping stale guest memory

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]

On Tue, Mar 10, 2015 at 05:29:02PM -0400, John Snow wrote:
> Currently, the AHCI device tries to re-map guest memory every time
> the low or high address registers are written to, whether or not the
> AHCI device is currently active. If the other register has stale
> information in it, this may lead to runtime failures.
> 
> Reconfigure the AHCI device to ignore writes to these registers while
> the device is active, and otherwise postpone the dma memory map until
> the device becomes active.
> 
> John Snow (2):
>   AHCI: Do not (re)map FB/CLB buffers while not running
>   AHCI: Protect cmd register
> 
>  hw/ide/ahci.c | 61 ++++++++++++++++++++++++++++++++++++++++++++---------------
>  hw/ide/ahci.h |  2 ++
>  2 files changed, 48 insertions(+), 15 deletions(-)

hw/ide/ahci.c: In function ‘ahci_state_post_load’:
hw/ide/ahci.c:1396:23: error: unused variable ‘pr’ [-Werror=unused-variable]
         AHCIPortRegs *pr = &ad->port_regs;


What happens if a malicious/buggy guest provides a bogus address?  It
looks like the code still sets the "on" bit in the cmd register because
it doesn't check whether the mapped pointer is non-NULL.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] AHCI: avoid mapping stale guest memory

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1003 bytes --]

On Tue, Mar 10, 2015 at 05:29:02PM -0400, John Snow wrote:
> Currently, the AHCI device tries to re-map guest memory every time
> the low or high address registers are written to, whether or not the
> AHCI device is currently active. If the other register has stale
> information in it, this may lead to runtime failures.
> 
> Reconfigure the AHCI device to ignore writes to these registers while
> the device is active, and otherwise postpone the dma memory map until
> the device becomes active.
> 
> John Snow (2):
>   AHCI: Do not (re)map FB/CLB buffers while not running
>   AHCI: Protect cmd register
> 
>  hw/ide/ahci.c | 61 ++++++++++++++++++++++++++++++++++++++++++++---------------
>  hw/ide/ahci.h |  2 ++
>  2 files changed, 48 insertions(+), 15 deletions(-)

By the way, despite the compiler warning and my comment, I'd like to get
a fix into QEMU 2.3.

I'll be away Friday 13th and Monday 16th of March, so please go ahead
without me.  Kevin could review.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]