[Qemu-devel] [PATCH v4 0/8] xen: xen-domid-restrict improvements

[Qemu-devel] [PATCH v4 0/8] xen: xen-domid-restrict improvements

[Ian Jackson]

I have been working on trying to get qemu, when running as a Xen
device model, to _actually_ not have power equivalent to root.

I think I have achieved this, with some limitations (which are
discussed in my series against xen.git.

However, there are changes to qemu needed.  In particular

 * The -xen-domid-restrict option does not work properly right now.
   It only restricts a small subset of the descriptors qemu has open.
   I am introducing a new library call in the Xen libraries for this,
   xentoolcore_restrict_all.

 * We need to call a different function on domain shutdown.

 * The restriction operation needs to be done at a slightly different
   time, necessitating a new hook.

 * Additionally, in the future, we intend to be able to set aside
   a uid range for these qemus to run in, and that involves being
   able to tell qemu to drop privilege by numeric uid and gid.

Thanks to Anthony Perard, Peter Maydell and Ross Lagerwall for
assistance, review and testing.

At least the first patch of this, "xen: link against xentoolcore",
will very likely be necessary, since the corresponding xen.git series
is likely to make Xen 4.10.

   1/8  xen: link against xentoolcore
   2/8  xen: restrict: use xentoolcore_restrict_all
   3/8  xen: defer call to xen_restrict until just before
   4/8  xen: destroy_hvm_domain: Move reason into a variable
   5/8  xen: move xc_interface compatibility fallback further up
   6/8  xen: destroy_hvm_domain: Try xendevicemodel_shutdown
 * 7/8  os-posix: Provide new -runas <uid>.<gid> facility
 @ 8/8  configure: do_compiler: Dump some extra info under bash

 * = patch changed in v4 of the series
 @ = "RFC" tag removed

Thanks,
Ian.

[PATCH v4 0/8] xen: xen-domid-restrict improvements

[Ian Jackson]

I have been working on trying to get qemu, when running as a Xen
device model, to _actually_ not have power equivalent to root.

I think I have achieved this, with some limitations (which are
discussed in my series against xen.git.

However, there are changes to qemu needed.  In particular

 * The -xen-domid-restrict option does not work properly right now.
   It only restricts a small subset of the descriptors qemu has open.
   I am introducing a new library call in the Xen libraries for this,
   xentoolcore_restrict_all.

 * We need to call a different function on domain shutdown.

 * The restriction operation needs to be done at a slightly different
   time, necessitating a new hook.

 * Additionally, in the future, we intend to be able to set aside
   a uid range for these qemus to run in, and that involves being
   able to tell qemu to drop privilege by numeric uid and gid.

Thanks to Anthony Perard, Peter Maydell and Ross Lagerwall for
assistance, review and testing.

At least the first patch of this, "xen: link against xentoolcore",
will very likely be necessary, since the corresponding xen.git series
is likely to make Xen 4.10.

   1/8  xen: link against xentoolcore
   2/8  xen: restrict: use xentoolcore_restrict_all
   3/8  xen: defer call to xen_restrict until just before
   4/8  xen: destroy_hvm_domain: Move reason into a variable
   5/8  xen: move xc_interface compatibility fallback further up
   6/8  xen: destroy_hvm_domain: Try xendevicemodel_shutdown
 * 7/8  os-posix: Provide new -runas <uid>.<gid> facility
 @ 8/8  configure: do_compiler: Dump some extra info under bash

 * = patch changed in v4 of the series
 @ = "RFC" tag removed

Thanks,
Ian.

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

[Qemu-devel] [PATCH 1/8] xen: link against xentoolcore

[Ian Jackson]

From: Anthony PERARD <anthony.perard@citrix.com>

Xen libraries 4.10 will include a new xentoolcore library, without
which xendevicemodel et al will not work.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 configure | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index fd7e3a5..6f691df 100755
--- a/configure
+++ b/configure
@@ -2072,7 +2072,7 @@ if test "$xen" != "no" ; then
       $($pkg_config --modversion xencontrol | sed 's/\./ /g') )"
     xen=yes
     xen_pc="xencontrol xenstore xenguest xenforeignmemory xengnttab"
-    xen_pc="$xen_pc xenevtchn xendevicemodel"
+    xen_pc="$xen_pc xenevtchn xendevicemodel xentoolcore"
     QEMU_CFLAGS="$QEMU_CFLAGS $($pkg_config --cflags $xen_pc)"
     libs_softmmu="$($pkg_config --libs $xen_pc) $libs_softmmu"
     LDFLAGS="$($pkg_config --libs $xen_pc) $LDFLAGS"
@@ -2104,18 +2104,20 @@ EOF
         cat > $TMPC <<EOF &&
 #undef XC_WANT_COMPAT_MAP_FOREIGN_API
 #include <xenforeignmemory.h>
+#include <xentoolcore.h>
 int main(void) {
   xenforeignmemory_handle *xfmem;
 
   xfmem = xenforeignmemory_open(0, 0);
   xenforeignmemory_map2(xfmem, 0, 0, 0, 0, 0, 0, 0);
+  xentoolcore_restrict_all(0);
 
   return 0;
 }
 EOF
-        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs"
+        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs -lxentoolcore"
       then
-      xen_stable_libs="-lxendevicemodel $xen_stable_libs"
+      xen_stable_libs="-lxendevicemodel $xen_stable_libs -lxentoolcore"
       xen_ctrl_version=41000
       xen=yes
     elif
-- 
2.1.4

[PATCH 1/8] xen: link against xentoolcore

[Ian Jackson]

From: Anthony PERARD <anthony.perard@citrix.com>

Xen libraries 4.10 will include a new xentoolcore library, without
which xendevicemodel et al will not work.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 configure | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index fd7e3a5..6f691df 100755
--- a/configure
+++ b/configure
@@ -2072,7 +2072,7 @@ if test "$xen" != "no" ; then
       $($pkg_config --modversion xencontrol | sed 's/\./ /g') )"
     xen=yes
     xen_pc="xencontrol xenstore xenguest xenforeignmemory xengnttab"
-    xen_pc="$xen_pc xenevtchn xendevicemodel"
+    xen_pc="$xen_pc xenevtchn xendevicemodel xentoolcore"
     QEMU_CFLAGS="$QEMU_CFLAGS $($pkg_config --cflags $xen_pc)"
     libs_softmmu="$($pkg_config --libs $xen_pc) $libs_softmmu"
     LDFLAGS="$($pkg_config --libs $xen_pc) $LDFLAGS"
@@ -2104,18 +2104,20 @@ EOF
         cat > $TMPC <<EOF &&
 #undef XC_WANT_COMPAT_MAP_FOREIGN_API
 #include <xenforeignmemory.h>
+#include <xentoolcore.h>
 int main(void) {
   xenforeignmemory_handle *xfmem;
 
   xfmem = xenforeignmemory_open(0, 0);
   xenforeignmemory_map2(xfmem, 0, 0, 0, 0, 0, 0, 0);
+  xentoolcore_restrict_all(0);
 
   return 0;
 }
 EOF
-        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs"
+        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs -lxentoolcore"
       then
-      xen_stable_libs="-lxendevicemodel $xen_stable_libs"
+      xen_stable_libs="-lxendevicemodel $xen_stable_libs -lxentoolcore"
       xen_ctrl_version=41000
       xen=yes
     elif
-- 
2.1.4


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

[Qemu-devel] [PATCH 2/8] xen: restrict: use xentoolcore_restrict_all

[Ian Jackson]

And insist that it works.

Drop individual use of xendevicemodel_restrict and
xenforeignmemory_restrict.  These are not actually effective in this
version of qemu, because qemu has a large number of fds open onto
various Xen control devices.

The restriction arrangements are still not right, because the
restriction needs to be done very late - after qemu has opened all of
its control fds.

xentoolcore_restrict_all and xentoolcore.h are available in Xen 4.10
and later, only.  Provide a compatibility stub.  And drop the
compatibility stubs for the old functions.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v2: Modify the compatibility code, too.
    Bump this patch ahead of "defer call to xen_restrict until running"
    Retain call to xentoolcore_restrict_all

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 include/hw/xen/xen_common.h | 46 +++++++++++----------------------------------
 1 file changed, 11 insertions(+), 35 deletions(-)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 86c7f26..3f44a63 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -91,6 +91,16 @@ static inline void *xenforeignmemory_map2(xenforeignmemory_handle *h,
     return xenforeignmemory_map(h, dom, prot, pages, arr, err);
 }
 
+static inline int xentoolcore_restrict_all(domid_t domid)
+{
+    errno = ENOTTY;
+    return -1;
+}
+
+#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 41000 */
+
+#include <xentoolcore.h>
+
 #endif
 
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
@@ -218,20 +228,6 @@ static inline int xendevicemodel_set_mem_type(
     return xc_hvm_set_mem_type(dmod, domid, mem_type, first_pfn, nr);
 }
 
-static inline int xendevicemodel_restrict(
-    xendevicemodel_handle *dmod, domid_t domid)
-{
-    errno = ENOTTY;
-    return -1;
-}
-
-static inline int xenforeignmemory_restrict(
-    xenforeignmemory_handle *fmem, domid_t domid)
-{
-    errno = ENOTTY;
-    return -1;
-}
-
 #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
 
 #undef XC_WANT_COMPAT_DEVICEMODEL_API
