* [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.