| Message ID | 20170320192726.11014-1-tamas.lengyel@zentific.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 20.03.17 at 20:27, <tamas.lengyel@zentific.com> wrote: > Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> > Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> > Acked-by: Jan Beulich <jbeulich@suse.com> I'll need to make this conditional upon a few more adjustments: > @@ -4370,18 +4370,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] ) I think it would be better for readers if you compared against XEN_ALTP2M_disabled here. > + { > + rc = -EINVAL; > + goto out; > + } > + > + if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d, > + d->arch.hvm_domain.params[HVM_PARAM_ALTP2M])) ) Note the implicit truncation here. Not a problem at present, but at the very least I'd like to ask for the function parameter to become unsigned int. Furthermore, wasn't HVMOP_altp2m_vcpu_enable_notify supposed to always be available to the guest (as long as altp2m is enabled)? You don't allow this here anymore. > --- a/xen/include/public/hvm/params.h > +++ b/xen/include/public/hvm/params.h > @@ -228,8 +228,16 @@ > /* Location of the VM Generation ID in guest physical address space. */ > #define HVM_PARAM_VM_GENERATION_ID_ADDR 34 > > -/* Boolean: Enable altp2m */ > +/* > + * Set mode for altp2m: > + * disabled: don't activate altp2m (default) > + * mixed: allow access to altp2m for both in-guest and external tools > + * external_only: allow access to external privileged tools only This needs to be more precise: VMFUNC is an access mechanism too, and aiui this isn't meant to be disabled by external_only. > + */ > #define HVM_PARAM_ALTP2M 35 > +#define XEN_ALTP2M_disabled 0 > +#define XEN_ALTP2M_mixed 1 > +#define XEN_ALTP2M_external_only 2 I'd drop the _only suffix. > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -555,10 +555,18 @@ static XSM_INLINE int > xsm_hvm_param_altp2mhvm(XSM_DEFAULT_ARG struct domain *d) > return xsm_default_action(action, current->domain, d); > } > > -static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d) > +static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d, int mode) > { > - XSM_ASSERT_ACTION(XSM_TARGET); > - return xsm_default_action(action, current->domain, d); > + XSM_ASSERT_ACTION(XSM_OTHER); > + switch ( mode ) > + { > + case XEN_ALTP2M_mixed: > + return xsm_default_action(XSM_TARGET, current->domain, d); > + case XEN_ALTP2M_external_only: > + return xsm_default_action(XSM_DM_PRIV, current->domain, d); > + default: > + return -EPERM; I think -EPERM is correct at most for XEN_ALTP2M_disabled, all others should get -EINVAL or -EOPNOTSUPP or some such. Perhaps that also doesn't really belong here, but rather into the caller (which right now produces -EINVAL for XEN_ALTP2M_disabled only). Jan
On Tue, Mar 21, 2017 at 3:54 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 20.03.17 at 20:27, <tamas.lengyel@zentific.com> wrote: >> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> >> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> >> Acked-by: Jan Beulich <jbeulich@suse.com> > > I'll need to make this conditional upon a few more adjustments: > >> @@ -4370,18 +4370,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] ) > > I think it would be better for readers if you compared against > XEN_ALTP2M_disabled here. > >> + { >> + rc = -EINVAL; >> + goto out; >> + } >> + >> + if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d, >> + d->arch.hvm_domain.params[HVM_PARAM_ALTP2M])) ) > > Note the implicit truncation here. Not a problem at present, but > at the very least I'd like to ask for the function parameter to > become unsigned int. Sure. > > Furthermore, wasn't HVMOP_altp2m_vcpu_enable_notify > supposed to always be available to the guest (as long as altp2m > is enabled)? You don't allow this here anymore. Absolutely not, that's one of the main reasons why I want the external_only option to be available in the first place. For malware analysis it is a huge hole if the guest can decide that it wants certain EPT violations to be handled by the guest without first going to the hypervisor or if it can start switching its EPT tables around. > >> --- a/xen/include/public/hvm/params.h >> +++ b/xen/include/public/hvm/params.h >> @@ -228,8 +228,16 @@ >> /* Location of the VM Generation ID in guest physical address space. */ >> #define HVM_PARAM_VM_GENERATION_ID_ADDR 34 >> >> -/* Boolean: Enable altp2m */ >> +/* >> + * Set mode for altp2m: >> + * disabled: don't activate altp2m (default) >> + * mixed: allow access to altp2m for both in-guest and external tools >> + * external_only: allow access to external privileged tools only > > This needs to be more precise: VMFUNC is an access mechanism too, > and aiui this isn't meant to be disabled by external_only. Yes, it is meant to be disabled by external_only, same as with #VE. > >> + */ >> #define HVM_PARAM_ALTP2M 35 >> +#define XEN_ALTP2M_disabled 0 >> +#define XEN_ALTP2M_mixed 1 >> +#define XEN_ALTP2M_external_only 2 > > I'd drop the _only suffix. Sure. > >> --- a/xen/include/xsm/dummy.h >> +++ b/xen/include/xsm/dummy.h >> @@ -555,10 +555,18 @@ static XSM_INLINE int >> xsm_hvm_param_altp2mhvm(XSM_DEFAULT_ARG struct domain *d) >> return xsm_default_action(action, current->domain, d); >> } >> >> -static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d) >> +static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d, int mode) >> { >> - XSM_ASSERT_ACTION(XSM_TARGET); >> - return xsm_default_action(action, current->domain, d); >> + XSM_ASSERT_ACTION(XSM_OTHER); >> + switch ( mode ) >> + { >> + case XEN_ALTP2M_mixed: >> + return xsm_default_action(XSM_TARGET, current->domain, d); >> + case XEN_ALTP2M_external_only: >> + return xsm_default_action(XSM_DM_PRIV, current->domain, d); >> + default: >> + return -EPERM; > > I think -EPERM is correct at most for XEN_ALTP2M_disabled, all > others should get -EINVAL or -EOPNOTSUPP or some such. Perhaps > that also doesn't really belong here, but rather into the caller (which > right now produces -EINVAL for XEN_ALTP2M_disabled only). > The reason I want -EPERM here is so that a malicious guest can't differentiate between a guest being created with "external_only" altp2m and one that has an XSM policy that denies these operations. What you propose would lead to information a leak that would make such differentiation possible to a malicious guest. Tamas
>>> On 21.03.17 at 17:30, <tamas.lengyel@zentific.com> wrote: > On Tue, Mar 21, 2017 at 3:54 AM, Jan Beulich <JBeulich@suse.com> wrote: >> Furthermore, wasn't HVMOP_altp2m_vcpu_enable_notify >> supposed to always be available to the guest (as long as altp2m >> is enabled)? You don't allow this here anymore. > > Absolutely not, that's one of the main reasons why I want the > external_only option to be available in the first place. For malware > analysis it is a huge hole if the guest can decide that it wants > certain EPT violations to be handled by the guest without first going > to the hypervisor or if it can start switching its EPT tables around. In which case I guess we need three modes (besides disabled): - guest can alter permissions - guest can pick tables - guest can do nothing >>> --- a/xen/include/xsm/dummy.h >>> +++ b/xen/include/xsm/dummy.h >>> @@ -555,10 +555,18 @@ static XSM_INLINE int >>> xsm_hvm_param_altp2mhvm(XSM_DEFAULT_ARG struct domain *d) >>> return xsm_default_action(action, current->domain, d); >>> } >>> >>> -static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d) >>> +static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d, int mode) >>> { >>> - XSM_ASSERT_ACTION(XSM_TARGET); >>> - return xsm_default_action(action, current->domain, d); >>> + XSM_ASSERT_ACTION(XSM_OTHER); >>> + switch ( mode ) >>> + { >>> + case XEN_ALTP2M_mixed: >>> + return xsm_default_action(XSM_TARGET, current->domain, d); >>> + case XEN_ALTP2M_external_only: >>> + return xsm_default_action(XSM_DM_PRIV, current->domain, d); >>> + default: >>> + return -EPERM; >> >> I think -EPERM is correct at most for XEN_ALTP2M_disabled, all >> others should get -EINVAL or -EOPNOTSUPP or some such. Perhaps >> that also doesn't really belong here, but rather into the caller (which >> right now produces -EINVAL for XEN_ALTP2M_disabled only). >> > > The reason I want -EPERM here is so that a malicious guest can't > differentiate between a guest being created with "external_only" > altp2m and one that has an XSM policy that denies these operations. > What you propose would lead to information a leak that would make such > differentiation possible to a malicious guest. What difference does it make security wise if the guest knows who denied access? The reason for my comment is that -EPERM does not correctly reflect the actual error here. Jan
On Tue, Mar 21, 2017 at 10:38 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 21.03.17 at 17:30, <tamas.lengyel@zentific.com> wrote: >> On Tue, Mar 21, 2017 at 3:54 AM, Jan Beulich <JBeulich@suse.com> wrote: >>> Furthermore, wasn't HVMOP_altp2m_vcpu_enable_notify >>> supposed to always be available to the guest (as long as altp2m >>> is enabled)? You don't allow this here anymore. >> >> Absolutely not, that's one of the main reasons why I want the >> external_only option to be available in the first place. For malware >> analysis it is a huge hole if the guest can decide that it wants >> certain EPT violations to be handled by the guest without first going >> to the hypervisor or if it can start switching its EPT tables around. > > In which case I guess we need three modes (besides disabled): > - guest can alter permissions > - guest can pick tables > - guest can do nothing Why do you think those other two modes would be needed? I have no use-case for any of these other then where the guest can do nothing. I also don't see what would be the usecase for the other two that would warrant their addition over the mixed use that exists already. > >>>> --- a/xen/include/xsm/dummy.h >>>> +++ b/xen/include/xsm/dummy.h >>>> @@ -555,10 +555,18 @@ static XSM_INLINE int >>>> xsm_hvm_param_altp2mhvm(XSM_DEFAULT_ARG struct domain *d) >>>> return xsm_default_action(action, current->domain, d); >>>> } >>>> >>>> -static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d) >>>> +static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d, int mode) >>>> { >>>> - XSM_ASSERT_ACTION(XSM_TARGET); >>>> - return xsm_default_action(action, current->domain, d); >>>> + XSM_ASSERT_ACTION(XSM_OTHER); >>>> + switch ( mode ) >>>> + { >>>> + case XEN_ALTP2M_mixed: >>>> + return xsm_default_action(XSM_TARGET, current->domain, d); >>>> + case XEN_ALTP2M_external_only: >>>> + return xsm_default_action(XSM_DM_PRIV, current->domain, d); >>>> + default: >>>> + return -EPERM; >>> >>> I think -EPERM is correct at most for XEN_ALTP2M_disabled, all >>> others should get -EINVAL or -EOPNOTSUPP or some such. Perhaps >>> that also doesn't really belong here, but rather into the caller (which >>> right now produces -EINVAL for XEN_ALTP2M_disabled only). >>> >> >> The reason I want -EPERM here is so that a malicious guest can't >> differentiate between a guest being created with "external_only" >> altp2m and one that has an XSM policy that denies these operations. >> What you propose would lead to information a leak that would make such >> differentiation possible to a malicious guest. > > What difference does it make security wise if the guest knows who > denied access? The reason for my comment is that -EPERM does > not correctly reflect the actual error here. My use-case is malware analysis. Any difference a malicious guest can pick up about its host environment can be used to make a malware lie dormant. Effectively it can aid the development of split-personality malware that does one thing in a real environment while doing something else when being analyzed. Tamas
>>> On 21.03.17 at 17:43, <tamas.lengyel@zentific.com> wrote: > On Tue, Mar 21, 2017 at 10:38 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 21.03.17 at 17:30, <tamas.lengyel@zentific.com> wrote: >>> On Tue, Mar 21, 2017 at 3:54 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> Furthermore, wasn't HVMOP_altp2m_vcpu_enable_notify >>>> supposed to always be available to the guest (as long as altp2m >>>> is enabled)? You don't allow this here anymore. >>> >>> Absolutely not, that's one of the main reasons why I want the >>> external_only option to be available in the first place. For malware >>> analysis it is a huge hole if the guest can decide that it wants >>> certain EPT violations to be handled by the guest without first going >>> to the hypervisor or if it can start switching its EPT tables around. >> >> In which case I guess we need three modes (besides disabled): >> - guest can alter permissions >> - guest can pick tables >> - guest can do nothing > > Why do you think those other two modes would be needed? I have no > use-case for any of these other then where the guest can do nothing. I > also don't see what would be the usecase for the other two that would > warrant their addition over the mixed use that exists already. Well, "mixed" I understand is what I've listed first. And the 2nd option clearly is more secure than the first _without_ taking away all control from the guest. The set above is basically my summary of things wanted by the different parties, as I've understood the discussion so far. I quite possibly may be wrong with that ... Jan
On Tue, Mar 21, 2017 at 11:06 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 21.03.17 at 17:43, <tamas.lengyel@zentific.com> wrote: >> On Tue, Mar 21, 2017 at 10:38 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 21.03.17 at 17:30, <tamas.lengyel@zentific.com> wrote: >>>> On Tue, Mar 21, 2017 at 3:54 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> Furthermore, wasn't HVMOP_altp2m_vcpu_enable_notify >>>>> supposed to always be available to the guest (as long as altp2m >>>>> is enabled)? You don't allow this here anymore. >>>> >>>> Absolutely not, that's one of the main reasons why I want the >>>> external_only option to be available in the first place. For malware >>>> analysis it is a huge hole if the guest can decide that it wants >>>> certain EPT violations to be handled by the guest without first going >>>> to the hypervisor or if it can start switching its EPT tables around. >>> >>> In which case I guess we need three modes (besides disabled): >>> - guest can alter permissions >>> - guest can pick tables >>> - guest can do nothing >> >> Why do you think those other two modes would be needed? I have no >> use-case for any of these other then where the guest can do nothing. I >> also don't see what would be the usecase for the other two that would >> warrant their addition over the mixed use that exists already. > > Well, "mixed" I understand is what I've listed first. And the 2nd > option clearly is more secure than the first _without_ taking > away all control from the guest. The set above is basically my > summary of things wanted by the different parties, as I've > understood the discussion so far. I quite possibly may be wrong > with that ... I might have missed it too but I don't think there is a need for config-based setup for where the guest can alter permissions but not switch tables, or vice-verse. The reason I want the external mode to be available as a domain config option is to avoid having to have all my users custom-compile Xen from source just to enable XSM to deny these altp2m hvm ops. I haven't seen anyone else having an issue with just using the mixed mode as-is when using altp2m. Tamas
>>> On 21.03.17 at 18:09, <tamas.lengyel@zentific.com> wrote: > On Tue, Mar 21, 2017 at 11:06 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 21.03.17 at 17:43, <tamas.lengyel@zentific.com> wrote: >>> On Tue, Mar 21, 2017 at 10:38 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 21.03.17 at 17:30, <tamas.lengyel@zentific.com> wrote: >>>>> On Tue, Mar 21, 2017 at 3:54 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> Furthermore, wasn't HVMOP_altp2m_vcpu_enable_notify >>>>>> supposed to always be available to the guest (as long as altp2m >>>>>> is enabled)? You don't allow this here anymore. >>>>> >>>>> Absolutely not, that's one of the main reasons why I want the >>>>> external_only option to be available in the first place. For malware >>>>> analysis it is a huge hole if the guest can decide that it wants >>>>> certain EPT violations to be handled by the guest without first going >>>>> to the hypervisor or if it can start switching its EPT tables around. >>>> >>>> In which case I guess we need three modes (besides disabled): >>>> - guest can alter permissions >>>> - guest can pick tables >>>> - guest can do nothing >>> >>> Why do you think those other two modes would be needed? I have no >>> use-case for any of these other then where the guest can do nothing. I >>> also don't see what would be the usecase for the other two that would >>> warrant their addition over the mixed use that exists already. >> >> Well, "mixed" I understand is what I've listed first. And the 2nd >> option clearly is more secure than the first _without_ taking >> away all control from the guest. The set above is basically my >> summary of things wanted by the different parties, as I've >> understood the discussion so far. I quite possibly may be wrong >> with that ... > > I might have missed it too but I don't think there is a need for > config-based setup for where the guest can alter permissions but not > switch tables, or vice-verse. The reason I want the external mode to > be available as a domain config option is to avoid having to have all > my users custom-compile Xen from source just to enable XSM to deny > these altp2m hvm ops. I haven't seen anyone else having an issue with > just using the mixed mode as-is when using altp2m. Hmm, the original (abstract) VMFUNC use case, as I have understood it, allows a guest to actively select between EPT variants without having (direct) control over their contents. Jan
On Tue, Mar 21, 2017 at 11:19 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 21.03.17 at 18:09, <tamas.lengyel@zentific.com> wrote: >> On Tue, Mar 21, 2017 at 11:06 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 21.03.17 at 17:43, <tamas.lengyel@zentific.com> wrote: >>>> On Tue, Mar 21, 2017 at 10:38 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>> On 21.03.17 at 17:30, <tamas.lengyel@zentific.com> wrote: >>>>>> On Tue, Mar 21, 2017 at 3:54 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> Furthermore, wasn't HVMOP_altp2m_vcpu_enable_notify >>>>>>> supposed to always be available to the guest (as long as altp2m >>>>>>> is enabled)? You don't allow this here anymore. >>>>>> >>>>>> Absolutely not, that's one of the main reasons why I want the >>>>>> external_only option to be available in the first place. For malware >>>>>> analysis it is a huge hole if the guest can decide that it wants >>>>>> certain EPT violations to be handled by the guest without first going >>>>>> to the hypervisor or if it can start switching its EPT tables around. >>>>> >>>>> In which case I guess we need three modes (besides disabled): >>>>> - guest can alter permissions >>>>> - guest can pick tables >>>>> - guest can do nothing >>>> >>>> Why do you think those other two modes would be needed? I have no >>>> use-case for any of these other then where the guest can do nothing. I >>>> also don't see what would be the usecase for the other two that would >>>> warrant their addition over the mixed use that exists already. >>> >>> Well, "mixed" I understand is what I've listed first. And the 2nd >>> option clearly is more secure than the first _without_ taking >>> away all control from the guest. The set above is basically my >>> summary of things wanted by the different parties, as I've >>> understood the discussion so far. I quite possibly may be wrong >>> with that ... >> >> I might have missed it too but I don't think there is a need for >> config-based setup for where the guest can alter permissions but not >> switch tables, or vice-verse. The reason I want the external mode to >> be available as a domain config option is to avoid having to have all >> my users custom-compile Xen from source just to enable XSM to deny >> these altp2m hvm ops. I haven't seen anyone else having an issue with >> just using the mixed mode as-is when using altp2m. > > Hmm, the original (abstract) VMFUNC use case, as I have > understood it, allows a guest to actively select between EPT > variants without having (direct) control over their contents. Correct. But even when altp2m is enabled on the domain VMFUNC and #VE is not available to the guest unless vmx_vcpu_update_vmfunc_ve is called via HVMOP_altp2m_vcpu_enable_notify. That HVMOP is created such that it can be called from the guest, which I need to be able to deny. Tamas
On Mon, Mar 20, 2017 at 01:27:26PM -0600, Tamas K Lengyel wrote: > > if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM && > (libxl_defbool_val(d_config->b_info.u.hvm.nested_hvm) && > - libxl_defbool_val(d_config->b_info.u.hvm.altp2m))) { > + (libxl_defbool_val(d_config->b_info.u.hvm.altp2m) || > + (d_config->b_info.altp2m != LIBXL_ALTP2M_MODE_DISABLED)))) { > + LOG(ERROR, "nestedhvm and altp2m cannot be used together"); Please use the LOGD macro here. Otherwise, the toolstack code looks good. There is a discussion pending on how many modes are needed, so I will wait until that to settle down before I ack the toolstack bits.
>>> On 21.03.17 at 18:25, <tamas.lengyel@zentific.com> wrote: > On Tue, Mar 21, 2017 at 11:19 AM, Jan Beulich <JBeulich@suse.com> wrote: >> Hmm, the original (abstract) VMFUNC use case, as I have >> understood it, allows a guest to actively select between EPT >> variants without having (direct) control over their contents. > > Correct. But even when altp2m is enabled on the domain VMFUNC and #VE > is not available to the guest unless vmx_vcpu_update_vmfunc_ve is > called via HVMOP_altp2m_vcpu_enable_notify. That HVMOP is created such > that it can be called from the guest, which I need to be able to deny. All understood, but with you agreeing with my statement above, I think you also agree that there need to be two levels of what can be denied: altp2m ops in general vs altp2m ops except HVMOP_altp2m_vcpu_enable_notify (then implying the guest being allowed to use VMFUNC and receive #VE). Which, as suggested earlier, results in a total of 3 modes (besides fully disabled altp2m). Jan
On Wed, Mar 22, 2017 at 2:06 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 21.03.17 at 18:25, <tamas.lengyel@zentific.com> wrote: >> On Tue, Mar 21, 2017 at 11:19 AM, Jan Beulich <JBeulich@suse.com> wrote: >>> Hmm, the original (abstract) VMFUNC use case, as I have >>> understood it, allows a guest to actively select between EPT >>> variants without having (direct) control over their contents. >> >> Correct. But even when altp2m is enabled on the domain VMFUNC and #VE >> is not available to the guest unless vmx_vcpu_update_vmfunc_ve is >> called via HVMOP_altp2m_vcpu_enable_notify. That HVMOP is created such >> that it can be called from the guest, which I need to be able to deny. > > All understood, but with you agreeing with my statement above, > I think you also agree that there need to be two levels of what > can be denied: altp2m ops in general vs altp2m ops except > HVMOP_altp2m_vcpu_enable_notify (then implying the guest > being allowed to use VMFUNC and receive #VE). Which, as > suggested earlier, results in a total of 3 modes (besides fully > disabled altp2m). > We can certainly add two levels of what can be denied. IMHO at the moment there isn't a usecase for that, so there isn't much point doing that. OTOH it shouldn't be much work to add it so fine I guess. Tamas
On Wed, Mar 22, 2017 at 9:40 AM, Tamas K Lengyel <tamas.lengyel@zentific.com> wrote: > On Wed, Mar 22, 2017 at 2:06 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 21.03.17 at 18:25, <tamas.lengyel@zentific.com> wrote: >>> On Tue, Mar 21, 2017 at 11:19 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> Hmm, the original (abstract) VMFUNC use case, as I have >>>> understood it, allows a guest to actively select between EPT >>>> variants without having (direct) control over their contents. >>> >>> Correct. But even when altp2m is enabled on the domain VMFUNC and #VE >>> is not available to the guest unless vmx_vcpu_update_vmfunc_ve is >>> called via HVMOP_altp2m_vcpu_enable_notify. That HVMOP is created such >>> that it can be called from the guest, which I need to be able to deny. >> >> All understood, but with you agreeing with my statement above, >> I think you also agree that there need to be two levels of what >> can be denied: altp2m ops in general vs altp2m ops except >> HVMOP_altp2m_vcpu_enable_notify (then implying the guest >> being allowed to use VMFUNC and receive #VE). Which, as >> suggested earlier, results in a total of 3 modes (besides fully >> disabled altp2m). >> > > We can certainly add two levels of what can be denied. IMHO at the > moment there isn't a usecase for that, so there isn't much point doing > that. OTOH it shouldn't be much work to add it so fine I guess. > So looking at the code this is easy to implement for when XSM is disabled. However, when XSM is enabled such "limited" mode for altp2m doesn't seem possible to achieve as things are right now as all altp2m hvmops are treated under a single XSM label. So I can add this mode to the case when XSM is disabled, but the development effort to split the XSM label is more then what I can do at the moment. Just an FYI. Tamas
diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in index 52802d5d60..9ecda1e02c 100644 --- a/docs/man/xl.cfg.pod.5.in +++ b/docs/man/xl.cfg.pod.5.in @@ -1283,6 +1283,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 B<altp2m=MODE> + +Specifies access mode to the alternate-p2m capability. Alternate-p2m allows a +guest to manage multiple p2m guest physical "memory views" (as opposed to a +single p2m). This option is disabled by default and is available to x86 hvm +domains. You may want this option if you want to access-control/isolate +access to specific guest physical memory pages accessed by the guest, e.g. for +domain memory introspection or for isolation/access-control of memory between +components within a single guest domain. + +The valid values are as follows: + +=over 4 + +=item B<"disabled"> + +Altp2m is disabled for the domain (default). + +=item B<"mixed"> + +The mixed mode allows access to the altp2m interface for both in-guest +and external tools as well. + +=item B<"external_only"> + +Enables access to the alternate-p2m capability for hvm guests only +by external privileged tools. Note: if XSM is enabled then the XSM policy +should be used to specify external-only access to the interface. + +=back + =item B<altp2mhvm=BOOLEAN> Enables or disables hvm guest access to alternate-p2m capability. @@ -1293,7 +1324,11 @@ You may want this option if you want to access-control/isolate access to specific guest physical memory pages accessed by the guest, e.g. for HVM domain memory introspection or for isolation/access-control of memory between components within -a single guest hvm domain. +a single guest hvm domain. This option is deprecated, use the option +"altp2m" instead. + +Note: While the option "altp2mhvm" is deprecated, legacy applications for +x86 systems will continue to work using it. =item B<nestedhvm=BOOLEAN> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 25389e13f8..95c1b7b2db 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -899,14 +899,17 @@ static void initiate_domain_create(libxl__egc *egc, if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM && (libxl_defbool_val(d_config->b_info.u.hvm.nested_hvm) && - libxl_defbool_val(d_config->b_info.u.hvm.altp2m))) { + (libxl_defbool_val(d_config->b_info.u.hvm.altp2m) || + (d_config->b_info.altp2m != LIBXL_ALTP2M_MODE_DISABLED)))) { + LOG(ERROR, "nestedhvm and altp2m cannot be used together"); ret = ERROR_INVAL; LOGD(ERROR, domid, "nestedhvm and altp2mhvm cannot be used together"); goto error_out; } if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM && - libxl_defbool_val(d_config->b_info.u.hvm.altp2m) && + (libxl_defbool_val(d_config->b_info.u.hvm.altp2m) || + (d_config->b_info.altp2m != LIBXL_ALTP2M_MODE_DISABLED)) && pod_enabled) { ret = ERROR_INVAL; LOGD(ERROR, domid, "Cannot enable PoD and ALTP2M at the same time"); diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c index e133962db3..464c0067e0 100644 --- a/tools/libxl/libxl_dom.c +++ b/tools/libxl/libxl_dom.c @@ -291,8 +291,6 @@ static void hvm_set_conf_params(xc_interface *handle, uint32_t domid, libxl_defbool_val(info->u.hvm.vpt_align)); xc_hvm_param_set(handle, domid, HVM_PARAM_NESTEDHVM, libxl_defbool_val(info->u.hvm.nested_hvm)); - xc_hvm_param_set(handle, domid, HVM_PARAM_ALTP2M, - libxl_defbool_val(info->u.hvm.altp2m)); } int libxl__build_pre(libxl__gc *gc, uint32_t domid, @@ -441,6 +439,22 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid, #endif } + /* Alternate p2m support on x86 is available only for HVM guests. */ + if (info->type == LIBXL_DOMAIN_TYPE_HVM) { + /* The config parameter "altp2m" replaces the parameter "altp2mhvm". For + * legacy reasons, both parameters are accepted on x86 HVM guests. + * + * If the legacy field info->u.hvm.altp2m is set, activate altp2m. + * Otherwise set altp2m based on the field info->altp2m. */ + if (info->altp2m == LIBXL_ALTP2M_MODE_DISABLED && + libxl_defbool_val(info->u.hvm.altp2m)) + xc_hvm_param_set(ctx->xch, domid, HVM_PARAM_ALTP2M, + libxl_defbool_val(info->u.hvm.altp2m)); + else + xc_hvm_param_set(ctx->xch, domid, HVM_PARAM_ALTP2M, + info->altp2m); + } + rc = libxl__arch_domain_create(gc, d_config, domid); return rc; diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index 2475a4df8d..985b577b53 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -440,6 +440,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), @@ -517,6 +524,9 @@ libxl_domain_build_info = Struct("domain_build_info",[ ("mmio_hole_memkb", MemKB), ("timer_mode", libxl_timer_mode), ("nested_hvm", libxl_defbool), + # The u.hvm.altp2m field is used solely + # for x86 HVM guests and is maintained + # for legacy purposes. ("altp2m", libxl_defbool), ("system_firmware", string), ("smbios_firmware", string), @@ -567,6 +577,9 @@ libxl_domain_build_info = Struct("domain_build_info",[ ("arch_arm", Struct(None, [("gic_version", libxl_gic_version), ])), + # Alternate p2m is not bound to any architecture or guest type, as it is + # supported by x86 HVM and ARM support is planned. + ("altp2m", libxl_altp2m_mode), ], dir=DIR_IN ) diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index b72f99059e..0648119c49 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -1155,7 +1155,9 @@ void parse_config_data(const char *config_source, xlu_cfg_get_defbool(config, "nestedhvm", &b_info->u.hvm.nested_hvm, 0); - xlu_cfg_get_defbool(config, "altp2mhvm", &b_info->u.hvm.altp2m, 0); + if (!xlu_cfg_get_defbool(config, "altp2mhvm", &b_info->u.hvm.altp2m, 0)) + fprintf(stderr, "WARNING: Specifying \"altp2mhvm\" is deprecated. " + "Please use \"altp2m\" instead.\n"); xlu_cfg_replace_string(config, "smbios_firmware", &b_info->u.hvm.smbios_firmware, 0); @@ -1215,6 +1217,22 @@ void parse_config_data(const char *config_source, abort(); } + if (!xlu_cfg_get_long(config, "altp2m", &l, 1)) { + if (l < LIBXL_ALTP2M_MODE_DISABLED || + l > LIBXL_ALTP2M_MODE_EXTERNAL_ONLY) { + fprintf(stderr, "ERROR: invalid value %ld for \"altp2m\"\n", l); + exit (1); + } + + b_info->altp2m = l; + } else if (!xlu_cfg_get_string(config, "altp2m", &buf, 0)) { + if (libxl_altp2m_mode_from_string(buf, &b_info->altp2m)) { + fprintf(stderr, "ERROR: invalid value \"%s\" for \"altp2m\"\n", + buf); + exit (1); + } + } + if (!xlu_cfg_get_list(config, "ioports", &ioports, &num_ioports, 0)) { b_info->num_ioports = num_ioports; b_info->ioports = calloc(num_ioports, sizeof(*b_info->ioports)); diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 0282986738..39e1c9a75e 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4110,7 +4110,7 @@ static int hvmop_set_param( rc = xsm_hvm_param_altp2mhvm(XSM_PRIV, d); if ( rc ) break; - if ( a.value > 1 ) + if ( a.value > XEN_ALTP2M_external_only ) rc = -EINVAL; if ( a.value && d->arch.hvm_domain.params[HVM_PARAM_NESTEDHVM] ) @@ -4370,18 +4370,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])) ) goto out; switch ( a.cmd ) { case HVMOP_altp2m_get_domain_state: - if ( !d->arch.hvm_domain.params[HVM_PARAM_ALTP2M] ) - { - rc = -EINVAL; - break; - } - a.u.domain_state.state = altp2m_active(d); rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; break; @@ -4391,8 +4392,7 @@ static int do_altp2m_op( struct vcpu *v; bool_t ostate; - if ( !d->arch.hvm_domain.params[HVM_PARAM_ALTP2M] || - nestedhvm_enabled(d) ) + if ( nestedhvm_enabled(d) ) { rc = -EINVAL; break; diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h index 58c8478ad5..9315af2c10 100644 --- a/xen/include/public/hvm/params.h +++ b/xen/include/public/hvm/params.h @@ -228,8 +228,16 @@ /* Location of the VM Generation ID in guest physical address space. */ #define HVM_PARAM_VM_GENERATION_ID_ADDR 34 -/* Boolean: Enable altp2m */ +/* + * Set mode for altp2m: + * disabled: don't activate altp2m (default) + * mixed: allow access to altp2m for both in-guest and external tools + * external_only: allow access to external privileged tools only + */ #define HVM_PARAM_ALTP2M 35 +#define XEN_ALTP2M_disabled 0 +#define XEN_ALTP2M_mixed 1 +#define XEN_ALTP2M_external_only 2 /* * Size of the x87 FPU FIP/FDP registers that the hypervisor needs to diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index ff730391d5..75b83779dd 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -555,10 +555,18 @@ static XSM_INLINE int xsm_hvm_param_altp2mhvm(XSM_DEFAULT_ARG struct domain *d) return xsm_default_action(action, current->domain, d); } -static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d) +static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d, int mode) { - XSM_ASSERT_ACTION(XSM_TARGET); - return xsm_default_action(action, current->domain, d); + XSM_ASSERT_ACTION(XSM_OTHER); + switch ( mode ) + { + case XEN_ALTP2M_mixed: + return xsm_default_action(XSM_TARGET, current->domain, d); + case XEN_ALTP2M_external_only: + return xsm_default_action(XSM_DM_PRIV, current->domain, d); + default: + return -EPERM; + }; } static XSM_INLINE int xsm_vm_event_control(XSM_DEFAULT_ARG struct domain *d, int mode, int op) diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index 2cf7ac10db..b59c9580b5 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -140,7 +140,7 @@ struct xsm_operations { int (*hvm_control) (struct domain *d, unsigned long op); int (*hvm_param_nested) (struct domain *d); int (*hvm_param_altp2mhvm) (struct domain *d); - int (*hvm_altp2mhvm_op) (struct domain *d); + int (*hvm_altp2mhvm_op) (struct domain *d, int mode); int (*get_vnumainfo) (struct domain *d); int (*vm_event_control) (struct domain *d, int mode, int op); @@ -579,9 +579,9 @@ static inline int xsm_hvm_param_altp2mhvm (xsm_default_t def, struct domain *d) return xsm_ops->hvm_param_altp2mhvm(d); } -static inline int xsm_hvm_altp2mhvm_op (xsm_default_t def, struct domain *d) +static inline int xsm_hvm_altp2mhvm_op (xsm_default_t def, struct domain *d, int mode) { - return xsm_ops->hvm_altp2mhvm_op(d); + return xsm_ops->hvm_altp2mhvm_op(d, mode); } static inline int xsm_get_vnumainfo (xsm_default_t def, struct domain *d) diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index 4baed39890..f1b8fa290e 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -1197,7 +1197,7 @@ static int flask_hvm_param_altp2mhvm(struct domain *d) return current_has_perm(d, SECCLASS_HVM, HVM__ALTP2MHVM); } -static int flask_hvm_altp2mhvm_op(struct domain *d) +static int flask_hvm_altp2mhvm_op(struct domain *d, int mode) { return current_has_perm(d, SECCLASS_HVM, HVM__ALTP2MHVM_OP); }