qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/4] ahci: unmap fixes
@ 2016-01-29 21:41 John Snow
  2016-01-29 21:41 ` [Qemu-devel] [PATCH 1/4] ahci: Do not unmap NULL addresses John Snow
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: John Snow @ 2016-01-29 21:41 UTC (permalink / raw)
  To: qemu-block
  Cc: peter.maydell, pjp, qemu-devel, zuozhi.fzz, pbonzini, 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH 1/4] ahci: Do not unmap NULL addresses
  2016-01-29 21:41 [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
@ 2016-01-29 21:41 ` John Snow
  2016-01-29 21:41 ` [Qemu-devel] [PATCH 2/4] ahci: handle LIST_ON and FIS_ON in map helpers John Snow
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: John Snow @ 2016-01-29 21:41 UTC (permalink / raw)
  To: qemu-block
  Cc: peter.maydell, pjp, qemu-devel, zuozhi.fzz, pbonzini, 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH 2/4] ahci: handle LIST_ON and FIS_ON in map helpers
  2016-01-29 21:41 [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
  2016-01-29 21:41 ` [Qemu-devel] [PATCH 1/4] ahci: Do not unmap NULL addresses John Snow
@ 2016-01-29 21:41 ` John Snow
  2016-01-29 21:41 ` [Qemu-devel] [PATCH 3/4] ahci: explicitly reject bad engine states on post_load John Snow
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: John Snow @ 2016-01-29 21:41 UTC (permalink / raw)
  To: qemu-block
  Cc: peter.maydell, pjp, qemu-devel, zuozhi.fzz, pbonzini, 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH 3/4] ahci: explicitly reject bad engine states on post_load
  2016-01-29 21:41 [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
  2016-01-29 21:41 ` [Qemu-devel] [PATCH 1/4] ahci: Do not unmap NULL addresses John Snow
  2016-01-29 21:41 ` [Qemu-devel] [PATCH 2/4] ahci: handle LIST_ON and FIS_ON in map helpers John Snow
@ 2016-01-29 21:41 ` John Snow
  2016-01-29 21:41 ` [Qemu-devel] [PATCH 4/4] ahci: prohibit "restarting" the FIS or CLB engines John Snow
  2016-02-08 16:53 ` [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
  4 siblings, 0 replies; 7+ messages in thread
From: John Snow @ 2016-01-29 21:41 UTC (permalink / raw)
  To: qemu-block
  Cc: peter.maydell, pjp, qemu-devel, zuozhi.fzz, pbonzini, 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH 4/4] ahci: prohibit "restarting" the FIS or CLB engines
  2016-01-29 21:41 [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
                   ` (2 preceding siblings ...)
  2016-01-29 21:41 ` [Qemu-devel] [PATCH 3/4] ahci: explicitly reject bad engine states on post_load John Snow
@ 2016-01-29 21:41 ` John Snow
  2016-02-08 16:53 ` [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
  4 siblings, 0 replies; 7+ messages in thread
From: John Snow @ 2016-01-29 21:41 UTC (permalink / raw)
  To: qemu-block
  Cc: peter.maydell, pjp, qemu-devel, zuozhi.fzz, pbonzini, 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH 0/4] ahci: unmap fixes
  2016-01-29 21:41 [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
                   ` (3 preceding siblings ...)
  2016-01-29 21:41 ` [Qemu-devel] [PATCH 4/4] ahci: prohibit "restarting" the FIS or CLB engines John Snow
@ 2016-02-08 16:53 ` John Snow
  2016-02-09 17:38   ` P J P
  4 siblings, 1 reply; 7+ messages in thread
From: John Snow @ 2016-02-08 16:53 UTC (permalink / raw)
  To: qemu-block; +Cc: peter.maydell, zuozhi.fzz, pbonzini, pjp, qemu-devel

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(-)
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH 0/4] ahci: unmap fixes
  2016-02-08 16:53 ` [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
@ 2016-02-09 17:38   ` P J P
  0 siblings, 0 replies; 7+ messages in thread
From: P J P @ 2016-02-09 17:38 UTC (permalink / raw)
  To: John Snow, qemu-block; +Cc: peter.maydell, pbonzini, qemu-devel, zuozhi.fzz

> 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-02-09 17:38 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-29 21:41 [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
2016-01-29 21:41 ` [Qemu-devel] [PATCH 1/4] ahci: Do not unmap NULL addresses John Snow
2016-01-29 21:41 ` [Qemu-devel] [PATCH 2/4] ahci: handle LIST_ON and FIS_ON in map helpers John Snow
2016-01-29 21:41 ` [Qemu-devel] [PATCH 3/4] ahci: explicitly reject bad engine states on post_load John Snow
2016-01-29 21:41 ` [Qemu-devel] [PATCH 4/4] ahci: prohibit "restarting" the FIS or CLB engines John Snow
2016-02-08 16:53 ` [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
2016-02-09 17:38   ` P J P

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).