* [PATCH 1/3] x86: fix guest CPUID handling
2014-04-30 14:11 [PATCH 0/3] domctl related adjustments Jan Beulich
@ 2014-04-30 14:22 ` Jan Beulich
2014-04-30 17:03 ` Andrew Cooper
2014-05-01 15:29 ` Tim Deegan
2014-04-30 14:23 ` [PATCH 2/3] domctl: perform initial post-XSA-77 auditing Jan Beulich
2014-04-30 14:24 ` [PATCH 3/3] domctl: tighten XEN_DOMCTL_*_permission Jan Beulich
2 siblings, 2 replies; 14+ messages in thread
From: Jan Beulich @ 2014-04-30 14:22 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Campbell, Keir Fraser, Ian Jackson, Tim Deegan
[-- Attachment #1: Type: text/plain, Size: 2528 bytes --]
The way XEN_DOMCTL_set_cpuid got handled so far allowed for surprises
to the caller. With this set of operations
- set leaf A (using array index 0)
- set leaf B (using array index 1)
- clear leaf A (clearing array index 0)
- set leaf B (using array index 0)
- clear leaf B (clearing array index 0)
the entry for leaf B at array index 1 would still be in place, while
the caller would expect it to be cleared.
While looking at the use sites of d->arch.cpuid[] I also noticed that
the allocation of the array needlessly uses the zeroing form - the
relevant fields of the array elements get set in a loop immediately
following the allocation.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- 2014-04-16.orig/xen/arch/x86/domain.c 2014-02-05 15:21:00.000000000 +0100
+++ 2014-04-16/xen/arch/x86/domain.c 2014-04-30 13:53:18.000000000 +0200
@@ -549,7 +549,7 @@ int arch_domain_create(struct domain *d,
if ( !is_idle_domain(d) )
{
- d->arch.cpuids = xzalloc_array(cpuid_input_t, MAX_CPUID_INPUT);
+ d->arch.cpuids = xmalloc_array(cpuid_input_t, MAX_CPUID_INPUT);
rc = -ENOMEM;
if ( d->arch.cpuids == NULL )
goto fail;
--- 2014-04-16.orig/xen/arch/x86/domctl.c 2014-02-05 15:21:01.000000000 +0100
+++ 2014-04-16/xen/arch/x86/domctl.c 2014-04-30 13:43:30.000000000 +0200
@@ -1006,14 +1006,18 @@ long arch_do_domctl(
case XEN_DOMCTL_set_cpuid:
{
xen_domctl_cpuid_t *ctl = &domctl->u.cpuid;
- cpuid_input_t *cpuid = NULL;
+ cpuid_input_t *cpuid, *unused = NULL;
for ( i = 0; i < MAX_CPUID_INPUT; i++ )
{
cpuid = &d->arch.cpuids[i];
if ( cpuid->input[0] == XEN_CPUID_INPUT_UNUSED )
- break;
+ {
+ if ( !unused )
+ unused = cpuid;
+ continue;
+ }
if ( (cpuid->input[0] == ctl->input[0]) &&
((cpuid->input[1] == XEN_CPUID_INPUT_UNUSED) ||
@@ -1021,15 +1025,12 @@ long arch_do_domctl(
break;
}
- if ( i == MAX_CPUID_INPUT )
- {
- ret = -ENOENT;
- }
+ if ( i < MAX_CPUID_INPUT )
+ *cpuid = *ctl;
+ else if ( unused )
+ *unused = *ctl;
else
- {
- memcpy(cpuid, ctl, sizeof(cpuid_input_t));
- ret = 0;
- }
+ ret = -ENOENT;
}
break;
[-- Attachment #2: x86-domctl-cpuid.patch --]
[-- Type: text/plain, Size: 2555 bytes --]
x86: fix guest CPUID handling
The way XEN_DOMCTL_set_cpuid got handled so far allowed for surprises
to the caller. With this set of operations
- set leaf A (using array index 0)
- set leaf B (using array index 1)
- clear leaf A (clearing array index 0)
- set leaf B (using array index 0)
- clear leaf B (clearing array index 0)
the entry for leaf B at array index 1 would still be in place, while
the caller would expect it to be cleared.
While looking at the use sites of d->arch.cpuid[] I also noticed that
the allocation of the array needlessly uses the zeroing form - the
relevant fields of the array elements get set in a loop immediately
following the allocation.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- 2014-04-16.orig/xen/arch/x86/domain.c 2014-02-05 15:21:00.000000000 +0100
+++ 2014-04-16/xen/arch/x86/domain.c 2014-04-30 13:53:18.000000000 +0200
@@ -549,7 +549,7 @@ int arch_domain_create(struct domain *d,
if ( !is_idle_domain(d) )
{
- d->arch.cpuids = xzalloc_array(cpuid_input_t, MAX_CPUID_INPUT);
+ d->arch.cpuids = xmalloc_array(cpuid_input_t, MAX_CPUID_INPUT);
rc = -ENOMEM;
if ( d->arch.cpuids == NULL )
goto fail;
--- 2014-04-16.orig/xen/arch/x86/domctl.c 2014-02-05 15:21:01.000000000 +0100
+++ 2014-04-16/xen/arch/x86/domctl.c 2014-04-30 13:43:30.000000000 +0200
@@ -1006,14 +1006,18 @@ long arch_do_domctl(
case XEN_DOMCTL_set_cpuid:
{
xen_domctl_cpuid_t *ctl = &domctl->u.cpuid;
- cpuid_input_t *cpuid = NULL;
+ cpuid_input_t *cpuid, *unused = NULL;
for ( i = 0; i < MAX_CPUID_INPUT; i++ )
{
cpuid = &d->arch.cpuids[i];
if ( cpuid->input[0] == XEN_CPUID_INPUT_UNUSED )
- break;
+ {
+ if ( !unused )
+ unused = cpuid;
+ continue;
+ }
if ( (cpuid->input[0] == ctl->input[0]) &&
((cpuid->input[1] == XEN_CPUID_INPUT_UNUSED) ||
@@ -1021,15 +1025,12 @@ long arch_do_domctl(
break;
}
- if ( i == MAX_CPUID_INPUT )
- {
- ret = -ENOENT;
- }
+ if ( i < MAX_CPUID_INPUT )
+ *cpuid = *ctl;
+ else if ( unused )
+ *unused = *ctl;
else
- {
- memcpy(cpuid, ctl, sizeof(cpuid_input_t));
- ret = 0;
- }
+ ret = -ENOENT;
}
break;
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] x86: fix guest CPUID handling
2014-04-30 14:22 ` [PATCH 1/3] x86: fix guest CPUID handling Jan Beulich
@ 2014-04-30 17:03 ` Andrew Cooper
2014-05-01 15:29 ` Tim Deegan
1 sibling, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2014-04-30 17:03 UTC (permalink / raw)
To: Jan Beulich; +Cc: Ian Campbell, xen-devel, Keir Fraser, Ian Jackson, Tim Deegan
[-- Attachment #1.1: Type: text/plain, Size: 2831 bytes --]
On 30/04/14 15:22, Jan Beulich wrote:
> The way XEN_DOMCTL_set_cpuid got handled so far allowed for surprises
> to the caller. With this set of operations
> - set leaf A (using array index 0)
> - set leaf B (using array index 1)
> - clear leaf A (clearing array index 0)
> - set leaf B (using array index 0)
> - clear leaf B (clearing array index 0)
> the entry for leaf B at array index 1 would still be in place, while
> the caller would expect it to be cleared.
>
> While looking at the use sites of d->arch.cpuid[] I also noticed that
> the allocation of the array needlessly uses the zeroing form - the
> relevant fields of the array elements get set in a loop immediately
> following the allocation.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> --- 2014-04-16.orig/xen/arch/x86/domain.c 2014-02-05 15:21:00.000000000 +0100
> +++ 2014-04-16/xen/arch/x86/domain.c 2014-04-30 13:53:18.000000000 +0200
> @@ -549,7 +549,7 @@ int arch_domain_create(struct domain *d,
>
> if ( !is_idle_domain(d) )
> {
> - d->arch.cpuids = xzalloc_array(cpuid_input_t, MAX_CPUID_INPUT);
> + d->arch.cpuids = xmalloc_array(cpuid_input_t, MAX_CPUID_INPUT);
> rc = -ENOMEM;
> if ( d->arch.cpuids == NULL )
> goto fail;
> --- 2014-04-16.orig/xen/arch/x86/domctl.c 2014-02-05 15:21:01.000000000 +0100
> +++ 2014-04-16/xen/arch/x86/domctl.c 2014-04-30 13:43:30.000000000 +0200
> @@ -1006,14 +1006,18 @@ long arch_do_domctl(
> case XEN_DOMCTL_set_cpuid:
> {
> xen_domctl_cpuid_t *ctl = &domctl->u.cpuid;
> - cpuid_input_t *cpuid = NULL;
> + cpuid_input_t *cpuid, *unused = NULL;
>
> for ( i = 0; i < MAX_CPUID_INPUT; i++ )
> {
> cpuid = &d->arch.cpuids[i];
>
> if ( cpuid->input[0] == XEN_CPUID_INPUT_UNUSED )
> - break;
> + {
> + if ( !unused )
> + unused = cpuid;
> + continue;
> + }
>
> if ( (cpuid->input[0] == ctl->input[0]) &&
> ((cpuid->input[1] == XEN_CPUID_INPUT_UNUSED) ||
> @@ -1021,15 +1025,12 @@ long arch_do_domctl(
> break;
> }
>
> - if ( i == MAX_CPUID_INPUT )
> - {
> - ret = -ENOENT;
> - }
> + if ( i < MAX_CPUID_INPUT )
> + *cpuid = *ctl;
> + else if ( unused )
> + *unused = *ctl;
> else
> - {
> - memcpy(cpuid, ctl, sizeof(cpuid_input_t));
> - ret = 0;
> - }
> + ret = -ENOENT;
> }
> break;
>
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
[-- Attachment #1.2: Type: text/html, Size: 3664 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] x86: fix guest CPUID handling
2014-04-30 14:22 ` [PATCH 1/3] x86: fix guest CPUID handling Jan Beulich
2014-04-30 17:03 ` Andrew Cooper
@ 2014-05-01 15:29 ` Tim Deegan
1 sibling, 0 replies; 14+ messages in thread
From: Tim Deegan @ 2014-05-01 15:29 UTC (permalink / raw)
To: Jan Beulich; +Cc: Ian Campbell, xen-devel, Keir Fraser, Ian Jackson
At 15:22 +0100 on 30 Apr (1398867751), Jan Beulich wrote:
> The way XEN_DOMCTL_set_cpuid got handled so far allowed for surprises
> to the caller. With this set of operations
> - set leaf A (using array index 0)
> - set leaf B (using array index 1)
> - clear leaf A (clearing array index 0)
> - set leaf B (using array index 0)
> - clear leaf B (clearing array index 0)
> the entry for leaf B at array index 1 would still be in place, while
> the caller would expect it to be cleared.
>
> While looking at the use sites of d->arch.cpuid[] I also noticed that
> the allocation of the array needlessly uses the zeroing form - the
> relevant fields of the array elements get set in a loop immediately
> following the allocation.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Tim Deegan <tim@xen.org>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] domctl: perform initial post-XSA-77 auditing
2014-04-30 14:11 [PATCH 0/3] domctl related adjustments Jan Beulich
2014-04-30 14:22 ` [PATCH 1/3] x86: fix guest CPUID handling Jan Beulich
@ 2014-04-30 14:23 ` Jan Beulich
2014-04-30 16:52 ` Andrew Cooper
2014-05-01 15:30 ` Tim Deegan
2014-04-30 14:24 ` [PATCH 3/3] domctl: tighten XEN_DOMCTL_*_permission Jan Beulich
2 siblings, 2 replies; 14+ messages in thread
From: Jan Beulich @ 2014-04-30 14:23 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Campbell, Keir Fraser, Ian Jackson, Tim Deegan
[-- Attachment #1: Type: text/plain, Size: 9352 bytes --]
In a number of cases, loops over each vCPU in a domain are involved
here. For large numbers of vCPU-s these may still take some time to
complete, but we're limiting them at a couple of thousand at most, so I
would think this should not by itself be an issue. I wonder though
whether it shouldn't be possible to have XSM restrict the vCPU count
that can be set through XEN_DOMCTL_max_vcpus.
XEN_DOMCTL_pausedomain:
A loop over vcpu_sleep_sync() for each of vCPU in the domain. That
function itself has a loop waiting for the subject vCPU to become non-
runnable, which ought to complete quickly (involving an IPI to be sent
and acted on). No other unbounded resource usage.
XEN_DOMCTL_unpausedomain:
Simply a loop calling vcpu_wake() (not having any loops or other
resource usage itself) for each of vCPU in the domain.
XEN_DOMCTL_getdomaininfo:
Two loops (one over all domains, i.e. bounded by the limit of 32k
domains, and another over all vCPU-s in the domain); no other
unbounded resource usage.
XEN_DOMCTL_getpageframeinfo:
Inquiring just a single MFN, i.e. no loops and no other unbounded
resource usage.
XEN_DOMCTL_getpageframeinfo{2,3}:
Number of inquired MFNs is limited to 1024. Beyond that just like
XEN_DOMCTL_getpageframeinfo.
XEN_DOMCTL_getvcpuinfo:
Only obtaining information on the vCPU, no loops or other resource
usage.
XEN_DOMCTL_setdomainhandle:
Simply a memcpy() of a very limited amount of data.
XEN_DOMCTL_setdebugging:
A domain_{,un}pause() pair (see XEN_DOMCTL_{,un}pausedomain) framing
the setting of a flag.
XEN_DOMCTL_hypercall_init:
Initializing a guest provided page with hypercall stubs. No other
resource consumption.
XEN_DOMCTL_arch_setup:
IA64 leftover, interface structure being removed from the public
header.
XEN_DOMCTL_settimeoffset:
Setting a couple of guest state fields. No other resource consumption.
XEN_DOMCTL_getvcpuaffinity:
XEN_DOMCTL_getnodeaffinity:
Involve temporary memory allocations (approximately) bounded by the
number of CPUs in the system / number of nodes built for, which is
okay. Beyond that trivial operation.
XEN_DOMCTL_real_mode_area:
PPC leftover, interface structure being removed from the public
header.
XEN_DOMCTL_resumedomain:
A domain_{,un}pause() pair framing operation very similar to
XEN_DOMCTL_unpausedomain (see above).
XEN_DOMCTL_sendtrigger:
Injects an interrupt (SCI or NMI) without any other resource
consumption.
XEN_DOMCTL_subscribe:
Updates the suspend event channel, i.e. affecting only the controlled
domain.
XEN_DOMCTL_disable_migrate:
XEN_DOMCTL_suppress_spurious_page_faults:
Just setting respective flags on the domain.
XEN_DOMCTL_get_address_size:
Simply reading the guest property.
XEN_DOMCTL_set_opt_feature:
Was already tagged IA64-only.
XEN_DOMCTL_set_cpuid:
MAX_CPUID_INPUT bounded loop, which is okay. No other resource
consumption.
XEN_DOMCTL_get_machine_address_size:
Simply obtaining the value set by XEN_DOMCTL_set_machine_address_size
(or the default set at domain creation time).
XEN_DOMCTL_gettscinfo:
XEN_DOMCTL_settscinfo:
Reading/writing of a couple of guest state fields wrapped in a
domain_{,un}pause() pair.
XEN_DOMCTL_audit_p2m:
Enabled only in debug builds.
XEN_DOMCTL_set_max_evtchn:
While the limit set here implies other (subsequent) resource usage,
this is the purpose of the operation.
I also verified that all removed domctls' handlers don't leak
hypervisor memory contents .
Inspected but questionable (and hence left in place for now):
XEN_DOMCTL_max_mem:
While only setting the field capping a domain's allocation (this
implies potential successive resource usage, but that's the purpose of
the operation). However, XSM doesn't see the value that's being set
here, so the net effect would be potential unbounded memory use.
XEN_DOMCTL_set_virq_handler:
This modifies a global array. While that is the purpose of the
operation, if multiple domains are granted permission they can badly
interfere with one another. Hence I'd appreciate a second opinion
here.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/docs/misc/xsm-flask.txt
+++ b/docs/misc/xsm-flask.txt
@@ -64,67 +64,39 @@ __HYPERVISOR_domctl (xen/include/public/
* XEN_DOMCTL_createdomain
* XEN_DOMCTL_destroydomain
- * XEN_DOMCTL_pausedomain
- * XEN_DOMCTL_unpausedomain
- * XEN_DOMCTL_getdomaininfo
* XEN_DOMCTL_getmemlist
- * XEN_DOMCTL_getpageframeinfo
- * XEN_DOMCTL_getpageframeinfo2
* XEN_DOMCTL_setvcpuaffinity
* XEN_DOMCTL_shadow_op
* XEN_DOMCTL_max_mem
* XEN_DOMCTL_setvcpucontext
* XEN_DOMCTL_getvcpucontext
- * XEN_DOMCTL_getvcpuinfo
* XEN_DOMCTL_max_vcpus
* XEN_DOMCTL_scheduler_op
- * XEN_DOMCTL_setdomainhandle
- * XEN_DOMCTL_setdebugging
* XEN_DOMCTL_irq_permission
* XEN_DOMCTL_iomem_permission
* XEN_DOMCTL_ioport_permission
- * XEN_DOMCTL_hypercall_init
- * XEN_DOMCTL_arch_setup
- * XEN_DOMCTL_settimeoffset
- * XEN_DOMCTL_getvcpuaffinity
- * XEN_DOMCTL_real_mode_area
- * XEN_DOMCTL_resumedomain
- * XEN_DOMCTL_sendtrigger
- * XEN_DOMCTL_subscribe
* XEN_DOMCTL_gethvmcontext
* XEN_DOMCTL_sethvmcontext
* XEN_DOMCTL_set_address_size
- * XEN_DOMCTL_get_address_size
* XEN_DOMCTL_assign_device
* XEN_DOMCTL_pin_mem_cacheattr
* XEN_DOMCTL_set_ext_vcpucontext
* XEN_DOMCTL_get_ext_vcpucontext
- * XEN_DOMCTL_set_opt_feature
* XEN_DOMCTL_test_assign_device
* XEN_DOMCTL_set_target
* XEN_DOMCTL_deassign_device
- * XEN_DOMCTL_set_cpuid
* XEN_DOMCTL_get_device_group
* XEN_DOMCTL_set_machine_address_size
- * XEN_DOMCTL_get_machine_address_size
- * XEN_DOMCTL_suppress_spurious_page_faults
* XEN_DOMCTL_debug_op
* XEN_DOMCTL_gethvmcontext_partial
* XEN_DOMCTL_mem_event_op
* XEN_DOMCTL_mem_sharing_op
- * XEN_DOMCTL_disable_migrate
- * XEN_DOMCTL_gettscinfo
- * XEN_DOMCTL_settscinfo
- * XEN_DOMCTL_getpageframeinfo3
* XEN_DOMCTL_setvcpuextstate
* XEN_DOMCTL_getvcpuextstate
* XEN_DOMCTL_set_access_required
- * XEN_DOMCTL_audit_p2m
* XEN_DOMCTL_set_virq_handler
* XEN_DOMCTL_set_broken_page_p2m
* XEN_DOMCTL_setnodeaffinity
- * XEN_DOMCTL_getnodeaffinity
- * XEN_DOMCTL_set_max_evtchn
* XEN_DOMCTL_gdbsx_guestmemio
* XEN_DOMCTL_gdbsx_pausevcpu
* XEN_DOMCTL_gdbsx_unpausevcpu
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -401,19 +401,6 @@ typedef struct xen_domctl_hypercall_init
DEFINE_XEN_GUEST_HANDLE(xen_domctl_hypercall_init_t);
-/* XEN_DOMCTL_arch_setup */
-#define _XEN_DOMAINSETUP_hvm_guest 0
-#define XEN_DOMAINSETUP_hvm_guest (1UL<<_XEN_DOMAINSETUP_hvm_guest)
-#define _XEN_DOMAINSETUP_query 1 /* Get parameters (for save) */
-#define XEN_DOMAINSETUP_query (1UL<<_XEN_DOMAINSETUP_query)
-#define _XEN_DOMAINSETUP_sioemu_guest 2
-#define XEN_DOMAINSETUP_sioemu_guest (1UL<<_XEN_DOMAINSETUP_sioemu_guest)
-typedef struct xen_domctl_arch_setup {
- uint64_aligned_t flags; /* XEN_DOMAINSETUP_* */
-} xen_domctl_arch_setup_t;
-DEFINE_XEN_GUEST_HANDLE(xen_domctl_arch_setup_t);
-
-
/* XEN_DOMCTL_settimeoffset */
struct xen_domctl_settimeoffset {
int32_t time_offset_seconds; /* applied to domain wallclock time */
@@ -440,14 +427,6 @@ typedef struct xen_domctl_address_size {
DEFINE_XEN_GUEST_HANDLE(xen_domctl_address_size_t);
-/* XEN_DOMCTL_real_mode_area */
-struct xen_domctl_real_mode_area {
- uint32_t log; /* log2 of Real Mode Area size */
-};
-typedef struct xen_domctl_real_mode_area xen_domctl_real_mode_area_t;
-DEFINE_XEN_GUEST_HANDLE(xen_domctl_real_mode_area_t);
-
-
/* XEN_DOMCTL_sendtrigger */
#define XEN_DOMCTL_SENDTRIGGER_NMI 0
#define XEN_DOMCTL_SENDTRIGGER_RESET 1
@@ -940,10 +919,10 @@ struct xen_domctl {
#define XEN_DOMCTL_iomem_permission 20
#define XEN_DOMCTL_ioport_permission 21
#define XEN_DOMCTL_hypercall_init 22
-#define XEN_DOMCTL_arch_setup 23
+#define XEN_DOMCTL_arch_setup 23 /* Obsolete IA64 only */
#define XEN_DOMCTL_settimeoffset 24
#define XEN_DOMCTL_getvcpuaffinity 25
-#define XEN_DOMCTL_real_mode_area 26
+#define XEN_DOMCTL_real_mode_area 26 /* Obsolete PPC only */
#define XEN_DOMCTL_resumedomain 27
#define XEN_DOMCTL_sendtrigger 28
#define XEN_DOMCTL_subscribe 29
@@ -1013,11 +992,9 @@ struct xen_domctl {
struct xen_domctl_iomem_permission iomem_permission;
struct xen_domctl_ioport_permission ioport_permission;
struct xen_domctl_hypercall_init hypercall_init;
- struct xen_domctl_arch_setup arch_setup;
struct xen_domctl_settimeoffset settimeoffset;
struct xen_domctl_disable_migrate disable_migrate;
struct xen_domctl_tsc_info tsc_info;
- struct xen_domctl_real_mode_area real_mode_area;
struct xen_domctl_hvmcontext hvmcontext;
struct xen_domctl_hvmcontext_partial hvmcontext_partial;
struct xen_domctl_address_size address_size;
[-- Attachment #2: domctl-simple.patch --]
[-- Type: text/plain, Size: 9396 bytes --]
domctl: perform initial post-XSA-77 auditing
In a number of cases, loops over each vCPU in a domain are involved
here. For large numbers of vCPU-s these may still take some time to
complete, but we're limiting them at a couple of thousand at most, so I
would think this should not by itself be an issue. I wonder though
whether it shouldn't be possible to have XSM restrict the vCPU count
that can be set through XEN_DOMCTL_max_vcpus.
XEN_DOMCTL_pausedomain:
A loop over vcpu_sleep_sync() for each of vCPU in the domain. That
function itself has a loop waiting for the subject vCPU to become non-
runnable, which ought to complete quickly (involving an IPI to be sent
and acted on). No other unbounded resource usage.
XEN_DOMCTL_unpausedomain:
Simply a loop calling vcpu_wake() (not having any loops or other
resource usage itself) for each of vCPU in the domain.
XEN_DOMCTL_getdomaininfo:
Two loops (one over all domains, i.e. bounded by the limit of 32k
domains, and another over all vCPU-s in the domain); no other
unbounded resource usage.
XEN_DOMCTL_getpageframeinfo:
Inquiring just a single MFN, i.e. no loops and no other unbounded
resource usage.
XEN_DOMCTL_getpageframeinfo{2,3}:
Number of inquired MFNs is limited to 1024. Beyond that just like
XEN_DOMCTL_getpageframeinfo.
XEN_DOMCTL_getvcpuinfo:
Only obtaining information on the vCPU, no loops or other resource
usage.
XEN_DOMCTL_setdomainhandle:
Simply a memcpy() of a very limited amount of data.
XEN_DOMCTL_setdebugging:
A domain_{,un}pause() pair (see XEN_DOMCTL_{,un}pausedomain) framing
the setting of a flag.
XEN_DOMCTL_hypercall_init:
Initializing a guest provided page with hypercall stubs. No other
resource consumption.
XEN_DOMCTL_arch_setup:
IA64 leftover, interface structure being removed from the public
header.
XEN_DOMCTL_settimeoffset:
Setting a couple of guest state fields. No other resource consumption.
XEN_DOMCTL_getvcpuaffinity:
XEN_DOMCTL_getnodeaffinity:
Involve temporary memory allocations (approximately) bounded by the
number of CPUs in the system / number of nodes built for, which is
okay. Beyond that trivial operation.
XEN_DOMCTL_real_mode_area:
PPC leftover, interface structure being removed from the public
header.
XEN_DOMCTL_resumedomain:
A domain_{,un}pause() pair framing operation very similar to
XEN_DOMCTL_unpausedomain (see above).
XEN_DOMCTL_sendtrigger:
Injects an interrupt (SCI or NMI) without any other resource
consumption.
XEN_DOMCTL_subscribe:
Updates the suspend event channel, i.e. affecting only the controlled
domain.
XEN_DOMCTL_disable_migrate:
XEN_DOMCTL_suppress_spurious_page_faults:
Just setting respective flags on the domain.
XEN_DOMCTL_get_address_size:
Simply reading the guest property.
XEN_DOMCTL_set_opt_feature:
Was already tagged IA64-only.
XEN_DOMCTL_set_cpuid:
MAX_CPUID_INPUT bounded loop, which is okay. No other resource
consumption.
XEN_DOMCTL_get_machine_address_size:
Simply obtaining the value set by XEN_DOMCTL_set_machine_address_size
(or the default set at domain creation time).
XEN_DOMCTL_gettscinfo:
XEN_DOMCTL_settscinfo:
Reading/writing of a couple of guest state fields wrapped in a
domain_{,un}pause() pair.
XEN_DOMCTL_audit_p2m:
Enabled only in debug builds.
XEN_DOMCTL_set_max_evtchn:
While the limit set here implies other (subsequent) resource usage,
this is the purpose of the operation.
I also verified that all removed domctls' handlers don't leak
hypervisor memory contents .
Inspected but questionable (and hence left in place for now):
XEN_DOMCTL_max_mem:
While only setting the field capping a domain's allocation (this
implies potential successive resource usage, but that's the purpose of
the operation). However, XSM doesn't see the value that's being set
here, so the net effect would be potential unbounded memory use.
XEN_DOMCTL_set_virq_handler:
This modifies a global array. While that is the purpose of the
operation, if multiple domains are granted permission they can badly
interfere with one another. Hence I'd appreciate a second opinion
here.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/docs/misc/xsm-flask.txt
+++ b/docs/misc/xsm-flask.txt
@@ -64,67 +64,39 @@ __HYPERVISOR_domctl (xen/include/public/
* XEN_DOMCTL_createdomain
* XEN_DOMCTL_destroydomain
- * XEN_DOMCTL_pausedomain
- * XEN_DOMCTL_unpausedomain
- * XEN_DOMCTL_getdomaininfo
* XEN_DOMCTL_getmemlist
- * XEN_DOMCTL_getpageframeinfo
- * XEN_DOMCTL_getpageframeinfo2
* XEN_DOMCTL_setvcpuaffinity
* XEN_DOMCTL_shadow_op
* XEN_DOMCTL_max_mem
* XEN_DOMCTL_setvcpucontext
* XEN_DOMCTL_getvcpucontext
- * XEN_DOMCTL_getvcpuinfo
* XEN_DOMCTL_max_vcpus
* XEN_DOMCTL_scheduler_op
- * XEN_DOMCTL_setdomainhandle
- * XEN_DOMCTL_setdebugging
* XEN_DOMCTL_irq_permission
* XEN_DOMCTL_iomem_permission
* XEN_DOMCTL_ioport_permission
- * XEN_DOMCTL_hypercall_init
- * XEN_DOMCTL_arch_setup
- * XEN_DOMCTL_settimeoffset
- * XEN_DOMCTL_getvcpuaffinity
- * XEN_DOMCTL_real_mode_area
- * XEN_DOMCTL_resumedomain
- * XEN_DOMCTL_sendtrigger
- * XEN_DOMCTL_subscribe
* XEN_DOMCTL_gethvmcontext
* XEN_DOMCTL_sethvmcontext
* XEN_DOMCTL_set_address_size
- * XEN_DOMCTL_get_address_size
* XEN_DOMCTL_assign_device
* XEN_DOMCTL_pin_mem_cacheattr
* XEN_DOMCTL_set_ext_vcpucontext
* XEN_DOMCTL_get_ext_vcpucontext
- * XEN_DOMCTL_set_opt_feature
* XEN_DOMCTL_test_assign_device
* XEN_DOMCTL_set_target
* XEN_DOMCTL_deassign_device
- * XEN_DOMCTL_set_cpuid
* XEN_DOMCTL_get_device_group
* XEN_DOMCTL_set_machine_address_size
- * XEN_DOMCTL_get_machine_address_size
- * XEN_DOMCTL_suppress_spurious_page_faults
* XEN_DOMCTL_debug_op
* XEN_DOMCTL_gethvmcontext_partial
* XEN_DOMCTL_mem_event_op
* XEN_DOMCTL_mem_sharing_op
- * XEN_DOMCTL_disable_migrate
- * XEN_DOMCTL_gettscinfo
- * XEN_DOMCTL_settscinfo
- * XEN_DOMCTL_getpageframeinfo3
* XEN_DOMCTL_setvcpuextstate
* XEN_DOMCTL_getvcpuextstate
* XEN_DOMCTL_set_access_required
- * XEN_DOMCTL_audit_p2m
* XEN_DOMCTL_set_virq_handler
* XEN_DOMCTL_set_broken_page_p2m
* XEN_DOMCTL_setnodeaffinity
- * XEN_DOMCTL_getnodeaffinity
- * XEN_DOMCTL_set_max_evtchn
* XEN_DOMCTL_gdbsx_guestmemio
* XEN_DOMCTL_gdbsx_pausevcpu
* XEN_DOMCTL_gdbsx_unpausevcpu
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -401,19 +401,6 @@ typedef struct xen_domctl_hypercall_init
DEFINE_XEN_GUEST_HANDLE(xen_domctl_hypercall_init_t);
-/* XEN_DOMCTL_arch_setup */
-#define _XEN_DOMAINSETUP_hvm_guest 0
-#define XEN_DOMAINSETUP_hvm_guest (1UL<<_XEN_DOMAINSETUP_hvm_guest)
-#define _XEN_DOMAINSETUP_query 1 /* Get parameters (for save) */
-#define XEN_DOMAINSETUP_query (1UL<<_XEN_DOMAINSETUP_query)
-#define _XEN_DOMAINSETUP_sioemu_guest 2
-#define XEN_DOMAINSETUP_sioemu_guest (1UL<<_XEN_DOMAINSETUP_sioemu_guest)
-typedef struct xen_domctl_arch_setup {
- uint64_aligned_t flags; /* XEN_DOMAINSETUP_* */
-} xen_domctl_arch_setup_t;
-DEFINE_XEN_GUEST_HANDLE(xen_domctl_arch_setup_t);
-
-
/* XEN_DOMCTL_settimeoffset */
struct xen_domctl_settimeoffset {
int32_t time_offset_seconds; /* applied to domain wallclock time */
@@ -440,14 +427,6 @@ typedef struct xen_domctl_address_size {
DEFINE_XEN_GUEST_HANDLE(xen_domctl_address_size_t);
-/* XEN_DOMCTL_real_mode_area */
-struct xen_domctl_real_mode_area {
- uint32_t log; /* log2 of Real Mode Area size */
-};
-typedef struct xen_domctl_real_mode_area xen_domctl_real_mode_area_t;
-DEFINE_XEN_GUEST_HANDLE(xen_domctl_real_mode_area_t);
-
-
/* XEN_DOMCTL_sendtrigger */
#define XEN_DOMCTL_SENDTRIGGER_NMI 0
#define XEN_DOMCTL_SENDTRIGGER_RESET 1
@@ -940,10 +919,10 @@ struct xen_domctl {
#define XEN_DOMCTL_iomem_permission 20
#define XEN_DOMCTL_ioport_permission 21
#define XEN_DOMCTL_hypercall_init 22
-#define XEN_DOMCTL_arch_setup 23
+#define XEN_DOMCTL_arch_setup 23 /* Obsolete IA64 only */
#define XEN_DOMCTL_settimeoffset 24
#define XEN_DOMCTL_getvcpuaffinity 25
-#define XEN_DOMCTL_real_mode_area 26
+#define XEN_DOMCTL_real_mode_area 26 /* Obsolete PPC only */
#define XEN_DOMCTL_resumedomain 27
#define XEN_DOMCTL_sendtrigger 28
#define XEN_DOMCTL_subscribe 29
@@ -1013,11 +992,9 @@ struct xen_domctl {
struct xen_domctl_iomem_permission iomem_permission;
struct xen_domctl_ioport_permission ioport_permission;
struct xen_domctl_hypercall_init hypercall_init;
- struct xen_domctl_arch_setup arch_setup;
struct xen_domctl_settimeoffset settimeoffset;
struct xen_domctl_disable_migrate disable_migrate;
struct xen_domctl_tsc_info tsc_info;
- struct xen_domctl_real_mode_area real_mode_area;
struct xen_domctl_hvmcontext hvmcontext;
struct xen_domctl_hvmcontext_partial hvmcontext_partial;
struct xen_domctl_address_size address_size;
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] domctl: perform initial post-XSA-77 auditing
2014-04-30 14:23 ` [PATCH 2/3] domctl: perform initial post-XSA-77 auditing Jan Beulich
@ 2014-04-30 16:52 ` Andrew Cooper
2014-05-01 15:30 ` Tim Deegan
1 sibling, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2014-04-30 16:52 UTC (permalink / raw)
To: Jan Beulich; +Cc: Ian Campbell, xen-devel, Keir Fraser, Ian Jackson, Tim Deegan
[-- Attachment #1.1: Type: text/plain, Size: 10372 bytes --]
On 30/04/14 15:23, Jan Beulich wrote:
> In a number of cases, loops over each vCPU in a domain are involved
> here. For large numbers of vCPU-s these may still take some time to
> complete, but we're limiting them at a couple of thousand at most, so I
> would think this should not by itself be an issue. I wonder though
> whether it shouldn't be possible to have XSM restrict the vCPU count
> that can be set through XEN_DOMCTL_max_vcpus.
>
> XEN_DOMCTL_pausedomain:
>
> A loop over vcpu_sleep_sync() for each of vCPU in the domain. That
> function itself has a loop waiting for the subject vCPU to become non-
> runnable, which ought to complete quickly (involving an IPI to be sent
> and acted on). No other unbounded resource usage.
>
> XEN_DOMCTL_unpausedomain:
>
> Simply a loop calling vcpu_wake() (not having any loops or other
> resource usage itself) for each of vCPU in the domain.
>
> XEN_DOMCTL_getdomaininfo:
>
> Two loops (one over all domains, i.e. bounded by the limit of 32k
> domains, and another over all vCPU-s in the domain); no other
> unbounded resource usage.
>
> XEN_DOMCTL_getpageframeinfo:
>
> Inquiring just a single MFN, i.e. no loops and no other unbounded
> resource usage.
>
> XEN_DOMCTL_getpageframeinfo{2,3}:
>
> Number of inquired MFNs is limited to 1024. Beyond that just like
> XEN_DOMCTL_getpageframeinfo.
>
> XEN_DOMCTL_getvcpuinfo:
>
> Only obtaining information on the vCPU, no loops or other resource
> usage.
>
> XEN_DOMCTL_setdomainhandle:
>
> Simply a memcpy() of a very limited amount of data.
>
> XEN_DOMCTL_setdebugging:
>
> A domain_{,un}pause() pair (see XEN_DOMCTL_{,un}pausedomain) framing
> the setting of a flag.
>
> XEN_DOMCTL_hypercall_init:
>
> Initializing a guest provided page with hypercall stubs. No other
> resource consumption.
>
> XEN_DOMCTL_arch_setup:
>
> IA64 leftover, interface structure being removed from the public
> header.
>
> XEN_DOMCTL_settimeoffset:
>
> Setting a couple of guest state fields. No other resource consumption.
>
> XEN_DOMCTL_getvcpuaffinity:
> XEN_DOMCTL_getnodeaffinity:
>
> Involve temporary memory allocations (approximately) bounded by the
> number of CPUs in the system / number of nodes built for, which is
> okay. Beyond that trivial operation.
>
> XEN_DOMCTL_real_mode_area:
>
> PPC leftover, interface structure being removed from the public
> header.
>
> XEN_DOMCTL_resumedomain:
>
> A domain_{,un}pause() pair framing operation very similar to
> XEN_DOMCTL_unpausedomain (see above).
>
> XEN_DOMCTL_sendtrigger:
>
> Injects an interrupt (SCI or NMI) without any other resource
> consumption.
>
> XEN_DOMCTL_subscribe:
>
> Updates the suspend event channel, i.e. affecting only the controlled
> domain.
>
> XEN_DOMCTL_disable_migrate:
> XEN_DOMCTL_suppress_spurious_page_faults:
>
> Just setting respective flags on the domain.
>
> XEN_DOMCTL_get_address_size:
>
> Simply reading the guest property.
>
> XEN_DOMCTL_set_opt_feature:
>
> Was already tagged IA64-only.
>
> XEN_DOMCTL_set_cpuid:
>
> MAX_CPUID_INPUT bounded loop, which is okay. No other resource
> consumption.
>
> XEN_DOMCTL_get_machine_address_size:
>
> Simply obtaining the value set by XEN_DOMCTL_set_machine_address_size
> (or the default set at domain creation time).
>
> XEN_DOMCTL_gettscinfo:
> XEN_DOMCTL_settscinfo:
>
> Reading/writing of a couple of guest state fields wrapped in a
> domain_{,un}pause() pair.
>
> XEN_DOMCTL_audit_p2m:
>
> Enabled only in debug builds.
>
> XEN_DOMCTL_set_max_evtchn:
>
> While the limit set here implies other (subsequent) resource usage,
> this is the purpose of the operation.
>
> I also verified that all removed domctls' handlers don't leak
> hypervisor memory contents .
>
> Inspected but questionable (and hence left in place for now):
>
> XEN_DOMCTL_max_mem:
>
> While only setting the field capping a domain's allocation (this
> implies potential successive resource usage, but that's the purpose of
> the operation). However, XSM doesn't see the value that's being set
> here, so the net effect would be potential unbounded memory use.
It is possibly a good thing for dom0/etc to set a limit which
domain-builder domains can't exceed.
>
> XEN_DOMCTL_set_virq_handler:
>
> This modifies a global array. While that is the purpose of the
> operation, if multiple domains are granted permission they can badly
> interfere with one another. Hence I'd appreciate a second opinion
> here.
Do you mean by domains nabbing each others ownership of global virqs?
Global virqs are all system level things. They are not needed for
disaggregated domain-builder domains or device-model domains.
If the core toolstack has been disaggregated to the point at which virqs
are going to different domains, and you can't trust those domains to
play nicely together, there are far larger problems than can be done by
this hypercall.
I think it is fine as-is.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> --- a/docs/misc/xsm-flask.txt
> +++ b/docs/misc/xsm-flask.txt
> @@ -64,67 +64,39 @@ __HYPERVISOR_domctl (xen/include/public/
>
> * XEN_DOMCTL_createdomain
> * XEN_DOMCTL_destroydomain
> - * XEN_DOMCTL_pausedomain
> - * XEN_DOMCTL_unpausedomain
> - * XEN_DOMCTL_getdomaininfo
> * XEN_DOMCTL_getmemlist
> - * XEN_DOMCTL_getpageframeinfo
> - * XEN_DOMCTL_getpageframeinfo2
> * XEN_DOMCTL_setvcpuaffinity
> * XEN_DOMCTL_shadow_op
> * XEN_DOMCTL_max_mem
> * XEN_DOMCTL_setvcpucontext
> * XEN_DOMCTL_getvcpucontext
> - * XEN_DOMCTL_getvcpuinfo
> * XEN_DOMCTL_max_vcpus
> * XEN_DOMCTL_scheduler_op
> - * XEN_DOMCTL_setdomainhandle
> - * XEN_DOMCTL_setdebugging
> * XEN_DOMCTL_irq_permission
> * XEN_DOMCTL_iomem_permission
> * XEN_DOMCTL_ioport_permission
> - * XEN_DOMCTL_hypercall_init
> - * XEN_DOMCTL_arch_setup
> - * XEN_DOMCTL_settimeoffset
> - * XEN_DOMCTL_getvcpuaffinity
> - * XEN_DOMCTL_real_mode_area
> - * XEN_DOMCTL_resumedomain
> - * XEN_DOMCTL_sendtrigger
> - * XEN_DOMCTL_subscribe
> * XEN_DOMCTL_gethvmcontext
> * XEN_DOMCTL_sethvmcontext
> * XEN_DOMCTL_set_address_size
> - * XEN_DOMCTL_get_address_size
> * XEN_DOMCTL_assign_device
> * XEN_DOMCTL_pin_mem_cacheattr
> * XEN_DOMCTL_set_ext_vcpucontext
> * XEN_DOMCTL_get_ext_vcpucontext
> - * XEN_DOMCTL_set_opt_feature
> * XEN_DOMCTL_test_assign_device
> * XEN_DOMCTL_set_target
> * XEN_DOMCTL_deassign_device
> - * XEN_DOMCTL_set_cpuid
> * XEN_DOMCTL_get_device_group
> * XEN_DOMCTL_set_machine_address_size
> - * XEN_DOMCTL_get_machine_address_size
> - * XEN_DOMCTL_suppress_spurious_page_faults
> * XEN_DOMCTL_debug_op
> * XEN_DOMCTL_gethvmcontext_partial
> * XEN_DOMCTL_mem_event_op
> * XEN_DOMCTL_mem_sharing_op
> - * XEN_DOMCTL_disable_migrate
> - * XEN_DOMCTL_gettscinfo
> - * XEN_DOMCTL_settscinfo
> - * XEN_DOMCTL_getpageframeinfo3
> * XEN_DOMCTL_setvcpuextstate
> * XEN_DOMCTL_getvcpuextstate
> * XEN_DOMCTL_set_access_required
> - * XEN_DOMCTL_audit_p2m
> * XEN_DOMCTL_set_virq_handler
> * XEN_DOMCTL_set_broken_page_p2m
> * XEN_DOMCTL_setnodeaffinity
> - * XEN_DOMCTL_getnodeaffinity
> - * XEN_DOMCTL_set_max_evtchn
> * XEN_DOMCTL_gdbsx_guestmemio
> * XEN_DOMCTL_gdbsx_pausevcpu
> * XEN_DOMCTL_gdbsx_unpausevcpu
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -401,19 +401,6 @@ typedef struct xen_domctl_hypercall_init
> DEFINE_XEN_GUEST_HANDLE(xen_domctl_hypercall_init_t);
>
>
> -/* XEN_DOMCTL_arch_setup */
> -#define _XEN_DOMAINSETUP_hvm_guest 0
> -#define XEN_DOMAINSETUP_hvm_guest (1UL<<_XEN_DOMAINSETUP_hvm_guest)
> -#define _XEN_DOMAINSETUP_query 1 /* Get parameters (for save) */
> -#define XEN_DOMAINSETUP_query (1UL<<_XEN_DOMAINSETUP_query)
> -#define _XEN_DOMAINSETUP_sioemu_guest 2
> -#define XEN_DOMAINSETUP_sioemu_guest (1UL<<_XEN_DOMAINSETUP_sioemu_guest)
> -typedef struct xen_domctl_arch_setup {
> - uint64_aligned_t flags; /* XEN_DOMAINSETUP_* */
> -} xen_domctl_arch_setup_t;
> -DEFINE_XEN_GUEST_HANDLE(xen_domctl_arch_setup_t);
> -
> -
> /* XEN_DOMCTL_settimeoffset */
> struct xen_domctl_settimeoffset {
> int32_t time_offset_seconds; /* applied to domain wallclock time */
> @@ -440,14 +427,6 @@ typedef struct xen_domctl_address_size {
> DEFINE_XEN_GUEST_HANDLE(xen_domctl_address_size_t);
>
>
> -/* XEN_DOMCTL_real_mode_area */
> -struct xen_domctl_real_mode_area {
> - uint32_t log; /* log2 of Real Mode Area size */
> -};
> -typedef struct xen_domctl_real_mode_area xen_domctl_real_mode_area_t;
> -DEFINE_XEN_GUEST_HANDLE(xen_domctl_real_mode_area_t);
> -
> -
> /* XEN_DOMCTL_sendtrigger */
> #define XEN_DOMCTL_SENDTRIGGER_NMI 0
> #define XEN_DOMCTL_SENDTRIGGER_RESET 1
> @@ -940,10 +919,10 @@ struct xen_domctl {
> #define XEN_DOMCTL_iomem_permission 20
> #define XEN_DOMCTL_ioport_permission 21
> #define XEN_DOMCTL_hypercall_init 22
> -#define XEN_DOMCTL_arch_setup 23
> +#define XEN_DOMCTL_arch_setup 23 /* Obsolete IA64 only */
> #define XEN_DOMCTL_settimeoffset 24
> #define XEN_DOMCTL_getvcpuaffinity 25
> -#define XEN_DOMCTL_real_mode_area 26
> +#define XEN_DOMCTL_real_mode_area 26 /* Obsolete PPC only */
> #define XEN_DOMCTL_resumedomain 27
> #define XEN_DOMCTL_sendtrigger 28
> #define XEN_DOMCTL_subscribe 29
> @@ -1013,11 +992,9 @@ struct xen_domctl {
> struct xen_domctl_iomem_permission iomem_permission;
> struct xen_domctl_ioport_permission ioport_permission;
> struct xen_domctl_hypercall_init hypercall_init;
> - struct xen_domctl_arch_setup arch_setup;
> struct xen_domctl_settimeoffset settimeoffset;
> struct xen_domctl_disable_migrate disable_migrate;
> struct xen_domctl_tsc_info tsc_info;
> - struct xen_domctl_real_mode_area real_mode_area;
> struct xen_domctl_hvmcontext hvmcontext;
> struct xen_domctl_hvmcontext_partial hvmcontext_partial;
> struct xen_domctl_address_size address_size;
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
[-- Attachment #1.2: Type: text/html, Size: 11197 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] domctl: perform initial post-XSA-77 auditing
2014-04-30 14:23 ` [PATCH 2/3] domctl: perform initial post-XSA-77 auditing Jan Beulich
2014-04-30 16:52 ` Andrew Cooper
@ 2014-05-01 15:30 ` Tim Deegan
1 sibling, 0 replies; 14+ messages in thread
From: Tim Deegan @ 2014-05-01 15:30 UTC (permalink / raw)
To: Jan Beulich; +Cc: Ian Campbell, xen-devel, Keir Fraser, Ian Jackson
At 15:23 +0100 on 30 Apr (1398867811), Jan Beulich wrote:
> In a number of cases, loops over each vCPU in a domain are involved
> here. For large numbers of vCPU-s these may still take some time to
> complete, but we're limiting them at a couple of thousand at most, so I
> would think this should not by itself be an issue. I wonder though
> whether it shouldn't be possible to have XSM restrict the vCPU count
> that can be set through XEN_DOMCTL_max_vcpus.
[...]
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Tim Deegan <tim@xen.org>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] domctl: tighten XEN_DOMCTL_*_permission
2014-04-30 14:11 [PATCH 0/3] domctl related adjustments Jan Beulich
2014-04-30 14:22 ` [PATCH 1/3] x86: fix guest CPUID handling Jan Beulich
2014-04-30 14:23 ` [PATCH 2/3] domctl: perform initial post-XSA-77 auditing Jan Beulich
@ 2014-04-30 14:24 ` Jan Beulich
2014-04-30 17:17 ` Andrew Cooper
2014-05-01 15:37 ` Tim Deegan
2 siblings, 2 replies; 14+ messages in thread
From: Jan Beulich @ 2014-04-30 14:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Campbell, Keir Fraser, Ian Jackson, Tim Deegan
[-- Attachment #1: Type: text/plain, Size: 3134 bytes --]
With proper permission (and, for the I/O port case, wrap-around) checks
added (note that for the I/O port case a count of zero is now being
disallowed, in line with I/O memory handling):
XEN_DOMCTL_irq_permission:
XEN_DOMCTL_ioport_permission:
Of both IRQs and I/O ports there is only a reasonably small amount, so
there's no excess resource consumption involved here. Additionally
they both have a specialized XSM hook associated.
XEN_DOMCTL_iomem_permission:
While this also has a specialized XSM hook associated (just like
XEN_DOMCTL_{irq,ioport}_permission), it's not clear whether it's
reasonable to expect XSM to restrict the number of ranges associated
with a domain via this hook (which is the main resource consumption
item here).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/docs/misc/xsm-flask.txt
+++ b/docs/misc/xsm-flask.txt
@@ -72,9 +72,7 @@ __HYPERVISOR_domctl (xen/include/public/
* XEN_DOMCTL_getvcpucontext
* XEN_DOMCTL_max_vcpus
* XEN_DOMCTL_scheduler_op
- * XEN_DOMCTL_irq_permission
* XEN_DOMCTL_iomem_permission
- * XEN_DOMCTL_ioport_permission
* XEN_DOMCTL_gethvmcontext
* XEN_DOMCTL_sethvmcontext
* XEN_DOMCTL_set_address_size
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -72,13 +72,11 @@ long arch_do_domctl(
unsigned int np = domctl->u.ioport_permission.nr_ports;
int allow = domctl->u.ioport_permission.allow_access;
- ret = -EINVAL;
- if ( (fp + np) > 65536 )
- break;
-
- if ( np == 0 )
- ret = 0;
- else if ( xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
+ if ( (fp + np - 1) < fp || (fp + np) > 0x10000 )
+ ret = -EINVAL;
+ else if ( !ioports_access_permitted(current->domain,
+ fp, fp + np - 1) ||
+ xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
ret = -EPERM;
else if ( allow )
ret = ioports_permit_access(d, fp, fp + np - 1);
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -790,7 +790,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
if ( pirq >= d->nr_pirqs )
ret = -EINVAL;
- else if ( xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
+ else if ( !pirq_access_permitted(current->domain, pirq) ||
+ xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
ret = -EPERM;
else if ( allow )
ret = pirq_permit_access(d, pirq);
@@ -809,7 +810,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
if ( (mfn + nr_mfns - 1) < mfn ) /* wrap? */
break;
- if ( xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, allow) )
+ if ( !iomem_access_permitted(current->domain,
+ mfn, mfn + nr_mfns - 1) ||
+ xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, allow) )
ret = -EPERM;
else if ( allow )
ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);
[-- Attachment #2: domctl-permit-access.patch --]
[-- Type: text/plain, Size: 3171 bytes --]
domctl: tighten XEN_DOMCTL_*_permission
With proper permission (and, for the I/O port case, wrap-around) checks
added (note that for the I/O port case a count of zero is now being
disallowed, in line with I/O memory handling):
XEN_DOMCTL_irq_permission:
XEN_DOMCTL_ioport_permission:
Of both IRQs and I/O ports there is only a reasonably small amount, so
there's no excess resource consumption involved here. Additionally
they both have a specialized XSM hook associated.
XEN_DOMCTL_iomem_permission:
While this also has a specialized XSM hook associated (just like
XEN_DOMCTL_{irq,ioport}_permission), it's not clear whether it's
reasonable to expect XSM to restrict the number of ranges associated
with a domain via this hook (which is the main resource consumption
item here).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/docs/misc/xsm-flask.txt
+++ b/docs/misc/xsm-flask.txt
@@ -72,9 +72,7 @@ __HYPERVISOR_domctl (xen/include/public/
* XEN_DOMCTL_getvcpucontext
* XEN_DOMCTL_max_vcpus
* XEN_DOMCTL_scheduler_op
- * XEN_DOMCTL_irq_permission
* XEN_DOMCTL_iomem_permission
- * XEN_DOMCTL_ioport_permission
* XEN_DOMCTL_gethvmcontext
* XEN_DOMCTL_sethvmcontext
* XEN_DOMCTL_set_address_size
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -72,13 +72,11 @@ long arch_do_domctl(
unsigned int np = domctl->u.ioport_permission.nr_ports;
int allow = domctl->u.ioport_permission.allow_access;
- ret = -EINVAL;
- if ( (fp + np) > 65536 )
- break;
-
- if ( np == 0 )
- ret = 0;
- else if ( xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
+ if ( (fp + np - 1) < fp || (fp + np) > 0x10000 )
+ ret = -EINVAL;
+ else if ( !ioports_access_permitted(current->domain,
+ fp, fp + np - 1) ||
+ xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
ret = -EPERM;
else if ( allow )
ret = ioports_permit_access(d, fp, fp + np - 1);
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -790,7 +790,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
if ( pirq >= d->nr_pirqs )
ret = -EINVAL;
- else if ( xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
+ else if ( !pirq_access_permitted(current->domain, pirq) ||
+ xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
ret = -EPERM;
else if ( allow )
ret = pirq_permit_access(d, pirq);
@@ -809,7 +810,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
if ( (mfn + nr_mfns - 1) < mfn ) /* wrap? */
break;
- if ( xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, allow) )
+ if ( !iomem_access_permitted(current->domain,
+ mfn, mfn + nr_mfns - 1) ||
+ xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, allow) )
ret = -EPERM;
else if ( allow )
ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 3/3] domctl: tighten XEN_DOMCTL_*_permission
2014-04-30 14:24 ` [PATCH 3/3] domctl: tighten XEN_DOMCTL_*_permission Jan Beulich
@ 2014-04-30 17:17 ` Andrew Cooper
2014-05-02 7:49 ` Jan Beulich
2014-05-01 15:37 ` Tim Deegan
1 sibling, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2014-04-30 17:17 UTC (permalink / raw)
To: Jan Beulich; +Cc: Ian Campbell, xen-devel, Keir Fraser, Ian Jackson, Tim Deegan
[-- Attachment #1.1: Type: text/plain, Size: 3811 bytes --]
On 30/04/14 15:24, Jan Beulich wrote:
> With proper permission (and, for the I/O port case, wrap-around) checks
> added (note that for the I/O port case a count of zero is now being
> disallowed, in line with I/O memory handling):
>
> XEN_DOMCTL_irq_permission:
> XEN_DOMCTL_ioport_permission:
>
> Of both IRQs and I/O ports there is only a reasonably small amount, so
> there's no excess resource consumption involved here. Additionally
> they both have a specialized XSM hook associated.
>
> XEN_DOMCTL_iomem_permission:
>
> While this also has a specialized XSM hook associated (just like
> XEN_DOMCTL_{irq,ioport}_permission), it's not clear whether it's
> reasonable to expect XSM to restrict the number of ranges associated
> with a domain via this hook (which is the main resource consumption
> item here).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/docs/misc/xsm-flask.txt
> +++ b/docs/misc/xsm-flask.txt
> @@ -72,9 +72,7 @@ __HYPERVISOR_domctl (xen/include/public/
> * XEN_DOMCTL_getvcpucontext
> * XEN_DOMCTL_max_vcpus
> * XEN_DOMCTL_scheduler_op
> - * XEN_DOMCTL_irq_permission
> * XEN_DOMCTL_iomem_permission
> - * XEN_DOMCTL_ioport_permission
> * XEN_DOMCTL_gethvmcontext
> * XEN_DOMCTL_sethvmcontext
> * XEN_DOMCTL_set_address_size
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -72,13 +72,11 @@ long arch_do_domctl(
> unsigned int np = domctl->u.ioport_permission.nr_ports;
> int allow = domctl->u.ioport_permission.allow_access;
>
> - ret = -EINVAL;
> - if ( (fp + np) > 65536 )
> - break;
> -
> - if ( np == 0 )
> - ret = 0;
> - else if ( xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
> + if ( (fp + np - 1) < fp || (fp + np) > 0x10000 )
> + ret = -EINVAL;
> + else if ( !ioports_access_permitted(current->domain,
> + fp, fp + np - 1) ||
> + xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
I don't see what the ioport permissions of the current domain have to do
with whether a domain is permitted to change the permissions for another
domain.
I would expect that any domain builder domains would have no ioport
permissions at all, which would cause this hypercall to unconditionally
fail with -EPERM even if the domain builder domain is permitted to
assign ioport permissions to the domain it is building.
~Andrew
> ret = -EPERM;
> else if ( allow )
> ret = ioports_permit_access(d, fp, fp + np - 1);
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -790,7 +790,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>
> if ( pirq >= d->nr_pirqs )
> ret = -EINVAL;
> - else if ( xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
> + else if ( !pirq_access_permitted(current->domain, pirq) ||
> + xsm_irq_permission(XSM_HOOK, d, pirq, allow) )
> ret = -EPERM;
> else if ( allow )
> ret = pirq_permit_access(d, pirq);
> @@ -809,7 +810,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
> if ( (mfn + nr_mfns - 1) < mfn ) /* wrap? */
> break;
>
> - if ( xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, allow) )
> + if ( !iomem_access_permitted(current->domain,
> + mfn, mfn + nr_mfns - 1) ||
> + xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, allow) )
> ret = -EPERM;
> else if ( allow )
> ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
[-- Attachment #1.2: Type: text/html, Size: 4591 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 3/3] domctl: tighten XEN_DOMCTL_*_permission
2014-04-30 17:17 ` Andrew Cooper
@ 2014-05-02 7:49 ` Jan Beulich
2014-05-02 8:19 ` Ian Campbell
0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2014-05-02 7:49 UTC (permalink / raw)
To: Andrew Cooper
Cc: Ian Campbell, xen-devel, Keir Fraser, Ian Jackson, Tim Deegan
>>> On 30.04.14 at 19:17, <andrew.cooper3@citrix.com> wrote:
> On 30/04/14 15:24, Jan Beulich wrote:
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -72,13 +72,11 @@ long arch_do_domctl(
>> unsigned int np = domctl->u.ioport_permission.nr_ports;
>> int allow = domctl->u.ioport_permission.allow_access;
>>
>> - ret = -EINVAL;
>> - if ( (fp + np) > 65536 )
>> - break;
>> -
>> - if ( np == 0 )
>> - ret = 0;
>> - else if ( xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow)
> )
>> + if ( (fp + np - 1) < fp || (fp + np) > 0x10000 )
>> + ret = -EINVAL;
>> + else if ( !ioports_access_permitted(current->domain,
>> + fp, fp + np - 1) ||
>> + xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
>
> I don't see what the ioport permissions of the current domain have to do
> with whether a domain is permitted to change the permissions for another
> domain.
>
> I would expect that any domain builder domains would have no ioport
> permissions at all, which would cause this hypercall to unconditionally
> fail with -EPERM even if the domain builder domain is permitted to
> assign ioport permissions to the domain it is building.
My perspective is quite different - a building/controlling domain shouldn't
be able to assign any resources it doesn't itself have control over. Which
matches up with XEN_DOMCTL_{memory,ioport}_mapping, which too
check requester permissions before doing the permission assignment
(albeit there seems to be a tendency to agreement that the combination
of permission granting and mapping is undesirable).
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 3/3] domctl: tighten XEN_DOMCTL_*_permission
2014-05-02 7:49 ` Jan Beulich
@ 2014-05-02 8:19 ` Ian Campbell
2014-05-02 20:34 ` Daniel De Graaf
0 siblings, 1 reply; 14+ messages in thread
From: Ian Campbell @ 2014-05-02 8:19 UTC (permalink / raw)
To: Jan Beulich
Cc: Keir Fraser, Andrew Cooper, Tim Deegan, xen-devel,
Daniel De Graaf, Ian Jackson
On Fri, 2014-05-02 at 08:49 +0100, Jan Beulich wrote:
> >>> On 30.04.14 at 19:17, <andrew.cooper3@citrix.com> wrote:
> > On 30/04/14 15:24, Jan Beulich wrote:
> >> --- a/xen/arch/x86/domctl.c
> >> +++ b/xen/arch/x86/domctl.c
> >> @@ -72,13 +72,11 @@ long arch_do_domctl(
> >> unsigned int np = domctl->u.ioport_permission.nr_ports;
> >> int allow = domctl->u.ioport_permission.allow_access;
> >>
> >> - ret = -EINVAL;
> >> - if ( (fp + np) > 65536 )
> >> - break;
> >> -
> >> - if ( np == 0 )
> >> - ret = 0;
> >> - else if ( xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow)
> > )
> >> + if ( (fp + np - 1) < fp || (fp + np) > 0x10000 )
> >> + ret = -EINVAL;
> >> + else if ( !ioports_access_permitted(current->domain,
> >> + fp, fp + np - 1) ||
> >> + xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
> >
> > I don't see what the ioport permissions of the current domain have to do
> > with whether a domain is permitted to change the permissions for another
> > domain.
> >
> > I would expect that any domain builder domains would have no ioport
> > permissions at all, which would cause this hypercall to unconditionally
> > fail with -EPERM even if the domain builder domain is permitted to
> > assign ioport permissions to the domain it is building.
>
> My perspective is quite different - a building/controlling domain shouldn't
> be able to assign any resources it doesn't itself have control over. Which
> matches up with XEN_DOMCTL_{memory,ioport}_mapping, which too
> check requester permissions before doing the permission assignment
> (albeit there seems to be a tendency to agreement that the combination
> of permission granting and mapping is undesirable).
AIUI that's pretty much how Daniel explained it to me as well.
Ian.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 3/3] domctl: tighten XEN_DOMCTL_*_permission
2014-05-02 8:19 ` Ian Campbell
@ 2014-05-02 20:34 ` Daniel De Graaf
0 siblings, 0 replies; 14+ messages in thread
From: Daniel De Graaf @ 2014-05-02 20:34 UTC (permalink / raw)
To: Ian Campbell, Jan Beulich
Cc: Andrew Cooper, Tim Deegan, Keir Fraser, Ian Jackson, xen-devel
On 05/02/2014 04:19 AM, Ian Campbell wrote:
> On Fri, 2014-05-02 at 08:49 +0100, Jan Beulich wrote:
>>>>> On 30.04.14 at 19:17, <andrew.cooper3@citrix.com> wrote:
>>> On 30/04/14 15:24, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/domctl.c
>>>> +++ b/xen/arch/x86/domctl.c
>>>> @@ -72,13 +72,11 @@ long arch_do_domctl(
>>>> unsigned int np = domctl->u.ioport_permission.nr_ports;
>>>> int allow = domctl->u.ioport_permission.allow_access;
>>>>
>>>> - ret = -EINVAL;
>>>> - if ( (fp + np) > 65536 )
>>>> - break;
>>>> -
>>>> - if ( np == 0 )
>>>> - ret = 0;
>>>> - else if ( xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow)
>>> )
>>>> + if ( (fp + np - 1) < fp || (fp + np) > 0x10000 )
>>>> + ret = -EINVAL;
>>>> + else if ( !ioports_access_permitted(current->domain,
>>>> + fp, fp + np - 1) ||
>>>> + xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
>>>
>>> I don't see what the ioport permissions of the current domain have to do
>>> with whether a domain is permitted to change the permissions for another
>>> domain.
>>>
>>> I would expect that any domain builder domains would have no ioport
>>> permissions at all, which would cause this hypercall to unconditionally
>>> fail with -EPERM even if the domain builder domain is permitted to
>>> assign ioport permissions to the domain it is building.
>>
>> My perspective is quite different - a building/controlling domain shouldn't
>> be able to assign any resources it doesn't itself have control over. Which
>> matches up with XEN_DOMCTL_{memory,ioport}_mapping, which too
>> check requester permissions before doing the permission assignment
>> (albeit there seems to be a tendency to agreement that the combination
>> of permission granting and mapping is undesirable).
>
> AIUI that's pretty much how Daniel explained it to me as well.
>
> Ian.
This is my understanding of the problem and the motivation for choosing
to require that the domain builder have access to resources it grants:
If a domain D has permission to use the XEN_DOMCTL_*_permission
hypercalls on a domain T, then D can try to grant T access to any
resource R on the platform. If an XSM policy is used, the XSM policy
restricts the set of resources (T must have RESOURCE__USE on R), the
ability to grant access (D must have RESOURCE_ADD_{IOMEM,IOPORT,IRQ} on
R), and the ability to modify a given target (D must have RESOURCE__ADD
on T). Removal has similar checks, but only on the domain D.
Because device model domains have access to XEN_DOMCTL_*_mapping, which
is a superset of XEN_DOMCTL_*_permission, in the absence of a tightly
written XSM policy, there need to be additional checks so that a device
model domain cannot grant its guest access to any resource on the
platform. This was implemented by limiting the resources that a device
model domain could specify in a permission grant to resources that it
could access itself.
Extending these models to disaggregated domain building can be done in
two ways (at least):
Method 1: domain builder not restricted. A domain builder does not need
to have access (as in ioports_access_permitted) to a resource in order
to grant a guest access to this resource. The XSM policy may still
restrict the addition, as described above, but there are no additional
checks. In particular, there is no check that the domain builder is not
granting access to any of the ranges that the hypervisor has removed
from domain 0's access list (on x86 this includes the PIC, PIT, ACPI PM
Timer, APIC, PCI configuration ports, ...).
Method 2: domain builder restricted by its own resource list. This
is what is being done now; it requires that a domain builder have access
to any resources that a domain it creates could use. Since the domain
builder is part of the TCB for any domain it builds, this is generally
considered harmless: a restricted domain builder could just create
sock-puppet domains and grant them the access it requires.
Creating an XSM policy that prevents sock-puppet domains is possible if
another trusted domain besides the domain builder is present. Newly
created domains with access to important resources could be required to
be inspected prior to being unpaused for the first time. Implementing
this type of check requires a stronger ability to revoke the access of
the domain builder than is currently provided by Xen: at the moment, a
malicious domain builder could refuse to unmap pages after building and
modify the domain after this inspection is complete. Without the
ability to prevent sock-puppet domains, there is no real advantage to
using method 1 over method 2.
While a tightly crafted XSM policy is capable of restricting method 2 to
be just as secure as method 1, this is likely to make the XSM policy
platform-specific as numeric values of I/O memory ranges will need to be
encoded in the policy. In some cases, a minor hardware or BIOS change
could cause resources to move around and would require changing the XSM
policy, which is not desirable.
--
Daniel De Graaf
National Security Agency
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] domctl: tighten XEN_DOMCTL_*_permission
2014-04-30 14:24 ` [PATCH 3/3] domctl: tighten XEN_DOMCTL_*_permission Jan Beulich
2014-04-30 17:17 ` Andrew Cooper
@ 2014-05-01 15:37 ` Tim Deegan
2014-05-02 7:41 ` Jan Beulich
1 sibling, 1 reply; 14+ messages in thread
From: Tim Deegan @ 2014-05-01 15:37 UTC (permalink / raw)
To: Jan Beulich; +Cc: Ian Campbell, xen-devel, Keir Fraser, Ian Jackson
At 15:24 +0100 on 30 Apr (1398867864), Jan Beulich wrote:
> @@ -72,13 +72,11 @@ long arch_do_domctl(
> unsigned int np = domctl->u.ioport_permission.nr_ports;
> int allow = domctl->u.ioport_permission.allow_access;
>
> - ret = -EINVAL;
> - if ( (fp + np) > 65536 )
> - break;
> -
> - if ( np == 0 )
> - ret = 0;
> - else if ( xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
> + if ( (fp + np - 1) < fp || (fp + np) > 0x10000 )
> + ret = -EINVAL;
I think this fails if (fp + np) overflows exactly to 0.
How about 'if ( (fp + np) <= fp || (fp + np) > 0x10000 )' ?
Tim.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] domctl: tighten XEN_DOMCTL_*_permission
2014-05-01 15:37 ` Tim Deegan
@ 2014-05-02 7:41 ` Jan Beulich
0 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2014-05-02 7:41 UTC (permalink / raw)
To: Tim Deegan; +Cc: Ian Campbell, xen-devel, Keir Fraser, Ian Jackson
>>> On 01.05.14 at 17:37, <tim@xen.org> wrote:
> At 15:24 +0100 on 30 Apr (1398867864), Jan Beulich wrote:
>> @@ -72,13 +72,11 @@ long arch_do_domctl(
>> unsigned int np = domctl->u.ioport_permission.nr_ports;
>> int allow = domctl->u.ioport_permission.allow_access;
>>
>> - ret = -EINVAL;
>> - if ( (fp + np) > 65536 )
>> - break;
>> -
>> - if ( np == 0 )
>> - ret = 0;
>> - else if ( xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow)
> )
>> + if ( (fp + np - 1) < fp || (fp + np) > 0x10000 )
>> + ret = -EINVAL;
>
> I think this fails if (fp + np) overflows exactly to 0.
> How about 'if ( (fp + np) <= fp || (fp + np) > 0x10000 )' ?
You're right, will fix.
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread