| Message ID | 1452531079-4483-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 11, 2016 at 04:51:19PM +0000, Andrew Cooper wrote: > Currently, hypercalls issued from HVM userspace will unconditionally fail with > -EPERM. > > This is inflexible, and a guest may wish to allow userspace to make > hypercalls. > > Introduce HVMOP_set_hypercall_dpl which allows the guest to alter the > permissions check for hypercalls. It behaves exactly like the dpl field for > GDT/LDT/IDT entries. Could you explain a bit of the use-case? As in why the ioctl via the kernel is no good? > > As the dpl is initialised to 0, hypercalls are restricted to cpl0 code until > the OS explicitly chooses an alternative. <scratchis his head> So we enable to make hypercalls but then we don't allow it unless it is in ring 0? > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > -- > CC: Jan Beulich <JBeulich@suse.com> > CC: Ian Campbell <ian.campbell@citrix.com> > CC: Stefano Stabellini <stefano.stabellini@citrix.com> > > v2: > * Fix rcu lock and dpl check. > * Use uint8_t for hypercall_dpl and reposition for better packing. > > The test framework (soon to be published officially) how has both positive and > negative tests to confirm the correct behaviour of this hypercall. > > Arm folks: Is something like this sufficiently generic to be useful on Arm, > perhaps with more generic naming? > > PV guest support for userspace hypercalls is substantially more involved, and > will take longer to complete. > --- > xen/arch/x86/hvm/hvm.c | 28 +++++++++++++++++++++++++++- > xen/include/asm-x86/hvm/domain.h | 2 ++ > xen/include/public/hvm/hvm_op.h | 8 ++++++++ > 3 files changed, 37 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 21470ec..5f3be6b 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -5228,7 +5228,8 @@ int hvm_do_hypercall(struct cpu_user_regs *regs) > case 4: > case 2: > hvm_get_segment_register(curr, x86_seg_ss, &sreg); > - if ( unlikely(sreg.attr.fields.dpl) ) > + if ( unlikely(sreg.attr.fields.dpl > > + currd->arch.hvm_domain.hypercall_dpl) ) > { > default: > regs->eax = -EPERM; > @@ -6839,6 +6840,31 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) > rc = do_altp2m_op(arg); > break; > > + case HVMOP_set_hypercall_dpl: > + { > + xen_hvm_hypercall_dpl_t a; > + struct domain *d; > + > + if ( copy_from_guest(&a, arg, 1 ) ) > + return -EFAULT; > + > + d = rcu_lock_domain_by_any_id(a.domid); > + if ( d == NULL ) > + return -ESRCH; > + > + if ( current->domain != d ) > + return -EPERM; > + > + if ( !is_hvm_domain(d) ) > + return -EINVAL; > + > + if ( a.dpl > 3 ) > + return -EDOM; > + > + d->arch.hvm_domain.hypercall_dpl = a.dpl; > + break; > + } > + > default: > { > gdprintk(XENLOG_DEBUG, "Bad HVM op %ld.\n", op); > diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h > index a8cc2ad..ac426ce 100644 > --- a/xen/include/asm-x86/hvm/domain.h > +++ b/xen/include/asm-x86/hvm/domain.h > @@ -123,6 +123,8 @@ struct hvm_domain { > spinlock_t uc_lock; > bool_t is_in_uc_mode; > > + uint8_t hypercall_dpl; > + > /* Pass-through */ > struct hvm_iommu hvm_iommu; > > diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h > index 1606185..f8247db 100644 > --- a/xen/include/public/hvm/hvm_op.h > +++ b/xen/include/public/hvm/hvm_op.h > @@ -489,6 +489,14 @@ struct xen_hvm_altp2m_op { > typedef struct xen_hvm_altp2m_op xen_hvm_altp2m_op_t; > DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_op_t); > > +#define HVMOP_set_hypercall_dpl 26 > +struct xen_hvm_hypercall_dpl { > + domid_t domid; > + uint16_t dpl; /* IN[1:0] cpl required to make hypercalls. */ > +}; > +typedef struct xen_hvm_hypercall_dpl xen_hvm_hypercall_dpl_t; > +DEFINE_XEN_GUEST_HANDLE(xen_hvm_hypercall_dpl_t); > + > #endif /* __XEN_PUBLIC_HVM_HVM_OP_H__ */ > > /* > -- > 2.1.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On 11/01/16 17:11, Konrad Rzeszutek Wilk wrote: > On Mon, Jan 11, 2016 at 04:51:19PM +0000, Andrew Cooper wrote: >> Currently, hypercalls issued from HVM userspace will unconditionally fail with >> -EPERM. >> >> This is inflexible, and a guest may wish to allow userspace to make >> hypercalls. >> >> Introduce HVMOP_set_hypercall_dpl which allows the guest to alter the >> permissions check for hypercalls. It behaves exactly like the dpl field for >> GDT/LDT/IDT entries. > > Could you explain a bit of the use-case? My specific usecase, http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen-test-framework.git;a=shortlog;h=refs/heads/wip-traps-v0.1 It isn't quite ready for formal release yet. > As in why the ioctl via the kernel is no good? Who says Linux is running? Hopefully answered in http://lists.xenproject.org/archives/html/xen-devel/2016-01/msg01155.html > >> As the dpl is initialised to 0, hypercalls are restricted to cpl0 code until >> the OS explicitly chooses an alternative. > <scratchis his head> So we enable to make hypercalls but then we don't allow > it unless it is in ring 0? Correct. Hypercalls are by default limited to cpl0 (i.e. the existing behaviour), but guests can use this new hypercall to change the permission check. Naturally, you have to be sufficiently privileged to make this hypercall in the first place, so only the kernel may opt to relax the check. ~Andrew
On Mon, Jan 11, 2016 at 05:58:47PM +0000, Andrew Cooper wrote: > On 11/01/16 17:11, Konrad Rzeszutek Wilk wrote: > > On Mon, Jan 11, 2016 at 04:51:19PM +0000, Andrew Cooper wrote: > >> Currently, hypercalls issued from HVM userspace will unconditionally fail with > >> -EPERM. > >> > >> This is inflexible, and a guest may wish to allow userspace to make > >> hypercalls. > >> > >> Introduce HVMOP_set_hypercall_dpl which allows the guest to alter the > >> permissions check for hypercalls. It behaves exactly like the dpl field for > >> GDT/LDT/IDT entries. > > > > Could you explain a bit of the use-case? > > My specific usecase, > http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen-test-framework.git;a=shortlog;h=refs/heads/wip-traps-v0.1 > > It isn't quite ready for formal release yet. > > > As in why the ioctl via the kernel is no good? > > Who says Linux is running? What else would there be :-) > > Hopefully answered in > http://lists.xenproject.org/archives/html/xen-devel/2016-01/msg01155.html Yes. If you could add it in the commit description that would be most helpful. Thank you! > > > > >> As the dpl is initialised to 0, hypercalls are restricted to cpl0 code until > >> the OS explicitly chooses an alternative. > > <scratchis his head> So we enable to make hypercalls but then we don't allow > > it unless it is in ring 0? > > Correct. Hypercalls are by default limited to cpl0 (i.e. the existing > behaviour), but guests can use this new hypercall to change the > permission check. > > Naturally, you have to be sufficiently privileged to make this hypercall > in the first place, so only the kernel may opt to relax the check. Right. Sorry I somehow had in mind that this hypercall would be made by the toolstack which is why I was confused. > > ~Andrew
>>> On 11.01.16 at 17:51, <andrew.cooper3@citrix.com> wrote: > Currently, hypercalls issued from HVM userspace will unconditionally fail > with > -EPERM. > > This is inflexible, and a guest may wish to allow userspace to make > hypercalls. > > Introduce HVMOP_set_hypercall_dpl which allows the guest to alter the > permissions check for hypercalls. It behaves exactly like the dpl field for > GDT/LDT/IDT entries. > > As the dpl is initialised to 0, hypercalls are restricted to cpl0 code until > the OS explicitly chooses an alternative. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > -- > CC: Jan Beulich <JBeulich@suse.com> > CC: Ian Campbell <ian.campbell@citrix.com> > CC: Stefano Stabellini <stefano.stabellini@citrix.com> > > v2: > * Fix rcu lock and dpl check. That's a bold statement considering ... > @@ -6839,6 +6840,31 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) > rc = do_altp2m_op(arg); > break; > > + case HVMOP_set_hypercall_dpl: > + { > + xen_hvm_hypercall_dpl_t a; > + struct domain *d; > + > + if ( copy_from_guest(&a, arg, 1 ) ) > + return -EFAULT; > + > + d = rcu_lock_domain_by_any_id(a.domid); > + if ( d == NULL ) > + return -ESRCH; > + > + if ( current->domain != d ) > + return -EPERM; > + > + if ( !is_hvm_domain(d) ) > + return -EINVAL; > + > + if ( a.dpl > 3 ) > + return -EDOM; > + > + d->arch.hvm_domain.hypercall_dpl = a.dpl; > + break; > + } ... there's no unlock anywhere here. Jan
On Mon, Jan 11, 2016 at 5:58 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 11/01/16 17:11, Konrad Rzeszutek Wilk wrote: >> On Mon, Jan 11, 2016 at 04:51:19PM +0000, Andrew Cooper wrote: >>> Currently, hypercalls issued from HVM userspace will unconditionally fail with >>> -EPERM. >>> >>> This is inflexible, and a guest may wish to allow userspace to make >>> hypercalls. >>> >>> Introduce HVMOP_set_hypercall_dpl which allows the guest to alter the >>> permissions check for hypercalls. It behaves exactly like the dpl field for >>> GDT/LDT/IDT entries. >> >> Could you explain a bit of the use-case? > > My specific usecase, > http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen-test-framework.git;a=shortlog;h=refs/heads/wip-traps-v0.1 > > It isn't quite ready for formal release yet. > >> As in why the ioctl via the kernel is no good? > > Who says Linux is running? > > Hopefully answered in > http://lists.xenproject.org/archives/html/xen-devel/2016-01/msg01155.html Not really. Obviously if you're running custom test code rather than Linux, then you aren't going to make an ioctl system call on a file descriptor; but what people are actually suggesting is just that you make *some* sort of system call from ring 3 which will then make the hypercall from ring 0. That's not "the Linux way" of doing things, it's the *operating system* way of doing things. From the previous discussion, ISTR that what you want to be able to log messages to the Xen console from your test code when running in ring 3. It should be fairly easy to set up a custom system call in your test system that will then make the appropriate hypercall from ring 0 and return, with minimal interaction with other parts of the system. (I think there were some other suggestions there as well.) Is there a reason that's not possible? -George
On Mon, 11 Jan 2016, Andrew Cooper wrote: > Currently, hypercalls issued from HVM userspace will unconditionally fail with > -EPERM. > > This is inflexible, and a guest may wish to allow userspace to make > hypercalls. > > Introduce HVMOP_set_hypercall_dpl which allows the guest to alter the > permissions check for hypercalls. It behaves exactly like the dpl field for > GDT/LDT/IDT entries. > > As the dpl is initialised to 0, hypercalls are restricted to cpl0 code until > the OS explicitly chooses an alternative. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > -- > CC: Jan Beulich <JBeulich@suse.com> > CC: Ian Campbell <ian.campbell@citrix.com> > CC: Stefano Stabellini <stefano.stabellini@citrix.com> > > v2: > * Fix rcu lock and dpl check. > * Use uint8_t for hypercall_dpl and reposition for better packing. > > The test framework (soon to be published officially) how has both positive and > negative tests to confirm the correct behaviour of this hypercall. > > Arm folks: Is something like this sufficiently generic to be useful on Arm, > perhaps with more generic naming? Hypercalls on ARM are made issuing an HVC instruction which is "UNDEFINED in Secure state, and in User mode in Non-secure state". In other words, it cannot work. > PV guest support for userspace hypercalls is substantially more involved, and > will take longer to complete. > --- > xen/arch/x86/hvm/hvm.c | 28 +++++++++++++++++++++++++++- > xen/include/asm-x86/hvm/domain.h | 2 ++ > xen/include/public/hvm/hvm_op.h | 8 ++++++++ > 3 files changed, 37 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 21470ec..5f3be6b 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -5228,7 +5228,8 @@ int hvm_do_hypercall(struct cpu_user_regs *regs) > case 4: > case 2: > hvm_get_segment_register(curr, x86_seg_ss, &sreg); > - if ( unlikely(sreg.attr.fields.dpl) ) > + if ( unlikely(sreg.attr.fields.dpl > > + currd->arch.hvm_domain.hypercall_dpl) ) > { > default: > regs->eax = -EPERM; > @@ -6839,6 +6840,31 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) > rc = do_altp2m_op(arg); > break; > > + case HVMOP_set_hypercall_dpl: > + { > + xen_hvm_hypercall_dpl_t a; > + struct domain *d; > + > + if ( copy_from_guest(&a, arg, 1 ) ) > + return -EFAULT; > + > + d = rcu_lock_domain_by_any_id(a.domid); > + if ( d == NULL ) > + return -ESRCH; > + > + if ( current->domain != d ) > + return -EPERM; > + > + if ( !is_hvm_domain(d) ) > + return -EINVAL; > + > + if ( a.dpl > 3 ) > + return -EDOM; > + > + d->arch.hvm_domain.hypercall_dpl = a.dpl; > + break; > + } > + > default: > { > gdprintk(XENLOG_DEBUG, "Bad HVM op %ld.\n", op); > diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h > index a8cc2ad..ac426ce 100644 > --- a/xen/include/asm-x86/hvm/domain.h > +++ b/xen/include/asm-x86/hvm/domain.h > @@ -123,6 +123,8 @@ struct hvm_domain { > spinlock_t uc_lock; > bool_t is_in_uc_mode; > > + uint8_t hypercall_dpl; > + > /* Pass-through */ > struct hvm_iommu hvm_iommu; > > diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h > index 1606185..f8247db 100644 > --- a/xen/include/public/hvm/hvm_op.h > +++ b/xen/include/public/hvm/hvm_op.h > @@ -489,6 +489,14 @@ struct xen_hvm_altp2m_op { > typedef struct xen_hvm_altp2m_op xen_hvm_altp2m_op_t; > DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_op_t); > > +#define HVMOP_set_hypercall_dpl 26 > +struct xen_hvm_hypercall_dpl { > + domid_t domid; > + uint16_t dpl; /* IN[1:0] cpl required to make hypercalls. */ > +}; > +typedef struct xen_hvm_hypercall_dpl xen_hvm_hypercall_dpl_t; > +DEFINE_XEN_GUEST_HANDLE(xen_hvm_hypercall_dpl_t); > + > #endif /* __XEN_PUBLIC_HVM_HVM_OP_H__ */ > > /* > -- > 2.1.4 >
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 21470ec..5f3be6b 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -5228,7 +5228,8 @@ int hvm_do_hypercall(struct cpu_user_regs *regs) case 4: case 2: hvm_get_segment_register(curr, x86_seg_ss, &sreg); - if ( unlikely(sreg.attr.fields.dpl) ) + if ( unlikely(sreg.attr.fields.dpl > + currd->arch.hvm_domain.hypercall_dpl) ) { default: regs->eax = -EPERM; @@ -6839,6 +6840,31 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) rc = do_altp2m_op(arg); break; + case HVMOP_set_hypercall_dpl: + { + xen_hvm_hypercall_dpl_t a; + struct domain *d; + + if ( copy_from_guest(&a, arg, 1 ) ) + return -EFAULT; + + d = rcu_lock_domain_by_any_id(a.domid); + if ( d == NULL ) + return -ESRCH; + + if ( current->domain != d ) + return -EPERM; + + if ( !is_hvm_domain(d) ) + return -EINVAL; + + if ( a.dpl > 3 ) + return -EDOM; + + d->arch.hvm_domain.hypercall_dpl = a.dpl; + break; + } + default: { gdprintk(XENLOG_DEBUG, "Bad HVM op %ld.\n", op); diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h index a8cc2ad..ac426ce 100644 --- a/xen/include/asm-x86/hvm/domain.h +++ b/xen/include/asm-x86/hvm/domain.h @@ -123,6 +123,8 @@ struct hvm_domain { spinlock_t uc_lock; bool_t is_in_uc_mode; + uint8_t hypercall_dpl; + /* Pass-through */ struct hvm_iommu hvm_iommu; diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h index 1606185..f8247db 100644 --- a/xen/include/public/hvm/hvm_op.h +++ b/xen/include/public/hvm/hvm_op.h @@ -489,6 +489,14 @@ struct xen_hvm_altp2m_op { typedef struct xen_hvm_altp2m_op xen_hvm_altp2m_op_t; DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_op_t); +#define HVMOP_set_hypercall_dpl 26 +struct xen_hvm_hypercall_dpl { + domid_t domid; + uint16_t dpl; /* IN[1:0] cpl required to make hypercalls. */ +}; +typedef struct xen_hvm_hypercall_dpl xen_hvm_hypercall_dpl_t; +DEFINE_XEN_GUEST_HANDLE(xen_hvm_hypercall_dpl_t); + #endif /* __XEN_PUBLIC_HVM_HVM_OP_H__ */ /*
Currently, hypercalls issued from HVM userspace will unconditionally fail with -EPERM. This is inflexible, and a guest may wish to allow userspace to make hypercalls. Introduce HVMOP_set_hypercall_dpl which allows the guest to alter the permissions check for hypercalls. It behaves exactly like the dpl field for GDT/LDT/IDT entries. As the dpl is initialised to 0, hypercalls are restricted to cpl0 code until the OS explicitly chooses an alternative. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> -- CC: Jan Beulich <JBeulich@suse.com> CC: Ian Campbell <ian.campbell@citrix.com> CC: Stefano Stabellini <stefano.stabellini@citrix.com> v2: * Fix rcu lock and dpl check. * Use uint8_t for hypercall_dpl and reposition for better packing. The test framework (soon to be published officially) how has both positive and negative tests to confirm the correct behaviour of this hypercall. Arm folks: Is something like this sufficiently generic to be useful on Arm, perhaps with more generic naming? PV guest support for userspace hypercalls is substantially more involved, and will take longer to complete. --- xen/arch/x86/hvm/hvm.c | 28 +++++++++++++++++++++++++++- xen/include/asm-x86/hvm/domain.h | 2 ++ xen/include/public/hvm/hvm_op.h | 8 ++++++++ 3 files changed, 37 insertions(+), 1 deletion(-)