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

[Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx>]

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().

And here is the upstream version.

>From 7e3619ca4049957e1d869dc382c651766a722e3d Mon Sep 17 00:00:00 2001
From: Jan Beulich <jbeulich@xxxxxxxx>
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@xxxxxxxxxx>
Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
[v2: Reduce #define/#undef usage in HYPERVISOR_physdev_op_compat().]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
---
 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


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