All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/3] xen: fix and cleanup domctl handling
@ 2022-04-19 13:52 Juergen Gross
  2022-04-19 13:52 ` [PATCH v3 1/3] xen: fix XEN_DOMCTL_gdbsx_guestmemio crash Juergen Gross
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Juergen Gross @ 2022-04-19 13:52 UTC (permalink / raw)
  To: xen-devel
  Cc: Juergen Gross, Andrew Cooper, George Dunlap, Jan Beulich,
	Julien Grall, Stefano Stabellini, Wei Liu, Elena Ufimtseva,
	Roger Pau Monné,
	Bertrand Marquis, Volodymyr Babchuk

A fix of a hypervisor crash in domctl handling and some related cleanup.

Juergen Gross (3):
  xen: fix XEN_DOMCTL_gdbsx_guestmemio crash
  xen: cleanup gdbsx_guest_mem_io() call
  xen/iommu: cleanup iommu related domctl handling

 xen/arch/arm/domctl.c               | 11 +----------
 xen/arch/x86/debug.c                | 12 +++---------
 xen/arch/x86/domctl.c               |  8 ++++----
 xen/arch/x86/include/asm/debugger.h |  2 +-
 xen/common/domctl.c                 |  8 +++++++-
 5 files changed, 16 insertions(+), 25 deletions(-)

-- 
2.34.1



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

* [PATCH v3 1/3] xen: fix XEN_DOMCTL_gdbsx_guestmemio crash
  2022-04-19 13:52 [PATCH v3 0/3] xen: fix and cleanup domctl handling Juergen Gross
@ 2022-04-19 13:52 ` Juergen Gross
  2022-04-19 14:32   ` Jan Beulich
  2022-04-19 14:37   ` Andrew Cooper
  2022-04-19 13:52 ` [PATCH v3 2/3] xen: cleanup gdbsx_guest_mem_io() call Juergen Gross
  2022-04-19 13:52 ` [PATCH v3 3/3] xen/iommu: cleanup iommu related domctl handling Juergen Gross
  2 siblings, 2 replies; 13+ messages in thread
From: Juergen Gross @ 2022-04-19 13:52 UTC (permalink / raw)
  To: xen-devel
  Cc: Juergen Gross, Andrew Cooper, George Dunlap, Jan Beulich,
	Julien Grall, Stefano Stabellini, Wei Liu, Cheyenne Wills

A hypervisor built without CONFIG_GDBSX will crash in case the
XEN_DOMCTL_gdbsx_guestmemio domctl is being called, as the call will
end up in iommu_do_domctl() with d == NULL:

(XEN) CPU:    6
(XEN) RIP:    e008:[<ffff82d040269984>] iommu_do_domctl+0x4/0x30
(XEN) RFLAGS: 0000000000010202   CONTEXT: hypervisor (d0v0)
(XEN) rax: 00000000000003e8   rbx: ffff830856277ef8   rcx: ffff830856277fff
...
(XEN) Xen call trace:
(XEN)    [<ffff82d040269984>] R iommu_do_domctl+0x4/0x30
(XEN)    [<ffff82d04035cd5f>] S arch_do_domctl+0x7f/0x2330
(XEN)    [<ffff82d040239e46>] S do_domctl+0xe56/0x1930
(XEN)    [<ffff82d040238ff0>] S do_domctl+0/0x1930
(XEN)    [<ffff82d0402f8c59>] S pv_hypercall+0x99/0x110
(XEN)    [<ffff82d0402f5161>] S arch/x86/pv/domain.c#_toggle_guest_pt+0x11/0x90
(XEN)    [<ffff82d040366288>] S lstar_enter+0x128/0x130
(XEN)
(XEN) Pagetable walk from 0000000000000144:
(XEN)  L4[0x000] = 0000000000000000 ffffffffffffffff
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 6:
(XEN) FATAL PAGE FAULT
(XEN) [error_code=0000]
(XEN) Faulting linear address: 0000000000000144

Fix this issue by making sure the domain pointer has a sane value.

Reported-by: Cheyenne Wills <cheyenne.wills@gmail.com>
Fixes: e726a82ca0dc ("xen: make gdbsx support configurable")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- use gdbsx_guest_mem_io() interface modification (Jan Beulich)
V3:
- avoid d being NULL (Andrew Cooper)
---
 xen/common/domctl.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 57135d4478..5879117580 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -308,7 +308,6 @@ long cf_check do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         if ( op->domain == DOMID_INVALID )
         {
     case XEN_DOMCTL_createdomain:
-    case XEN_DOMCTL_gdbsx_guestmemio:
             d = NULL;
             break;
         }
