[Xen-devel] [PATCH 3/4] x86/debugger: Use copy_to/from_guest() in dbg_rw_guest_mem()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

Using gdbsx on Broadwell systems suffers a SMAP violation because
dbg_rw_guest_mem() uses memcpy() with a userspace pointer.

The functions dbg_rw_mem() and dbg_rw_guest_mem() have been updated to pass
'void * __user' pointers which indicates their nature clearly.

Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
CC: Jan Beulich <JBeulich@xxxxxxxx>

---
After these changes, 'gdbsx -c' works as well as it did before (i.e. not at
all, for the stack trace), but doesn't take Xen down with it.

There are other issues in this area, and XEN_DOMCTL_gdbsx_guestmemio is
certainly not fit yet for removal from the XSA-77 exclusion list.
---
 xen/arch/x86/debug.c           |   45 +++++++++++++++++++++++-----------------
 xen/arch/x86/domctl.c          |   14 ++++++-------
 xen/include/asm-x86/debugger.h |    7 +++----
 3 files changed, 36 insertions(+), 30 deletions(-)

diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index 435bd40..801dcf2 100644
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -41,6 +41,9 @@
 #define DBGP2(...) ((void)0)
 #endif
 
+typedef unsigned long dbgva_t;
+typedef unsigned char dbgbyte_t;
+
 /* Returns: mfn for the given (hvm guest) vaddr */
 static unsigned long 
 dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr,
@@ -154,13 +157,14 @@
 }
 
 /* Returns: number of bytes remaining to be copied */
-static int
-dbg_rw_guest_mem(dbgva_t addr, dbgbyte_t *buf, int len, struct domain *dp, 
-                 int toaddr, uint64_t pgd3)
+unsigned int dbg_rw_guest_mem(struct domain *dp, void * __user gaddr,
+                              void * __user buf, unsigned int len,
+                              bool_t toaddr, uint64_t pgd3)
 {
     while ( len > 0 )
     {
         char *va;
+        unsigned long addr = (unsigned long)gaddr;
         unsigned long mfn, gfn = INVALID_GFN, pagecnt;
 
         pagecnt = min_t(long, PAGE_SIZE - (addr & ~PAGE_MASK), len);
@@ -176,12 +180,12 @@
 
         if ( toaddr )
         {
-            memcpy(va, buf, pagecnt);    /* va = buf */
+            copy_from_user(va, buf, pagecnt);    /* va = buf */
             paging_mark_dirty(dp, mfn);
         }
         else
         {
-            memcpy(buf, va, pagecnt);    /* buf = va */
+            copy_to_user(buf, va, pagecnt);    /* buf = va */
         }
 
         unmap_domain_page(va);
@@ -203,27 +207,30 @@
  * pgd3: value of init_mm.pgd[3] in guest. see above.
  * Returns: number of bytes remaining to be copied. 
  */
-int
-dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf, int len, domid_t domid, int toaddr,
-           uint64_t pgd3)
+unsigned int dbg_rw_mem(void * __user addr, void * __user buf,
+                        unsigned int len, domid_t domid, bool_t toaddr,
+                        uint64_t pgd3)
 {
-    struct domain *dp = get_domain_by_id(domid);
-    int hyp = (domid == DOMID_IDLE);
+    DBGP2("gmem:addr:%lx buf:%p len:$%u domid:%d toaddr:%x\n",
+          addr, buf, len, domid, toaddr);
 
-    DBGP2("gmem:addr:%lx buf:%p len:$%d domid:%x toaddr:%x dp:%p\n", 
-          addr, buf, len, domid, toaddr, dp);
-    if ( hyp )
+    if ( domid == DOMID_IDLE )
     {
         if ( toaddr )
-            len = __copy_to_user((void *)addr, buf, len);
+            len = __copy_to_user(addr, buf, len);
         else
-            len = __copy_from_user(buf, (void *)addr, len);
+            len = __copy_from_user(buf, addr, len);
     }
-    else if ( dp )
+    else
     {
-        if ( !dp->is_dying )   /* make sure guest is still there */
-            len= dbg_rw_guest_mem(addr, buf, len, dp, toaddr, pgd3);
-        put_domain(dp);
+        struct domain *d = get_domain_by_id(domid);
+
+        if ( d )
+        {
+            if ( !d->is_dying )
+                len = dbg_rw_guest_mem(d, addr, buf, len, toaddr, pgd3);
+            put_domain(d);
+        }
     }
 
     DBGP2("gmem:exit:len:$%d\n", len);
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index e9f76d0..1d3854f 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -37,14 +37,14 @@
 #include <asm/debugger.h>
 #include <asm/psr.h>
 
-static int gdbsx_guest_mem_io(
-    domid_t domid, struct xen_domctl_gdbsx_memio *iop)
+static int gdbsx_guest_mem_io(domid_t domid, struct xen_domctl_gdbsx_memio 
*iop)
 {
-    ulong l_uva = (ulong)iop->uva;
-    iop->remain = dbg_rw_mem(
-        (dbgva_t)iop->gva, (dbgbyte_t *)l_uva, iop->len, domid,
-        iop->gwr, iop->pgd3val);
-    return (iop->remain ? -EFAULT : 0);
+    void * __user gva = (void *)iop->gva, * __user uva = (void *)iop->uva;
+
+    iop->remain = dbg_rw_mem(gva, uva, iop->len, domid,
+                             !!iop->gwr, iop->pgd3val);
+
+    return iop->remain ? -EFAULT : 0;
 }
 
 #define MAX_IOPORTS 0x10000
diff --git a/xen/include/asm-x86/debugger.h b/xen/include/asm-x86/debugger.h
index 0408bec..33f4700 100644
--- a/xen/include/asm-x86/debugger.h
+++ b/xen/include/asm-x86/debugger.h
@@ -82,9 +82,8 @@ static inline int debugger_trap_entry(
     return 0;
 }
 
-typedef unsigned long dbgva_t;
-typedef unsigned char dbgbyte_t;
-extern int dbg_rw_mem(dbgva_t addr, dbgbyte_t *buf, int len,
-                      domid_t domid, int toaddr, uint64_t pgd3);
+unsigned int dbg_rw_mem(void * __user addr, void * __user buf,
+                        unsigned int len, domid_t domid, bool_t toaddr,
+                        uint64_t pgd3);
 
 #endif /* __X86_DEBUGGER_H__ */
-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel