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

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 09/11/2012 04:54 AM, Ian Campbell wrote:
> On Mon, 2012-09-10 at 22:35 +0100, Keir Fraser wrote:
>> On 10/09/2012 22:10, "Daniel De Graaf" <dgdegra@tycho.nsa.gov> wrote:
>>
>>> On 09/10/2012 04:51 PM, Keir Fraser wrote:
>>>> On 10/09/2012 20:48, "Daniel De Graaf" <dgdegra@tycho.nsa.gov> 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.

This is almost certainly going to confuse the compiler, since often the
XSM hook is after some minimal parameter validation which will break on
failure. Using an initialized variable would work, but I like the other
option better.

> 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 even just default back to IS_PRIV, and a FLASK unknown_domctl permission
that is documented as only for development (or just -EPERM in FLASK, since
most developers who would turn that on would also be interested in adding
the full XSM hook).

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


-- 
Daniel De Graaf
National Security Agency