-- 
2.34.1



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

* [PATCH v3 2/3] xen: cleanup gdbsx_guest_mem_io() call
  2022-04-19 13:52 [PATCH v3 0/3] xen: fix and cleanup domctl handling Juergen Gross
  2022-04-19 13:52 ` [PATCH v3 1/3] xen: fix XEN_DOMCTL_gdbsx_guestmemio crash Juergen Gross
@ 2022-04-19 13:52 ` Juergen Gross
  2022-04-19 14:32   ` Jan Beulich
  2022-04-19 14:41   ` Andrew Cooper
  2022-04-19 13:52 ` [PATCH v3 3/3] xen/iommu: cleanup iommu related domctl handling Juergen Gross
  2 siblings, 2 replies; 13+ messages in thread
From: Juergen Gross @ 2022-04-19 13:52 UTC (permalink / raw)
  To: xen-devel
  Cc: Juergen Gross, Elena Ufimtseva, Jan Beulich, Andrew Cooper,
	Roger Pau Monné,
	Wei Liu

Modify the gdbsx_guest_mem_io() interface to take the already known
domain pointer as parameter instead of the domid. This enables to
remove some more code further down the call tree.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- split off from previous patch (Andrew Cooper)
---
 xen/arch/x86/debug.c                | 12 +++---------
 xen/arch/x86/domctl.c               |  6 +++---
 xen/arch/x86/include/asm/debugger.h |  2 +-
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index d90dc93056..62fbabb084 100644
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -159,17 +159,11 @@ static unsigned int dbg_rw_guest_mem(struct domain *dp, unsigned long addr,
  * Returns: number of bytes remaining to be copied.
  */
 unsigned int dbg_rw_mem(unsigned long gva, XEN_GUEST_HANDLE_PARAM(void) buf,
-                        unsigned int len, domid_t domid, bool toaddr,
+                        unsigned int len, struct domain *d, bool toaddr,
                         uint64_t pgd3)
 {
-    struct domain *d = rcu_lock_domain_by_id(domid);
-
-    if ( d )
-    {
-        if ( !d->is_dying )
-            len = dbg_rw_guest_mem(d, gva, buf, len, toaddr, pgd3);
-        rcu_unlock_domain(d);
-    }
+    if ( d && !d->is_dying )
+        len = dbg_rw_guest_mem(d, gva, buf, len, toaddr, pgd3);
 
     return len;
 }
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index e49f9e91b9..a6aae500a3 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -38,10 +38,10 @@
 #include <asm/cpuid.h>
 
 #ifdef CONFIG_GDBSX
-static int gdbsx_guest_mem_io(domid_t domid, struct xen_domctl_gdbsx_memio *iop)
+static int gdbsx_guest_mem_io(struct domain *d, struct xen_domctl_gdbsx_memio *iop)
 {
     iop->remain = dbg_rw_mem(iop->gva, guest_handle_from_ptr(iop->uva, void),
-                             iop->len, domid, iop->gwr, iop->pgd3val);
+                             iop->len, d, iop->gwr, iop->pgd3val);
 
     return iop->remain ? -EFAULT : 0;
 }
@@ -828,7 +828,7 @@ long arch_do_domctl(
 #ifdef CONFIG_GDBSX
     case XEN_DOMCTL_gdbsx_guestmemio:
         domctl->u.gdbsx_guest_memio.remain = domctl->u.gdbsx_guest_memio.len;
-        ret = gdbsx_guest_mem_io(domctl->domain, &domctl->u.gdbsx_guest_memio);
+        ret = gdbsx_guest_mem_io(d, &domctl->u.gdbsx_guest_memio);
         if ( !ret )
            copyback = true;
         break;
diff --git a/xen/arch/x86/include/asm/debugger.h b/xen/arch/x86/include/asm/debugger.h
index 99803bfd0c..221bcde137 100644
--- a/xen/arch/x86/include/asm/debugger.h
+++ b/xen/arch/x86/include/asm/debugger.h
@@ -94,7 +94,7 @@ static inline bool debugger_trap_entry(
 
 #ifdef CONFIG_GDBSX
 unsigned int dbg_rw_mem(unsigned long gva, XEN_GUEST_HANDLE_PARAM(void) buf,
-                        unsigned int len, domid_t domid, bool toaddr,
+                        unsigned int len, struct domain *d, bool toaddr,
                         uint64_t pgd3);
 #endif
 
-- 
2.34.1



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

* [PATCH v3 3/3] xen/iommu: cleanup iommu related domctl handling
  2022-04-19 13:52 [PATCH v3 0/3] xen: fix and cleanup domctl handling Juergen Gross
  2022-04-19 13:52 ` [PATCH v3 1/3] xen: fix XEN_DOMCTL_gdbsx_guestmemio crash Juergen Gross
  2022-04-19 13:52 ` [PATCH v3 2/3] xen: cleanup gdbsx_guest_mem_io() call Juergen Gross
@ 2022-04-19 13:52 ` Juergen Gross
  2022-04-19 14:41   ` Jan Beulich
  2022-04-19 14:51   ` Andrew Cooper
  2 siblings, 2 replies; 13+ messages in thread
