| Message ID | 20170518150758.9414-3-apop@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 18, 2017 at 9:07 AM, Adrian Pop <apop@bitdefender.com> wrote: > Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a > domain to change the value of the #VE suppress bit for a page. > > Signed-off-by: Adrian Pop <apop@bitdefender.com> > --- > xen/arch/x86/hvm/hvm.c | 14 ++++++++++++ > xen/arch/x86/mm/mem_access.c | 48 +++++++++++++++++++++++++++++++++++++++++ > xen/include/public/hvm/hvm_op.h | 15 +++++++++++++ > xen/include/xen/mem_access.h | 3 +++ > 4 files changed, 80 insertions(+) > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 2e76c2345b..eb01527c5b 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -4356,6 +4356,7 @@ static int do_altp2m_op( > case HVMOP_altp2m_destroy_p2m: > case HVMOP_altp2m_switch_p2m: > case HVMOP_altp2m_set_mem_access: > + case HVMOP_altp2m_set_suppress_ve: > case HVMOP_altp2m_change_gfn: > break; > default: > @@ -4472,6 +4473,19 @@ static int do_altp2m_op( > a.u.set_mem_access.view); > break; > > + case HVMOP_altp2m_set_suppress_ve: > + if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 ) > + rc = -EINVAL; > + else > + { > + gfn_t gfn = _gfn(a.u.set_mem_access.gfn); > + unsigned int altp2m_idx = a.u.set_mem_access.view; > + uint8_t suppress_ve = a.u.set_suppress_ve.suppress_ve; > + > + rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx); > + } > + break; > + > case HVMOP_altp2m_change_gfn: > if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 ) > rc = -EINVAL; > diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c > index d0b0767855..b9e611d3db 100644 > --- a/xen/arch/x86/mm/mem_access.c > +++ b/xen/arch/x86/mm/mem_access.c > @@ -466,6 +466,54 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access) > } > > /* > + * Set/clear the #VE suppress bit for a page. Only available on VMX. > + */ > +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve, > + unsigned int altp2m_idx) > +{ > + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); > + struct p2m_domain *ap2m = NULL; > + struct p2m_domain *p2m = NULL; > + mfn_t mfn; > + p2m_access_t a; > + p2m_type_t t; > + unsigned long gfn_l; > + int rc = 0; > + > + if ( !cpu_has_vmx ) > + return -EOPNOTSUPP; > + > + if ( altp2m_idx > 0 ) > + { > + if ( altp2m_idx >= MAX_ALTP2M || > + d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) > + return -EINVAL; > + > + p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx]; > + } > + else > + { > + p2m = host_p2m; > + } IMHO there should be some further checks here to verify that the domain has issued HVMOP_altp2m_vcpu_enable_notify before and that it was allowed (ie. this hypercall should not be able to enable the suppress_bit if there is no veinfo_gfn). That said, is there anything that would prevent a malicious application issuing rouge altp2m HVMOPs from messing with this if it is activated (which I guess stands for the rest of the altp2m ops too)? > + > + p2m_lock(host_p2m); > + if ( ap2m ) > + p2m_lock(ap2m); > + > + gfn_l = gfn_x(gfn); > + mfn = p2m->get_entry(p2m, gfn_l, &t, &a, 0, NULL, NULL); > + if ( !mfn_valid(mfn) ) > + return -ESRCH; > + rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, > + suppress_ve); > + if ( ap2m ) > + p2m_unlock(ap2m); > + p2m_unlock(host_p2m); > + > + return rc; > +} > + > +/* > * Local variables: > * mode: C > * c-file-style: "BSD" > diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h > index bc00ef0e65..9736092f58 100644 > --- a/xen/include/public/hvm/hvm_op.h > +++ b/xen/include/public/hvm/hvm_op.h > @@ -231,6 +231,18 @@ struct xen_hvm_altp2m_set_mem_access { > typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t; > DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t); > > +struct xen_hvm_altp2m_set_suppress_ve { > + /* view */ > + uint16_t view; > + uint8_t suppress_ve; > + uint8_t pad1; > + uint32_t pad2; > + /* gfn */ > + uint64_t gfn; > +}; > +typedef struct xen_hvm_altp2m_set_suppress_ve xen_hvm_altp2m_set_suppress_ve_t; > +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_suppress_ve_t); > + > struct xen_hvm_altp2m_change_gfn { > /* view */ > uint16_t view; > @@ -262,6 +274,8 @@ struct xen_hvm_altp2m_op { > #define HVMOP_altp2m_set_mem_access 7 > /* Change a p2m entry to have a different gfn->mfn mapping */ > #define HVMOP_altp2m_change_gfn 8 > +/* Set the "Suppress #VE" bit on a page */ > +#define HVMOP_altp2m_set_suppress_ve 9 > domid_t domain; > uint16_t pad1; > uint32_t pad2; > @@ -270,6 +284,7 @@ struct xen_hvm_altp2m_op { > struct xen_hvm_altp2m_vcpu_enable_notify enable_notify; > struct xen_hvm_altp2m_view view; > struct xen_hvm_altp2m_set_mem_access set_mem_access; > + struct xen_hvm_altp2m_set_suppress_ve set_suppress_ve; > struct xen_hvm_altp2m_change_gfn change_gfn; > uint8_t pad[64]; > } u; > diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h > index 5ab34c1553..b6e6a7650a 100644 > --- a/xen/include/xen/mem_access.h > +++ b/xen/include/xen/mem_access.h > @@ -78,6 +78,9 @@ long p2m_set_mem_access_multi(struct domain *d, > */ > int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access); > > +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve, > + unsigned int altp2m_idx); > + > #ifdef CONFIG_HAS_MEM_ACCESS > int mem_access_memop(unsigned long cmd, > XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg); > -- > 2.12.1 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
On Thu, May 18, 2017 at 11:27:44AM -0600, Tamas K Lengyel wrote: > On Thu, May 18, 2017 at 9:07 AM, Adrian Pop <apop@bitdefender.com> wrote: > > Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a > > domain to change the value of the #VE suppress bit for a page. > > > > Signed-off-by: Adrian Pop <apop@bitdefender.com> > > --- > > xen/arch/x86/hvm/hvm.c | 14 ++++++++++++ > > xen/arch/x86/mm/mem_access.c | 48 +++++++++++++++++++++++++++++++++++++++++ > > xen/include/public/hvm/hvm_op.h | 15 +++++++++++++ > > xen/include/xen/mem_access.h | 3 +++ > > 4 files changed, 80 insertions(+) > > > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > > index 2e76c2345b..eb01527c5b 100644 > > --- a/xen/arch/x86/hvm/hvm.c > > +++ b/xen/arch/x86/hvm/hvm.c > > @@ -4356,6 +4356,7 @@ static int do_altp2m_op( > > case HVMOP_altp2m_destroy_p2m: > > case HVMOP_altp2m_switch_p2m: > > case HVMOP_altp2m_set_mem_access: > > + case HVMOP_altp2m_set_suppress_ve: > > case HVMOP_altp2m_change_gfn: > > break; > > default: > > @@ -4472,6 +4473,19 @@ static int do_altp2m_op( > > a.u.set_mem_access.view); > > break; > > > > + case HVMOP_altp2m_set_suppress_ve: > > + if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 ) > > + rc = -EINVAL; > > + else > > + { > > + gfn_t gfn = _gfn(a.u.set_mem_access.gfn); > > + unsigned int altp2m_idx = a.u.set_mem_access.view; > > + uint8_t suppress_ve = a.u.set_suppress_ve.suppress_ve; > > + > > + rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx); > > + } > > + break; > > + > > case HVMOP_altp2m_change_gfn: > > if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 ) > > rc = -EINVAL; > > diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c > > index d0b0767855..b9e611d3db 100644 > > --- a/xen/arch/x86/mm/mem_access.c > > +++ b/xen/arch/x86/mm/mem_access.c > > @@ -466,6 +466,54 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access) > > } > > > > /* > > + * Set/clear the #VE suppress bit for a page. Only available on VMX. > > + */ > > +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve, > > + unsigned int altp2m_idx) > > +{ > > + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); > > + struct p2m_domain *ap2m = NULL; > > + struct p2m_domain *p2m = NULL; > > + mfn_t mfn; > > + p2m_access_t a; > > + p2m_type_t t; > > + unsigned long gfn_l; > > + int rc = 0; > > + > > + if ( !cpu_has_vmx ) > > + return -EOPNOTSUPP; > > + > > + if ( altp2m_idx > 0 ) > > + { > > + if ( altp2m_idx >= MAX_ALTP2M || > > + d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) > > + return -EINVAL; > > + > > + p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx]; > > + } > > + else > > + { > > + p2m = host_p2m; > > + } > > > IMHO there should be some further checks here to verify that the > domain has issued HVMOP_altp2m_vcpu_enable_notify before and that it > was allowed (ie. this hypercall should not be able to enable the > suppress_bit if there is no veinfo_gfn). That said, is there anything Ok, a check can be added easily. The reasoning behind not adding it in the first place was that should the #VE be disabled in the VM's VMCS, setting/clearing the suppress #VE bit would do nothing (it is ignored). > that would prevent a malicious application issuing rouge altp2m HVMOPs > from messing with this if it is activated (which I guess stands for > the rest of the altp2m ops too)? AFAIK there isn't any safeguard of this sort. I might just be excessively ignorant, though. On the other hand the current default behavior is to enable #VE for all the pages. The default with these patches would be to issue a VM-Exit, and either a SVA, the Dom0 or the target DomU itself could modify this behavior to generate #VE instead of VM-Exit. In any case I'll investigate some more. Thanks! > > + > > + p2m_lock(host_p2m); > > + if ( ap2m ) > > + p2m_lock(ap2m); > > + > > + gfn_l = gfn_x(gfn); > > + mfn = p2m->get_entry(p2m, gfn_l, &t, &a, 0, NULL, NULL); > > + if ( !mfn_valid(mfn) ) > > + return -ESRCH; > > + rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, > > + suppress_ve); > > + if ( ap2m ) > > + p2m_unlock(ap2m); > > + p2m_unlock(host_p2m); > > + > > + return rc; > > +} > > + > > +/* > > * Local variables: > > * mode: C > > * c-file-style: "BSD" > > diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h > > index bc00ef0e65..9736092f58 100644 > > --- a/xen/include/public/hvm/hvm_op.h > > +++ b/xen/include/public/hvm/hvm_op.h > > @@ -231,6 +231,18 @@ struct xen_hvm_altp2m_set_mem_access { > > typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t; > > DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t); > > > > +struct xen_hvm_altp2m_set_suppress_ve { > > + /* view */ > > + uint16_t view; > > + uint8_t suppress_ve; > > + uint8_t pad1; > > + uint32_t pad2; > > + /* gfn */ > > + uint64_t gfn; > > +}; > > +typedef struct xen_hvm_altp2m_set_suppress_ve xen_hvm_altp2m_set_suppress_ve_t; > > +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_suppress_ve_t); > > + > > struct xen_hvm_altp2m_change_gfn { > > /* view */ > > uint16_t view; > > @@ -262,6 +274,8 @@ struct xen_hvm_altp2m_op { > > #define HVMOP_altp2m_set_mem_access 7 > > /* Change a p2m entry to have a different gfn->mfn mapping */ > > #define HVMOP_altp2m_change_gfn 8 > > +/* Set the "Suppress #VE" bit on a page */ > > +#define HVMOP_altp2m_set_suppress_ve 9 > > domid_t domain; > > uint16_t pad1; > > uint32_t pad2; > > @@ -270,6 +284,7 @@ struct xen_hvm_altp2m_op { > > struct xen_hvm_altp2m_vcpu_enable_notify enable_notify; > > struct xen_hvm_altp2m_view view; > > struct xen_hvm_altp2m_set_mem_access set_mem_access; > > + struct xen_hvm_altp2m_set_suppress_ve set_suppress_ve; > > struct xen_hvm_altp2m_change_gfn change_gfn; > > uint8_t pad[64]; > > } u; > > diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h > > index 5ab34c1553..b6e6a7650a 100644 > > --- a/xen/include/xen/mem_access.h > > +++ b/xen/include/xen/mem_access.h > > @@ -78,6 +78,9 @@ long p2m_set_mem_access_multi(struct domain *d, > > */ > > int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access); > > > > +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve, > > + unsigned int altp2m_idx); > > + > > #ifdef CONFIG_HAS_MEM_ACCESS > > int mem_access_memop(unsigned long cmd, > > XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg); > > -- > > 2.12.1 > > > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > https://lists.xen.org/xen-devel
>>> On 18.05.17 at 17:07, <apop@bitdefender.com> wrote: > --- a/xen/arch/x86/mm/mem_access.c > +++ b/xen/arch/x86/mm/mem_access.c > @@ -466,6 +466,54 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access) > } > > /* > + * Set/clear the #VE suppress bit for a page. Only available on VMX. > + */ > +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve, suppress_ve presumably is meant to be boolean. > + unsigned int altp2m_idx) > +{ > + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); > + struct p2m_domain *ap2m = NULL; > + struct p2m_domain *p2m = NULL; Pointless initializer. > + mfn_t mfn; > + p2m_access_t a; > + p2m_type_t t; > + unsigned long gfn_l; Please avoid this local variable and use gfn_x() in the two places you need to. > + int rc = 0; Pointless initializer again. > + > + if ( !cpu_has_vmx ) > + return -EOPNOTSUPP; Is this enough? Wouldn't it be better to signal the caller whenever hardware (or even software) isn't going to honor the request? > + if ( altp2m_idx > 0 ) > + { > + if ( altp2m_idx >= MAX_ALTP2M || > + d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) Indentation. > + return -EINVAL; > + > + p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx]; > + } > + else > + { > + p2m = host_p2m; > + } Unnecessary braces. > + p2m_lock(host_p2m); > + if ( ap2m ) > + p2m_lock(ap2m); > + > + gfn_l = gfn_x(gfn); > + mfn = p2m->get_entry(p2m, gfn_l, &t, &a, 0, NULL, NULL); > + if ( !mfn_valid(mfn) ) > + return -ESRCH; > + rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, > + suppress_ve); > + if ( ap2m ) > + p2m_unlock(ap2m); > + p2m_unlock(host_p2m); To fiddle with a single gfn, this looks to be very heavy locking. While for now gfn_lock() is the same as p2m_lock(), from an abstract perspective I'd expect gfn_lock() to suffice here at least in the non-altp2m case. And then there are two general questions: Without a libxc layer function, how is one supposed to use this new sub-op? Is it really intended to permit a guest to call this for itself? Jan
Hello, On Mon, May 29, 2017 at 08:38:33AM -0600, Jan Beulich wrote: > >>> On 18.05.17 at 17:07, <apop@bitdefender.com> wrote: > > --- a/xen/arch/x86/mm/mem_access.c > > +++ b/xen/arch/x86/mm/mem_access.c > > @@ -466,6 +466,54 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access) > > } > > > > /* > > + * Set/clear the #VE suppress bit for a page. Only available on VMX. > > + */ > > +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve, > > suppress_ve presumably is meant to be boolean. Yes. It can be changed to bool. > > + unsigned int altp2m_idx) > > +{ > > + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); > > + struct p2m_domain *ap2m = NULL; > > + struct p2m_domain *p2m = NULL; > > Pointless initializer. Ok. > > + mfn_t mfn; > > + p2m_access_t a; > > + p2m_type_t t; > > + unsigned long gfn_l; > > Please avoid this local variable and use gfn_x() in the two places > you need to. Sure. > > + int rc = 0; > > Pointless initializer again. Right. > > + > > + if ( !cpu_has_vmx ) > > + return -EOPNOTSUPP; > > Is this enough? Wouldn't it be better to signal the caller whenever > hardware (or even software) isn't going to honor the request? Well, the caller checks the return value. The libxc function xc_altp2m_set_suppress_ve for instance will return a negative in this case. > > + if ( altp2m_idx > 0 ) > > + { > > + if ( altp2m_idx >= MAX_ALTP2M || > > + d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) > > Indentation. Ok. > > + return -EINVAL; > > + > > + p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx]; > > + } > > + else > > + { > > + p2m = host_p2m; > > + } > > Unnecessary braces. Ok. > > + p2m_lock(host_p2m); > > + if ( ap2m ) > > + p2m_lock(ap2m); > > + > > + gfn_l = gfn_x(gfn); > > + mfn = p2m->get_entry(p2m, gfn_l, &t, &a, 0, NULL, NULL); > > + if ( !mfn_valid(mfn) ) > > + return -ESRCH; > > + rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, > > + suppress_ve); > > + if ( ap2m ) > > + p2m_unlock(ap2m); > > + p2m_unlock(host_p2m); > > To fiddle with a single gfn, this looks to be very heavy locking. > While for now gfn_lock() is the same as p2m_lock(), from an > abstract perspective I'd expect gfn_lock() to suffice here at > least in the non-altp2m case. Ok. > And then there are two general questions: Without a libxc layer > function, how is one supposed to use this new sub-op? Is it > really intended to permit a guest to call this for itself? Well, the sub-op could be used from a Linux kernel module if libxc is not available if struct xen_hvm_altp2m_op and struct xen_hvm_altp2m_set_suppress_ve are defined. Our use case, though, involves either Dom0 or a "privileged" DomU altering the suppress #VE bit for the target guest. > Jan > Thanks!
>>> On 06.06.17 at 15:00, <apop@bitdefender.com> wrote: > On Mon, May 29, 2017 at 08:38:33AM -0600, Jan Beulich wrote: >> >>> On 18.05.17 at 17:07, <apop@bitdefender.com> wrote: >> > + >> > + if ( !cpu_has_vmx ) >> > + return -EOPNOTSUPP; >> >> Is this enough? Wouldn't it be better to signal the caller whenever >> hardware (or even software) isn't going to honor the request? > > Well, the caller checks the return value. The libxc function > xc_altp2m_set_suppress_ve for instance will return a negative in this > case. The question wasn't what the caller does but whether checking just cpu_has_vmx is enough. What if you're running on a machine with VMX but no #VE support? >> And then there are two general questions: Without a libxc layer >> function, how is one supposed to use this new sub-op? Is it >> really intended to permit a guest to call this for itself? > > Well, the sub-op could be used from a Linux kernel module if libxc is > not available if struct xen_hvm_altp2m_op and struct > xen_hvm_altp2m_set_suppress_ve are defined. > > Our use case, though, involves either Dom0 or a "privileged" DomU > altering the suppress #VE bit for the target guest. This doesn't really answer the question: What are the security implications if a guest can invoke this on itself? (FTR I think my first question was kind of pointless, as patch 3 looks like it does introduce a libxc function; I simply didn't realize that back then, because I'd generally have expected the consumer of the hypercall to be introduce together with the producer.) Jan
On Tue, Jun 06, 2017 at 07:08:43AM -0600, Jan Beulich wrote: > >>> On 06.06.17 at 15:00, <apop@bitdefender.com> wrote: > > On Mon, May 29, 2017 at 08:38:33AM -0600, Jan Beulich wrote: > >> >>> On 18.05.17 at 17:07, <apop@bitdefender.com> wrote: > >> > + > >> > + if ( !cpu_has_vmx ) > >> > + return -EOPNOTSUPP; > >> > >> Is this enough? Wouldn't it be better to signal the caller whenever > >> hardware (or even software) isn't going to honor the request? > > > > Well, the caller checks the return value. The libxc function > > xc_altp2m_set_suppress_ve for instance will return a negative in this > > case. > > The question wasn't what the caller does but whether checking just > cpu_has_vmx is enough. What if you're running on a machine with > VMX but no #VE support? Oh, all right. I misinterpreted it. Yes, at least using something like cpu_has_vmx_virt_exceptions instead of cpu_has_vmx would definitely be more appropriate in this case. Do you think there should be a more thorough check? > >> And then there are two general questions: Without a libxc layer > >> function, how is one supposed to use this new sub-op? Is it > >> really intended to permit a guest to call this for itself? > > > > Well, the sub-op could be used from a Linux kernel module if libxc is > > not available if struct xen_hvm_altp2m_op and struct > > xen_hvm_altp2m_set_suppress_ve are defined. > > > > Our use case, though, involves either Dom0 or a "privileged" DomU > > altering the suppress #VE bit for the target guest. > > This doesn't really answer the question: What are the security > implications if a guest can invoke this on itself? Indeed it would be desirable that the guest doesn't use this hvmop on itself. It's also less than ideal that a DomU can call this on other DomUs. After some talks it turns out that restricting this hvmop to a privileged domain solves this issue and still works for our use case. What do you think? > (FTR I think my first question was kind of pointless, as patch 3 > looks like it does introduce a libxc function; I simply didn't realize > that back then, because I'd generally have expected the > consumer of the hypercall to be introduce together with the > producer.) I can merge these two patches for v2 if that's what you want. > Jan Thank you!
>>> On 08.06.17 at 15:49, <apop@bitdefender.com> wrote: > On Tue, Jun 06, 2017 at 07:08:43AM -0600, Jan Beulich wrote: >> >>> On 06.06.17 at 15:00, <apop@bitdefender.com> wrote: >> > On Mon, May 29, 2017 at 08:38:33AM -0600, Jan Beulich wrote: >> >> >>> On 18.05.17 at 17:07, <apop@bitdefender.com> wrote: >> >> > + >> >> > + if ( !cpu_has_vmx ) >> >> > + return -EOPNOTSUPP; >> >> >> >> Is this enough? Wouldn't it be better to signal the caller whenever >> >> hardware (or even software) isn't going to honor the request? >> > >> > Well, the caller checks the return value. The libxc function >> > xc_altp2m_set_suppress_ve for instance will return a negative in this >> > case. >> >> The question wasn't what the caller does but whether checking just >> cpu_has_vmx is enough. What if you're running on a machine with >> VMX but no #VE support? > > Oh, all right. I misinterpreted it. Yes, at least using something like > cpu_has_vmx_virt_exceptions instead of cpu_has_vmx would definitely be > more appropriate in this case. Do you think there should be a more > thorough check? Depends on what "more thorough" means: You'll want to check all features the code requires; I'm not certain if virt_exceptions is all it needs. >> >> And then there are two general questions: Without a libxc layer >> >> function, how is one supposed to use this new sub-op? Is it >> >> really intended to permit a guest to call this for itself? >> > >> > Well, the sub-op could be used from a Linux kernel module if libxc is >> > not available if struct xen_hvm_altp2m_op and struct >> > xen_hvm_altp2m_set_suppress_ve are defined. >> > >> > Our use case, though, involves either Dom0 or a "privileged" DomU >> > altering the suppress #VE bit for the target guest. >> >> This doesn't really answer the question: What are the security >> implications if a guest can invoke this on itself? > > Indeed it would be desirable that the guest doesn't use this hvmop on > itself. It's also less than ideal that a DomU can call this on other > DomUs. The latter is an absolute no-go. > After some talks it turns out that restricting this hvmop to a > privileged domain solves this issue and still works for our use case. > What do you think? Restrictions should generally be put in place because of abstract considerations, not because of them not harming one's particular use case. >> (FTR I think my first question was kind of pointless, as patch 3 >> looks like it does introduce a libxc function; I simply didn't realize >> that back then, because I'd generally have expected the >> consumer of the hypercall to be introduce together with the >> producer.) > > I can merge these two patches for v2 if that's what you want. I'd prefer that, but others may have differing opinions. And there are certainly benefits in keeping hypervisor and tools changes separate. Jan
On Thu, Jun 08, 2017 at 08:08:56AM -0600, Jan Beulich wrote: > >>> On 08.06.17 at 15:49, <apop@bitdefender.com> wrote: > > On Tue, Jun 06, 2017 at 07:08:43AM -0600, Jan Beulich wrote: > >> >>> On 06.06.17 at 15:00, <apop@bitdefender.com> wrote: > >> > On Mon, May 29, 2017 at 08:38:33AM -0600, Jan Beulich wrote: > >> >> >>> On 18.05.17 at 17:07, <apop@bitdefender.com> wrote: > >> >> > + > >> >> > + if ( !cpu_has_vmx ) > >> >> > + return -EOPNOTSUPP; > >> >> > >> >> Is this enough? Wouldn't it be better to signal the caller whenever > >> >> hardware (or even software) isn't going to honor the request? > >> > > >> > Well, the caller checks the return value. The libxc function > >> > xc_altp2m_set_suppress_ve for instance will return a negative in this > >> > case. > >> > >> The question wasn't what the caller does but whether checking just > >> cpu_has_vmx is enough. What if you're running on a machine with > >> VMX but no #VE support? > > > > Oh, all right. I misinterpreted it. Yes, at least using something like > > cpu_has_vmx_virt_exceptions instead of cpu_has_vmx would definitely be > > more appropriate in this case. Do you think there should be a more > > thorough check? > > Depends on what "more thorough" means: You'll want to check all > features the code requires; I'm not certain if virt_exceptions is all > it needs. The checks so far would be: - is the domain invoking this hvmop privileged? - does the cpu have the #VE feature? - is #VE enabled on this vcpu? > >> >> And then there are two general questions: Without a libxc layer > >> >> function, how is one supposed to use this new sub-op? Is it > >> >> really intended to permit a guest to call this for itself? > >> > > >> > Well, the sub-op could be used from a Linux kernel module if libxc is > >> > not available if struct xen_hvm_altp2m_op and struct > >> > xen_hvm_altp2m_set_suppress_ve are defined. > >> > > >> > Our use case, though, involves either Dom0 or a "privileged" DomU > >> > altering the suppress #VE bit for the target guest. > >> > >> This doesn't really answer the question: What are the security > >> implications if a guest can invoke this on itself? > > > > Indeed it would be desirable that the guest doesn't use this hvmop on > > itself. It's also less than ideal that a DomU can call this on other > > DomUs. > > The latter is an absolute no-go. Indeed. > > After some talks it turns out that restricting this hvmop to a > > privileged domain solves this issue and still works for our use case. > > What do you think? > > Restrictions should generally be put in place because of > abstract considerations, not because of them not harming > one's particular use case. Of course. > >> (FTR I think my first question was kind of pointless, as patch 3 > >> looks like it does introduce a libxc function; I simply didn't realize > >> that back then, because I'd generally have expected the > >> consumer of the hypercall to be introduce together with the > >> producer.) > > > > I can merge these two patches for v2 if that's what you want. > > I'd prefer that, but others may have differing opinions. And > there are certainly benefits in keeping hypervisor and tools > changes separate. Ok then.
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 2e76c2345b..eb01527c5b 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4356,6 +4356,7 @@ static int do_altp2m_op( case HVMOP_altp2m_destroy_p2m: case HVMOP_altp2m_switch_p2m: case HVMOP_altp2m_set_mem_access: + case HVMOP_altp2m_set_suppress_ve: case HVMOP_altp2m_change_gfn: break; default: @@ -4472,6 +4473,19 @@ static int do_altp2m_op( a.u.set_mem_access.view); break; + case HVMOP_altp2m_set_suppress_ve: + if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 ) + rc = -EINVAL; + else + { + gfn_t gfn = _gfn(a.u.set_mem_access.gfn); + unsigned int altp2m_idx = a.u.set_mem_access.view; + uint8_t suppress_ve = a.u.set_suppress_ve.suppress_ve; + + rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx); + } + break; + case HVMOP_altp2m_change_gfn: if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 ) rc = -EINVAL; diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c index d0b0767855..b9e611d3db 100644 --- a/xen/arch/x86/mm/mem_access.c +++ b/xen/arch/x86/mm/mem_access.c @@ -466,6 +466,54 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access) } /* + * Set/clear the #VE suppress bit for a page. Only available on VMX. + */ +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve, + unsigned int altp2m_idx) +{ + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); + struct p2m_domain *ap2m = NULL; + struct p2m_domain *p2m = NULL; + mfn_t mfn; + p2m_access_t a; + p2m_type_t t; + unsigned long gfn_l; + int rc = 0; + + if ( !cpu_has_vmx ) + return -EOPNOTSUPP; + + if ( altp2m_idx > 0 ) + { + if ( altp2m_idx >= MAX_ALTP2M || + d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) + return -EINVAL; + + p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx]; + } + else + { + p2m = host_p2m; + } + + p2m_lock(host_p2m); + if ( ap2m ) + p2m_lock(ap2m); + + gfn_l = gfn_x(gfn); + mfn = p2m->get_entry(p2m, gfn_l, &t, &a, 0, NULL, NULL); + if ( !mfn_valid(mfn) ) + return -ESRCH; + rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, + suppress_ve); + if ( ap2m ) + p2m_unlock(ap2m); + p2m_unlock(host_p2m); + + return rc; +} + +/* * Local variables: * mode: C * c-file-style: "BSD" diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h index bc00ef0e65..9736092f58 100644 --- a/xen/include/public/hvm/hvm_op.h +++ b/xen/include/public/hvm/hvm_op.h @@ -231,6 +231,18 @@ struct xen_hvm_altp2m_set_mem_access { typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t; DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t); +struct xen_hvm_altp2m_set_suppress_ve { + /* view */ + uint16_t view; + uint8_t suppress_ve; + uint8_t pad1; + uint32_t pad2; + /* gfn */ + uint64_t gfn; +}; +typedef struct xen_hvm_altp2m_set_suppress_ve xen_hvm_altp2m_set_suppress_ve_t; +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_suppress_ve_t); + struct xen_hvm_altp2m_change_gfn { /* view */ uint16_t view; @@ -262,6 +274,8 @@ struct xen_hvm_altp2m_op { #define HVMOP_altp2m_set_mem_access 7 /* Change a p2m entry to have a different gfn->mfn mapping */ #define HVMOP_altp2m_change_gfn 8 +/* Set the "Suppress #VE" bit on a page */ +#define HVMOP_altp2m_set_suppress_ve 9 domid_t domain; uint16_t pad1; uint32_t pad2; @@ -270,6 +284,7 @@ struct xen_hvm_altp2m_op { struct xen_hvm_altp2m_vcpu_enable_notify enable_notify; struct xen_hvm_altp2m_view view; struct xen_hvm_altp2m_set_mem_access set_mem_access; + struct xen_hvm_altp2m_set_suppress_ve set_suppress_ve; struct xen_hvm_altp2m_change_gfn change_gfn; uint8_t pad[64]; } u; diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h index 5ab34c1553..b6e6a7650a 100644 --- a/xen/include/xen/mem_access.h +++ b/xen/include/xen/mem_access.h @@ -78,6 +78,9 @@ long p2m_set_mem_access_multi(struct domain *d, */ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access); +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve, + unsigned int altp2m_idx); + #ifdef CONFIG_HAS_MEM_ACCESS int mem_access_memop(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg);
Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a domain to change the value of the #VE suppress bit for a page. Signed-off-by: Adrian Pop <apop@bitdefender.com> --- xen/arch/x86/hvm/hvm.c | 14 ++++++++++++ xen/arch/x86/mm/mem_access.c | 48 +++++++++++++++++++++++++++++++++++++++++ xen/include/public/hvm/hvm_op.h | 15 +++++++++++++ xen/include/xen/mem_access.h | 3 +++ 4 files changed, 80 insertions(+)