Re: [Xen-devel] [PATCH v3] xsm: add a default policy to .init.data
On Fri, Jul 01, 2016 at 01:19:51AM -0600, Jan Beulich wrote: > >>> On 30.06.16 at 17:13, wrote: > > 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: > >> > > --- 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? > > I agree, fwiw, for the extern-s to be moved to a header. That > header should then also be included by the generated source > file producing the symbols. I don't, however, see why the > function should go to a header. I was thinking that since the function is a nop under #CONFIG_XSM_POLICY you could just make it an inline static function, like: #ifdef CONFIG_XSM_POLICY static inline void xsm_policy_init(void) {... } #else #define xsm_policy_init(void) ; #endif Or such. But this is really minor, and pls ignore it if means doing more work than neccessary. > > Jan > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] xsm: add a default policy to .init.data
>>> On 30.06.16 at 17:13, wrote: > 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: >> > > --- 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? I agree, fwiw, for the extern-s to be moved to a header. That header should then also be included by the generated source file producing the symbols. I don't, however, see why the function should go to a header. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] xsm: add a default policy to .init.data
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 > > > --- > > > > > > 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
Re: [Xen-devel] [PATCH v3] xsm: add a default policy to .init.data
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 --- 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. .. 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. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] xsm: add a default policy to .init.data
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 > --- > > 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? .. 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? > static int __init xsm_core_init(void) > { > if ( verify(&dummy_xsm_ops) ) > @@ -46,6 +64,7 @@ static int __init xsm_core_init(void) > } > > xsm_ops = &dummy_xsm_ops; > +xsm_policy_init(); > flask_init(); > > return 0; > @@ -98,7 +117,8 @@ int __init xsm_dt_init(void) > > ret = xsm_core_init(); > > -xfree(policy_buffer); > +if ( policy_buffer != xsm_init_policy ) > +xfree(policy_buffer); > > return ret; > } > -- > 2.7.4 > > > ___ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel