Mailing List Archive

[PATCH 20/22] arch/x86: use XSM hooks for get_pg_owner access checks
There are three callers of get_pg_owner:
* do_mmuext_op, which does not have XSM hooks on all subfunctions
* do_mmu_update, which has hooks that are inefficient
* do_update_va_mapping_otherdomain, which has a simple XSM hook

In order to preserve return values for the do_mmuext_op hypercall, an
additional XSM hook is required to check the operation even for those
subfunctions that do not use the pg_owner field. This also covers the
MMUEXT_UNPIN_TABLE operation which did previously have an XSM hook.

The XSM hooks in do_mmu_update were capable of replacing the checks in
get_pg_owner; however, the hooks are buried in the inner loop of the
function - not very good for performance when XSM is enabled and these
turn in to indirect function calls. This patch removes the PTE from the
hooks and replaces it with a bitfield describing what accesses are being
requested. The XSM hook can then be called only when additional bits are
set instead of once per iteration of the loop.

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>
---
tools/flask/policy/policy/flask/access_vectors | 1 +
tools/flask/policy/policy/modules/xen/xen.if | 4 +-
xen/arch/x86/mm.c | 53 +++++++++++--------
xen/include/xsm/dummy.h | 15 ++++--
xen/include/xsm/xsm.h | 25 +++++----
xen/xsm/dummy.c | 4 +-
xen/xsm/flask/hooks.c | 71 +++++++++-----------------
xen/xsm/flask/include/av_perm_to_string.h | 1 +
xen/xsm/flask/include/av_permissions.h | 1 +
9 files changed, 88 insertions(+), 87 deletions(-)

diff --git a/tools/flask/policy/policy/flask/access_vectors b/tools/flask/policy/policy/flask/access_vectors
index 45ac437..8324725 100644
--- a/tools/flask/policy/policy/flask/access_vectors
+++ b/tools/flask/policy/policy/flask/access_vectors
@@ -141,6 +141,7 @@ class mmu
mfnlist
memorymap
remote_remap
+ mmuext_op
}

class shadow
diff --git a/tools/flask/policy/policy/modules/xen/xen.if b/tools/flask/policy/policy/modules/xen/xen.if
index d630f47..725d7a1 100644
--- a/tools/flask/policy/policy/modules/xen/xen.if
+++ b/tools/flask/policy/policy/modules/xen/xen.if
@@ -7,7 +7,7 @@
################################################################################
define(`declare_domain_common', `
allow $1 $2:grant { query setup };
- allow $1 $2:mmu { adjust physmap map_read map_write stat pinpage updatemp };
+ allow $1 $2:mmu { adjust physmap map_read map_write stat pinpage updatemp mmuext_op };
allow $1 $2:hvm { getparam setparam };
')

@@ -51,7 +51,7 @@ define(`create_domain_common', `
allow $1 $2:domain2 { set_cpuid settsc };
allow $1 $2:security check_context;
allow $1 $2:shadow enable;
- allow $1 $2:mmu {map_read map_write adjust memorymap physmap pinpage};
+ 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 };
')
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index c0c2bf3..04a056f 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2619,11 +2619,6 @@ static struct domain *get_pg_owner(domid_t domid)
pg_owner = rcu_lock_domain(dom_io);
break;
case DOMID_XEN:
- if ( !IS_PRIV(curr) )
- {
- MEM_LOG("Cannot set foreign dom");
- break;
- }
pg_owner = rcu_lock_domain(dom_xen);
break;
default:
@@ -2632,12 +2627,6 @@ static struct domain *get_pg_owner(domid_t domid)
MEM_LOG("Unknown domain '%u'", domid);
break;
}
- if ( !IS_PRIV_FOR(curr, pg_owner) )
- {
- MEM_LOG("Cannot set foreign dom");
- rcu_unlock_domain(pg_owner);
- pg_owner = NULL;
- }
break;
}

@@ -2725,6 +2714,13 @@ long do_mmuext_op(
goto out;
}

