Changes since v2:
- Handle XEN_SYSCTL_CPUPOOL_OP_MOVEDOMAIN separately
- Use is_control_domain for CPUID
- Use is_{control,hardware}_domain for TSC
- MAINTAINERS patch included in series
---
Following the conversion of most IS_PRIV hooks to XSM, the remaining
references to this function generally deal with direct hardware access
and not with the type of privilege checks that are best controlled by
XSM. To reflect this, the IS_PRIV check is renamed to is_hardware_domain
and is used only when dealing with accesses that are both required by
dom0 and where it does not make sense to grant access to a domain other
than dom0.
There are a number of existing places in the hypervisor that check
domain_id for equality to zero to make some distinction on dom0; this
series replaces these checks with is_hardware_domain to be consistent in
how the hypervisor checks a domain''s access.
Independent changes related to this series:
[PATCH 01/11] MAINTAINERS: Add myself as XSM maintainer
[PATCH 08/11] xen/cpupool: prevent a domain from moving itself
Cleanup of IS_PRIV checks that should not be is_hardware_domain:
[PATCH 02/11] xen/arch/x86: remove IS_PRIV access check bypasses
[PATCH 03/11] xen/xsm: add hooks for claim
[PATCH 04/11] hvm: convert access check for nested HVM to XSM
[PATCH 05/11] xen/arch/x86: remove IS_PRIV_FOR references
[PATCH 06/11] xen/arch/arm: remove rcu_lock_target_domain_by_id
Replace remaining calls to IS_PRIV:
[PATCH 07/11] xen: rename IS_PRIV to is_hardware_domain
Use is_hardware_domain locations where (domid == 0) was used:
[PATCH 09/11] xen: use domid check in is_hardware_domain
[PATCH 10/11] xen/arch/x86: clarify domid == 0 checks
[PATCH 11/11] IOMMU: use is_hardware_domain instead of domid == 0
Daniel De Graaf
2013-Apr-12 21:04 UTC
[PATCH 01/11] MAINTAINERS: Add myself as XSM maintainer
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> Acked-by: Jan Beulich <jbeulich@suse.com> Acked-by: Keir Fraser <keir@xen.org> --- MAINTAINERS | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 7c24152..bcde139 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -308,6 +308,14 @@ S: Supported F: tools/xentrace/ F: xen/common/trace.c +XSM/FLASK +M: Daniel De Graaf <dgdegra@tycho.nsa.gov> +S: Supported +F: tools/flask/ +F: xen/include/xsm/ +F: xen/xsm/ +F: docs/misc/xsm-flask.txt + THE REST M: Keir Fraser <keir@xen.org> L: xen-devel@lists.xen.org -- 1.8.1.4
Daniel De Graaf
2013-Apr-12 21:04 UTC
[PATCH 02/11] xen/arch/x86: remove IS_PRIV access check bypasses
Several domctl functions dealing with rangesets contain a short-circuit
bypass if the domain is privileged. Since the construction of domain 0
permits access to all I/O ranges, the call to irq_access_permitted will
normally return true even without the IS_PRIV check, and the presence of
the IS_PRIV check prevents the creation of a privileged domain without
access to specific devices or IO memory ranges.
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/domctl.c | 12 ++++--------
xen/arch/x86/irq.c | 3 +--
2 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index a196e2a..327a792 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -578,8 +578,7 @@ long arch_do_domctl(
break;
ret = -EPERM;
- if ( !IS_PRIV(current->domain) &&
- !irq_access_permitted(current->domain, bind->machine_irq) )
+ if ( !irq_access_permitted(current->domain, bind->machine_irq) )
break;
ret = -ESRCH;
@@ -602,8 +601,7 @@ long arch_do_domctl(
bind = &(domctl->u.bind_pt_irq);
ret = -EPERM;
- if ( !IS_PRIV(current->domain) &&
- !irq_access_permitted(current->domain, bind->machine_irq) )
+ if ( !irq_access_permitted(current->domain, bind->machine_irq) )
break;
ret = xsm_unbind_pt_irq(XSM_HOOK, d, bind);
@@ -637,8 +635,7 @@ long arch_do_domctl(
break;
ret = -EPERM;
- if ( !IS_PRIV(current->domain) &&
- !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns -
1) )
+ if ( !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns -
1) )
break;
ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, add);
@@ -715,8 +712,7 @@ long arch_do_domctl(
}
ret = -EPERM;
- if ( !IS_PRIV(current->domain) &&
- !ioports_access_permitted(current->domain, fmp, fmp + np - 1) )
+ if ( !ioports_access_permitted(current->domain, fmp, fmp + np - 1) )
break;
ret = xsm_ioport_mapping(XSM_HOOK, d, fmp, fmp + np - 1, add);
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index dffb33a..33480fe 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1872,8 +1872,7 @@ int map_domain_pirq(
ASSERT(spin_is_locked(&d->event_lock));
- if ( !IS_PRIV(current->domain) &&
- !irq_access_permitted(current->domain, pirq))
+ if ( !irq_access_permitted(current->domain, pirq))
return -EPERM;
if ( pirq < 0 || pirq >= d->nr_pirqs || irq < 0 || irq >=
nr_irqs )
--
1.8.1.4
This replaces the IS_PRIV checks on these newly introduced operations
with equivalent XSM hooks, and adds FLASK access vectors for them.
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Dan Magenheimer <dan.magenheimer@oracle.com>
---
tools/flask/policy/policy/modules/xen/xen.if | 2 +-
xen/common/memory.c | 15 ++++++++-------
xen/include/xsm/dummy.h | 12 ++++++++++++
xen/include/xsm/xsm.h | 12 ++++++++++++
xen/xsm/dummy.c | 2 ++
xen/xsm/flask/hooks.c | 13 +++++++++++++
xen/xsm/flask/policy/access_vectors | 4 +++-
7 files changed, 51 insertions(+), 9 deletions(-)
diff --git a/tools/flask/policy/policy/modules/xen/xen.if
b/tools/flask/policy/policy/modules/xen/xen.if
index 2ce2212..fbb329d 100644
--- a/tools/flask/policy/policy/modules/xen/xen.if
+++ b/tools/flask/policy/policy/modules/xen/xen.if
@@ -49,7 +49,7 @@ define(`create_domain_common'', `
getdomaininfo hypercall setvcpucontext setextvcpucontext
getscheduler getvcpuinfo getvcpuextstate getaddrsize
getvcpuaffinity setvcpuaffinity };
- allow $1 $2:domain2 { set_cpuid settsc setscheduler };
+ allow $1 $2:domain2 { set_cpuid settsc setscheduler setclaim };
allow $1 $2:security check_context;
allow $1 $2:shadow enable;
allow $1 $2:mmu { map_read map_write adjust memorymap physmap pinpage
mmuext_op };
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 68501d1..3239d53 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -712,9 +712,6 @@ long do_memory_op(unsigned long cmd,
XEN_GUEST_HANDLE_PARAM(void) arg)
}
case XENMEM_claim_pages:
- if ( !IS_PRIV(current->domain) )
- return -EPERM;
-
if ( copy_from_guest(&reservation, arg, 1) )
return -EFAULT;
@@ -731,17 +728,21 @@ long do_memory_op(unsigned long cmd,
XEN_GUEST_HANDLE_PARAM(void) arg)
if ( d == NULL )
return -EINVAL;
- rc = domain_set_outstanding_pages(d, reservation.nr_extents);
+ rc = xsm_claim_pages(XSM_PRIV, d);
+
+ if ( !rc )
+ rc = domain_set_outstanding_pages(d, reservation.nr_extents);
rcu_unlock_domain(d);
break;
case XENMEM_get_outstanding_pages:
- if ( !IS_PRIV(current->domain) )
- return -EPERM;
+ rc = xsm_xenmem_get_outstanding_pages(XSM_PRIV);
+
+ if ( !rc )
+ rc = get_outstanding_claims();
- rc = get_outstanding_claims();
break;
default:
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 025936a..ae81a51 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -247,6 +247,18 @@ static XSM_INLINE int xsm_memory_pin_page(XSM_DEFAULT_ARG
struct domain *d1, str
return xsm_default_action(action, d1, d2);
}
+static XSM_INLINE int xsm_claim_pages(XSM_DEFAULT_ARG struct domain *d)
+{
+ XSM_ASSERT_ACTION(XSM_PRIV);
+ return xsm_default_action(action, current->domain, d);
+}
+
+static XSM_INLINE int xsm_xenmem_get_outstanding_pages(XSM_DEFAULT_VOID)
+{
+ XSM_ASSERT_ACTION(XSM_PRIV);
+ return xsm_default_action(action, current->domain, NULL);
+}
+
static XSM_INLINE int xsm_evtchn_unbound(XSM_DEFAULT_ARG struct domain *d,
struct evtchn *chn,
domid_t id2)
{
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index cba744c..bfe5635 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -92,6 +92,8 @@ struct xsm_operations {
int (*memory_pin_page) (struct domain *d1, struct domain *d2, struct
page_info *page);
int (*add_to_physmap) (struct domain *d1, struct domain *d2);
int (*remove_from_physmap) (struct domain *d1, struct domain *d2);
+ int (*claim_pages) (struct domain *d);
+ int (*xenmem_get_outstanding_pages) (void);
int (*console_io) (struct domain *d, int cmd);
@@ -348,6 +350,16 @@ static inline int xsm_remove_from_physmap(xsm_default_t
def, struct domain *d1,
return xsm_ops->remove_from_physmap(d1, d2);
}
+static inline int xsm_claim_pages(xsm_default_t def, struct domain *d)
+{
+ return xsm_ops->claim_pages(d);
+}
+
+static inline int xsm_xenmem_get_outstanding_pages(xsm_default_t def)
+{
+ return xsm_ops->xenmem_get_outstanding_pages();
+}
+
static inline int xsm_console_io (xsm_default_t def, struct domain *d, int cmd)
{
return xsm_ops->console_io(d, cmd);
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 6f1e0b4..594bbca 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -66,6 +66,8 @@ void xsm_fixup_ops (struct xsm_operations *ops)
set_to_dummy_if_null(ops, memory_adjust_reservation);
set_to_dummy_if_null(ops, memory_stat_reservation);
set_to_dummy_if_null(ops, memory_pin_page);
+ set_to_dummy_if_null(ops, claim_pages);
+ set_to_dummy_if_null(ops, xenmem_get_outstanding_pages);
set_to_dummy_if_null(ops, console_io);
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 29a78dd..832bc3f 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -417,6 +417,17 @@ static int flask_memory_pin_page(struct domain *d1, struct
domain *d2,
return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PINPAGE);
}
+static int flask_claim_pages(struct domain *d)
+{
+ return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SETCLAIM);
+}
+
+static int flask_xenmem_get_outstanding_pages(void)
+{
+ return avc_current_has_perm(SECINITSID_XEN, SECCLASS_XEN,
+ XEN__HEAP, NULL);
+}
+
static int flask_console_io(struct domain *d, int cmd)
{
u32 perm;
@@ -1473,6 +1484,8 @@ static struct xsm_operations flask_ops = {
.memory_adjust_reservation = flask_memory_adjust_reservation,
.memory_stat_reservation = flask_memory_stat_reservation,
.memory_pin_page = flask_memory_pin_page,
+ .claim_pages = flask_claim_pages,
+ .xenmem_get_outstanding_pages = flask_xenmem_get_outstanding_pages,
.console_io = flask_console_io,
diff --git a/xen/xsm/flask/policy/access_vectors
b/xen/xsm/flask/policy/access_vectors
index 36cbacf..a2a502f 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -54,7 +54,7 @@ class xen
debug
# XEN_SYSCTL_getcpuinfo, XENPF_get_cpu_version, XENPF_get_cpuinfo
getcpuinfo
-# XEN_SYSCTL_availheap
+# XEN_SYSCTL_availheap, XENMEM_get_outstanding_pages
heap
# XEN_SYSCTL_get_pmstat, XEN_SYSCTL_pm_op, XENPF_set_processor_pminfo,
# XENPF_core_parking
@@ -190,6 +190,8 @@ class domain2
settsc
# XEN_DOMCTL_scheduler_op with XEN_DOMCTL_SCHEDOP_putinfo
setscheduler
+# XENMEM_claim_pages
+ setclaim
}
# Similar to class domain, but primarily contains domctls related to HVM
domains
--
1.8.1.4
Daniel De Graaf
2013-Apr-12 21:04 UTC
[PATCH 04/11] hvm: convert access check for nested HVM to XSM
This adds an XSM hook for enabling nested HVM support, replacing an
IS_PRIV check. This hook is a partial duplicate with the xsm_hvm_param
hook, but using the existing hook would require adding the index to the
hook and would require the use of a custom hook for the xsm-disabled
case (using XSM_OTHER, which is less immediately readable) - whereas
adding a new hook retains the clarity of the existing code.
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
---
tools/flask/policy/policy/modules/xen/xen.if | 3 ++-
xen/arch/x86/hvm/hvm.c | 6 ++----
xen/include/xsm/dummy.h | 6 ++++++
xen/include/xsm/xsm.h | 6 ++++++
xen/xsm/dummy.c | 1 +
xen/xsm/flask/hooks.c | 6 ++++++
xen/xsm/flask/policy/access_vectors | 2 ++
7 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/tools/flask/policy/policy/modules/xen/xen.if
b/tools/flask/policy/policy/modules/xen/xen.if
index fbb329d..f6f24a5 100644
--- a/tools/flask/policy/policy/modules/xen/xen.if
+++ b/tools/flask/policy/policy/modules/xen/xen.if
@@ -54,7 +54,8 @@ define(`create_domain_common'', `
allow $1 $2:shadow enable;
allow $1 $2:mmu { map_read map_write adjust memorymap physmap pinpage
mmuext_op };
allow $1 $2:grant setup;
- allow $1 $2:hvm { cacheattr getparam hvmctl irqlevel pciroute sethvmc setparam
pcilevel trackdirtyvram };
+ allow $1 $2:hvm { cacheattr getparam hvmctl irqlevel pciroute sethvmc
+ setparam pcilevel trackdirtyvram nested };
'')
# create_domain(priv, target)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 38e87ce..8522963 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3909,11 +3909,9 @@ long do_hvm_op(unsigned long op,
XEN_GUEST_HANDLE_PARAM(void) arg)
rc = -EINVAL;
break;
case HVM_PARAM_NESTEDHVM:
- if ( !IS_PRIV(current->domain) )
- {
- rc = -EPERM;
+ rc = xsm_hvm_param_nested(XSM_PRIV, d);
+ if ( rc )
break;
- }
if ( a.value > 1 )
rc = -EINVAL;
/* Remove the check below once we have
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index ae81a51..d53132b 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -485,6 +485,12 @@ static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct
domain *d, unsigned l
return xsm_default_action(action, current->domain, d);
}
+static XSM_INLINE int xsm_hvm_param_nested(XSM_DEFAULT_ARG struct domain *d)
+{
+ XSM_ASSERT_ACTION(XSM_PRIV);
+ return xsm_default_action(action, current->domain, d);
+}
+
#ifdef CONFIG_X86
static XSM_INLINE int xsm_shadow_control(XSM_DEFAULT_ARG struct domain *d,
uint32_t op)
{
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index bfe5635..b4065ad 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -132,6 +132,7 @@ struct xsm_operations {
long (*do_xsm_op) (XEN_GUEST_HANDLE_PARAM(xsm_op_t) op);
int (*hvm_param) (struct domain *d, unsigned long op);
+ int (*hvm_param_nested) (struct domain *d);
#ifdef CONFIG_X86
int (*shadow_control) (struct domain *d, uint32_t op);
@@ -505,6 +506,11 @@ static inline int xsm_hvm_param (xsm_default_t def, struct
domain *d, unsigned l
return xsm_ops->hvm_param(d, op);
}
+static inline int xsm_hvm_param_nested (xsm_default_t def, struct domain *d)
+{
+ return xsm_ops->hvm_param_nested(d);
+}
+
#ifdef CONFIG_X86
static inline int xsm_shadow_control (xsm_default_t def, struct domain *d,
uint32_t op)
{
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 594bbca..f769642 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -103,6 +103,7 @@ void xsm_fixup_ops (struct xsm_operations *ops)
set_to_dummy_if_null(ops, tmem_op);
set_to_dummy_if_null(ops, tmem_control);
set_to_dummy_if_null(ops, hvm_param);
+ set_to_dummy_if_null(ops, hvm_param_nested);
set_to_dummy_if_null(ops, do_xsm_op);
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 832bc3f..78f0b92 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1101,6 +1101,11 @@ static int flask_hvm_param(struct domain *d, unsigned
long op)
return current_has_perm(d, SECCLASS_HVM, perm);
}
+static int flask_hvm_param_nested(struct domain *d)
+{
+ return current_has_perm(d, SECCLASS_HVM, HVM__NESTED);
+}
+
#ifdef CONFIG_X86
static int flask_shadow_control(struct domain *d, uint32_t op)
{
@@ -1517,6 +1522,7 @@ static struct xsm_operations flask_ops = {
.tmem_op = flask_tmem_op,
.tmem_control = flask_tmem_control,
.hvm_param = flask_hvm_param,
+ .hvm_param_nested = flask_hvm_param_nested,
.do_xsm_op = do_flask_op,
diff --git a/xen/xsm/flask/policy/access_vectors
b/xen/xsm/flask/policy/access_vectors
index a2a502f..176235c 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -234,6 +234,8 @@ class hvm
# source = domain whose memory is being shared
# target = client domain
share_mem
+# HVMOP_set_param setting HVM_PARAM_NESTEDHVM
+ nested
}
# Class event describes event channels. Interdomain event channels have their
--
1.8.1.4
Daniel De Graaf
2013-Apr-12 21:04 UTC
[PATCH 05/11] xen/arch/x86: remove IS_PRIV_FOR references
The check in guest_physmap_mark_populate_on_demand is redundant, since
its only caller is populate_physmap whose only caller checks the
xsm_memory_adjust_reservation hook prior to calling.
Add a new XSM hook for the other two checks since they allow privileged
domains to arbitrarily map a guest''s memory.
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>
---
xen/arch/x86/mm.c | 2 +-
xen/arch/x86/mm/p2m-pod.c | 3 ---
xen/arch/x86/mm/shadow/multi.c | 19 +++++++++++--------
xen/include/xsm/dummy.h | 6 ++++++
xen/include/xsm/xsm.h | 6 ++++++
xen/xsm/dummy.c | 1 +
xen/xsm/flask/hooks.c | 6 ++++++
xen/xsm/flask/policy/access_vectors | 3 +++
8 files changed, 34 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 58e1402..0cd4203 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -815,7 +815,7 @@ get_page_from_l1e(
* minor hack can go away.
*/
if ( (real_pg_owner == NULL) || (pg_owner == l1e_owner) ||
- !IS_PRIV_FOR(pg_owner, real_pg_owner) )
+ xsm_priv_mapping(XSM_TARGET, pg_owner, real_pg_owner) )
{
MEM_LOG("pg_owner %d l1e_owner %d, but real_pg_owner %d",
pg_owner->domain_id, l1e_owner->domain_id,
diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c
index 55936c6..04ffbcb 100644
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -1117,9 +1117,6 @@ guest_physmap_mark_populate_on_demand(struct domain *d,
unsigned long gfn,
mfn_t omfn;
int rc = 0;
- if ( !IS_PRIV_FOR(current->domain, d) )
- return -EPERM;
-
if ( !paging_mode_translate(d) )
return -EINVAL;
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index a593f76..a8ef75e 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -29,6 +29,7 @@
#include <xen/perfc.h>
#include <xen/domain_page.h>
#include <xen/iocap.h>
+#include <xsm/xsm.h>
#include <asm/page.h>
#include <asm/current.h>
#include <asm/shadow.h>
@@ -849,14 +850,16 @@ shadow_get_page_from_l1e(shadow_l1e_t sl1e, struct domain
*d, p2m_type_t type)
!shadow_mode_translate(d) &&
mfn_valid(mfn = shadow_l1e_get_mfn(sl1e)) &&
(owner = page_get_owner(mfn_to_page(mfn))) &&
- (d != owner) &&
- IS_PRIV_FOR(d, owner))
- {
- res = get_page_from_l1e(sl1e, d, owner);
- SHADOW_PRINTK("privileged domain %d installs map of mfn %05lx
"
- "which is owned by domain %d: %s\n",
- d->domain_id, mfn_x(mfn), owner->domain_id,
- res >= 0 ? "success" : "failed");
+ (d != owner) )
+ {
+ res = xsm_priv_mapping(XSM_TARGET, d, owner);
+ if ( !res ) {
+ res = get_page_from_l1e(sl1e, d, owner);
+ SHADOW_PRINTK("privileged domain %d installs map of mfn %05lx
"
+ "which is owned by domain %d: %s\n",
+ d->domain_id, mfn_x(mfn), owner->domain_id,
+ res >= 0 ? "success" :
"failed");
+ }
}
/* Okay, it might still be a grant mapping PTE. Try it. */
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index d53132b..9bfe596 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -586,6 +586,12 @@ static XSM_INLINE int xsm_update_va_mapping(XSM_DEFAULT_ARG
struct domain *d, st
return xsm_default_action(action, d, f);
}
+static XSM_INLINE int xsm_priv_mapping(XSM_DEFAULT_ARG struct domain *d, struct
domain *t)
+{
+ XSM_ASSERT_ACTION(XSM_TARGET);
+ return xsm_default_action(action, d, t);
+}
+
static XSM_INLINE int xsm_bind_pt_irq(XSM_DEFAULT_ARG struct domain *d, struct
xen_domctl_bind_pt_irq *bind)
{
XSM_ASSERT_ACTION(XSM_HOOK);
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index b4065ad..69fe64a 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -156,6 +156,7 @@ struct xsm_operations {
struct domain *f, uint32_t flags);
int (*mmuext_op) (struct domain *d, struct domain *f);
int (*update_va_mapping) (struct domain *d, struct domain *f, l1_pgentry_t
pte);
+ int (*priv_mapping) (struct domain *d, struct domain *t);
int (*bind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq *bind);
int (*unbind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq
*bind);
int (*ioport_permission) (struct domain *d, uint32_t s, uint32_t e, uint8_t
allow);
@@ -594,6 +595,11 @@ static inline int xsm_update_va_mapping(xsm_default_t def,
struct domain *d, str
return xsm_ops->update_va_mapping(d, f, pte);
}
+static inline int xsm_priv_mapping(xsm_default_t def, struct domain *d, struct
domain *t)
+{
+ return xsm_ops->priv_mapping(d, t);
+}
+
static inline int xsm_bind_pt_irq(xsm_default_t def, struct domain *d,
struct xen_domctl_bind_pt_irq
*bind)
{
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index f769642..3d84e73 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -126,6 +126,7 @@ void xsm_fixup_ops (struct xsm_operations *ops)
set_to_dummy_if_null(ops, mmu_update);
set_to_dummy_if_null(ops, mmuext_op);
set_to_dummy_if_null(ops, update_va_mapping);
+ set_to_dummy_if_null(ops, priv_mapping);
set_to_dummy_if_null(ops, bind_pt_irq);
set_to_dummy_if_null(ops, unbind_pt_irq);
set_to_dummy_if_null(ops, ioport_permission);
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 78f0b92..809e0f9 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1354,6 +1354,11 @@ static int flask_update_va_mapping(struct domain *d,
struct domain *f,
return domain_has_perm(d, f, SECCLASS_MMU, map_perms);
}
+static int flask_priv_mapping(struct domain *d, struct domain *t)
+{
+ return domain_has_perm(d, t, SECCLASS_MMU, MMU__TARGET_HACK);
+}
+
static int flask_get_device_group(uint32_t machine_bdf)
{
u32 rsid;
@@ -1545,6 +1550,7 @@ static struct xsm_operations flask_ops = {
.mmu_update = flask_mmu_update,
.mmuext_op = flask_mmuext_op,
.update_va_mapping = flask_update_va_mapping,
+ .priv_mapping = flask_priv_mapping,
.get_device_group = flask_get_device_group,
.test_assign_device = flask_test_assign_device,
.assign_device = flask_assign_device,
diff --git a/xen/xsm/flask/policy/access_vectors
b/xen/xsm/flask/policy/access_vectors
index 176235c..63ee4dd 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -330,6 +330,9 @@ class mmu
# source = domain making the hypercall
# target = domain whose pages are being exchanged
exchange
+# Allow a privileged domain to install a map of a page it does not own. Used
+# for stub domain device models with the PV framebuffer.
+ target_hack
}
# control of the paging_domctl split by subop
--
1.8.1.4
Daniel De Graaf
2013-Apr-12 21:04 UTC
[PATCH 06/11] xen/arch/arm: remove rcu_lock_target_domain_by_id
This function has been replaced with rcu_lock_domain_by_any_id and an
XSM check. Two callers already had an XSM check; add a check to the
third.
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Keir Fraser <keir@xen.org>
---
xen/arch/arm/mm.c | 23 +++++++++++++++--------
xen/common/domain.c | 34 ----------------------------------
xen/include/xen/sched.h | 14 --------------
xen/include/xsm/dummy.h | 8 ++++++++
xen/include/xsm/xsm.h | 11 +++++++++++
xen/xsm/dummy.c | 3 +++
xen/xsm/flask/hooks.c | 10 ++++++++++
7 files changed, 47 insertions(+), 56 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index ba3140d..35cd1c9 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -624,9 +624,16 @@ static int xenmem_add_to_physmap_one(
{
paddr_t maddr;
struct domain *od;
- rc = rcu_lock_target_domain_by_id(foreign_domid, &od);
- if ( rc < 0 )
+ od = rcu_lock_domain_by_any_id(foreign_domid);
+ if ( od == NULL )
+ return -ESRCH;
+
+ rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od);
+ if ( rc )
+ {
+ rcu_unlock_domain(od);
return rc;
+ }
maddr = p2m_lookup(od, idx << PAGE_SHIFT);
if ( maddr == INVALID_PADDR )
@@ -718,9 +725,9 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void)
arg)
if ( xatp.space == XENMAPSPACE_gmfn_foreign )
return -EINVAL;
- rc = rcu_lock_target_domain_by_id(xatp.domid, &d);
- if ( rc != 0 )
- return rc;
+ d = rcu_lock_domain_by_any_id(xatp.domid);
+ if ( d == NULL )
+ return -ESRCH;
rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d);
if ( rc )
@@ -749,9 +756,9 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void)
arg)
if ( xatpr.space == XENMAPSPACE_gmfn_range )
return -EINVAL;
- rc = rcu_lock_target_domain_by_id(xatpr.domid, &d);
- if ( rc != 0 )
- return rc;
+ d = rcu_lock_domain_by_any_id(xatpr.domid);
+ if ( d == NULL )
+ return -ESRCH;
rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d);
if ( rc )
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 590548e..ce6747c 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -429,40 +429,6 @@ struct domain *rcu_lock_domain_by_any_id(domid_t dom)
return rcu_lock_domain_by_id(dom);
}
-int rcu_lock_target_domain_by_id(domid_t dom, struct domain **d)
-{
- if ( dom == DOMID_SELF )
- {
- *d = rcu_lock_current_domain();
- return 0;
- }
-
- if ( (*d = rcu_lock_domain_by_id(dom)) == NULL )
- return -ESRCH;
-
- if ( !IS_PRIV_FOR(current->domain, *d) )
- {
- rcu_unlock_domain(*d);
- return -EPERM;
- }
-
- return 0;
-}
-
-int rcu_lock_remote_target_domain_by_id(domid_t dom, struct domain **d)
-{
- if ( (*d = rcu_lock_domain_by_id(dom)) == NULL )
- return -ESRCH;
-
- if ( (*d == current->domain) || !IS_PRIV_FOR(current->domain, *d) )
- {
- rcu_unlock_domain(*d);
- return -EPERM;
- }
-
- return 0;
-}
-
int rcu_lock_remote_domain_by_id(domid_t dom, struct domain **d)
{
if ( (*d = rcu_lock_domain_by_id(dom)) == NULL )
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index d15d567..723885c 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -471,20 +471,6 @@ struct domain *rcu_lock_domain_by_id(domid_t dom);
struct domain *rcu_lock_domain_by_any_id(domid_t dom);
/*
- * As above function, but accounts for current domain context:
- * - Translates target DOMID_SELF into caller''s domain id; and
- * - Checks that caller has permission to act on the target domain.
- */
-int rcu_lock_target_domain_by_id(domid_t dom, struct domain **d);
-
-/*
- * As rcu_lock_target_domain_by_id(), but will fail EPERM rather than resolve
- * to local domain. Successful return always resolves to a remote domain that
- * the local domain is privileged to control.
- */
-int rcu_lock_remote_target_domain_by_id(domid_t dom, struct domain **d);
-
-/*
* As rcu_lock_domain_by_id(), but will fail EPERM or ESRCH rather than resolve
* to local domain.
*/
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 9bfe596..3912bd9 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -616,4 +616,12 @@ static XSM_INLINE int xsm_ioport_mapping(XSM_DEFAULT_ARG
struct domain *d, uint3
return xsm_default_action(action, current->domain, d);
}
+#endif /* CONFIG_X86 */
+
+#ifdef CONFIG_ARM
+static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d,
struct domain *t)
+{
+ XSM_ASSERT_ACTION(XSM_TARGET);
+ return xsm_default_action(action, d, t);
+}
#endif
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 69fe64a..58a4fbb 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -162,6 +162,9 @@ struct xsm_operations {
int (*ioport_permission) (struct domain *d, uint32_t s, uint32_t e, uint8_t
allow);
int (*ioport_mapping) (struct domain *d, uint32_t s, uint32_t e, uint8_t
allow);
#endif
+#ifdef CONFIG_ARM
+ int (*map_gmfn_foreign) (struct domain *d, struct domain *t);
+#endif
};
#ifdef XSM_ENABLE
@@ -622,6 +625,14 @@ static inline int xsm_ioport_mapping (xsm_default_t def,
struct domain *d, uint3
return xsm_ops->ioport_mapping(d, s, e, allow);
}
#endif /* CONFIG_X86 */
+
+#ifdef CONFIG_ARM
+static inline int xsm_map_gmfn_foreign (struct domain *d, struct domain *t)
+{
+ return xsm_ops->map_gmfn_foreign(d, t);
+}
+#endif /* CONFIG_ARM */
+
#endif /* XSM_NO_WRAPPERS */
extern int xsm_init(unsigned long *module_map, const multiboot_info_t *mbi,
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 3d84e73..937761f 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -132,4 +132,7 @@ void xsm_fixup_ops (struct xsm_operations *ops)
set_to_dummy_if_null(ops, ioport_permission);
set_to_dummy_if_null(ops, ioport_mapping);
#endif
+#ifdef CONFIG_ARM
+ set_to_dummy_if_null(ops, map_gmfn_foreign);
+#endif
}
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 809e0f9..6512c22 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1452,6 +1452,13 @@ static int flask_unbind_pt_irq (struct domain *d, struct
xen_domctl_bind_pt_irq
{
return current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__REMOVE);
}
+#endif /* CONFIG_X86 */
+
+#ifdef CONFIG_ARM
+static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
+{
+ return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
+}
#endif
long do_flask_op(XEN_GUEST_HANDLE_PARAM(xsm_op_t) u_flask_op);
@@ -1560,6 +1567,9 @@ static struct xsm_operations flask_ops = {
.ioport_permission = flask_ioport_permission,
.ioport_mapping = flask_ioport_mapping,
#endif
+#ifdef CONFIG_ARM
+ .map_gmfn_foreign = flask_map_gmfn_foreign,
+#endif
};
static __init int flask_init(void)
--
1.8.1.4
Daniel De Graaf
2013-Apr-12 21:04 UTC
[PATCH 07/11] xen: rename IS_PRIV to is_hardware_domain
Since the remaining uses of IS_PRIV are actually concerned with the
domain having control of the hardware (i.e. being the initial domain),
clarify this by renaming IS_PRIV to is_hardware_domain. This also
removes IS_PRIV_FOR since the only remaining user was xsm/dummy.h.
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/domctl.c | 6 +++---
xen/arch/x86/msi.c | 2 +-
xen/arch/x86/physdev.c | 4 ++--
xen/arch/x86/traps.c | 12 ++++++------
xen/include/xen/sched.h | 12 ++++++++++--
xen/include/xsm/dummy.h | 32 ++++++++++++++++++++------------
6 files changed, 42 insertions(+), 26 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 327a792..ed51106 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -662,7 +662,7 @@ long arch_do_domctl(
while ( i-- )
clear_mmio_p2m_entry(d, gfn + i);
if ( iomem_deny_access(d, mfn, mfn + nr_mfns - 1)
&&
- IS_PRIV(current->domain) )
+ is_hardware_domain(current->domain) )
printk(XENLOG_ERR
"memory_map: failed to deny dom%d access to
[%lx,%lx]\n",
d->domain_id, mfn, mfn + nr_mfns - 1);
@@ -681,7 +681,7 @@ long arch_do_domctl(
ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
if ( !ret && add )
ret = -EIO;
- if ( ret && IS_PRIV(current->domain) )
+ if ( ret && is_hardware_domain(current->domain) )
printk(XENLOG_ERR
"memory_map: error %ld %s dom%d access to
[%lx,%lx]\n",
ret, add ? "removing" : "denying",
d->domain_id,
@@ -768,7 +768,7 @@ long arch_do_domctl(
break;
}
ret = ioports_deny_access(d, fmp, fmp + np - 1);
- if ( ret && IS_PRIV(current->domain) )
+ if ( ret && is_hardware_domain(current->domain) )
printk(XENLOG_ERR
"ioport_map: error %ld denying dom%d access to
[%x,%x]\n",
ret, d->domain_id, fmp, fmp + np - 1);
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 6cc8f7a..4059b32 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -810,7 +810,7 @@ static int msix_capability_init(struct pci_dev *dev,
break;
if ( d )
{
- if ( !IS_PRIV(d) && dev->msix_warned !=
d->domain_id )
+ if ( !is_hardware_domain(d) && dev->msix_warned !=
d->domain_id )
{
dev->msix_warned = d->domain_id;
printk(XENLOG_ERR
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 876ac9d..226905d 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -128,7 +128,7 @@ int physdev_map_pirq(domid_t domid, int type, int *index,
int *pirq_p,
irq = domain_pirq_to_irq(current->domain, *index);
if ( irq <= 0 )
{
- if ( IS_PRIV(current->domain) )
+ if ( is_hardware_domain(current->domain) )
irq = *index;
else {
dprintk(XENLOG_G_ERR, "dom%d: map pirq with incorrect
irq!\n",
@@ -691,7 +691,7 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void)
arg)
case PHYSDEVOP_dbgp_op: {
struct physdev_dbgp_op op;
- if ( !IS_PRIV(v->domain) )
+ if ( !is_hardware_domain(v->domain) )
ret = -EPERM;
else if ( copy_from_guest(&op, arg, 1) )
ret = -EFAULT;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index d36eddd..80d7892 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1303,7 +1303,7 @@ static int fixup_page_fault(unsigned long addr, struct
cpu_user_regs *regs)
ptwr_do_page_fault(v, addr, regs) )
return EXCRET_fault_fixed;
- if ( IS_PRIV(d) && (regs->error_code &
PFEC_page_present) &&
+ if ( is_hardware_domain(d) && (regs->error_code &
PFEC_page_present) &&
mmio_ro_do_page_fault(v, addr, regs) )
return EXCRET_fault_fixed;
}
@@ -1623,7 +1623,7 @@ static int pci_cfg_ok(struct domain *d, int write, int
size)
{
uint32_t machine_bdf;
uint16_t start, end;
- if (!IS_PRIV(d))
+ if (!is_hardware_domain(d))
return 0;
machine_bdf = (d->arch.pci_cf8 >> 8) & 0xFFFF;
@@ -2404,7 +2404,7 @@ static int emulate_privileged_op(struct cpu_user_regs
*regs)
if ( boot_cpu_data.x86_vendor != X86_VENDOR_AMD ||
boot_cpu_data.x86 < 0x10 || boot_cpu_data.x86 > 0x17 )
goto fail;
- if ( !IS_PRIV(v->domain) || !is_pinned_vcpu(v) )
+ if ( !is_hardware_domain(v->domain) || !is_pinned_vcpu(v) )
break;
if ( (rdmsr_safe(MSR_AMD64_NB_CFG, val) != 0) ||
(eax != (uint32_t)val) ||
@@ -2417,7 +2417,7 @@ static int emulate_privileged_op(struct cpu_user_regs
*regs)
if ( boot_cpu_data.x86_vendor != X86_VENDOR_AMD ||
boot_cpu_data.x86 < 0x10 || boot_cpu_data.x86 > 0x17 )
goto fail;
- if ( !IS_PRIV(v->domain) || !is_pinned_vcpu(v) )
+ if ( !is_hardware_domain(v->domain) || !is_pinned_vcpu(v) )
break;
if ( (rdmsr_safe(MSR_FAM10H_MMIO_CONF_BASE, val) != 0) )
goto fail;
@@ -2437,7 +2437,7 @@ static int emulate_privileged_op(struct cpu_user_regs
*regs)
case MSR_IA32_UCODE_REV:
if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL )
goto fail;
- if ( !IS_PRIV(v->domain) || !is_pinned_vcpu(v) )
+ if ( !is_hardware_domain(v->domain) || !is_pinned_vcpu(v) )
break;
if ( rdmsr_safe(regs->ecx, val) )
goto fail;
@@ -2473,7 +2473,7 @@ static int emulate_privileged_op(struct cpu_user_regs
*regs)
case MSR_IA32_ENERGY_PERF_BIAS:
if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL )
goto fail;
- if ( !IS_PRIV(v->domain) || !is_pinned_vcpu(v) )
+ if ( !is_hardware_domain(v->domain) || !is_pinned_vcpu(v) )
break;
if ( wrmsr_safe(regs->ecx, msr_content) != 0 )
goto fail;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 723885c..e1cb363 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -705,8 +705,16 @@ uint64_t get_cpu_idle_time(unsigned int cpu);
void watchdog_domain_init(struct domain *d);
void watchdog_domain_destroy(struct domain *d);
-#define IS_PRIV(_d) ((_d)->is_privileged)
-#define IS_PRIV_FOR(_d, _t) (IS_PRIV(_d) || ((_d)->target &&
(_d)->target == (_t)))
+/*
+ * Use this check when the following are both true:
+ * - Using this feature or interface requires full access to the hardware
+ * (that is, this is would not be suitable for a driver domain)
+ * - There is never a reason to deny dom0 access to this
+ */
+#define is_hardware_domain(_d) ((_d)->is_privileged)
+
+/* This check is for functionality specific to a control domain */
+#define is_control_domain(_d) ((_d)->is_privileged)
#define VM_ASSIST(_d,_t) (test_bit((_t), &(_d)->vm_assist))
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 3912bd9..a872056 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -60,17 +60,23 @@ static always_inline int xsm_default_action(
case XSM_HOOK:
return 0;
case XSM_DM_PRIV:
- if ( !IS_PRIV_FOR(src, target) )
- return -EPERM;
- return 0;
+ if ( src->is_privileged )
+ return 0;
+ if ( target && src->target == target )
+ return 0;
+ return -EPERM;
case XSM_TARGET:
- if ( src != target && !IS_PRIV_FOR(src, target) )
- return -EPERM;
- return 0;
+ if ( src == target )
+ return 0;
+ if ( src->is_privileged )
+ return 0;
+ if ( target && src->target == target )
+ return 0;
+ return -EPERM;
case XSM_PRIV:
- if ( !IS_PRIV(src) )
- return -EPERM;
- return 0;
+ if ( src->is_privileged )
+ return 0;
+ return -EPERM;
default:
LINKER_BUG_ON(1);
return -EPERM;
@@ -567,10 +573,12 @@ static XSM_INLINE int
xsm_domain_memory_map(XSM_DEFAULT_ARG struct domain *d)
static XSM_INLINE int xsm_mmu_update(XSM_DEFAULT_ARG struct domain *d, struct
domain *t,
struct domain *f, uint32_t flags)
{
+ int rc;
XSM_ASSERT_ACTION(XSM_TARGET);
- if ( t && d != t && !IS_PRIV_FOR(d, t) )
- return -EPERM;
- return xsm_default_action(action, d, f);
+ rc = xsm_default_action(action, d, f);
+ if ( t && !rc )
+ rc = xsm_default_action(action, d, t);
+ return rc;
}
static XSM_INLINE int xsm_mmuext_op(XSM_DEFAULT_ARG struct domain *d, struct
domain *f)
--
1.8.1.4
Daniel De Graaf
2013-Apr-12 21:04 UTC
[PATCH 08/11] xen/cpupool: prevent a domain from moving itself
In the XEN_SYSCTL_CPUPOOL_OP_MOVEDOMAIN operation, the existing check
for domid == 0 should be checking that a domain does not attempt to
modify its own cpupool; fix this by using rcu_lock_remote_domain_by_id.
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Juergen Gross <juergen.gross@ts.fujitsu.com>
Cc: Jan Beulich <JBeulich@suse.com>
---
xen/common/cpupool.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index 10b10f8..c901f7e 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -546,12 +546,8 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
{
struct domain *d;
- ret = -EINVAL;
- if ( op->domid == 0 )
- break;
- ret = -ESRCH;
- d = rcu_lock_domain_by_id(op->domid);
- if ( d == NULL )
+ ret = rcu_lock_remote_domain_by_id(op->domid, &d);
+ if ( ret )
break;
if ( d->cpupool == NULL )
{
--
1.8.1.4
Daniel De Graaf
2013-Apr-12 21:04 UTC
[PATCH 09/11] xen: use domid check in is_hardware_domain
Instead of checking is_privileged to determine if a domain should
control the hardware, check that the domain_id is equal to zero (which
is currently the only domain for which is_privileged is true). This
allows other places where domain_id is checked for zero to be replaced
with is_hardware_domain.
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Keir Fraser <keir@xen.org>
---
xen/common/domain.c | 10 +++++-----
xen/include/xen/sched.h | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/xen/common/domain.c b/xen/common/domain.c
index ce6747c..02b631f 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -235,7 +235,7 @@ struct domain *domain_create(
if ( domcr_flags & DOMCRF_hvm )
d->is_hvm = 1;
- if ( domid == 0 )
+ if ( is_hardware_domain(d) )
{
d->is_pinned = opt_dom0_vcpus_pin;
d->disable_migrate = 1;
@@ -260,10 +260,10 @@ struct domain *domain_create(
d->is_paused_by_controller = 1;
atomic_inc(&d->pause_count);
- if ( domid )
- d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
- else
+ if ( is_hardware_domain(d) )
d->nr_pirqs = nr_static_irqs + extra_dom0_irqs;
+ else
+ d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
if ( d->nr_pirqs > nr_irqs )
d->nr_pirqs = nr_irqs;
@@ -543,7 +543,7 @@ void domain_shutdown(struct domain *d, u8 reason)
d->shutdown_code = reason;
reason = d->shutdown_code;
- if ( d->domain_id == 0 )
+ if ( is_hardware_domain(d) )
dom0_shutdown(reason);
if ( d->is_shutting_down )
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index e1cb363..64cc39b 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -711,7 +711,7 @@ void watchdog_domain_destroy(struct domain *d);
* (that is, this is would not be suitable for a driver domain)
* - There is never a reason to deny dom0 access to this
*/
-#define is_hardware_domain(_d) ((_d)->is_privileged)
+#define is_hardware_domain(_d) ((_d)->domain_id == 0)
/* This check is for functionality specific to a control domain */
#define is_control_domain(_d) ((_d)->is_privileged)
--
1.8.1.4
Daniel De Graaf
2013-Apr-12 21:04 UTC
[PATCH 10/11] xen/arch/x86: clarify domid == 0 checks
This makes checks for dom0 more explicit than checking the domain ID,
using is_hardware_domain for checks relating to hardware access and
is_control_domain for things that a control domain may need.
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/domain.c | 2 +-
xen/arch/x86/hvm/i8254.c | 2 +-
xen/arch/x86/time.c | 4 ++--
xen/arch/x86/traps.c | 4 ++--
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 0d7a40c..45d5b63 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1571,7 +1571,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
}
set_cpuid_faulting(!is_hvm_vcpu(next) &&
- (next->domain->domain_id != 0));
+ !is_control_domain(next->domain));
}
context_saved(prev);
diff --git a/xen/arch/x86/hvm/i8254.c b/xen/arch/x86/hvm/i8254.c
index c0d6bc2..add61e3 100644
--- a/xen/arch/x86/hvm/i8254.c
+++ b/xen/arch/x86/hvm/i8254.c
@@ -541,7 +541,7 @@ int pv_pit_handler(int port, int data, int write)
.data = data
};
- if ( (current->domain->domain_id == 0) &&
dom0_pit_access(&ioreq) )
+ if ( is_hardware_domain(current->domain) &&
dom0_pit_access(&ioreq) )
{
/* nothing to do */;
}
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 6e94847..dc38ead 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1928,7 +1928,7 @@ void tsc_set_info(struct domain *d,
uint32_t tsc_mode, uint64_t elapsed_nsec,
uint32_t gtsc_khz, uint32_t incarnation)
{
- if ( is_idle_domain(d) || (d->domain_id == 0) )
+ if ( is_idle_domain(d) || is_hardware_domain(d) || is_control_domain(d) )
{
d->arch.vtsc = 0;
return;
@@ -2005,7 +2005,7 @@ static void dump_softtsc(unsigned char key)
"warp=%lu (count=%lu)\n", tsc_max_warp,
tsc_check_count);
for_each_domain ( d )
{
- if ( d->domain_id == 0 && d->arch.tsc_mode ==
TSC_MODE_DEFAULT )
+ if ( is_hardware_domain(d) && d->arch.tsc_mode ==
TSC_MODE_DEFAULT )
continue;
printk("dom%u%s: mode=%d",d->domain_id,
is_hvm_domain(d) ? "(hvm)" : "",
d->arch.tsc_mode);
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 80d7892..c82684e 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -737,7 +737,7 @@ static void pv_cpuid(struct cpu_user_regs *regs)
c = regs->ecx;
d = regs->edx;
- if ( current->domain->domain_id != 0 )
+ if ( !is_control_domain(current->domain) )
{
unsigned int cpuid_leaf = a, sub_leaf = c;
@@ -1828,7 +1828,7 @@ static inline uint64_t guest_misc_enable(uint64_t val)
static int is_cpufreq_controller(struct domain *d)
{
return ((cpufreq_controller == FREQCTL_dom0_kernel) &&
- (d->domain_id == 0));
+ is_hardware_domain(d));
}
#include "x86_64/mmconfig.h"
--
1.8.1.4
Daniel De Graaf
2013-Apr-12 21:04 UTC
[PATCH 11/11] IOMMU: use is_hardware_domain instead of domid == 0
This makes checks for dom0 more explicit than checking the domain ID.
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Jacob Shin <jacob.shin@amd.com>
Cc: Xiantao Zhang <xiantao.zhang@intel.com>
---
xen/drivers/passthrough/amd/pci_amd_iommu.c | 2 +-
xen/drivers/passthrough/vtd/iommu.c | 8 ++++----
xen/drivers/passthrough/vtd/x86/vtd.c | 2 +-
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 60696d7..687f1a3 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -122,7 +122,7 @@ static void amd_iommu_setup_domain_device(
BUG_ON( !hd->root_table || !hd->paging_mode ||
!iommu->dev_table.buffer );
- if ( iommu_passthrough && (domain->domain_id == 0) )
+ if ( iommu_passthrough && is_hardware_domain(domain) )
valid = 0;
if ( ats_enabled )
diff --git a/xen/drivers/passthrough/vtd/iommu.c
b/xen/drivers/passthrough/vtd/iommu.c
index 0fc10de..8d5c43d 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1336,7 +1336,7 @@ int domain_context_mapping_one(
return res;
}
- if ( iommu_passthrough && (domain->domain_id == 0) )
+ if ( iommu_passthrough && is_hardware_domain(domain) )
{
context_set_translation_type(*context, CONTEXT_TT_PASS_THRU);
agaw = level_to_agaw(iommu->nr_pt_levels);
@@ -1710,7 +1710,7 @@ static int intel_iommu_map_page(
return 0;
/* do nothing if dom0 and iommu supports pass thru */
- if ( iommu_passthrough && (d->domain_id == 0) )
+ if ( iommu_passthrough && is_hardware_domain(d) )
return 0;
spin_lock(&hd->mapping_lock);
@@ -1754,7 +1754,7 @@ static int intel_iommu_map_page(
static int intel_iommu_unmap_page(struct domain *d, unsigned long gfn)
{
/* Do nothing if dom0 and iommu supports pass thru. */
- if ( iommu_passthrough && (d->domain_id == 0) )
+ if ( iommu_passthrough && is_hardware_domain(d) )
return 0;
dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K);
@@ -1927,7 +1927,7 @@ static int intel_iommu_remove_device(u8 devfn, struct
pci_dev *pdev)
/* If the device belongs to dom0, and it has RMRR, don''t remove it
* from dom0, because BIOS may use RMRR at booting time.
*/
- if ( pdev->domain->domain_id == 0 )
+ if ( is_hardware_domain(pdev->domain) )
{
for_each_rmrr_device ( rmrr, bdf, i )
{
diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c
b/xen/drivers/passthrough/vtd/x86/vtd.c
index 875b033..eb9e52a 100644
--- a/xen/drivers/passthrough/vtd/x86/vtd.c
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c
@@ -117,7 +117,7 @@ void __init iommu_set_dom0_mapping(struct domain *d)
{
unsigned long i, j, tmp, top;
- BUG_ON(d->domain_id != 0);
+ BUG_ON(!is_hardware_domain(d));
top = max(max_pdx, pfn_to_pdx(0xffffffffUL >> PAGE_SHIFT) + 1);
--
1.8.1.4
Jan Beulich
2013-Apr-15 08:51 UTC
Re: [PATCH 10/11] xen/arch/x86: clarify domid == 0 checks
>>> On 12.04.13 at 23:04, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote: > --- a/xen/arch/x86/time.c > +++ b/xen/arch/x86/time.c > @@ -1928,7 +1928,7 @@ void tsc_set_info(struct domain *d, > uint32_t tsc_mode, uint64_t elapsed_nsec, > uint32_t gtsc_khz, uint32_t incarnation) > { > - if ( is_idle_domain(d) || (d->domain_id == 0) ) > + if ( is_idle_domain(d) || is_hardware_domain(d) || is_control_domain(d) > ) > { > d->arch.vtsc = 0; > return; > @@ -2005,7 +2005,7 @@ static void dump_softtsc(unsigned char key) > "warp=%lu (count=%lu)\n", tsc_max_warp, tsc_check_count); > for_each_domain ( d ) > { > - if ( d->domain_id == 0 && d->arch.tsc_mode == TSC_MODE_DEFAULT ) > + if ( is_hardware_domain(d) && d->arch.tsc_mode == TSC_MODE_DEFAULT ) > continue; > printk("dom%u%s: mode=%d",d->domain_id, > is_hvm_domain(d) ? "(hvm)" : "", d->arch.tsc_mode);I am of the opinion that the two checks should match, i.e. the second one should also become is_hardware || is_control. But I say this without really recalling why Dom0 is being special cased here in the first place. Jan
Daniel De Graaf
2013-Apr-15 13:47 UTC
Re: [PATCH 10/11] xen/arch/x86: clarify domid == 0 checks
On 04/15/2013 04:51 AM, Jan Beulich wrote:>>>> On 12.04.13 at 23:04, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote: >> --- a/xen/arch/x86/time.c >> +++ b/xen/arch/x86/time.c >> @@ -1928,7 +1928,7 @@ void tsc_set_info(struct domain *d, >> uint32_t tsc_mode, uint64_t elapsed_nsec, >> uint32_t gtsc_khz, uint32_t incarnation) >> { >> - if ( is_idle_domain(d) || (d->domain_id == 0) ) >> + if ( is_idle_domain(d) || is_hardware_domain(d) || is_control_domain(d) >> ) >> { >> d->arch.vtsc = 0; >> return; >> @@ -2005,7 +2005,7 @@ static void dump_softtsc(unsigned char key) >> "warp=%lu (count=%lu)\n", tsc_max_warp, tsc_check_count); >> for_each_domain ( d ) >> { >> - if ( d->domain_id == 0 && d->arch.tsc_mode == TSC_MODE_DEFAULT ) >> + if ( is_hardware_domain(d) && d->arch.tsc_mode == TSC_MODE_DEFAULT ) >> continue; >> printk("dom%u%s: mode=%d",d->domain_id, >> is_hvm_domain(d) ? "(hvm)" : "", d->arch.tsc_mode); > > I am of the opinion that the two checks should match, i.e. the > second one should also become is_hardware || is_control. But > I say this without really recalling why Dom0 is being special cased > here in the first place. > > Jan >Since this is just an output function, my best guess is to avoid displaying dom0 statistics that aren''t relevant for a query that is intended for domUs, so it doesn''t really matter what is tested. -- Daniel De Graaf National Security Agency
Jan Beulich
2013-Apr-18 15:09 UTC
Re: [PATCH 10/11] xen/arch/x86: clarify domid == 0 checks
>>> On 15.04.13 at 15:47, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote: > On 04/15/2013 04:51 AM, Jan Beulich wrote: >>>>> On 12.04.13 at 23:04, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote: >>> --- a/xen/arch/x86/time.c >>> +++ b/xen/arch/x86/time.c >>> @@ -1928,7 +1928,7 @@ void tsc_set_info(struct domain *d, >>> uint32_t tsc_mode, uint64_t elapsed_nsec, >>> uint32_t gtsc_khz, uint32_t incarnation) >>> { >>> - if ( is_idle_domain(d) || (d->domain_id == 0) ) >>> + if ( is_idle_domain(d) || is_hardware_domain(d) || is_control_domain(d) >>> ) >>> { >>> d->arch.vtsc = 0; >>> return; >>> @@ -2005,7 +2005,7 @@ static void dump_softtsc(unsigned char key) >>> "warp=%lu (count=%lu)\n", tsc_max_warp, tsc_check_count); >>> for_each_domain ( d ) >>> { >>> - if ( d->domain_id == 0 && d->arch.tsc_mode == TSC_MODE_DEFAULT ) >>> + if ( is_hardware_domain(d) && d->arch.tsc_mode == TSC_MODE_DEFAULT ) >>> continue; >>> printk("dom%u%s: mode=%d",d->domain_id, >>> is_hvm_domain(d) ? "(hvm)" : "", d->arch.tsc_mode); >> >> I am of the opinion that the two checks should match, i.e. the >> second one should also become is_hardware || is_control. But >> I say this without really recalling why Dom0 is being special cased >> here in the first place. > > Since this is just an output function, my best guess is to avoid displaying > dom0 statistics that aren''t relevant for a query that is intended for domUs, > so it doesn''t really matter what is tested.Right now it doesn''t, but the ultimate goal is for is_control_domain() and is_hardware_domain() to check different aspects. The latter seems to be the correct fit here, so I suppose the first check above should also test just that. Jan
>>> On 12.04.13 at 23:04, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote: > Changes since v2: > - Handle XEN_SYSCTL_CPUPOOL_OP_MOVEDOMAIN separately > - Use is_control_domain for CPUID > - Use is_{control,hardware}_domain for TSC > - MAINTAINERS patch included in series > > --- > > Following the conversion of most IS_PRIV hooks to XSM, the remaining > references to this function generally deal with direct hardware access > and not with the type of privilege checks that are best controlled by > XSM. To reflect this, the IS_PRIV check is renamed to is_hardware_domain > and is used only when dealing with accesses that are both required by > dom0 and where it does not make sense to grant access to a domain other > than dom0. > > There are a number of existing places in the hypervisor that check > domain_id for equality to zero to make some distinction on dom0; this > series replaces these checks with is_hardware_domain to be consistent in > how the hypervisor checks a domain''s access. > > Independent changes related to this series: > [PATCH 01/11] MAINTAINERS: Add myself as XSM maintainer > [PATCH 08/11] xen/cpupool: prevent a domain from moving itself > > Cleanup of IS_PRIV checks that should not be is_hardware_domain: > [PATCH 02/11] xen/arch/x86: remove IS_PRIV access check bypasses > [PATCH 03/11] xen/xsm: add hooks for claim > [PATCH 04/11] hvm: convert access check for nested HVM to XSM > [PATCH 05/11] xen/arch/x86: remove IS_PRIV_FOR references > [PATCH 06/11] xen/arch/arm: remove rcu_lock_target_domain_by_id > > Replace remaining calls to IS_PRIV: > [PATCH 07/11] xen: rename IS_PRIV to is_hardware_domain > > Use is_hardware_domain locations where (domid == 0) was used: > [PATCH 09/11] xen: use domid check in is_hardware_domain > [PATCH 10/11] xen/arch/x86: clarify domid == 0 checks > [PATCH 11/11] IOMMU: use is_hardware_domain instead of domid == 0While patch 1 went in a few days ago, patch 2 was held up just by XSA-46, which went public today. Consequently I also committed patch 2 a few minutes ago. For the rest of the series, however, I would want you two to work out the release related aspects, and I''d look into committing parts that I''m permitted to commit once I saw George''s ack. Patch 10 may, according to the reply I just sent, need another small tweak before getting applied - as long as you agree, I could certainly do this while committing. Jan
On 04/18/2013 11:12 AM, Jan Beulich wrote:>>>> On 12.04.13 at 23:04, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote: >> Changes since v2: >> - Handle XEN_SYSCTL_CPUPOOL_OP_MOVEDOMAIN separately >> - Use is_control_domain for CPUID >> - Use is_{control,hardware}_domain for TSC >> - MAINTAINERS patch included in series >> >> --- >> >> Following the conversion of most IS_PRIV hooks to XSM, the remaining >> references to this function generally deal with direct hardware access >> and not with the type of privilege checks that are best controlled by >> XSM. To reflect this, the IS_PRIV check is renamed to is_hardware_domain >> and is used only when dealing with accesses that are both required by >> dom0 and where it does not make sense to grant access to a domain other >> than dom0. >> >> There are a number of existing places in the hypervisor that check >> domain_id for equality to zero to make some distinction on dom0; this >> series replaces these checks with is_hardware_domain to be consistent in >> how the hypervisor checks a domain''s access. >> >> Independent changes related to this series: >> [PATCH 01/11] MAINTAINERS: Add myself as XSM maintainer >> [PATCH 08/11] xen/cpupool: prevent a domain from moving itself >> >> Cleanup of IS_PRIV checks that should not be is_hardware_domain: >> [PATCH 02/11] xen/arch/x86: remove IS_PRIV access check bypasses >> [PATCH 03/11] xen/xsm: add hooks for claim >> [PATCH 04/11] hvm: convert access check for nested HVM to XSM >> [PATCH 05/11] xen/arch/x86: remove IS_PRIV_FOR references >> [PATCH 06/11] xen/arch/arm: remove rcu_lock_target_domain_by_id >> >> Replace remaining calls to IS_PRIV: >> [PATCH 07/11] xen: rename IS_PRIV to is_hardware_domain >> >> Use is_hardware_domain locations where (domid == 0) was used: >> [PATCH 09/11] xen: use domid check in is_hardware_domain >> [PATCH 10/11] xen/arch/x86: clarify domid == 0 checks >> [PATCH 11/11] IOMMU: use is_hardware_domain instead of domid == 0 > > While patch 1 went in a few days ago, patch 2 was held up just by > XSA-46, which went public today. Consequently I also committed > patch 2 a few minutes ago.After looking at the XSA-46 patch, I think a the IS_PRIV removal can still be usefully applied to the XEN_DOMCTL_{bind,unbind}_pt_irq functions - I''ll send the patch in a minute for comment.> For the rest of the series, however, I would want you two to work > out the release related aspects, and I''d look into committing parts > that I''m permitted to commit once I saw George''s ack.George: Here are my thoughts on the viability of these patches for a freeze exception; I tried to address them in order from most to least important. Patch 8 fixes a bug with cpupools, although it is unlikely this bug will be hit on a normal setup since it requires the cpupool operation to be invoked from a domain other than dom0. It is also independent of the other patches in this series, and I see no reason not to include it in 4.3. Patches 3-5 just change the behavior with XSM enabled, and makes these access vectors more consistent with the rest of the hypervisor in the use of XSM hooks when doing access checks related to domain permissions. I think there is good reason to include these in 4.3, assuming there are no technical objections. Patch 6 only changes ARM code, and should only change behavior with XSM enabled. Since XSM on ARM is not really tested yet, this is less important for 4.3 - although it allows removing some of the functions that were replaced by XSM hooks, which may help avoid reintroducing users of these functions. Patches 7 and 9-11 introduce is_hardware_domain as a wrapper on (domid == 0). Since the hypervisor does not currently support setting is_privileged on non-dom0 domains, the test is equivalent to IS_PRIV - just a cosmetic change. The changes to .c files in patches 9-11 should produce identical code after preprocessing and so have almost zero chance of introducing a bug.> Patch 10 may, according to the reply I just sent, need another small > tweak before getting applied - as long as you agree, I could certainly > do this while committing. > > JanAssuming that the is_hardware_domain changes go in, the tweak you mentioned looks good to me. -- Daniel De Graaf National Security Agency
Daniel De Graaf
2013-Apr-18 16:11 UTC
[PATCH] xen/arch/x86: remove IS_PRIV bypass on IRQ check
This prevents a process in dom0 from granting a domU access to an IRQ without
adding the IRQ to the domU''s list of permitted IRQs. This operation
currently
succeeds in dom0 but would fail if the device model were running in a stubdom,
so making the failure consistent should ease debugging of the device-model
stubdoms.
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/domctl.c | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 9580390..c71df43 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -566,8 +566,10 @@ long arch_do_domctl(
case XEN_DOMCTL_bind_pt_irq:
{
xen_domctl_bind_pt_irq_t * bind;
+ int irq;
bind = &(domctl->u.bind_pt_irq);
+ irq = domain_pirq_to_irq(d, bind->machine_irq);
ret = -EINVAL;
if ( !is_hvm_domain(d) )
@@ -578,13 +580,8 @@ long arch_do_domctl(
break;
ret = -EPERM;
- if ( !IS_PRIV(current->domain) )
- {
- int irq = domain_pirq_to_irq(d, bind->machine_irq);
-
- if ( irq <= 0 || !irq_access_permitted(current->domain, irq)
)
- break;
- }
+ if ( irq <= 0 || !irq_access_permitted(current->domain, irq) )
+ break;
ret = -ESRCH;
if ( iommu_enabled )
@@ -602,17 +599,14 @@ long arch_do_domctl(
case XEN_DOMCTL_unbind_pt_irq:
{
xen_domctl_bind_pt_irq_t * bind;
+ int irq;
bind = &(domctl->u.bind_pt_irq);
+ irq = domain_pirq_to_irq(d, bind->machine_irq);
ret = -EPERM;
- if ( !IS_PRIV(current->domain) )
- {
- int irq = domain_pirq_to_irq(d, bind->machine_irq);
-
- if ( irq <= 0 || !irq_access_permitted(current->domain, irq)
)
- break;
- }
+ if ( irq <= 0 || !irq_access_permitted(current->domain, irq) )
+ break;
ret = xsm_unbind_pt_irq(XSM_HOOK, d, bind);
if ( ret )
--
1.8.1.4
Jan Beulich
2013-Apr-18 16:20 UTC
Re: [PATCH] xen/arch/x86: remove IS_PRIV bypass on IRQ check
>>> On 18.04.13 at 18:11, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote: > This prevents a process in dom0 from granting a domU access to an IRQ without > adding the IRQ to the domU''s list of permitted IRQs. This operation > currently > succeeds in dom0 but would fail if the device model were running in a > stubdom, > so making the failure consistent should ease debugging of the device-model > stubdoms.I''m sorry for having lost half of your original patch - I know I resolved the conflicts with the security one, but apparently then popped it without first refreshing... Jan> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> > Cc: Keir Fraser <keir@xen.org> > Cc: Jan Beulich <jbeulich@suse.com> > --- > xen/arch/x86/domctl.c | 22 ++++++++-------------- > 1 file changed, 8 insertions(+), 14 deletions(-) > > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c > index 9580390..c71df43 100644 > --- a/xen/arch/x86/domctl.c > +++ b/xen/arch/x86/domctl.c > @@ -566,8 +566,10 @@ long arch_do_domctl( > case XEN_DOMCTL_bind_pt_irq: > { > xen_domctl_bind_pt_irq_t * bind; > + int irq; > > bind = &(domctl->u.bind_pt_irq); > + irq = domain_pirq_to_irq(d, bind->machine_irq); > > ret = -EINVAL; > if ( !is_hvm_domain(d) ) > @@ -578,13 +580,8 @@ long arch_do_domctl( > break; > > ret = -EPERM; > - if ( !IS_PRIV(current->domain) ) > - { > - int irq = domain_pirq_to_irq(d, bind->machine_irq); > - > - if ( irq <= 0 || !irq_access_permitted(current->domain, irq) ) > - break; > - } > + if ( irq <= 0 || !irq_access_permitted(current->domain, irq) ) > + break; > > ret = -ESRCH; > if ( iommu_enabled ) > @@ -602,17 +599,14 @@ long arch_do_domctl( > case XEN_DOMCTL_unbind_pt_irq: > { > xen_domctl_bind_pt_irq_t * bind; > + int irq; > > bind = &(domctl->u.bind_pt_irq); > + irq = domain_pirq_to_irq(d, bind->machine_irq); > > ret = -EPERM; > - if ( !IS_PRIV(current->domain) ) > - { > - int irq = domain_pirq_to_irq(d, bind->machine_irq); > - > - if ( irq <= 0 || !irq_access_permitted(current->domain, irq) ) > - break; > - } > + if ( irq <= 0 || !irq_access_permitted(current->domain, irq) ) > + break; > > ret = xsm_unbind_pt_irq(XSM_HOOK, d, bind); > if ( ret ) > -- > 1.8.1.4
Jürgen Groß
2013-Apr-19 09:43 UTC
Re: [PATCH 08/11] xen/cpupool: prevent a domain from moving itself
Am 12.04.2013 23:04, schrieb Daniel De Graaf:> In the XEN_SYSCTL_CPUPOOL_OP_MOVEDOMAIN operation, the existing check > for domid == 0 should be checking that a domain does not attempt to > modify its own cpupool; fix this by using rcu_lock_remote_domain_by_id. > > Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> > Cc: Juergen Gross <juergen.gross@ts.fujitsu.com> > Cc: Jan Beulich <JBeulich@suse.com>Sorry for late answer, but I''m on vacation and reading mails not very often... Acked-by: Juergen Gross <juergen.gross@ts.fujitsu.com>> --- > xen/common/cpupool.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c > index 10b10f8..c901f7e 100644 > --- a/xen/common/cpupool.c > +++ b/xen/common/cpupool.c > @@ -546,12 +546,8 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op) > { > struct domain *d; > > - ret = -EINVAL; > - if ( op->domid == 0 ) > - break; > - ret = -ESRCH; > - d = rcu_lock_domain_by_id(op->domid); > - if ( d == NULL ) > + ret = rcu_lock_remote_domain_by_id(op->domid, &d); > + if ( ret ) > break; > if ( d->cpupool == NULL ) > { >
On 18/04/13 17:10, Daniel De Graaf wrote:> On 04/18/2013 11:12 AM, Jan Beulich wrote: >>>>> On 12.04.13 at 23:04, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote: >>> Changes since v2: >>> - Handle XEN_SYSCTL_CPUPOOL_OP_MOVEDOMAIN separately >>> - Use is_control_domain for CPUID >>> - Use is_{control,hardware}_domain for TSC >>> - MAINTAINERS patch included in series >>> >>> --- >>> >>> Following the conversion of most IS_PRIV hooks to XSM, the remaining >>> references to this function generally deal with direct hardware access >>> and not with the type of privilege checks that are best controlled by >>> XSM. To reflect this, the IS_PRIV check is renamed to is_hardware_domain >>> and is used only when dealing with accesses that are both required by >>> dom0 and where it does not make sense to grant access to a domain other >>> than dom0. >>> >>> There are a number of existing places in the hypervisor that check >>> domain_id for equality to zero to make some distinction on dom0; this >>> series replaces these checks with is_hardware_domain to be consistent in >>> how the hypervisor checks a domain''s access. >>> >>> Independent changes related to this series: >>> [PATCH 01/11] MAINTAINERS: Add myself as XSM maintainer >>> [PATCH 08/11] xen/cpupool: prevent a domain from moving itself >>> >>> Cleanup of IS_PRIV checks that should not be is_hardware_domain: >>> [PATCH 02/11] xen/arch/x86: remove IS_PRIV access check bypasses >>> [PATCH 03/11] xen/xsm: add hooks for claim >>> [PATCH 04/11] hvm: convert access check for nested HVM to XSM >>> [PATCH 05/11] xen/arch/x86: remove IS_PRIV_FOR references >>> [PATCH 06/11] xen/arch/arm: remove rcu_lock_target_domain_by_id >>> >>> Replace remaining calls to IS_PRIV: >>> [PATCH 07/11] xen: rename IS_PRIV to is_hardware_domain >>> >>> Use is_hardware_domain locations where (domid == 0) was used: >>> [PATCH 09/11] xen: use domid check in is_hardware_domain >>> [PATCH 10/11] xen/arch/x86: clarify domid == 0 checks >>> [PATCH 11/11] IOMMU: use is_hardware_domain instead of domid == 0 >> While patch 1 went in a few days ago, patch 2 was held up just by >> XSA-46, which went public today. Consequently I also committed >> patch 2 a few minutes ago. > After looking at the XSA-46 patch, I think a the IS_PRIV removal can still > be usefully applied to the XEN_DOMCTL_{bind,unbind}_pt_irq functions - I''ll > send the patch in a minute for comment. > >> For the rest of the series, however, I would want you two to work >> out the release related aspects, and I''d look into committing parts >> that I''m permitted to commit once I saw George''s ack. > George: > > Here are my thoughts on the viability of these patches for a freeze > exception; I tried to address them in order from most to least important. > > Patch 8 fixes a bug with cpupools, although it is unlikely this bug will > be hit on a normal setup since it requires the cpupool operation to be > invoked from a domain other than dom0. It is also independent of the other > patches in this series, and I see no reason not to include it in 4.3. > > Patches 3-5 just change the behavior with XSM enabled, and makes these > access vectors more consistent with the rest of the hypervisor in the use > of XSM hooks when doing access checks related to domain permissions. I > think there is good reason to include these in 4.3, assuming there are no > technical objections. > > Patch 6 only changes ARM code, and should only change behavior with XSM > enabled. Since XSM on ARM is not really tested yet, this is less important > for 4.3 - although it allows removing some of the functions that were replaced > by XSM hooks, which may help avoid reintroducing users of these functions. > > Patches 7 and 9-11 introduce is_hardware_domain as a wrapper on (domid == 0). > Since the hypervisor does not currently support setting is_privileged on > non-dom0 domains, the test is equivalent to IS_PRIV - just a cosmetic change. > The changes to .c files in patches 9-11 should produce identical code after > preprocessing and so have almost zero chance of introducing a bug.Remind me what the main benefit of this patch series is? Does this allow a greater degree of disaggregation? And, given that this is primarily about security, do you really want a brand new feature in users'' hands with only 8 weeks of in-tree testing? -George
On 04/22/2013 08:13 AM, George Dunlap wrote:> On 18/04/13 17:10, Daniel De Graaf wrote: >> On 04/18/2013 11:12 AM, Jan Beulich wrote: >>>>>> On 12.04.13 at 23:04, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote: >>>> Changes since v2: >>>> - Handle XEN_SYSCTL_CPUPOOL_OP_MOVEDOMAIN separately >>>> - Use is_control_domain for CPUID >>>> - Use is_{control,hardware}_domain for TSC >>>> - MAINTAINERS patch included in series >>>> >>>> --- >>>> >>>> Following the conversion of most IS_PRIV hooks to XSM, the remaining >>>> references to this function generally deal with direct hardware access >>>> and not with the type of privilege checks that are best controlled by >>>> XSM. To reflect this, the IS_PRIV check is renamed to is_hardware_domain >>>> and is used only when dealing with accesses that are both required by >>>> dom0 and where it does not make sense to grant access to a domain other >>>> than dom0. >>>> >>>> There are a number of existing places in the hypervisor that check >>>> domain_id for equality to zero to make some distinction on dom0; this >>>> series replaces these checks with is_hardware_domain to be consistent in >>>> how the hypervisor checks a domain''s access. >>>> >>>> Independent changes related to this series: >>>> [PATCH 01/11] MAINTAINERS: Add myself as XSM maintainer >>>> [PATCH 08/11] xen/cpupool: prevent a domain from moving itself >>>> >>>> Cleanup of IS_PRIV checks that should not be is_hardware_domain: >>>> [PATCH 02/11] xen/arch/x86: remove IS_PRIV access check bypasses >>>> [PATCH 03/11] xen/xsm: add hooks for claim >>>> [PATCH 04/11] hvm: convert access check for nested HVM to XSM >>>> [PATCH 05/11] xen/arch/x86: remove IS_PRIV_FOR references >>>> [PATCH 06/11] xen/arch/arm: remove rcu_lock_target_domain_by_id >>>> >>>> Replace remaining calls to IS_PRIV: >>>> [PATCH 07/11] xen: rename IS_PRIV to is_hardware_domain >>>> >>>> Use is_hardware_domain locations where (domid == 0) was used: >>>> [PATCH 09/11] xen: use domid check in is_hardware_domain >>>> [PATCH 10/11] xen/arch/x86: clarify domid == 0 checks >>>> [PATCH 11/11] IOMMU: use is_hardware_domain instead of domid == 0 >>> While patch 1 went in a few days ago, patch 2 was held up just by >>> XSA-46, which went public today. Consequently I also committed >>> patch 2 a few minutes ago. >> After looking at the XSA-46 patch, I think a the IS_PRIV removal can still >> be usefully applied to the XEN_DOMCTL_{bind,unbind}_pt_irq functions - I''ll >> send the patch in a minute for comment. >> >>> For the rest of the series, however, I would want you two to work >>> out the release related aspects, and I''d look into committing parts >>> that I''m permitted to commit once I saw George''s ack. >> George: >> >> Here are my thoughts on the viability of these patches for a freeze >> exception; I tried to address them in order from most to least important. >> >> Patch 8 fixes a bug with cpupools, although it is unlikely this bug will >> be hit on a normal setup since it requires the cpupool operation to be >> invoked from a domain other than dom0. It is also independent of the other >> patches in this series, and I see no reason not to include it in 4.3. >> >> Patches 3-5 just change the behavior with XSM enabled, and makes these >> access vectors more consistent with the rest of the hypervisor in the use >> of XSM hooks when doing access checks related to domain permissions. I >> think there is good reason to include these in 4.3, assuming there are no >> technical objections. >> >> Patch 6 only changes ARM code, and should only change behavior with XSM >> enabled. Since XSM on ARM is not really tested yet, this is less important >> for 4.3 - although it allows removing some of the functions that were replaced >> by XSM hooks, which may help avoid reintroducing users of these functions. >> >> Patches 7 and 9-11 introduce is_hardware_domain as a wrapper on (domid == 0). >> Since the hypervisor does not currently support setting is_privileged on >> non-dom0 domains, the test is equivalent to IS_PRIV - just a cosmetic change. >> The changes to .c files in patches 9-11 should produce identical code after >> preprocessing and so have almost zero chance of introducing a bug. > > Remind me what the main benefit of this patch series is? Does this allow a greater degree of disaggregation? > > And, given that this is primarily about security, do you really want a brand new feature in users'' hands with only 8 weeks of in-tree testing? > > -GeorgePatch 8 is a pure bugfix, not really security related except that it fixes a deadlock from an interface usually restricted to dom0 only. The main benefit of the series is the increased granularity in control for the additional access vectors, so that a disaggregated system can limit each component to the minimal access it requires. Most of this was already done in prior patches; the remainder here (patches 3-6) just touches code that either wasn''t in xen-unstable at the time I submitted the original IS_PRIV->XSM conversion, or code that was left alone because it was marked as a hack. Patches 7,9,10,11 are more function renames than code changes, and could easily wait for 4.4 - the primary reason I posted them for 4.3 is to avoid additional users of IS_PRIV being introduced during the freeze or early in 4.4 (by IanC''s suggestion), and because they are fairly trivial. Regarding only 8 weeks'' testing: assuming the patch doesn''t actually break the logic it changes (I believe it doesn''t, and it would be noticed rather quickly if it did), the only change that it would require is adding the new permissions to the XSM policy if they are used where they have not already been added. This is something that users who modify the example policy already need to do for other permissions as part of their changes. -- Daniel De Graaf National Security Agency
On 22/04/13 16:05, Daniel De Graaf wrote:> On 04/22/2013 08:13 AM, George Dunlap wrote: >> On 18/04/13 17:10, Daniel De Graaf wrote: >>> On 04/18/2013 11:12 AM, Jan Beulich wrote: >>>>>>> On 12.04.13 at 23:04, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote: >>>>> Changes since v2: >>>>> - Handle XEN_SYSCTL_CPUPOOL_OP_MOVEDOMAIN separately >>>>> - Use is_control_domain for CPUID >>>>> - Use is_{control,hardware}_domain for TSC >>>>> - MAINTAINERS patch included in series >>>>> >>>>> --- >>>>> >>>>> Following the conversion of most IS_PRIV hooks to XSM, the remaining >>>>> references to this function generally deal with direct hardware access >>>>> and not with the type of privilege checks that are best controlled by >>>>> XSM. To reflect this, the IS_PRIV check is renamed to is_hardware_domain >>>>> and is used only when dealing with accesses that are both required by >>>>> dom0 and where it does not make sense to grant access to a domain other >>>>> than dom0. >>>>> >>>>> There are a number of existing places in the hypervisor that check >>>>> domain_id for equality to zero to make some distinction on dom0; this >>>>> series replaces these checks with is_hardware_domain to be consistent in >>>>> how the hypervisor checks a domain''s access. >>>>> >>>>> Independent changes related to this series: >>>>> [PATCH 01/11] MAINTAINERS: Add myself as XSM maintainer >>>>> [PATCH 08/11] xen/cpupool: prevent a domain from moving itself >>>>> >>>>> Cleanup of IS_PRIV checks that should not be is_hardware_domain: >>>>> [PATCH 02/11] xen/arch/x86: remove IS_PRIV access check bypasses >>>>> [PATCH 03/11] xen/xsm: add hooks for claim >>>>> [PATCH 04/11] hvm: convert access check for nested HVM to XSM >>>>> [PATCH 05/11] xen/arch/x86: remove IS_PRIV_FOR references >>>>> [PATCH 06/11] xen/arch/arm: remove rcu_lock_target_domain_by_id >>>>> >>>>> Replace remaining calls to IS_PRIV: >>>>> [PATCH 07/11] xen: rename IS_PRIV to is_hardware_domain >>>>> >>>>> Use is_hardware_domain locations where (domid == 0) was used: >>>>> [PATCH 09/11] xen: use domid check in is_hardware_domain >>>>> [PATCH 10/11] xen/arch/x86: clarify domid == 0 checks >>>>> [PATCH 11/11] IOMMU: use is_hardware_domain instead of domid == 0 >>>> While patch 1 went in a few days ago, patch 2 was held up just by >>>> XSA-46, which went public today. Consequently I also committed >>>> patch 2 a few minutes ago. >>> After looking at the XSA-46 patch, I think a the IS_PRIV removal can still >>> be usefully applied to the XEN_DOMCTL_{bind,unbind}_pt_irq functions - I''ll >>> send the patch in a minute for comment. >>> >>>> For the rest of the series, however, I would want you two to work >>>> out the release related aspects, and I''d look into committing parts >>>> that I''m permitted to commit once I saw George''s ack. >>> George: >>> >>> Here are my thoughts on the viability of these patches for a freeze >>> exception; I tried to address them in order from most to least important. >>> >>> Patch 8 fixes a bug with cpupools, although it is unlikely this bug will >>> be hit on a normal setup since it requires the cpupool operation to be >>> invoked from a domain other than dom0. It is also independent of the other >>> patches in this series, and I see no reason not to include it in 4.3. >>> >>> Patches 3-5 just change the behavior with XSM enabled, and makes these >>> access vectors more consistent with the rest of the hypervisor in the use >>> of XSM hooks when doing access checks related to domain permissions. I >>> think there is good reason to include these in 4.3, assuming there are no >>> technical objections. >>> >>> Patch 6 only changes ARM code, and should only change behavior with XSM >>> enabled. Since XSM on ARM is not really tested yet, this is less important >>> for 4.3 - although it allows removing some of the functions that were replaced >>> by XSM hooks, which may help avoid reintroducing users of these functions. >>> >>> Patches 7 and 9-11 introduce is_hardware_domain as a wrapper on (domid == 0). >>> Since the hypervisor does not currently support setting is_privileged on >>> non-dom0 domains, the test is equivalent to IS_PRIV - just a cosmetic change. >>> The changes to .c files in patches 9-11 should produce identical code after >>> preprocessing and so have almost zero chance of introducing a bug. >> Remind me what the main benefit of this patch series is? Does this allow a greater degree of disaggregation? >> >> And, given that this is primarily about security, do you really want a brand new feature in users'' hands with only 8 weeks of in-tree testing? >> >> -George > Patch 8 is a pure bugfix, not really security related except that it > fixes a deadlock from an interface usually restricted to dom0 only. > > The main benefit of the series is the increased granularity in control > for the additional access vectors, so that a disaggregated system can > limit each component to the minimal access it requires. Most of this > was already done in prior patches; the remainder here (patches 3-6) just > touches code that either wasn''t in xen-unstable at the time I submitted > the original IS_PRIV->XSM conversion, or code that was left alone because > it was marked as a hack. > > Patches 7,9,10,11 are more function renames than code changes, and could > easily wait for 4.4 - the primary reason I posted them for 4.3 is to > avoid additional users of IS_PRIV being introduced during the freeze or > early in 4.4 (by IanC''s suggestion), and because they are fairly trivial. > > Regarding only 8 weeks'' testing: assuming the patch doesn''t actually break > the logic it changes (I believe it doesn''t, and it would be noticed rather > quickly if it did), the only change that it would require is adding the new > permissions to the XSM policy if they are used where they have not already > been added. This is something that users who modify the example policy > already need to do for other permissions as part of their changes.Thanks for the explanation. It sounds like these should mainly be low-risk changes, so re a release perspective: Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
>>> On 22.04.13 at 17:09, George Dunlap <george.dunlap@eu.citrix.com> wrote: > Thanks for the explanation. It sounds like these should mainly be > low-risk changes, so re a release perspective: > > Acked-by: George Dunlap <george.dunlap@eu.citrix.com>I applied what I''m allowed to without (and what is independent of) further acks. Jan
>>> On 22.04.13 at 17:09, George Dunlap <george.dunlap@eu.citrix.com> wrote: > Thanks for the explanation. It sounds like these should mainly be > low-risk changes, so re a release perspective: > > Acked-by: George Dunlap <george.dunlap@eu.citrix.com>Daniel, as said elsewhere before, without suitable acks (and George''s is required, but not sufficient) these won''t go in, and I think it is your responsibility to follow up with the necessary people. I''m sure you''re aware that we''re progressing towards more strict code freeze, and hence chances reduce over time that the remaining patches in this series will make it into 4.3. Jan