Re: [Xen-devel] [PATCH RFC 5/5] xen/xsm: include default hook action in name

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 11/22/2012 11:57 AM, Tim Deegan wrote:
> Hi, 
> 
> At 09:51 -0500 on 20 Nov (1353405098), Daniel De Graaf wrote:
>> Include the default XSM hook action in the name of the hook in order to
>> allow quick understanding of how the call site is expected to be used
>> (dom0-only, arbitrary guest, or target-only).
> 
> I like this a lot; it's much less worrying than having to follow every
> xsm hook to see what it does. 
> 
> I would like it even more if the process were automatic -- as it stands
> your dummy hooks must be manually coded to do what their name implies,
> and if the code gets out of sync with the name it could be a long time
> before anyone spotted it.

That's one thing I didn't like about this scheme - it's effectively 
coding a default security policy into function names, so changing that 
policy will make larger-than-necessary diffs, assuming all the locations
are changed (although this will produce compile errors if tested with
both XSM enabled and disabled, so not a big problem). However, I don't
think the behavior of the dummy hooks will change very much. In the
event of a change, it is more likely that parameters (such as a new
struct domain*) will need to be added in order to facilitate the new
permission check, so changing the name at that time should be easy to
catch.

> Would it be acceptable for every XSM hook to take a default action as
> its first argument?  Then the call sites look like:
> 
>   xsm_do_mca(XSM_PRIV),
>   xsm_unbind_pt_irq(XSM_HOOK, d, bind);
> 
> and so forth, and every one of the dummy hooks is just a wrapper around
> a single central hook that DTRT with the default:
> 
>   static XSM_INLINE int xsm_do_mca(xsm_default_t default)
>   {
>       return xsm_default_action(default);
>   }
> 
>   static XSM_INLINE int xsm_unbind_pt_irq(xsm_default_t default, struct 
> domain *d, struct xen_domctl_bind_pt_irq *bind)
>   {
>       return xsm_default_action(default);
>   }
> 
> and so forth.  It should be very hard to get it wrong.  I guess there
> might have to be a standard order for things that take a domain or an
> mfn, but do you think something along those lines could work?

This would not work for all the hooks, although it would work for most:
domctl and mmu_update are both too complex for this type of hack, and
console_io would need its #define moved back up to the source. For the
other hooks, you would have to code in the domain arguments manually
(sometimes current->domain is available, sometimes it's not) so it's not
automatically an improvement in readability since you still have to look
inside dummy.h to see what domain arguments are being checked.

When I first looked at this, I had thought that it could be implemented
by discarding the xsm_default_t argument if XSM is enabled, but this
would make it very likely that the behavior of the dummy XSM module
would be different from a build with XSM disabled, which we want to
avoid.

And, now that I've actually looked at implementing this, the arguments
to xsm_default_action are difficult to specify. The obvious prototype:
int xsm_default_action(xsm_default_t, struct domain* s, struct domain* d)
would require passing something (either NULL or current->domain) for
those hooks that do not have source and destination domains. This adds 
another possible issue: if someone changes xsm_unbind_pt_irq from 
XSM_HOOK to XSM_TARGET, they might get surprise null dereferences or 
end up checking current->domain instead of the actual target - which 
will pass even when it should not. This also makes the dummy module
larger when compiled: since the compiler can't propagate constants
through a function pointer call, the dummy module will end up needing to
handle all possible values of xsm_default_t even though it will only
ever be called with one.

> If adding an arg is too much overhead I'm sure the dummy ops could 
> be autogenerated from the name with some nasty CPP. :)
> 
> Tim.

I just got rid of some nasty CPP in order to make ctags able to follow
xsm_* to the dummy implementation, which I personally find more helpful
in understanding XSM hooks at a glance - especially if you are trying to
decipher what exactly is implied by the prefixes or constant names.

I think a solution similar to yours could work, although it's not as 
nice looking - use the constants in the source files but have them 
compiled out of the XSM hooks, and have the dummy ops include a
compile-time assertion in the XSM-disabled case to verify that the
constant passed in matches their implementation; something like:

   #define BUILD_ASSERT(x) if (!(x)) cause_linker_error_if_not_inlined_out()
   #ifdef XSM_ENABLE
   /* XSM_INLINE == empty; compiled in to dummy.c */
   #define XSM_DEFAULT_VOID void
   #define XSM_DEFAULT_ARG
   #define XSM_ASSERT_ACTION(action) xsm_default_t default = action
   #else
   /* XSM_INLINE == inline; compiled as a header */
   #define XSM_DEFAULT_VOID xsm_default_t default
   #define XSM_DEFAULT_ARG xsm_default_t default,
   #define XSM_ASSERT_ACTION(action) BUILD_ASSERT(default == action)
   #endif

   static XSM_INLINE int xsm_do_mca(XSM_DEFAULT_VOID)
   {
        XSM_ASSERT_ACTION(XSM_PRIV);
        return xsm_default_action(default, current->domain, NULL);
   }
 
   static XSM_INLINE int xsm_unbind_pt_irq(XSM_DEFAULT_ARG struct domain *d, 
struct xen_domctl_bind_pt_irq *bind)
   {
        XSM_ASSERT_ACTION(XSM_HOOK);
        return xsm_default_action(default, current->domain, d);
   }

Does this seem reasonable? It would force some level of compiler
optimization to be used at all times, but I think that's already
required due to GCC otherwise wasting lots of stack space.

-- 
Daniel De Graaf
National Security Agency

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