[PATCH] linux-2.6.18: fix hypercall fallback code for very old hypervisors

[PATCH] linux-2.6.18: fix hypercall fallback code for very old hypervisors

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4978 bytes --]

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 containter's.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- 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,88 @@
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <asm/bug.h>
+#include <asm/hypervisor.h>
+
+#if 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) \
+	case PHYSDEVOP_##pop: \
+		memcpy(arg, &op.u.pop, sizeof(op.u.pop)); \
+		break
+
+	COPY_BACK(irq_status_query);
+#define apic_read apic_op
+	COPY_BACK(apic_read);
+#undef apic_read
+#define ASSIGN_VECTOR irq_op
+	COPY_BACK(ASSIGN_VECTOR);
+#undef ASSIGN_VECTOR
+#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>



[-- Attachment #2: xen-hypercall-fallback.patch --]
[-- Type: text/plain, Size: 5030 bytes --]

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 containter's.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- 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,88 @@
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <asm/bug.h>
+#include <asm/hypervisor.h>
+
+#if 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) \
+	case PHYSDEVOP_##pop: \
+		memcpy(arg, &op.u.pop, sizeof(op.u.pop)); \
+		break
+
+	COPY_BACK(irq_status_query);
+#define apic_read apic_op
+	COPY_BACK(apic_read);
+#undef apic_read
+#define ASSIGN_VECTOR irq_op
+	COPY_BACK(ASSIGN_VECTOR);
+#undef ASSIGN_VECTOR
+#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>

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Ian Campbell]

On Tue, 2012-10-16 at 13:29 +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 containter's.

CC-ing Konrad (and therefore not trimming quotes)

Typo: "containers"

> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- 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,88 @@
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <asm/bug.h>
> +#include <asm/hypervisor.h>
> +
> +#if 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) \
> +	case PHYSDEVOP_##pop: \
> +		memcpy(arg, &op.u.pop, sizeof(op.u.pop)); \
> +		break
> +
> +	COPY_BACK(irq_status_query);
> +#define apic_read apic_op
> +	COPY_BACK(apic_read);
> +#undef apic_read
> +#define ASSIGN_VECTOR irq_op
> +	COPY_BACK(ASSIGN_VECTOR);
> +#undef ASSIGN_VECTOR
> +#undef COPY_BACK

I think rather than this def/undef dance either a two argument macro or
open coding the two cases would be cleaner.

> +
> +	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>
> 
> 

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

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4997 bytes --]

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@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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>



[-- Attachment #2: xen-hypercall-fallback.patch --]
[-- Type: text/plain, Size: 5049 bytes --]

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@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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>

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Konrad Rzeszutek Wilk]

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@oracle.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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@oracle.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Konrad Rzeszutek Wilk]

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@oracle.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Reduce #define/#undef usage in HYPERVISOR_physdev_op_compat().

And here is the upstream version.

>From 7e3619ca4049957e1d869dc382c651766a722e3d Mon Sep 17 00:00:00 2001
From: Jan Beulich <jbeulich@suse.com>
Date: Fri, 19 Oct 2012 15:25:37 -0400
Subject: [PATCH] xen/hypercall: 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@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
[v2: Reduce #define/#undef usage in HYPERVISOR_physdev_op_compat().]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/include/asm/xen/hypercall.h |   21 +++------
 drivers/xen/Makefile                 |    2 +-
 drivers/xen/fallback.c               |   78 ++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+), 15 deletions(-)
 create mode 100644 drivers/xen/fallback.c

diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index 59c226d..c812360 100644
--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -359,18 +359,14 @@ HYPERVISOR_update_va_mapping(unsigned long va, pte_t new_val,
 		return _hypercall4(int, update_va_mapping, va,
 				   new_val.pte, new_val.pte >> 32, flags);
 }
+int __must_check HYPERVISOR_event_channel_op_compat(int, void *);
 
 static inline int
 HYPERVISOR_event_channel_op(int cmd, void *arg)
 {
 	int rc = _hypercall2(int, event_channel_op, cmd, arg);
-	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);
 	return rc;
 }
 
@@ -386,17 +382,14 @@ HYPERVISOR_console_io(int cmd, int count, char *str)
 	return _hypercall3(int, console_io, cmd, count, str);
 }
 
+int __must_check HYPERVISOR_physdev_op_compat(int, void *);
+
 static inline int
 HYPERVISOR_physdev_op(int cmd, void *arg)
 {
 	int rc = _hypercall2(int, physdev_op, cmd, arg);
-	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);
 	return rc;
 }
 
diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index 0e863703..46de6cd 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -2,7 +2,7 @@ ifneq ($(CONFIG_ARM),y)
 obj-y	+= manage.o balloon.o
 obj-$(CONFIG_HOTPLUG_CPU)		+= cpu_hotplug.o
 endif
-obj-y	+= grant-table.o features.o events.o
+obj-y	+= grant-table.o features.o events.o fallback.o
 obj-y	+= xenbus/
 
 nostackp := $(call cc-option, -fno-stack-protector)
diff --git a/drivers/xen/fallback.c b/drivers/xen/fallback.c
new file mode 100644
index 0000000..0bdc468
--- /dev/null
+++ b/drivers/xen/fallback.c
@@ -0,0 +1,78 @@
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/bug.h>
+#include <asm/hypervisor.h>
+#include <asm/xen/hypercall.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;
+}
-- 
1.7.7.6

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

[Jan Beulich]

>>> On 19.10.12 at 21:23, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> 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@oracle.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> 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?

Spotted this left-over (from build testing) before committing,
luckily. Thanks for noticing nevertheless!

Jan