@@ -290,28 +286,8 @@ static inline int xen_modified_memory(domid_t domid, uint64_t first_pfn,
 static inline int xen_restrict(domid_t domid)
 {
     int rc;
-
-    /* Attempt to restrict devicemodel operations */
-    rc = xendevicemodel_restrict(xen_dmod, domid);
+    rc = xentoolcore_restrict_all(domid);
     trace_xen_domid_restrict(rc ? errno : 0);
-
-    if (rc < 0) {
-        /*
-         * If errno is ENOTTY then restriction is not implemented so
-         * there's no point in trying to restrict other types of
-         * operation, but it should not be treated as a failure.
-         */
-        if (errno == ENOTTY) {
-            return 0;
-        }
-
-        return rc;
-    }
-
-    /* Restrict foreignmemory operations */
-    rc = xenforeignmemory_restrict(xen_fmem, domid);
-    trace_xen_domid_restrict(rc ? errno : 0);
-
     return rc;
 }
 
-- 
2.1.4

[PATCH 2/8] xen: restrict: use xentoolcore_restrict_all

[Ian Jackson]

And insist that it works.

Drop individual use of xendevicemodel_restrict and
xenforeignmemory_restrict.  These are not actually effective in this
version of qemu, because qemu has a large number of fds open onto
various Xen control devices.

The restriction arrangements are still not right, because the
restriction needs to be done very late - after qemu has opened all of
its control fds.

xentoolcore_restrict_all and xentoolcore.h are available in Xen 4.10
and later, only.  Provide a compatibility stub.  And drop the
compatibility stubs for the old functions.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v2: Modify the compatibility code, too.
    Bump this patch ahead of "defer call to xen_restrict until running"
    Retain call to xentoolcore_restrict_all

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 include/hw/xen/xen_common.h | 46 +++++++++++----------------------------------
 1 file changed, 11 insertions(+), 35 deletions(-)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 86c7f26..3f44a63 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -91,6 +91,16 @@ static inline void *xenforeignmemory_map2(xenforeignmemory_handle *h,
     return xenforeignmemory_map(h, dom, prot, pages, arr, err);
 }
 
+static inline int xentoolcore_restrict_all(domid_t domid)
+{
+    errno = ENOTTY;
+    return -1;
+}
+
+#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 41000 */
+
+#include <xentoolcore.h>
+
 #endif
 
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
@@ -218,20 +228,6 @@ static inline int xendevicemodel_set_mem_type(
     return xc_hvm_set_mem_type(dmod, domid, mem_type, first_pfn, nr);
 }
 
-static inline int xendevicemodel_restrict(
-    xendevicemodel_handle *dmod, domid_t domid)
-{
-    errno = ENOTTY;
-    return -1;
-}
-
-static inline int xenforeignmemory_restrict(
-    xenforeignmemory_handle *fmem, domid_t domid)
-{
-    errno = ENOTTY;
-    return -1;
-}
-
 #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
 
 #undef XC_WANT_COMPAT_DEVICEMODEL_API
@@ -290,28 +286,8 @@ static inline int xen_modified_memory(domid_t domid, uint64_t first_pfn,
 static inline int xen_restrict(domid_t domid)
 {
     int rc;
-
-    /* Attempt to restrict devicemodel operations */
-    rc = xendevicemodel_restrict(xen_dmod, domid);
+    rc = xentoolcore_restrict_all(domid);
     trace_xen_domid_restrict(rc ? errno : 0);
-
-    if (rc < 0) {
-        /*
-         * If errno is ENOTTY then restriction is not implemented so
-         * there's no point in trying to restrict other types of
-         * operation, but it should not be treated as a failure.
-         */
-        if (errno == ENOTTY) {
-            return 0;
-        }
-
-        return rc;
-    }
-
-    /* Restrict foreignmemory operations */
-    rc = xenforeignmemory_restrict(xen_fmem, domid);
-    trace_xen_domid_restrict(rc ? errno : 0);
-
     return rc;
 }
 
-- 
2.1.4


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

[Qemu-devel] [PATCH 3/8] xen: defer call to xen_restrict until just before os_setup_post

[Ian Jackson]

We need to restrict *all* the control fds that qemu opens.  Looking in
/proc/PID/fd shows there are many; their allocation seems scattered
throughout Xen support code in qemu.

We must postpone the restrict call until roughly the same time as qemu
changes its uid, chroots (if applicable), and so on.

There doesn't seem to be an appropriate hook already.  The RunState
change hook fires at different times depending on exactly what mode
qemu is operating in.

And it appears that no-one but the Xen code wants a hook at this phase
of execution.  So, introduce a bare call to a new function
xen_setup_post, just before os_setup_post.  Also provide the
appropriate stub for when Xen compilation is disabled.

We do the restriction before rather than after os_setup_post, because
xen_restrict may need to open /dev/null, and os_setup_post might have
called chroot.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v3: Do xen_setup_post just before, not just after, os_setup_post,
    to improve interaction with chroot.  Thanks to Ross Lagerwall.
---
 hw/i386/xen/xen-hvm.c   |  8 --------
 hw/xen/xen-common.c     | 13 +++++++++++++
 include/sysemu/sysemu.h |  2 ++
 stubs/xen-hvm.c         |  5 +++++
 vl.c                    |  1 +
 5 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index d9ccd5d..7b60ec6 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1254,14 +1254,6 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
         goto err;
     }
 
-    if (xen_domid_restrict) {
-        rc = xen_restrict(xen_domid);
-        if (rc < 0) {
-            error_report("failed to restrict: error %d", errno);
-            goto err;
-        }
-    }
-
     xen_create_ioreq_server(xen_domid, &state->ioservid);
 
     state->exit.notify = xen_exit_notifier;
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 632a938..4056420 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -117,6 +117,19 @@ static void xen_change_state_handler(void *opaque, int running,
     }
 }
 
+void xen_setup_post(void)
+{
+    int rc;
+
+    if (xen_domid_restrict) {
+        rc = xen_restrict(xen_domid);
+        if (rc < 0) {
+            perror("xen: failed to restrict");
+            exit(1);
+        }
+    }
+}
+
 static int xen_init(MachineState *ms)
 {
     xen_xc = xc_interface_open(0, 0, 0);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index b213696..b064a55 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -93,6 +93,8 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify);
 
 void qemu_announce_self(void);
 
+void xen_setup_post(void);
+
 extern int autostart;
 
 typedef enum {
diff --git a/stubs/xen-hvm.c b/stubs/xen-hvm.c
index 3ca6c51..9701feb 100644
--- a/stubs/xen-hvm.c
+++ b/stubs/xen-hvm.c
@@ -13,6 +13,7 @@
 #include "hw/xen/xen.h"
 #include "exec/memory.h"
 #include "qmp-commands.h"
+#include "sysemu/sysemu.h"
 
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
 {
@@ -61,3 +62,7 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
 void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
 {
 }
+
+void xen_setup_post(void)
+{
+}
diff --git a/vl.c b/vl.c
index fb1f05b..ca06553 100644
--- a/vl.c
+++ b/vl.c
@@ -4792,6 +4792,7 @@ int main(int argc, char **argv, char **envp)
         vm_start();
     }
 
+    xen_setup_post();
     os_setup_post();
 
     main_loop();
-- 
2.1.4

[PATCH 3/8] xen: defer call to xen_restrict until just before os_setup_post

[Ian Jackson]

We need to restrict *all* the control fds that qemu opens.  Looking in
/proc/PID/fd shows there are many; their allocation seems scattered
throughout Xen support code in qemu.

We must postpone the restrict call until roughly the same time as qemu
changes its uid, chroots (if applicable), and so on.

There doesn't seem to be an appropriate hook already.  The RunState
change hook fires at different times depending on exactly what mode
qemu is operating in.

And it appears that no-one but the Xen code wants a hook at this phase
of execution.  So, introduce a bare call to a new function
xen_setup_post, just before os_setup_post.  Also provide the
appropriate stub for when Xen compilation is disabled.

We do the restriction before rather than after os_setup_post, because
xen_restrict may need to open /dev/null, and os_setup_post might have
called chroot.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v3: Do xen_setup_post just before, not just after, os_setup_post,
    to improve interaction with chroot.  Thanks to Ross Lagerwall.
---
 hw/i386/xen/xen-hvm.c   |  8 --------
 hw/xen/xen-common.c     | 13 +++++++++++++
 include/sysemu/sysemu.h |  2 ++
 stubs/xen-hvm.c         |  5 +++++
 vl.c                    |  1 +
 5 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index d9ccd5d..7b60ec6 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1254,14 +1254,6 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
         goto err;
     }
 
-    if (xen_domid_restrict) {
-        rc = xen_restrict(xen_domid);
-        if (rc < 0) {
-            error_report("failed to restrict: error %d", errno);
-            goto err;
-        }
-    }
-
     xen_create_ioreq_server(xen_domid, &state->ioservid);
 
     state->exit.notify = xen_exit_notifier;
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 632a938..4056420 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -117,6 +117,19 @@ static void xen_change_state_handler(void *opaque, int running,
     }
 }
 
+void xen_setup_post(void)
+{
+    int rc;
+
+    if (xen_domid_restrict) {
+        rc = xen_restrict(xen_domid);
+        if (rc < 0) {
+            perror("xen: failed to restrict");
+            exit(1);
+        }
+    }
+}
+
 static int xen_init(MachineState *ms)
 {
     xen_xc = xc_interface_open(0, 0, 0);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index b213696..b064a55 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -93,6 +93,8 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify);
 
 void qemu_announce_self(void);
 
