Re: [Xen-devel] [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

2016-08-16 Thread Daniel De Graaf 

On 08/11/2016 10:51 AM, Jan Beulich wrote:

On 11.08.16 at 16:37,  wrote:

On Aug 11, 2016 06:02, "Jan Beulich"  wrote:



On 10.08.16 at 17:00,  wrote:

@@ -5238,18 +5238,19 @@ static int do_altp2m_op(
 goto out;
 }

-if ( (rc = xsm_hvm_altp2mhvm_op(XSM_TARGET, d)) )
+if ( !d->arch.hvm_domain.params[HVM_PARAM_ALTP2M] )
+{
+rc = -EINVAL;
+goto out;
+}
+
+if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d,
+d->arch.hvm_domain.params[HVM_PARAM_ALTP2M])) )


I'm sorry that this didn't occur to me on v1 already, but is there
really a need for passing this extra argument, when the callee
could - if it cared in the first place - read the value itself?


I'm not sure if it's ok to have xsm poke around in arch specific parts like
this. We are adding this hvm param for ARM in another series but still..


Daniel, what's your opinion?

Jan


XSM does have some required arch-specific knowledge already (x86 IO port
labeling, in particular), so it's really a style question.  I'd prefer the
form with the value passed in so that it's clearer what the XSM check is
inspecting to determine what to do, especially in this case where it changes
what permissions are actually being enforced (in the non-FLASK case).

--
Daniel De Graaf
National Security Agency

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