From: Juergen Gross @ 2022-04-19 13:52 UTC (permalink / raw)
  To: xen-devel
  Cc: Juergen Gross, Stefano Stabellini, Julien Grall,
	Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Jan Beulich, Wei Liu, Roger Pau Monné

Today iommu_do_domctl() is being called from arch_do_domctl() in the
"default:" case of a switch statement. This has led already to crashes
due to unvalidated parameters.

Fix that by moving the call of iommu_do_domctl() to the main switch
statement of do_domctl().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
Another possibility would even be to merge iommu_do_domctl() completely
into do_domctl(), but I wanted to start with a less intrusive variant.
V3:
- new patch
---
 xen/arch/arm/domctl.c | 11 +----------
 xen/arch/x86/domctl.c |  2 +-
 xen/common/domctl.c   |  7 +++++++
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index 6245af6d0b..1baf25c3d9 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -176,16 +176,7 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
         return rc;
     }
     default:
-    {
-        int rc;
-
-        rc = subarch_do_domctl(domctl, d, u_domctl);
-
-        if ( rc == -ENOSYS )
-            rc = iommu_do_domctl(domctl, d, u_domctl);
-
-        return rc;
-    }
+        return subarch_do_domctl(domctl, d, u_domctl);
     }
 }
 
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index a6aae500a3..c9699bb868 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1380,7 +1380,7 @@ long arch_do_domctl(
         break;
 
     default:
-        ret = iommu_do_domctl(domctl, d, u_domctl);
+        ret = -ENOSYS;
         break;
     }
 
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 5879117580..0a866e3132 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -871,6 +871,13 @@ long cf_check do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             copyback = 1;
         break;
 
+    case XEN_DOMCTL_assign_device:
+    case XEN_DOMCTL_test_assign_device:
+    case XEN_DOMCTL_deassign_device:
+    case XEN_DOMCTL_get_device_group:
+        ret = iommu_do_domctl(op, d, u_domctl);
+        break;
+
     default:
         ret = arch_do_domctl(op, d, u_domctl);
         break;
-- 
2.34.1



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

