Re: [Xen-devel] [PATCH 17/17] xsm: add a default policy to .init.data

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> > > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > For ARM bits:
> >
> > Acked-by: Julien Grall <julien.grall@xxxxxxx>
> >
> > 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

--
Daniel De Graaf
National Security Agency

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