[Qemu-devel] [PATCH] usb:xhci: no DMA on HC reset

[Qemu-devel] [PATCH] usb:xhci: no DMA on HC reset

[Roman Kagan]

This patch is a rough fix to a memory corruption we are observing when
running VMs with xhci USB controller and OVMF firmware.

Specifically, on the following call chain

xhci_reset
  xhci_disable_slot
    xhci_disable_ep
      xhci_set_ep_state

QEMU overwrites guest memory using stale guest addresses.

This doesn't happen when the guest (firmware) driver sets up xhci for
the first time as there are no slots configured yet.  However when the
firmware hands over the control to the OS some slots and endpoints are
already set up with their context in the guest RAM.  Now the OS' driver
resets the controller again and xhci_set_ep_state then reads and writes
that memory which is now owned by the OS.

As a quick fix, skip calling xhci_set_ep_state in xhci_disable_ep if the
device context base address array pointer is zero (indicating we're in
the HC reset and no DMA is possible).

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-xhci.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index bcde8a2..43ba615 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1531,7 +1531,10 @@ static TRBCCode xhci_disable_ep(XHCIState *xhci, unsigned int slotid,
         usb_packet_cleanup(&epctx->transfers[i].packet);
     }
 
-    xhci_set_ep_state(xhci, epctx, NULL, EP_DISABLED);
+    /* only touch guest RAM if we're not resetting the HC */
+    if (xhci->dcbaap_low || xhci->dcbaap_high) {
+        xhci_set_ep_state(xhci, epctx, NULL, EP_DISABLED);
+    }
 
     timer_free(epctx->kick_timer);
     g_free(epctx);
-- 
2.5.5

Re: [Qemu-devel] (for 2.6?) [PATCH] usb:xhci: no DMA on HC reset

[Denis V. Lunev]

On 05/04/2016 08:53 PM, Roman Kagan wrote:
> This patch is a rough fix to a memory corruption we are observing when
> running VMs with xhci USB controller and OVMF firmware.
>
> Specifically, on the following call chain
>
> xhci_reset
>    xhci_disable_slot
>      xhci_disable_ep
>        xhci_set_ep_state
>
> QEMU overwrites guest memory using stale guest addresses.
>
> This doesn't happen when the guest (firmware) driver sets up xhci for
> the first time as there are no slots configured yet.  However when the
> firmware hands over the control to the OS some slots and endpoints are
> already set up with their context in the guest RAM.  Now the OS' driver
> resets the controller again and xhci_set_ep_state then reads and writes
> that memory which is now owned by the OS.
>
> As a quick fix, skip calling xhci_set_ep_state in xhci_disable_ep if the
> device context base address array pointer is zero (indicating we're in
> the HC reset and no DMA is possible).
>
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   hw/usb/hcd-xhci.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index bcde8a2..43ba615 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -1531,7 +1531,10 @@ static TRBCCode xhci_disable_ep(XHCIState *xhci, unsigned int slotid,
>           usb_packet_cleanup(&epctx->transfers[i].packet);
>       }
>   
> -    xhci_set_ep_state(xhci, epctx, NULL, EP_DISABLED);
> +    /* only touch guest RAM if we're not resetting the HC */
> +    if (xhci->dcbaap_low || xhci->dcbaap_high) {
> +        xhci_set_ep_state(xhci, epctx, NULL, EP_DISABLED);
> +    }
>   
>       timer_free(epctx->kick_timer);
>       g_free(epctx);
the situation is quite reliable with win2k12 and EFI
guest. Kernel image gets corrupted and the guest
crashes on boot.

The problem is serious and really severe.

Den

Re: [Qemu-devel] [PATCH] usb:xhci: no DMA on HC reset

[Denis V. Lunev]

On 05/04/2016 08:53 PM, Roman Kagan wrote:
> This patch is a rough fix to a memory corruption we are observing when
> running VMs with xhci USB controller and OVMF firmware.
>
> Specifically, on the following call chain
>
> xhci_reset
>    xhci_disable_slot
>      xhci_disable_ep
>        xhci_set_ep_state
>
> QEMU overwrites guest memory using stale guest addresses.
>
> This doesn't happen when the guest (firmware) driver sets up xhci for
> the first time as there are no slots configured yet.  However when the
> firmware hands over the control to the OS some slots and endpoints are
> already set up with their context in the guest RAM.  Now the OS' driver
> resets the controller again and xhci_set_ep_state then reads and writes
> that memory which is now owned by the OS.
>
> As a quick fix, skip calling xhci_set_ep_state in xhci_disable_ep if the
> device context base address array pointer is zero (indicating we're in
> the HC reset and no DMA is possible).
>
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   hw/usb/hcd-xhci.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index bcde8a2..43ba615 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -1531,7 +1531,10 @@ static TRBCCode xhci_disable_ep(XHCIState *xhci, unsigned int slotid,
>           usb_packet_cleanup(&epctx->transfers[i].packet);
>       }
>   
> -    xhci_set_ep_state(xhci, epctx, NULL, EP_DISABLED);
> +    /* only touch guest RAM if we're not resetting the HC */
> +    if (xhci->dcbaap_low || xhci->dcbaap_high) {
> +        xhci_set_ep_state(xhci, epctx, NULL, EP_DISABLED);
> +    }
>   
>       timer_free(epctx->kick_timer);
>       g_free(epctx);
ping

Re: [Qemu-devel] [PATCH] usb:xhci: no DMA on HC reset

[Gerd Hoffmann]

On Mi, 2016-05-04 at 20:53 +0300, Roman Kagan wrote:
> This patch is a rough fix to a memory corruption we are observing when
> running VMs with xhci USB controller and OVMF firmware.

Added to usb patch queue.

thanks,
  Gerd