2016-08-12 Thread Wei Liu 
On Fri, Aug 12, 2016 at 08:51:14AM -0600, Tamas K Lengyel wrote:
> On Aug 12, 2016 05:24, "Julien Grall"  wrote:
> >
> > Hello Tamas,
> >
> >
> > On 10/08/2016 17:00, Tamas K Lengyel wrote:
> >>
> >> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> >> index ef614be..97948fd 100644
> >> --- a/tools/libxl/libxl_types.idl
> >> +++ b/tools/libxl/libxl_types.idl
> >> @@ -439,6 +439,13 @@ libxl_rdm_reserve = Struct("rdm_reserve", [
> >>  ("policy",  libxl_rdm_reserve_policy),
> >>  ])
> >>
> >> +# Consistent with the values defined for HVM_PARAM_ALTP2M
> >> +libxl_altp2m_mode = Enumeration("altp2m_mode", [
> >> +(0, "disabled"),
> >> +(1, "mixed"),
> >> +(2, "external_only"),
> >> +], init_val = "LIBXL_ALTP2M_MODE_DISABLED")
> >> +
> >>  libxl_domain_build_info = Struct("domain_build_info",[
> >>  ("max_vcpus",   integer),
> >>  ("avail_vcpus", libxl_bitmap),
> >> @@ -512,7 +519,7 @@ libxl_domain_build_info =
> Struct("domain_build_info",[
> >> ("mmio_hole_memkb",  MemKB),
> >> ("timer_mode",
>  libxl_timer_mode),
> >> ("nested_hvm",
>  libxl_defbool),
> >> -   ("altp2m",
>  libxl_defbool),
> >> +   ("altp2m",
>  libxl_altp2m_mode),
> >
> >
> > Should we move altp2m directly outside of the structure "hvm" to avoid
> yet another change when altp2m will be supported on ARM? (see [1])
> >
> > Regards,
> >
> > [1] https://lists.xen.org/archives/html/xen-devel/2016-08/msg00147.html
> >
> > --
> > Julien Grall
> 
> Hi Julien,
> We can do that here if preferred by the tools maintainers though I'm on the
> opinion that it should be adjusted in the ARM altp2m series once it
> actually becomes available there.
> 

I agree. We shouldn't mix things together.

Wei.

> Tamas

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

2016-08-12 Thread Tamas K Lengyel 
On Aug 12, 2016 05:24, "Julien Grall"  wrote:
>
> Hello Tamas,
>
>
> On 10/08/2016 17:00, Tamas K Lengyel wrote:
>>
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index ef614be..97948fd 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -439,6 +439,13 @@ libxl_rdm_reserve = Struct("rdm_reserve", [
>>  ("policy",  libxl_rdm_reserve_policy),
>>  ])
>>
>> +# Consistent with the values defined for HVM_PARAM_ALTP2M
>> +libxl_altp2m_mode = Enumeration("altp2m_mode", [
>> +(0, "disabled"),
>> +(1, "mixed"),
>> +(2, "external_only"),
>> +], init_val = "LIBXL_ALTP2M_MODE_DISABLED")
>> +
>>  libxl_domain_build_info = Struct("domain_build_info",[
>>  ("max_vcpus",   integer),
>>  ("avail_vcpus", libxl_bitmap),
>> @@ -512,7 +519,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
>> ("mmio_hole_memkb",  MemKB),
>> ("timer_mode",
 libxl_timer_mode),
>> ("nested_hvm",
 libxl_defbool),
>> -   ("altp2m",
 libxl_defbool),
>> +   ("altp2m",
 libxl_altp2m_mode),
>
>
> Should we move altp2m directly outside of the structure "hvm" to avoid
yet another change when altp2m will be supported on ARM? (see [1])
>
> Regards,
>
> [1] https://lists.xen.org/archives/html/xen-devel/2016-08/msg00147.html
>
> --
> Julien Grall

Hi Julien,
We can do that here if preferred by the tools maintainers though I'm on the
opinion that it should be adjusted in the ARM altp2m series once it
actually becomes available there.

Tamas
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

2016-08-12 Thread Julien Grall 

Hello Tamas,

On 10/08/2016 17:00, Tamas K Lengyel wrote:

diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index ef614be..97948fd 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -439,6 +439,13 @@ libxl_rdm_reserve = Struct("rdm_reserve", [
 ("policy",  libxl_rdm_reserve_policy),
 ])

+# Consistent with the values defined for HVM_PARAM_ALTP2M
+libxl_altp2m_mode = Enumeration("altp2m_mode", [
+(0, "disabled"),
+(1, "mixed"),
+(2, "external_only"),
+], init_val = "LIBXL_ALTP2M_MODE_DISABLED")
+
 libxl_domain_build_info = Struct("domain_build_info",[
 ("max_vcpus",   integer),
 ("avail_vcpus", libxl_bitmap),
@@ -512,7 +519,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("mmio_hole_memkb",  MemKB),
("timer_mode",   libxl_timer_mode),
("nested_hvm",   libxl_defbool),
-   ("altp2m",   libxl_defbool),
+   ("altp2m",   libxl_altp2m_mode),


Should we move altp2m directly outside of the structure "hvm" to avoid 
yet another change when altp2m will be supported on ARM? (see [1])


Regards,

[1] https://lists.xen.org/archives/html/xen-devel/2016-08/msg00147.html

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

2016-08-11 Thread Wei Liu 
On Thu, Aug 11, 2016 at 11:50:44AM -0600, Tamas K Lengyel wrote:
> On Thu, Aug 11, 2016 at 11:17 AM, Wei Liu  wrote:
> > On Wed, Aug 10, 2016 at 09:00:15AM -0600, Tamas K Lengyel wrote:
> >> Currently setting altp2mhvm=1 in the domain configuration allows access to 
> >> the
> >> altp2m interface for both in-guest and external privileged tools. This 
> >> poses
> >> a problem for use-cases where only external access should be allowed, 
> >> requiring
> >> the user to compile Xen with XSM enabled to be able to appropriately 
> >> restrict
> >> access.
> >>
> >> In this patch we deprecate the altp2mhvm domain configuration option and
> >> introduce the altp2m option, which allows specifying if by default the 
> >> altp2m
> >> interface should be external-only. The information is stored in
> >> HVM_PARAM_ALTP2M which we now define with specific XEN_ALTP2M_* modes.
> >> If external_only mode is selected, the XSM check is shifted to use 
> >> XSM_DM_PRIV
> >> type check, thus restricting access to the interface by the guest itself. 
> >> Note
> >> that we keep the default XSM policy untouched. Users of XSM who wish to 
> >> enforce
> >> external_only mode for altp2m can do so by adjusting their XSM policy 
> >> directly,
> >> as this domain config option does not override an active XSM policy.
> >>
> >> Also, as part of this patch we adjust the hvmop handler to require
> >> HVM_PARAM_ALTP2M to be of a type other then disabled for all ops. This has 
> >> been
> >> previously only required for get/set altp2m domain state, all other options
> >> were gated on altp2m_enabled. Since altp2m_enabled only gets set during set
> >> altp2m domain state, this change introduces no new requirements to the 
> >> other
> >> ops but makes it more clear that it is required for all ops.
> >>
> >> Signed-off-by: Tamas K Lengyel 
> >> ---
> >> Cc: Ian Jackson 
> >> Cc: Wei Liu 
> >> Cc: Jan Beulich 
> >> Cc: Andrew Cooper 
> >> Cc: Daniel De Graaf 
> >>
> >> v2: Rename HVMALTP2M_* to XEN_ALTP2M_*
> >> Relax xsm check to XSM_DM_PRIV for external-only mode
> >> ---
> >>  docs/man/xl.cfg.pod.5.in| 31 +++
> >>  tools/libxl/libxl_create.c  |  5 ++---
> >>  tools/libxl/libxl_dom.c |  2 +-
> >>  tools/libxl/libxl_types.idl |  9 -
> >>  tools/libxl/xl_cmdimpl.c| 34 +-
> >>  xen/arch/x86/hvm/hvm.c  | 20 ++--
> >>  xen/include/public/hvm/params.h | 10 +-
> >>  xen/include/xsm/dummy.h | 14 +++---
> >>  xen/include/xsm/xsm.h   |  6 +++---
> >>  xen/xsm/flask/hooks.c   |  2 +-
> >>  10 files changed, 109 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
> >> index 48c9c0d..0044b98 100644
> >> --- a/docs/man/xl.cfg.pod.5.in
> >> +++ b/docs/man/xl.cfg.pod.5.in
> >> @@ -1268,6 +1268,37 @@ enabled by default and you should usually omit it. 
> >> It may be necessary
> >>  to disable the HPET in order to improve compatibility with guest
> >>  Operating Systems (X86 only)
> >>
> >> +=item

Re: [Xen-devel] [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

2016-08-11 Thread Tamas K Lengyel 
On Thu, Aug 11, 2016 at 11:17 AM, Wei Liu  wrote:
> On Wed, Aug 10, 2016 at 09:00:15AM -0600, Tamas K Lengyel wrote:
>> Currently setting altp2mhvm=1 in the domain configuration allows access to 
>> the
>> altp2m interface for both in-guest and external privileged tools. This poses
>> a problem for use-cases where only external access should be allowed, 
>> requiring
>> the user to compile Xen with XSM enabled to be able to appropriately restrict
>> access.
>>
>> In this patch we deprecate the altp2mhvm domain configuration option and
>> introduce the altp2m option, which allows specifying if by default the altp2m
>> interface should be external-only. The information is stored in
>> HVM_PARAM_ALTP2M which we now define with specific XEN_ALTP2M_* modes.
>> If external_only mode is selected, the XSM check is shifted to use 
>> XSM_DM_PRIV
>> type check, thus restricting access to the interface by the guest itself. 
>> Note
>> that we keep the default XSM policy untouched. Users of XSM who wish to 
>> enforce
>> external_only mode for altp2m can do so by adjusting their XSM policy 
>> directly,
>> as this domain config option does not override an active XSM policy.
>>
>> Also, as part of this patch we adjust the hvmop handler to require
>> HVM_PARAM_ALTP2M to be of a type other then disabled for all ops. This has 
>> been
>> previously only required for get/set altp2m domain state, all other options
>> were gated on altp2m_enabled. Since altp2m_enabled only gets set during set
>> altp2m domain state, this change introduces no new requirements to the other
>> ops but makes it more clear that it is required for all ops.
>>
>> Signed-off-by: Tamas K Lengyel 
>> ---
>> Cc: Ian Jackson 
>> Cc: Wei Liu 
>> Cc: Jan Beulich 
>> Cc: Andrew Cooper 
>> Cc: Daniel De Graaf 
>>
>> v2: Rename HVMALTP2M_* to XEN_ALTP2M_*
>> Relax xsm check to XSM_DM_PRIV for external-only mode
>> ---
>>  docs/man/xl.cfg.pod.5.in| 31 +++
>>  tools/libxl/libxl_create.c  |  5 ++---
>>  tools/libxl/libxl_dom.c |  2 +-
>>  tools/libxl/libxl_types.idl |  9 -
>>  tools/libxl/xl_cmdimpl.c| 34 +-
>>  xen/arch/x86/hvm/hvm.c  | 20 ++--
>>  xen/include/public/hvm/params.h | 10 +-
>>  xen/include/xsm/dummy.h | 14 +++---
>>  xen/include/xsm/xsm.h   |  6 +++---
>>  xen/xsm/flask/hooks.c   |  2 +-
>>  10 files changed, 109 insertions(+), 24 deletions(-)
>>
>> diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
>> index 48c9c0d..0044b98 100644
>> --- a/docs/man/xl.cfg.pod.5.in
>> +++ b/docs/man/xl.cfg.pod.5.in
>> @@ -1268,6 +1268,37 @@ enabled by default and you should usually omit it. It 
>> may be necessary
>>  to disable the HPET in order to improve compatibility with guest
>>  Operating Systems (X86 only)
>>
>> +=item

Re: [Xen-devel] [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

2016-08-11 Thread Wei Liu 
On Wed, Aug 10, 2016 at 09:00:15AM -0600, Tamas K Lengyel wrote:
> Currently setting altp2mhvm=1 in the domain configuration allows access to the
> altp2m interface for both in-guest and external privileged tools. This poses
> a problem for use-cases where only external access should be allowed, 
> requiring
> the user to compile Xen with XSM enabled to be able to appropriately restrict
> access.
> 
> In this patch we deprecate the altp2mhvm domain configuration option and
> introduce the altp2m option, which allows specifying if by default the altp2m
> interface should be external-only. The information is stored in
> HVM_PARAM_ALTP2M which we now define with specific XEN_ALTP2M_* modes.
> If external_only mode is selected, the XSM check is shifted to use XSM_DM_PRIV
> type check, thus restricting access to the interface by the guest itself. Note
> that we keep the default XSM policy untouched. Users of XSM who wish to 
> enforce
> external_only mode for altp2m can do so by adjusting their XSM policy 
> directly,
> as this domain config option does not override an active XSM policy.
> 
> Also, as part of this patch we adjust the hvmop handler to require
> HVM_PARAM_ALTP2M to be of a type other then disabled for all ops. This has 
> been
> previously only required for get/set altp2m domain state, all other options
> were gated on altp2m_enabled. Since altp2m_enabled only gets set during set
> altp2m domain state, this change introduces no new requirements to the other
> ops but makes it more clear that it is required for all ops.
> 
> Signed-off-by: Tamas K Lengyel 
> ---
> Cc: Ian Jackson 
> Cc: Wei Liu 
> Cc: Jan Beulich 
> Cc: Andrew Cooper 
> Cc: Daniel De Graaf 
> 
> v2: Rename HVMALTP2M_* to XEN_ALTP2M_*
> Relax xsm check to XSM_DM_PRIV for external-only mode
> ---
>  docs/man/xl.cfg.pod.5.in| 31 +++
>  tools/libxl/libxl_create.c  |  5 ++---
>  tools/libxl/libxl_dom.c |  2 +-
>  tools/libxl/libxl_types.idl |  9 -
>  tools/libxl/xl_cmdimpl.c| 34 +-
>  xen/arch/x86/hvm/hvm.c  | 20 ++--
>  xen/include/public/hvm/params.h | 10 +-
>  xen/include/xsm/dummy.h | 14 +++---
>  xen/include/xsm/xsm.h   |  6 +++---
>  xen/xsm/flask/hooks.c   |  2 +-
>  10 files changed, 109 insertions(+), 24 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
> index 48c9c0d..0044b98 100644
> --- a/docs/man/xl.cfg.pod.5.in
> +++ b/docs/man/xl.cfg.pod.5.in
> @@ -1268,6 +1268,37 @@ enabled by default and you should usually omit it. It 
> may be necessary
>  to disable the HPET in order to improve compatibility with guest
>  Operating Systems (X86 only)
>  
> +=item

Re: [Xen-devel] [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

2016-08-11 Thread Jan Beulich 
>>> On 11.08.16 at 16:37,  wrote:
> On Aug 11, 2016 06:02, "Jan Beulich"  wrote:
>>
>> >>> On 10.08.16 at 17:00,  wrote:
>> > @@ -5238,18 +5238,19 @@ static int do_altp2m_op(
>> >  goto out;
>> >  }
>> >
>> > -if ( (rc = xsm_hvm_altp2mhvm_op(XSM_TARGET, d)) )
>> > +if ( !d->arch.hvm_domain.params[HVM_PARAM_ALTP2M] )
>> > +{
>> > +rc = -EINVAL;
>> > +goto out;
>> > +}
>> > +
>> > +if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d,
>> > +d->arch.hvm_domain.params[HVM_PARAM_ALTP2M])) )
>>
>> I'm sorry that this didn't occur to me on v1 already, but is there
>> really a need for passing this extra argument, when the callee
>> could - if it cared in the first place - read the value itself?
> 
> I'm not sure if it's ok to have xsm poke around in arch specific parts like
> this. We are adding this hvm param for ARM in another series but still..

Daniel, what's your opinion?

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

2016-08-11 Thread Tamas K Lengyel 
On Aug 11, 2016 06:02, "Jan Beulich"  wrote:
>
> >>> On 10.08.16 at 17:00,  wrote:
> > @@ -5238,18 +5238,19 @@ static int do_altp2m_op(
> >  goto out;
> >  }
> >
> > -if ( (rc = xsm_hvm_altp2mhvm_op(XSM_TARGET, d)) )
> > +if ( !d->arch.hvm_domain.params[HVM_PARAM_ALTP2M] )
> > +{
> > +rc = -EINVAL;
> > +goto out;
> > +}
> > +
> > +if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d,
> > +d->arch.hvm_domain.params[HVM_PARAM_ALTP2M])) )
>
> I'm sorry that this didn't occur to me on v1 already, but is there
> really a need for passing this extra argument, when the callee
> could - if it cared in the first place - read the value itself?
>

I'm not sure if it's ok to have xsm poke around in arch specific parts like
this. We are adding this hvm param for ARM in another series but still..

Tamas
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

2016-08-11 Thread Jan Beulich 
>>> On 10.08.16 at 17:00,  wrote:
> @@ -5238,18 +5238,19 @@ static int do_altp2m_op(
>  goto out;
>  }
>  
> -if ( (rc = xsm_hvm_altp2mhvm_op(XSM_TARGET, d)) )
> +if ( !d->arch.hvm_domain.params[HVM_PARAM_ALTP2M] )
> +{
> +rc = -EINVAL;
> +goto out;
> +}
> +
> +if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d,
> +d->arch.hvm_domain.params[HVM_PARAM_ALTP2M])) )

I'm sorry that this didn't occur to me on v1 already, but is there
really a need for passing this extra argument, when the callee
could - if it cared in the first place - read the value itself?

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v2 2/2] x86/altp2m: allow specifying external-only use-case

2016-08-10 Thread Tamas K Lengyel 
Currently setting altp2mhvm=1 in the domain configuration allows access to the
altp2m interface for both in-guest and external privileged tools. This poses
a problem for use-cases where only external access should be allowed, requiring
the user to compile Xen with XSM enabled to be able to appropriately restrict
access.

In this patch we deprecate the altp2mhvm domain configuration option and
introduce the altp2m option, which allows specifying if by default the altp2m
interface should be external-only. The information is stored in
HVM_PARAM_ALTP2M which we now define with specific XEN_ALTP2M_* modes.
If external_only mode is selected, the XSM check is shifted to use XSM_DM_PRIV
type check, thus restricting access to the interface by the guest itself. Note
that we keep the default XSM policy untouched. Users of XSM who wish to enforce
external_only mode for altp2m can do so by adjusting their XSM policy directly,
as this domain config option does not override an active XSM policy.

Also, as part of this patch we adjust the hvmop handler to require
HVM_PARAM_ALTP2M to be of a type other then disabled for all ops. This has been
previously only required for get/set altp2m domain state, all other options
were gated on altp2m_enabled. Since altp2m_enabled only gets set during set
altp2m domain state, this change introduces no new requirements to the other
ops but makes it more clear that it is required for all ops.

Signed-off-by: Tamas K Lengyel 
---
Cc: Ian Jackson 
Cc: Wei Liu 
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Daniel De Graaf 

v2: Rename HVMALTP2M_* to XEN_ALTP2M_*
Relax xsm check to XSM_DM_PRIV for external-only mode
---
 docs/man/xl.cfg.pod.5.in| 31 +++
 tools/libxl/libxl_create.c  |  5 ++---
 tools/libxl/libxl_dom.c |  2 +-
 tools/libxl/libxl_types.idl |  9 -
 tools/libxl/xl_cmdimpl.c| 34 +-
 xen/arch/x86/hvm/hvm.c  | 20 ++--
 xen/include/public/hvm/params.h | 10 +-
 xen/include/xsm/dummy.h | 14 +++---
 xen/include/xsm/xsm.h   |  6 +++---
 xen/xsm/flask/hooks.c   |  2 +-
 10 files changed, 109 insertions(+), 24 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index 48c9c0d..0044b98 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -1268,6 +1268,37 @@ enabled by default and you should usually omit it. It 
may be necessary
 to disable the HPET in order to improve compatibility with guest
 Operating Systems (X86 only)
 
+=item