Re: [PATCH v3] xsm: add a default policy to .init.data

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Jun 30, 2016 at 10:01:18AM -0400, Daniel De Graaf wrote:
> On 06/30/2016 09:45 AM, Konrad Rzeszutek Wilk wrote:
> > On Wed, Jun 29, 2016 at 11:09:01AM -0400, Daniel De Graaf wrote:
> > > This adds a Kconfig option and support for including the XSM policy from
> > > tools/flask/policy in the hypervisor so that the bootloader does not
> > > need to provide a policy to get sane behavior from an XSM-enabled
> > > hypervisor.  The policy provided by the bootloader, if present, will
> > > override the built-in policy.
> > > 
> > > The XSM policy is not moved out of tools because that remains the
> > > primary location for installing and configuring the policy.
> > > 
> > > Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> > > ---
> > > 
> > > Changes from v2 (dropped acks and reviewed-by):
> > >  - Drop linker script changes, use python binary-to-C file script
> > >  - Make the config option always include the policy if selected
> > >  - Note the new conditional dependency on checkpolicy in INSTALL
> > 
> > I liked the previous patch of putting in it in __init section.
> > Is that something this patch could do? Ah, n/m. I see that
> > the python script generates the binary with __init!
> > 
> > Secondly I was wondering why the suggestion I had - which was to check
> > of the 'checkpolicy' availability - and if not found - then
> > hide the Kconfig option was not mentioned?
> 
> At the moment, since FLASK is the only XSM implementation and it is not
> enabled by default, if someone enables XSM they will need to generate a
> security policy at some point: either during the hypervisor build or in
> the tools build.  Otherwise, they will need to disable FLASK at runtime
> or (worst case) run without a policy at all, which means all domains can
> act as dom0.  If you plan to disable FLASK at runtime, it is better to
> compile without XSM enabled so you don't pay the cost of the function
> calls to the XSM hooks on (almost) every hypercall.
> 
> Otherwise, I was planning to just change the default from always "y" to
> "y" if checkpolicy was found and "n" if not.  This would end up running
> "checkpolicy -h" on every (recursive) Make invocation if placed in the
> top-level Config.mk - I'm unsure if that would be a problem or not.

We already do quite a lot of other checks in there. I am not sure if
an extra one would be so bad. Do other maintainers care about it?
> 
> > .. snip...
> > > +sys.stdout.write("\n};\nconst int __init xsm_init_policy_size = %d;\n" % policy_size)
> > > diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
> > > index 8df1a3c..93c7d43 100644
> > > --- a/xen/xsm/xsm_core.c
> > > +++ b/xen/xsm/xsm_core.c
> > > @@ -36,6 +36,24 @@ static inline int verify(struct xsm_operations *ops)
> > >      return 0;
> > >  }
> > > 
> > > +#ifdef CONFIG_XSM_POLICY
> > > +extern char xsm_init_policy[];
> > 
> > > +extern int xsm_init_policy_size;
> > > +#else
> > > +#define xsm_init_policy 0
> > > +#endif
> > > +
> > > +static void __init xsm_policy_init(void)
> > > +{
> > > +#ifdef CONFIG_XSM_POLICY
> > > +    if ( policy_size == 0 )
> > > +    {
> > > +        policy_buffer = xsm_init_policy;
> > > +        policy_size = xsm_init_policy_size;
> > > +    }
> > > +#endif
> > > +}
> > > +
> > 
> > This all looks like it could go in a header file?
> 
> Sure, I could move it to xsm.h.  I thought it might be clearer to have the
> assignment and the xfree() check in the same file.

You are the maintainer of that code, so it is your call.

Having externs in C code is not that great (from my perspective),
so if you move all of this to a header file that should get easier?

But as said - you are the maintainer and I am just voicing my
preferences - and if the code looks "nicer" your way - by all means
do it that way! I am not going to get offended :-)


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