| Message ID | 20210805140644.357-6-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xsm: refactoring xsm hooks | expand |
On 05.08.2021 16:06, Daniel P. Smith wrote: > --- /dev/null > +++ b/xen/include/xsm/xsm-core.h > @@ -0,0 +1,273 @@ > +/* > + * This file contains the XSM hook definitions for Xen. > + * > + * This work is based on the LSM implementation in Linux 2.6.13.4. > + * > + * Author: George Coker, <gscoker@alpha.ncsc.mil> > + * > + * Contributors: Michael LeMay, <mdlemay@epoch.ncsc.mil> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2, > + * as published by the Free Software Foundation. > + */ > + > +#ifndef __XSM_CORE_H__ > +#define __XSM_CORE_H__ > + > +#include <xen/sched.h> > +#include <xen/multiboot.h> I was going to ask to invert the order (as we try to arrange #include-s alphabetically), but it looks like multiboot.h isn't fit for this. > +typedef void xsm_op_t; > +DEFINE_XEN_GUEST_HANDLE(xsm_op_t); Just FTR - I consider this dubious. If void is meant, I don't see why a void handle can't be used. > +/* policy magic number (defined by XSM_MAGIC) */ > +typedef uint32_t xsm_magic_t; > + > +#ifdef CONFIG_XSM_FLASK > +#define XSM_MAGIC 0xf97cff8c > +#else > +#define XSM_MAGIC 0x0 > +#endif > + > +/* These annotations are used by callers and in dummy.h to document the > + * default actions of XSM hooks. They should be compiled out otherwise. > + */ I realize you only move code, but like e.g. the u32 -> uint32_t change in context above I'd like to encourage you to also address other style issues in the newly introduced file. Here I'm talking about comment style, requiring /* to be on its own line. > +enum xsm_default { > + XSM_HOOK, /* Guests can normally access the hypercall */ > + XSM_DM_PRIV, /* Device model can perform on its target domain */ > + XSM_TARGET, /* Can perform on self or your target domain */ > + XSM_PRIV, /* Privileged - normally restricted to dom0 */ > + XSM_XS_PRIV, /* Xenstore domain - can do some privileged operations */ > + XSM_OTHER /* Something more complex */ > +}; > +typedef enum xsm_default xsm_default_t; > + > +struct xsm_ops { > + void (*security_domaininfo) (struct domain *d, Similarly here (and below) - we don't normally put a blank between the closing and opening parentheses in function pointer declarations. The majority does so here, but ... >[...] > + int (*page_offline)(uint32_t cmd); > + int (*hypfs_op)(void); ... there are exceptions. >[...] > + int (*platform_op) (uint32_t cmd); > + > +#ifdef CONFIG_X86 > + int (*do_mca) (void); > + int (*shadow_control) (struct domain *d, uint32_t op); > + int (*mem_sharing_op) (struct domain *d, struct domain *cd, int op); > + int (*apic) (struct domain *d, int cmd); > + int (*memtype) (uint32_t access); > + int (*machine_memory_map) (void); > + int (*domain_memory_map) (struct domain *d); > +#define XSM_MMU_UPDATE_READ 1 > +#define XSM_MMU_UPDATE_WRITE 2 > +#define XSM_MMU_NORMAL_UPDATE 4 > +#define XSM_MMU_MACHPHYS_UPDATE 8 > + int (*mmu_update) (struct domain *d, struct domain *t, > + struct domain *f, uint32_t flags); > + int (*mmuext_op) (struct domain *d, struct domain *f); > + int (*update_va_mapping) (struct domain *d, struct domain *f, > + l1_pgentry_t pte); > + int (*priv_mapping) (struct domain *d, struct domain *t); > + int (*ioport_permission) (struct domain *d, uint32_t s, uint32_t e, > + uint8_t allow); > + int (*ioport_mapping) (struct domain *d, uint32_t s, uint32_t e, > + uint8_t allow); > + int (*pmu_op) (struct domain *d, unsigned int op); > +#endif > + int (*dm_op) (struct domain *d); To match grouping elsewhere, a blank line above here, ... > + int (*xen_version) (uint32_t cmd); > + int (*domain_resource_map) (struct domain *d); > +#ifdef CONFIG_ARGO ... and here would be nice. > + int (*argo_enable) (const struct domain *d); > + int (*argo_register_single_source) (const struct domain *d, > + const struct domain *t); > + int (*argo_register_any_source) (const struct domain *d); > + int (*argo_send) (const struct domain *d, const struct domain *t); > +#endif > +}; > + > +extern void xsm_fixup_ops(struct xsm_ops *ops); > + > +#ifdef CONFIG_XSM > + > +#ifdef CONFIG_MULTIBOOT > +extern int xsm_multiboot_init(unsigned long *module_map, > + const multiboot_info_t *mbi); > +extern int xsm_multiboot_policy_init(unsigned long *module_map, > + const multiboot_info_t *mbi, > + void **policy_buffer, > + size_t *policy_size); > +#endif > + > +#ifdef CONFIG_HAS_DEVICE_TREE > +/* > + * Initialize XSM > + * > + * On success, return 1 if using SILO mode else 0. > + */ > +extern int xsm_dt_init(void); > +extern int xsm_dt_policy_init(void **policy_buffer, size_t *policy_size); > +extern bool has_xsm_magic(paddr_t); > +#endif > + > +#ifdef CONFIG_XSM_FLASK > +extern const struct xsm_ops *flask_init(const void *policy_buffer, > + size_t policy_size); > +#else > +static inline const struct xsm_ops *flask_init(const void *policy_buffer, > + size_t policy_size) > +{ > + return NULL; > +} > +#endif > + > +#ifdef CONFIG_XSM_FLASK_POLICY > +extern const unsigned char xsm_flask_init_policy[]; > +extern const unsigned int xsm_flask_init_policy_size; > +#endif To be honest, I don't think this belongs in any header. This interfaces with a generated assembly file. In such a case I would always suggest to limit visibility of the symbols as much as possible, i.e. put the declarations in the sole file referencing them. > +#ifdef CONFIG_XSM_SILO > +extern const struct xsm_ops *silo_init(void); > +#else > +static const inline struct xsm_ops *silo_init(void) > +{ > + return NULL; > +} > +#endif > + > +#else /* CONFIG_XSM */ > + > +#ifdef CONFIG_MULTIBOOT > +static inline int xsm_multiboot_init (unsigned long *module_map, Nit: Stray blank ahead of the opening parenthesis. Looking back there's also the question of "extern" on function declarations. In new code I don't think we put the redundant storage class specifier there; they're only needed on data declarations. I'm inclined to suggest to drop all of them while moving the code. Preferably with these adjustments Acked-by: Jan Beulich <jbeulich@suse.com> Jan
On 8/26/21 4:13 AM, Jan Beulich wrote: > On 05.08.2021 16:06, Daniel P. Smith wrote: >> --- /dev/null >> +++ b/xen/include/xsm/xsm-core.h >> @@ -0,0 +1,273 @@ >> +/* >> + * This file contains the XSM hook definitions for Xen. >> + * >> + * This work is based on the LSM implementation in Linux 2.6.13.4. >> + * >> + * Author: George Coker, <gscoker@alpha.ncsc.mil> >> + * >> + * Contributors: Michael LeMay, <mdlemay@epoch.ncsc.mil> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2, >> + * as published by the Free Software Foundation. >> + */ >> + >> +#ifndef __XSM_CORE_H__ >> +#define __XSM_CORE_H__ >> + >> +#include <xen/sched.h> >> +#include <xen/multiboot.h> > > I was going to ask to invert the order (as we try to arrange #include-s > alphabetically), but it looks like multiboot.h isn't fit for this. So my understanding is to leave this as is. >> +typedef void xsm_op_t; >> +DEFINE_XEN_GUEST_HANDLE(xsm_op_t); > > Just FTR - I consider this dubious. If void is meant, I don't see why > a void handle can't be used. Unless I am misunderstanding what you are calling for, I am afraid this will trickle further that what intended to be addressed in this patch set. If disagree and would like to provide me a suggest that stays bounded, I would gladly incorporate. >> +/* policy magic number (defined by XSM_MAGIC) */ >> +typedef uint32_t xsm_magic_t; >> + >> +#ifdef CONFIG_XSM_FLASK >> +#define XSM_MAGIC 0xf97cff8c >> +#else >> +#define XSM_MAGIC 0x0 >> +#endif >> + >> +/* These annotations are used by callers and in dummy.h to document the >> + * default actions of XSM hooks. They should be compiled out otherwise. >> + */ > > I realize you only move code, but like e.g. the u32 -> uint32_t change > in context above I'd like to encourage you to also address other style > issues in the newly introduced file. Here I'm talking about comment > style, requiring /* to be on its own line. Ack. >> +enum xsm_default { >> + XSM_HOOK, /* Guests can normally access the hypercall */ >> + XSM_DM_PRIV, /* Device model can perform on its target domain */ >> + XSM_TARGET, /* Can perform on self or your target domain */ >> + XSM_PRIV, /* Privileged - normally restricted to dom0 */ >> + XSM_XS_PRIV, /* Xenstore domain - can do some privileged operations */ >> + XSM_OTHER /* Something more complex */ >> +}; >> +typedef enum xsm_default xsm_default_t; >> + >> +struct xsm_ops { >> + void (*security_domaininfo) (struct domain *d, > > Similarly here (and below) - we don't normally put a blank between > the closing and opening parentheses in function pointer declarations. > The majority does so here, but ... Ack. >> [...] >> + int (*page_offline)(uint32_t cmd); >> + int (*hypfs_op)(void); > > ... there are exceptions. > >> [...] >> + int (*platform_op) (uint32_t cmd); >> + >> +#ifdef CONFIG_X86 >> + int (*do_mca) (void); >> + int (*shadow_control) (struct domain *d, uint32_t op); >> + int (*mem_sharing_op) (struct domain *d, struct domain *cd, int op); >> + int (*apic) (struct domain *d, int cmd); >> + int (*memtype) (uint32_t access); >> + int (*machine_memory_map) (void); >> + int (*domain_memory_map) (struct domain *d); >> +#define XSM_MMU_UPDATE_READ 1 >> +#define XSM_MMU_UPDATE_WRITE 2 >> +#define XSM_MMU_NORMAL_UPDATE 4 >> +#define XSM_MMU_MACHPHYS_UPDATE 8 >> + int (*mmu_update) (struct domain *d, struct domain *t, >> + struct domain *f, uint32_t flags); >> + int (*mmuext_op) (struct domain *d, struct domain *f); >> + int (*update_va_mapping) (struct domain *d, struct domain *f, >> + l1_pgentry_t pte); >> + int (*priv_mapping) (struct domain *d, struct domain *t); >> + int (*ioport_permission) (struct domain *d, uint32_t s, uint32_t e, >> + uint8_t allow); >> + int (*ioport_mapping) (struct domain *d, uint32_t s, uint32_t e, >> + uint8_t allow); >> + int (*pmu_op) (struct domain *d, unsigned int op); >> +#endif >> + int (*dm_op) (struct domain *d); > > To match grouping elsewhere, a blank line above here, ... Ack. >> + int (*xen_version) (uint32_t cmd); >> + int (*domain_resource_map) (struct domain *d); >> +#ifdef CONFIG_ARGO > > ... and here would be nice. Ack. >> + int (*argo_enable) (const struct domain *d); >> + int (*argo_register_single_source) (const struct domain *d, >> + const struct domain *t); >> + int (*argo_register_any_source) (const struct domain *d); >> + int (*argo_send) (const struct domain *d, const struct domain *t); >> +#endif >> +}; >> + >> +extern void xsm_fixup_ops(struct xsm_ops *ops); >> + >> +#ifdef CONFIG_XSM >> + >> +#ifdef CONFIG_MULTIBOOT >> +extern int xsm_multiboot_init(unsigned long *module_map, >> + const multiboot_info_t *mbi); >> +extern int xsm_multiboot_policy_init(unsigned long *module_map, >> + const multiboot_info_t *mbi, >> + void **policy_buffer, >> + size_t *policy_size); >> +#endif >> + >> +#ifdef CONFIG_HAS_DEVICE_TREE >> +/* >> + * Initialize XSM >> + * >> + * On success, return 1 if using SILO mode else 0. >> + */ >> +extern int xsm_dt_init(void); >> +extern int xsm_dt_policy_init(void **policy_buffer, size_t *policy_size); >> +extern bool has_xsm_magic(paddr_t); >> +#endif >> + >> +#ifdef CONFIG_XSM_FLASK >> +extern const struct xsm_ops *flask_init(const void *policy_buffer, >> + size_t policy_size); >> +#else >> +static inline const struct xsm_ops *flask_init(const void *policy_buffer, >> + size_t policy_size) >> +{ >> + return NULL; >> +} >> +#endif >> + >> +#ifdef CONFIG_XSM_FLASK_POLICY >> +extern const unsigned char xsm_flask_init_policy[]; >> +extern const unsigned int xsm_flask_init_policy_size; >> +#endif > > To be honest, I don't think this belongs in any header. This interfaces > with a generated assembly file. In such a case I would always suggest > to limit visibility of the symbols as much as possible, i.e. put the > declarations in the sole file referencing them. Confirmed only used in xsm_core.c, so will move there. >> +#ifdef CONFIG_XSM_SILO >> +extern const struct xsm_ops *silo_init(void); >> +#else >> +static const inline struct xsm_ops *silo_init(void) >> +{ >> + return NULL; >> +} >> +#endif >> + >> +#else /* CONFIG_XSM */ >> + >> +#ifdef CONFIG_MULTIBOOT >> +static inline int xsm_multiboot_init (unsigned long *module_map, > > Nit: Stray blank ahead of the opening parenthesis. Ack. > Looking back there's also the question of "extern" on function > declarations. In new code I don't think we put the redundant > storage class specifier there; they're only needed on data > declarations. I'm inclined to suggest to drop all of them while > moving the code. > > Preferably with these adjustments > Acked-by: Jan Beulich <jbeulich@suse.com> I believe I have pretty much incorporated all your adjustments. Will include your Acked-by unless you feel I have not. v/r, dps
On 27.08.2021 16:06, Daniel P. Smith wrote: > On 8/26/21 4:13 AM, Jan Beulich wrote: >> On 05.08.2021 16:06, Daniel P. Smith wrote: >>> --- /dev/null >>> +++ b/xen/include/xsm/xsm-core.h >>> @@ -0,0 +1,273 @@ >>> +/* >>> + * This file contains the XSM hook definitions for Xen. >>> + * >>> + * This work is based on the LSM implementation in Linux 2.6.13.4. >>> + * >>> + * Author: George Coker, <gscoker@alpha.ncsc.mil> >>> + * >>> + * Contributors: Michael LeMay, <mdlemay@epoch.ncsc.mil> >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License version 2, >>> + * as published by the Free Software Foundation. >>> + */ >>> + >>> +#ifndef __XSM_CORE_H__ >>> +#define __XSM_CORE_H__ >>> + >>> +#include <xen/sched.h> >>> +#include <xen/multiboot.h> >> >> I was going to ask to invert the order (as we try to arrange #include-s >> alphabetically), but it looks like multiboot.h isn't fit for this. > > So my understanding is to leave this as is. Yes, unfortunately. >>> +typedef void xsm_op_t; >>> +DEFINE_XEN_GUEST_HANDLE(xsm_op_t); >> >> Just FTR - I consider this dubious. If void is meant, I don't see why >> a void handle can't be used. > > Unless I am misunderstanding what you are calling for, I am afraid this > will trickle further that what intended to be addressed in this patch > set. If disagree and would like to provide me a suggest that stays > bounded, I would gladly incorporate. All I'm asking is to remove this pointless typedef and handle definition, seeing that you're doing a major rework anyway. I'm afraid I don't see how this would collide with the purpose of the overall series (albeit I may also have misunderstood your reply, as the 2nd half of the first sentence makes me struggle some with trying to parse it). Jan
On 8/30/21 9:24 AM, Jan Beulich wrote: > On 27.08.2021 16:06, Daniel P. Smith wrote: >> On 8/26/21 4:13 AM, Jan Beulich wrote: >>> On 05.08.2021 16:06, Daniel P. Smith wrote: >>>> --- /dev/null >>>> +++ b/xen/include/xsm/xsm-core.h >>>> @@ -0,0 +1,273 @@ >>>> +/* >>>> + * This file contains the XSM hook definitions for Xen. >>>> + * >>>> + * This work is based on the LSM implementation in Linux 2.6.13.4. >>>> + * >>>> + * Author: George Coker, <gscoker@alpha.ncsc.mil> >>>> + * >>>> + * Contributors: Michael LeMay, <mdlemay@epoch.ncsc.mil> >>>> + * >>>> + * This program is free software; you can redistribute it and/or modify >>>> + * it under the terms of the GNU General Public License version 2, >>>> + * as published by the Free Software Foundation. >>>> + */ >>>> + >>>> +#ifndef __XSM_CORE_H__ >>>> +#define __XSM_CORE_H__ >>>> + >>>> +#include <xen/sched.h> >>>> +#include <xen/multiboot.h> >>> >>> I was going to ask to invert the order (as we try to arrange #include-s >>> alphabetically), but it looks like multiboot.h isn't fit for this. >> >> So my understanding is to leave this as is. > > Yes, unfortunately. > >>>> +typedef void xsm_op_t; >>>> +DEFINE_XEN_GUEST_HANDLE(xsm_op_t); >>> >>> Just FTR - I consider this dubious. If void is meant, I don't see why >>> a void handle can't be used. >> >> Unless I am misunderstanding what you are calling for, I am afraid this >> will trickle further that what intended to be addressed in this patch >> set. If disagree and would like to provide me a suggest that stays >> bounded, I would gladly incorporate. > > All I'm asking is to remove this pointless typedef and handle definition, > seeing that you're doing a major rework anyway. I'm afraid I don't see > how this would collide with the purpose of the overall series (albeit I > may also have misunderstood your reply, as the 2nd half of the first > sentence makes me struggle some with trying to parse it). > > Jan > If I drop the typedef and start changing everywhere xsm_op_t is referenced to void, this now adds hypercall.h to the files I am now touching. In the end it is not about whether the change is big or small, but that more and more unrelated small changes/clean ups keep getting requested. There has to be a cut-off point to limit the scope of changes down to the purpose of the patch set, which is to unravel and simplify the XSM hooks. And this is being done so, so that the the XSM-Roles work can be introduced to bring a more solid definition to the the default access control system, which itself is needed to bring in hyperlaunch. v/r, dps
On 30.08.2021 15:41, Daniel P. Smith wrote: > On 8/30/21 9:24 AM, Jan Beulich wrote: >> On 27.08.2021 16:06, Daniel P. Smith wrote: >>> On 8/26/21 4:13 AM, Jan Beulich wrote: >>>> On 05.08.2021 16:06, Daniel P. Smith wrote: >>>>> --- /dev/null >>>>> +++ b/xen/include/xsm/xsm-core.h >>>>> @@ -0,0 +1,273 @@ >>>>> +/* >>>>> + * This file contains the XSM hook definitions for Xen. >>>>> + * >>>>> + * This work is based on the LSM implementation in Linux 2.6.13.4. >>>>> + * >>>>> + * Author: George Coker, <gscoker@alpha.ncsc.mil> >>>>> + * >>>>> + * Contributors: Michael LeMay, <mdlemay@epoch.ncsc.mil> >>>>> + * >>>>> + * This program is free software; you can redistribute it and/or modify >>>>> + * it under the terms of the GNU General Public License version 2, >>>>> + * as published by the Free Software Foundation. >>>>> + */ >>>>> + >>>>> +#ifndef __XSM_CORE_H__ >>>>> +#define __XSM_CORE_H__ >>>>> + >>>>> +#include <xen/sched.h> >>>>> +#include <xen/multiboot.h> >>>> >>>> I was going to ask to invert the order (as we try to arrange #include-s >>>> alphabetically), but it looks like multiboot.h isn't fit for this. >>> >>> So my understanding is to leave this as is. >> >> Yes, unfortunately. >> >>>>> +typedef void xsm_op_t; >>>>> +DEFINE_XEN_GUEST_HANDLE(xsm_op_t); >>>> >>>> Just FTR - I consider this dubious. If void is meant, I don't see why >>>> a void handle can't be used. >>> >>> Unless I am misunderstanding what you are calling for, I am afraid this >>> will trickle further that what intended to be addressed in this patch >>> set. If disagree and would like to provide me a suggest that stays >>> bounded, I would gladly incorporate. >> >> All I'm asking is to remove this pointless typedef and handle definition, >> seeing that you're doing a major rework anyway. I'm afraid I don't see >> how this would collide with the purpose of the overall series (albeit I >> may also have misunderstood your reply, as the 2nd half of the first >> sentence makes me struggle some with trying to parse it). > > If I drop the typedef and start changing everywhere xsm_op_t is > referenced to void, this now adds hypercall.h to the files I am now > touching. > > In the end it is not about whether the change is big or small, but that > more and more unrelated small changes/clean ups keep getting requested. > There has to be a cut-off point to limit the scope of changes down to > the purpose of the patch set, which is to unravel and simplify the XSM > hooks. And this is being done so, so that the the XSM-Roles work can be > introduced to bring a more solid definition to the the default access > control system, which itself is needed to bring in hyperlaunch. Well, yes, you effectively suffer from XSM not having been actively maintained for a number of years. As said in the original reply, I'd prefer my ack to cover all the suggested changes, but I didn't mean to insist. If this particular one goes too far for your taste, so be it. Jan
On 8/30/21 9:46 AM, Jan Beulich wrote: > On 30.08.2021 15:41, Daniel P. Smith wrote: >> On 8/30/21 9:24 AM, Jan Beulich wrote: >>> On 27.08.2021 16:06, Daniel P. Smith wrote: >>>> On 8/26/21 4:13 AM, Jan Beulich wrote: >>>>> On 05.08.2021 16:06, Daniel P. Smith wrote: >>>>>> --- /dev/null >>>>>> +++ b/xen/include/xsm/xsm-core.h >>>>>> @@ -0,0 +1,273 @@ >>>>>> +/* >>>>>> + * This file contains the XSM hook definitions for Xen. >>>>>> + * >>>>>> + * This work is based on the LSM implementation in Linux 2.6.13.4. >>>>>> + * >>>>>> + * Author: George Coker, <gscoker@alpha.ncsc.mil> >>>>>> + * >>>>>> + * Contributors: Michael LeMay, <mdlemay@epoch.ncsc.mil> >>>>>> + * >>>>>> + * This program is free software; you can redistribute it and/or modify >>>>>> + * it under the terms of the GNU General Public License version 2, >>>>>> + * as published by the Free Software Foundation. >>>>>> + */ >>>>>> + >>>>>> +#ifndef __XSM_CORE_H__ >>>>>> +#define __XSM_CORE_H__ >>>>>> + >>>>>> +#include <xen/sched.h> >>>>>> +#include <xen/multiboot.h> >>>>> >>>>> I was going to ask to invert the order (as we try to arrange #include-s >>>>> alphabetically), but it looks like multiboot.h isn't fit for this. >>>> >>>> So my understanding is to leave this as is. >>> >>> Yes, unfortunately. >>> >>>>>> +typedef void xsm_op_t; >>>>>> +DEFINE_XEN_GUEST_HANDLE(xsm_op_t); >>>>> >>>>> Just FTR - I consider this dubious. If void is meant, I don't see why >>>>> a void handle can't be used. >>>> >>>> Unless I am misunderstanding what you are calling for, I am afraid this >>>> will trickle further that what intended to be addressed in this patch >>>> set. If disagree and would like to provide me a suggest that stays >>>> bounded, I would gladly incorporate. >>> >>> All I'm asking is to remove this pointless typedef and handle definition, >>> seeing that you're doing a major rework anyway. I'm afraid I don't see >>> how this would collide with the purpose of the overall series (albeit I >>> may also have misunderstood your reply, as the 2nd half of the first >>> sentence makes me struggle some with trying to parse it). >> >> If I drop the typedef and start changing everywhere xsm_op_t is >> referenced to void, this now adds hypercall.h to the files I am now >> touching. >> >> In the end it is not about whether the change is big or small, but that >> more and more unrelated small changes/clean ups keep getting requested. >> There has to be a cut-off point to limit the scope of changes down to >> the purpose of the patch set, which is to unravel and simplify the XSM >> hooks. And this is being done so, so that the the XSM-Roles work can be >> introduced to bring a more solid definition to the the default access >> control system, which itself is needed to bring in hyperlaunch. > > Well, yes, you effectively suffer from XSM not having been actively > maintained for a number of years. As said in the original reply, I'd > prefer my ack to cover all the suggested changes, but I didn't mean > to insist. If this particular one goes too far for your taste, so be > it. > > Jan > I think we can agree that XSM has not been receiving appropriate care and maintenance. It can't all be fixed in one go and I am trying to step up to get it back into shape. Since this is really undoing a masking of void, I can add this and the risk is very low of breaking something else. Which this is my biggest concern as I see new files getting brought in with each cleanup requested. v/r, dps
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 363c6d7798..c445c5681b 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -16,7 +16,7 @@ */ #include <xen/sched.h> -#include <xsm/xsm.h> +#include <xsm/xsm-core.h> #include <public/hvm/params.h> /* Cannot use BUILD_BUG_ON here because the expressions we check are not diff --git a/xen/include/xsm/xsm-core.h b/xen/include/xsm/xsm-core.h new file mode 100644 index 0000000000..49b00d688c --- /dev/null +++ b/xen/include/xsm/xsm-core.h @@ -0,0 +1,273 @@ +/* + * This file contains the XSM hook definitions for Xen. + * + * This work is based on the LSM implementation in Linux 2.6.13.4. + * + * Author: George Coker, <gscoker@alpha.ncsc.mil> + * + * Contributors: Michael LeMay, <mdlemay@epoch.ncsc.mil> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2, + * as published by the Free Software Foundation. + */ + +#ifndef __XSM_CORE_H__ +#define __XSM_CORE_H__ + +#include <xen/sched.h> +#include <xen/multiboot.h> + +typedef void xsm_op_t; +DEFINE_XEN_GUEST_HANDLE(xsm_op_t); + +/* policy magic number (defined by XSM_MAGIC) */ +typedef uint32_t xsm_magic_t; + +#ifdef CONFIG_XSM_FLASK +#define XSM_MAGIC 0xf97cff8c +#else +#define XSM_MAGIC 0x0 +#endif + +/* These annotations are used by callers and in dummy.h to document the + * default actions of XSM hooks. They should be compiled out otherwise. + */ +enum xsm_default { + XSM_HOOK, /* Guests can normally access the hypercall */ + XSM_DM_PRIV, /* Device model can perform on its target domain */ + XSM_TARGET, /* Can perform on self or your target domain */ + XSM_PRIV, /* Privileged - normally restricted to dom0 */ + XSM_XS_PRIV, /* Xenstore domain - can do some privileged operations */ + XSM_OTHER /* Something more complex */ +}; +typedef enum xsm_default xsm_default_t; + +struct xsm_ops { + void (*security_domaininfo) (struct domain *d, + struct xen_domctl_getdomaininfo *info); + int (*domain_create) (struct domain *d, uint32_t ssidref); + int (*getdomaininfo) (struct domain *d); + int (*domctl_scheduler_op) (struct domain *d, int op); + int (*sysctl_scheduler_op) (int op); + int (*set_target) (struct domain *d, struct domain *e); + int (*domctl) (struct domain *d, int cmd); + int (*sysctl) (int cmd); + int (*readconsole) (uint32_t clear); + + int (*evtchn_unbound) (struct domain *d, struct evtchn *chn, domid_t id2); + int (*evtchn_interdomain) (struct domain *d1, struct evtchn *chn1, + struct domain *d2, struct evtchn *chn2); + void (*evtchn_close_post) (struct evtchn *chn); + int (*evtchn_send) (struct domain *d, struct evtchn *chn); + int (*evtchn_status) (struct domain *d, struct evtchn *chn); + int (*evtchn_reset) (struct domain *d1, struct domain *d2); + + int (*grant_mapref) (struct domain *d1, struct domain *d2, uint32_t flags); + int (*grant_unmapref) (struct domain *d1, struct domain *d2); + int (*grant_setup) (struct domain *d1, struct domain *d2); + int (*grant_transfer) (struct domain *d1, struct domain *d2); + int (*grant_copy) (struct domain *d1, struct domain *d2); + int (*grant_query_size) (struct domain *d1, struct domain *d2); + + int (*alloc_security_domain) (struct domain *d); + void (*free_security_domain) (struct domain *d); + int (*alloc_security_evtchns) (struct evtchn chn[], unsigned int nr); + void (*free_security_evtchns) (struct evtchn chn[], unsigned int nr); + char *(*show_security_evtchn) (struct domain *d, const struct evtchn *chn); + int (*init_hardware_domain) (struct domain *d); + + int (*get_pod_target) (struct domain *d); + int (*set_pod_target) (struct domain *d); + int (*memory_exchange) (struct domain *d); + int (*memory_adjust_reservation) (struct domain *d1, struct domain *d2); + int (*memory_stat_reservation) (struct domain *d1, struct domain *d2); + int (*memory_pin_page) (struct domain *d1, struct domain *d2, + struct page_info *page); + int (*add_to_physmap) (struct domain *d1, struct domain *d2); + int (*remove_from_physmap) (struct domain *d1, struct domain *d2); + int (*map_gmfn_foreign) (struct domain *d, struct domain *t); + int (*claim_pages) (struct domain *d); + + int (*console_io) (struct domain *d, int cmd); + + int (*profile) (struct domain *d, int op); + + int (*kexec) (void); + int (*schedop_shutdown) (struct domain *d1, struct domain *d2); + + char *(*show_irq_sid) (int irq); + int (*map_domain_pirq) (struct domain *d); + int (*map_domain_irq) (struct domain *d, int irq, const void *data); + int (*unmap_domain_pirq) (struct domain *d); + int (*unmap_domain_irq) (struct domain *d, int irq, const void *data); + int (*bind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq *bind); + int (*unbind_pt_irq) (struct domain *d, + struct xen_domctl_bind_pt_irq *bind); + int (*irq_permission) (struct domain *d, int pirq, uint8_t allow); + int (*iomem_permission) (struct domain *d, uint64_t s, uint64_t e, + uint8_t allow); + int (*iomem_mapping) (struct domain *d, uint64_t s, uint64_t e, + uint8_t allow); + int (*pci_config_permission) (struct domain *d, uint32_t machine_bdf, + uint16_t start, uint16_t end, uint8_t access); + +#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI) + int (*get_device_group) (uint32_t machine_bdf); + int (*assign_device) (struct domain *d, uint32_t machine_bdf); + int (*deassign_device) (struct domain *d, uint32_t machine_bdf); +#endif + +#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE) + int (*assign_dtdevice) (struct domain *d, const char *dtpath); + int (*deassign_dtdevice) (struct domain *d, const char *dtpath); +#endif + + int (*resource_plug_core) (void); + int (*resource_unplug_core) (void); + int (*resource_plug_pci) (uint32_t machine_bdf); + int (*resource_unplug_pci) (uint32_t machine_bdf); + int (*resource_setup_pci) (uint32_t machine_bdf); + int (*resource_setup_gsi) (int gsi); + int (*resource_setup_misc) (void); + + int (*page_offline)(uint32_t cmd); + int (*hypfs_op)(void); + + long (*do_xsm_op) (XEN_GUEST_HANDLE_PARAM(xsm_op_t) op); +#ifdef CONFIG_COMPAT + int (*do_compat_op) (XEN_GUEST_HANDLE_PARAM(xsm_op_t) op); +#endif + + int (*hvm_param) (struct domain *d, unsigned long op); + int (*hvm_control) (struct domain *d, unsigned long op); + int (*hvm_param_altp2mhvm) (struct domain *d); + int (*hvm_altp2mhvm_op) (struct domain *d, uint64_t mode, uint32_t op); + int (*get_vnumainfo) (struct domain *d); + + int (*vm_event_control) (struct domain *d, int mode, int op); + +#ifdef CONFIG_MEM_ACCESS + int (*mem_access) (struct domain *d); +#endif + +#ifdef CONFIG_MEM_PAGING + int (*mem_paging) (struct domain *d); +#endif + +#ifdef CONFIG_MEM_SHARING + int (*mem_sharing) (struct domain *d); +#endif + + int (*platform_op) (uint32_t cmd); + +#ifdef CONFIG_X86 + int (*do_mca) (void); + int (*shadow_control) (struct domain *d, uint32_t op); + int (*mem_sharing_op) (struct domain *d, struct domain *cd, int op); + int (*apic) (struct domain *d, int cmd); + int (*memtype) (uint32_t access); + int (*machine_memory_map) (void); + int (*domain_memory_map) (struct domain *d); +#define XSM_MMU_UPDATE_READ 1 +#define XSM_MMU_UPDATE_WRITE 2 +#define XSM_MMU_NORMAL_UPDATE 4 +#define XSM_MMU_MACHPHYS_UPDATE 8 + int (*mmu_update) (struct domain *d, struct domain *t, + struct domain *f, uint32_t flags); + int (*mmuext_op) (struct domain *d, struct domain *f); + int (*update_va_mapping) (struct domain *d, struct domain *f, + l1_pgentry_t pte); + int (*priv_mapping) (struct domain *d, struct domain *t); + int (*ioport_permission) (struct domain *d, uint32_t s, uint32_t e, + uint8_t allow); + int (*ioport_mapping) (struct domain *d, uint32_t s, uint32_t e, + uint8_t allow); + int (*pmu_op) (struct domain *d, unsigned int op); +#endif + int (*dm_op) (struct domain *d); + int (*xen_version) (uint32_t cmd); + int (*domain_resource_map) (struct domain *d); +#ifdef CONFIG_ARGO + int (*argo_enable) (const struct domain *d); + int (*argo_register_single_source) (const struct domain *d, + const struct domain *t); + int (*argo_register_any_source) (const struct domain *d); + int (*argo_send) (const struct domain *d, const struct domain *t); +#endif +}; + +extern void xsm_fixup_ops(struct xsm_ops *ops); + +#ifdef CONFIG_XSM + +#ifdef CONFIG_MULTIBOOT +extern int xsm_multiboot_init(unsigned long *module_map, + const multiboot_info_t *mbi); +extern int xsm_multiboot_policy_init(unsigned long *module_map, + const multiboot_info_t *mbi, + void **policy_buffer, + size_t *policy_size); +#endif + +#ifdef CONFIG_HAS_DEVICE_TREE +/* + * Initialize XSM + * + * On success, return 1 if using SILO mode else 0. + */ +extern int xsm_dt_init(void); +extern int xsm_dt_policy_init(void **policy_buffer, size_t *policy_size); +extern bool has_xsm_magic(paddr_t); +#endif + +#ifdef CONFIG_XSM_FLASK +extern const struct xsm_ops *flask_init(const void *policy_buffer, + size_t policy_size); +#else +static inline const struct xsm_ops *flask_init(const void *policy_buffer, + size_t policy_size) +{ + return NULL; +} +#endif + +#ifdef CONFIG_XSM_FLASK_POLICY +extern const unsigned char xsm_flask_init_policy[]; +extern const unsigned int xsm_flask_init_policy_size; +#endif + +#ifdef CONFIG_XSM_SILO +extern const struct xsm_ops *silo_init(void); +#else +static const inline struct xsm_ops *silo_init(void) +{ + return NULL; +} +#endif + +#else /* CONFIG_XSM */ + +#ifdef CONFIG_MULTIBOOT +static inline int xsm_multiboot_init (unsigned long *module_map, + const multiboot_info_t *mbi) +{ + return 0; +} +#endif + +#ifdef CONFIG_HAS_DEVICE_TREE +static inline int xsm_dt_init(void) +{ + return 0; +} + +static inline bool has_xsm_magic(paddr_t start) +{ + return false; +} +#endif /* CONFIG_HAS_DEVICE_TREE */ + +#endif /* CONFIG_XSM */ + +#endif /* __XSM_CORE_H */ diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index 6a54ee883c..673b818ac7 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -18,184 +18,12 @@ #include <xen/alternative-call.h> #include <xen/sched.h> #include <xen/multiboot.h> - -typedef void xsm_op_t; -DEFINE_XEN_GUEST_HANDLE(xsm_op_t); - -/* policy magic number (defined by XSM_MAGIC) */ -typedef u32 xsm_magic_t; - -#ifdef CONFIG_XSM_FLASK -#define XSM_MAGIC 0xf97cff8c -#else -#define XSM_MAGIC 0x0 -#endif - -/* These annotations are used by callers and in dummy.h to document the - * default actions of XSM hooks. They should be compiled out otherwise. - */ -enum xsm_default { - XSM_HOOK, /* Guests can normally access the hypercall */ - XSM_DM_PRIV, /* Device model can perform on its target domain */ - XSM_TARGET, /* Can perform on self or your target domain */ - XSM_PRIV, /* Privileged - normally restricted to dom0 */ - XSM_XS_PRIV, /* Xenstore domain - can do some privileged operations */ - XSM_OTHER /* Something more complex */ -}; -typedef enum xsm_default xsm_default_t; - -struct xsm_ops { - void (*security_domaininfo) (struct domain *d, - struct xen_domctl_getdomaininfo *info); - int (*domain_create) (struct domain *d, u32 ssidref); - int (*getdomaininfo) (struct domain *d); - int (*domctl_scheduler_op) (struct domain *d, int op); - int (*sysctl_scheduler_op) (int op); - int (*set_target) (struct domain *d, struct domain *e); - int (*domctl) (struct domain *d, int cmd); - int (*sysctl) (int cmd); - int (*readconsole) (uint32_t clear); - - int (*evtchn_unbound) (struct domain *d, struct evtchn *chn, domid_t id2); - int (*evtchn_interdomain) (struct domain *d1, struct evtchn *chn1, - struct domain *d2, struct evtchn *chn2); - void (*evtchn_close_post) (struct evtchn *chn); - int (*evtchn_send) (struct domain *d, struct evtchn *chn); - int (*evtchn_status) (struct domain *d, struct evtchn *chn); - int (*evtchn_reset) (struct domain *d1, struct domain *d2); - - int (*grant_mapref) (struct domain *d1, struct domain *d2, uint32_t flags); - int (*grant_unmapref) (struct domain *d1, struct domain *d2); - int (*grant_setup) (struct domain *d1, struct domain *d2); - int (*grant_transfer) (struct domain *d1, struct domain *d2); - int (*grant_copy) (struct domain *d1, struct domain *d2); - int (*grant_query_size) (struct domain *d1, struct domain *d2); - - int (*alloc_security_domain) (struct domain *d); - void (*free_security_domain) (struct domain *d); - int (*alloc_security_evtchns) (struct evtchn chn[], unsigned int nr); - void (*free_security_evtchns) (struct evtchn chn[], unsigned int nr); - char *(*show_security_evtchn) (struct domain *d, const struct evtchn *chn); - int (*init_hardware_domain) (struct domain *d); - - int (*get_pod_target) (struct domain *d); - int (*set_pod_target) (struct domain *d); - int (*memory_exchange) (struct domain *d); - int (*memory_adjust_reservation) (struct domain *d1, struct domain *d2); - int (*memory_stat_reservation) (struct domain *d1, struct domain *d2); - int (*memory_pin_page) (struct domain *d1, struct domain *d2, struct page_info *page); - int (*add_to_physmap) (struct domain *d1, struct domain *d2); - int (*remove_from_physmap) (struct domain *d1, struct domain *d2); - int (*map_gmfn_foreign) (struct domain *d, struct domain *t); - int (*claim_pages) (struct domain *d); - - int (*console_io) (struct domain *d, int cmd); - - int (*profile) (struct domain *d, int op); - - int (*kexec) (void); - int (*schedop_shutdown) (struct domain *d1, struct domain *d2); - - char *(*show_irq_sid) (int irq); - int (*map_domain_pirq) (struct domain *d); - int (*map_domain_irq) (struct domain *d, int irq, const void *data); - int (*unmap_domain_pirq) (struct domain *d); - int (*unmap_domain_irq) (struct domain *d, int irq, const void *data); - int (*bind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq *bind); - int (*unbind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq *bind); - int (*irq_permission) (struct domain *d, int pirq, uint8_t allow); - int (*iomem_permission) (struct domain *d, uint64_t s, uint64_t e, uint8_t allow); - int (*iomem_mapping) (struct domain *d, uint64_t s, uint64_t e, uint8_t allow); - int (*pci_config_permission) (struct domain *d, uint32_t machine_bdf, uint16_t start, uint16_t end, uint8_t access); - -#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI) - int (*get_device_group) (uint32_t machine_bdf); - int (*assign_device) (struct domain *d, uint32_t machine_bdf); - int (*deassign_device) (struct domain *d, uint32_t machine_bdf); -#endif - -#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE) - int (*assign_dtdevice) (struct domain *d, const char *dtpath); - int (*deassign_dtdevice) (struct domain *d, const char *dtpath); -#endif - - int (*resource_plug_core) (void); - int (*resource_unplug_core) (void); - int (*resource_plug_pci) (uint32_t machine_bdf); - int (*resource_unplug_pci) (uint32_t machine_bdf); - int (*resource_setup_pci) (uint32_t machine_bdf); - int (*resource_setup_gsi) (int gsi); - int (*resource_setup_misc) (void); - - int (*page_offline)(uint32_t cmd); - int (*hypfs_op)(void); - - long (*do_xsm_op) (XEN_GUEST_HANDLE_PARAM(xsm_op_t) op); -#ifdef CONFIG_COMPAT - int (*do_compat_op) (XEN_GUEST_HANDLE_PARAM(xsm_op_t) op); -#endif - - int (*hvm_param) (struct domain *d, unsigned long op); - int (*hvm_control) (struct domain *d, unsigned long op); - int (*hvm_param_altp2mhvm) (struct domain *d); - int (*hvm_altp2mhvm_op) (struct domain *d, uint64_t mode, uint32_t op); - int (*get_vnumainfo) (struct domain *d); - - int (*vm_event_control) (struct domain *d, int mode, int op); - -#ifdef CONFIG_MEM_ACCESS - int (*mem_access) (struct domain *d); -#endif - -#ifdef CONFIG_MEM_PAGING - int (*mem_paging) (struct domain *d); -#endif - -#ifdef CONFIG_MEM_SHARING - int (*mem_sharing) (struct domain *d); -#endif - - int (*platform_op) (uint32_t cmd); - -#ifdef CONFIG_X86 - int (*do_mca) (void); - int (*shadow_control) (struct domain *d, uint32_t op); - int (*mem_sharing_op) (struct domain *d, struct domain *cd, int op); - int (*apic) (struct domain *d, int cmd); - int (*memtype) (uint32_t access); - int (*machine_memory_map) (void); - int (*domain_memory_map) (struct domain *d); -#define XSM_MMU_UPDATE_READ 1 -#define XSM_MMU_UPDATE_WRITE 2 -#define XSM_MMU_NORMAL_UPDATE 4 -#define XSM_MMU_MACHPHYS_UPDATE 8 - int (*mmu_update) (struct domain *d, struct domain *t, - struct domain *f, uint32_t flags); - int (*mmuext_op) (struct domain *d, struct domain *f); - int (*update_va_mapping) (struct domain *d, struct domain *f, l1_pgentry_t pte); - int (*priv_mapping) (struct domain *d, struct domain *t); - int (*ioport_permission) (struct domain *d, uint32_t s, uint32_t e, uint8_t allow); - int (*ioport_mapping) (struct domain *d, uint32_t s, uint32_t e, uint8_t allow); - int (*pmu_op) (struct domain *d, unsigned int op); -#endif - int (*dm_op) (struct domain *d); - int (*xen_version) (uint32_t cmd); - int (*domain_resource_map) (struct domain *d); -#ifdef CONFIG_ARGO - int (*argo_enable) (const struct domain *d); - int (*argo_register_single_source) (const struct domain *d, - const struct domain *t); - int (*argo_register_any_source) (const struct domain *d); - int (*argo_send) (const struct domain *d, const struct domain *t); -#endif -}; +#include <xsm/xsm-core.h> #ifdef CONFIG_XSM extern struct xsm_ops xsm_ops; -#ifndef XSM_NO_WRAPPERS - static inline void xsm_security_domaininfo (struct domain *d, struct xen_domctl_getdomaininfo *info) { @@ -726,79 +554,10 @@ static inline int xsm_argo_send(const struct domain *d, const struct domain *t) #endif /* CONFIG_ARGO */ -#endif /* XSM_NO_WRAPPERS */ - -#ifdef CONFIG_MULTIBOOT -extern int xsm_multiboot_init(unsigned long *module_map, - const multiboot_info_t *mbi); -extern int xsm_multiboot_policy_init(unsigned long *module_map, - const multiboot_info_t *mbi, - void **policy_buffer, - size_t *policy_size); -#endif - -#ifdef CONFIG_HAS_DEVICE_TREE -/* - * Initialize XSM - * - * On success, return 1 if using SILO mode else 0. - */ -extern int xsm_dt_init(void); -extern int xsm_dt_policy_init(void **policy_buffer, size_t *policy_size); -extern bool has_xsm_magic(paddr_t); -#endif - -extern void xsm_fixup_ops(struct xsm_ops *ops); - -#ifdef CONFIG_XSM_FLASK -extern const struct xsm_ops *flask_init(const void *policy_buffer, - size_t policy_size); -#else -static inline struct xsm_ops *flask_init(const void *policy_buffer, - size_t policy_size) -{ - return NULL; -} -#endif - -#ifdef CONFIG_XSM_FLASK_POLICY -extern const unsigned char xsm_flask_init_policy[]; -extern const unsigned int xsm_flask_init_policy_size; -#endif - -#ifdef CONFIG_XSM_SILO -extern const struct xsm_ops *silo_init(void); -#else -static inline struct xsm_ops *silo_init(void) -{ - return NULL; -} -#endif - #else /* CONFIG_XSM */ #include <xsm/dummy.h> -#ifdef CONFIG_MULTIBOOT -static inline int xsm_multiboot_init (unsigned long *module_map, - const multiboot_info_t *mbi) -{ - return 0; -} -#endif - -#ifdef CONFIG_HAS_DEVICE_TREE -static inline int xsm_dt_init(void) -{ - return 0; -} - -static inline bool has_xsm_magic(paddr_t start) -{ - return false; -} -#endif /* CONFIG_HAS_DEVICE_TREE */ - #endif /* CONFIG_XSM */ #endif /* __XSM_H */ diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c index d8c935328e..b848580eaa 100644 --- a/xen/xsm/dummy.c +++ b/xen/xsm/dummy.c @@ -10,7 +10,6 @@ * as published by the Free Software Foundation. */ -#define XSM_NO_WRAPPERS #include <xsm/dummy.h> #define set_to_dummy_if_null(ops, function) \ diff --git a/xen/xsm/silo.c b/xen/xsm/silo.c index 3a2dea13fe..4ef40bd712 100644 --- a/xen/xsm/silo.c +++ b/xen/xsm/silo.c @@ -17,7 +17,6 @@ * You should have received a copy of the GNU General Public License along with * this program; If not, see <http://www.gnu.org/licenses/>. */ -#define XSM_NO_WRAPPERS #include <xsm/dummy.h> /*
Multiple preprocessor defines were used as a mechanism to selective include parts of the xsm.h header file. This makes it difficult to know which portion is being included at any one time. This commit works to simplify this by separating the core structures and functions of XSM into xsm-core.h away from the wrapper functions which remain in xsm.h and dummy.h. Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> --- xen/include/xsm/dummy.h | 2 +- xen/include/xsm/xsm-core.h | 273 +++++++++++++++++++++++++++++++++++++ xen/include/xsm/xsm.h | 243 +-------------------------------- xen/xsm/dummy.c | 1 - xen/xsm/silo.c | 1 - 5 files changed, 275 insertions(+), 245 deletions(-) create mode 100644 xen/include/xsm/xsm-core.h