diff mbox

[17/17] xsm: add a default policy to .init.data

Message ID 1466431466-28055-18-git-send-email-dgdegra@tycho.nsa.gov (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel De Graaf June 20, 2016, 2:04 p.m. UTC
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.

Enabling this option only builds the policy if checkpolicy is available
during compilation of the hypervisor; otherwise, it does nothing.  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>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 docs/misc/xen-command-line.markdown | 16 +++++++++-------
 docs/misc/xsm-flask.txt             | 30 +++++++++++++++---------------
 xen/arch/arm/xen.lds.S              |  4 ++++
 xen/arch/x86/xen.lds.S              |  5 +++++
 xen/common/Kconfig                  | 17 +++++++++++++++++
 xen/xsm/flask/Makefile              | 17 +++++++++++++++++
 xen/xsm/xsm_core.c                  | 15 ++++++++++++++-
 7 files changed, 81 insertions(+), 23 deletions(-)

Comments

Douglas Goldstein June 20, 2016, 2:52 p.m. UTC | #1
On 6/20/16 9:04 AM, 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.
> 
> Enabling this option only builds the policy if checkpolicy is available
> during compilation of the hypervisor; otherwise, it does nothing.  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>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>
Julien Grall June 24, 2016, 4:30 p.m. UTC | #2
Hello Daniel,

Please try to CC relevant maintainers on your patch. I would have missed 
it if Andrew did not ping me on IRC.

On 20/06/16 15:04, 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.
>
> Enabling this option only builds the policy if checkpolicy is available
> during compilation of the hypervisor; otherwise, it does nothing.  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>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

For ARM bits:

Acked-by: Julien Grall <julien.grall@arm.com>

Although, I one a question below.

[...]

> diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile
> index 12fc3a9..eefd37c 100644
> --- a/xen/xsm/flask/Makefile
> +++ b/xen/xsm/flask/Makefile
> @@ -27,6 +27,23 @@ $(FLASK_H_FILES): $(FLASK_H_DEPEND)
>   $(AV_H_FILES): $(AV_H_DEPEND)
>   	$(CONFIG_SHELL) policy/mkaccess_vector.sh $(AWK) $(AV_H_DEPEND)
>
> +ifeq ($(CONFIG_XSM_POLICY),y)
> +HAS_CHECKPOLICY := $(shell checkpolicy -h 2>&1 | grep -q xen && echo y || echo n)
> +
> +obj-$(HAS_CHECKPOLICY) += policy.o

I would have expect a warning (if not an error) here to tell the user 
that checkpolicy is not available. Otherwise it may take some time to 
the user to understand why the policy is not loaded/present. Because if 
you enable XSM, you don't necessarily check which other options have 
been enabled by default.

> +endif

Regards,
Konrad Rzeszutek Wilk June 24, 2016, 4:50 p.m. UTC | #3
On Fri, Jun 24, 2016 at 05:30:32PM +0100, Julien Grall wrote:
> Hello Daniel,
> 
> Please try to CC relevant maintainers on your patch. I would have missed it
> if Andrew did not ping me on IRC.
> 
> On 20/06/16 15:04, 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.
> >
> >Enabling this option only builds the policy if checkpolicy is available
> >during compilation of the hypervisor; otherwise, it does nothing.  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>
> >Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> For ARM bits:
> 
> Acked-by: Julien Grall <julien.grall@arm.com>
> 
> Although, I one a question below.
> 
> [...]
> 
> >diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile
> >index 12fc3a9..eefd37c 100644
> >--- a/xen/xsm/flask/Makefile
> >+++ b/xen/xsm/flask/Makefile
> >@@ -27,6 +27,23 @@ $(FLASK_H_FILES): $(FLASK_H_DEPEND)
> >  $(AV_H_FILES): $(AV_H_DEPEND)
> >  	$(CONFIG_SHELL) policy/mkaccess_vector.sh $(AWK) $(AV_H_DEPEND)
> >
> >+ifeq ($(CONFIG_XSM_POLICY),y)
> >+HAS_CHECKPOLICY := $(shell checkpolicy -h 2>&1 | grep -q xen && echo y || echo n)
> >+
> >+obj-$(HAS_CHECKPOLICY) += policy.o
> 
> I would have expect a warning (if not an error) here to tell the user that
> checkpolicy is not available. Otherwise it may take some time to the user to
> understand why the policy is not loaded/present. Because if you enable XSM,
> you don't necessarily check which other options have been enabled by
> default.

Good point! And we should probably update the INSTALL document too to mention
that you need checkpoint tool!

> 
> >+endif
> 
> Regards,
> 
> -- 
> Julien Grall
Daniel De Graaf June 24, 2016, 5:34 p.m. UTC | #4
On 06/24/2016 12:50 PM, Konrad Rzeszutek Wilk wrote:
> On Fri, Jun 24, 2016 at 05:30:32PM +0100, Julien Grall wrote:
>> Hello Daniel,
>>
>> Please try to CC relevant maintainers on your patch. I would have missed it
>> if Andrew did not ping me on IRC.
>>
>> On 20/06/16 15:04, 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.
>>>
>>> Enabling this option only builds the policy if checkpolicy is available
>>> during compilation of the hypervisor; otherwise, it does nothing.  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>
>>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>> For ARM bits:
>>
>> Acked-by: Julien Grall <julien.grall@arm.com>
>>
>> Although, I one a question below.
>>
>> [...]
>>
>>> diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile
>>> index 12fc3a9..eefd37c 100644
>>> --- a/xen/xsm/flask/Makefile
>>> +++ b/xen/xsm/flask/Makefile
>>> @@ -27,6 +27,23 @@ $(FLASK_H_FILES): $(FLASK_H_DEPEND)
>>>  $(AV_H_FILES): $(AV_H_DEPEND)
>>>  	$(CONFIG_SHELL) policy/mkaccess_vector.sh $(AWK) $(AV_H_DEPEND)
>>>
>>> +ifeq ($(CONFIG_XSM_POLICY),y)
>>> +HAS_CHECKPOLICY := $(shell checkpolicy -h 2>&1 | grep -q xen && echo y || echo n)
>>> +
>>> +obj-$(HAS_CHECKPOLICY) += policy.o
>>
>> I would have expect a warning (if not an error) here to tell the user that
>> checkpolicy is not available. Otherwise it may take some time to the user to
>> understand why the policy is not loaded/present. Because if you enable XSM,
>> you don't necessarily check which other options have been enabled by
>> default.
>
> Good point! And we should probably update the INSTALL document too to mention
> that you need checkpoint tool!

The dependency on checkpolicy is called out in the Kconfig item that
enables this option.  Are you suggesting I should add a mention below
the instructions on running menuconfig for XSM in INSTALL?

I can remove the HAS_CHECKPOLICY check completely and make the call to
checkpolicy only conditional on the Kconfig option.  I think this is
less complicated than stopping the compile one step above the invocation
of checkpolicy, and probably just as informative (and better, if the
detection heuristic ever breaks).

>>
>>> +endif
>>
>> Regards,
>>
>> --
>> Julien Grall
>
Konrad Rzeszutek Wilk June 24, 2016, 5:40 p.m. UTC | #5
On Fri, Jun 24, 2016 at 01:34:29PM -0400, Daniel De Graaf wrote:
> On 06/24/2016 12:50 PM, Konrad Rzeszutek Wilk wrote:
> >On Fri, Jun 24, 2016 at 05:30:32PM +0100, Julien Grall wrote:
> >>Hello Daniel,
> >>
> >>Please try to CC relevant maintainers on your patch. I would have missed it
> >>if Andrew did not ping me on IRC.
> >>
> >>On 20/06/16 15:04, 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.
> >>>
> >>>Enabling this option only builds the policy if checkpolicy is available
> >>>during compilation of the hypervisor; otherwise, it does nothing.  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>
> >>>Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >>
> >>For ARM bits:
> >>
> >>Acked-by: Julien Grall <julien.grall@arm.com>
> >>
> >>Although, I one a question below.
> >>
> >>[...]
> >>
> >>>diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile
> >>>index 12fc3a9..eefd37c 100644
> >>>--- a/xen/xsm/flask/Makefile
> >>>+++ b/xen/xsm/flask/Makefile
> >>>@@ -27,6 +27,23 @@ $(FLASK_H_FILES): $(FLASK_H_DEPEND)
> >>> $(AV_H_FILES): $(AV_H_DEPEND)
> >>> 	$(CONFIG_SHELL) policy/mkaccess_vector.sh $(AWK) $(AV_H_DEPEND)
> >>>
> >>>+ifeq ($(CONFIG_XSM_POLICY),y)
> >>>+HAS_CHECKPOLICY := $(shell checkpolicy -h 2>&1 | grep -q xen && echo y || echo n)
> >>>+
> >>>+obj-$(HAS_CHECKPOLICY) += policy.o
> >>
> >>I would have expect a warning (if not an error) here to tell the user that
> >>checkpolicy is not available. Otherwise it may take some time to the user to
> >>understand why the policy is not loaded/present. Because if you enable XSM,
> >>you don't necessarily check which other options have been enabled by
> >>default.
> >
> >Good point! And we should probably update the INSTALL document too to mention
> >that you need checkpoint tool!
> 
> The dependency on checkpolicy is called out in the Kconfig item that
> enables this option.  Are you suggesting I should add a mention below
> the instructions on running menuconfig for XSM in INSTALL?

No, I just thinking that the INSTALL has a list of packages one needs.
And it may be good to have checkpolicy on it.

> 
> I can remove the HAS_CHECKPOLICY check completely and make the call to
> checkpolicy only conditional on the Kconfig option.  I think this is
> less complicated than stopping the compile one step above the invocation
> of checkpolicy, and probably just as informative (and better, if the
> detection heuristic ever breaks).

I actually like the way you have it - with the checkpolicy check determining
whether the Kconfig option for XSM is shown or not.
Konrad Rzeszutek Wilk June 24, 2016, 5:41 p.m. UTC | #6
On Fri, Jun 24, 2016 at 01:34:29PM -0400, Daniel De Graaf wrote:
> On 06/24/2016 12:50 PM, Konrad Rzeszutek Wilk wrote:
> >On Fri, Jun 24, 2016 at 05:30:32PM +0100, Julien Grall wrote:
> >>Hello Daniel,
> >>
> >>Please try to CC relevant maintainers on your patch. I would have missed it
> >>if Andrew did not ping me on IRC.
> >>
> >>On 20/06/16 15:04, 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.
> >>>
> >>>Enabling this option only builds the policy if checkpolicy is available
> >>>during compilation of the hypervisor; otherwise, it does nothing.  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>
> >>>Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >>
> >>For ARM bits:
> >>
> >>Acked-by: Julien Grall <julien.grall@arm.com>
> >>
> >>Although, I one a question below.
> >>
> >>[...]
> >>
> >>>diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile
> >>>index 12fc3a9..eefd37c 100644
> >>>--- a/xen/xsm/flask/Makefile
> >>>+++ b/xen/xsm/flask/Makefile
> >>>@@ -27,6 +27,23 @@ $(FLASK_H_FILES): $(FLASK_H_DEPEND)
> >>> $(AV_H_FILES): $(AV_H_DEPEND)
> >>> 	$(CONFIG_SHELL) policy/mkaccess_vector.sh $(AWK) $(AV_H_DEPEND)
> >>>
> >>>+ifeq ($(CONFIG_XSM_POLICY),y)
> >>>+HAS_CHECKPOLICY := $(shell checkpolicy -h 2>&1 | grep -q xen && echo y || echo n)
> >>>+
> >>>+obj-$(HAS_CHECKPOLICY) += policy.o
> >>
> >>I would have expect a warning (if not an error) here to tell the user that
> >>checkpolicy is not available. Otherwise it may take some time to the user to
> >>understand why the policy is not loaded/present. Because if you enable XSM,
> >>you don't necessarily check which other options have been enabled by
> >>default.

You won't be able to enable XSM if checkpolicy is not present.
As in, it won't show up as an option in the oldconfig or menuconfig at all.
Daniel De Graaf June 24, 2016, 5:42 p.m. UTC | #7
On 06/24/2016 01:40 PM, Konrad Rzeszutek Wilk wrote:
> On Fri, Jun 24, 2016 at 01:34:29PM -0400, Daniel De Graaf wrote:
>> On 06/24/2016 12:50 PM, Konrad Rzeszutek Wilk wrote:
>>> On Fri, Jun 24, 2016 at 05:30:32PM +0100, Julien Grall wrote:
>>>> Hello Daniel,
>>>>
>>>> Please try to CC relevant maintainers on your patch. I would have missed it
>>>> if Andrew did not ping me on IRC.
>>>>
>>>> On 20/06/16 15:04, 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.
>>>>>
>>>>> Enabling this option only builds the policy if checkpolicy is available
>>>>> during compilation of the hypervisor; otherwise, it does nothing.  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>
>>>>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>>
>>>> For ARM bits:
>>>>
>>>> Acked-by: Julien Grall <julien.grall@arm.com>
>>>>
>>>> Although, I one a question below.
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile
>>>>> index 12fc3a9..eefd37c 100644
>>>>> --- a/xen/xsm/flask/Makefile
>>>>> +++ b/xen/xsm/flask/Makefile
>>>>> @@ -27,6 +27,23 @@ $(FLASK_H_FILES): $(FLASK_H_DEPEND)
>>>>> $(AV_H_FILES): $(AV_H_DEPEND)
>>>>> 	$(CONFIG_SHELL) policy/mkaccess_vector.sh $(AWK) $(AV_H_DEPEND)
>>>>>
>>>>> +ifeq ($(CONFIG_XSM_POLICY),y)
>>>>> +HAS_CHECKPOLICY := $(shell checkpolicy -h 2>&1 | grep -q xen && echo y || echo n)
>>>>> +
>>>>> +obj-$(HAS_CHECKPOLICY) += policy.o
>>>>
>>>> I would have expect a warning (if not an error) here to tell the user that
>>>> checkpolicy is not available. Otherwise it may take some time to the user to
>>>> understand why the policy is not loaded/present. Because if you enable XSM,
>>>> you don't necessarily check which other options have been enabled by
>>>> default.
>>>
>>> Good point! And we should probably update the INSTALL document too to mention
>>> that you need checkpoint tool!
>>
>> The dependency on checkpolicy is called out in the Kconfig item that
>> enables this option.  Are you suggesting I should add a mention below
>> the instructions on running menuconfig for XSM in INSTALL?
>
> No, I just thinking that the INSTALL has a list of packages one needs.
> And it may be good to have checkpolicy on it.
>
>>
>> I can remove the HAS_CHECKPOLICY check completely and make the call to
>> checkpolicy only conditional on the Kconfig option.  I think this is
>> less complicated than stopping the compile one step above the invocation
>> of checkpolicy, and probably just as informative (and better, if the
>> detection heuristic ever breaks).
>
> I actually like the way you have it - with the checkpolicy check determining
> whether the Kconfig option for XSM is shown or not.

Is that possible?  That's not what I have; the check I have only determines
if the Kconfig option does anything or not, it is still visible regardless.
Julien Grall June 24, 2016, 5:44 p.m. UTC | #8
Hi Daniel,

On 24/06/16 18:34, Daniel De Graaf wrote:
> On 06/24/2016 12:50 PM, Konrad Rzeszutek Wilk wrote:
>> On Fri, Jun 24, 2016 at 05:30:32PM +0100, Julien Grall wrote:
>>>> diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile
>>>> index 12fc3a9..eefd37c 100644
>>>> --- a/xen/xsm/flask/Makefile
>>>> +++ b/xen/xsm/flask/Makefile
>>>> @@ -27,6 +27,23 @@ $(FLASK_H_FILES): $(FLASK_H_DEPEND)
>>>>  $(AV_H_FILES): $(AV_H_DEPEND)
>>>>      $(CONFIG_SHELL) policy/mkaccess_vector.sh $(AWK) $(AV_H_DEPEND)
>>>>
>>>> +ifeq ($(CONFIG_XSM_POLICY),y)
>>>> +HAS_CHECKPOLICY := $(shell checkpolicy -h 2>&1 | grep -q xen &&
>>>> echo y || echo n)
>>>> +
>>>> +obj-$(HAS_CHECKPOLICY) += policy.o
>>>
>>> I would have expect a warning (if not an error) here to tell the user
>>> that
>>> checkpolicy is not available. Otherwise it may take some time to the
>>> user to
>>> understand why the policy is not loaded/present. Because if you
>>> enable XSM,
>>> you don't necessarily check which other options have been enabled by
>>> default.
>>
>> Good point! And we should probably update the INSTALL document too to
>> mention
>> that you need checkpoint tool!
>
> The dependency on checkpolicy is called out in the Kconfig item that
> enables this option.

AFAICT, the dependency is only called out for CONFIG_XSM_POLICY. 
However, this option is enabled by default if CONFIG_XSM is selected.

User may not read what was enabled by default because of CONFIG_XSM 
selected.

In any case, silently disable an option is a bad idea and will likely 
cause trouble in the future if we ever decide to enable XSM by default. 
Another use case would be, a user post his .config on the ML asking why 
the policy is not loaded.

Regards,
Konrad Rzeszutek Wilk June 24, 2016, 5:46 p.m. UTC | #9
> >>I can remove the HAS_CHECKPOLICY check completely and make the call to
> >>checkpolicy only conditional on the Kconfig option.  I think this is
> >>less complicated than stopping the compile one step above the invocation
> >>of checkpolicy, and probably just as informative (and better, if the
> >>detection heuristic ever breaks).
> >
> >I actually like the way you have it - with the checkpolicy check determining
> >whether the Kconfig option for XSM is shown or not.
> 
> Is that possible?  That's not what I have; the check I have only determines
> if the Kconfig option does anything or not, it is still visible regardless.

Totally!

See 95111a94f0168699d5154c7a25bd33865559e2c xsplice: Stacking build-id dependency checking.

Thanks.
Julien Grall June 24, 2016, 5:47 p.m. UTC | #10
Hello Konrad,

On 24/06/16 18:41, Konrad Rzeszutek Wilk wrote:
> On Fri, Jun 24, 2016 at 01:34:29PM -0400, Daniel De Graaf wrote:
>> On 06/24/2016 12:50 PM, Konrad Rzeszutek Wilk wrote:
>>> On Fri, Jun 24, 2016 at 05:30:32PM +0100, Julien Grall wrote:
>>>> I would have expect a warning (if not an error) here to tell the user that
>>>> checkpolicy is not available. Otherwise it may take some time to the user to
>>>> understand why the policy is not loaded/present. Because if you enable XSM,
>>>> you don't necessarily check which other options have been enabled by
>>>> default.
>
> You won't be able to enable XSM if checkpolicy is not present.
> As in, it won't show up as an option in the oldconfig or menuconfig at all.

Really? I don't have checkpolicy on my platform and XSM is still present 
in the menuconfig.

Regards,
Daniel De Graaf June 24, 2016, 6:02 p.m. UTC | #11
On 06/24/2016 01:46 PM, Konrad Rzeszutek Wilk wrote:
>>>> I can remove the HAS_CHECKPOLICY check completely and make the call to
>>>> checkpolicy only conditional on the Kconfig option.  I think this is
>>>> less complicated than stopping the compile one step above the invocation
>>>> of checkpolicy, and probably just as informative (and better, if the
>>>> detection heuristic ever breaks).
>>>
>>> I actually like the way you have it - with the checkpolicy check determining
>>> whether the Kconfig option for XSM is shown or not.
>>
>> Is that possible?  That's not what I have; the check I have only determines
>> if the Kconfig option does anything or not, it is still visible regardless.
>
> Totally!
>
> See 95111a94f0168699d5154c7a25bd33865559e2c xsplice: Stacking build-id dependency checking.
>
> Thanks.

Ah, I hadn't considered setting the variable in the top-level Config.mk.
If I were to add the HAS_CHECKPOLICY check there, I think it would make
sense to have it adjust the default value of CONFIG_XSM_POLICY, but
not hide the option.  If someone deliberately enables the option, then
having the compile error show up is less confusing than the current
method where it gets enabled when only selecting XSM.

Anyway, since checkpolicy is required to make use of FLASK, anyone who
currently enables XSM is going to need to install it at some point: either
in the hypervisor compile for the built-in policy or the tools compile for
the bootloader- or dom0-provided policy.  Having the error show up sooner
is not all that much of a problem.  This would change if XSM were to be
enabled by default, because I would then expect "xsm enabled, flask disabled"
to become a more common case - and that does not require a policy.
Konrad Rzeszutek Wilk June 24, 2016, 6:36 p.m. UTC | #12
On Fri, Jun 24, 2016 at 02:02:42PM -0400, Daniel De Graaf wrote:
> On 06/24/2016 01:46 PM, Konrad Rzeszutek Wilk wrote:
> >>>>I can remove the HAS_CHECKPOLICY check completely and make the call to
> >>>>checkpolicy only conditional on the Kconfig option.  I think this is
> >>>>less complicated than stopping the compile one step above the invocation
> >>>>of checkpolicy, and probably just as informative (and better, if the
> >>>>detection heuristic ever breaks).
> >>>
> >>>I actually like the way you have it - with the checkpolicy check determining
> >>>whether the Kconfig option for XSM is shown or not.
> >>
> >>Is that possible?  That's not what I have; the check I have only determines
> >>if the Kconfig option does anything or not, it is still visible regardless.
> >
> >Totally!
> >
> >See 95111a94f0168699d5154c7a25bd33865559e2c xsplice: Stacking build-id dependency checking.
> >
> >Thanks.
> 
> Ah, I hadn't considered setting the variable in the top-level Config.mk.
> If I were to add the HAS_CHECKPOLICY check there, I think it would make
> sense to have it adjust the default value of CONFIG_XSM_POLICY, but
> not hide the option.  If someone deliberately enables the option, then
> having the compile error show up is less confusing than the current
> method where it gets enabled when only selecting XSM.

Ah, that would work too and I believe satisfy Julien as well!
> 
> Anyway, since checkpolicy is required to make use of FLASK, anyone who
> currently enables XSM is going to need to install it at some point: either
> in the hypervisor compile for the built-in policy or the tools compile for
> the bootloader- or dom0-provided policy.  Having the error show up sooner
> is not all that much of a problem.  This would change if XSM were to be
> enabled by default, because I would then expect "xsm enabled, flask disabled"
> to become a more common case - and that does not require a policy.

/me nods.
> 
> -- 
> Daniel De Graaf
> National Security Agency
diff mbox

Patch

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index fed732c..c85d1dc 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -704,13 +704,15 @@  enabled by running either:
   with untrusted guests.  If a policy is provided by the bootloader, it will be
   loaded; errors will be reported to the ring buffer but will not prevent
   booting.  The policy can be changed to enforcing mode using "xl setenforce".
-* `enforcing`: This requires a security policy to be provided by the bootloader
-  and will enter enforcing mode prior to the creation of domain 0.  If a valid
-  policy is not provided, the hypervisor will not continue booting.
-* `late`: This disables loading of the security policy from the bootloader.
-  FLASK will be enabled but will not enforce access controls until a policy is
-  loaded by a domain using "xl loadpolicy".  Once a policy is loaded, FLASK will
-  run in enforcing mode unless "xl setenforce" has changed that setting.
+* `enforcing`: This will cause the security server to enter enforcing mode prior
+  to the creation of domain 0.  If an valid policy is not provided by the
+  bootloader and no built-in policy is present, the hypervisor will not continue
+  booting.
+* `late`: This disables loading of the built-in security policy or the policy
+  provided by the bootloader.  FLASK will be enabled but will not enforce access
+  controls until a policy is loaded by a domain using "xl loadpolicy".  Once a
+  policy is loaded, FLASK will run in enforcing mode unless "xl setenforce" has
+  changed that setting.
 * `disabled`: This causes the XSM framework to revert to the dummy module.  The
   dummy module provides the same security policy as is used when compiling the
   hypervisor without support for XSM.  The xsm\_op hypercall can also be used to
diff --git a/docs/misc/xsm-flask.txt b/docs/misc/xsm-flask.txt
index 2f42585..62f15dd 100644
--- a/docs/misc/xsm-flask.txt
+++ b/docs/misc/xsm-flask.txt
@@ -141,21 +141,21 @@  only type enforcement is used and the user and role are set to system_u and
 system_r for all domains.
 
 The FLASK security framework is mostly configured using a security policy file.
-This policy file is not normally generated during the Xen build process because
-it relies on the SELinux compiler "checkpolicy"; run
-
-	make -C tools/flask/policy
-
-to compile the example policy included with Xen. The policy is generated from
-definition files under this directory. Most changes to security policy will
-involve creating or modifying modules found in tools/flask/policy/modules/.  The
-modules.conf file there defines what modules are enabled and has short
-descriptions of each module.
-
-The XSM policy file needs to be copied to /boot and loaded as a module by grub.
-The exact position of the module does not matter as long as it is after the Xen
-kernel; it is normally placed either just above the dom0 kernel or at the end.
-Once dom0 is running, the policy can be reloaded using "xl loadpolicy".
+It relies on the SELinux compiler "checkpolicy"; if this is available, the
+policy will be compiled as part of the tools build.  If hypervisor support for a
+built-in policy is enabled ("Compile Xen with a built-in security policy"), the
+policy will be built during the hypervisor build.
+
+The policy is generated from definition files in tools/flask/policy.  Most
+changes to security policy will involve creating or modifying modules found in
+tools/flask/policy/modules/.  The modules.conf file there defines what modules
+are enabled and has short descriptions of each module.
+
+If not using the built-in policy, the XSM policy file needs to be copied to
+/boot and loaded as a module by grub.  The exact position and filename of the
+module does not matter as long as it is after the Xen kernel; it is normally
+placed either just above the dom0 kernel or at the end.  Once dom0 is running,
+the policy can be reloaded using "xl loadpolicy".
 
 The example policy included with Xen demonstrates most of the features of FLASK
 that can be used without dom0 disaggregation. The main types for domUs are:
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 8320381..80c2299 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -139,6 +139,10 @@  SECTIONS
        *(.init.data.rel)
        *(.init.data.rel.*)
 
+       __xsm_init_policy_start = .;
+       *(.init.xsm_policy)
+       __xsm_init_policy_end = .;
+
        . = ALIGN(8);
        __ctors_start = .;
        *(.init_array)
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index dcbb8fe..9072e1c 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -155,6 +155,11 @@  SECTIONS
        *(.init.data)
        *(.init.data.rel)
        *(.init.data.rel.*)
+
+       __xsm_init_policy_start = .;
+       *(.init.xsm_policy)
+       __xsm_init_policy_end = .;
+
        . = ALIGN(4);
        __trampoline_rel_start = .;
        *(.trampoline_rel)
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 8fb5a68..6e2a583 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -143,6 +143,23 @@  config FLASK_AVC_STATS
 
 	  If unsure, say Y.
 
+config XSM_POLICY
+	bool "Compile Xen with a built-in security policy"
+	default y
+	depends on XSM
+	---help---
+	  This includes a default XSM policy in the hypervisor so that the
+	  bootloader does not need to load a policy to get sane behavior from an
+	  XSM-enabled hypervisor.  If this is disabled, a policy must be
+	  provided by the bootloader or by Domain 0.  Even if this is enabled, a
+	  policy provided by the bootloader will override it.
+
+	  This requires that the SELinux policy compiler (checkpolicy) be
+	  available when compiling the hypervisor; if this tool is not found, no
+	  policy will be added.
+
+	  If unsure, say Y.
+
 # Enable schedulers
 menu "Schedulers"
 	visible if EXPERT = "y"
diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile
index 12fc3a9..eefd37c 100644
--- a/xen/xsm/flask/Makefile
+++ b/xen/xsm/flask/Makefile
@@ -27,6 +27,23 @@  $(FLASK_H_FILES): $(FLASK_H_DEPEND)
 $(AV_H_FILES): $(AV_H_DEPEND)
 	$(CONFIG_SHELL) policy/mkaccess_vector.sh $(AWK) $(AV_H_DEPEND)
 
+ifeq ($(CONFIG_XSM_POLICY),y)
+HAS_CHECKPOLICY := $(shell checkpolicy -h 2>&1 | grep -q xen && echo y || echo n)
+
+obj-$(HAS_CHECKPOLICY) += policy.o
+endif
+
+LDFLAGS += --accept-unknown-input-arch
+
+POLICY_SRC := $(XEN_ROOT)/tools/flask/policy/xenpolicy-$(XEN_FULLVERSION)
+
+policy.bin: FORCE
+	$(MAKE) -C $(XEN_ROOT)/tools/flask/policy
+	cmp -s $(POLICY_SRC) $@ || cp $(POLICY_SRC) $@
+
+policy.o: policy.bin
+	$(OBJCOPY) -S -I binary -O elf64-little --rename-section=.data=.init.xsm_policy $< $@
+
 .PHONY: clean
 clean::
 	rm -f $(ALL_H_FILES) *.o $(DEPS)
diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
index 8df1a3c..509210c 100644
--- a/xen/xsm/xsm_core.c
+++ b/xen/xsm/xsm_core.c
@@ -36,6 +36,17 @@  static inline int verify(struct xsm_operations *ops)
     return 0;
 }
 
+extern char __xsm_init_policy_start[], __xsm_init_policy_end[];
+
+static void __init xsm_policy_init(void)
+{
+    if ( policy_size == 0 && __xsm_init_policy_end != __xsm_init_policy_start )
+    {
+        policy_buffer = __xsm_init_policy_start;
+        policy_size = __xsm_init_policy_end - __xsm_init_policy_start;
+    }
+}
+
 static int __init xsm_core_init(void)
 {
     if ( verify(&dummy_xsm_ops) )
@@ -46,6 +57,7 @@  static int __init xsm_core_init(void)
     }
 
     xsm_ops = &dummy_xsm_ops;
+    xsm_policy_init();
     flask_init();
 
     return 0;
@@ -98,7 +110,8 @@  int __init xsm_dt_init(void)
 
     ret = xsm_core_init();
 
-    xfree(policy_buffer);
+    if ( policy_buffer != __xsm_init_policy_start )
+        xfree(policy_buffer);
 
     return ret;
 }