Re: [Xen-devel] [PATCH 06/19] xen: use XSM instead of IS_PRIV where duplicated

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 11/19/2012 06:00 AM, Ian Campbell wrote:
> On Fri, 2012-11-16 at 18:28 +0000, Daniel De Graaf wrote:
>> The Xen hypervisor has two basic access control function calls: IS_PRIV
>> and the xsm_* functions. Most privileged operations currently require
>> that both checks succeed, and many times the checks are at different
>> locations in the code. This patch eliminates the explicit and implicit
>> IS_PRIV checks that are duplicated in XSM hooks.
> 
> The downside is that you loose some context in the hypercall
> implementation, i.e. the hint about whether a hypercall is available
> only to the toolstack, or to only stubdomains or more generally to all
> guests etc which is useful to have in front of you when changing the
> code.
> 
> IOW I worry that we may end up introducing bugs due to hiding this
> context away behind the xsm wrapper, e.g. you think a function is
> privileged guest only but actually it is guest accessible.
> 
> I wonder if there is any way we can get the benefits of this change
> without this downside?
> 
> Perhaps in the name of the hook? e.g. For each existing xsm_<foo> we
> instead define exactly one of xsm_guest_<foo>, xsm_stubdom_<foo> or
> xsm_priv_<foo>. This might also let us define some macros for use in
> dummy.[ch] which simultaneously construct the correct function name and
> include the appropriate boilerplate perm check thus ensuring they don't
> get out of sync.

I think something like this would help clarify the intended purpose of the
access control check. I think it may also be useful to split "priv" further
into "dom0" for hardware functionality and "ctl" for domain administration;
without looking at the hooks in detail, that should cover most categories.

One other problem that I would like to resolve is that the XSM functions
in dummy.h aren't visible to ctags; it might be useful to name the dummy
functions identically to the XSM hooks and construct dummy.c to avoid
including the duplicate definitions.

>> Some checks are removed due to non-obvious duplicates in their callers:
> 
> If the duplicates are non-obvious wouldn't it be better to keep them as
> a belt-and-braces measure?
> 
> Ian.

In this case, "non-obvious" meant "not obvious from a patch context". Keeping
the duplicated calls would require either leaving the IS_PRIV checks (where
the whole point of this patch is to remove them) or creating new, redundant
XSM hooks.

-- 
Daniel De Graaf
National Security Agency

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