Xen project Mailing List

Re: [Xen-devel] [PATCH, v2] linux-2.6.18: fix hypercall fallback code for very old hypervisors

From: Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx>

Date: Fri, 19 Oct 2012 15:23:01 -0400

Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>, Dan Carpenter <dan.carpenter@xxxxxxxxxx>, xen-devel <xen-devel@xxxxxxxxxxxxx>

Delivery-date: Fri, 19 Oct 2012 19:35:31 +0000

List-id: Xen developer discussion <xen-devel.lists.xen.org>

On Tue, Oct 16, 2012 at 02:46:21PM +0100, Jan Beulich wrote: > While copying the argument structures in HYPERVISOR_event_channel_op() > and HYPERVISOR_physdev_op() into the local variable is sufficiently > safe even if the actual structure is smaller than the container one, > copying back eventual output values the same way isn't: This may > collide with on-stack variables (particularly "rc") which may change > between the first and second memcpy() (i.e. the second memcpy() could > discard that change). > > Move the fallback code into out-of-line functions, and handle all of > the operations known by this old a hypervisor individually: Some don't > require copying back anything at all, and for the rest use the > individual argument structures' sizes rather than the container's. > > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> > --- > v2: Reduce #define/#undef usage in HYPERVISOR_physdev_op_compat(). > > --- a/drivers/xen/core/Makefile > +++ b/drivers/xen/core/Makefile > @@ -2,7 +2,7 @@ > # Makefile for the linux kernel. > # > > -obj-y := evtchn.o gnttab.o features.o reboot.o machine_reboot.o > +obj-y := evtchn.o gnttab.o features.o reboot.o machine_reboot.o fallback.o > > obj-$(CONFIG_PCI) += pci.o > obj-$(CONFIG_XEN_PRIVILEGED_GUEST) += firmware.o > --- /dev/null > +++ b/drivers/xen/core/fallback.c > @@ -0,0 +1,84 @@ > +#include <linux/kernel.h> > +#include <linux/string.h> > +#include <asm/bug.h> > +#include <asm/hypervisor.h> > + > +#if 1//temp CONFIG_XEN_COMPAT <= 0x030002 Um? > + > +#include <xen/interface/event_channel.h> > +#include <xen/interface/physdev.h> > + > +int HYPERVISOR_event_channel_op_compat(int cmd, void *arg) > +{ > + struct evtchn_op op; > + int rc; > + > + op.cmd = cmd; > + memcpy(&op.u, arg, sizeof(op.u)); > + rc = _hypercall1(int, event_channel_op_compat, &op); > + > + switch (cmd) { > + case EVTCHNOP_close: > + case EVTCHNOP_send: > + case EVTCHNOP_bind_vcpu: > + case EVTCHNOP_unmask: > + /* no output */ > + break; > + > +#define COPY_BACK(eop) \ > + case EVTCHNOP_##eop: \ > + memcpy(arg, &op.u.eop, sizeof(op.u.eop)); \ > + break > + > + COPY_BACK(bind_interdomain); > + COPY_BACK(bind_virq); > + COPY_BACK(bind_pirq); > + COPY_BACK(status); > + COPY_BACK(alloc_unbound); > + COPY_BACK(bind_ipi); > +#undef COPY_BACK > + > + default: > + WARN_ON(rc != -ENOSYS); > + break; > + } > + > + return rc; > +} > + > +int HYPERVISOR_physdev_op_compat(int cmd, void *arg) > +{ > + struct physdev_op op; > + int rc; > + > + op.cmd = cmd; > + memcpy(&op.u, arg, sizeof(op.u)); > + rc = _hypercall1(int, physdev_op_compat, &op); > + > + switch (cmd) { > + case PHYSDEVOP_IRQ_UNMASK_NOTIFY: > + case PHYSDEVOP_set_iopl: > + case PHYSDEVOP_set_iobitmap: > + case PHYSDEVOP_apic_write: > + /* no output */ > + break; > + > +#define COPY_BACK(pop, fld) \ > + case PHYSDEVOP_##pop: \ > + memcpy(arg, &op.u.fld, sizeof(op.u.fld)); \ > + break > + > + COPY_BACK(irq_status_query, irq_status_query); > + COPY_BACK(apic_read, apic_op); > + COPY_BACK(ASSIGN_VECTOR, irq_op); > +#undef COPY_BACK > + > + default: > + WARN_ON(rc != -ENOSYS); > + break; > + } > + > + return rc; > +} > + > +#endif /* CONFIG_XEN_COMPAT <= 0x030002 */ > --- a/include/asm-i386/mach-xen/asm/hypercall.h > +++ b/include/asm-i386/mach-xen/asm/hypercall.h > @@ -33,7 +33,6 @@ > #ifndef __HYPERCALL_H__ > #define __HYPERCALL_H__ > > -#include <linux/string.h> /* memcpy() */ > #include <linux/stringify.h> > > #ifndef __HYPERVISOR_H__ > @@ -128,6 +127,11 @@ > __res; \ > }) > > +#if CONFIG_XEN_COMPAT <= 0x030002 > +int __must_check HYPERVISOR_event_channel_op_compat(int, void *); > +int __must_check HYPERVISOR_physdev_op_compat(int, void *); > +#endif > + > static inline int __must_check > HYPERVISOR_set_trap_table( > const trap_info_t *table) > @@ -267,13 +271,8 @@ HYPERVISOR_event_channel_op( > int rc = _hypercall2(int, event_channel_op, cmd, arg); > > #if CONFIG_XEN_COMPAT <= 0x030002 > - if (unlikely(rc == -ENOSYS)) { > - struct evtchn_op op; > - op.cmd = cmd; > - memcpy(&op.u, arg, sizeof(op.u)); > - rc = _hypercall1(int, event_channel_op_compat, &op); > - memcpy(arg, &op.u, sizeof(op.u)); > - } > + if (unlikely(rc == -ENOSYS)) > + rc = HYPERVISOR_event_channel_op_compat(cmd, arg); > #endif > > return rc; > @@ -300,13 +299,8 @@ HYPERVISOR_physdev_op( > int rc = _hypercall2(int, physdev_op, cmd, arg); > > #if CONFIG_XEN_COMPAT <= 0x030002 > - if (unlikely(rc == -ENOSYS)) { > - struct physdev_op op; > - op.cmd = cmd; > - memcpy(&op.u, arg, sizeof(op.u)); > - rc = _hypercall1(int, physdev_op_compat, &op); > - memcpy(arg, &op.u, sizeof(op.u)); > - } > + if (unlikely(rc == -ENOSYS)) > + rc = HYPERVISOR_physdev_op_compat(cmd, arg); > #endif > > return rc; > --- a/include/asm-i386/mach-xen/asm/hypervisor.h > +++ b/include/asm-i386/mach-xen/asm/hypervisor.h > @@ -39,8 +39,6 @@ > #include <linux/errno.h> > #include <xen/interface/xen.h> > #include <xen/interface/platform.h> > -#include <xen/interface/event_channel.h> > -#include <xen/interface/physdev.h> > #include <xen/interface/sched.h> > #include <xen/interface/nmi.h> > #include <xen/interface/tmem.h> > > > fix hypercall fallback code for very old hypervisors > > While copying the argument structures in HYPERVISOR_event_channel_op() > and HYPERVISOR_physdev_op() into the local variable is sufficiently > safe even if the actual structure is smaller than the container one, > copying back eventual output values the same way isn't: This may > collide with on-stack variables (particularly "rc") which may change > between the first and second memcpy() (i.e. the second memcpy() could > discard that change). > > Move the fallback code into out-of-line functions, and handle all of > the operations known by this old a hypervisor individually: Some don't > require copying back anything at all, and for the rest use the > individual argument structures' sizes rather than the container's. > > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> > --- > v2: Reduce #define/#undef usage in HYPERVISOR_physdev_op_compat(). > > --- a/drivers/xen/core/Makefile > +++ b/drivers/xen/core/Makefile > @@ -2,7 +2,7 @@ > # Makefile for the linux kernel. > # > > -obj-y := evtchn.o gnttab.o features.o reboot.o machine_reboot.o > +obj-y := evtchn.o gnttab.o features.o reboot.o machine_reboot.o fallback.o > > obj-$(CONFIG_PCI) += pci.o > obj-$(CONFIG_XEN_PRIVILEGED_GUEST) += firmware.o > --- /dev/null > +++ b/drivers/xen/core/fallback.c > @@ -0,0 +1,84 @@ > +#include <linux/kernel.h> > +#include <linux/string.h> > +#include <asm/bug.h> > +#include <asm/hypervisor.h> > + > +#if 1//temp CONFIG_XEN_COMPAT <= 0x030002 > + > +#include <xen/interface/event_channel.h> > +#include <xen/interface/physdev.h> > + > +int HYPERVISOR_event_channel_op_compat(int cmd, void *arg) > +{ > + struct evtchn_op op; > + int rc; > + > + op.cmd = cmd; > + memcpy(&op.u, arg, sizeof(op.u)); > + rc = _hypercall1(int, event_channel_op_compat, &op); > + > + switch (cmd) { > + case EVTCHNOP_close: > + case EVTCHNOP_send: > + case EVTCHNOP_bind_vcpu: > + case EVTCHNOP_unmask: > + /* no output */ > + break; > + > +#define COPY_BACK(eop) \ > + case EVTCHNOP_##eop: \ > + memcpy(arg, &op.u.eop, sizeof(op.u.eop)); \ > + break > + > + COPY_BACK(bind_interdomain); > + COPY_BACK(bind_virq); > + COPY_BACK(bind_pirq); > + COPY_BACK(status); > + COPY_BACK(alloc_unbound); > + COPY_BACK(bind_ipi); > +#undef COPY_BACK > + > + default: > + WARN_ON(rc != -ENOSYS); > + break; > + } > + > + return rc; > +} > + > +int HYPERVISOR_physdev_op_compat(int cmd, void *arg) > +{ > + struct physdev_op op; > + int rc; > + > + op.cmd = cmd; > + memcpy(&op.u, arg, sizeof(op.u)); > + rc = _hypercall1(int, physdev_op_compat, &op); > + > + switch (cmd) { > + case PHYSDEVOP_IRQ_UNMASK_NOTIFY: > + case PHYSDEVOP_set_iopl: > + case PHYSDEVOP_set_iobitmap: > + case PHYSDEVOP_apic_write: > + /* no output */ > + break; > + > +#define COPY_BACK(pop, fld) \ > + case PHYSDEVOP_##pop: \ > + memcpy(arg, &op.u.fld, sizeof(op.u.fld)); \ > + break > + > + COPY_BACK(irq_status_query, irq_status_query); > + COPY_BACK(apic_read, apic_op); > + COPY_BACK(ASSIGN_VECTOR, irq_op); > +#undef COPY_BACK > + > + default: > + WARN_ON(rc != -ENOSYS); > + break; > + } > + > + return rc; > +} > + > +#endif /* CONFIG_XEN_COMPAT <= 0x030002 */ > --- a/include/asm-i386/mach-xen/asm/hypercall.h > +++ b/include/asm-i386/mach-xen/asm/hypercall.h > @@ -33,7 +33,6 @@ > #ifndef __HYPERCALL_H__ > #define __HYPERCALL_H__ > > -#include <linux/string.h> /* memcpy() */ > #include <linux/stringify.h> > > #ifndef __HYPERVISOR_H__ > @@ -128,6 +127,11 @@ > __res; \ > }) > > +#if CONFIG_XEN_COMPAT <= 0x030002 > +int __must_check HYPERVISOR_event_channel_op_compat(int, void *); > +int __must_check HYPERVISOR_physdev_op_compat(int, void *); > +#endif > + > static inline int __must_check > HYPERVISOR_set_trap_table( > const trap_info_t *table) > @@ -267,13 +271,8 @@ HYPERVISOR_event_channel_op( > int rc = _hypercall2(int, event_channel_op, cmd, arg); > > #if CONFIG_XEN_COMPAT <= 0x030002 > - if (unlikely(rc == -ENOSYS)) { > - struct evtchn_op op; > - op.cmd = cmd; > - memcpy(&op.u, arg, sizeof(op.u)); > - rc = _hypercall1(int, event_channel_op_compat, &op); > - memcpy(arg, &op.u, sizeof(op.u)); > - } > + if (unlikely(rc == -ENOSYS)) > + rc = HYPERVISOR_event_channel_op_compat(cmd, arg); > #endif > > return rc; > @@ -300,13 +299,8 @@ HYPERVISOR_physdev_op( > int rc = _hypercall2(int, physdev_op, cmd, arg); > > #if CONFIG_XEN_COMPAT <= 0x030002 > - if (unlikely(rc == -ENOSYS)) { > - struct physdev_op op; > - op.cmd = cmd; > - memcpy(&op.u, arg, sizeof(op.u)); > - rc = _hypercall1(int, physdev_op_compat, &op); > - memcpy(arg, &op.u, sizeof(op.u)); > - } > + if (unlikely(rc == -ENOSYS)) > + rc = HYPERVISOR_physdev_op_compat(cmd, arg); > #endif > > return rc; > --- a/include/asm-i386/mach-xen/asm/hypervisor.h > +++ b/include/asm-i386/mach-xen/asm/hypervisor.h > @@ -39,8 +39,6 @@ > #include <linux/errno.h> > #include <xen/interface/xen.h> > #include <xen/interface/platform.h> > -#include <xen/interface/event_channel.h> > -#include <xen/interface/physdev.h> > #include <xen/interface/sched.h> > #include <xen/interface/nmi.h> > #include <xen/interface/tmem.h> > _______________________________________________ > Xen-devel mailing list > Xen-devel@xxxxxxxxxxxxx > http://lists.xen.org/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.