+void xen_setup_post(void);
+
 extern int autostart;
 
 typedef enum {
diff --git a/stubs/xen-hvm.c b/stubs/xen-hvm.c
index 3ca6c51..9701feb 100644
--- a/stubs/xen-hvm.c
+++ b/stubs/xen-hvm.c
@@ -13,6 +13,7 @@
 #include "hw/xen/xen.h"
 #include "exec/memory.h"
 #include "qmp-commands.h"
+#include "sysemu/sysemu.h"
 
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
 {
@@ -61,3 +62,7 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
 void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
 {
 }
+
+void xen_setup_post(void)
+{
+}
diff --git a/vl.c b/vl.c
index fb1f05b..ca06553 100644
--- a/vl.c
+++ b/vl.c
@@ -4792,6 +4792,7 @@ int main(int argc, char **argv, char **envp)
         vm_start();
     }
 
+    xen_setup_post();
     os_setup_post();
 
     main_loop();
-- 
2.1.4


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

[Qemu-devel] [PATCH 4/8] xen: destroy_hvm_domain: Move reason into a variable

[Ian Jackson]

We are going to want to reuse this.

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/i386/xen/xen-hvm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 7b60ec6..83420cd 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1387,12 +1387,13 @@ void destroy_hvm_domain(bool reboot)
     xc_interface *xc_handle;
     int sts;
 
+    unsigned int reason = reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff;
+
     xc_handle = xc_interface_open(0, 0, 0);
     if (xc_handle == NULL) {
         fprintf(stderr, "Cannot acquire xenctrl handle\n");
     } else {
-        sts = xc_domain_shutdown(xc_handle, xen_domid,
-                                 reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff);
+        sts = xc_domain_shutdown(xc_handle, xen_domid, reason);
         if (sts != 0) {
             fprintf(stderr, "xc_domain_shutdown failed to issue %s, "
                     "sts %d, %s\n", reboot ? "reboot" : "poweroff",
-- 
2.1.4

[PATCH 4/8] xen: destroy_hvm_domain: Move reason into a variable

[Ian Jackson]

We are going to want to reuse this.

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/i386/xen/xen-hvm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 7b60ec6..83420cd 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1387,12 +1387,13 @@ void destroy_hvm_domain(bool reboot)
     xc_interface *xc_handle;
     int sts;
 
+    unsigned int reason = reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff;
+
     xc_handle = xc_interface_open(0, 0, 0);
     if (xc_handle == NULL) {
         fprintf(stderr, "Cannot acquire xenctrl handle\n");
     } else {
-        sts = xc_domain_shutdown(xc_handle, xen_domid,
-                                 reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff);
+        sts = xc_domain_shutdown(xc_handle, xen_domid, reason);
         if (sts != 0) {
             fprintf(stderr, "xc_domain_shutdown failed to issue %s, "
                     "sts %d, %s\n", reboot ? "reboot" : "poweroff",
-- 
2.1.4


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

[Qemu-devel] [PATCH 5/8] xen: move xc_interface compatibility fallback further up the file

[Ian Jackson]

We are going to want to use the dummy xendevicemodel_handle type in
new stub functions in the CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
section.  So we need to provide that definition, or (as applicable)
include the appropriate header, earlier in the file.

(Ideally the newer compatibility layers would be at the bottom of the
file, so that they can naturally benefit from the compatibility layers
for earlier version.  But that's rather too much for this series.)

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v2: New patch in v2 of the series
---
 include/hw/xen/xen_common.h | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 3f44a63..8efdb8a 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -78,6 +78,17 @@ static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
 
 extern xenforeignmemory_handle *xen_fmem;
 
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
+
+typedef xc_interface xendevicemodel_handle;
+
+#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
+
+#undef XC_WANT_COMPAT_DEVICEMODEL_API
+#include <xendevicemodel.h>
+
+#endif
+
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
 
 #define XEN_COMPAT_PHYSMAP
@@ -105,8 +116,6 @@ static inline int xentoolcore_restrict_all(domid_t domid)
 
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
 
-typedef xc_interface xendevicemodel_handle;
-
 static inline xendevicemodel_handle *xendevicemodel_open(
     struct xentoollog_logger *logger, unsigned int open_flags)
 {
@@ -228,11 +237,6 @@ static inline int xendevicemodel_set_mem_type(
     return xc_hvm_set_mem_type(dmod, domid, mem_type, first_pfn, nr);
 }
 
-#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
-
-#undef XC_WANT_COMPAT_DEVICEMODEL_API
-#include <xendevicemodel.h>
-
 #endif
 
 extern xendevicemodel_handle *xen_dmod;
-- 
2.1.4

[PATCH 5/8] xen: move xc_interface compatibility fallback further up the file

[Ian Jackson]

We are going to want to use the dummy xendevicemodel_handle type in
new stub functions in the CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
section.  So we need to provide that definition, or (as applicable)
include the appropriate header, earlier in the file.

(Ideally the newer compatibility layers would be at the bottom of the
file, so that they can naturally benefit from the compatibility layers
for earlier version.  But that's rather too much for this series.)

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v2: New patch in v2 of the series
---
 include/hw/xen/xen_common.h | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 3f44a63..8efdb8a 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -78,6 +78,17 @@ static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
 
 extern xenforeignmemory_handle *xen_fmem;
 
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
+
+typedef xc_interface xendevicemodel_handle;
+
+#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
+
+#undef XC_WANT_COMPAT_DEVICEMODEL_API
+#include <xendevicemodel.h>
+
+#endif
+
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
 
 #define XEN_COMPAT_PHYSMAP
@@ -105,8 +116,6 @@ static inline int xentoolcore_restrict_all(domid_t domid)
 
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
 
-typedef xc_interface xendevicemodel_handle;
-
 static inline xendevicemodel_handle *xendevicemodel_open(
     struct xentoollog_logger *logger, unsigned int open_flags)
 {
@@ -228,11 +237,6 @@ static inline int xendevicemodel_set_mem_type(
     return xc_hvm_set_mem_type(dmod, domid, mem_type, first_pfn, nr);
 }
 
-#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
-
-#undef XC_WANT_COMPAT_DEVICEMODEL_API
-#include <xendevicemodel.h>
-
 #endif
 
 extern xendevicemodel_handle *xen_dmod;
-- 
2.1.4


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

[Qemu-devel] [PATCH 6/8] xen: destroy_hvm_domain: Try xendevicemodel_shutdown

[Ian Jackson]

xc_interface_open etc. is not going to work if we have dropped
privilege, but xendevicemodel_shutdown will if everything is new
enough.

xendevicemodel_shutdown is only availabe in Xen 4.10 and later, so
provide a stub for earlier versions.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v2: Add compatibility stub for Xen < 4.10.
    Fix coding style.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 hw/i386/xen/xen-hvm.c       | 10 ++++++++++
 include/hw/xen/xen_common.h |  7 +++++++
 2 files changed, 17 insertions(+)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 83420cd..25b8b14 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1386,9 +1386,19 @@ void destroy_hvm_domain(bool reboot)
 {
     xc_interface *xc_handle;
     int sts;
+    int rc;
 
     unsigned int reason = reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff;
 
+    if (xen_dmod) {
+        rc = xendevicemodel_shutdown(xen_dmod, xen_domid, reason);
+        if (!rc) {
+            return;
+        }
+        perror("xendevicemodel_shutdown failed");
+        /* well, try the old thing then */
+    }
+
     xc_handle = xc_interface_open(0, 0, 0);
     if (xc_handle == NULL) {
         fprintf(stderr, "Cannot acquire xenctrl handle\n");
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 8efdb8a..1d6fb57 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -108,6 +108,13 @@ static inline int xentoolcore_restrict_all(domid_t domid)
     return -1;
 }
 
+static inline int xendevicemodel_shutdown(xendevicemodel_handle *dmod,
+                                          domid_t domid, unsigned int reason)
+{
+    errno = ENOTTY;
+    return -1;
+}
+
 #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 41000 */
 
 #include <xentoolcore.h>
-- 
2.1.4

[PATCH 6/8] xen: destroy_hvm_domain: Try xendevicemodel_shutdown

[Ian Jackson]

xc_interface_open etc. is not going to work if we have dropped
privilege, but xendevicemodel_shutdown will if everything is new
enough.

xendevicemodel_shutdown is only availabe in Xen 4.10 and later, so
provide a stub for earlier versions.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v2: Add compatibility stub for Xen < 4.10.
    Fix coding style.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 hw/i386/xen/xen-hvm.c       | 10 ++++++++++
 include/hw/xen/xen_common.h |  7 +++++++
 2 files changed, 17 insertions(+)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 83420cd..25b8b14 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1386,9 +1386,19 @@ void destroy_hvm_domain(bool reboot)
 {
     xc_interface *xc_handle;
     int sts;
+    int rc;
 
     unsigned int reason = reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff;
 
+    if (xen_dmod) {
+        rc = xendevicemodel_shutdown(xen_dmod, xen_domid, reason);
+        if (!rc) {
+            return;
+        }
+        perror("xendevicemodel_shutdown failed");
+        /* well, try the old thing then */
+    }
+
     xc_handle = xc_interface_open(0, 0, 0);
     if (xc_handle == NULL) {
         fprintf(stderr, "Cannot acquire xenctrl handle\n");
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 8efdb8a..1d6fb57 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -108,6 +108,13 @@ static inline int xentoolcore_restrict_all(domid_t domid)
     return -1;
 }
 
+static inline int xendevicemodel_shutdown(xendevicemodel_handle *dmod,
+                                          domid_t domid, unsigned int reason)
+{
+    errno = ENOTTY;
+    return -1;
+}
+
 #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 41000 */
 
 #include <xentoolcore.h>
-- 
2.1.4


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

[Qemu-devel] [PATCH 7/8] os-posix: Provide new -runas <uid>.<gid> facility

[Ian Jackson]

This allows the caller to specify a uid and gid to use, even if there
is no corresponding password entry.  This will be useful in certain
Xen configurations.

We don't support just -runas <uid> because: (i) deprivileging without
calling setgroups would be ineffective (ii) given only a uid we don't
know what gid we ought to use (since uids may eppear in multiple
passwd file entries with different gids).

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v4: Changed to reuse option -runas
v3: Error messages fixed.  Thanks to Peter Maydell and Ross Lagerwall.
v2: Coding style fixes.
---
 os-posix.c      | 64 +++++++++++++++++++++++++++++++++++++++++++++++----------
 qemu-options.hx |  3 ++-
 2 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index 92e9d85..9418bd4 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -43,6 +43,8 @@
 #endif
 
 static struct passwd *user_pwd;
+static uid_t user_uid = (uid_t)-1;
+static gid_t user_gid = (gid_t)-1;
 static const char *chroot_dir;
 static int daemonize;
 static int daemon_pipe;
@@ -128,6 +130,34 @@ void os_set_proc_name(const char *s)
 #endif
 }
 
+
+static bool os_parse_runas_uid_gid(const char *optarg)
+{
+    unsigned long lv;
+    char *ep;
+    uid_t got_uid;
+    gid_t got_gid;
+    int rc;
+
+    errno = 0;
+    lv = strtoul(optarg, &ep, 0); /* can't qemu_strtoul, want *ep=='.' */
+    got_uid = lv; /* overflow here is ID in C99 */
+    if (errno || *ep != '.' || got_uid != lv || got_uid == (uid_t)-1) {
+        return false;
+    }
+
+    lv = 0;
+    rc = qemu_strtoul(ep + 1, 0, 0, &lv);
+    got_gid = lv; /* overflow here is ID in C99 */
+    if (rc || got_gid != lv || got_gid == (gid_t)-1) {
+        return false;
+    }
+
+    user_uid = got_uid;
+    user_gid = got_gid;
+    return true;
+}
+
 /*
  * Parse OS specific command line options.
  * return 0 if option handled, -1 otherwise
@@ -145,8 +175,10 @@ void os_parse_cmd_args(int index, const char *optarg)
 #endif
     case QEMU_OPTION_runas:
         user_pwd = getpwnam(optarg);
-        if (!user_pwd) {
-            fprintf(stderr, "User \"%s\" doesn't exist\n", optarg);
+        if (!user_pwd && !os_parse_runas_uid_gid(optarg)) {
+            fprintf(stderr,
+                    "User \"%s\" doesn't exist (and is not <uid>.<gid>)\n",
+                    optarg);
             exit(1);
         }
         break;
@@ -166,18 +198,28 @@ void os_parse_cmd_args(int index, const char *optarg)
 
 static void change_process_uid(void)
 {
-    if (user_pwd) {
-        if (setgid(user_pwd->pw_gid) < 0) {
-            fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
+    if (user_pwd || user_uid != (uid_t)-1) {
+        gid_t intended_gid = user_pwd ? user_pwd->pw_gid : user_gid;
+        uid_t intended_uid = user_pwd ? user_pwd->pw_uid : user_uid;
+        if (setgid(intended_gid) < 0) {
+            fprintf(stderr, "Failed to setgid(%d)\n", intended_gid);
             exit(1);
         }
-        if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
-            fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
-                    user_pwd->pw_name, user_pwd->pw_gid);
-            exit(1);
+        if (user_pwd) {
+            if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
+                fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
+                        user_pwd->pw_name, user_pwd->pw_gid);
+                exit(1);
+            }
+        } else {
+            if (setgroups(1, &user_gid) < 0) {
+                fprintf(stderr, "Failed to setgroups(1, [%d])",
+                        user_gid);
+                exit(1);
+            }
         }
-        if (setuid(user_pwd->pw_uid) < 0) {
-            fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
+        if (setuid(intended_uid) < 0) {
+            fprintf(stderr, "Failed to setuid(%d)\n", intended_uid);
             exit(1);
         }
         if (setuid(0) != -1) {
diff --git a/qemu-options.hx b/qemu-options.hx
index 9f6e2ad..bdff99f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3958,7 +3958,8 @@ ETEXI
 
 #ifndef _WIN32
 DEF("runas", HAS_ARG, QEMU_OPTION_runas, \
-    "-runas user     change to user id user just before starting the VM\n",
+    "-runas user     change to user id user just before starting the VM\n" \
+    "                user can be numeric uid.gid instead\n",
     QEMU_ARCH_ALL)
 #endif
 STEXI
-- 
2.1.4

[PATCH 7/8] os-posix: Provide new -runas <uid>.<gid> facility

[Ian Jackson]

This allows the caller to specify a uid and gid to use, even if there
is no corresponding password entry.  This will be useful in certain
Xen configurations.

We don't support just -runas <uid> because: (i) deprivileging without
calling setgroups would be ineffective (ii) given only a uid we don't
know what gid we ought to use (since uids may eppear in multiple
passwd file entries with different gids).

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v4: Changed to reuse option -runas
v3: Error messages fixed.  Thanks to Peter Maydell and Ross Lagerwall.
v2: Coding style fixes.
---
 os-posix.c      | 64 +++++++++++++++++++++++++++++++++++++++++++++++----------
 qemu-options.hx |  3 ++-
 2 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index 92e9d85..9418bd4 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -43,6 +43,8 @@
 #endif
 
 static struct passwd *user_pwd;
+static uid_t user_uid = (uid_t)-1;
+static gid_t user_gid = (gid_t)-1;
 static const char *chroot_dir;
 static int daemonize;
 static int daemon_pipe;
@@ -128,6 +130,34 @@ void os_set_proc_name(const char *s)
 #endif
 }
 
+
+static bool os_parse_runas_uid_gid(const char *optarg)
+{
+    unsigned long lv;
+    char *ep;
+    uid_t got_uid;
+    gid_t got_gid;
+    int rc;
+
+    errno = 0;
+    lv = strtoul(optarg, &ep, 0); /* can't qemu_strtoul, want *ep=='.' */
+    got_uid = lv; /* overflow here is ID in C99 */
+    if (errno || *ep != '.' || got_uid != lv || got_uid == (uid_t)-1) {
+        return false;
+    }
+
+    lv = 0;
+    rc = qemu_strtoul(ep + 1, 0, 0, &lv);
+    got_gid = lv; /* overflow here is ID in C99 */
+    if (rc || got_gid != lv || got_gid == (gid_t)-1) {
+        return false;
+    }
+
+    user_uid = got_uid;
+    user_gid = got_gid;
+    return true;
+}
+
 /*
  * Parse OS specific command line options.
  * return 0 if option handled, -1 otherwise
@@ -145,8 +175,10 @@ void os_parse_cmd_args(int index, const char *optarg)
 #endif
     case QEMU_OPTION_runas:
         user_pwd = getpwnam(optarg);
-        if (!user_pwd) {
-            fprintf(stderr, "User \"%s\" doesn't exist\n", optarg);
+        if (!user_pwd && !os_parse_runas_uid_gid(optarg)) {
+            fprintf(stderr,
+                    "User \"%s\" doesn't exist (and is not <uid>.<gid>)\n",
+                    optarg);
             exit(1);
         }
         break;
@@ -166,18 +198,28 @@ void os_parse_cmd_args(int index, const char *optarg)
 
 static void change_process_uid(void)
 {
-    if (user_pwd) {
-        if (setgid(user_pwd->pw_gid) < 0) {
-            fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
+    if (user_pwd || user_uid != (uid_t)-1) {
+        gid_t intended_gid = user_pwd ? user_pwd->pw_gid : user_gid;
+        uid_t intended_uid = user_pwd ? user_pwd->pw_uid : user_uid;
+        if (setgid(intended_gid) < 0) {
+            fprintf(stderr, "Failed to setgid(%d)\n", intended_gid);
             exit(1);
         }
-        if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
-            fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
-                    user_pwd->pw_name, user_pwd->pw_gid);
-            exit(1);
+        if (user_pwd) {
+            if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
+                fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
+                        user_pwd->pw_name, user_pwd->pw_gid);
+                exit(1);
+            }
+        } else {
+            if (setgroups(1, &user_gid) < 0) {
+                fprintf(stderr, "Failed to setgroups(1, [%d])",
+                        user_gid);
+                exit(1);
+            }
         }
-        if (setuid(user_pwd->pw_uid) < 0) {
-            fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
+        if (setuid(intended_uid) < 0) {
+            fprintf(stderr, "Failed to setuid(%d)\n", intended_uid);
             exit(1);
         }
         if (setuid(0) != -1) {
diff --git a/qemu-options.hx b/qemu-options.hx
index 9f6e2ad..bdff99f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3958,7 +3958,8 @@ ETEXI
 
 #ifndef _WIN32
 DEF("runas", HAS_ARG, QEMU_OPTION_runas, \
-    "-runas user     change to user id user just before starting the VM\n",
+    "-runas user     change to user id user just before starting the VM\n" \
+    "                user can be numeric uid.gid instead\n",
     QEMU_ARCH_ALL)
 #endif
 STEXI
-- 
2.1.4


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

[Qemu-devel] [PATCH 8/8] configure: do_compiler: Dump some extra info under bash

[Ian Jackson]

This makes it much easier to find a particular thing in config.log.

The information may be lacking in other shells, resulting in harmless
empty output.  (This is why we don't use the proper ${FUNCNAME[*]}
array syntax - other shells will choke on that.)

The extra output is only printed if configure is run with bash.  The
something), it is necessary to say   bash ./configure  to get the extra
debug info in the log.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v4: No longer tag this patch RFC.
---
 configure | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/configure b/configure
index 6f691df..21a2b15 100755
--- a/configure
+++ b/configure
@@ -60,6 +60,10 @@ do_compiler() {
     # is compiler binary to execute.
     local compiler="$1"
     shift
+    echo >>config.log "
+funcs: ${FUNCNAME}
+lines: ${BASH_LINENO}
+files: ${BASH_SOURCE}"
     echo $compiler "$@" >> config.log
     $compiler "$@" >> config.log 2>&1 || return $?
     # Test passed. If this is an --enable-werror build, rerun
-- 
2.1.4

[PATCH 8/8] configure: do_compiler: Dump some extra info under bash

[Ian Jackson]

This makes it much easier to find a particular thing in config.log.

The information may be lacking in other shells, resulting in harmless
empty output.  (This is why we don't use the proper ${FUNCNAME[*]}
array syntax - other shells will choke on that.)

The extra output is only printed if configure is run with bash.  The
something), it is necessary to say   bash ./configure  to get the extra
debug info in the log.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v4: No longer tag this patch RFC.
---
 configure | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/configure b/configure
index 6f691df..21a2b15 100755
--- a/configure
+++ b/configure
@@ -60,6 +60,10 @@ do_compiler() {
     # is compiler binary to execute.
     local compiler="$1"
     shift
+    echo >>config.log "
+funcs: ${FUNCNAME}
+lines: ${BASH_LINENO}
+files: ${BASH_SOURCE}"
     echo $compiler "$@" >> config.log
     $compiler "$@" >> config.log 2>&1 || return $?
     # Test passed. If this is an --enable-werror build, rerun
-- 
2.1.4


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

Re: [Qemu-devel] [PATCH 1/8] xen: link against xentoolcore

[Ian Jackson]

Ian Jackson writes ("[PATCH 1/8] xen: link against xentoolcore"):
> From: Anthony PERARD <anthony.perard@citrix.com>
> 
> Xen libraries 4.10 will include a new xentoolcore library, without
> which xendevicemodel et al will not work.

The xentoolcore library was just committed to xen.git#staging, so at
least this patch (or something like it) should go into qemu RSN.

Anthony, do you plan to cherry pick this into the xen-specific qemu
branch used for osstest tests ?

Thanks,
Ian.

Re: [PATCH 1/8] xen: link against xentoolcore

[Ian Jackson]

Ian Jackson writes ("[PATCH 1/8] xen: link against xentoolcore"):
> From: Anthony PERARD <anthony.perard@citrix.com>
> 
> Xen libraries 4.10 will include a new xentoolcore library, without
> which xendevicemodel et al will not work.

The xentoolcore library was just committed to xen.git#staging, so at
least this patch (or something like it) should go into qemu RSN.

Anthony, do you plan to cherry pick this into the xen-specific qemu
branch used for osstest tests ?

Thanks,
Ian.

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

Re: [Qemu-devel] [PATCH v4 0/8] xen: xen-domid-restrict improvements

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1507564902-9000-1-git-send-email-ian.jackson@eu.citrix.com
Subject: [Qemu-devel] [PATCH v4 0/8] xen: xen-domid-restrict improvements

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
2fefd4e512 configure: do_compiler: Dump some extra info under bash
b0ee2db430 os-posix: Provide new -runas <uid>.<gid> facility
bbe6e622ba xen: destroy_hvm_domain: Try xendevicemodel_shutdown
443cfed9da xen: move xc_interface compatibility fallback further up the file
7e9d286c99 xen: destroy_hvm_domain: Move reason into a variable
597a0de500 xen: defer call to xen_restrict until just before os_setup_post
9b5e7d8ef8 xen: restrict: use xentoolcore_restrict_all
54f9b2484b xen: link against xentoolcore

=== OUTPUT BEGIN ===
Checking PATCH 1/8: xen: link against xentoolcore...
Checking PATCH 2/8: xen: restrict: use xentoolcore_restrict_all...
Checking PATCH 3/8: xen: defer call to xen_restrict until just before os_setup_post...
Checking PATCH 4/8: xen: destroy_hvm_domain: Move reason into a variable...
Checking PATCH 5/8: xen: move xc_interface compatibility fallback further up the file...
Checking PATCH 6/8: xen: destroy_hvm_domain: Try xendevicemodel_shutdown...
Checking PATCH 7/8: os-posix: Provide new -runas <uid>.<gid> facility...
ERROR: consider using qemu_strtoul in preference to strtoul
#45: FILE: os-posix.c:143:
+    lv = strtoul(optarg, &ep, 0); /* can't qemu_strtoul, want *ep=='.' */

total: 1 errors, 0 warnings, 100 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 8/8: configure: do_compiler: Dump some extra info under bash...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v4 0/8] xen: xen-domid-restrict improvements

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1507564902-9000-1-git-send-email-ian.jackson@eu.citrix.com
Subject: [Qemu-devel] [PATCH v4 0/8] xen: xen-domid-restrict improvements

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
2fefd4e512 configure: do_compiler: Dump some extra info under bash
b0ee2db430 os-posix: Provide new -runas <uid>.<gid> facility
bbe6e622ba xen: destroy_hvm_domain: Try xendevicemodel_shutdown
443cfed9da xen: move xc_interface compatibility fallback further up the file
7e9d286c99 xen: destroy_hvm_domain: Move reason into a variable
597a0de500 xen: defer call to xen_restrict until just before os_setup_post
9b5e7d8ef8 xen: restrict: use xentoolcore_restrict_all
54f9b2484b xen: link against xentoolcore

=== OUTPUT BEGIN ===
Checking PATCH 1/8: xen: link against xentoolcore...
Checking PATCH 2/8: xen: restrict: use xentoolcore_restrict_all...
Checking PATCH 3/8: xen: defer call to xen_restrict until just before os_setup_post...
Checking PATCH 4/8: xen: destroy_hvm_domain: Move reason into a variable...
Checking PATCH 5/8: xen: move xc_interface compatibility fallback further up the file...
Checking PATCH 6/8: xen: destroy_hvm_domain: Try xendevicemodel_shutdown...
Checking PATCH 7/8: os-posix: Provide new -runas <uid>.<gid> facility...
ERROR: consider using qemu_strtoul in preference to strtoul
#45: FILE: os-posix.c:143:
+    lv = strtoul(optarg, &ep, 0); /* can't qemu_strtoul, want *ep=='.' */

total: 1 errors, 0 warnings, 100 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 8/8: configure: do_compiler: Dump some extra info under bash...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH 1/8] xen: link against xentoolcore

[Anthony PERARD]

On Mon, Oct 09, 2017 at 05:28:08PM +0100, Ian Jackson wrote:
> Ian Jackson writes ("[PATCH 1/8] xen: link against xentoolcore"):
> > From: Anthony PERARD <anthony.perard@citrix.com>
> > 
> > Xen libraries 4.10 will include a new xentoolcore library, without
> > which xendevicemodel et al will not work.
> 
> The xentoolcore library was just committed to xen.git#staging, so at
> least this patch (or something like it) should go into qemu RSN.

I don't think it is necessary to do anything in qemu. The linker should
find on its own the new libxentoolcore as long as an option
-Wl,-rpath-link= provide the right path to xentoolcore when building
qemu from xen.git.  In other cases, the pkg-config files should be
enough (configure doesn't need to known about a new xentoolcore.pc
file).

> Anthony, do you plan to cherry pick this into the xen-specific qemu
> branch used for osstest tests ?
> 
> Thanks,
> Ian.

-- 
Anthony PERARD

Re: [PATCH 1/8] xen: link against xentoolcore

[Anthony PERARD]

On Mon, Oct 09, 2017 at 05:28:08PM +0100, Ian Jackson wrote:
> Ian Jackson writes ("[PATCH 1/8] xen: link against xentoolcore"):
> > From: Anthony PERARD <anthony.perard@citrix.com>
> > 
> > Xen libraries 4.10 will include a new xentoolcore library, without
> > which xendevicemodel et al will not work.
> 
> The xentoolcore library was just committed to xen.git#staging, so at
> least this patch (or something like it) should go into qemu RSN.

I don't think it is necessary to do anything in qemu. The linker should
find on its own the new libxentoolcore as long as an option
-Wl,-rpath-link= provide the right path to xentoolcore when building
qemu from xen.git.  In other cases, the pkg-config files should be
enough (configure doesn't need to known about a new xentoolcore.pc
file).

> Anthony, do you plan to cherry pick this into the xen-specific qemu
> branch used for osstest tests ?
> 
> Thanks,
> Ian.

-- 
Anthony PERARD

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

Re: [Qemu-devel] [PATCH 2/8] xen: restrict: use xentoolcore_restrict_all

[Anthony PERARD]

On Mon, Oct 09, 2017 at 05:01:36PM +0100, Ian Jackson wrote:
> And insist that it works.
> 
> Drop individual use of xendevicemodel_restrict and
> xenforeignmemory_restrict.  These are not actually effective in this
> version of qemu, because qemu has a large number of fds open onto
> various Xen control devices.
> 
> The restriction arrangements are still not right, because the
> restriction needs to be done very late - after qemu has opened all of
> its control fds.
> 
> xentoolcore_restrict_all and xentoolcore.h are available in Xen 4.10
> and later, only.  Provide a compatibility stub.  And drop the
> compatibility stubs for the old functions.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

Re: [PATCH 2/8] xen: restrict: use xentoolcore_restrict_all

[Anthony PERARD]

On Mon, Oct 09, 2017 at 05:01:36PM +0100, Ian Jackson wrote:
> And insist that it works.
> 
> Drop individual use of xendevicemodel_restrict and
> xenforeignmemory_restrict.  These are not actually effective in this
> version of qemu, because qemu has a large number of fds open onto
> various Xen control devices.
> 
> The restriction arrangements are still not right, because the
> restriction needs to be done very late - after qemu has opened all of
> its control fds.
> 
> xentoolcore_restrict_all and xentoolcore.h are available in Xen 4.10
> and later, only.  Provide a compatibility stub.  And drop the
> compatibility stubs for the old functions.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

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

Re: [Qemu-devel] [PATCH 3/8] xen: defer call to xen_restrict until just before os_setup_post

[Anthony PERARD]

On Mon, Oct 09, 2017 at 05:01:37PM +0100, Ian Jackson wrote:
> We need to restrict *all* the control fds that qemu opens.  Looking in
> /proc/PID/fd shows there are many; their allocation seems scattered
> throughout Xen support code in qemu.
> 
> We must postpone the restrict call until roughly the same time as qemu
> changes its uid, chroots (if applicable), and so on.
> 
> There doesn't seem to be an appropriate hook already.  The RunState
> change hook fires at different times depending on exactly what mode
> qemu is operating in.
> 
> And it appears that no-one but the Xen code wants a hook at this phase
> of execution.  So, introduce a bare call to a new function
> xen_setup_post, just before os_setup_post.  Also provide the
> appropriate stub for when Xen compilation is disabled.
> 
> We do the restriction before rather than after os_setup_post, because
> xen_restrict may need to open /dev/null, and os_setup_post might have
> called chroot.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

Re: [PATCH 3/8] xen: defer call to xen_restrict until just before os_setup_post

[Anthony PERARD]

On Mon, Oct 09, 2017 at 05:01:37PM +0100, Ian Jackson wrote:
> We need to restrict *all* the control fds that qemu opens.  Looking in
> /proc/PID/fd shows there are many; their allocation seems scattered
> throughout Xen support code in qemu.
> 
> We must postpone the restrict call until roughly the same time as qemu
> changes its uid, chroots (if applicable), and so on.
> 
> There doesn't seem to be an appropriate hook already.  The RunState
> change hook fires at different times depending on exactly what mode
> qemu is operating in.
> 
> And it appears that no-one but the Xen code wants a hook at this phase
> of execution.  So, introduce a bare call to a new function
> xen_setup_post, just before os_setup_post.  Also provide the
> appropriate stub for when Xen compilation is disabled.
> 
> We do the restriction before rather than after os_setup_post, because
> xen_restrict may need to open /dev/null, and os_setup_post might have
> called chroot.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

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

Re: [Qemu-devel] [PATCH 5/8] xen: move xc_interface compatibility fallback further up the file

[Anthony PERARD]

On Mon, Oct 09, 2017 at 05:01:39PM +0100, Ian Jackson wrote:
> We are going to want to use the dummy xendevicemodel_handle type in
> new stub functions in the CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
> section.  So we need to provide that definition, or (as applicable)
> include the appropriate header, earlier in the file.
> 
> (Ideally the newer compatibility layers would be at the bottom of the
> file, so that they can naturally benefit from the compatibility layers
> for earlier version.  But that's rather too much for this series.)
> 
> No functional change.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

Re: [PATCH 5/8] xen: move xc_interface compatibility fallback further up the file

[Anthony PERARD]

On Mon, Oct 09, 2017 at 05:01:39PM +0100, Ian Jackson wrote:
> We are going to want to use the dummy xendevicemodel_handle type in
> new stub functions in the CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
> section.  So we need to provide that definition, or (as applicable)
> include the appropriate header, earlier in the file.
> 
> (Ideally the newer compatibility layers would be at the bottom of the
> file, so that they can naturally benefit from the compatibility layers
> for earlier version.  But that's rather too much for this series.)
> 
> No functional change.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

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

Re: [Qemu-devel] [PATCH 6/8] xen: destroy_hvm_domain: Try xendevicemodel_shutdown

[Anthony PERARD]

On Mon, Oct 09, 2017 at 05:01:40PM +0100, Ian Jackson wrote:
> xc_interface_open etc. is not going to work if we have dropped
> privilege, but xendevicemodel_shutdown will if everything is new
> enough.
> 
> xendevicemodel_shutdown is only availabe in Xen 4.10 and later, so
> provide a stub for earlier versions.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

Re: [PATCH 6/8] xen: destroy_hvm_domain: Try xendevicemodel_shutdown

[Anthony PERARD]

On Mon, Oct 09, 2017 at 05:01:40PM +0100, Ian Jackson wrote:
> xc_interface_open etc. is not going to work if we have dropped
> privilege, but xendevicemodel_shutdown will if everything is new
> enough.
> 
> xendevicemodel_shutdown is only availabe in Xen 4.10 and later, so
> provide a stub for earlier versions.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

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

Re: [Qemu-devel] [PATCH 1/8] xen: link against xentoolcore

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 1/8] xen: link against xentoolcore"):
> On Mon, Oct 09, 2017 at 05:28:08PM +0100, Ian Jackson wrote:
> > The xentoolcore library was just committed to xen.git#staging, so at
> > least this patch (or something like it) should go into qemu RSN.
> 
> I don't think it is necessary to do anything in qemu. The linker should
> find on its own the new libxentoolcore as long as an option
> -Wl,-rpath-link= provide the right path to xentoolcore when building
> qemu from xen.git.

But, the -rpath-link= specification in libxendevicemodel refers not to
the xen.git build tree, but to the eventual installed copy.

In my tests, this does in fact break the build.

>  In other cases, the pkg-config files should be
> enough (configure doesn't need to known about a new xentoolcore.pc
> file).

In that case, yes, the .pc works.

Ian.

Re: [PATCH 1/8] xen: link against xentoolcore

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 1/8] xen: link against xentoolcore"):
> On Mon, Oct 09, 2017 at 05:28:08PM +0100, Ian Jackson wrote:
> > The xentoolcore library was just committed to xen.git#staging, so at
> > least this patch (or something like it) should go into qemu RSN.
> 
> I don't think it is necessary to do anything in qemu. The linker should
> find on its own the new libxentoolcore as long as an option
> -Wl,-rpath-link= provide the right path to xentoolcore when building
> qemu from xen.git.

But, the -rpath-link= specification in libxendevicemodel refers not to
the xen.git build tree, but to the eventual installed copy.

In my tests, this does in fact break the build.

>  In other cases, the pkg-config files should be
> enough (configure doesn't need to known about a new xentoolcore.pc
> file).

In that case, yes, the .pc works.

Ian.

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

Re: [Qemu-devel] [PATCH 3/8] xen: defer call to xen_restrict until just before os_setup_post

[Ross Lagerwall]

On 10/09/2017 05:01 PM, Ian Jackson wrote:
> We need to restrict *all* the control fds that qemu opens.  Looking in
> /proc/PID/fd shows there are many; their allocation seems scattered
> throughout Xen support code in qemu.
> 
> We must postpone the restrict call until roughly the same time as qemu
> changes its uid, chroots (if applicable), and so on.
> 
> There doesn't seem to be an appropriate hook already.  The RunState
> change hook fires at different times depending on exactly what mode
> qemu is operating in.
> 
> And it appears that no-one but the Xen code wants a hook at this phase
> of execution.  So, introduce a bare call to a new function
> xen_setup_post, just before os_setup_post.  Also provide the
> appropriate stub for when Xen compilation is disabled.
> 
> We do the restriction before rather than after os_setup_post, because
> xen_restrict may need to open /dev/null, and os_setup_post might have
> called chroot.
> 
This works for normally starting a VM but doesn't seem to work when 
resuming/migrating.

Here is the order of selected operations when starting a VM normally:
     VNC server running on 127.0.0.1:5901
     xen_change_state_handler()
     xenstore_record_dm_state: running
     xen_setup_post()
     xentoolcore_restrict_all: rc = 0
     os_setup_post()
     main_loop()

Here is the order of selected operations when starting QEMU with 
-incoming fd:... :
     VNC server running on 127.0.0.1:5902
     migration_fd_incoming()
     xen_setup_post()
     xentoolcore_restrict_all: rc = 0
     os_setup_post()
     main_loop()
     migration_set_incoming_channel()
     migrate_set_state()
     xen_change_state_handler()
     xenstore_record_dm_state: running
     error recording dm state
     qemu exited with code 1

The issue is that QEMU needs xenstore access to write "running" but this 
is after it has already been restricted. Moving xen_setup_post() into 
xen_change_state_handler() works fine. The only issue is that in the 
migration case, it executes after os_setup_post() so QEMU might be 
chrooted in which case opening /dev/null to restrict fds doesn't work 
(unless its new root has a /dev/null).

-- 
Ross Lagerwall

Re: [PATCH 3/8] xen: defer call to xen_restrict until just before os_setup_post

[Ross Lagerwall]

On 10/09/2017 05:01 PM, Ian Jackson wrote:
> We need to restrict *all* the control fds that qemu opens.  Looking in
> /proc/PID/fd shows there are many; their allocation seems scattered
> throughout Xen support code in qemu.
> 
> We must postpone the restrict call until roughly the same time as qemu
> changes its uid, chroots (if applicable), and so on.
> 
> There doesn't seem to be an appropriate hook already.  The RunState
> change hook fires at different times depending on exactly what mode
> qemu is operating in.
> 
> And it appears that no-one but the Xen code wants a hook at this phase
> of execution.  So, introduce a bare call to a new function
> xen_setup_post, just before os_setup_post.  Also provide the
> appropriate stub for when Xen compilation is disabled.
> 
> We do the restriction before rather than after os_setup_post, because
> xen_restrict may need to open /dev/null, and os_setup_post might have
> called chroot.
> 
This works for normally starting a VM but doesn't seem to work when 
resuming/migrating.

Here is the order of selected operations when starting a VM normally:
     VNC server running on 127.0.0.1:5901
     xen_change_state_handler()
     xenstore_record_dm_state: running
     xen_setup_post()
     xentoolcore_restrict_all: rc = 0
     os_setup_post()
     main_loop()

Here is the order of selected operations when starting QEMU with 
-incoming fd:... :
     VNC server running on 127.0.0.1:5902
     migration_fd_incoming()
     xen_setup_post()
     xentoolcore_restrict_all: rc = 0
     os_setup_post()
     main_loop()
     migration_set_incoming_channel()
     migrate_set_state()
     xen_change_state_handler()
     xenstore_record_dm_state: running
     error recording dm state
     qemu exited with code 1

The issue is that QEMU needs xenstore access to write "running" but this 
is after it has already been restricted. Moving xen_setup_post() into 
xen_change_state_handler() works fine. The only issue is that in the 
migration case, it executes after os_setup_post() so QEMU might be 
chrooted in which case opening /dev/null to restrict fds doesn't work 
(unless its new root has a /dev/null).

-- 
Ross Lagerwall

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

Re: [Qemu-devel] [Xen-devel] [PATCH 3/8] xen: defer call to xen_restrict until just before os_setup_post

[Andrew Cooper]

On 13/10/2017 09:37, Ross Lagerwall wrote:
> On 10/09/2017 05:01 PM, Ian Jackson wrote:
>> We need to restrict *all* the control fds that qemu opens.  Looking in
>> /proc/PID/fd shows there are many; their allocation seems scattered
>> throughout Xen support code in qemu.
>>
>> We must postpone the restrict call until roughly the same time as qemu
>> changes its uid, chroots (if applicable), and so on.
>>
>> There doesn't seem to be an appropriate hook already.  The RunState
>> change hook fires at different times depending on exactly what mode
>> qemu is operating in.
>>
>> And it appears that no-one but the Xen code wants a hook at this phase
>> of execution.  So, introduce a bare call to a new function
>> xen_setup_post, just before os_setup_post.  Also provide the
>> appropriate stub for when Xen compilation is disabled.
>>
>> We do the restriction before rather than after os_setup_post, because
>> xen_restrict may need to open /dev/null, and os_setup_post might have
>> called chroot.
>>
> This works for normally starting a VM but doesn't seem to work when
> resuming/migrating.
>
> Here is the order of selected operations when starting a VM normally:
>     VNC server running on 127.0.0.1:5901
>     xen_change_state_handler()
>     xenstore_record_dm_state: running
>     xen_setup_post()
>     xentoolcore_restrict_all: rc = 0
>     os_setup_post()
>     main_loop()
>
> Here is the order of selected operations when starting QEMU with
> -incoming fd:... :
>     VNC server running on 127.0.0.1:5902
>     migration_fd_incoming()
>     xen_setup_post()
>     xentoolcore_restrict_all: rc = 0
>     os_setup_post()
>     main_loop()
>     migration_set_incoming_channel()
>     migrate_set_state()
>     xen_change_state_handler()
>     xenstore_record_dm_state: running
>     error recording dm state
>     qemu exited with code 1
>
> The issue is that QEMU needs xenstore access to write "running" but
> this is after it has already been restricted. Moving xen_setup_post()
> into xen_change_state_handler() works fine. The only issue is that in
> the migration case, it executes after os_setup_post() so QEMU might be
> chrooted in which case opening /dev/null to restrict fds doesn't work
> (unless its new root has a /dev/null).
>

Wasn't the agreement in the end to remove all use of xenstore from the
device mode?  This running notification can and should be QMP, at which
point we break a causal dependency.

For safety reasons, qemu needs to have restricted/dropped/etc all
permissions before it looks at a single byte of incoming migration data,
to protect against buggy or malicious alterations to the migration stream.

~Andrew

Re: [PATCH 3/8] xen: defer call to xen_restrict until just before os_setup_post

[Andrew Cooper]

On 13/10/2017 09:37, Ross Lagerwall wrote:
> On 10/09/2017 05:01 PM, Ian Jackson wrote:
>> We need to restrict *all* the control fds that qemu opens.  Looking in
>> /proc/PID/fd shows there are many; their allocation seems scattered
>> throughout Xen support code in qemu.
>>
>> We must postpone the restrict call until roughly the same time as qemu
>> changes its uid, chroots (if applicable), and so on.
>>
>> There doesn't seem to be an appropriate hook already.  The RunState
>> change hook fires at different times depending on exactly what mode
>> qemu is operating in.
>>
>> And it appears that no-one but the Xen code wants a hook at this phase
>> of execution.  So, introduce a bare call to a new function
>> xen_setup_post, just before os_setup_post.  Also provide the
>> appropriate stub for when Xen compilation is disabled.
>>
>> We do the restriction before rather than after os_setup_post, because
>> xen_restrict may need to open /dev/null, and os_setup_post might have
>> called chroot.
>>
> This works for normally starting a VM but doesn't seem to work when
> resuming/migrating.
>
> Here is the order of selected operations when starting a VM normally:
>     VNC server running on 127.0.0.1:5901
>     xen_change_state_handler()
>     xenstore_record_dm_state: running
>     xen_setup_post()
>     xentoolcore_restrict_all: rc = 0
>     os_setup_post()
>     main_loop()
>
> Here is the order of selected operations when starting QEMU with
> -incoming fd:... :
>     VNC server running on 127.0.0.1:5902
>     migration_fd_incoming()
>     xen_setup_post()
>     xentoolcore_restrict_all: rc = 0
>     os_setup_post()
>     main_loop()
>     migration_set_incoming_channel()
>     migrate_set_state()
>     xen_change_state_handler()
>     xenstore_record_dm_state: running
>     error recording dm state
>     qemu exited with code 1
>
> The issue is that QEMU needs xenstore access to write "running" but
> this is after it has already been restricted. Moving xen_setup_post()
> into xen_change_state_handler() works fine. The only issue is that in
> the migration case, it executes after os_setup_post() so QEMU might be
> chrooted in which case opening /dev/null to restrict fds doesn't work
> (unless its new root has a /dev/null).
>

Wasn't the agreement in the end to remove all use of xenstore from the
device mode?  This running notification can and should be QMP, at which
point we break a causal dependency.

For safety reasons, qemu needs to have restricted/dropped/etc all
permissions before it looks at a single byte of incoming migration data,
to protect against buggy or malicious alterations to the migration stream.

~Andrew

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

Re: [Qemu-devel] [Xen-devel] [PATCH 3/8] xen: defer call to xen_restrict until just before os_setup_post

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> Andrew Cooper
> Sent: 13 October 2017 10:00
> To: Ross Lagerwall <ross.lagerwall@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; qemu-devel@nongnu.org
> Cc: Anthony Perard <anthony.perard@citrix.com>; Juergen Gross
> <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel@lists.xenproject.org
> Subject: Re: [Xen-devel] [PATCH 3/8] xen: defer call to xen_restrict until just
> before os_setup_post
> 
> On 13/10/2017 09:37, Ross Lagerwall wrote:
> > On 10/09/2017 05:01 PM, Ian Jackson wrote:
> >> We need to restrict *all* the control fds that qemu opens.  Looking in
> >> /proc/PID/fd shows there are many; their allocation seems scattered
> >> throughout Xen support code in qemu.
> >>
> >> We must postpone the restrict call until roughly the same time as qemu
> >> changes its uid, chroots (if applicable), and so on.
> >>
> >> There doesn't seem to be an appropriate hook already.  The RunState
> >> change hook fires at different times depending on exactly what mode
> >> qemu is operating in.
> >>
> >> And it appears that no-one but the Xen code wants a hook at this phase
> >> of execution.  So, introduce a bare call to a new function
> >> xen_setup_post, just before os_setup_post.  Also provide the
> >> appropriate stub for when Xen compilation is disabled.
> >>
> >> We do the restriction before rather than after os_setup_post, because
> >> xen_restrict may need to open /dev/null, and os_setup_post might have
> >> called chroot.
> >>
> > This works for normally starting a VM but doesn't seem to work when
> > resuming/migrating.
> >
> > Here is the order of selected operations when starting a VM normally:
> >     VNC server running on 127.0.0.1:5901
> >     xen_change_state_handler()
> >     xenstore_record_dm_state: running
> >     xen_setup_post()
> >     xentoolcore_restrict_all: rc = 0
> >     os_setup_post()
> >     main_loop()
> >
> > Here is the order of selected operations when starting QEMU with
> > -incoming fd:... :
> >     VNC server running on 127.0.0.1:5902
> >     migration_fd_incoming()
> >     xen_setup_post()
> >     xentoolcore_restrict_all: rc = 0
> >     os_setup_post()
> >     main_loop()
> >     migration_set_incoming_channel()
> >     migrate_set_state()
> >     xen_change_state_handler()
> >     xenstore_record_dm_state: running
> >     error recording dm state
> >     qemu exited with code 1
> >
> > The issue is that QEMU needs xenstore access to write "running" but
> > this is after it has already been restricted. Moving xen_setup_post()
> > into xen_change_state_handler() works fine. The only issue is that in
> > the migration case, it executes after os_setup_post() so QEMU might be
> > chrooted in which case opening /dev/null to restrict fds doesn't work
> > (unless its new root has a /dev/null).
> >
> 
> Wasn't the agreement in the end to remove all use of xenstore from the
> device mode?  This running notification can and should be QMP, at which
> point we break a causal dependency.
> 

Yes, that was the agreement. One problem is that there is not yet adequate separation in either QEMU's common and pv/hvm init routines to do this yet. Nor, I believe, is there support in libxl to spawn separate xenpv and xenfv instances of QEMU for the same guest.

  Paul

> For safety reasons, qemu needs to have restricted/dropped/etc all
> permissions before it looks at a single byte of incoming migration data,
> to protect against buggy or malicious alterations to the migration stream.
> 
> ~Andrew
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

Re: [PATCH 3/8] xen: defer call to xen_restrict until just before os_setup_post

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> Andrew Cooper
> Sent: 13 October 2017 10:00
> To: Ross Lagerwall <ross.lagerwall@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; qemu-devel@nongnu.org
> Cc: Anthony Perard <anthony.perard@citrix.com>; Juergen Gross
> <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel@lists.xenproject.org
> Subject: Re: [Xen-devel] [PATCH 3/8] xen: defer call to xen_restrict until just
> before os_setup_post
> 
> On 13/10/2017 09:37, Ross Lagerwall wrote:
> > On 10/09/2017 05:01 PM, Ian Jackson wrote:
> >> We need to restrict *all* the control fds that qemu opens.  Looking in
> >> /proc/PID/fd shows there are many; their allocation seems scattered
> >> throughout Xen support code in qemu.
> >>
> >> We must postpone the restrict call until roughly the same time as qemu
> >> changes its uid, chroots (if applicable), and so on.
> >>
> >> There doesn't seem to be an appropriate hook already.  The RunState
> >> change hook fires at different times depending on exactly what mode
> >> qemu is operating in.
> >>
> >> And it appears that no-one but the Xen code wants a hook at this phase
> >> of execution.  So, introduce a bare call to a new function
> >> xen_setup_post, just before os_setup_post.  Also provide the
> >> appropriate stub for when Xen compilation is disabled.
> >>
> >> We do the restriction before rather than after os_setup_post, because
> >> xen_restrict may need to open /dev/null, and os_setup_post might have
> >> called chroot.
> >>
> > This works for normally starting a VM but doesn't seem to work when
> > resuming/migrating.
> >
> > Here is the order of selected operations when starting a VM normally:
> >     VNC server running on 127.0.0.1:5901
> >     xen_change_state_handler()
> >     xenstore_record_dm_state: running
> >     xen_setup_post()
> >     xentoolcore_restrict_all: rc = 0
> >     os_setup_post()
> >     main_loop()
> >
> > Here is the order of selected operations when starting QEMU with
> > -incoming fd:... :
> >     VNC server running on 127.0.0.1:5902
> >     migration_fd_incoming()
> >     xen_setup_post()
> >     xentoolcore_restrict_all: rc = 0
> >     os_setup_post()
> >     main_loop()
> >     migration_set_incoming_channel()
> >     migrate_set_state()
> >     xen_change_state_handler()
> >     xenstore_record_dm_state: running
> >     error recording dm state
> >     qemu exited with code 1
> >
> > The issue is that QEMU needs xenstore access to write "running" but
> > this is after it has already been restricted. Moving xen_setup_post()
> > into xen_change_state_handler() works fine. The only issue is that in
> > the migration case, it executes after os_setup_post() so QEMU might be
> > chrooted in which case opening /dev/null to restrict fds doesn't work
> > (unless its new root has a /dev/null).
> >
> 
> Wasn't the agreement in the end to remove all use of xenstore from the
> device mode?  This running notification can and should be QMP, at which
> point we break a causal dependency.
> 

Yes, that was the agreement. One problem is that there is not yet adequate separation in either QEMU's common and pv/hvm init routines to do this yet. Nor, I believe, is there support in libxl to spawn separate xenpv and xenfv instances of QEMU for the same guest.

  Paul

> For safety reasons, qemu needs to have restricted/dropped/etc all
> permissions before it looks at a single byte of incoming migration data,
> to protect against buggy or malicious alterations to the migration stream.
> 
> ~Andrew
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH 3/8] xen: defer call to xen_restrict until just before os_setup_post

[Ian Jackson]

Ross Lagerwall writes ("Re: [PATCH 3/8] xen: defer call to xen_restrict until just before os_setup_post"):
> This works for normally starting a VM but doesn't seem to work when 
> resuming/migrating.
> 
> Here is the order of selected operations when starting a VM normally:
>      VNC server running on 127.0.0.1:5901
>      xen_change_state_handler()
>      xenstore_record_dm_state: running
>      xen_setup_post()
>      xentoolcore_restrict_all: rc = 0
>      os_setup_post()
>      main_loop()
> 
> Here is the order of selected operations when starting QEMU with 
> -incoming fd:... :
>      VNC server running on 127.0.0.1:5902
>      migration_fd_incoming()
>      xen_setup_post()
>      xentoolcore_restrict_all: rc = 0
>      os_setup_post()
>      main_loop()
>      migration_set_incoming_channel()
>      migrate_set_state()
>      xen_change_state_handler()
>      xenstore_record_dm_state: running
>      error recording dm state
>      qemu exited with code 1
> 
> The issue is that QEMU needs xenstore access to write "running" but this 
> is after it has already been restricted. Moving xen_setup_post() into 
> xen_change_state_handler() works fine. The only issue is that in the 
> migration case, it executes after os_setup_post() so QEMU might be 
> chrooted in which case opening /dev/null to restrict fds doesn't work 
> (unless its new root has a /dev/null).

Thanks for the extensive diagnosis.

We do in fact want the restriction to occur before the migration
stream is read.  This is because we are trying to defend against a
guest which can exploit a bug in qemu.  That means that the sending
qemu must be assumed to be compromised.  In this context I don't think
qemu's migration stream receiver can be regarded as hardened against
hostile input; it's far too complicated, even if efforts have been
made in that direction (I haven't checked).  So to avoid the receiving
qemu being compromised while still unrestricted, we should restrict
before starting to read the migration stream.

The correct fix is to use a different technique to notify the
toolstack that qemu is up and running.  That obviously requires
changes on the Xen tools side.  I will look into that for the Xen 4.11
release cycle.

Ian.

Re: [PATCH 3/8] xen: defer call to xen_restrict until just before os_setup_post

[Ian Jackson]

Ross Lagerwall writes ("Re: [PATCH 3/8] xen: defer call to xen_restrict until just before os_setup_post"):
> This works for normally starting a VM but doesn't seem to work when 
> resuming/migrating.
> 
> Here is the order of selected operations when starting a VM normally:
>      VNC server running on 127.0.0.1:5901
>      xen_change_state_handler()
>      xenstore_record_dm_state: running
>      xen_setup_post()
>      xentoolcore_restrict_all: rc = 0
>      os_setup_post()
>      main_loop()
> 
> Here is the order of selected operations when starting QEMU with 
> -incoming fd:... :
>      VNC server running on 127.0.0.1:5902
>      migration_fd_incoming()
>      xen_setup_post()
>      xentoolcore_restrict_all: rc = 0
>      os_setup_post()
>      main_loop()
>      migration_set_incoming_channel()
>      migrate_set_state()
>      xen_change_state_handler()
>      xenstore_record_dm_state: running
>      error recording dm state
>      qemu exited with code 1
> 
> The issue is that QEMU needs xenstore access to write "running" but this 
> is after it has already been restricted. Moving xen_setup_post() into 
> xen_change_state_handler() works fine. The only issue is that in the 
> migration case, it executes after os_setup_post() so QEMU might be 
> chrooted in which case opening /dev/null to restrict fds doesn't work 
> (unless its new root has a /dev/null).

Thanks for the extensive diagnosis.

We do in fact want the restriction to occur before the migration
stream is read.  This is because we are trying to defend against a
guest which can exploit a bug in qemu.  That means that the sending
qemu must be assumed to be compromised.  In this context I don't think
qemu's migration stream receiver can be regarded as hardened against
hostile input; it's far too complicated, even if efforts have been
made in that direction (I haven't checked).  So to avoid the receiving
qemu being compromised while still unrestricted, we should restrict
before starting to read the migration stream.

The correct fix is to use a different technique to notify the
toolstack that qemu is up and running.  That obviously requires
changes on the Xen tools side.  I will look into that for the Xen 4.11
release cycle.

Ian.

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

Re: [Qemu-devel] [PATCH 1/8] xen: link against xentoolcore

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 1/8] xen: link against xentoolcore"):
> I don't think it is necessary to do anything in qemu. The linker should
> find on its own the new libxentoolcore as long as an option
> -Wl,-rpath-link= provide the right path to xentoolcore when building
> qemu from xen.git.  In other cases, the pkg-config files should be
> enough (configure doesn't need to known about a new xentoolcore.pc
> file).

It seems that the build still works, without this patch but without
the rest of my series too, because we end up passing
  -L/u/iwj/work/xen.git/tools/../tools/libs/toolcore
  -Wl,-rpath-link=/u/iwj/work/xen.git/tools/../tools/libs/toolcore
(or similar) on the qemu link line.  So when ld links against
libxenstore (say) it finds that xenstore needs toolcore and finds
toolcore in the relevant paths.

We still need this patch for the rest of the series, though.

Ian.

Re: [PATCH 1/8] xen: link against xentoolcore

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 1/8] xen: link against xentoolcore"):
> I don't think it is necessary to do anything in qemu. The linker should
> find on its own the new libxentoolcore as long as an option
> -Wl,-rpath-link= provide the right path to xentoolcore when building
> qemu from xen.git.  In other cases, the pkg-config files should be
> enough (configure doesn't need to known about a new xentoolcore.pc
> file).

It seems that the build still works, without this patch but without
the rest of my series too, because we end up passing
  -L/u/iwj/work/xen.git/tools/../tools/libs/toolcore
  -Wl,-rpath-link=/u/iwj/work/xen.git/tools/../tools/libs/toolcore
(or similar) on the qemu link line.  So when ld links against
libxenstore (say) it finds that xenstore needs toolcore and finds
toolcore in the relevant paths.

We still need this patch for the rest of the series, though.

Ian.

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

Re: [Qemu-devel] [PATCH 1/8] xen: link against xentoolcore

[Anthony PERARD]

On Thu, Oct 19, 2017 at 05:38:10PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [PATCH 1/8] xen: link against xentoolcore"):
> > I don't think it is necessary to do anything in qemu. The linker should
> > find on its own the new libxentoolcore as long as an option
> > -Wl,-rpath-link= provide the right path to xentoolcore when building
> > qemu from xen.git.  In other cases, the pkg-config files should be
> > enough (configure doesn't need to known about a new xentoolcore.pc
> > file).
> 
> It seems that the build still works, without this patch but without
> the rest of my series too, because we end up passing
>   -L/u/iwj/work/xen.git/tools/../tools/libs/toolcore
>   -Wl,-rpath-link=/u/iwj/work/xen.git/tools/../tools/libs/toolcore
> (or similar) on the qemu link line.  So when ld links against
> libxenstore (say) it finds that xenstore needs toolcore and finds
> toolcore in the relevant paths.
> 
> We still need this patch for the rest of the series, though.

Of course, I was only arguing that this patch on its own is not usefull.

Do you need a signed-off-by or review-by from me? Since it looks like
I'm the author.

-- 
Anthony PERARD

Re: [PATCH 1/8] xen: link against xentoolcore

[Anthony PERARD]

On Thu, Oct 19, 2017 at 05:38:10PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [PATCH 1/8] xen: link against xentoolcore"):
> > I don't think it is necessary to do anything in qemu. The linker should
> > find on its own the new libxentoolcore as long as an option
> > -Wl,-rpath-link= provide the right path to xentoolcore when building
> > qemu from xen.git.  In other cases, the pkg-config files should be
> > enough (configure doesn't need to known about a new xentoolcore.pc
> > file).
> 
> It seems that the build still works, without this patch but without
> the rest of my series too, because we end up passing
>   -L/u/iwj/work/xen.git/tools/../tools/libs/toolcore
>   -Wl,-rpath-link=/u/iwj/work/xen.git/tools/../tools/libs/toolcore
> (or similar) on the qemu link line.  So when ld links against
> libxenstore (say) it finds that xenstore needs toolcore and finds
> toolcore in the relevant paths.
> 
> We still need this patch for the rest of the series, though.

Of course, I was only arguing that this patch on its own is not usefull.

Do you need a signed-off-by or review-by from me? Since it looks like
I'm the author.

-- 
Anthony PERARD

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

Re: [Qemu-devel] [PATCH 1/8] xen: link against xentoolcore

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 1/8] xen: link against xentoolcore"):
> On Thu, Oct 19, 2017 at 05:38:10PM +0100, Ian Jackson wrote:
> > We still need this patch for the rest of the series, though.
> 
> Of course, I was only arguing that this patch on its own is not usefull.

Right.  I wanted to accurately reflect the situation in the commit
message, hence the explanation.

> Do you need a signed-off-by or review-by from me? Since it looks like
> I'm the author.

Indeed, so I don't think it makes sense for you to say you've reviewed
it.

Ian.

Re: [PATCH 1/8] xen: link against xentoolcore

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 1/8] xen: link against xentoolcore"):
> On Thu, Oct 19, 2017 at 05:38:10PM +0100, Ian Jackson wrote:
> > We still need this patch for the rest of the series, though.
> 
> Of course, I was only arguing that this patch on its own is not usefull.

Right.  I wanted to accurately reflect the situation in the commit
message, hence the explanation.

> Do you need a signed-off-by or review-by from me? Since it looks like
> I'm the author.

Indeed, so I don't think it makes sense for you to say you've reviewed
it.

Ian.

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