* Re: [PATCH v3 1/3] xen: fix XEN_DOMCTL_gdbsx_guestmemio crash
  2022-04-19 13:52 ` [PATCH v3 1/3] xen: fix XEN_DOMCTL_gdbsx_guestmemio crash Juergen Gross
@ 2022-04-19 14:32   ` Jan Beulich
  2022-04-19 14:37   ` Andrew Cooper
  1 sibling, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2022-04-19 14:32 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
	Wei Liu, Cheyenne Wills, xen-devel

On 19.04.2022 15:52, Juergen Gross wrote:
> A hypervisor built without CONFIG_GDBSX will crash in case the
> XEN_DOMCTL_gdbsx_guestmemio domctl is being called, as the call will
> end up in iommu_do_domctl() with d == NULL:
> 
> (XEN) CPU:    6
> (XEN) RIP:    e008:[<ffff82d040269984>] iommu_do_domctl+0x4/0x30
> (XEN) RFLAGS: 0000000000010202   CONTEXT: hypervisor (d0v0)
> (XEN) rax: 00000000000003e8   rbx: ffff830856277ef8   rcx: ffff830856277fff
> ...
> (XEN) Xen call trace:
> (XEN)    [<ffff82d040269984>] R iommu_do_domctl+0x4/0x30
> (XEN)    [<ffff82d04035cd5f>] S arch_do_domctl+0x7f/0x2330
> (XEN)    [<ffff82d040239e46>] S do_domctl+0xe56/0x1930
> (XEN)    [<ffff82d040238ff0>] S do_domctl+0/0x1930
> (XEN)    [<ffff82d0402f8c59>] S pv_hypercall+0x99/0x110
> (XEN)    [<ffff82d0402f5161>] S arch/x86/pv/domain.c#_toggle_guest_pt+0x11/0x90
> (XEN)    [<ffff82d040366288>] S lstar_enter+0x128/0x130
> (XEN)
> (XEN) Pagetable walk from 0000000000000144:
> (XEN)  L4[0x000] = 0000000000000000 ffffffffffffffff
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 6:
> (XEN) FATAL PAGE FAULT
> (XEN) [error_code=0000]
> (XEN) Faulting linear address: 0000000000000144
> 
> Fix this issue by making sure the domain pointer has a sane value.
> 
> Reported-by: Cheyenne Wills <cheyenne.wills@gmail.com>
> Fixes: e726a82ca0dc ("xen: make gdbsx support configurable")
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

* Re: [PATCH v3 2/3] xen: cleanup gdbsx_guest_mem_io() call
  2022-04-19 13:52 ` [PATCH v3 2/3] xen: cleanup gdbsx_guest_mem_io() call Juergen Gross
@ 2022-04-19 14:32   ` Jan Beulich
  2022-04-19 14:41   ` Andrew Cooper
  1 sibling, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2022-04-19 14:32 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Elena Ufimtseva, Andrew Cooper, Roger Pau Monné, Wei Liu, xen-devel

On 19.04.2022 15:52, Juergen Gross wrote:
> Modify the gdbsx_guest_mem_io() interface to take the already known
> domain pointer as parameter instead of the domid. This enables to
> remove some more code further down the call tree.
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

* Re: [PATCH v3 1/3] xen: fix XEN_DOMCTL_gdbsx_guestmemio crash
  2022-04-19 13:52 ` [PATCH v3 1/3] xen: fix XEN_DOMCTL_gdbsx_guestmemio crash Juergen Gross
  2022-04-19 14:32   ` Jan Beulich
@ 2022-04-19 14:37   ` Andrew Cooper
  2022-04-19 14:50     ` Juergen Gross
  1 sibling, 1 reply; 13+ messages in thread
From: Andrew Cooper @ 2022-04-19 14:37 UTC (permalink / raw)
  To: Juergen Gross, xen-devel
  Cc: George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini,
	Wei Liu, Cheyenne Wills

On 19/04/2022 14:52, Juergen Gross wrote:
> A hypervisor built without CONFIG_GDBSX will crash in case the
> XEN_DOMCTL_gdbsx_guestmemio domctl is being called, as the call will
> end up in iommu_do_domctl() with d == NULL:
>
> (XEN) CPU:    6
> (XEN) RIP:    e008:[<ffff82d040269984>] iommu_do_domctl+0x4/0x30
> (XEN) RFLAGS: 0000000000010202   CONTEXT: hypervisor (d0v0)
> (XEN) rax: 00000000000003e8   rbx: ffff830856277ef8   rcx: ffff830856277fff
> ...
> (XEN) Xen call trace:
> (XEN)    [<ffff82d040269984>] R iommu_do_domctl+0x4/0x30
> (XEN)    [<ffff82d04035cd5f>] S arch_do_domctl+0x7f/0x2330
> (XEN)    [<ffff82d040239e46>] S do_domctl+0xe56/0x1930
> (XEN)    [<ffff82d040238ff0>] S do_domctl+0/0x1930
> (XEN)    [<ffff82d0402f8c59>] S pv_hypercall+0x99/0x110
> (XEN)    [<ffff82d0402f5161>] S arch/x86/pv/domain.c#_toggle_guest_pt+0x11/0x90
> (XEN)    [<ffff82d040366288>] S lstar_enter+0x128/0x130
> (XEN)
> (XEN) Pagetable walk from 0000000000000144:
> (XEN)  L4[0x000] = 0000000000000000 ffffffffffffffff
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 6:
> (XEN) FATAL PAGE FAULT
> (XEN) [error_code=0000]
> (XEN) Faulting linear address: 0000000000000144
>
> Fix this issue by making sure the domain pointer has a sane value.
>
> Reported-by: Cheyenne Wills <cheyenne.wills@gmail.com>
> Fixes: e726a82ca0dc ("xen: make gdbsx support configurable")
> Signed-off-by: Juergen Gross <jgross@suse.com>

