[Qemu-devel] [PATCH v2 0/*] xen: xen-domid-restrict improvements

[Qemu-devel] [PATCH v2 0/*] xen: xen-domid-restrict improvements

[Ian Jackson]

(Resending this because 1. I got the CC for xen-devel wrong; 2. I got
the subject wrong: there are actually 8 patches; 3. I mangled
Anthony's name in theheaders.  Sorry for the noise.)

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 will be
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 very much to Anthony Perard for his review of the first, RFC,
version, and for helping out with configure.

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.

Ian.

[PATCH v2 0/*] xen: xen-domid-restrict improvements

[Ian Jackson]

(Resending this because 1. I got the CC for xen-devel wrong; 2. I got
the subject wrong: there are actually 8 patches; 3. I mangled
Anthony's name in theheaders.  Sorry for the noise.)

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 will be
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 very much to Anthony Perard for his review of the first, RFC,
version, and for helping out with configure.

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.

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 after 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 after os_setup_post.  Also provide the
appropriate stub for when Xen compilation is disabled.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 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..9e7d541 100644
--- a/vl.c
+++ b/vl.c
@@ -4793,6 +4793,7 @@ int main(int argc, char **argv, char **envp)
     }
 
     os_setup_post();
+    xen_setup_post();
 
     main_loop();
     replay_disable_events();
-- 
2.1.4

[PATCH 3/8] xen: defer call to xen_restrict until after 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 after os_setup_post.  Also provide the
appropriate stub for when Xen compilation is disabled.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 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..9e7d541 100644
--- a/vl.c
+++ b/vl.c
@@ -4793,6 +4793,7 @@ int main(int argc, char **argv, char **envp)
     }
 
     os_setup_post();
+    xen_setup_post();
 
     main_loop();
     replay_disable_events();
-- 
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 -runasid option

[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.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v2: Coding style fixes.
---
 os-posix.c      | 31 +++++++++++++++++++++++++++----
 qemu-options.hx | 12 ++++++++++++
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index 92e9d85..d63680b 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;
@@ -134,6 +136,9 @@ void os_set_proc_name(const char *s)
  */
 void os_parse_cmd_args(int index, const char *optarg)
 {
+    unsigned long lv;
+    char *ep;
+    int rc;
     switch (index) {
 #ifdef CONFIG_SLIRP
     case QEMU_OPTION_smb:
@@ -150,6 +155,22 @@ void os_parse_cmd_args(int index, const char *optarg)
             exit(1);
         }
         break;
+    case QEMU_OPTION_runasid:
+        errno = 0;
+        lv = strtoul(optarg, &ep, 0); /* can't qemu_strtoul, want *ep=='.' */
+        user_uid = lv; /* overflow here is ID in C99 */
+        if (errno || *ep != '.' || user_uid != lv || user_uid == (uid_t)-1) {
+            fprintf(stderr, "Could not obtain uid from \"%s\"", optarg);
+            exit(1);
+        }
+        lv = 0;
+        rc = qemu_strtoul(ep + 1, 0, 0, &lv);
+        user_gid = lv; /* overflow here is ID in C99 */
+        if (rc || user_gid != lv || user_gid == (gid_t)-1) {
+            fprintf(stderr, "Could not obtain gid from \"%s\"", optarg);
+            exit(1);
+        }
+        break;
     case QEMU_OPTION_chroot:
         chroot_dir = optarg;
         break;
@@ -166,17 +187,19 @@ 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) {
+    if (user_pwd || user_uid != (uid_t)-1) {
+        if (setgid(user_pwd ? user_pwd->pw_gid : user_gid) < 0) {
             fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
             exit(1);
         }
-        if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
+        if ((user_pwd
+             ? initgroups(user_pwd->pw_name, user_pwd->pw_gid)
+             : setgroups(1, &user_gid)) < 0) {
             fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
                     user_pwd->pw_name, user_pwd->pw_gid);
             exit(1);
         }
-        if (setuid(user_pwd->pw_uid) < 0) {
+        if (setuid(user_pwd ? user_pwd->pw_uid : user_gid) < 0) {
             fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
             exit(1);
         }
diff --git a/qemu-options.hx b/qemu-options.hx
index 9f6e2ad..34a5329 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3968,6 +3968,18 @@ Immediately before starting guest execution, drop root privileges, switching
 to the specified user.
 ETEXI
 
+#ifndef _WIN32
+DEF("runasid", HAS_ARG, QEMU_OPTION_runasid, \
+    "-runasid uid.gid     change to numeric uid and gid just before starting the VM\n",
+    QEMU_ARCH_ALL)
+#endif
+STEXI
+@item -runasid @var{uid}.@var{gid}
+@findex -runasid
+Immediately before starting guest execution, drop root privileges, switching
+to the specified uid and gid.
+ETEXI
+
 DEF("prom-env", HAS_ARG, QEMU_OPTION_prom_env,
     "-prom-env variable=value\n"
     "                set OpenBIOS nvram variables\n",
-- 
2.1.4

[PATCH 7/8] os-posix: Provide new -runasid option

[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.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v2: Coding style fixes.
---
 os-posix.c      | 31 +++++++++++++++++++++++++++----
 qemu-options.hx | 12 ++++++++++++
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index 92e9d85..d63680b 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;
@@ -134,6 +136,9 @@ void os_set_proc_name(const char *s)
  */
 void os_parse_cmd_args(int index, const char *optarg)
 {
+    unsigned long lv;
+    char *ep;
+    int rc;
     switch (index) {
 #ifdef CONFIG_SLIRP
     case QEMU_OPTION_smb:
@@ -150,6 +155,22 @@ void os_parse_cmd_args(int index, const char *optarg)
             exit(1);
         }
         break;
+    case QEMU_OPTION_runasid:
+        errno = 0;
+        lv = strtoul(optarg, &ep, 0); /* can't qemu_strtoul, want *ep=='.' */
+        user_uid = lv; /* overflow here is ID in C99 */
+        if (errno || *ep != '.' || user_uid != lv || user_uid == (uid_t)-1) {
+            fprintf(stderr, "Could not obtain uid from \"%s\"", optarg);
+            exit(1);
+        }
+        lv = 0;
+        rc = qemu_strtoul(ep + 1, 0, 0, &lv);
+        user_gid = lv; /* overflow here is ID in C99 */
+        if (rc || user_gid != lv || user_gid == (gid_t)-1) {
+            fprintf(stderr, "Could not obtain gid from \"%s\"", optarg);
+            exit(1);
+        }
+        break;
     case QEMU_OPTION_chroot:
         chroot_dir = optarg;
         break;
@@ -166,17 +187,19 @@ 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) {
+    if (user_pwd || user_uid != (uid_t)-1) {
+        if (setgid(user_pwd ? user_pwd->pw_gid : user_gid) < 0) {
             fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
             exit(1);
         }
-        if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
+        if ((user_pwd
+             ? initgroups(user_pwd->pw_name, user_pwd->pw_gid)
+             : setgroups(1, &user_gid)) < 0) {
             fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
                     user_pwd->pw_name, user_pwd->pw_gid);
             exit(1);
         }
-        if (setuid(user_pwd->pw_uid) < 0) {
+        if (setuid(user_pwd ? user_pwd->pw_uid : user_gid) < 0) {
             fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
             exit(1);
         }
diff --git a/qemu-options.hx b/qemu-options.hx
index 9f6e2ad..34a5329 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3968,6 +3968,18 @@ Immediately before starting guest execution, drop root privileges, switching
 to the specified user.
 ETEXI
 
+#ifndef _WIN32
+DEF("runasid", HAS_ARG, QEMU_OPTION_runasid, \
+    "-runasid uid.gid     change to numeric uid and gid just before starting the VM\n",
+    QEMU_ARCH_ALL)
+#endif
+STEXI
+@item -runasid @var{uid}.@var{gid}
+@findex -runasid
+Immediately before starting guest execution, drop root privileges, switching
+to the specified uid and gid.
+ETEXI
+
 DEF("prom-env", HAS_ARG, QEMU_OPTION_prom_env,
     "-prom-env variable=value\n"
     "                set OpenBIOS nvram variables\n",
-- 
2.1.4


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

[Qemu-devel] [PATCH 8/8] RFC 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>
---
 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] RFC 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>
---
 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 7/8] os-posix: Provide new -runasid option

[Ross Lagerwall]

On 10/04/2017 05:18 PM, Ian Jackson wrote:
> 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.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
snip
> @@ -166,17 +187,19 @@ 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) {
> +    if (user_pwd || user_uid != (uid_t)-1) {
> +        if (setgid(user_pwd ? user_pwd->pw_gid : user_gid) < 0) {
>               fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
>               exit(1);
>           }
> -        if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
> +        if ((user_pwd
> +             ? initgroups(user_pwd->pw_name, user_pwd->pw_gid)
> +             : setgroups(1, &user_gid)) < 0) {
>               fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
>                       user_pwd->pw_name, user_pwd->pw_gid);
>               exit(1);
>           }
> -        if (setuid(user_pwd->pw_uid) < 0) {
> +        if (setuid(user_pwd ? user_pwd->pw_uid : user_gid) < 0) {
>               fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
>               exit(1);
>           }

This last one should be user_uid, not user_gid.

-- 
Ross Lagerwall

Re: [PATCH 7/8] os-posix: Provide new -runasid option

[Ross Lagerwall]

On 10/04/2017 05:18 PM, Ian Jackson wrote:
> 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.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
snip
> @@ -166,17 +187,19 @@ 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) {
> +    if (user_pwd || user_uid != (uid_t)-1) {
> +        if (setgid(user_pwd ? user_pwd->pw_gid : user_gid) < 0) {
>               fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
>               exit(1);
>           }
> -        if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
> +        if ((user_pwd
> +             ? initgroups(user_pwd->pw_name, user_pwd->pw_gid)
> +             : setgroups(1, &user_gid)) < 0) {
>               fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
>                       user_pwd->pw_name, user_pwd->pw_gid);
>               exit(1);
>           }
> -        if (setuid(user_pwd->pw_uid) < 0) {
> +        if (setuid(user_pwd ? user_pwd->pw_uid : user_gid) < 0) {
>               fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
>               exit(1);
>           }

This last one should be user_uid, not user_gid.

-- 
Ross Lagerwall

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

Re: [Qemu-devel] [PATCH v2 0/*] xen: xen-domid-restrict improvements

[Ross Lagerwall]

On 10/04/2017 05:18 PM, Ian Jackson wrote:
> (Resending this because 1. I got the CC for xen-devel wrong; 2. I got
> the subject wrong: there are actually 8 patches; 3. I mangled
> Anthony's name in theheaders.  Sorry for the noise.)
> 
> 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 will be
> 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.
> 

Hi Ian,

I'm testing your QEMU and Xen patch series and found that after being 
restricted, QEMU fails to setup up the VGA memory properly which causes 
a complete stall with stdvga. With cirrus it mostly works although it 
seems to have reduced performance.

I think it happens when the VM sets up the BAR some time after 
xen_restrict() has been called. The failure comes from QEMU calling 
xc_domain_add_to_physmap() which calls do_memory_op() and finally 
xencall2(). But the underlying xencall fd has been replaced with /dev/null.

Here is some debug output, all of which occurs after the call to
xentoolcore_restrict_all() has completed. 00:02.0 is the VGA device.

pci_update_mappings_add d=0x7fee0b154000 00:03.0 1,0xf0000000+0x1000000
pci_update_mappings_add d=0x7fee0b1de000 00:02.0 0,0xf1000000+0x800000
xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
xen: mapping vram to f1000000 - f1800000
xen: add_to_physmap MFN 80000 to PFN f1000 failed: 25 (errno: 25)
pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
pci_update_mappings_add d=0x7fee0b1a4000 00:04.0 0,0xc100+0x100
pci_update_mappings_add d=0x7fee0b1a4000 00:04.0 1,0xf1800000+0x100
pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
pci_update_mappings_add d=0x7fee0b1de000 00:02.0 0,0xf1000000+0x800000
xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
pci_update_mappings_add d=0x7fee0b1de000 00:02.0 0,0xf1000000+0x800000
xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
pci_update_mappings_add d=0x7fee0b1de000 00:02.0 0,0xf1000000+0x800000
xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
pci_update_mappings_add d=0x7fee0b1de000 00:02.0 0,0xf1000000+0x800000
xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
pci_update_mappings_add d=0x7fee0b1de000 00:02.0 0,0xf1000000+0x800000
xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
pci_update_mappings_add d=0x7fee0b1de000 00:02.0 0,0xf1000000+0x800000
xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
pci_update_mappings_add d=0x7fee0b1de000 00:02.0 0,0xf1000000+0x800000
xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
pci_update_mappings_add d=0x7fee0b154000 00:03.0 1,0xf0000000+0x1000000
pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
pci_update_mappings_add d=0x7fee0b154000 00:03.0 1,0xf0000000+0x1000000
pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
pci_update_mappings_add d=0x7fee0b154000 00:03.0 1,0xf0000000+0x1000000
pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
pci_update_mappings_add d=0x7fee0b154000 00:03.0 1,0xf0000000+0x1000000
pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
pci_update_mappings_add d=0x7fee0b154000 00:03.0 1,0xf0000000+0x1000000
pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
pci_update_mappings_add d=0x7fee0b154000 00:03.0 1,0xf0000000+0x1000000
pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
pci_update_mappings_add d=0x7fee0b154000 00:03.0 1,0xf0000000+0x1000000

Apart from this issue, I was able to boot up and shut down a VM, using 
-xen-domid-restrict, -chroot, and -runasid.

There is a caveat when using -xen-domid-restrict and -chroot at the same 
time. The restriction happens after chrooting, so the chroot directory 
has to contain a valid /dev/null. This is a bit annoying and prevents 
the chroot being on a "nodev" mount.

Regards,
-- 
Ross Lagerwall

Re: [PATCH v2 0/*] xen: xen-domid-restrict improvements

[Ross Lagerwall]

On 10/04/2017 05:18 PM, Ian Jackson wrote:
> (Resending this because 1. I got the CC for xen-devel wrong; 2. I got
> the subject wrong: there are actually 8 patches; 3. I mangled
> Anthony's name in theheaders.  Sorry for the noise.)
> 
> 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 will be
> 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.
> 

Hi Ian,

I'm testing your QEMU and Xen patch series and found that after being 
restricted, QEMU fails to setup up the VGA memory properly which causes 
a complete stall with stdvga. With cirrus it mostly works although it 
seems to have reduced performance.

I think it happens when the VM sets up the BAR some time after 
xen_restrict() has been called. The failure comes from QEMU calling 
xc_domain_add_to_physmap() which calls do_memory_op() and finally 
xencall2(). But the underlying xencall fd has been replaced with /dev/null.

Here is some debug output, all of which occurs after the call to
xentoolcore_restrict_all() has completed. 00:02.0 is the VGA device.

pci_update_mappings_add d=0x7fee0b154000 00:03.0 1,0xf0000000+0x1000000
pci_update_mappings_add d=0x7fee0b1de000 00:02.0 0,0xf1000000+0x800000
xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
xen: mapping vram to f1000000 - f1800000
xen: add_to_physmap MFN 80000 to PFN f1000 failed: 25 (errno: 25)
pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
pci_update_mappings_add d=0x7fee0b1a4000 00:04.0 0,0xc100+0x100
pci_update_mappings_add d=0x7fee0b1a4000 00:04.0 1,0xf1800000+0x100
pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
pci_update_mappings_add d=0x7fee0b1de000 00:02.0 0,0xf1000000+0x800000
xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
pci_update_mappings_add d=0x7fee0b1de000 00:02.0 0,0xf1000000+0x800000
xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
pci_update_mappings_add d=0x7fee0b1de000 00:02.0 0,0xf1000000+0x800000
xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
pci_update_mappings_add d=0x7fee0b1de000 00:02.0 0,0xf1000000+0x800000
xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
pci_update_mappings_add d=0x7fee0b1de000 00:02.0 0,0xf1000000+0x800000
xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
pci_update_mappings_add d=0x7fee0b1de000 00:02.0 0,0xf1000000+0x800000
xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
pci_update_mappings_add d=0x7fee0b1de000 00:02.0 0,0xf1000000+0x800000
xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
pci_update_mappings_add d=0x7fee0b154000 00:03.0 1,0xf0000000+0x1000000
pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
pci_update_mappings_add d=0x7fee0b154000 00:03.0 1,0xf0000000+0x1000000
pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
pci_update_mappings_add d=0x7fee0b154000 00:03.0 1,0xf0000000+0x1000000
pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
pci_update_mappings_add d=0x7fee0b154000 00:03.0 1,0xf0000000+0x1000000
pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
pci_update_mappings_add d=0x7fee0b154000 00:03.0 1,0xf0000000+0x1000000
pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
pci_update_mappings_add d=0x7fee0b154000 00:03.0 1,0xf0000000+0x1000000
pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
pci_update_mappings_add d=0x7fee0b154000 00:03.0 1,0xf0000000+0x1000000

Apart from this issue, I was able to boot up and shut down a VM, using 
-xen-domid-restrict, -chroot, and -runasid.

There is a caveat when using -xen-domid-restrict and -chroot at the same 
time. The restriction happens after chrooting, so the chroot directory 
has to contain a valid /dev/null. This is a bit annoying and prevents 
the chroot being on a "nodev" mount.

Regards,
-- 
Ross Lagerwall

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

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Peter Maydell]

On 4 October 2017 at 17:18, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
> 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.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---


> @@ -166,17 +187,19 @@ 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) {
> +    if (user_pwd || user_uid != (uid_t)-1) {
> +        if (setgid(user_pwd ? user_pwd->pw_gid : user_gid) < 0) {
>              fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);

If you're changing the gid we pass to setgid() I think you should
also change the value we tell the user we tried to use in the
error message, or it could be rather confusing.

>              exit(1);
>          }
> -        if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
> +        if ((user_pwd
> +             ? initgroups(user_pwd->pw_name, user_pwd->pw_gid)
> +             : setgroups(1, &user_gid)) < 0) {
>              fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
>                      user_pwd->pw_name, user_pwd->pw_gid);

...and here we might claim we failed initgroups() when we actually
failed setgroups().

>              exit(1);
>          }
> -        if (setuid(user_pwd->pw_uid) < 0) {
> +        if (setuid(user_pwd ? user_pwd->pw_uid : user_gid) < 0) {
>              fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);

This error message should be updated too.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Peter Maydell]

On 4 October 2017 at 17:18, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
> 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.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---


> @@ -166,17 +187,19 @@ 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) {
> +    if (user_pwd || user_uid != (uid_t)-1) {
> +        if (setgid(user_pwd ? user_pwd->pw_gid : user_gid) < 0) {
>              fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);

If you're changing the gid we pass to setgid() I think you should
also change the value we tell the user we tried to use in the
error message, or it could be rather confusing.

>              exit(1);
>          }
> -        if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
> +        if ((user_pwd
> +             ? initgroups(user_pwd->pw_name, user_pwd->pw_gid)
> +             : setgroups(1, &user_gid)) < 0) {
>              fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
>                      user_pwd->pw_name, user_pwd->pw_gid);

...and here we might claim we failed initgroups() when we actually
failed setgroups().

>              exit(1);
>          }
> -        if (setuid(user_pwd->pw_uid) < 0) {
> +        if (setuid(user_pwd ? user_pwd->pw_uid : user_gid) < 0) {
>              fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);

This error message should be updated too.

thanks
-- PMM

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

Re: [Qemu-devel] [Xen-devel] [PATCH v2 0/*] xen: xen-domid-restrict improvements

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> Ross Lagerwall
> Sent: 06 October 2017 13:58
> To: Ian Jackson <Ian.Jackson@citrix.com>; qemu-devel@nongnu.org
> Cc: Anthony Perard <anthony.perard@citrix.com>; xen-
> devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>;
> Juergen Gross <jgross@suse.com>
> Subject: Re: [Xen-devel] [PATCH v2 0/*] xen: xen-domid-restrict
> improvements
> 
> On 10/04/2017 05:18 PM, Ian Jackson wrote:
> > (Resending this because 1. I got the CC for xen-devel wrong; 2. I got
> > the subject wrong: there are actually 8 patches; 3. I mangled
> > Anthony's name in theheaders.  Sorry for the noise.)
> >
> > 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 will be
> > 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.
> >
> 
> Hi Ian,
> 
> I'm testing your QEMU and Xen patch series and found that after being
> restricted, QEMU fails to setup up the VGA memory properly which causes
> a complete stall with stdvga. With cirrus it mostly works although it
> seems to have reduced performance.
> 
> I think it happens when the VM sets up the BAR some time after
> xen_restrict() has been called. The failure comes from QEMU calling
> xc_domain_add_to_physmap() which calls do_memory_op() and finally
> xencall2(). But the underlying xencall fd has been replaced with /dev/null.

Oh, that's a PITA. This is because of the slightly hacky way that the VRAM is dealt with (it needing to be moved into the BAR of the PCI VGA device at the point where guest firmware programs it). Maybe we need a new dm_op for this?

  Paul

> 
> Here is some debug output, all of which occurs after the call to
> xentoolcore_restrict_all() has completed. 00:02.0 is the VGA device.
> 
> pci_update_mappings_add d=0x7fee0b154000 00:03.0
> 1,0xf0000000+0x1000000
> pci_update_mappings_add d=0x7fee0b1de000 00:02.0
> 0,0xf1000000+0x800000
> xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
> xen: mapping vram to f1000000 - f1800000
> xen: add_to_physmap MFN 80000 to PFN f1000 failed: 25 (errno: 25)
> pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
> pci_update_mappings_add d=0x7fee0b1a4000 00:04.0 0,0xc100+0x100
> pci_update_mappings_add d=0x7fee0b1a4000 00:04.0 1,0xf1800000+0x100
> pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
> pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
> pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
> pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
> pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
> pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
> pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
> pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
> pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
> pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
> pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
> pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
> pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
> pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
> pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
> pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
> pci_update_mappings_add d=0x7fee0b1de000 00:02.0
> 0,0xf1000000+0x800000
> xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
> pci_update_mappings_add d=0x7fee0b1de000 00:02.0
> 0,0xf1000000+0x800000
> xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
> pci_update_mappings_add d=0x7fee0b1de000 00:02.0
> 0,0xf1000000+0x800000
> xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
> pci_update_mappings_add d=0x7fee0b1de000 00:02.0
> 0,0xf1000000+0x800000
> xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
> pci_update_mappings_add d=0x7fee0b1de000 00:02.0
> 0,0xf1000000+0x800000
> xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
> pci_update_mappings_add d=0x7fee0b1de000 00:02.0
> 0,0xf1000000+0x800000
> xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
> pci_update_mappings_add d=0x7fee0b1de000 00:02.0
> 0,0xf1000000+0x800000
> xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
> pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
> pci_update_mappings_add d=0x7fee0b154000 00:03.0
> 1,0xf0000000+0x1000000
> pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
> pci_update_mappings_add d=0x7fee0b154000 00:03.0
> 1,0xf0000000+0x1000000
> pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
> pci_update_mappings_add d=0x7fee0b154000 00:03.0
> 1,0xf0000000+0x1000000
> pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
> pci_update_mappings_add d=0x7fee0b154000 00:03.0
> 1,0xf0000000+0x1000000
> pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
> pci_update_mappings_add d=0x7fee0b154000 00:03.0
> 1,0xf0000000+0x1000000
> pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
> pci_update_mappings_add d=0x7fee0b154000 00:03.0
> 1,0xf0000000+0x1000000
> pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
> pci_update_mappings_add d=0x7fee0b154000 00:03.0
> 1,0xf0000000+0x1000000
> 
> Apart from this issue, I was able to boot up and shut down a VM, using
> -xen-domid-restrict, -chroot, and -runasid.
> 
> There is a caveat when using -xen-domid-restrict and -chroot at the same
> time. The restriction happens after chrooting, so the chroot directory
> has to contain a valid /dev/null. This is a bit annoying and prevents
> the chroot being on a "nodev" mount.
> 
> Regards,
> --
> Ross Lagerwall
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

Re: [PATCH v2 0/*] xen: xen-domid-restrict improvements

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> Ross Lagerwall
> Sent: 06 October 2017 13:58
> To: Ian Jackson <Ian.Jackson@citrix.com>; qemu-devel@nongnu.org
> Cc: Anthony Perard <anthony.perard@citrix.com>; xen-
> devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>;
> Juergen Gross <jgross@suse.com>
> Subject: Re: [Xen-devel] [PATCH v2 0/*] xen: xen-domid-restrict
> improvements
> 
> On 10/04/2017 05:18 PM, Ian Jackson wrote:
> > (Resending this because 1. I got the CC for xen-devel wrong; 2. I got
> > the subject wrong: there are actually 8 patches; 3. I mangled
> > Anthony's name in theheaders.  Sorry for the noise.)
> >
> > 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 will be
> > 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.
> >
> 
> Hi Ian,
> 
> I'm testing your QEMU and Xen patch series and found that after being
> restricted, QEMU fails to setup up the VGA memory properly which causes
> a complete stall with stdvga. With cirrus it mostly works although it
> seems to have reduced performance.
> 
> I think it happens when the VM sets up the BAR some time after
> xen_restrict() has been called. The failure comes from QEMU calling
> xc_domain_add_to_physmap() which calls do_memory_op() and finally
> xencall2(). But the underlying xencall fd has been replaced with /dev/null.

Oh, that's a PITA. This is because of the slightly hacky way that the VRAM is dealt with (it needing to be moved into the BAR of the PCI VGA device at the point where guest firmware programs it). Maybe we need a new dm_op for this?

  Paul

> 
> Here is some debug output, all of which occurs after the call to
> xentoolcore_restrict_all() has completed. 00:02.0 is the VGA device.
> 
> pci_update_mappings_add d=0x7fee0b154000 00:03.0
> 1,0xf0000000+0x1000000
> pci_update_mappings_add d=0x7fee0b1de000 00:02.0
> 0,0xf1000000+0x800000
> xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
> xen: mapping vram to f1000000 - f1800000
> xen: add_to_physmap MFN 80000 to PFN f1000 failed: 25 (errno: 25)
> pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
> pci_update_mappings_add d=0x7fee0b1a4000 00:04.0 0,0xc100+0x100
> pci_update_mappings_add d=0x7fee0b1a4000 00:04.0 1,0xf1800000+0x100
> pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
> pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
> pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
> pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
> pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
> pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
> pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
> pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
> pci_update_mappings_add d=0x7fee0b65c000 00:01.1 4,0xc220+0x10
> pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
> pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
> pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
> pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
> pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
> pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
> pci_update_mappings_add d=0x7fee0b006e00 00:01.2 4,0xc200+0x20
> pci_update_mappings_add d=0x7fee0b1de000 00:02.0
> 0,0xf1000000+0x800000
> xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
> pci_update_mappings_add d=0x7fee0b1de000 00:02.0
> 0,0xf1000000+0x800000
> xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
> pci_update_mappings_add d=0x7fee0b1de000 00:02.0
> 0,0xf1000000+0x800000
> xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
> pci_update_mappings_add d=0x7fee0b1de000 00:02.0
> 0,0xf1000000+0x800000
> xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
> pci_update_mappings_add d=0x7fee0b1de000 00:02.0
> 0,0xf1000000+0x800000
> xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
> pci_update_mappings_add d=0x7fee0b1de000 00:02.0
> 0,0xf1000000+0x800000
> xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
> pci_update_mappings_add d=0x7fee0b1de000 00:02.0
> 0,0xf1000000+0x800000
> xen_client_set_memory 0xf1000000 size 0x800000, log_dirty 1
> pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
> pci_update_mappings_add d=0x7fee0b154000 00:03.0
> 1,0xf0000000+0x1000000
> pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
> pci_update_mappings_add d=0x7fee0b154000 00:03.0
> 1,0xf0000000+0x1000000
> pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
> pci_update_mappings_add d=0x7fee0b154000 00:03.0
> 1,0xf0000000+0x1000000
> pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
> pci_update_mappings_add d=0x7fee0b154000 00:03.0
> 1,0xf0000000+0x1000000
> pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
> pci_update_mappings_add d=0x7fee0b154000 00:03.0
> 1,0xf0000000+0x1000000
> pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
> pci_update_mappings_add d=0x7fee0b154000 00:03.0
> 1,0xf0000000+0x1000000
> pci_update_mappings_add d=0x7fee0b154000 00:03.0 0,0xc000+0x100
> pci_update_mappings_add d=0x7fee0b154000 00:03.0
> 1,0xf0000000+0x1000000
> 
> Apart from this issue, I was able to boot up and shut down a VM, using
> -xen-domid-restrict, -chroot, and -runasid.
> 
> There is a caveat when using -xen-domid-restrict and -chroot at the same
> time. The restriction happens after chrooting, so the chroot directory
> has to contain a valid /dev/null. This is a bit annoying and prevents
> the chroot being on a "nodev" mount.
> 
> Regards,
> --
> Ross Lagerwall
> 
> _______________________________________________
> 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 v2 0/*] xen: xen-domid-restrict improvements

[Ian Jackson]

Ross Lagerwall writes ("Re: [PATCH v2 0/*] xen: xen-domid-restrict improvements"):
> On 10/04/2017 05:18 PM, Ian Jackson wrote:
> > 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.
...
> I'm testing your QEMU and Xen patch series and found that after being 
> restricted, QEMU fails to setup up the VGA memory properly which causes 
> a complete stall with stdvga. With cirrus it mostly works although it 
> seems to have reduced performance.

Thanks for your testing.

I admit that I didn't look at the VGA console of my guest.  I'm using
cirrus but my guest isn't using it very much.  I use the "serial"
console instead.

> I think it happens when the VM sets up the BAR some time after 
> xen_restrict() has been called. The failure comes from QEMU calling 
> xc_domain_add_to_physmap() which calls do_memory_op() and finally 
> xencall2(). But the underlying xencall fd has been replaced with /dev/null.

I think to fix this properly, we will need to add a dmop version of
XENMEM_add_to_physmap.  I don't propose to try to do that for Xen
4.10.  In the meantime I think this is good enough for "tech preview",
and provides a base to work on.

> There is a caveat when using -xen-domid-restrict and -chroot at the same 
> time. The restriction happens after chrooting, so the chroot directory 
> has to contain a valid /dev/null. This is a bit annoying and prevents 
> the chroot being on a "nodev" mount.

How annoying.  I will fix the relevant qemu patch to do the Xen
restrict before os_setup_post.

Ian.

Re: [PATCH v2 0/*] xen: xen-domid-restrict improvements

[Ian Jackson]

Ross Lagerwall writes ("Re: [PATCH v2 0/*] xen: xen-domid-restrict improvements"):
> On 10/04/2017 05:18 PM, Ian Jackson wrote:
> > 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.
...
> I'm testing your QEMU and Xen patch series and found that after being 
> restricted, QEMU fails to setup up the VGA memory properly which causes 
> a complete stall with stdvga. With cirrus it mostly works although it 
> seems to have reduced performance.

Thanks for your testing.

I admit that I didn't look at the VGA console of my guest.  I'm using
cirrus but my guest isn't using it very much.  I use the "serial"
console instead.

> I think it happens when the VM sets up the BAR some time after 
> xen_restrict() has been called. The failure comes from QEMU calling 
> xc_domain_add_to_physmap() which calls do_memory_op() and finally 
> xencall2(). But the underlying xencall fd has been replaced with /dev/null.

I think to fix this properly, we will need to add a dmop version of
XENMEM_add_to_physmap.  I don't propose to try to do that for Xen
4.10.  In the meantime I think this is good enough for "tech preview",
and provides a base to work on.

> There is a caveat when using -xen-domid-restrict and -chroot at the same 
> time. The restriction happens after chrooting, so the chroot directory 
> has to contain a valid /dev/null. This is a bit annoying and prevents 
> the chroot being on a "nodev" mount.

How annoying.  I will fix the relevant qemu patch to do the Xen
restrict before os_setup_post.

Ian.

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

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Ian Jackson]

Peter Maydell writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> On 4 October 2017 at 17:18, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
> >  static void change_process_uid(void)
> >  {
> > -    if (user_pwd) {
> > -        if (setgid(user_pwd->pw_gid) < 0) {
> > +    if (user_pwd || user_uid != (uid_t)-1) {
> > +        if (setgid(user_pwd ? user_pwd->pw_gid : user_gid) < 0) {
> >              fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
> 
> If you're changing the gid we pass to setgid() I think you should
> also change the value we tell the user we tried to use in the
> error message, or it could be rather confusing.

Thanks for the review.  Sorry about the mess here.  I have fixed all
these problems.

Ian.

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Ian Jackson]

Peter Maydell writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> On 4 October 2017 at 17:18, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
> >  static void change_process_uid(void)
> >  {
> > -    if (user_pwd) {
> > -        if (setgid(user_pwd->pw_gid) < 0) {
> > +    if (user_pwd || user_uid != (uid_t)-1) {
> > +        if (setgid(user_pwd ? user_pwd->pw_gid : user_gid) < 0) {
> >              fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
> 
> If you're changing the gid we pass to setgid() I think you should
> also change the value we tell the user we tried to use in the
> error message, or it could be rather confusing.

Thanks for the review.  Sorry about the mess here.  I have fixed all
these problems.

Ian.

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

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Ian Jackson]

Ross Lagerwall writes ("Re: [PATCH 7/8] os-posix: Provide new -runasid option"):
> On 10/04/2017 05:18 PM, Ian Jackson wrote:
> > -        if (setuid(user_pwd->pw_uid) < 0) {
> > +        if (setuid(user_pwd ? user_pwd->pw_uid : user_gid) < 0) {
> >               fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
> >               exit(1);
> >           }
> 
> This last one should be user_uid, not user_gid.

Thanks, fixed.

Ian.

Re: [PATCH 7/8] os-posix: Provide new -runasid option

[Ian Jackson]

Ross Lagerwall writes ("Re: [PATCH 7/8] os-posix: Provide new -runasid option"):
> On 10/04/2017 05:18 PM, Ian Jackson wrote:
> > -        if (setuid(user_pwd->pw_uid) < 0) {
> > +        if (setuid(user_pwd ? user_pwd->pw_uid : user_gid) < 0) {
> >               fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
> >               exit(1);
> >           }
> 
> This last one should be user_uid, not user_gid.

Thanks, fixed.

Ian.

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

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Ian Jackson]

(resending to fix xen-devel CC)

Peter Maydell writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> On 4 October 2017 at 17:18, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
> >  static void change_process_uid(void)
> >  {
> > -    if (user_pwd) {
> > -        if (setgid(user_pwd->pw_gid) < 0) {
> > +    if (user_pwd || user_uid != (uid_t)-1) {
> > +        if (setgid(user_pwd ? user_pwd->pw_gid : user_gid) < 0) {
> >              fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
> 
> If you're changing the gid we pass to setgid() I think you should
> also change the value we tell the user we tried to use in the
> error message, or it could be rather confusing.

Thanks for the review.  Sorry about the mess here.  I have fixed all
these problems.

Ian.

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

Re: [PATCH 7/8] os-posix: Provide new -runasid option

[Ian Jackson]

(resending to fix xen-devel CC)

Ross Lagerwall writes ("Re: [PATCH 7/8] os-posix: Provide new -runasid option"):
> On 10/04/2017 05:18 PM, Ian Jackson wrote:
> > -        if (setuid(user_pwd->pw_uid) < 0) {
> > +        if (setuid(user_pwd ? user_pwd->pw_uid : user_gid) < 0) {
> >               fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
> >               exit(1);
> >           }
> 
> This last one should be user_uid, not user_gid.

Thanks, fixed.

Ian.

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

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Markus Armbruster]

Ian Jackson <ian.jackson@eu.citrix.com> writes:

> 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.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
[...]
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9f6e2ad..34a5329 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3968,6 +3968,18 @@ Immediately before starting guest execution, drop root privileges, switching
>  to the specified user.
>  ETEXI
>  
> +#ifndef _WIN32
> +DEF("runasid", HAS_ARG, QEMU_OPTION_runasid, \
> +    "-runasid uid.gid     change to numeric uid and gid just before starting the VM\n",
> +    QEMU_ARCH_ALL)
> +#endif
> +STEXI
> +@item -runasid @var{uid}.@var{gid}
> +@findex -runasid
> +Immediately before starting guest execution, drop root privileges, switching
> +to the specified uid and gid.
> +ETEXI
> +
>  DEF("prom-env", HAS_ARG, QEMU_OPTION_prom_env,
>      "-prom-env variable=value\n"
>      "                set OpenBIOS nvram variables\n",

The last thing the QEMU command line needs is more exotic options.  Are
you sure we need a new one here?  Can we make existing -runas serve?
Precedence: Coreutils[*].  Pseudo-code:

    if argument is a decimal number starting with '+':
        user ID
    else if argument is a valid user name:
        user name
    else if argument is a valid user ID:
        user ID
    else:
        error

[*] https://www.gnu.org/software/coreutils/manual/html_node/Disambiguating-names-and-IDs.html

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Markus Armbruster]

Ian Jackson <ian.jackson@eu.citrix.com> writes:

> 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.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
[...]
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9f6e2ad..34a5329 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3968,6 +3968,18 @@ Immediately before starting guest execution, drop root privileges, switching
>  to the specified user.
>  ETEXI
>  
> +#ifndef _WIN32
> +DEF("runasid", HAS_ARG, QEMU_OPTION_runasid, \
> +    "-runasid uid.gid     change to numeric uid and gid just before starting the VM\n",
> +    QEMU_ARCH_ALL)
> +#endif
> +STEXI
> +@item -runasid @var{uid}.@var{gid}
> +@findex -runasid
> +Immediately before starting guest execution, drop root privileges, switching
> +to the specified uid and gid.
> +ETEXI
> +
>  DEF("prom-env", HAS_ARG, QEMU_OPTION_prom_env,
>      "-prom-env variable=value\n"
>      "                set OpenBIOS nvram variables\n",

The last thing the QEMU command line needs is more exotic options.  Are
you sure we need a new one here?  Can we make existing -runas serve?
Precedence: Coreutils[*].  Pseudo-code:

    if argument is a decimal number starting with '+':
        user ID
    else if argument is a valid user name:
        user name
    else if argument is a valid user ID:
        user ID
    else:
        error

[*] https://www.gnu.org/software/coreutils/manual/html_node/Disambiguating-names-and-IDs.html

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

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Ian Jackson]

Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> The last thing the QEMU command line needs is more exotic options.  Are
> you sure we need a new one here?  Can we make existing -runas serve?
> Precedence: Coreutils[*].  Pseudo-code:
> 
>     if argument is a decimal number starting with '+':
>         user ID
>     else if argument is a valid user name:
>         user name
>     else if argument is a valid user ID:
>         user ID
>     else:
>         error

I can do this.  So -runas <uid>.<gid> then.  I don't think it makes
sense to try to -runas <uid> because: you wouldn't have a username
to pass to initgroups: not calling initgroups would be a bear trap;
and otherwise we wouldn't know what gid to use.

Ian.

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Ian Jackson]

Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> The last thing the QEMU command line needs is more exotic options.  Are
> you sure we need a new one here?  Can we make existing -runas serve?
> Precedence: Coreutils[*].  Pseudo-code:
> 
>     if argument is a decimal number starting with '+':
>         user ID
>     else if argument is a valid user name:
>         user name
>     else if argument is a valid user ID:
>         user ID
>     else:
>         error

I can do this.  So -runas <uid>.<gid> then.  I don't think it makes
sense to try to -runas <uid> because: you wouldn't have a username
to pass to initgroups: not calling initgroups would be a bear trap;
and otherwise we wouldn't know what gid to use.

Ian.

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

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Ian Jackson]

(resending to right address for xen-devel)

Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> The last thing the QEMU command line needs is more exotic options.  Are
> you sure we need a new one here?  Can we make existing -runas serve?
> Precedence: Coreutils[*].  Pseudo-code:
> 
>     if argument is a decimal number starting with '+':
>         user ID
>     else if argument is a valid user name:
>         user name
>     else if argument is a valid user ID:
>         user ID
>     else:
>         error

I can do this.  So -runas <uid>.<gid> then.  I don't think it makes
sense to try to -runas <uid> because: you wouldn't have a username
to pass to initgroups: not calling initgroups would be a bear trap;
and otherwise we wouldn't know what gid to use.

Ian.

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

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Daniel P. Berrange]

On Mon, Oct 09, 2017 at 04:05:10PM +0100, Ian Jackson wrote:
> Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> > The last thing the QEMU command line needs is more exotic options.  Are
> > you sure we need a new one here?  Can we make existing -runas serve?
> > Precedence: Coreutils[*].  Pseudo-code:
> > 
> >     if argument is a decimal number starting with '+':
> >         user ID
> >     else if argument is a valid user name:
> >         user name
> >     else if argument is a valid user ID:
> >         user ID
> >     else:
> >         error
> 
> I can do this.  So -runas <uid>.<gid> then.  I don't think it makes
> sense to try to -runas <uid> because: you wouldn't have a username
> to pass to initgroups: not calling initgroups would be a bear trap;
> and otherwise we wouldn't know what gid to use.

Just use  getpwuid() to get the "struct passwd *", then change_process_uid()
doesn't need any changes at all AFAICT.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Daniel P. Berrange]

On Mon, Oct 09, 2017 at 04:05:10PM +0100, Ian Jackson wrote:
> Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> > The last thing the QEMU command line needs is more exotic options.  Are
> > you sure we need a new one here?  Can we make existing -runas serve?
> > Precedence: Coreutils[*].  Pseudo-code:
> > 
> >     if argument is a decimal number starting with '+':
> >         user ID
> >     else if argument is a valid user name:
> >         user name
> >     else if argument is a valid user ID:
> >         user ID
> >     else:
> >         error
> 
> I can do this.  So -runas <uid>.<gid> then.  I don't think it makes
> sense to try to -runas <uid> because: you wouldn't have a username
> to pass to initgroups: not calling initgroups would be a bear trap;
> and otherwise we wouldn't know what gid to use.

Just use  getpwuid() to get the "struct passwd *", then change_process_uid()
doesn't need any changes at all AFAICT.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

_______________________________________________
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 after os_setup_post

[Anthony PERARD]

On Wed, Oct 04, 2017 at 05:18:06PM +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 after os_setup_post.  Also provide the
> appropriate stub for when Xen compilation is disabled.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  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;

We probably want to check here if Xen is enable (via xen_enabled()).
xen_domid_restrict could be true when Xen is not used, even if it does
not make sense to use -xen-domid-restrict in that case.

> +    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..9e7d541 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4793,6 +4793,7 @@ int main(int argc, char **argv, char **envp)
>      }
>  
>      os_setup_post();
> +    xen_setup_post();
>  
>      main_loop();
>      replay_disable_events();
> -- 
> 2.1.4
> 

-- 
Anthony PERARD

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

[Anthony PERARD]

On Wed, Oct 04, 2017 at 05:18:06PM +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 after os_setup_post.  Also provide the
> appropriate stub for when Xen compilation is disabled.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  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;

We probably want to check here if Xen is enable (via xen_enabled()).
xen_domid_restrict could be true when Xen is not used, even if it does
not make sense to use -xen-domid-restrict in that case.

> +    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..9e7d541 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4793,6 +4793,7 @@ int main(int argc, char **argv, char **envp)
>      }
>  
>      os_setup_post();
> +    xen_setup_post();
>  
>      main_loop();
>      replay_disable_events();
> -- 
> 2.1.4
> 

-- 
Anthony PERARD

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

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Ian Jackson]

Daniel P. Berrange writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> Just use  getpwuid() to get the "struct passwd *", then change_process_uid()
> doesn't need any changes at all AFAICT.

See my comments in the commit message.  There may be multiple passwd
entries referring to a particular uid.  I think in this area it would
be better to expect the caller to be explicit, rather than taking
getpwuid's notion of the first one.

Ian.

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Ian Jackson]

Daniel P. Berrange writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> Just use  getpwuid() to get the "struct passwd *", then change_process_uid()
> doesn't need any changes at all AFAICT.

See my comments in the commit message.  There may be multiple passwd
entries referring to a particular uid.  I think in this area it would
be better to expect the caller to be explicit, rather than taking
getpwuid's notion of the first one.

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 after os_setup_post

[Ian Jackson]

(My resend has crossed with your review.  Sorry about that.)

Anthony PERARD writes ("Re: [PATCH 3/8] xen: defer call to xen_restrict until after os_setup_post"):
> On Wed, Oct 04, 2017 at 05:18:06PM +0100, Ian Jackson wrote:

> > +void xen_setup_post(void)
> > +{
> > +    int rc;
> 
> We probably want to check here if Xen is enable (via xen_enabled()).
> xen_domid_restrict could be true when Xen is not used, even if it does
> not make sense to use -xen-domid-restrict in that case.

Should -xen-domid-restrict without xen_enabled() not fail ?  IMO it is
normally better for an option which requests enhanced security to fail
when it can't do its job, rather than just hoping that its
inapplicability is intentional.

OTOH I suppose there is an argument that without xen_enabled() the
function of -xen-domid-restrict is achieved, in that without
xen_enabled() qemu is unable (after dropping privileges) to act on
Xen domains at all...

Thanks,
Ian.

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

[Ian Jackson]

(My resend has crossed with your review.  Sorry about that.)

Anthony PERARD writes ("Re: [PATCH 3/8] xen: defer call to xen_restrict until after os_setup_post"):
> On Wed, Oct 04, 2017 at 05:18:06PM +0100, Ian Jackson wrote:

> > +void xen_setup_post(void)
> > +{
> > +    int rc;
> 
> We probably want to check here if Xen is enable (via xen_enabled()).
> xen_domid_restrict could be true when Xen is not used, even if it does
> not make sense to use -xen-domid-restrict in that case.

Should -xen-domid-restrict without xen_enabled() not fail ?  IMO it is
normally better for an option which requests enhanced security to fail
when it can't do its job, rather than just hoping that its
inapplicability is intentional.

OTOH I suppose there is an argument that without xen_enabled() the
function of -xen-domid-restrict is achieved, in that without
xen_enabled() qemu is unable (after dropping privileges) to act on
Xen domains at all...

Thanks,
Ian.

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

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Ian Jackson]

(resending, more competently this time)

Daniel P. Berrange writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> Just use  getpwuid() to get the "struct passwd *", then change_process_uid()
> doesn't need any changes at all AFAICT.

See my comments in the commit message.  There may be multiple passwd
entries referring to a particular uid.  I think in this area it would
be better to expect the caller to be explicit, rather than taking
getpwuid's notion of the first one.

Ian.

_______________________________________________
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 after os_setup_post

[Ian Jackson]

(My resend has crossed with your review.  Sorry about that.)

Anthony PERARD writes ("Re: [PATCH 3/8] xen: defer call to xen_restrict until after os_setup_post"):
> On Wed, Oct 04, 2017 at 05:18:06PM +0100, Ian Jackson wrote:

> > +void xen_setup_post(void)
> > +{
> > +    int rc;
> 
> We probably want to check here if Xen is enable (via xen_enabled()).
> xen_domid_restrict could be true when Xen is not used, even if it does
> not make sense to use -xen-domid-restrict in that case.

Should -xen-domid-restrict without xen_enabled() not fail ?  IMO it is
normally better for an option which requests enhanced security to fail
when it can't do its job, rather than just hoping that its
inapplicability is intentional.

OTOH I suppose there is an argument that without xen_enabled() the
function of -xen-domid-restrict is achieved, in that without
xen_enabled() qemu is unable (after dropping privileges) to act on
Xen domains at all...

Thanks,
Ian.

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

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Markus Armbruster]

Ian Jackson <ian.jackson@eu.citrix.com> writes:

> Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
>> The last thing the QEMU command line needs is more exotic options.  Are
>> you sure we need a new one here?  Can we make existing -runas serve?
>> Precedence: Coreutils[*].  Pseudo-code:
>> 
>>     if argument is a decimal number starting with '+':
>>         user ID
>>     else if argument is a valid user name:
>>         user name
>>     else if argument is a valid user ID:
>>         user ID
>>     else:
>>         error
>
> I can do this.  So -runas <uid>.<gid> then.  I don't think it makes
> sense to try to -runas <uid> because: you wouldn't have a username
> to pass to initgroups: not calling initgroups would be a bear trap;
> and otherwise we wouldn't know what gid to use.

Actually, a numeric UID without group name or ID could be made to work
just fine as long as it maps to a user name.  The use case may not be
worth the bother, though.

Using '.' to separate user and group is suboptimal, because POSIX
portable user and group names may contain it:

    3.426 User Name

    A string that is used to identify a user; see also User Database.
    To be portable across systems conforming to IEEE Std 1003.1-2001,
    the value is composed of characters from the portable filename
    character set.  The hyphen should not be used as the first character
    of a portable user name.

and

    3.276 Portable Filename Character Set

    The set of characters from which portable filenames are constructed.

        A B C D E F G H I J K L M N O P Q R S T U V W X Y Z
        a b c d e f g h i j k l m n o p q r s t u v w x y z
        0 1 2 3 4 5 6 7 8 9 . _ -

http://pubs.opengroup.org/onlinepubs/000095399/basedefs/xbd_chap03.html

Coreutils uses ':'.  Let's follow its lead.

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Markus Armbruster]

Ian Jackson <ian.jackson@eu.citrix.com> writes:

> Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
>> The last thing the QEMU command line needs is more exotic options.  Are
>> you sure we need a new one here?  Can we make existing -runas serve?
>> Precedence: Coreutils[*].  Pseudo-code:
>> 
>>     if argument is a decimal number starting with '+':
>>         user ID
>>     else if argument is a valid user name:
>>         user name
>>     else if argument is a valid user ID:
>>         user ID
>>     else:
>>         error
>
> I can do this.  So -runas <uid>.<gid> then.  I don't think it makes
> sense to try to -runas <uid> because: you wouldn't have a username
> to pass to initgroups: not calling initgroups would be a bear trap;
> and otherwise we wouldn't know what gid to use.

Actually, a numeric UID without group name or ID could be made to work
just fine as long as it maps to a user name.  The use case may not be
worth the bother, though.

Using '.' to separate user and group is suboptimal, because POSIX
portable user and group names may contain it:

    3.426 User Name

    A string that is used to identify a user; see also User Database.
    To be portable across systems conforming to IEEE Std 1003.1-2001,
    the value is composed of characters from the portable filename
    character set.  The hyphen should not be used as the first character
    of a portable user name.

and

    3.276 Portable Filename Character Set

    The set of characters from which portable filenames are constructed.

        A B C D E F G H I J K L M N O P Q R S T U V W X Y Z
        a b c d e f g h i j k l m n o p q r s t u v w x y z
        0 1 2 3 4 5 6 7 8 9 . _ -

http://pubs.opengroup.org/onlinepubs/000095399/basedefs/xbd_chap03.html

Coreutils uses ':'.  Let's follow its lead.

_______________________________________________
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 after os_setup_post

[Anthony PERARD]

On Mon, Oct 09, 2017 at 05:58:17PM +0100, Ian Jackson wrote:
> (My resend has crossed with your review.  Sorry about that.)
> 
> Anthony PERARD writes ("Re: [PATCH 3/8] xen: defer call to xen_restrict until after os_setup_post"):
> > On Wed, Oct 04, 2017 at 05:18:06PM +0100, Ian Jackson wrote:
> 
> > > +void xen_setup_post(void)
> > > +{
> > > +    int rc;
> > 
> > We probably want to check here if Xen is enable (via xen_enabled()).
> > xen_domid_restrict could be true when Xen is not used, even if it does
> > not make sense to use -xen-domid-restrict in that case.
> 
> Should -xen-domid-restrict without xen_enabled() not fail ?  IMO it is
> normally better for an option which requests enhanced security to fail
> when it can't do its job, rather than just hoping that its
> inapplicability is intentional.

I'm tring to find out what does calling xen_restrict_all(0), when
running an non-Xen guest. I think it would just lock(), then unlock()
then there should not be any handle to restrict, and return 0; is that
right?

So I think the code is fine like this. I'll put my Reviewed-by to the
last version.

Thanks.

> OTOH I suppose there is an argument that without xen_enabled() the
> function of -xen-domid-restrict is achieved, in that without
> xen_enabled() qemu is unable (after dropping privileges) to act on
> Xen domains at all...
> 
> Thanks,
> Ian.

-- 
Anthony PERARD

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

[Anthony PERARD]

On Mon, Oct 09, 2017 at 05:58:17PM +0100, Ian Jackson wrote:
> (My resend has crossed with your review.  Sorry about that.)
> 
> Anthony PERARD writes ("Re: [PATCH 3/8] xen: defer call to xen_restrict until after os_setup_post"):
> > On Wed, Oct 04, 2017 at 05:18:06PM +0100, Ian Jackson wrote:
> 
> > > +void xen_setup_post(void)
> > > +{
> > > +    int rc;
> > 
> > We probably want to check here if Xen is enable (via xen_enabled()).
> > xen_domid_restrict could be true when Xen is not used, even if it does
> > not make sense to use -xen-domid-restrict in that case.
> 
> Should -xen-domid-restrict without xen_enabled() not fail ?  IMO it is
> normally better for an option which requests enhanced security to fail
> when it can't do its job, rather than just hoping that its
> inapplicability is intentional.

I'm tring to find out what does calling xen_restrict_all(0), when
running an non-Xen guest. I think it would just lock(), then unlock()
then there should not be any handle to restrict, and return 0; is that
right?

So I think the code is fine like this. I'll put my Reviewed-by to the
last version.

Thanks.

> OTOH I suppose there is an argument that without xen_enabled() the
> function of -xen-domid-restrict is achieved, in that without
> xen_enabled() qemu is unable (after dropping privileges) to act on
> Xen domains at all...
> 
> Thanks,
> Ian.

-- 
Anthony PERARD

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

Re: [Qemu-devel] [Xen-devel] [PATCH v2 0/*] xen: xen-domid-restrict improvements

[Ross Lagerwall]

On 10/06/2017 02:19 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
>> Ross Lagerwall
>> Sent: 06 October 2017 13:58
>> To: Ian Jackson <Ian.Jackson@citrix.com>; qemu-devel@nongnu.org
>> Cc: Anthony Perard <anthony.perard@citrix.com>; xen-
>> devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>;
>> Juergen Gross <jgross@suse.com>
>> Subject: Re: [Xen-devel] [PATCH v2 0/*] xen: xen-domid-restrict
>> improvements
>>
>> On 10/04/2017 05:18 PM, Ian Jackson wrote:
>>> (Resending this because 1. I got the CC for xen-devel wrong; 2. I got
>>> the subject wrong: there are actually 8 patches; 3. I mangled
>>> Anthony's name in theheaders.  Sorry for the noise.)
>>>
>>> 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 will be
>>> 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.
>>>
>>
>> Hi Ian,
>>
>> I'm testing your QEMU and Xen patch series and found that after being
>> restricted, QEMU fails to setup up the VGA memory properly which causes
>> a complete stall with stdvga. With cirrus it mostly works although it
>> seems to have reduced performance.
>>
>> I think it happens when the VM sets up the BAR some time after
>> xen_restrict() has been called. The failure comes from QEMU calling
>> xc_domain_add_to_physmap() which calls do_memory_op() and finally
>> xencall2(). But the underlying xencall fd has been replaced with /dev/null.
> 
> Oh, that's a PITA. This is because of the slightly hacky way that the VRAM is dealt with (it needing to be moved into the BAR of the PCI VGA device at the point where guest firmware programs it). Maybe we need a new dm_op for this?
> 
If no one objects, I propose adding the following calls to 
libxendevicemodel (with underlying Xen implementations, of course) that 
would be usable after the xendevicemodel handle has been restricted.

xendevicemodel_add_to_physmap(xendevicemodel_handle *dmod,
                               domid_t domid,
                               unsigned long idx,
                               xen_pfn_t gpfn);

This is equivalent to xc_domain_add_to_physmap() with
space==XENMAPSPACE_gmfn.

xendevicemodel_pin_memory_cacheattr(xendevicemodel_handle *dmod,
                                     domid_t domid,
                                     uint64_t start,
                                     uint64_t end,
                                     uint32_t type);

This is equivalent to xc_domain_pin_memory_cacheattr().

-- 
Ross Lagerwall

Re: [PATCH v2 0/*] xen: xen-domid-restrict improvements

[Ross Lagerwall]

On 10/06/2017 02:19 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
>> Ross Lagerwall
>> Sent: 06 October 2017 13:58
>> To: Ian Jackson <Ian.Jackson@citrix.com>; qemu-devel@nongnu.org
>> Cc: Anthony Perard <anthony.perard@citrix.com>; xen-
>> devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>;
>> Juergen Gross <jgross@suse.com>
>> Subject: Re: [Xen-devel] [PATCH v2 0/*] xen: xen-domid-restrict
>> improvements
>>
>> On 10/04/2017 05:18 PM, Ian Jackson wrote:
>>> (Resending this because 1. I got the CC for xen-devel wrong; 2. I got
>>> the subject wrong: there are actually 8 patches; 3. I mangled
>>> Anthony's name in theheaders.  Sorry for the noise.)
>>>
>>> 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 will be
>>> 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.
>>>
>>
>> Hi Ian,
>>
>> I'm testing your QEMU and Xen patch series and found that after being
>> restricted, QEMU fails to setup up the VGA memory properly which causes
>> a complete stall with stdvga. With cirrus it mostly works although it
>> seems to have reduced performance.
>>
>> I think it happens when the VM sets up the BAR some time after
>> xen_restrict() has been called. The failure comes from QEMU calling
>> xc_domain_add_to_physmap() which calls do_memory_op() and finally
>> xencall2(). But the underlying xencall fd has been replaced with /dev/null.
> 
> Oh, that's a PITA. This is because of the slightly hacky way that the VRAM is dealt with (it needing to be moved into the BAR of the PCI VGA device at the point where guest firmware programs it). Maybe we need a new dm_op for this?
> 
If no one objects, I propose adding the following calls to 
libxendevicemodel (with underlying Xen implementations, of course) that 
would be usable after the xendevicemodel handle has been restricted.

xendevicemodel_add_to_physmap(xendevicemodel_handle *dmod,
                               domid_t domid,
                               unsigned long idx,
                               xen_pfn_t gpfn);

This is equivalent to xc_domain_add_to_physmap() with
space==XENMAPSPACE_gmfn.

xendevicemodel_pin_memory_cacheattr(xendevicemodel_handle *dmod,
                                     domid_t domid,
                                     uint64_t start,
                                     uint64_t end,
                                     uint32_t type);

This is equivalent to xc_domain_pin_memory_cacheattr().

-- 
Ross Lagerwall

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

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Ian Jackson]

Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> Actually, a numeric UID without group name or ID could be made to work
> just fine as long as it maps to a user name.  The use case may not be
> worth the bother, though.

In libxl's use case, it wouldn't map to a name.

> Using '.' to separate user and group is suboptimal, because POSIX
> portable user and group names may contain it:
...
> Coreutils uses ':'.  Let's follow its lead.

I'm happy to change it to use `:'.

Can you confirm that this command line interface is then OK ?
We would like to stablise the corresponding code in libxl...

Ian.

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Ian Jackson]

Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> Actually, a numeric UID without group name or ID could be made to work
> just fine as long as it maps to a user name.  The use case may not be
> worth the bother, though.

In libxl's use case, it wouldn't map to a name.

> Using '.' to separate user and group is suboptimal, because POSIX
> portable user and group names may contain it:
...
> Coreutils uses ':'.  Let's follow its lead.

I'm happy to change it to use `:'.

Can you confirm that this command line interface is then OK ?
We would like to stablise the corresponding code in libxl...

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 after os_setup_post

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 3/8] xen: defer call to xen_restrict until after os_setup_post"):
> I'm tring to find out what does calling xen_restrict_all(0), when
> running an non-Xen guest. I think it would just lock(), then unlock()
> then there should not be any handle to restrict, and return 0; is that
> right?

Yes.  If the process has not opened any Xen control handles,
xentoolcore_restrict_all is a no-op.

Ian.

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

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 3/8] xen: defer call to xen_restrict until after os_setup_post"):
> I'm tring to find out what does calling xen_restrict_all(0), when
> running an non-Xen guest. I think it would just lock(), then unlock()
> then there should not be any handle to restrict, and return 0; is that
> right?

Yes.  If the process has not opened any Xen control handles,
xentoolcore_restrict_all is a no-op.

Ian.

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

Re: [Qemu-devel] [Xen-devel] [PATCH v2 0/*] xen: xen-domid-restrict improvements

[Ian Jackson]

Ross Lagerwall writes ("Re: [Xen-devel] [PATCH v2 0/*] xen: xen-domid-restrict improvements"):
> If no one objects, I propose adding the following calls to 
> libxendevicemodel (with underlying Xen implementations, of course) that 
> would be usable after the xendevicemodel handle has been restricted.
> 
> xendevicemodel_add_to_physmap(xendevicemodel_handle *dmod,
...
> xendevicemodel_pin_memory_cacheattr(xendevicemodel_handle *dmod,
...
> This is equivalent to xc_domain_pin_memory_cacheattr().

These seem fine to me.  If you want this in Xen 4.10 I think you will
want to make a case to Julien (not CC'd) for a release ack.

Ian.

Re: [PATCH v2 0/*] xen: xen-domid-restrict improvements

[Ian Jackson]

Ross Lagerwall writes ("Re: [Xen-devel] [PATCH v2 0/*] xen: xen-domid-restrict improvements"):
> If no one objects, I propose adding the following calls to 
> libxendevicemodel (with underlying Xen implementations, of course) that 
> would be usable after the xendevicemodel handle has been restricted.
> 
> xendevicemodel_add_to_physmap(xendevicemodel_handle *dmod,
...
> xendevicemodel_pin_memory_cacheattr(xendevicemodel_handle *dmod,
...
> This is equivalent to xc_domain_pin_memory_cacheattr().

These seem fine to me.  If you want this in Xen 4.10 I think you will
want to make a case to Julien (not CC'd) for a release ack.

Ian.

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

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Ian Jackson]

Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option"):
> Actually, a numeric UID without group name or ID could be made to work
> just fine as long as it maps to a user name.  The use case may not be
> worth the bother, though.

In libxl's use case, it wouldn't map to a name.

> Using '.' to separate user and group is suboptimal, because POSIX
> portable user and group names may contain it:
...
> Coreutils uses ':'.  Let's follow its lead.

I'm happy to change it to use `:'.

Can you confirm that this command line interface is then OK ?
We would like to stablise the corresponding code in libxl...

Ian.

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