[Qemu-devel] [PATCH 0/4] ahci: unmap fixes

[Qemu-devel] [PATCH 0/4] ahci: unmap fixes

[John Snow]

As reported by Zuozhi fzz <zuozhi.fzz@alibaba-inc.com>, there's a problem
you can expose in AHCI by rewriting the command list buffer and/or FIS
receive buffer addresses, then re-starting the AHCI device before bringing
it to a stop. Depending on the success of the remap operations, you may
be able to transition the device to a state where it thinks it is "running"
but no longer has a guest memory mapping.

When you try to transition it to the stopped state, QEMU crashes.

Tighten up the start/stop conditions, and pepper in a paranoia check inside
of the unmap function.

________________________________________________________________________________

For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch ahci-unmap-fixes
https://github.com/jnsnow/qemu/tree/ahci-unmap-fixes

This version is tagged ahci-unmap-fixes-v1:
https://github.com/jnsnow/qemu/releases/tag/ahci-unmap-fixes-v1

John Snow (4):
  ahci: Do not unmap NULL addresses
  ahci: handle LIST_ON and FIS_ON in map helpers
  ahci: explicitly reject bad engine states on post_load
  ahci: prohibit "restarting" the FIS or CLB engines

 hw/ide/ahci.c | 96 ++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 59 insertions(+), 37 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/4] ahci: Do not unmap NULL addresses

[John Snow]

Definitely don't try to unmap a garbage address.