Thanks, but I was hoping for a bit of discussion on the use of DOMID_IDLE.

It used to be permitted to pass DOMID_IDLE to dbg_rw_mem() to access Xen
memory, which is why the XEN_DOMCTL_gdbsx_guestmemio special case existed.

It turns out that it was also e726a82ca0dc which dropped the ability to
use DOMID_IDLE, meaning that this fix is a missing hunk from the
original change too.

This is relevant backport information, and would have created
complexities if they hadn't been the same changeset.

So, now about:

"It used to be permitted to pass DOMID_IDLE to dbg_rw_mem(), which is
why the special case excluding domid checks exists.  Now that it is only
permitted to pass proper domids, remove the special case, thus making
'd' always valid."

?

Can be fixed on commit, so Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com> for everything else.

~Andrew

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

* Re: [PATCH v3 2/3] xen: cleanup gdbsx_guest_mem_io() call
  2022-04-19 13:52 ` [PATCH v3 2/3] xen: cleanup gdbsx_guest_mem_io() call Juergen Gross
  2022-04-19 14:32   ` Jan Beulich
@ 2022-04-19 14:41   ` Andrew Cooper
  1 sibling, 0 replies; 13+ messages in thread
From: Andrew Cooper @ 2022-04-19 14:41 UTC (permalink / raw)
  To: Juergen Gross, xen-devel
  Cc: Elena Ufimtseva, Jan Beulich, Roger Pau Monne, Wei Liu

On 19/04/2022 14:52, Juergen Gross wrote:
> Modify the gdbsx_guest_mem_io() interface to take the already known
> domain pointer as parameter instead of the domid. This enables to
> remove some more code further down the call tree.
>
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I'll rebase the other debugger cleanup over this.

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

* Re: [PATCH v3 3/3] xen/iommu: cleanup iommu related domctl handling
  2022-04-19 13:52 ` [PATCH v3 3/3] xen/iommu: cleanup iommu related domctl handling Juergen Gross
@ 2022-04-19 14:41   ` Jan Beulich
  2022-04-19 14:51   ` Andrew Cooper
  1 sibling, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2022-04-19 14:41 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
	Roger Pau Monné,
	xen-devel

On 19.04.2022 15:52, Juergen Gross wrote:
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -871,6 +871,13 @@ long cf_check do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>              copyback = 1;
>          break;
>  
> +    case XEN_DOMCTL_assign_device:
> +    case XEN_DOMCTL_test_assign_device:
> +    case XEN_DOMCTL_deassign_device:
> +    case XEN_DOMCTL_get_device_group:

As said in reply to Andrew, I'm not convinced having these enumerated here
is a good idea. But in any event the whole addition wants framing by
#ifdef CONFIG_HAS_PASSTHROUGH now.

