Re: [Xen-devel] [PATCH v2] Merge IS_PRIV checks into XSM hooks

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2012-09-10 at 22:35 +0100, Keir Fraser wrote:
> On 10/09/2012 22:10, "Daniel De Graaf" <dgdegra@xxxxxxxxxxxxx> wrote:
> 
> > On 09/10/2012 04:51 PM, Keir Fraser wrote:
> >> On 10/09/2012 20:48, "Daniel De Graaf" <dgdegra@xxxxxxxxxxxxx> wrote:
> >> 
> >>> Overall, this series should not change the behavior of Xen when XSM is
> >>> not enabled; however, in some cases, the exact errors that are returned
> >>> will be different because security checks have been moved below validity
> >>> checks. Also, once applied, newly introduced domctls and sysctls will
> >>> not automatically be guarded by IS_PRIV checks - they will need to add
> >>> their own permission checking code.
> >> 
> >> How do we guard against accidentally forgetting to do this?
> > 
> > The same way you guard against it when adding a new hypercall: when adding
> > new functionality that needs access checks, also add the access checks.
> 
> So... We just shouldn't accidentally forget. That will work well. ;)
> Historically XSM has not been top of many committers' checklists.

Can we play tricks like

        int did_priv_check;

        switch(op)
        case FOO:
                if (xsm_check_foo(&did_priv_check))
                ....
                break;
        }
        ASSERT(did_priv_check)

I'd expect the compiler to catch missing checks due to the use of the
uninitialised var, although that relies somewhat on the switch not being
too big and confusing it.

The other option might be to define domctl_do_perm_check, which has a
similar switch over op but an explicit default: return -EPERM. Then call
that from the top of do_domctl instead of scattering the priv checks
throughout the main switch statement? That way if you forget to add the
perm check it simply won't work and you'll see that the first time you
try and test it...

Or an array of do_perm_check function pointers of NR_DOMCTL in size and
a NULL check? The differing prototypes probably make this one a
non-starter without loads of per op helper functions.

Ian.

>  -- Keir
> 
> >>> The ARM architecture is not touched at all in these patches. The only
> >>> obvious breakage that I can see is due to rcu_lock_target_domain_by_id
> >>> being removed, but XSM hooks will be needed for domctls and sysctls.
> >> 
> >> So ARM build is broken? And/or ARM is made insecure because of unchecked
> >> sysctls/domctls?
> >> 
> >>  -- Keir
> > 
> > The ARM build is broken by patch #19 in this series; fixing it is fairly
> > simple (I'll send a non-compile-tested version as 21/20), or you could
> > postpone that patch as it's just cleanup.
> > 
> > Since ARM doesn't have any arch-specific domctls or sysctls yet, they are
> > not insecure. You could also add an IS_PRIV check at the top of ARM's
> > arch_do_{dom,sys}ctl functions if you don't want to add XSM hooks for each
> > operation as in x86.
> > 
> >> 
> >>> The rcu_lock_target_domain_by_id and rcu_lock_remote_target_domain_by_id
> >>> functions are removed by this series because they act as wrappers around
> >>> IS_PRIV_FOR; their callers have been changed to use XSM checks instead.
> >>> 
> >>> Miscellaneous updates to FLASK:
> >>>     [PATCH 01/20] xsm/flask: remove inherited class attributes
> >>>     [PATCH 02/20] xsm/flask: remove unneeded create_sid field
> >>>     [PATCH 03/20] xen: Add versions of rcu_lock_*_domain without IS_PRIV
> >>>     [PATCH 04/20] xsm/flask: add domain relabel support
> >>>     [PATCH 05/20] libxl: introduce XSM relabel on build
> >>>     [PATCH 06/20] flask/policy: Add domain relabel example
> >>> 
> >>> Preparatory new hooks:
> >>>     [PATCH 07/20] arch/x86: add distinct XSM hooks for map/unmap
> >>>     [PATCH 08/20] arch/x86: add missing XSM checks to XENPF_ commands
> >>>     [PATCH 09/20] xsm/flask: Add checks on the domain performing the
> >>> 
> >>> Refactoring:
> >>>     [PATCH 10/20] xsm: Add IS_PRIV checks to dummy XSM module
> >>>     [PATCH 11/20] xen: use XSM instead of IS_PRIV where duplicated
> >>>     [PATCH 12/20] xen: avoid calling rcu_lock_*target_domain when an XSM
> >>> 
> >>> Remaining IS_PRIV calls:
> >>>     [PATCH 13/20] arch/x86: Add missing domctl and mem_sharing XSM hooks
> >>>     [PATCH 14/20] tmem: Add access control check
> >>>     [PATCH 17/20] arch/x86: use XSM hooks for get_pg_owner access checks
> >>>     [PATCH 18/20] xen: Add XSM hook for XENMEM_exchange
> >>> 
> >>> Cleanup, FLASK updates to support IS_PRIV emulation:
> >>>     [PATCH 15/20] xsm: remove unneeded xsm_call macro
> >>>     [PATCH 16/20] xsm/flask: add distinct SIDs for self/target access
> >>>     [PATCH 19/20] xen: remove rcu_lock_{remote_,}target_domain_by_id
> >>>     [PATCH 20/20] flask: add missing operations
> >>> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



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