+ rc = xsm_mmuext_op(d, pg_owner);
+ if ( rc )
+ {
+ rcu_unlock_domain(pg_owner);
+ goto out;
+ }
+
for ( i = 0; i < count; i++ )
{
if ( hypercall_preempt_check() )
@@ -3164,6 +3160,8 @@ long do_mmu_update(
struct vcpu *v = current;
struct domain *d = v->domain, *pt_owner = d, *pg_owner;
struct domain_mmap_cache mapcache;
+ uint32_t xsm_needed = 0;
+ uint32_t xsm_checked = 0;

if ( unlikely(count & MMU_UPDATE_PREEMPTED) )
{
@@ -3195,11 +3193,6 @@ long do_mmu_update(
rc = -EINVAL;
goto out;
}
- if ( !IS_PRIV_FOR(d, pt_owner) )
- {
- rc = -ESRCH;
- goto out;
- }
}

if ( (pg_owner = get_pg_owner((uint16_t)foreigndom)) == NULL )
@@ -3239,9 +3232,20 @@ long do_mmu_update(
{
p2m_type_t p2mt;

- rc = xsm_mmu_normal_update(d, pt_owner, pg_owner, req.val);
- if ( rc )
- break;
+ xsm_needed |= XSM_MMU_NORMAL_UPDATE;
+ if ( get_pte_flags(req.val) & _PAGE_PRESENT )
+ {
+ xsm_needed |= XSM_MMU_UPDATE_READ;
+ if ( get_pte_flags(req.val) & _PAGE_RW )
+ xsm_needed |= XSM_MMU_UPDATE_WRITE;
+ }
+ if ( xsm_needed != xsm_checked )
+ {
+ rc = xsm_mmu_update(d, pt_owner, pg_owner, xsm_needed);
+ if ( rc )
+ break;
+ xsm_checked = xsm_needed;
+ }
rc = -EINVAL;

req.ptr -= cmd;
@@ -3353,9 +3357,14 @@ long do_mmu_update(
mfn = req.ptr >> PAGE_SHIFT;
gpfn = req.val;

- rc = xsm_mmu_machphys_update(d, pg_owner, mfn);
- if ( rc )
- break;
+ xsm_needed |= XSM_MMU_MACHPHYS_UPDATE;
+ if ( xsm_needed != xsm_checked )
+ {
+ rc = xsm_mmu_update(d, pt_owner, pg_owner, xsm_needed);
+ if ( rc )
+ break;
+ xsm_checked = xsm_needed;
+ }

if ( unlikely(!get_page_from_pagenr(mfn, pg_owner)) )
{
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 1760cd9..9c13d5c037 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -662,21 +662,28 @@ static XSM_DEFAULT(int, domain_memory_map) (struct domain *d)
return 0;
}

-static XSM_DEFAULT(int, mmu_normal_update) (struct domain *d, struct domain *t,
- struct domain *f, intpte_t fpte)
+static XSM_DEFAULT(int, mmu_update) (struct domain *d, struct domain *t,
+ struct domain *f, uint32_t flags)
{
+ if ( d != t && !IS_PRIV_FOR(d, t) )
+ return -EPERM;
+ if ( d != f && !IS_PRIV_FOR(d, f) )
+ return -EPERM;
return 0;
}

-static XSM_DEFAULT(int, mmu_machphys_update) (struct domain *d, struct domain *f,
- unsigned long mfn)
+static XSM_DEFAULT(int, mmuext_op) (struct domain *d, struct domain *f)
{
+ if ( d != f && !IS_PRIV_FOR(d, f) )
+ return -EPERM;
return 0;
}

static XSM_DEFAULT(int, update_va_mapping) (struct domain *d, struct domain *f,
l1_pgentry_t pte)
{
+ if ( d != f && !IS_PRIV_FOR(d, f) )
+ return -EPERM;
return 0;
}

diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index c2c1efa..6d970b1 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -27,8 +27,6 @@ typedef u32 xsm_magic_t;
#define XSM_MAGIC 0x00000000
#endif

-#ifdef XSM_ENABLE
-
extern char *policy_buffer;
extern u32 policy_size;

@@ -170,9 +168,13 @@ struct xsm_operations {
int (*getidletime) (void);
int (*machine_memory_map) (void);
int (*domain_memory_map) (struct domain *d);
- int (*mmu_normal_update) (struct domain *d, struct domain *t,
- struct domain *f, intpte_t fpte);
- int (*mmu_machphys_update) (struct domain *d1, struct domain *d2, unsigned long mfn);
+#define XSM_MMU_UPDATE_READ 1
+#define XSM_MMU_UPDATE_WRITE 2
+#define XSM_MMU_NORMAL_UPDATE 4
+#define XSM_MMU_MACHPHYS_UPDATE 8
+ int (*mmu_update) (struct domain *d, struct domain *t,
+ 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 (*add_to_physmap) (struct domain *d1, struct domain *d2);
int (*sendtrigger) (struct domain *d);
@@ -186,6 +188,8 @@ struct xsm_operations {
#endif
};

+#ifdef XSM_ENABLE
+
extern struct xsm_operations *xsm_ops;

static inline void xsm_security_domaininfo (struct domain *d,
@@ -761,16 +765,15 @@ static inline int xsm_domain_memory_map(struct domain *d)
return xsm_ops->domain_memory_map(d);
}

-static inline int xsm_mmu_normal_update (struct domain *d, struct domain *t,
- struct domain *f, intpte_t fpte)
+static inline int xsm_mmu_update (struct domain *d, struct domain *t,
+ struct domain *f, uint32_t flags)
{
- return xsm_ops->mmu_normal_update(d, t, f, fpte);
+ return xsm_ops->mmu_update(d, t, f, flags);
}

-static inline int xsm_mmu_machphys_update (struct domain *d1, struct domain *d2,
- unsigned long mfn)
+static inline int xsm_mmuext_op (struct domain *d, struct domain *f)
{
- return xsm_ops->mmu_machphys_update(d1, d2, mfn);
+ return xsm_ops->mmuext_op(d, f);
}

static inline int xsm_update_va_mapping(struct domain *d, struct domain *f,
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index c82c464..9476be6 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -154,8 +154,8 @@ void xsm_fixup_ops (struct xsm_operations *ops)
set_to_dummy_if_null(ops, getidletime);
set_to_dummy_if_null(ops, machine_memory_map);
set_to_dummy_if_null(ops, domain_memory_map);
- set_to_dummy_if_null(ops, mmu_normal_update);
- set_to_dummy_if_null(ops, mmu_machphys_update);
+ 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, add_to_physmap);
set_to_dummy_if_null(ops, remove_from_physmap);
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index f993696..bd2d792 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1432,65 +1432,44 @@ static int flask_domain_memory_map(struct domain *d)
return current_has_perm(d, SECCLASS_MMU, MMU__MEMORYMAP);
}

-static int domain_memory_perm(struct domain *d, struct domain *f, l1_pgentry_t pte)
+static int flask_mmu_update(struct domain *d, struct domain *t,
+ struct domain *f, uint32_t flags)
{
int rc = 0;
- u32 map_perms = MMU__MAP_READ;
- unsigned long fgfn, fmfn;
- p2m_type_t p2mt;
-
- if ( !(l1e_get_flags(pte) & _PAGE_PRESENT) )
- return 0;
-
- if ( l1e_get_flags(pte) & _PAGE_RW )
- map_perms |= MMU__MAP_WRITE;
-
- fgfn = l1e_get_pfn(pte);
- fmfn = mfn_x(get_gfn_query(f, fgfn, &p2mt));
- put_gfn(f, fgfn);
+ u32 map_perms = 0;

- if ( f->domain_id == DOMID_IO || !mfn_valid(fmfn) )
- {
- struct avc_audit_data ad;
- u32 dsid, fsid;
- rc = security_iomem_sid(fmfn, &fsid);
- if ( rc )
- return rc;
- AVC_AUDIT_DATA_INIT(&ad, MEMORY);
- ad.sdom = d;
- ad.tdom = f;
- ad.memory.pte = pte.l1;
- ad.memory.mfn = fmfn;
- dsid = domain_sid(d);
- return avc_has_perm(dsid, fsid, SECCLASS_MMU, map_perms, &ad);
- }
-
- return domain_has_perm(d, f, SECCLASS_MMU, map_perms);
-}
-
-static int flask_mmu_normal_update(struct domain *d, struct domain *t,
- struct domain *f, intpte_t fpte)
-{
- int rc = 0;
-
- if (d != t)
+ if ( (flags & XSM_MMU_NORMAL_UPDATE) && d != t )
rc = domain_has_perm(d, t, SECCLASS_MMU, MMU__REMOTE_REMAP);
if ( rc )
return rc;

- return domain_memory_perm(d, f, l1e_from_intpte(fpte));
+ if ( flags & XSM_MMU_UPDATE_READ )
+ map_perms |= MMU__MAP_READ;
+ if ( flags & XSM_MMU_UPDATE_WRITE )
+ map_perms |= MMU__MAP_WRITE;
+ if ( flags & XSM_MMU_MACHPHYS_UPDATE )
+ map_perms |= MMU__UPDATEMP;
+
+ if ( map_perms )
+ rc = domain_has_perm(d, f, SECCLASS_MMU, map_perms);
+ return rc;
}

-static int flask_mmu_machphys_update(struct domain *d1, struct domain *d2,
- unsigned long mfn)
+static int flask_mmuext_op(struct domain *d, struct domain *f)
{
- return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__UPDATEMP);
+ return domain_has_perm(d, f, SECCLASS_MMU, MMU__MMUEXT_OP);
}

static int flask_update_va_mapping(struct domain *d, struct domain *f,
l1_pgentry_t pte)
{
- return domain_memory_perm(d, f, pte);
+ u32 map_perms = MMU__MAP_READ;
+ if ( !(l1e_get_flags(pte) & _PAGE_PRESENT) )
+ return 0;
+ if ( l1e_get_flags(pte) & _PAGE_RW )
+ map_perms |= MMU__MAP_WRITE;
+
+ return domain_has_perm(d, f, SECCLASS_MMU, map_perms);
}

static int flask_add_to_physmap(struct domain *d1, struct domain *d2)
@@ -1771,8 +1750,8 @@ static struct xsm_operations flask_ops = {
.getidletime = flask_getidletime,
.machine_memory_map = flask_machine_memory_map,
.domain_memory_map = flask_domain_memory_map,
- .mmu_normal_update = flask_mmu_normal_update,
- .mmu_machphys_update = flask_mmu_machphys_update,
+ .mmu_update = flask_mmu_update,
+ .mmuext_op = flask_mmuext_op,
.update_va_mapping = flask_update_va_mapping,
.add_to_physmap = flask_add_to_physmap,
.remove_from_physmap = flask_remove_from_physmap,
diff --git a/xen/xsm/flask/include/av_perm_to_string.h b/xen/xsm/flask/include/av_perm_to_string.h
index 186f1fa..8f65b96 100644
--- a/xen/xsm/flask/include/av_perm_to_string.h
+++ b/xen/xsm/flask/include/av_perm_to_string.h
@@ -111,6 +111,7 @@
S_(SECCLASS_MMU, MMU__MFNLIST, "mfnlist")
S_(SECCLASS_MMU, MMU__MEMORYMAP, "memorymap")
S_(SECCLASS_MMU, MMU__REMOTE_REMAP, "remote_remap")
+ S_(SECCLASS_MMU, MMU__MMUEXT_OP, "mmuext_op")
S_(SECCLASS_SHADOW, SHADOW__DISABLE, "disable")
S_(SECCLASS_SHADOW, SHADOW__ENABLE, "enable")
S_(SECCLASS_SHADOW, SHADOW__LOGDIRTY, "logdirty")
diff --git a/xen/xsm/flask/include/av_permissions.h b/xen/xsm/flask/include/av_permissions.h
index b3831f6..18454fd 100644
--- a/xen/xsm/flask/include/av_permissions.h
+++ b/xen/xsm/flask/include/av_permissions.h
@@ -117,6 +117,7 @@
#define MMU__MFNLIST 0x00000400UL
#define MMU__MEMORYMAP 0x00000800UL
#define MMU__REMOTE_REMAP 0x00001000UL
+#define MMU__MMUEXT_OP 0x00002000UL

#define SHADOW__DISABLE 0x00000001UL
#define SHADOW__ENABLE 0x00000002UL
--
1.7.11.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH 20/22] arch/x86: use XSM hooks for get_pg_owner access checks [ In reply to ]
>>> On 12.09.12 at 17:59, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
> @@ -3353,9 +3357,14 @@ long do_mmu_update(
> mfn = req.ptr >> PAGE_SHIFT;
> gpfn = req.val;
>
> - rc = xsm_mmu_machphys_update(d, pg_owner, mfn);
> - if ( rc )
> - break;
> + xsm_needed |= XSM_MMU_MACHPHYS_UPDATE;
> + if ( xsm_needed != xsm_checked )
> + {
> + rc = xsm_mmu_update(d, pt_owner, pg_owner, xsm_needed);

If you're already updating it this way, it would seem appropriate
to remove the over-checking here: pt_owner is meaningless for
this operation (there are no page tables involved), and hence
you could/should pass d instead.

Jan

> + if ( rc )
> + break;
> + xsm_checked = xsm_needed;
> + }
>
> if ( unlikely(!get_page_from_pagenr(mfn, pg_owner)) )
> {



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH 20/22] arch/x86: use XSM hooks for get_pg_owner access checks [ In reply to ]
On 09/13/2012 04:13 AM, Jan Beulich wrote:
>>>> On 12.09.12 at 17:59, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>> @@ -3353,9 +3357,14 @@ long do_mmu_update(
>> mfn = req.ptr >> PAGE_SHIFT;
>> gpfn = req.val;
>>
>> - rc = xsm_mmu_machphys_update(d, pg_owner, mfn);
>> - if ( rc )
>> - break;
>> + xsm_needed |= XSM_MMU_MACHPHYS_UPDATE;
>> + if ( xsm_needed != xsm_checked )
>> + {
>> + rc = xsm_mmu_update(d, pt_owner, pg_owner, xsm_needed);
>
> If you're already updating it this way, it would seem appropriate
> to remove the over-checking here: pt_owner is meaningless for
> this operation (there are no page tables involved), and hence
> you could/should pass d instead.
>
> Jan
>

While this is safe, it makes thinking about the arguments to the XSM hook
harder: the second argument would be defined as "pt_owner if called with
XSM_MMU_NORMAL_UPDATE set and either XSM_MMU_MACHPHYS_UPDATE unset or
XSM_MMU_MACHPHYS_UPDATE set in the previous call; otherwise, d." I would
prefer the simpler method of passing pt_owner every time, and only checking
it if XSM_MMU_NORMAL_UPDATE is set (which I now notice that the default
XSM hook does not do, although the FLASK hook does; I'll fix that).

--
Daniel De Graaf
National Security Agency

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH 20/22] arch/x86: use XSM hooks for get_pg_owner access checks [ In reply to ]
>>> On 13.09.12 at 15:55, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
> On 09/13/2012 04:13 AM, Jan Beulich wrote:
>>>>> On 12.09.12 at 17:59, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>>> @@ -3353,9 +3357,14 @@ long do_mmu_update(
>>> mfn = req.ptr >> PAGE_SHIFT;
>>> gpfn = req.val;
>>>
>>> - rc = xsm_mmu_machphys_update(d, pg_owner, mfn);
>>> - if ( rc )
>>> - break;
>>> + xsm_needed |= XSM_MMU_MACHPHYS_UPDATE;
>>> + if ( xsm_needed != xsm_checked )
>>> + {
>>> + rc = xsm_mmu_update(d, pt_owner, pg_owner, xsm_needed);
>>
>> If you're already updating it this way, it would seem appropriate
>> to remove the over-checking here: pt_owner is meaningless for
>> this operation (there are no page tables involved), and hence
>> you could/should pass d instead.
>
> While this is safe, it makes thinking about the arguments to the XSM hook
> harder: the second argument would be defined as "pt_owner if called with
> XSM_MMU_NORMAL_UPDATE set and either XSM_MMU_MACHPHYS_UPDATE unset or
> XSM_MMU_MACHPHYS_UPDATE set in the previous call; otherwise, d." I would
> prefer the simpler method of passing pt_owner every time, and only checking
> it if XSM_MMU_NORMAL_UPDATE is set (which I now notice that the default
> XSM hook does not do, although the FLASK hook does; I'll fix that).

If that's a concern, then rather pass NULL here (and deal with it
in the XSM code), indicating that there are no page tables being
updated.

Jan


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