For x86 I wonder whether the adjustment wouldn't allow to drop domctl.c's
including of xen/iommu.h (but perhaps it's included indirectly anyway).

Jan

> +        ret = iommu_do_domctl(op, d, u_domctl);
> +        break;
> +
>      default:
>          ret = arch_do_domctl(op, d, u_domctl);
>          break;



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

* Re: [PATCH v3 1/3] xen: fix XEN_DOMCTL_gdbsx_guestmemio crash
  2022-04-19 14:37   ` Andrew Cooper
@ 2022-04-19 14:50     ` Juergen Gross
  0 siblings, 0 replies; 13+ messages in thread
From: Juergen Gross @ 2022-04-19 14:50 UTC (permalink / raw)
  To: Andrew Cooper, xen-devel
  Cc: George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini,
	Wei Liu, Cheyenne Wills


[-- Attachment #1.1.1: Type: text/plain, Size: 2522 bytes --]

On 19.04.22 16:37, Andrew Cooper wrote:
> On 19/04/2022 14:52, Juergen Gross wrote:
>> A hypervisor built without CONFIG_GDBSX will crash in case the
>> XEN_DOMCTL_gdbsx_guestmemio domctl is being called, as the call will
>> end up in iommu_do_domctl() with d == NULL:
>>
>> (XEN) CPU:    6
>> (XEN) RIP:    e008:[<ffff82d040269984>] iommu_do_domctl+0x4/0x30
>> (XEN) RFLAGS: 0000000000010202   CONTEXT: hypervisor (d0v0)
>> (XEN) rax: 00000000000003e8   rbx: ffff830856277ef8   rcx: ffff830856277fff
>> ...
>> (XEN) Xen call trace:
>> (XEN)    [<ffff82d040269984>] R iommu_do_domctl+0x4/0x30
>> (XEN)    [<ffff82d04035cd5f>] S arch_do_domctl+0x7f/0x2330
>> (XEN)    [<ffff82d040239e46>] S do_domctl+0xe56/0x1930
>> (XEN)    [<ffff82d040238ff0>] S do_domctl+0/0x1930
>> (XEN)    [<ffff82d0402f8c59>] S pv_hypercall+0x99/0x110
>> (XEN)    [<ffff82d0402f5161>] S arch/x86/pv/domain.c#_toggle_guest_pt+0x11/0x90
>> (XEN)    [<ffff82d040366288>] S lstar_enter+0x128/0x130
>> (XEN)
>> (XEN) Pagetable walk from 0000000000000144:
>> (XEN)  L4[0x000] = 0000000000000000 ffffffffffffffff
>> (XEN)
>> (XEN) ****************************************
>> (XEN) Panic on CPU 6:
>> (XEN) FATAL PAGE FAULT
>> (XEN) [error_code=0000]
>> (XEN) Faulting linear address: 0000000000000144
>>
>> Fix this issue by making sure the domain pointer has a sane value.
>>
>> Reported-by: Cheyenne Wills <cheyenne.wills@gmail.com>
>> Fixes: e726a82ca0dc ("xen: make gdbsx support configurable")
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Thanks, but I was hoping for a bit of discussion on the use of DOMID_IDLE.
> 
> It used to be permitted to pass DOMID_IDLE to dbg_rw_mem() to access Xen
> memory, which is why the XEN_DOMCTL_gdbsx_guestmemio special case existed.
> 
> It turns out that it was also e726a82ca0dc which dropped the ability to
> use DOMID_IDLE, meaning that this fix is a missing hunk from the
> original change too.
> 
> This is relevant backport information, and would have created
> complexities if they hadn't been the same changeset.
> 
> So, now about:
> 
> "It used to be permitted to pass DOMID_IDLE to dbg_rw_mem(), which is
> why the special case excluding domid checks exists.  Now that it is only
> permitted to pass proper domids, remove the special case, thus making
> 'd' always valid."
> 
> ?

Fine with me.

> 
> Can be fixed on commit, so Reviewed-by: Andrew Cooper
> <andrew.cooper3@citrix.com> for everything else.

Thanks,


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

* Re: [PATCH v3 3/3] xen/iommu: cleanup iommu related domctl handling
  2022-04-19 13:52 ` [PATCH v3 3/3] xen/iommu: cleanup iommu related domctl handling Juergen Gross
  2022-04-19 14:41   ` Jan Beulich
@ 2022-04-19 14:51   ` Andrew Cooper
  2022-04-19 14:56     ` Juergen Gross
  1 sibling, 1 reply; 13+ messages in thread
From: Andrew Cooper @ 2022-04-19 14:51 UTC (permalink / raw)
  To: Juergen Gross, xen-devel
  Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
	Volodymyr Babchuk, George Dunlap, Jan Beulich, Wei Liu,
	Roger Pau Monne

On 19/04/2022 14:52, Juergen Gross wrote:
> Today iommu_do_domctl() is being called from arch_do_domctl() in the
> "default:" case of a switch statement. This has led already to crashes
> due to unvalidated parameters.
>
> Fix that by moving the call of iommu_do_domctl() to the main switch
> statement of do_domctl().
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> Another possibility would even be to merge iommu_do_domctl() completely
> into do_domctl(), but I wanted to start with a less intrusive variant.
> V3:
> - new patch

I definitely prefer this approach, thanks.  In addition to being
clearer, it's also faster because there isn't a long line of "do you
understand this subop?" calls when we know exactly what it is.

However, I think we need stub for the !HAS_PASSTHROUGH case now that it
is being called from common code.

I'd forgotten that it was used on ARM now, and yes - it absolutely
should be called from somewhere common, not from the arch hooks.

~Andrew

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

* Re: [PATCH v3 3/3] xen/iommu: cleanup iommu related domctl handling
  2022-04-19 14:51   ` Andrew Cooper
@ 2022-04-19 14:56     ` Juergen Gross
  2022-04-19 16:11       ` Jan Beulich
  0 siblings, 1 reply; 13+ messages in thread
From: Juergen Gross @ 2022-04-19 14:56 UTC (permalink / raw)
  To: Andrew Cooper, xen-devel
  Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
	Volodymyr Babchuk, George Dunlap, Jan Beulich, Wei Liu,
	Roger Pau Monne


[-- Attachment #1.1.1: Type: text/plain, Size: 1006 bytes --]

On 19.04.22 16:51, Andrew Cooper wrote:
> On 19/04/2022 14:52, Juergen Gross wrote:
>> Today iommu_do_domctl() is being called from arch_do_domctl() in the
>> "default:" case of a switch statement. This has led already to crashes
>> due to unvalidated parameters.
>>
>> Fix that by moving the call of iommu_do_domctl() to the main switch
>> statement of do_domctl().
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> Another possibility would even be to merge iommu_do_domctl() completely
>> into do_domctl(), but I wanted to start with a less intrusive variant.
>> V3:
>> - new patch
> 
> I definitely prefer this approach, thanks.  In addition to being
> clearer, it's also faster because there isn't a long line of "do you
> understand this subop?" calls when we know exactly what it is.
> 
> However, I think we need stub for the !HAS_PASSTHROUGH case now that it
> is being called from common code.

Okay, I'll add it. Jan, are you fine with a stub?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

* Re: [PATCH v3 3/3] xen/iommu: cleanup iommu related domctl handling
  2022-04-19 14:56     ` Juergen Gross
@ 2022-04-19 16:11       ` Jan Beulich
  0 siblings, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2022-04-19 16:11 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
	Volodymyr Babchuk, George Dunlap, Wei Liu, Andrew Cooper,
	xen-devel, Roger Pau Monne

On 19.04.2022 16:56, Juergen Gross wrote:
> On 19.04.22 16:51, Andrew Cooper wrote:
>> On 19/04/2022 14:52, Juergen Gross wrote:
>>> Today iommu_do_domctl() is being called from arch_do_domctl() in the
>>> "default:" case of a switch statement. This has led already to crashes
>>> due to unvalidated parameters.
>>>
>>> Fix that by moving the call of iommu_do_domctl() to the main switch
>>> statement of do_domctl().
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> Another possibility would even be to merge iommu_do_domctl() completely
>>> into do_domctl(), but I wanted to start with a less intrusive variant.
>>> V3:
>>> - new patch
>>
>> I definitely prefer this approach, thanks.  In addition to being
>> clearer, it's also faster because there isn't a long line of "do you
>> understand this subop?" calls when we know exactly what it is.
>>
>> However, I think we need stub for the !HAS_PASSTHROUGH case now that it
>> is being called from common code.
> 
> Okay, I'll add it. Jan, are you fine with a stub?

Sure.

Jan



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

end of thread, other threads:[~2022-04-19 16:12 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-19 13:52 [PATCH v3 0/3] xen: fix and cleanup domctl handling Juergen Gross
2022-04-19 13:52 ` [PATCH v3 1/3] xen: fix XEN_DOMCTL_gdbsx_guestmemio crash Juergen Gross
2022-04-19 14:32   ` Jan Beulich
2022-04-19 14:37   ` Andrew Cooper
2022-04-19 14:50     ` Juergen Gross
2022-04-19 13:52 ` [PATCH v3 2/3] xen: cleanup gdbsx_guest_mem_io() call Juergen Gross
2022-04-19 14:32   ` Jan Beulich
2022-04-19 14:41   ` Andrew Cooper
2022-04-19 13:52 ` [PATCH v3 3/3] xen/iommu: cleanup iommu related domctl handling Juergen Gross
2022-04-19 14:41   ` Jan Beulich
2022-04-19 14:51   ` Andrew Cooper
2022-04-19 14:56     ` Juergen Gross
2022-04-19 16:11       ` Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.