Reported-by: Zuozhi fzz <zuozhi.fzz@alibaba-inc.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 17f1cbd..cdc9299 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -661,6 +661,10 @@ static bool ahci_map_fis_address(AHCIDevice *ad)
 
 static void ahci_unmap_fis_address(AHCIDevice *ad)
 {
+    if (ad->res_fis == NULL) {
+        DPRINTF(ad->port_no, "Attempt to unmap NULL FIS address\n");
+        return;
+    }
     dma_memory_unmap(ad->hba->as, ad->res_fis, 256,
                      DMA_DIRECTION_FROM_DEVICE, 256);
     ad->res_fis = NULL;
@@ -677,6 +681,10 @@ static bool ahci_map_clb_address(AHCIDevice *ad)
 
 static void ahci_unmap_clb_address(AHCIDevice *ad)
 {
+    if (ad->lst == NULL) {
+        DPRINTF(ad->port_no, "Attempt to unmap NULL CLB address\n");
+        return;
+    }
     dma_memory_unmap(ad->hba->as, ad->lst, 1024,
                      DMA_DIRECTION_FROM_DEVICE, 1024);
     ad->lst = NULL;
-- 
2.4.3

[Qemu-devel] [PATCH 2/4] ahci: handle LIST_ON and FIS_ON in map helpers

[John Snow]

Instead of relying on ahci_cond_start_engines to maintain the
engine status indicators itself, have the lower-layer CLB and FIS mapper
helpers do it themselves.

This makes the cond_start routine slightly nicer to read, and makes sure
that the status indicators will always be correct.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index cdc9299..0653396 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -209,9 +209,7 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
     AHCIPortRegs *pr = &ad->port_regs;
 
     if (pr->cmd & PORT_CMD_START) {
-        if (ahci_map_clb_address(ad)) {
-            pr->cmd |= PORT_CMD_LIST_ON;
-        } else {
+        if (!ahci_map_clb_address(ad)) {
             error_report("AHCI: Failed to start DMA engine: "
                          "bad command list buffer address");
             return -1;
@@ -219,7 +217,6 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
     } else if (pr->cmd & PORT_CMD_LIST_ON) {
         if (allow_stop) {
             ahci_unmap_clb_address(ad);
-            pr->cmd = pr->cmd & ~(PORT_CMD_LIST_ON);
         } else {
             error_report("AHCI: DMA engine should be off, "
                          "but appears to still be running");
@@ -228,9 +225,7 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
     }
 
     if (pr->cmd & PORT_CMD_FIS_RX) {
-        if (ahci_map_fis_address(ad)) {
-            pr->cmd |= PORT_CMD_FIS_ON;
-        } else {
+        if (!ahci_map_fis_address(ad)) {
             error_report("AHCI: Failed to start FIS receive engine: "
                          "bad FIS receive buffer address");
             return -1;
@@ -238,7 +233,6 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
     } else if (pr->cmd & PORT_CMD_FIS_ON) {
         if (allow_stop) {
             ahci_unmap_fis_address(ad);
-            pr->cmd = pr->cmd & ~(PORT_CMD_FIS_ON);
         } else {
             error_report("AHCI: FIS receive engine should be off, "
                          "but appears to still be running");
@@ -656,7 +650,13 @@ static bool 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);
-    return ad->res_fis != NULL;
+    if (ad->res_fis != NULL) {
+        pr->cmd |= PORT_CMD_FIS_ON;
+        return true;
+    }
+
+    pr->cmd &= ~PORT_CMD_FIS_ON;
+    return false;
 }
 
 static void ahci_unmap_fis_address(AHCIDevice *ad)
@@ -665,6 +665,7 @@ static void ahci_unmap_fis_address(AHCIDevice *ad)
         DPRINTF(ad->port_no, "Attempt to unmap NULL FIS address\n");
         return;
     }
+    ad->port_regs.cmd &= ~PORT_CMD_FIS_ON;
     dma_memory_unmap(ad->hba->as, ad->res_fis, 256,
                      DMA_DIRECTION_FROM_DEVICE, 256);
     ad->res_fis = NULL;
@@ -676,7 +677,13 @@ static bool ahci_map_clb_address(AHCIDevice *ad)
     ad->cur_cmd = NULL;
     map_page(ad->hba->as, &ad->lst,
              ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
-    return ad->lst != NULL;
+    if (ad->lst != NULL) {
+        pr->cmd |= PORT_CMD_LIST_ON;
+        return true;
+    }
+
+    pr->cmd &= ~PORT_CMD_LIST_ON;
+    return false;
 }
 
 static void ahci_unmap_clb_address(AHCIDevice *ad)
@@ -685,6 +692,7 @@ static void ahci_unmap_clb_address(AHCIDevice *ad)
         DPRINTF(ad->port_no, "Attempt to unmap NULL CLB address\n");
         return;
     }
+    ad->port_regs.cmd &= ~PORT_CMD_LIST_ON;
     dma_memory_unmap(ad->hba->as, ad->lst, 1024,
                      DMA_DIRECTION_FROM_DEVICE, 1024);
     ad->lst = NULL;
-- 
2.4.3

[Qemu-devel] [PATCH 3/4] ahci: explicitly reject bad engine states on post_load

[John Snow]

Currently, we let ahci_cond_start_engines reject weird configurations
where either the DMA (CLB) or FIS engines are said to be started, but
their matching on/off control bit is toggled off.

There should be no way to achieve this, since any time you toggle the
control bit off, the status bit should always follow synchronously.

Preparing for a refactor in cond_start_engines, move the rejection logic
straight up into post_load.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 0653396..e856ad5 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -217,10 +217,6 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
     } else if (pr->cmd & PORT_CMD_LIST_ON) {
         if (allow_stop) {
             ahci_unmap_clb_address(ad);
-        } else {
-            error_report("AHCI: DMA engine should be off, "
-                         "but appears to still be running");
-            return -1;
         }
     }
 
@@ -233,10 +229,6 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
     } else if (pr->cmd & PORT_CMD_FIS_ON) {
         if (allow_stop) {
             ahci_unmap_fis_address(ad);
-        } else {
-            error_report("AHCI: FIS receive engine should be off, "
-                         "but appears to still be running");
-            return -1;
         }
     }
 
@@ -1567,10 +1559,23 @@ static int ahci_state_post_load(void *opaque, int version_id)
     int i, j;
     struct AHCIDevice *ad;
     NCQTransferState *ncq_tfs;
+    AHCIPortRegs *pr;
     AHCIState *s = opaque;
 
     for (i = 0; i < s->ports; i++) {
         ad = &s->dev[i];
+        pr = &ad->port_regs;
+
+        if (!(pr->cmd & PORT_CMD_START) && (pr->cmd & PORT_CMD_LIST_ON)) {
+            error_report("AHCI: DMA engine should be off, but status bit "
+                         "indicates it is still running.");
+            return -1;
+        }
+        if (!(pr->cmd & PORT_CMD_FIS_RX) && (pr->cmd & PORT_CMD_FIS_ON)) {
+            error_report("AHCI: FIS RX engine should be off, but status bit "
+                         "indicates it is still running.");
+            return -1;
+        }
 
         /* Only remap the CLB address if appropriate, disallowing a state
          * transition from 'on' to 'off' it should be consistent here. */
-- 
2.4.3

[Qemu-devel] [PATCH 4/4] ahci: prohibit "restarting" the FIS or CLB engines

[John Snow]

If the FIS or DMA engines are already started, do not allow them to be
"restarted." As a side-effect of this change, the migration post-load
routine must be modified to cope. If the engines are listed as "on"
in the migrated registers, they must be cleared to allow the startup
routine to see the transition from "off" to "on".

As a second side-effect, the extra argument to ahci_cond_engine_start
is removed in favor of consistent behavior.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index e856ad5..f0b8263 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -198,38 +198,38 @@ static void map_page(AddressSpace *as, uint8_t **ptr, uint64_t addr,
  * Check the cmd register to see if we should start or stop
  * the DMA or FIS RX engines.
  *
- * @ad: Device to engage.
- * @allow_stop: Allow device to transition from started to stopped?
- *   'no' is useful for migration post_load, which does not expect a transition.
+ * @ad: Device to dis/engage.
  *
  * @return 0 on success, -1 on error.
  */
-static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
+static int ahci_cond_start_engines(AHCIDevice *ad)
 {
     AHCIPortRegs *pr = &ad->port_regs;
+    bool cmd_start = pr->cmd & PORT_CMD_START;
+    bool cmd_on    = pr->cmd & PORT_CMD_LIST_ON;
+    bool fis_start = pr->cmd & PORT_CMD_FIS_RX;
+    bool fis_on    = pr->cmd & PORT_CMD_FIS_ON;
 
-    if (pr->cmd & PORT_CMD_START) {
+    if (cmd_start && !cmd_on) {
         if (!ahci_map_clb_address(ad)) {
+            pr->cmd &= ~PORT_CMD_START;
             error_report("AHCI: Failed to start DMA engine: "
                          "bad command list buffer address");
             return -1;
         }
-    } else if (pr->cmd & PORT_CMD_LIST_ON) {
-        if (allow_stop) {
-            ahci_unmap_clb_address(ad);
-        }
+    } else if (!cmd_start && cmd_on) {
+        ahci_unmap_clb_address(ad);
     }
 
-    if (pr->cmd & PORT_CMD_FIS_RX) {
+    if (fis_start && !fis_on) {
         if (!ahci_map_fis_address(ad)) {
+            pr->cmd &= ~PORT_CMD_FIS_RX;
             error_report("AHCI: Failed to start FIS receive engine: "
                          "bad FIS receive buffer address");
             return -1;
         }
-    } else if (pr->cmd & PORT_CMD_FIS_ON) {
-        if (allow_stop) {
-            ahci_unmap_fis_address(ad);
-        }
+    } else if (!fis_start && fis_on) {
+        ahci_unmap_fis_address(ad);
     }
 
     return 0;
@@ -271,8 +271,8 @@ static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
             pr->cmd = (pr->cmd & PORT_CMD_RO_MASK) |
                       (val & ~(PORT_CMD_RO_MASK|PORT_CMD_ICC_MASK));
 
-            /* Check FIS RX and CLB engines, allow transition to false: */
-            ahci_cond_start_engines(&s->dev[port], true);
+            /* Check FIS RX and CLB engines */
+            ahci_cond_start_engines(&s->dev[port]);
 
             /* XXX usually the FIS would be pending on the bus here and
                    issuing deferred until the OS enables FIS receival.
@@ -1577,9 +1577,10 @@ static int ahci_state_post_load(void *opaque, int version_id)
             return -1;
         }
 
-        /* Only remap the CLB address if appropriate, disallowing a state
-         * transition from 'on' to 'off' it should be consistent here. */
-        if (ahci_cond_start_engines(ad, false) != 0) {
+        /* After a migrate, the DMA/FIS engines are "off" and
+         * need to be conditionally restarted */
+        pr->cmd &= ~(PORT_CMD_LIST_ON | PORT_CMD_FIS_ON);
+        if (ahci_cond_start_engines(ad) != 0) {
             return -1;
         }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH 0/4] ahci: unmap fixes

[John Snow]

PJP, ping? Look good?

On 01/29/2016 04:41 PM, John Snow wrote:
> As reported by Zuozhi fzz <zuozhi.fzz@alibaba-inc.com>, there's a problem
> you can expose in AHCI by rewriting the command list buffer and/or FIS
> receive buffer addresses, then re-starting the AHCI device before bringing
> it to a stop. Depending on the success of the remap operations, you may
> be able to transition the device to a state where it thinks it is "running"
> but no longer has a guest memory mapping.
> 
> When you try to transition it to the stopped state, QEMU crashes.
> 
> Tighten up the start/stop conditions, and pepper in a paranoia check inside
> of the unmap function.
> 
> ________________________________________________________________________________
> 
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch ahci-unmap-fixes
> https://github.com/jnsnow/qemu/tree/ahci-unmap-fixes
> 
> This version is tagged ahci-unmap-fixes-v1:
> https://github.com/jnsnow/qemu/releases/tag/ahci-unmap-fixes-v1
> 
> John Snow (4):
>   ahci: Do not unmap NULL addresses
>   ahci: handle LIST_ON and FIS_ON in map helpers
>   ahci: explicitly reject bad engine states on post_load
>   ahci: prohibit "restarting" the FIS or CLB engines
> 
>  hw/ide/ahci.c | 96 ++++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 59 insertions(+), 37 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/4] ahci: unmap fixes

[P J P]

> On Monday, 8 February 2016 10:23 PM, John Snow <jsnow@redhat.com> wrote:
>> PJP, ping? Look good?

    Oops, sorry!

> On 01/29/2016 04:41 PM, John Snow wrote:
>> As reported by Zuozhi fzz <zuozhi.fzz@alibaba-inc.com>, there's a problem
>> you can expose in AHCI by rewriting the command list buffer and/or FIS
>> receive buffer addresses, then re-starting the AHCI device before bringing
>> it to a stop. Depending on the success of the remap operations, you may
>> be able to transition the device to a state where it thinks it is
>> "running" but no longer has a guest memory mapping.
>>
>> When you try to transition it to the stopped state, QEMU crashes.
>>
>> Tighten up the start/stop conditions, and pepper in a paranoia check inside
>> of the unmap function.
>>
>> John Snow (4):
>>   ahci: Do not unmap NULL addresses
>>   ahci: handle LIST_ON and FIS_ON in map helpers
>>   ahci: explicitly reject bad engine states on post_load

>> ahci: prohibit "restarting" the FIS or CLB engines

  Yes, they look good.

Thank you.

---  -P J P
http://feedmug.com