diff mbox series

[v4,09/11] drivers/char: mark DMA buffers as reserved for the XHCI

Message ID b3d917bd1d46393857bd82bcdef9d4486d8f3945.1660354597.git-series.marmarek@invisiblethingslab.com (mailing list archive)
State Superseded
Headers show
Series Add Xue - console over USB 3 Debug Capability | expand

Commit Message

Marek Marczykowski-Górecki Aug. 13, 2022, 1:38 a.m. UTC 
The important part is to include those buffers in IOMMU page table
relevant for the USB controller. Otherwise, DbC will stop working as
soon as IOMMU is enabled, regardless of to which domain device assigned
(be it xen or dom0).
If the device is passed through to dom0 or other domain (see later
patches), that domain will effectively have access to those buffers too.
It does give such domain yet another way to DoS the system (as is the
case when having PCI device assigned already), but also possibly steal
the console ring content. Thus, such domain should be a trusted one.
In any case, prevent anything else being placed on those pages by adding
artificial padding.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v3:
- adjust for xhci-dbc rename
- do not raise MAX_USER_RMRR_PAGES
- adjust alignment of DMA buffers
---
 xen/drivers/char/xhci-dbc.c | 42 +++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 14 deletions(-)

Comments

Jan Beulich Aug. 17, 2022, 2:41 p.m. UTC | #1 
On 13.08.2022 03:38, Marek Marczykowski-Górecki wrote:
> @@ -1050,13 +1051,20 @@ static struct uart_driver dbc_uart_driver = {
>  };
>  
>  /* Those are accessed via DMA. */
> -static struct xhci_trb evt_trb[DBC_TRB_RING_CAP];
> -static struct xhci_trb out_trb[DBC_TRB_RING_CAP];
> -static struct xhci_trb in_trb[DBC_TRB_RING_CAP];
> -static struct xhci_erst_segment erst __aligned(16);
> -static struct xhci_dbc_ctx ctx __aligned(16);
> -static uint8_t out_wrk_buf[DBC_WORK_RING_CAP];
> -static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
> +struct dbc_dma_bufs {
> +    struct xhci_trb evt_trb[DBC_TRB_RING_CAP];
> +    struct xhci_trb out_trb[DBC_TRB_RING_CAP];
> +    struct xhci_trb in_trb[DBC_TRB_RING_CAP];
> +    uint8_t out_wrk_buf[DBC_WORK_RING_CAP];
> +    struct xhci_erst_segment erst __aligned(16);
> +    struct xhci_dbc_ctx ctx __aligned(16);
> +    struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
> +    /*
> +     * Don't place anything else on this page - it will be
> +     * DMA-reachable by the USB controller.
> +     */
> +};
> +static struct dbc_dma_bufs dbc_dma_bufs __section(".bss.page_aligned");

Don't you also need __aligned(PAGE_SIZE) here? (Strictly speaking
the type itself would need page alignment, but enforcing it just
for the variable is enough since we force suitable "post"-alignment
in the linker script. Provided, of course, that all other
contributions to the section also force the necessary alignment,
but I think we can take this for given - I do recall changing some
items several years ago, and a quick grep looks to confirm it.)

Jan
diff mbox series

Patch

diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index ca7d4a62139e..8d83d8307fcd 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -20,6 +20,7 @@ 
  */
 
 #include <xen/delay.h>
+#include <xen/iommu.h>
 #include <xen/mm.h>
 #include <xen/param.h>
 #include <xen/serial.h>
@@ -1050,13 +1051,20 @@  static struct uart_driver dbc_uart_driver = {
 };
 
 /* Those are accessed via DMA. */
-static struct xhci_trb evt_trb[DBC_TRB_RING_CAP];
-static struct xhci_trb out_trb[DBC_TRB_RING_CAP];
-static struct xhci_trb in_trb[DBC_TRB_RING_CAP];
-static struct xhci_erst_segment erst __aligned(16);
-static struct xhci_dbc_ctx ctx __aligned(16);
-static uint8_t out_wrk_buf[DBC_WORK_RING_CAP];
-static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
+struct dbc_dma_bufs {
+    struct xhci_trb evt_trb[DBC_TRB_RING_CAP];
+    struct xhci_trb out_trb[DBC_TRB_RING_CAP];
+    struct xhci_trb in_trb[DBC_TRB_RING_CAP];
+    uint8_t out_wrk_buf[DBC_WORK_RING_CAP];
+    struct xhci_erst_segment erst __aligned(16);
+    struct xhci_dbc_ctx ctx __aligned(16);
+    struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
+    /*
+     * Don't place anything else on this page - it will be
+     * DMA-reachable by the USB controller.
+     */
+};
+static struct dbc_dma_bufs dbc_dma_bufs __section(".bss.page_aligned");
 
 static char __initdata opt_dbgp[30];
 
@@ -1093,16 +1101,22 @@  void __init xhci_dbc_uart_init(void)
         dbc->sbdf = PCI_SBDF(0, bus, slot, func);
     }
 
-    dbc->dbc_ctx = &ctx;
-    dbc->dbc_erst = &erst;
-    dbc->dbc_ering.trb = evt_trb;
-    dbc->dbc_oring.trb = out_trb;
-    dbc->dbc_iring.trb = in_trb;
-    dbc->dbc_owork.buf = out_wrk_buf;
-    dbc->dbc_str = str_buf;
+    dbc->dbc_ctx = &dbc_dma_bufs.ctx;
+    dbc->dbc_erst = &dbc_dma_bufs.erst;
+    dbc->dbc_ering.trb = dbc_dma_bufs.evt_trb;
+    dbc->dbc_oring.trb = dbc_dma_bufs.out_trb;
+    dbc->dbc_iring.trb = dbc_dma_bufs.in_trb;
+    dbc->dbc_owork.buf = dbc_dma_bufs.out_wrk_buf;
+    dbc->dbc_str = dbc_dma_bufs.str_buf;
 
     if ( dbc_open(dbc) )
+    {
+        iommu_add_extra_reserved_device_memory(
+                PFN_DOWN(virt_to_maddr(&dbc_dma_bufs)),
+                PFN_UP(sizeof(dbc_dma_bufs)),
+                uart->dbc.sbdf);
         serial_register_uart(SERHND_DBGP, &dbc_uart_driver, &dbc_uart);
+    }
 }
 
 #ifdef DBC_DEBUG