[PATCH 00/22] Provide some actual restriction of qemu

[PATCH 00/22] Provide some actual restriction of qemu

[Ian Jackson]

With this series, it is possible to run qemu in a way that I think
really does not have global privilege any more>

I have verified that it runs as a non-root user.  I have checked all
of its fds and they are either privcmd (which I have arranged to
neuter), or /dev/null, or harmless sockets and pipes, or evtchn.

Unfortunately this needs a new "xentoolcore" library, which all the
existing libraries register with so that the restrict call is
effective.

Also there are a number of lacunae.  In particular:

 - if we are not using a shared uid, we should kill all processes
   belonging to the chosen uid both at domain start time and at
   domain shutdown time

 - we should have qemu chroot

 - some audit and/or review of the resulting situation would be
   good before we offer security support for the new boundary

 - use of rlimits may be useful to mitigate the risk of DOS
   by a compromised qemu

 - cdrom insert would have to be done via fd passing and is not
   yet implemented

 - we need to think about what happens during migration (currently
   privileges are dropped very late, certainly after the receiving
   qemu has read the migration stream from its
   now-supposedly-untrusted peer)

The series depends for its functionality on a still-RFC qemu series I
have just posted, but should be harmless without the new qemu patches.

Thanks,
Ian.

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

[PATCH 01/22] xen: Provide XEN_DMOP_remote_shutdown

[Ian Jackson]

SCHEDOP_remote_shutdown should be a DMOP so that a deprivileged qemu
can do the propery tidying up.

We should remove SCHEDOP_remote_shutdown at some point.

CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 xen/arch/x86/hvm/dm.c          |  9 +++++++++
 xen/include/public/hvm/dm_op.h | 12 ++++++++++++
 xen/include/xlat.lst           |  1 +
 3 files changed, 22 insertions(+)

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index 4cf6dee..9eee9fb 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -630,6 +630,14 @@ static int dm_op(const struct dmop_args *op_args)
         rc = hvm_inject_msi(d, data->addr, data->data);
         break;
     }
+    case XEN_DMOP_remote_shutdown:
+    {
+        const struct xen_dm_op_remote_shutdown *data =
+            &op.u.remote_shutdown;
+
+        domain_shutdown(d, data->reason);
+        break;
+    }
 
     default:
         rc = -EOPNOTSUPP;
@@ -659,6 +667,7 @@ CHECK_dm_op_modified_memory;
 CHECK_dm_op_set_mem_type;
 CHECK_dm_op_inject_event;
 CHECK_dm_op_inject_msi;
+CHECK_dm_op_remote_shutdown;
 
 int compat_dm_op(domid_t domid,
                  unsigned int nr_bufs,
diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
index 2a4c3d9..21552b7 100644
--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -357,6 +357,17 @@ struct xen_dm_op_map_mem_type_to_ioreq_server {
                            has to be set to zero by the caller */
 };
 
+/*
+ * XEN_DMOP_remote_shutdown : Declare a shutdown for another domain
+ *                            Identical to SCHEDOP_remote_shutdown
+ */
+#define XEN_DMOP_remote_shutdown 16
+
+struct xen_dm_op_remote_shutdown {
+    uint32_t reason;       /* SHUTDOWN_* => enum sched_shutdown_reason */
+                           /* (Other reason values are not blocked) */
+};
+
 struct xen_dm_op {
     uint32_t op;
     uint32_t pad;
@@ -377,6 +388,7 @@ struct xen_dm_op {
         struct xen_dm_op_inject_msi inject_msi;
         struct xen_dm_op_map_mem_type_to_ioreq_server
                 map_mem_type_to_ioreq_server;
+        struct xen_dm_op_remote_shutdown remote_shutdown;
     } u;
 };
 
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index 0f17000..20055b4 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -70,6 +70,7 @@
 ?	dm_op_set_pci_intx_level	hvm/dm_op.h
 ?	dm_op_set_pci_link_route	hvm/dm_op.h
 ?	dm_op_track_dirty_vram		hvm/dm_op.h
+?	dm_op_remote_shutdown		hvm/dm_op.h
 ?	vcpu_hvm_context		hvm/hvm_vcpu.h
 ?	vcpu_hvm_x86_32			hvm/hvm_vcpu.h
 ?	vcpu_hvm_x86_64			hvm/hvm_vcpu.h
-- 
2.1.4


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

[PATCH 02/22] tools: libxendevicemodel: Provide xendevicemodel_shutdown

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libs/devicemodel/core.c                   | 16 ++++++++++++++++
 tools/libs/devicemodel/include/xendevicemodel.h |  9 +++++++++
 tools/libs/devicemodel/libxendevicemodel.map    |  1 +
 3 files changed, 26 insertions(+)

diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
index d7c6476..2093884 100644
--- a/tools/libs/devicemodel/core.c
+++ b/tools/libs/devicemodel/core.c
@@ -532,6 +532,22 @@ int xendevicemodel_inject_event(
     return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
 }
 
+int xendevicemodel_shutdown(
+    xendevicemodel_handle *dmod, domid_t domid, unsigned int reason)
+{
+    struct xen_dm_op op;
+    struct xen_dm_op_remote_shutdown *data;
+
+    memset(&op, 0, sizeof(op));
+
+    op.op = XEN_DMOP_remote_shutdown;
+    data = &op.u.remote_shutdown;
+
+    data->reason = reason;
+
+    return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
+}
+
 int xendevicemodel_restrict(xendevicemodel_handle *dmod, domid_t domid)
 {
     return osdep_xendevicemodel_restrict(dmod, domid);
diff --git a/tools/libs/devicemodel/include/xendevicemodel.h b/tools/libs/devicemodel/include/xendevicemodel.h
index 580fad2..6ce39d1 100644
--- a/tools/libs/devicemodel/include/xendevicemodel.h
+++ b/tools/libs/devicemodel/include/xendevicemodel.h
@@ -317,6 +317,15 @@ int xendevicemodel_inject_event(
     uint8_t type, uint32_t error_code, uint8_t insn_len, uint64_t cr2);
 
 /**
+ * Shuts the domain down.
+ *
+ * @parm reason usually enum sched_shutdown_reason, see xen/sched.h
+ * @return 0 on success, -1 on failure.
+ */
+int xendevicemodel_shutdown(
+    xendevicemodel_handle *dmod, domid_t domid, unsigned int reason);
+
+/**
  * This function restricts the use of this handle to the specified
  * domain.
  *
diff --git a/tools/libs/devicemodel/libxendevicemodel.map b/tools/libs/devicemodel/libxendevicemodel.map
index 130222c..fd57e79 100644
--- a/tools/libs/devicemodel/libxendevicemodel.map
+++ b/tools/libs/devicemodel/libxendevicemodel.map
@@ -18,6 +18,7 @@ VERS_1.0 {
 		xendevicemodel_modified_memory;
 		xendevicemodel_set_mem_type;
 		xendevicemodel_inject_event;
+		xendevicemodel_shutdown;
 		xendevicemodel_restrict;
 		xendevicemodel_close;
 	local: *; /* Do not expose anything by default */
-- 
2.1.4


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

[PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation

[Ian Jackson]

In practice, qemu opens a great many fds.  Tracking them all down and
playing whack-a-mole is unattractive.  It is also potentially fragile
in that future changes might accidentally undo our efforts.

Instead, we are going to teach all the Xen libraries how to register
their fds so that they can be neutered with one qemu call.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 .gitignore                                         |   4 +
 tools/Rules.mk                                     |   6 ++
 tools/libs/Makefile                                |   1 +
 tools/libs/toolcore/Makefile                       | 101 ++++++++++++++++++++
 tools/libs/toolcore/handlereg.c                    |  78 ++++++++++++++++
 tools/libs/toolcore/include/xentoolcore.h          |  71 ++++++++++++++
 tools/libs/toolcore/include/xentoolcore_internal.h | 102 +++++++++++++++++++++
 tools/libs/toolcore/libxentoolcore.map             |   7 ++
 tools/libs/toolcore/xentoolcore.pc.in              |   9 ++
 9 files changed, 379 insertions(+)
 create mode 100644 tools/libs/toolcore/Makefile
 create mode 100644 tools/libs/toolcore/handlereg.c
 create mode 100644 tools/libs/toolcore/include/xentoolcore.h
 create mode 100644 tools/libs/toolcore/include/xentoolcore_internal.h
 create mode 100644 tools/libs/toolcore/libxentoolcore.map
 create mode 100644 tools/libs/toolcore/xentoolcore.pc.in

diff --git a/.gitignore b/.gitignore
index ecb198f..269ab0c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -72,6 +72,7 @@ stubdom/libxencall-*
 stubdom/libxenevtchn-*
 stubdom/libxenforeignmemory-*
 stubdom/libxengnttab-*
+stubdom/libxentoolcore-*
 stubdom/libxentoollog-*
 stubdom/lwip-*
 stubdom/lwip/
@@ -97,6 +98,8 @@ tools/config.cache
 config/Tools.mk
 config/Stubdom.mk
 config/Docs.mk
+tools/libs/toolcore/headers.chk
+tools/libs/toolcore/xentoolcore.pc
 tools/libs/toollog/headers.chk
 tools/libs/toollog/xentoollog.pc
 tools/libs/evtchn/headers.chk
@@ -351,6 +354,7 @@ tools/include/xen-foreign/arm64.h
 .git
 tools/misc/xen-hptool
 tools/misc/xen-mfndump
+tools/libs/toolcore/include/_*.h
 tools/libxc/_*.[ch]
 tools/libxl/_*.[ch]
 tools/libxl/testidl
diff --git a/tools/Rules.mk b/tools/Rules.mk
index dbc7635..5e1c7cb 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -10,6 +10,7 @@ export _INSTALL := $(INSTALL)
 INSTALL = $(XEN_ROOT)/tools/cross-install
 
 XEN_INCLUDE        = $(XEN_ROOT)/tools/include
+XEN_LIBXENTOOLCORE  = $(XEN_ROOT)/tools/libs/toolcore
 XEN_LIBXENTOOLLOG  = $(XEN_ROOT)/tools/libs/toollog
 XEN_LIBXENEVTCHN   = $(XEN_ROOT)/tools/libs/evtchn
 XEN_LIBXENGNTTAB   = $(XEN_ROOT)/tools/libs/gnttab
@@ -102,6 +103,11 @@ SHDEPS_libxentoollog =
 LDLIBS_libxentoollog = $(SHDEPS_libxentoollog) $(XEN_LIBXENTOOLLOG)/libxentoollog$(libextension)
 SHLIB_libxentoollog  = $(SHDEPS_libxentoollog) -Wl,-rpath-link=$(XEN_LIBXENTOOLLOG)
 
+CFLAGS_libxentoolcore = -I$(XEN_LIBXENTOOLCORE)/include $(CFLAGS_xeninclude)
+SHDEPS_libxentoolcore =
+LDLIBS_libxentoolcore = $(SHDEPS_libxentoolcore) $(XEN_LIBXENTOOLCORE)/libxentoolcore$(libextension)
+SHLIB_libxentoolcore  = $(SHDEPS_libxentoolcore) -Wl,-rpath-link=$(XEN_LIBXENTOOLCORE)
+
 CFLAGS_libxenevtchn = -I$(XEN_LIBXENEVTCHN)/include $(CFLAGS_xeninclude)
 SHDEPS_libxenevtchn =
 LDLIBS_libxenevtchn = $(SHDEPS_libxenevtchn) $(XEN_LIBXENEVTCHN)/libxenevtchn$(libextension)
diff --git a/tools/libs/Makefile b/tools/libs/Makefile
index 2035873..ea9a64d 100644
--- a/tools/libs/Makefile
+++ b/tools/libs/Makefile
@@ -2,6 +2,7 @@ XEN_ROOT = $(CURDIR)/../..
 include $(XEN_ROOT)/tools/Rules.mk
 
 SUBDIRS-y :=
+SUBDIRS-y += toolcore
 SUBDIRS-y += toollog
 SUBDIRS-y += evtchn
 SUBDIRS-y += gnttab
diff --git a/tools/libs/toolcore/Makefile b/tools/libs/toolcore/Makefile
new file mode 100644
index 0000000..f86f839
--- /dev/null
+++ b/tools/libs/toolcore/Makefile
@@ -0,0 +1,101 @@
+XEN_ROOT = $(CURDIR)/../../..
+include $(XEN_ROOT)/tools/Rules.mk
+
+MAJOR	= 1
+MINOR	= 0
+SHLIB_LDFLAGS += -Wl,--version-script=libxentoolcore.map
+
+CFLAGS	+= -Werror -Wmissing-prototypes
+CFLAGS	+= -I./include
+
+SRCS-y	+= handlereg.c
+
+LIB_OBJS := $(patsubst %.c,%.o,$(SRCS-y))
+PIC_OBJS := $(patsubst %.c,%.opic,$(SRCS-y))
+
+LIB := libxentoolcore.a
+ifneq ($(nosharedlibs),y)
+LIB += libxentoolcore.so
+endif
+
+PKG_CONFIG := xentoolcore.pc
+PKG_CONFIG_VERSION := $(MAJOR).$(MINOR)
+
+ifneq ($(CONFIG_LIBXC_MINIOS),y)
+PKG_CONFIG_INST := $(PKG_CONFIG)
+$(PKG_CONFIG_INST): PKG_CONFIG_PREFIX = $(prefix)
+$(PKG_CONFIG_INST): PKG_CONFIG_INCDIR = $(includedir)
+$(PKG_CONFIG_INST): PKG_CONFIG_LIBDIR = $(libdir)
+endif
+
+PKG_CONFIG_LOCAL := $(foreach pc,$(PKG_CONFIG),$(PKG_CONFIG_DIR)/$(pc))
+
+$(PKG_CONFIG_LOCAL): PKG_CONFIG_PREFIX = $(XEN_ROOT)
+$(PKG_CONFIG_LOCAL): PKG_CONFIG_INCDIR = $(XEN_LIBXENTOOLCORE)/include
+$(PKG_CONFIG_LOCAL): PKG_CONFIG_LIBDIR = $(CURDIR)
+
+AUTOINCS=include/_xentoolcore_list.h
+
+.PHONY: all
+all: build
+
+.PHONY: build
+build:
+	$(MAKE) libs
+
+.PHONY: libs
+libs: headers.chk $(LIB) $(PKG_CONFIG_INST) $(PKG_CONFIG_LOCAL)
+
+%.o: $(AUTOINCS)
+%.opic: $(AUTOINCS)
+
+headers.chk: $(wildcard include/*.h) $(AUTOINCS)
+
+include/_xentoolcore_list.h: $(XEN_INCLUDE)/xen-external/bsd-sys-queue-h-seddery $(XEN_INCLUDE)/xen-external/bsd-sys-queue.h
+	$(PERL) $^ --prefix=xentoolcore >$@.new
+	$(call move-if-changed,$@.new,$@)
+
+libxentoolcore.a: $(LIB_OBJS)
+	$(AR) rc $@ $^
+
+libxentoolcore.so: libxentoolcore.so.$(MAJOR)
+	$(SYMLINK_SHLIB) $< $@
+libxentoolcore.so.$(MAJOR): libxentoolcore.so.$(MAJOR).$(MINOR)
+	$(SYMLINK_SHLIB) $< $@
+
+libxentoolcore.so.$(MAJOR).$(MINOR): $(PIC_OBJS) libxentoolcore.map
+	$(CC) $(LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxentoolcore.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $(PIC_OBJS) $(APPEND_LDFLAGS)
+
+.PHONY: install
+install: build
+	$(INSTALL_DIR) $(DESTDIR)$(libdir)
+	$(INSTALL_DIR) $(DESTDIR)$(includedir)
+	$(INSTALL_SHLIB) libxentoolcore.so.$(MAJOR).$(MINOR) $(DESTDIR)$(libdir)
+	$(INSTALL_DATA) libxentoolcore.a $(DESTDIR)$(libdir)
+	$(SYMLINK_SHLIB) libxentoolcore.so.$(MAJOR).$(MINOR) $(DESTDIR)$(libdir)/libxentoolcore.so.$(MAJOR)
+	$(SYMLINK_SHLIB) libxentoolcore.so.$(MAJOR) $(DESTDIR)$(libdir)/libxentoolcore.so
+	$(INSTALL_DATA) include/xentoolcore.h $(DESTDIR)$(includedir)
+	$(INSTALL_DATA) xentoolcore.pc $(DESTDIR)$(PKG_INSTALLDIR)
+
+.PHONY: uinstall
+uninstall:
+	rm -f $(DESTDIR)$(PKG_INSTALLDIR)/xentoolcore.pc
+	rm -f $(DESTDIR)$(includedir)/xentoolcore.h
+	rm -f $(DESTDIR)$(libdir)/libxentoolcore.so
+	rm -f $(DESTDIR)$(libdir)/libxentoolcore.so.$(MAJOR)
+	rm -f $(DESTDIR)$(libdir)/libxentoolcore.so.$(MAJOR).$(MINOR)
+	rm -f $(DESTDIR)$(libdir)/libxentoolcore.a
+
+.PHONY: TAGS
+TAGS:
+	etags -t *.c *.h
+
+.PHONY: clean
+clean:
+	rm -rf *.rpm $(LIB) *~ $(DEPS_RM) $(LIB_OBJS) $(PIC_OBJS)
+	rm -f libxentoolcore.so.$(MAJOR).$(MINOR) libxentoolcore.so.$(MAJOR)
+	rm -f headers.chk
+	rm -f xentoolcore.pc
+
+.PHONY: distclean
+distclean: clean
diff --git a/tools/libs/toolcore/handlereg.c b/tools/libs/toolcore/handlereg.c
new file mode 100644
index 0000000..cfd01a2
--- /dev/null
+++ b/tools/libs/toolcore/handlereg.c
@@ -0,0 +1,78 @@
+/*
+ * handlreg.c
+ *
+ * implementation of xentoolcore_restrict_all
+ *
+ * Copyright (c) 2017 Citrix
+ * Part of a generic logging interface used by various dom0 userland libraries.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation;
+ * version 2.1 of the License.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "xentoolcore_internal.h"
+
+#include <pthread.h>
+#include <assert.h>
+
+static pthread_mutex_t handles_lock = PTHREAD_MUTEX_INITIALIZER;
+static XENTOOLCORE_LIST_HEAD(, Xentoolcore__Active_Handle) handles;
+
+static void lock(void) {
+    int e = pthread_mutex_unlock(&handles_lock);
+    assert(!e);
+}
+
+static void unlock(void) {
+    int e = pthread_mutex_unlock(&handles_lock);
+    assert(!e);
+}
+
+void xentoolcore__register_active_handle(Xentoolcore__Active_Handle *ah) {
+    lock();
+    XENTOOLCORE_LIST_INSERT_HEAD(&handles, ah, entry);
+    unlock();
+}
+
+void xentoolcore__deregister_active_handle(Xentoolcore__Active_Handle *ah) {
+    lock();
+    XENTOOLCORE_LIST_REMOVE(ah, entry);
+    unlock();
+}
+
+int xentoolcore_restrict_all(uint32_t domid) {
+/* xxx lock */
+    int r;
+    Xentoolcore__Active_Handle *ah;
+
+    lock();
+    XENTOOLCORE_LIST_FOREACH(ah, &handles, entry) {
+        r = ah->restrict_callback(ah, domid);
+        if (r) goto out;
+    }
+
+    r = 0;
+ out:
+    unlock();
+    return r;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libs/toolcore/include/xentoolcore.h b/tools/libs/toolcore/include/xentoolcore.h
new file mode 100644
index 0000000..1ab646e
--- /dev/null
+++ b/tools/libs/toolcore/include/xentoolcore.h
@@ -0,0 +1,71 @@
+/*
+ * xentoolcore.h
+ *
+ * Copyright (c) 2017 Citrix
+ * 
+ * Common features used/provided by all Xen tools libraries
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation;
+ * version 2.1 of the License.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef XENTOOLCORE_H
+#define XENTOOLCORE_H
+
+#include <stdint.h>
+
+/*
+ * int xentoolcore_restrict_all(uint32_t domid);
+ *
+ * Arranges that Xen library handles (fds etc.) which are currently held
+ * by Xen libraries, can no longer be used other than to affect domid.
+ *
+ * If this cannot be achieved, returns -1 and sets errno.
+ * Idempotent if domid is always the same.
+ *
+ *  ====================================================================
+ *  IMPORTANT - IMPLEMENTATION STATUS
+ *
+ *  This function will be implemented insofar as it appears necessary
+ *  for the purposes of running a deprivileged qemu.
+ *
+ *  However, this function is NOT implemented for all Xen libraries.
+ *  For each use case of this function, the designer must evaluate and
+ *  audit whether the implementation is sufficient in their specific
+ *  context.
+ *
+ *  Of course, patches to extend the implementation are very welcome.
+ *  ====================================================================
+ *
+ * Thread safe.
+ *
+ * We expect that no callers do the following:
+ *   - in one thread call xen_somelibrary_open|close
+ *   - in another thread call fork
+ *   - in the child of the fork, before exec, call
+ *     xen_some[other]library_open|close or xentoolcore_restrict_all
+ *
+ */
+int xentoolcore_restrict_all(uint32_t domid);
+
+#endif /* XENTOOLCORE_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libs/toolcore/include/xentoolcore_internal.h b/tools/libs/toolcore/include/xentoolcore_internal.h
new file mode 100644
index 0000000..670e29d
--- /dev/null
+++ b/tools/libs/toolcore/include/xentoolcore_internal.h
@@ -0,0 +1,102 @@
+/*
+ * xentoolcore_internal.h
+ *
+ * Interfaces of xentoolcore directed internally at other Xen libraries
+ *
+ * Copyright (c) 2017 Citrix
+ * 
+ * Common code used by all Xen tools libraries
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation;
+ * version 2.1 of the License.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef XENTOOLCORE_INTERNAL_H
+#define XENTOOLCORE_INTERNAL_H
+
+#include "xentoolcore.h"
+#include "_xentoolcore_list.h"
+
+/*---------- active handle registration ----------*/
+
+/*
+ * This is all to support xentoolcore_restrict_all
+ *
+ * Any libxl library that opens a Xen control handle of any kind which
+ * might allow manipulation of dom0, of other domains, or of the whole
+ * machine, must:
+ *   I. arrange that their own datastructure contains a
+ *          Xentoolcore__Active_Handle
+ * 
+ *   II. during the "open handle" function
+ *     1. allocate the memory for the own datastructure and initialise it
+ *     2. set Xentoolcore__Active_Handle.restrict_callback
+ *     3. call xentoolcore__register_active_handle
+ *       3a. if the open fails, call xentoolcore__deregister_active_handle
+ *     4. ONLY THEN actually open the relevant fd or whatever
+ *
+ *   III. during the "close handle" function
+ *     1. FIRST close the relevant fd or whatever
+ *     2. call xentoolcore__deregister_active_handle
+ *
+ *   IV. in the restrict_callback function
+ *     * Arrange that the fd (or other handle) can no longer by used
+ *       other than with respect to domain domid.
+ *     * Future attempts to manipulate other domains (or the whole
+ *       host) via this handle must cause an error return (and
+ *       perhaps a log message), not a crash
+ *     * If selective restriction is not possible, the handle must
+ *       be completely invalidated so that it is not useable;
+ *       subsequent manipulations may not crash
+ *     * The restrict_callback function should not normally fail
+ *       if this can be easily avoided - it is better to make the
+ *       handle nonfunction instead.
+ *     * NB that restrict_callback might be called again.  That must
+ *       work properly: if the domid is the same, it is idempotent.
+ *       If the domid is different. then either the handle must be
+ *       completely invalidated, or restrict_callback must fail.)
+ *
+ * Thread safety:
+ *    xentoolcore__[de]register_active_handle are threadsafe
+ *      but MUST NOT be called within restrict_callback
+ *
+ * Fork safety:
+ *    Libraries which use these functions do not on that account
+ *    need to take any special care over forks occurring in
+ *    other threads, provided that they obey the rules above.
+ */
+
+typedef struct Xentoolcore__Active_Handle Xentoolcore__Active_Handle;
+
+typedef int Xentoolcore__Restrict_Callback(Xentoolcore__Active_Handle*,
+                                           uint32_t domid);
+
+struct Xentoolcore__Active_Handle {
+    Xentoolcore__Restrict_Callback *restrict_callback;
+    XENTOOLCORE_LIST_ENTRY(Xentoolcore__Active_Handle) entry;
+};
+
+void xentoolcore__register_active_handle(Xentoolcore__Active_Handle*);
+void xentoolcore__deregister_active_handle(Xentoolcore__Active_Handle*);
+
+#endif /* XENTOOLCORE_INTERNAL_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libs/toolcore/libxentoolcore.map b/tools/libs/toolcore/libxentoolcore.map
new file mode 100644
index 0000000..eb5d251
--- /dev/null
+++ b/tools/libs/toolcore/libxentoolcore.map
@@ -0,0 +1,7 @@
+VERS_1.0 {
+	global:
+		xentoolcore_restrict_all;
+		xentoolcore__register_active_handle;
+		xentoolcore__deregister_active_handle;
+	local: *; /* Do not expose anything by default */
+};
diff --git a/tools/libs/toolcore/xentoolcore.pc.in b/tools/libs/toolcore/xentoolcore.pc.in
new file mode 100644
index 0000000..55ff4e2
--- /dev/null
+++ b/tools/libs/toolcore/xentoolcore.pc.in
@@ -0,0 +1,9 @@
+prefix=@@prefix@@
+includedir=@@incdir@@
+libdir=@@libdir@@
+
+Name: Xentoolcore
+Description: Central support for Xen Hypervisor userland libraries
+Version: @@version@@
+Cflags: -I${includedir}
+Libs: @@libsflag@@${libdir} -lxentoolcore
-- 
2.1.4


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

[PATCH 04/22] tools: qemu-xen build: prepare to link against xentoolcore

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/Makefile b/tools/Makefile
index 11ad42c..03d326a 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -244,6 +244,7 @@ subdir-all-qemu-xen-dir: qemu-xen-dir-find
 		-DXC_WANT_COMPAT_MAP_FOREIGN_API=1 \
 		-DXC_WANT_COMPAT_DEVICEMODEL_API=1 \
 		-I$(XEN_ROOT)/tools/include \
+		-I$(XEN_ROOT)/tools/libs/toolcore/include \
 		-I$(XEN_ROOT)/tools/libs/toollog/include \
 		-I$(XEN_ROOT)/tools/libs/evtchn/include \
 		-I$(XEN_ROOT)/tools/libs/gnttab/include \
@@ -255,10 +256,12 @@ subdir-all-qemu-xen-dir: qemu-xen-dir-find
 		$(EXTRA_CFLAGS_QEMU_XEN)" \
 		--extra-ldflags="-L$(XEN_ROOT)/tools/libxc \
 		-L$(XEN_ROOT)/tools/xenstore \
+		-L$(XEN_ROOT)/tools/libs/toolcore \
 		-L$(XEN_ROOT)/tools/libs/evtchn \
 		-L$(XEN_ROOT)/tools/libs/gnttab \
 		-L$(XEN_ROOT)/tools/libs/foreignmemory \
 		-L$(XEN_ROOT)/tools/libs/devicemodel \
+		-Wl,-rpath-link=$(XEN_ROOT)/tools/libs/toolcore \
 		-Wl,-rpath-link=$(XEN_ROOT)/tools/libs/toollog \
 		-Wl,-rpath-link=$(XEN_ROOT)/tools/libs/evtchn \
 		-Wl,-rpath-link=$(XEN_ROOT)/tools/libs/gnttab \
-- 
2.1.4


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

[PATCH 05/22] libxl: #include "xentoolcore_internal.h"

[Ian Jackson]

We are going to want to move something here.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/Makefile         | 11 ++++++-----
 tools/libxl/libxl_internal.h |  2 ++
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index aee0a4c..2c2b047 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -20,12 +20,13 @@ LIBUUID_LIBS += -luuid
 endif
 
 LIBXL_LIBS =
-LIBXL_LIBS = $(LDLIBS_libxentoollog) $(LDLIBS_libxenevtchn) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
+LIBXL_LIBS = $(LDLIBS_libxentoollog) $(LDLIBS_libxenevtchn) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenstore) $(LDLIBS_libblktapctl) $(LDLIBS_libxentoolcore) $(PTYFUNCS_LIBS) $(LIBUUID_LIBS)
 ifeq ($(CONFIG_LIBNL),y)
 LIBXL_LIBS += $(LIBNL3_LIBS)
 endif
 
 CFLAGS_LIBXL += $(CFLAGS_libxentoollog)
+CFLAGS_LIBXL += $(CFLAGS_libxentoolcore)
 CFLAGS_LIBXL += $(CFLAGS_libxenevtchn)
 CFLAGS_LIBXL += $(CFLAGS_libxenctrl)
 CFLAGS_LIBXL += $(CFLAGS_libxenguest)
@@ -178,7 +179,7 @@ LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
 	libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o
 $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
 
-$(TEST_PROG_OBJS) _libxl.api-for-check: CFLAGS += $(CFLAGS_libxentoollog)
+$(TEST_PROG_OBJS) _libxl.api-for-check: CFLAGS += $(CFLAGS_libxentoollog) $(CFLAGS_libxentoolcore)
 
 CLIENTS = testidl libxl-save-helper
 
@@ -301,13 +302,13 @@ libxlutil.a: $(LIBXLU_OBJS)
 	$(AR) rcs libxlutil.a $^
 
 test_%: test_%.o test_common.o libxlutil.so libxenlight_test.so
-	$(CC) $(LDFLAGS) -o $@ $^ $(filter-out %libxenlight.so, $(LDLIBS_libxenlight)) $(LDLIBS_libxentoollog) -lyajl $(APPEND_LDFLAGS)
+	$(CC) $(LDFLAGS) -o $@ $^ $(filter-out %libxenlight.so, $(LDLIBS_libxenlight)) $(LDLIBS_libxentoollog) $(LDLIBS_libxentoolcore) -lyajl $(APPEND_LDFLAGS)
 
 libxl-save-helper: $(SAVE_HELPER_OBJS) libxenlight.so
-	$(CC) $(LDFLAGS) -o $@ $(SAVE_HELPER_OBJS) $(LDLIBS_libxentoollog) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(APPEND_LDFLAGS)
+	$(CC) $(LDFLAGS) -o $@ $(SAVE_HELPER_OBJS) $(LDLIBS_libxentoollog) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxentoolcore) $(APPEND_LDFLAGS)
 
 testidl: testidl.o libxlutil.so libxenlight.so
-	$(CC) $(LDFLAGS) -o $@ testidl.o libxlutil.so $(LDLIBS_libxenlight) $(LDLIBS_libxentoollog) $(APPEND_LDFLAGS)
+	$(CC) $(LDFLAGS) -o $@ testidl.o libxlutil.so $(LDLIBS_libxenlight) $(LDLIBS_libxentoollog) $(LDLIBS_libxentoolcore) $(APPEND_LDFLAGS)
 
 .PHONY: install
 install: all
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 7247509..04be10a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -19,6 +19,8 @@
 
 #include "libxl_osdeps.h" /* must come before any other headers */
 
+#include "xentoolcore_internal.h"
+
 #include "libxl_sr_stream_format.h"
 
 #include <assert.h>
-- 
2.1.4


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

[PATCH 06/22] tools: move CONTAINER_OF to xentoolcore_internal.h

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libs/toolcore/include/xentoolcore_internal.h | 42 ++++++++++++++++++++++
 tools/libxl/libxl_internal.h                       | 30 ----------------
 2 files changed, 42 insertions(+), 30 deletions(-)

diff --git a/tools/libs/toolcore/include/xentoolcore_internal.h b/tools/libs/toolcore/include/xentoolcore_internal.h
index 670e29d..27497d6 100644
--- a/tools/libs/toolcore/include/xentoolcore_internal.h
+++ b/tools/libs/toolcore/include/xentoolcore_internal.h
@@ -24,6 +24,8 @@
 #ifndef XENTOOLCORE_INTERNAL_H
 #define XENTOOLCORE_INTERNAL_H
 
+#include <stddef.h>
+
 #include "xentoolcore.h"
 #include "_xentoolcore_list.h"
 
@@ -89,6 +91,46 @@ struct Xentoolcore__Active_Handle {
 void xentoolcore__register_active_handle(Xentoolcore__Active_Handle*);
 void xentoolcore__deregister_active_handle(Xentoolcore__Active_Handle*);
 
+/* ---------- convenient stuff ---------- */
+
+/*
+ * This does not appear in xentoolcore.h because it is a bit
+ * namespace-unclean.
+ */
+
+/*
+ * Convenience macros.
+ */
+
+/*
+ * CONTAINER_OF work like this.  Given:
+ *    typedef struct {
+ *      ...
+ *      member_type member_name;
+ *      ...
+ *    } outer_type;
+ *    outer_type outer, *outer_var;
+ *    member_type *inner_ptr = &outer->member_name;
+ *
+ * Then, effectively:
+ *    outer_type *CONTAINER_OF(member_type *inner_ptr,
+ *                             *outer_var, // or type name for outer_type
+ *                             member_name);
+ *
+ * So that:
+ *    CONTAINER_OF(inner_ptr, *outer_var, member_name) == &outer
+ *    CONTAINER_OF(inner_ptr, outer_type, member_name) == &outer
+ */
+#define CONTAINER_OF(inner_ptr, outer, member_name)                     \
+    ({                                                                  \
+        typeof(outer) *container_of_;                                   \
+        container_of_ = (void*)((char*)(inner_ptr) -                    \
+                                offsetof(typeof(outer), member_name));  \
+        (void)(&container_of_->member_name ==                           \
+               (typeof(inner_ptr))0) /* type check */;                  \
+        container_of_;                                                  \
+    })
+
 #endif /* XENTOOLCORE_INTERNAL_H */
 
 /*
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 04be10a..4f2f433 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3804,36 +3804,6 @@ _hidden void libxl__remus_restore_setup(libxl__egc *egc,
  * Convenience macros.
  */
 
-/*
- * CONTAINER_OF work like this.  Given:
- *    typedef struct {
- *      ...
- *      member_type member_name;
- *      ...
- *    } outer_type;
- *    outer_type outer, *outer_var;
- *    member_type *inner_ptr = &outer->member_name;
- *
- * Then, effectively:
- *    outer_type *CONTAINER_OF(member_type *inner_ptr,
- *                             *outer_var, // or type name for outer_type
- *                             member_name);
- *
- * So that:
- *    CONTAINER_OF(inner_ptr, *outer_var, member_name) == &outer
- *    CONTAINER_OF(inner_ptr, outer_type, member_name) == &outer
- */
-#define CONTAINER_OF(inner_ptr, outer, member_name)                     \
-    ({                                                                  \
-        typeof(outer) *container_of_;                                   \
-        container_of_ = (void*)((char*)(inner_ptr) -                    \
-                                offsetof(typeof(outer), member_name));  \
-        (void)(&container_of_->member_name ==                           \
-               (typeof(inner_ptr))0) /* type check */;                  \
-        container_of_;                                                  \
-    })
-
-
 #define FILLZERO LIBXL_FILLZERO
 
 
-- 
2.1.4


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

[PATCH 07/22] xentoolcore_restrict_all: Implement for libxendevicemodel

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/Rules.mk                              |  2 +-
 tools/libs/devicemodel/Makefile             |  3 ++-
 tools/libs/devicemodel/core.c               | 16 ++++++++++++++++
 tools/libs/devicemodel/private.h            |  3 +++
 tools/libs/devicemodel/xendevicemodel.pc.in |  2 +-
 5 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/tools/Rules.mk b/tools/Rules.mk
index 5e1c7cb..9b2fe36 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -129,7 +129,7 @@ LDLIBS_libxenforeignmemory = $(SHDEPS_libxenforeignmemory) $(XEN_LIBXENFOREIGNME
 SHLIB_libxenforeignmemory  = $(SHDEPS_libxenforeignmemory) -Wl,-rpath-link=$(XEN_LIBXENFOREIGNMEMORY)
 
 CFLAGS_libxendevicemodel = -I$(XEN_LIBXENDEVICEMODEL)/include $(CFLAGS_xeninclude)
-SHDEPS_libxendevicemodel = $(SHLIB_libxentoollog) $(SHLIB_xencall)
+SHDEPS_libxendevicemodel = $(SHLIB_libxentoollog) $(SHLIB_libxentoolcore) $(SHLIB_xencall)
 LDLIBS_libxendevicemodel = $(SHDEPS_libxendevicemodel) $(XEN_LIBXENDEVICEMODEL)/libxendevicemodel$(libextension)
 SHLIB_libxendevicemodel  = $(SHDEPS_libxendevicemodel) -Wl,-rpath-link=$(XEN_LIBXENDEVICEMODEL)
 
diff --git a/tools/libs/devicemodel/Makefile b/tools/libs/devicemodel/Makefile
index f0e1e6c..3f7efd5 100644
--- a/tools/libs/devicemodel/Makefile
+++ b/tools/libs/devicemodel/Makefile
@@ -8,6 +8,7 @@ SHLIB_LDFLAGS += -Wl,--version-script=libxendevicemodel.map
 CFLAGS   += -Werror -Wmissing-prototypes
 CFLAGS   += -I./include $(CFLAGS_xeninclude)
 CFLAGS   += $(CFLAGS_libxentoollog)
+CFLAGS   += $(CFLAGS_libxentoolcore)
 CFLAGS   += $(CFLAGS_libxencall)
 
 SRCS-y                 += core.c
@@ -63,7 +64,7 @@ libxendevicemodel.so.$(MAJOR): libxendevicemodel.so.$(MAJOR).$(MINOR)
 	$(SYMLINK_SHLIB) $< $@
 
 libxendevicemodel.so.$(MAJOR).$(MINOR): $(PIC_OBJS) libxendevicemodel.map
-	$(CC) $(LDFLAGS) $(PTHREAD_LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxendevicemodel.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $(PIC_OBJS) $(LDLIBS_libxentoollog) $(LDLIBS_libxencall) $(APPEND_LDFLAGS)
+	$(CC) $(LDFLAGS) $(PTHREAD_LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxendevicemodel.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $(PIC_OBJS) $(LDLIBS_libxentoollog) $(LDLIBS_libxencall) $(LDLIBS_libxentoolcore) $(APPEND_LDFLAGS)
 
 .PHONY: install
 install: build
diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
index 2093884..3292e53 100644
--- a/tools/libs/devicemodel/core.c
+++ b/tools/libs/devicemodel/core.c
@@ -21,6 +21,16 @@
 
 #include "private.h"
 
+static int all_restrict_cb(Xentoolcore__Active_Handle *ah, uint32_t domid) {
+    xendevicemodel_handle *dmod = CONTAINER_OF(ah, *dmod, tc_ah);
+
+    if (dmod->fd < 0)
+        /* just in case */
+        return 0;
+
+    return xendevicemodel_restrict(dmod, domid);
+}
+
 xendevicemodel_handle *xendevicemodel_open(xentoollog_logger *logger,
                                            unsigned open_flags)
 {
@@ -30,6 +40,10 @@ xendevicemodel_handle *xendevicemodel_open(xentoollog_logger *logger,
     if (!dmod)
         return NULL;
 
+    dmod->fd = -1;
+    dmod->tc_ah.restrict_callback = all_restrict_cb;
+    xentoolcore__register_active_handle(&dmod->tc_ah);
+
     dmod->flags = open_flags;
     dmod->logger = logger;
     dmod->logger_tofree = NULL;
@@ -55,6 +69,7 @@ xendevicemodel_handle *xendevicemodel_open(xentoollog_logger *logger,
 err:
     xtl_logger_destroy(dmod->logger_tofree);
     xencall_close(dmod->xcall);
+    xentoolcore__deregister_active_handle(&dmod->tc_ah);
     free(dmod);
     return NULL;
 }
@@ -69,6 +84,7 @@ int xendevicemodel_close(xendevicemodel_handle *dmod)
     rc = osdep_xendevicemodel_close(dmod);
 
     xencall_close(dmod->xcall);
+    xentoolcore__deregister_active_handle(&dmod->tc_ah);
     xtl_logger_destroy(dmod->logger_tofree);
     free(dmod);
     return rc;
diff --git a/tools/libs/devicemodel/private.h b/tools/libs/devicemodel/private.h
index 4ce5aac..c4a225f 100644
--- a/tools/libs/devicemodel/private.h
+++ b/tools/libs/devicemodel/private.h
@@ -7,11 +7,14 @@
 #include <xendevicemodel.h>
 #include <xencall.h>
 
+#include <xentoolcore_internal.h>
+
 struct xendevicemodel_handle {
     xentoollog_logger *logger, *logger_tofree;
     unsigned int flags;
     xencall_handle *xcall;
     int fd;
+    Xentoolcore__Active_Handle tc_ah;
 };
 
 struct xendevicemodel_buf {
diff --git a/tools/libs/devicemodel/xendevicemodel.pc.in b/tools/libs/devicemodel/xendevicemodel.pc.in
index ed08f83..8bd04fa 100644
--- a/tools/libs/devicemodel/xendevicemodel.pc.in
+++ b/tools/libs/devicemodel/xendevicemodel.pc.in
@@ -7,4 +7,4 @@ Description: The Xendevicemodel library for Xen hypervisor
 Version: @@version@@
 Cflags: -I${includedir} @@cflagslocal@@
 Libs: @@libsflag@@${libdir} -lxendevicemodel
-Requires.private: xentoollog,xencall
+Requires.private: xentoolcore,xentoollog,xencall
-- 
2.1.4


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

[PATCH 08/22] xentoolcore_restrict_all: "Implement" for libxencall

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/Rules.mk                |  2 +-
 tools/libs/call/Makefile      |  4 ++--
 tools/libs/call/core.c        | 37 +++++++++++++++++++++++++++++++++++++
 tools/libs/call/linux.c       |  4 ++++
 tools/libs/call/private.h     |  2 ++
 tools/libs/call/xencall.pc.in |  2 +-
 6 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/tools/Rules.mk b/tools/Rules.mk
index 9b2fe36..71037a1 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -119,7 +119,7 @@ LDLIBS_libxengnttab = $(SHDEPS_libxengnttab) $(XEN_LIBXENGNTTAB)/libxengnttab$(l
 SHLIB_libxengnttab  = $(SHDEPS_libxengnttab) -Wl,-rpath-link=$(XEN_LIBXENGNTTAB)
 
 CFLAGS_libxencall = -I$(XEN_LIBXENCALL)/include $(CFLAGS_xeninclude)
-SHDEPS_libxencall =
+SHDEPS_libxencall = $(SHLIB_libxentoolcore)
 LDLIBS_libxencall = $(SHDEPS_libxencall) $(XEN_LIBXENCALL)/libxencall$(libextension)
 SHLIB_libxencall  = $(SHDEPS_libxencall) -Wl,-rpath-link=$(XEN_LIBXENCALL)
 
diff --git a/tools/libs/call/Makefile b/tools/libs/call/Makefile
index 1ccd5fd..39dd207 100644
--- a/tools/libs/call/Makefile
+++ b/tools/libs/call/Makefile
@@ -7,7 +7,7 @@ SHLIB_LDFLAGS += -Wl,--version-script=libxencall.map
 
 CFLAGS   += -Werror -Wmissing-prototypes
 CFLAGS   += -I./include $(CFLAGS_xeninclude)
-CFLAGS   += $(CFLAGS_libxentoollog)
+CFLAGS   += $(CFLAGS_libxentoollog) $(CFLAGS_libxentoolcore)
 
 SRCS-y                 += core.c buffer.c
 SRCS-$(CONFIG_Linux)   += linux.c
@@ -62,7 +62,7 @@ libxencall.so.$(MAJOR): libxencall.so.$(MAJOR).$(MINOR)
 	$(SYMLINK_SHLIB) $< $@
 
 libxencall.so.$(MAJOR).$(MINOR): $(PIC_OBJS) libxencall.map
-	$(CC) $(LDFLAGS) $(PTHREAD_LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxencall.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $(PIC_OBJS) $(LDLIBS_libxentoollog) $(APPEND_LDFLAGS)
+	$(CC) $(LDFLAGS) $(PTHREAD_LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxencall.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $(PIC_OBJS) $(LDLIBS_libxentoollog) $(LDLIBS_libxentoolcore) $(APPEND_LDFLAGS)
 
 .PHONY: install
 install: build
diff --git a/tools/libs/call/core.c b/tools/libs/call/core.c
index 5ca0372..11ecc87 100644
--- a/tools/libs/call/core.c
+++ b/tools/libs/call/core.c
@@ -15,8 +15,41 @@
 
 #include <stdlib.h>
 
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
+
 #include "private.h"
 
+static int all_restrict_cb(Xentoolcore__Active_Handle *ah, uint32_t domid) {
+    xencall_handle *xcall = CONTAINER_OF(ah, *xcall, tc_ah);
+    int nullfd = -1, r;
+
+    if (xcall->fd < 0)
+        /* just in case */
+        return 0;
+
+    /*
+     * We don't implement a restrict function.  We neuter the fd by
+     * dup'ing /dev/null onto it.  This is better than closing it,
+     * because it does not involve locking against concurrent uses
+     * of xencall in other threads.
+     */
+    nullfd = open("/dev/null",O_RDONLY);
+    if (nullfd < 0) goto err;
+
+    r = dup2(nullfd, xcall->fd);
+    if (r < 0) goto err;
+
+    close(nullfd);
+    return 0;
+
+err:
+    if (nullfd >= 0) close(nullfd);
+    return -1;
+}
+
 xencall_handle *xencall_open(xentoollog_logger *logger, unsigned open_flags)
 {
     xencall_handle *xcall = malloc(sizeof(*xcall));
@@ -25,6 +58,8 @@ xencall_handle *xencall_open(xentoollog_logger *logger, unsigned open_flags)
     if (!xcall) return NULL;
 
     xcall->fd = -1;
+    xcall->tc_ah.restrict_callback = all_restrict_cb;
+    xentoolcore__register_active_handle(&xcall->tc_ah);
 
     xcall->flags = open_flags;
     xcall->buffer_cache_nr = 0;
@@ -53,6 +88,7 @@ xencall_handle *xencall_open(xentoollog_logger *logger, unsigned open_flags)
 
 err:
     osdep_xencall_close(xcall);
+    xentoolcore__deregister_active_handle(&xcall->tc_ah);
     xtl_logger_destroy(xcall->logger_tofree);
     free(xcall);
     return NULL;
@@ -66,6 +102,7 @@ int xencall_close(xencall_handle *xcall)
         return 0;
 
     rc = osdep_xencall_close(xcall);
+    xentoolcore__deregister_active_handle(&xcall->tc_ah);
     buffer_release_cache(xcall);
     xtl_logger_destroy(xcall->logger_tofree);
     free(xcall);
diff --git a/tools/libs/call/linux.c b/tools/libs/call/linux.c
index e8e0311..3f1b691 100644
--- a/tools/libs/call/linux.c
+++ b/tools/libs/call/linux.c
@@ -21,6 +21,10 @@
 #include <fcntl.h>
 #include <unistd.h>
 
+#include <stdlib.h>
+#include <assert.h>
+#include <stdio.h>
+
 #include <sys/mman.h>
 #include <sys/ioctl.h>
 
diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
index 37dd15f..533f0c4 100644
--- a/tools/libs/call/private.h
+++ b/tools/libs/call/private.h
@@ -2,6 +2,7 @@
 #define XENCALL_PRIVATE_H
 
 #include <xentoollog.h>
+#include <xentoolcore_internal.h>
 
 #include <xencall.h>
 
@@ -20,6 +21,7 @@ struct xencall_handle {
     xentoollog_logger *logger, *logger_tofree;
     unsigned flags;
     int fd;
+    Xentoolcore__Active_Handle tc_ah;
 
     /*
      * A simple cache of unused, single page, hypercall buffers
diff --git a/tools/libs/call/xencall.pc.in b/tools/libs/call/xencall.pc.in
index 475c133..409773e 100644
--- a/tools/libs/call/xencall.pc.in
+++ b/tools/libs/call/xencall.pc.in
@@ -7,4 +7,4 @@ Description: The Xencall library for Xen hypervisor
 Version: @@version@@
 Cflags: -I${includedir} @@cflagslocal@@
 Libs: @@libsflag@@${libdir} -lxencall
-Requires.private: xentoollog
+Requires.private: xentoollog,xentoolcore
-- 
2.1.4


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

[PATCH 09/22] xentoolcore_restrict: Break out xentoolcore__restrict_by_dup2_null

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libs/call/core.c                             | 30 +---------------------
 tools/libs/toolcore/handlereg.c                    | 26 +++++++++++++++++++
 tools/libs/toolcore/include/xentoolcore_internal.h | 12 +++++++++
 tools/libs/toolcore/libxentoolcore.map             |  1 +
 4 files changed, 40 insertions(+), 29 deletions(-)

diff --git a/tools/libs/call/core.c b/tools/libs/call/core.c
index 11ecc87..d6ce73d 100644
--- a/tools/libs/call/core.c
+++ b/tools/libs/call/core.c
@@ -15,39 +15,11 @@
 
 #include <stdlib.h>
 
-#include <sys/types.h>
-#include <sys/stat.h>
-#include <fcntl.h>
-#include <unistd.h>
-
 #include "private.h"
 
 static int all_restrict_cb(Xentoolcore__Active_Handle *ah, uint32_t domid) {
     xencall_handle *xcall = CONTAINER_OF(ah, *xcall, tc_ah);
-    int nullfd = -1, r;
-
-    if (xcall->fd < 0)
-        /* just in case */
-        return 0;
-
-    /*
-     * We don't implement a restrict function.  We neuter the fd by
-     * dup'ing /dev/null onto it.  This is better than closing it,
-     * because it does not involve locking against concurrent uses
-     * of xencall in other threads.
-     */
-    nullfd = open("/dev/null",O_RDONLY);
-    if (nullfd < 0) goto err;
-
-    r = dup2(nullfd, xcall->fd);
-    if (r < 0) goto err;
-
-    close(nullfd);
-    return 0;
-
-err:
-    if (nullfd >= 0) close(nullfd);
-    return -1;
+    return xentoolcore__restrict_by_dup2_null(xcall->fd);
 }
 
 xencall_handle *xencall_open(xentoollog_logger *logger, unsigned open_flags)
diff --git a/tools/libs/toolcore/handlereg.c b/tools/libs/toolcore/handlereg.c
index cfd01a2..56d8b2d 100644
--- a/tools/libs/toolcore/handlereg.c
+++ b/tools/libs/toolcore/handlereg.c
@@ -22,6 +22,11 @@
 
 #include "xentoolcore_internal.h"
 
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
+
 #include <pthread.h>
 #include <assert.h>
 
@@ -67,6 +72,27 @@ int xentoolcore_restrict_all(uint32_t domid) {
     return r;
 }
 
+int xentoolcore__restrict_by_dup2_null(int fd) {
+    int nullfd = -1, r;
+
+    if (fd < 0)
+        /* just in case */
+        return 0;
+
+    nullfd = open("/dev/null",O_RDONLY);
+    if (nullfd < 0) goto err;
+
+    r = dup2(nullfd, fd);
+    if (r < 0) goto err;
+
+    close(nullfd);
+    return 0;
+
+err:
+    if (nullfd >= 0) close(nullfd);
+    return -1;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/toolcore/include/xentoolcore_internal.h b/tools/libs/toolcore/include/xentoolcore_internal.h
index 27497d6..7e96a48 100644
--- a/tools/libs/toolcore/include/xentoolcore_internal.h
+++ b/tools/libs/toolcore/include/xentoolcore_internal.h
@@ -91,6 +91,18 @@ struct Xentoolcore__Active_Handle {
 void xentoolcore__register_active_handle(Xentoolcore__Active_Handle*);
 void xentoolcore__deregister_active_handle(Xentoolcore__Active_Handle*);
 
+/*
+ * Utility function for use in restrict_callback in libraries whose
+ * handles don't have a useful restrict function.  We neuter the fd by
+ * dup'ing /dev/null onto it.  This is better than closing it, because
+ * it does not involve locking against concurrent uses of in other
+ * threads.
+ *
+ * Returns the value that restrict_callback should return.
+ * fd may be < 0.
+ */
+int xentoolcore__restrict_by_dup2_null(int fd);
+
 /* ---------- convenient stuff ---------- */
 
 /*
diff --git a/tools/libs/toolcore/libxentoolcore.map b/tools/libs/toolcore/libxentoolcore.map
index eb5d251..0b7d925 100644
--- a/tools/libs/toolcore/libxentoolcore.map
+++ b/tools/libs/toolcore/libxentoolcore.map
@@ -3,5 +3,6 @@ VERS_1.0 {
 		xentoolcore_restrict_all;
 		xentoolcore__register_active_handle;
 		xentoolcore__deregister_active_handle;
+		xentoolcore__restrict_by_dup2_null;
 	local: *; /* Do not expose anything by default */
 };
-- 
2.1.4


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

[PATCH 10/22] xentoolcore_restrict_all: Implement for libxenforeignmemory

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/Rules.mk                                  |  2 +-
 tools/libs/foreignmemory/Makefile               |  4 ++--
 tools/libs/foreignmemory/core.c                 | 15 +++++++++++++++
 tools/libs/foreignmemory/private.h              |  3 +++
 tools/libs/foreignmemory/xenforeignmemory.pc.in |  2 +-
 5 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/tools/Rules.mk b/tools/Rules.mk
index 71037a1..7dd126a 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -124,7 +124,7 @@ LDLIBS_libxencall = $(SHDEPS_libxencall) $(XEN_LIBXENCALL)/libxencall$(libextens
 SHLIB_libxencall  = $(SHDEPS_libxencall) -Wl,-rpath-link=$(XEN_LIBXENCALL)
 
 CFLAGS_libxenforeignmemory = -I$(XEN_LIBXENFOREIGNMEMORY)/include $(CFLAGS_xeninclude)
-SHDEPS_libxenforeignmemory =
+SHDEPS_libxenforeignmemory = $(SHLIB_libxentoolcore)
 LDLIBS_libxenforeignmemory = $(SHDEPS_libxenforeignmemory) $(XEN_LIBXENFOREIGNMEMORY)/libxenforeignmemory$(libextension)
 SHLIB_libxenforeignmemory  = $(SHDEPS_libxenforeignmemory) -Wl,-rpath-link=$(XEN_LIBXENFOREIGNMEMORY)
 
diff --git a/tools/libs/foreignmemory/Makefile b/tools/libs/foreignmemory/Makefile
index ab7f873..cbe815f 100644
--- a/tools/libs/foreignmemory/Makefile
+++ b/tools/libs/foreignmemory/Makefile
@@ -7,7 +7,7 @@ SHLIB_LDFLAGS += -Wl,--version-script=libxenforeignmemory.map
 
 CFLAGS   += -Werror -Wmissing-prototypes
 CFLAGS   += -I./include $(CFLAGS_xeninclude)
-CFLAGS   += $(CFLAGS_libxentoollog)
+CFLAGS   += $(CFLAGS_libxentoollog) $(CFLAGS_libxentoolcore)
 
 SRCS-y                 += core.c
 SRCS-$(CONFIG_Linux)   += linux.c
@@ -62,7 +62,7 @@ libxenforeignmemory.so.$(MAJOR): libxenforeignmemory.so.$(MAJOR).$(MINOR)
 	$(SYMLINK_SHLIB) $< $@
 
 libxenforeignmemory.so.$(MAJOR).$(MINOR): $(PIC_OBJS) libxenforeignmemory.map
-	$(CC) $(LDFLAGS) $(PTHREAD_LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxenforeignmemory.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $(PIC_OBJS) $(LDLIBS_libxentoollog) $(APPEND_LDFLAGS)
+	$(CC) $(LDFLAGS) $(PTHREAD_LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxenforeignmemory.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $(PIC_OBJS) $(LDLIBS_libxentoollog) $(LDLIBS_libxentoolcore) $(APPEND_LDFLAGS)
 
 .PHONY: install
 install: build
diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
index a6897dc..b48ecba 100644
--- a/tools/libs/foreignmemory/core.c
+++ b/tools/libs/foreignmemory/core.c
@@ -19,6 +19,16 @@
 
 #include "private.h"
 
+static int all_restrict_cb(Xentoolcore__Active_Handle *ah, uint32_t domid) {
+    xenforeignmemory_handle *fmem = CONTAINER_OF(ah, *fmem, tc_ah);
+
+    if (fmem->fd < 0)
+        /* just in case */
+        return 0;
+
+    return xenforeignmemory_restrict(fmem, domid);
+}
+
 xenforeignmemory_handle *xenforeignmemory_open(xentoollog_logger *logger,
                                                unsigned open_flags)
 {
@@ -31,6 +41,9 @@ xenforeignmemory_handle *xenforeignmemory_open(xentoollog_logger *logger,
     fmem->logger = logger;
     fmem->logger_tofree = NULL;
 
+    fmem->tc_ah.restrict_callback = all_restrict_cb;
+    xentoolcore__register_active_handle(&fmem->tc_ah);
+
     if (!fmem->logger) {
         fmem->logger = fmem->logger_tofree =
             (xentoollog_logger*)
@@ -45,6 +58,7 @@ xenforeignmemory_handle *xenforeignmemory_open(xentoollog_logger *logger,
 
 err:
     osdep_xenforeignmemory_close(fmem);
+    xentoolcore__deregister_active_handle(&fmem->tc_ah);
     xtl_logger_destroy(fmem->logger_tofree);
     free(fmem);
     return NULL;
@@ -58,6 +72,7 @@ int xenforeignmemory_close(xenforeignmemory_handle *fmem)
         return 0;
 
     rc = osdep_xenforeignmemory_close(fmem);
+    xentoolcore__deregister_active_handle(&fmem->tc_ah);
     xtl_logger_destroy(fmem->logger_tofree);
     free(fmem);
     return rc;
diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h
index c5c07cc..2470f3c 100644
--- a/tools/libs/foreignmemory/private.h
+++ b/tools/libs/foreignmemory/private.h
@@ -5,6 +5,8 @@
 
 #include <xenforeignmemory.h>
 
+#include <xentoolcore_internal.h>
+
 #include <xen/xen.h>
 #include <xen/sys/privcmd.h>
 
@@ -20,6 +22,7 @@ struct xenforeignmemory_handle {
     xentoollog_logger *logger, *logger_tofree;
     unsigned flags;
     int fd;
+    Xentoolcore__Active_Handle tc_ah;
 };
 
 int osdep_xenforeignmemory_open(xenforeignmemory_handle *fmem);
diff --git a/tools/libs/foreignmemory/xenforeignmemory.pc.in b/tools/libs/foreignmemory/xenforeignmemory.pc.in
index 63432dc..61c9def 100644
--- a/tools/libs/foreignmemory/xenforeignmemory.pc.in
+++ b/tools/libs/foreignmemory/xenforeignmemory.pc.in
@@ -7,4 +7,4 @@ Description: The Xenforeignmemory library for Xen hypervisor
 Version: @@version@@
 Cflags: -I${includedir} @@cflagslocal@@
 Libs: @@libsflag@@${libdir} -lxenforeignmemory
-Requires.private: xentoollog
+Requires.private: xentoollog,xentoolcore
-- 
2.1.4


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

[PATCH 11/22] xentoolcore_restrict_all: Declare problems due to no evtchn support

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libs/toolcore/include/xentoolcore.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/libs/toolcore/include/xentoolcore.h b/tools/libs/toolcore/include/xentoolcore.h
index 1ab646e..1210d7d 100644
--- a/tools/libs/toolcore/include/xentoolcore.h
+++ b/tools/libs/toolcore/include/xentoolcore.h
@@ -30,6 +30,11 @@
  * Arranges that Xen library handles (fds etc.) which are currently held
  * by Xen libraries, can no longer be used other than to affect domid.
  *
+ * Does not prevent effects that amount only to
+ *   - denial of service, possibly host-wide, by resource exhaustion etc.
+ *   - leak of not-very-interesting metainformation about other domains
+ *     eg, specifically, event channel signals relating to other domains
+ *
  * If this cannot be achieved, returns -1 and sets errno.
  * Idempotent if domid is always the same.
  *
-- 
2.1.4


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

[PATCH 12/22] xentoolcore_restrict_all: "Implement" for xengnttab

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/Rules.mk                    |  2 +-
 tools/libs/gnttab/Makefile        |  4 ++--
 tools/libs/gnttab/gnttab_core.c   | 10 ++++++++++
 tools/libs/gnttab/private.h       |  2 ++
 tools/libs/gnttab/xengnttab.pc.in |  2 +-
 5 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/tools/Rules.mk b/tools/Rules.mk
index 7dd126a..3239e76 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -114,7 +114,7 @@ LDLIBS_libxenevtchn = $(SHDEPS_libxenevtchn) $(XEN_LIBXENEVTCHN)/libxenevtchn$(l
 SHLIB_libxenevtchn  = $(SHDEPS_libxenevtchn) -Wl,-rpath-link=$(XEN_LIBXENEVTCHN)
 
 CFLAGS_libxengnttab = -I$(XEN_LIBXENGNTTAB)/include $(CFLAGS_xeninclude)
-SHDEPS_libxengnttab = $(SHLIB_libxentoollog)
+SHDEPS_libxengnttab = $(SHLIB_libxentoollog) $(SHLIB_libxentoolcore)
 LDLIBS_libxengnttab = $(SHDEPS_libxengnttab) $(XEN_LIBXENGNTTAB)/libxengnttab$(libextension)
 SHLIB_libxengnttab  = $(SHDEPS_libxengnttab) -Wl,-rpath-link=$(XEN_LIBXENGNTTAB)
 
diff --git a/tools/libs/gnttab/Makefile b/tools/libs/gnttab/Makefile
index afb924f..dcfe686 100644
--- a/tools/libs/gnttab/Makefile
+++ b/tools/libs/gnttab/Makefile
@@ -7,7 +7,7 @@ SHLIB_LDFLAGS += -Wl,--version-script=libxengnttab.map
 
 CFLAGS   += -Werror -Wmissing-prototypes
 CFLAGS   += -I./include $(CFLAGS_xeninclude)
-CFLAGS   += $(CFLAGS_libxentoollog)
+CFLAGS   += $(CFLAGS_libxentoollog) $(CFLAGS_libxentoolcore)
 
 SRCS-GNTTAB            += gnttab_core.c
 SRCS-GNTSHR            += gntshr_core.c
@@ -64,7 +64,7 @@ libxengnttab.so.$(MAJOR): libxengnttab.so.$(MAJOR).$(MINOR)
 	$(SYMLINK_SHLIB) $< $@
 
 libxengnttab.so.$(MAJOR).$(MINOR): $(PIC_OBJS) libxengnttab.map
-	$(CC) $(LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxengnttab.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $(PIC_OBJS) $(LDLIBS_libxentoollog) $(APPEND_LDFLAGS)
+	$(CC) $(LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxengnttab.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $(PIC_OBJS) $(LDLIBS_libxentoollog) $(LDLIBS_libxentoolcore) $(APPEND_LDFLAGS)
 
 .PHONY: install
 install: build
diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c
index 968c833..bc88110 100644
--- a/tools/libs/gnttab/gnttab_core.c
+++ b/tools/libs/gnttab/gnttab_core.c
@@ -22,6 +22,11 @@
 
 #include "private.h"
 
+static int all_restrict_cb(Xentoolcore__Active_Handle *ah, uint32_t domid) {
+    xengnttab_handle *xgt = CONTAINER_OF(ah, *xgt, tc_ah);
+    return xentoolcore__restrict_by_dup2_null(xgt->fd);
+}
+
 xengnttab_handle *xengnttab_open(xentoollog_logger *logger, unsigned open_flags)
 {
     xengnttab_handle *xgt = malloc(sizeof(*xgt));
@@ -33,6 +38,9 @@ xengnttab_handle *xengnttab_open(xentoollog_logger *logger, unsigned open_flags)
     xgt->logger = logger;
     xgt->logger_tofree  = NULL;
 
+    xgt->tc_ah.restrict_callback = all_restrict_cb;
+    xentoolcore__register_active_handle(&xgt->tc_ah);
+
     if (!xgt->logger) {
         xgt->logger = xgt->logger_tofree =
             (xentoollog_logger*)
@@ -47,6 +55,7 @@ xengnttab_handle *xengnttab_open(xentoollog_logger *logger, unsigned open_flags)
 
 err:
     osdep_gnttab_close(xgt);
+    xentoolcore__deregister_active_handle(&xgt->tc_ah);
     xtl_logger_destroy(xgt->logger_tofree);
     free(xgt);
     return NULL;
@@ -60,6 +69,7 @@ int xengnttab_close(xengnttab_handle *xgt)
         return 0;
 
     rc = osdep_gnttab_close(xgt);
+    xentoolcore__deregister_active_handle(&xgt->tc_ah);
     xtl_logger_destroy(xgt->logger_tofree);
     free(xgt);
     return rc;
diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h
index 3ce4205..ed8df40 100644
--- a/tools/libs/gnttab/private.h
+++ b/tools/libs/gnttab/private.h
@@ -2,6 +2,7 @@
 #define XENGNTTAB_PRIVATE_H
 
 #include <xentoollog.h>
+#include <xentoolcore_internal.h>
 #include <xengnttab.h>
 
 /* Set of macros/defines used by both Linux and FreeBSD */
@@ -13,6 +14,7 @@
 struct xengntdev_handle {
     xentoollog_logger *logger, *logger_tofree;
     int fd;
+    Xentoolcore__Active_Handle tc_ah;
 };
 
 int osdep_gnttab_open(xengnttab_handle *xgt);
diff --git a/tools/libs/gnttab/xengnttab.pc.in b/tools/libs/gnttab/xengnttab.pc.in
index 51aad22..4c3beed 100644
--- a/tools/libs/gnttab/xengnttab.pc.in
+++ b/tools/libs/gnttab/xengnttab.pc.in
@@ -7,4 +7,4 @@ Description: The Xengnttab library for Xen hypervisor
 Version: @@version@@
 Cflags: -I${includedir} @@cflagslocal@@
 Libs: @@libsflag@@${libdir} -lxengnttab
-Requires.private: xentoollog
+Requires.private: xentoollog,xentoolcore
-- 
2.1.4


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

[PATCH 13/22] tools/xenstore: get_handle: use "goto err" error handling style

[Ian Jackson]

Replace the ad-hoc exit clauses with the error handling style where
  - local variables contain either things to be freed, or sentinels
  - all error exits go via an "err" label which frees everything

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/xenstore/xs.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index 56caac7..65cba86 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -226,7 +226,7 @@ static struct xs_handle *get_handle(const char *connect_to)
 	int fd = -1, saved_errno;
 
 	if (stat(connect_to, &buf) != 0)
-		return NULL;
+		goto err;
 
 	if (S_ISSOCK(buf.st_mode))
 		fd = get_socket(connect_to);
@@ -234,15 +234,11 @@ static struct xs_handle *get_handle(const char *connect_to)
 		fd = get_dev(connect_to);
 
 	if (fd == -1)
-		return NULL;
+		goto err;
 
 	h = malloc(sizeof(*h));
-	if (h == NULL) {
-		saved_errno = errno;
-		close(fd);
-		errno = saved_errno;
-		return NULL;
-	}
+	if (h == NULL)
+		goto err;
 
 	memset(h, 0, sizeof(*h));
 
@@ -267,6 +263,15 @@ static struct xs_handle *get_handle(const char *connect_to)
 #endif
 
 	return h;
+
+err:
+	saved_errno = errno;
+
+	if (fd >= 0) close(fd);
+	free(h);
+
+	errno = saved_errno;
+	return NULL;
 }
 
 struct xs_handle *xs_daemon_open(void)
-- 
2.1.4


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

[PATCH 14/22] tools/xenstore: get_handle: Allocate struct before opening fd

[Ian Jackson]

Now we can also abolish the temporary local variable "fd" and simply
use h->fd.

This ordering is necessary to be able to call
xentoolcore__register_active_handle sensibly.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/xenstore/xs.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index 65cba86..7f85bb2 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -223,27 +223,26 @@ static struct xs_handle *get_handle(const char *connect_to)
 {
 	struct stat buf;
 	struct xs_handle *h = NULL;
-	int fd = -1, saved_errno;
+	int saved_errno;
+
+	h = malloc(sizeof(*h));
+	if (h == NULL)
+		goto err;
+
+	memset(h, 0, sizeof(*h));
+	h->fd = -1;
 
 	if (stat(connect_to, &buf) != 0)
 		goto err;
 
 	if (S_ISSOCK(buf.st_mode))
-		fd = get_socket(connect_to);
+		h->fd = get_socket(connect_to);
 	else
-		fd = get_dev(connect_to);
+		h->fd = get_dev(connect_to);
 
-	if (fd == -1)
+	if (h->fd == -1)
 		goto err;
 
-	h = malloc(sizeof(*h));
-	if (h == NULL)
-		goto err;
-
-	memset(h, 0, sizeof(*h));
-
-	h->fd = fd;
-
 	INIT_LIST_HEAD(&h->reply_list);
 	INIT_LIST_HEAD(&h->watch_list);
 
@@ -267,7 +266,10 @@ static struct xs_handle *get_handle(const char *connect_to)
 err:
 	saved_errno = errno;
 
-	if (fd >= 0) close(fd);
+	if (h) {
+		if (h->fd >= 0)
+			close(h->fd);
+	}
 	free(h);
 
 	errno = saved_errno;
-- 
2.1.4


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

[PATCH 15/22] xentoolcore_restrict_all: "Implement" for xenstore

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/Rules.mk                |  2 +-
 tools/xenstore/Makefile       |  7 ++++---
 tools/xenstore/xenstore.pc.in |  2 +-
 tools/xenstore/xs.c           | 14 ++++++++++++++
 4 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/tools/Rules.mk b/tools/Rules.mk
index 3239e76..be92f0a 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -146,7 +146,7 @@ LDLIBS_libxenguest = $(SHDEPS_libxenguest) $(XEN_LIBXC)/libxenguest$(libextensio
 SHLIB_libxenguest  = $(SHDEPS_libxenguest) -Wl,-rpath-link=$(XEN_LIBXC)
 
 CFLAGS_libxenstore = -I$(XEN_XENSTORE)/include $(CFLAGS_xeninclude)
-SHDEPS_libxenstore =
+SHDEPS_libxenstore = $(SHLIB_libxentoolcore)
 LDLIBS_libxenstore = $(SHDEPS_libxenstore) $(XEN_XENSTORE)/libxenstore$(libextension)
 SHLIB_libxenstore  = $(SHDEPS_libxenstore) -Wl,-rpath-link=$(XEN_XENSTORE)
 
diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile
index ff428e2..2b99d2b 100644
--- a/tools/xenstore/Makefile
+++ b/tools/xenstore/Makefile
@@ -11,6 +11,7 @@ CFLAGS += -include $(XEN_ROOT)/tools/config.h
 CFLAGS += -I./include
 CFLAGS += $(CFLAGS_libxenevtchn)
 CFLAGS += $(CFLAGS_libxenctrl)
+CFLAGS += $(CFLAGS_libxentoolcore)
 CFLAGS += -DXEN_LIB_STORED="\"$(XEN_LIB_STORED)\""
 CFLAGS += -DXEN_RUN_STORED="\"$(XEN_RUN_STORED)\""
 
@@ -85,10 +86,10 @@ $(CLIENTS): xenstore
 	ln -f xenstore $@
 
 xenstore: xenstore_client.o $(LIBXENSTORE)
-	$(CC) $< $(LDFLAGS) $(LDLIBS_libxenstore) $(SOCKET_LIBS) -o $@ $(APPEND_LDFLAGS)
+	$(CC) $< $(LDFLAGS) $(LDLIBS_libxenstore) $(LDLIBS_libxentoolcore) $(SOCKET_LIBS) -o $@ $(APPEND_LDFLAGS)
 
 xenstore-control: xenstore_control.o $(LIBXENSTORE)
-	$(CC) $< $(LDFLAGS) $(LDLIBS_libxenstore) $(SOCKET_LIBS) -o $@ $(APPEND_LDFLAGS)
+	$(CC) $< $(LDFLAGS) $(LDLIBS_libxenstore) $(LDLIBS_libxentoolcore) $(SOCKET_LIBS) -o $@ $(APPEND_LDFLAGS)
 
 xs_tdb_dump: xs_tdb_dump.o utils.o tdb.o talloc.o
 	$(CC) $^ $(LDFLAGS) -o $@ $(APPEND_LDFLAGS)
@@ -101,7 +102,7 @@ libxenstore.so.$(MAJOR): libxenstore.so.$(MAJOR).$(MINOR)
 xs.opic: CFLAGS += -DUSE_PTHREAD
 
 libxenstore.so.$(MAJOR).$(MINOR): xs.opic xs_lib.opic
-	$(CC) $(LDFLAGS) $(PTHREAD_LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxenstore.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $^ $(SOCKET_LIBS) $(PTHREAD_LIBS) $(APPEND_LDFLAGS)
+	$(CC) $(LDFLAGS) $(PTHREAD_LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxenstore.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $^ $(LDLIBS_libxentoolcore) $(SOCKET_LIBS) $(PTHREAD_LIBS) $(APPEND_LDFLAGS)
 
 libxenstore.a: xs.o xs_lib.o
 	$(AR) rcs $@ $^
diff --git a/tools/xenstore/xenstore.pc.in b/tools/xenstore/xenstore.pc.in
index 45dc6b0..6fd72a1 100644
--- a/tools/xenstore/xenstore.pc.in
+++ b/tools/xenstore/xenstore.pc.in
@@ -7,4 +7,4 @@ Description: The Xenstore library for Xen hypervisor
 Version: @@version@@
 Cflags: -I${includedir} @@cflagslocal@@
 Libs: @@libsflag@@${libdir} -lxenstore
-Requires.private: xenevtchn,xencontrol,xengnttab
+Requires.private: xenevtchn,xencontrol,xengnttab,xentoolcore
diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index 7f85bb2..ae4b878 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -35,6 +35,8 @@
 #include "list.h"
 #include "utils.h"
 
+#include <xentoolcore_internal.h>
+
 struct xs_stored_msg {
 	struct list_head list;
 	struct xsd_sockmsg hdr;
@@ -48,6 +50,7 @@ struct xs_stored_msg {
 struct xs_handle {
 	/* Communications channel to xenstore daemon. */
 	int fd;
+	Xentoolcore__Active_Handle tc_ah; /* for restrict */
 
 	/*
          * A read thread which pulls messages off the comms channel and
@@ -122,6 +125,7 @@ static void *read_thread(void *arg);
 
 struct xs_handle {
 	int fd;
+	Xentoolcore__Active_Handle tc_ah; /* for restrict */
 	struct list_head reply_list;
 	struct list_head watch_list;
 	/* Clients can select() on this pipe to wait for a watch to fire. */
@@ -219,6 +223,11 @@ static int get_dev(const char *connect_to)
 	return open(connect_to, O_RDWR);
 }
 
+static int all_restrict_cb(Xentoolcore__Active_Handle *ah, uint32_t domid) {
+    struct xs_handle *h = CONTAINER_OF(ah, *h, tc_ah);
+    return xentoolcore__restrict_by_dup2_null(h->fd);
+}
+
 static struct xs_handle *get_handle(const char *connect_to)
 {
 	struct stat buf;
@@ -232,6 +241,9 @@ static struct xs_handle *get_handle(const char *connect_to)
 	memset(h, 0, sizeof(*h));
 	h->fd = -1;
 
+	h->tc_ah.restrict_callback = all_restrict_cb;
+	xentoolcore__register_active_handle(&h->tc_ah);
+
 	if (stat(connect_to, &buf) != 0)
 		goto err;
 
@@ -269,6 +281,7 @@ err:
 	if (h) {
 		if (h->fd >= 0)
 			close(h->fd);
+		xentoolcore__deregister_active_handle(&h->tc_ah);
 	}
 	free(h);
 
@@ -330,6 +343,7 @@ static void close_fds_free(struct xs_handle *h) {
 	}
 
         close(h->fd);
+	xentoolcore__deregister_active_handle(&h->tc_ah);
         
 	free(h);
 }
-- 
2.1.4


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

[PATCH 16/22] xentoolcore, _restrict_all: Document implementation "complete"

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libs/toolcore/include/xentoolcore.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/libs/toolcore/include/xentoolcore.h b/tools/libs/toolcore/include/xentoolcore.h
index 1210d7d..52f7aa3 100644
--- a/tools/libs/toolcore/include/xentoolcore.h
+++ b/tools/libs/toolcore/include/xentoolcore.h
@@ -41,8 +41,10 @@
  *  ====================================================================
  *  IMPORTANT - IMPLEMENTATION STATUS
  *
- *  This function will be implemented insofar as it appears necessary
- *  for the purposes of running a deprivileged qemu.
+ *  This function has been implemented insofar as it appears necessary
+ *  for the purposes of running a deprivileged qemu, and is believed to
+ *  be sufficient (subject to the caveats discussed in the appropriate
+ *  libxl documatation for this feature).
  *
  *  However, this function is NOT implemented for all Xen libraries.
  *  For each use case of this function, the designer must evaluate and
-- 
2.1.4


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

[PATCH 17/22] xl, libxl: Provide dm_restrict

[Ian Jackson]

This functionality is still quite imperfect, but it will be useful in
certain restricted use cases.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 docs/man/xl.cfg.pod.5.in    | 86 +++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_create.c  |  1 +
 tools/libxl/libxl_dm.c      |  9 +++++
 tools/libxl/libxl_types.idl |  1 +
 tools/xl/xl_parse.c         |  3 ++
 5 files changed, 100 insertions(+)

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index 79cb2ea..e3a73bc 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -2045,6 +2045,92 @@ specified, enabling the use of XenServer PV drivers in the guest.
 This parameter only takes effect when device_model_version=qemu-xen.
 See B<xen-pci-device-reservations(7)> for more information.
 
+=item B<dm_restrict=BOOLEAN>
+
+Restrict the HVM device model after startup,
+to limit the consequencese of security vulnerabilities in qemu.
+
+With this feature enabled,
+a compromise of the device model,
+via such a vulnerability,
+will not provide a privilege escalation attack on the whole system.
+
+This feature is a B<technology preview>.
+There are some significant limitations:
+
+=over 4
+
+=item
+
+You must have a new enough qemu.
+In particular,
+if your qemu does not have the commit
+B<xen: restrict: use xentoolcore_restrict_all>
+the restriction request will be silently ineffective!
+
+=item
+
+The mechanisms used are not effective against
+denial of service problems.
+A compromised qemu can probably still impair
+or perhaps even prevent
+the proper functioning of the whole system,
+(at the very least, but not limited to,
+through resource exhaustion).
+
+=item
+
+It is not known whether the protection is
+effective when a domain is migrated.
+
+=item
+
+Some domain management functions do not work.
+For example, cdrom insert will fail.
+
+=item
+
+You must create user(s) for qemu to run as.
+Currently, you should either create
+B<xen-qemuuser-domid$domid>
+for every $domid from 1 to 32751 inclusive,
+or
+B<xen-qemuuser-shared>
+(in which case different guests will not
+be protected against each other).
+And if you do not create the user,
+the restriction request will be silently ineffective!
+
+=item
+
+There are no countermeasures taken against reuse
+of the same unix user (uid)
+for subsequent domains,
+even if the B<xen-qemuuser-domid$domid> users are created.
+So a past domain with the same domid may be able to
+interferer with future domains.
+Possibly, even after a reboot.
+
+=item
+
+A compromised qemu will be able to read world-readable
+files in the dom0 operating system.
+
+=item
+
+Because of these limitations, this functionality,
+while it may enhance your security,
+should not be relied on.
+Any further limitations discovered in the current version
+will B<not> be handled via the Xen Project Security Process.
+
+=item
+
+In the future as we enhance this feature to improve the security,
+we may break backward compatibility.
+
+=back
+
 =back
 
 =head2 Device-Model Options
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 9123585..c757651 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -322,6 +322,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         libxl_defbool_setdefault(&b_info->u.hvm.altp2m,             false);
         libxl_defbool_setdefault(&b_info->u.hvm.usb,                false);
         libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,   true);
+        libxl_defbool_setdefault(&b_info->u.hvm.dm_restrict,        false);
 
         libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);
         if (!libxl_defbool_val(b_info->u.hvm.spice.enable) &&
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index e0e6a99..472a42b 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -641,6 +641,12 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
             flexarray_append(dm_args, "-nographic");
     }
 
+    if (libxl_defbool_val(b_info->u.hvm.dm_restrict)) {
+        LOGD(ERROR, domid,
+             "dm_restrict not supported by qemu-xen-traditional");
+        return ERROR_INVAL;
+    }
+
     if (state->saved_state) {
         flexarray_vappend(dm_args, "-loadvm", state->saved_state, NULL);
     }
@@ -1396,6 +1402,9 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
         }
     }
 
+    if (libxl_defbool_val(b_info->u.hvm.dm_restrict))
+        flexarray_append(dm_args, "-xen-domid-restrict");
+
     if (state->saved_state) {
         /* This file descriptor is meant to be used by QEMU */
         *dm_state_fd = open(state->saved_state, O_RDONLY);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 173d70a..fc39b32 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -565,6 +565,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                        ("rdm", libxl_rdm_reserve),
                                        ("rdm_mem_boundary_memkb", MemKB),
                                        ("mca_caps",         uint64),
+                                       ("dm_restrict",      libxl_defbool),
                                        ])),
                  ("pv", Struct(None, [("kernel", string),
                                       ("slack_memkb", MemKB),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 02ddd2e..2c86527 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2085,6 +2085,9 @@ skip_usbdev:
 
             b_info->u.hvm.vendor_device = d;
         }
+
+        xlu_cfg_get_defbool(config, "dm_restrict",
+                            &b_info->u.hvm.dm_restrict, 0);
     }
 
     if (!xlu_cfg_get_string (config, "gic_version", &buf, 1)) {
-- 
2.1.4


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

[PATCH 18/22] libxl: Rationalise calculation of user to run qemu as

[Ian Jackson]

If the config specifies a user we use that.  Otherwise:

When we are not restricting qemu, there is very little point running
it as a different user than root.  Indeed, previously, creating the
"magic" users would cause qemu to become slightly dysfunctional (for
example, you can't insert a cd that the qemu user can't read).
So, in that case, default to running it as root.

Conversely, if restriction is requested, we must insist on running
qemu as a non-root user.

Sadly the admin is still required to create 2^16-epsilon users!

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 docs/man/xl.cfg.pod.5.in |  2 --
 tools/libxl/libxl_dm.c   | 13 ++++++++++---
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index e3a73bc..166ad4e 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -2098,8 +2098,6 @@ or
 B<xen-qemuuser-shared>
 (in which case different guests will not
 be protected against each other).
-And if you do not create the user,
-the restriction request will be silently ineffective!
 
 =item
 
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 472a42b..831c397 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1633,6 +1633,12 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
             goto end_search;
         }
 
+        if (!libxl_defbool_val(b_info->u.hvm.dm_restrict)) {
+            LOGD(DEBUG, guest_domid,
+                 "dm_restrict disabled, starting QEMU as root");
+            goto end_search;
+        }
+
         user = GCSPRINTF("%s%d", LIBXL_QEMU_USER_BASE, guest_domid);
         ret = libxl__dm_runas_helper(gc, user);
         if (ret < 0)
@@ -1650,9 +1656,10 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
             goto end_search;
         }
 
-        user = NULL;
-        LOGD(DEBUG, guest_domid, "Could not find user %s, starting QEMU as root",
-             LIBXL_QEMU_USER_SHARED);
+        LOGD(ERROR, guest_domid,
+             "Could not find user %s%d or %s, cannot restrict",
+             LIBXL_QEMU_USER_BASE, guest_domid, LIBXL_QEMU_USER_SHARED);
+        return ERROR_INVAL;
 
 end_search:
         if (user != NULL && strcmp(user, "root")) {
-- 
2.1.4


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

[PATCH 19/22] libxl: libxl__dm_runas_helper: return pwd

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl_dm.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 831c397..3c4ef2b 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -749,7 +749,8 @@ libxl__detect_gfx_passthru_kind(libxl__gc *gc,
 }
 
 /* return 1 if the user was found, 0 if it was not, -1 on error */
-static int libxl__dm_runas_helper(libxl__gc *gc, const char *username)
+static int libxl__dm_runas_helper(libxl__gc *gc, const char *username,
+                                       struct passwd **pwd_r)
 {
     struct passwd pwd, *user = NULL;
     char *buf = NULL;
@@ -773,8 +774,10 @@ static int libxl__dm_runas_helper(libxl__gc *gc, const char *username)
         }
         if (ret != 0)
             return ERROR_FAIL;
-        if (user != NULL)
+        if (user != NULL) {
+            if (pwd_r) *pwd_r = pwd;
             return 1;
+        }
         return 0;
     }
 }
@@ -1640,14 +1643,14 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
         }
 
         user = GCSPRINTF("%s%d", LIBXL_QEMU_USER_BASE, guest_domid);
-        ret = libxl__dm_runas_helper(gc, user);
+        ret = libxl__dm_runas_helper(gc, user, 0);
         if (ret < 0)
             return ret;
         if (ret > 0)
             goto end_search;
 
         user = LIBXL_QEMU_USER_SHARED;
-        ret = libxl__dm_runas_helper(gc, user);
+        ret = libxl__dm_runas_helper(gc, user, 0);
         if (ret < 0)
             return ret;
         if (ret > 0) {
-- 
2.1.4


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

[PATCH 20/22] libxl: userlookup_helper_getpwnam rename and turn into a macro

[Ian Jackson]

We are going to want versions of getpwuid, too.  And maybe in the
future getgr*.

This is most sanely achieved with a macro, as otherwise the types are
a mess.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl_dm.c | 79 ++++++++++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 35 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3c4ef2b..10da40c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -748,39 +748,48 @@ libxl__detect_gfx_passthru_kind(libxl__gc *gc,
     return LIBXL_GFX_PASSTHRU_KIND_DEFAULT;
 }
 
-/* return 1 if the user was found, 0 if it was not, -1 on error */
-static int libxl__dm_runas_helper(libxl__gc *gc, const char *username,
-                                       struct passwd **pwd_r)
-{
-    struct passwd pwd, *user = NULL;
-    char *buf = NULL;
-    long buf_size;
-    int ret;
-
-    buf_size = sysconf(_SC_GETPW_R_SIZE_MAX);
-    if (buf_size < 0) {
-        buf_size = 2048;
-        LOG(DEBUG,
-"sysconf(_SC_GETPW_R_SIZE_MAX) failed, setting the initial buffer size to %ld",
-            buf_size);
-    }
-
-    while (1) {
-        buf = libxl__realloc(gc, buf, buf_size);
-        ret = getpwnam_r(username, &pwd, buf, buf_size, &user);
-        if (ret == ERANGE) {
-            buf_size += 128;
-            continue;
-        }
-        if (ret != 0)
-            return ERROR_FAIL;
-        if (user != NULL) {
-            if (pwd_r) *pwd_r = pwd;
-            return 1;
-        }
-        return 0;
-    }
-}
+/*
+ *  userlookup_helper_getpwnam(libxl__gc*, const char *user,
+ *                             struct passwd **pwd_r);
+ *
+ *  returns 1 if the user was found, 0 if it was not, -1 on error
+ */
+#define DEFINE_USERLOOKUP_HELPER(NAME,SPEC_TYPE,STRUCTNAME,SYSCONF)          \
+    static int userlookup_helper_##NAME(libxl__gc *gc,                  \
+                                        SPEC_TYPE spec,                 \
+                                           struct STRUCTNAME **out)   \
+    {                                                                   \
+        struct STRUCTNAME resultbuf, *resultp = NULL;                   \
+        char *buf = NULL;                                               \
+        long buf_size;                                                  \
+        int ret;                                                        \
+                                                                        \
+        buf_size = sysconf(SYSCONF);                                    \
+        if (buf_size < 0) {                                             \
+            buf_size = 2048;                                            \
+            LOG(DEBUG,                                                  \
+    "sysconf failed, setting the initial buffer size to %ld",           \
+                buf_size);                                              \
+        }                                                               \
+                                                                        \
+        while (1) {                                                     \
+            buf = libxl__realloc(gc, buf, buf_size);                    \
+            ret = NAME##_r(spec, &resultbuf, buf, buf_size, &resultp); \
+            if (ret == ERANGE) {                                        \
+                buf_size += 128;                                        \
+                continue;                                               \
+            }                                                           \
+            if (ret != 0)                                               \
+                return ERROR_FAIL;                                      \
+            if (resultp != NULL) {                                       \
+                if (out) *out = resultp;                                \
+                return 1;                                               \
+            }                                                           \
+            return 0;                                                   \
+        }                                                               \
+    }
+
+DEFINE_USERLOOKUP_HELPER(getpwnam, const char*, passwd, _SC_GETPW_R_SIZE_MAX);
 
 /* colo mode */
 enum {
@@ -1643,14 +1652,14 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
         }
 
         user = GCSPRINTF("%s%d", LIBXL_QEMU_USER_BASE, guest_domid);
-        ret = libxl__dm_runas_helper(gc, user, 0);
+        ret = userlookup_helper_getpwnam(gc, user, 0);
         if (ret < 0)
             return ret;
         if (ret > 0)
             goto end_search;
 
         user = LIBXL_QEMU_USER_SHARED;
-        ret = libxl__dm_runas_helper(gc, user, 0);
+        ret = userlookup_helper_getpwnam(gc, user, 0);
         if (ret < 0)
             return ret;
         if (ret > 0) {
-- 
2.1.4


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

[PATCH 21/22] libxl: dm_restrict: Support uid range user

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 docs/man/xl.cfg.pod.5.in     | 11 ++++++++++-
 tools/libxl/libxl_dm.c       | 32 ++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |  1 +
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index 166ad4e..1c8d52d 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -2091,7 +2091,16 @@ For example, cdrom insert will fail.
 =item
 
 You must create user(s) for qemu to run as.
-Currently, you should either create
+
+Ideally, set aside a range of 32752 uids
+(from N to N+32751)
+and create a user
+whose name is B<xen-qemuuser-range-base>
+and whose uid is N
+and whose gid is a plain unprivileged gid.
+libxl will use one such user for each domid.
+
+Alternatively, either create
 B<xen-qemuuser-domid$domid>
 for every $domid from 1 to 32751 inclusive,
 or
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 10da40c..52feac0 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -23,6 +23,7 @@
 #include <xen/hvm/e820.h>
 #include <sys/types.h>
 #include <pwd.h>
+#include <grp.h>
 
 static const char *libxl_tapif_script(libxl__gc *gc)
 {
@@ -752,6 +753,9 @@ libxl__detect_gfx_passthru_kind(libxl__gc *gc,
  *  userlookup_helper_getpwnam(libxl__gc*, const char *user,
  *                             struct passwd **pwd_r);
  *
+ *  userlookup_helper_getpwuid(libxl__gc*, uid_t uid,
+ *                             struct passwd **pwd_r);
+ *
  *  returns 1 if the user was found, 0 if it was not, -1 on error
  */
 #define DEFINE_USERLOOKUP_HELPER(NAME,SPEC_TYPE,STRUCTNAME,SYSCONF)          \
@@ -790,6 +794,7 @@ libxl__detect_gfx_passthru_kind(libxl__gc *gc,
     }
 
 DEFINE_USERLOOKUP_HELPER(getpwnam, const char*, passwd, _SC_GETPW_R_SIZE_MAX);
+DEFINE_USERLOOKUP_HELPER(getpwuid, uid_t,       passwd, _SC_GETPW_R_SIZE_MAX);
 
 /* colo mode */
 enum {
@@ -950,6 +955,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
     uint64_t ram_size;
     const char *path, *chardev;
     char *user = NULL;
+    struct passwd *user_base;
 
     dm_args = flexarray_make(gc, 16, 1);
     dm_envs = flexarray_make(gc, 16, 1);
@@ -1658,6 +1664,32 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
         if (ret > 0)
             goto end_search;
 
+        ret = userlookup_helper_getpwnam(gc, LIBXL_QEMU_USER_RANGE_BASE,
+                                         &user_base);
+        if (ret < 0)
+            return ret;
+        if (ret > 0) {
+            struct passwd *user_clash;
+            uid_t intended_uid = user_base->pw_uid + guest_domid;
+            ret = userlookup_helper_getpwuid(gc, intended_uid, &user_clash);
+            if (ret < 0)
+                return ret;
+            if (ret > 0) {
+                LOGD(ERROR, guest_domid,
+                     "wanted to use uid %ld (%s + %d) but that is user %s !",
+                     (long)intended_uid, LIBXL_QEMU_USER_RANGE_BASE,
+                     guest_domid, user_clash->pw_name);
+                return ERROR_FAIL;
+            }
+            LOGD(DEBUG, guest_domid, "using uid %ld", (long)intended_uid);
+            flexarray_append(dm_args, "-runasid");
+            flexarray_append(dm_args,
+                             GCSPRINTF("%ld.%ld", (long)intended_uid,
+                                       (long)user_base->pw_gid));
+            user = NULL; /* we have taken care of it */
+            goto end_search;
+        }
+
         user = LIBXL_QEMU_USER_SHARED;
         ret = userlookup_helper_getpwnam(gc, user, 0);
         if (ret < 0)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 4f2f433..761501e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4316,6 +4316,7 @@ _hidden int libxl__read_sysfs_file_contents(libxl__gc *gc,
 #define LIBXL_QEMU_USER_PREFIX "xen-qemuuser"
 #define LIBXL_QEMU_USER_BASE   LIBXL_QEMU_USER_PREFIX"-domid"
 #define LIBXL_QEMU_USER_SHARED LIBXL_QEMU_USER_PREFIX"-shared"
+#define LIBXL_QEMU_USER_RANGE_BASE LIBXL_QEMU_USER_PREFIX"-range-base"
 
 static inline bool libxl__acpi_defbool_val(const libxl_domain_build_info *b_info)
 {
-- 
2.1.4


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

[PATCH 22/22] RFC: tools: xentoolcore_restrict_all: use domid_t

[Ian Jackson]

This is an RFC because it does not currently compile, because not all
the places that use xentoolcore have a definition of domid in scope!

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libs/toolcore/handlereg.c                    | 2 +-
 tools/libs/toolcore/include/xentoolcore.h          | 4 ++--
 tools/libs/toolcore/include/xentoolcore_internal.h | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/libs/toolcore/handlereg.c b/tools/libs/toolcore/handlereg.c
index 56d8b2d..ca88c9e 100644
--- a/tools/libs/toolcore/handlereg.c
+++ b/tools/libs/toolcore/handlereg.c
@@ -55,7 +55,7 @@ void xentoolcore__deregister_active_handle(Xentoolcore__Active_Handle *ah) {
     unlock();
 }
 
-int xentoolcore_restrict_all(uint32_t domid) {
+int xentoolcore_restrict_all(domid_t domid) {
 /* xxx lock */
     int r;
     Xentoolcore__Active_Handle *ah;
diff --git a/tools/libs/toolcore/include/xentoolcore.h b/tools/libs/toolcore/include/xentoolcore.h
index 52f7aa3..6d9cada 100644
--- a/tools/libs/toolcore/include/xentoolcore.h
+++ b/tools/libs/toolcore/include/xentoolcore.h
@@ -25,7 +25,7 @@
 #include <stdint.h>
 
 /*
- * int xentoolcore_restrict_all(uint32_t domid);
+ * int xentoolcore_restrict_all(domid_t domid);
  *
  * Arranges that Xen library handles (fds etc.) which are currently held
  * by Xen libraries, can no longer be used other than to affect domid.
@@ -63,7 +63,7 @@
  *     xen_some[other]library_open|close or xentoolcore_restrict_all
  *
  */
-int xentoolcore_restrict_all(uint32_t domid);
+int xentoolcore_restrict_all(domid_t domid);
 
 #endif /* XENTOOLCORE_H */
 
diff --git a/tools/libs/toolcore/include/xentoolcore_internal.h b/tools/libs/toolcore/include/xentoolcore_internal.h
index 7e96a48..dbdb1dd 100644
--- a/tools/libs/toolcore/include/xentoolcore_internal.h
+++ b/tools/libs/toolcore/include/xentoolcore_internal.h
@@ -81,7 +81,7 @@
 typedef struct Xentoolcore__Active_Handle Xentoolcore__Active_Handle;
 
 typedef int Xentoolcore__Restrict_Callback(Xentoolcore__Active_Handle*,
-                                           uint32_t domid);
+                                           domid_t domid);
 
 struct Xentoolcore__Active_Handle {
     Xentoolcore__Restrict_Callback *restrict_callback;
-- 
2.1.4


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

Re: [PATCH 01/22] xen: Provide XEN_DMOP_remote_shutdown

[Jan Beulich]

>>> On 15.09.17 at 20:48, <ian.jackson@eu.citrix.com> wrote:
> SCHEDOP_remote_shutdown should be a DMOP so that a deprivileged qemu
> can do the propery tidying up.
> 
> We should remove SCHEDOP_remote_shutdown at some point.

Except we can't for ABI stability reasons, plus how would you
remote-shutdown a PV guest then?

> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -630,6 +630,14 @@ static int dm_op(const struct dmop_args *op_args)
>          rc = hvm_inject_msi(d, data->addr, data->data);
>          break;
>      }
> +    case XEN_DMOP_remote_shutdown:

With a blank line added above here, and preferably that second
sentence of the commit message removed
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH 01/22] xen: Provide XEN_DMOP_remote_shutdown

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 01/22] xen: Provide XEN_DMOP_remote_shutdown"):
> >>> On 15.09.17 at 20:48, <ian.jackson@eu.citrix.com> wrote:
> > SCHEDOP_remote_shutdown should be a DMOP so that a deprivileged qemu
> > can do the propery tidying up.
> > 
> > We should remove SCHEDOP_remote_shutdown at some point.
> 
> Except we can't for ABI stability reasons, plus how would you
> remote-shutdown a PV guest then?

Thanks for the review.  I have replaced that sentence in the commit
message with this:

 We need to keep SCHEDOP_remote_shutdown for ABI stability reasons and
 because it is needed for PV guests.

> > --- a/xen/arch/x86/hvm/dm.c
> > +++ b/xen/arch/x86/hvm/dm.c
> > @@ -630,6 +630,14 @@ static int dm_op(const struct dmop_args *op_args)
> >          rc = hvm_inject_msi(d, data->addr, data->data);
> >          break;
> >      }
> > +    case XEN_DMOP_remote_shutdown:
> 
> With a blank line added above here,

Thanks.  I copied the lack of newline from between
XEN_DMOP_inject_event and XEN_DMOP_inject_msi.

I will add a trivial extra patch to add that missing newline (unless
you object).

> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.  In the expectation that what I say above in the commit
message meets with your approval, I will include that R-B in my next
posting of the series.

Ian.

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

Re: [PATCH 01/22] xen: Provide XEN_DMOP_remote_shutdown

[Jan Beulich]

>>> On 18.09.17 at 15:57, <ian.jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH 01/22] xen: Provide 
> XEN_DMOP_remote_shutdown"):
>> >>> On 15.09.17 at 20:48, <ian.jackson@eu.citrix.com> wrote:
>> > SCHEDOP_remote_shutdown should be a DMOP so that a deprivileged qemu
>> > can do the propery tidying up.
>> > 
>> > We should remove SCHEDOP_remote_shutdown at some point.
>> 
>> Except we can't for ABI stability reasons, plus how would you
>> remote-shutdown a PV guest then?
> 
> Thanks for the review.  I have replaced that sentence in the commit
> message with this:
> 
>  We need to keep SCHEDOP_remote_shutdown for ABI stability reasons and
>  because it is needed for PV guests.

Sounds good.

>> > --- a/xen/arch/x86/hvm/dm.c
>> > +++ b/xen/arch/x86/hvm/dm.c
>> > @@ -630,6 +630,14 @@ static int dm_op(const struct dmop_args *op_args)
>> >          rc = hvm_inject_msi(d, data->addr, data->data);
>> >          break;
>> >      }
>> > +    case XEN_DMOP_remote_shutdown:
>> 
>> With a blank line added above here,
> 
> Thanks.  I copied the lack of newline from between
> XEN_DMOP_inject_event and XEN_DMOP_inject_msi.

Oh, I see - the only bad example in this function.

> I will add a trivial extra patch to add that missing newline (unless
> you object).

Feel free to put my ack on it, or even merge it into the patch here.

>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Thanks.  In the expectation that what I say above in the commit
> message meets with your approval, I will include that R-B in my next
> posting of the series.

Yes please.

Jan


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

Re: [PATCH 01/22] xen: Provide XEN_DMOP_remote_shutdown

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:38PM +0100, Ian Jackson wrote:
> diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
> index 0f17000..20055b4 100644
> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -70,6 +70,7 @@
>  ?	dm_op_set_pci_intx_level	hvm/dm_op.h
>  ?	dm_op_set_pci_link_route	hvm/dm_op.h
>  ?	dm_op_track_dirty_vram		hvm/dm_op.h
> +?	dm_op_remote_shutdown		hvm/dm_op.h

Can you please move this a few lines above to make the dm_op section
sorted alphabetically?

With that and Jan's comments addressed:

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 02/22] tools: libxendevicemodel: Provide xendevicemodel_shutdown

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:39PM +0100, Ian Jackson wrote:
> diff --git a/tools/libs/devicemodel/libxendevicemodel.map b/tools/libs/devicemodel/libxendevicemodel.map
> index 130222c..fd57e79 100644
> --- a/tools/libs/devicemodel/libxendevicemodel.map
> +++ b/tools/libs/devicemodel/libxendevicemodel.map
> @@ -18,6 +18,7 @@ VERS_1.0 {
>  		xendevicemodel_modified_memory;
>  		xendevicemodel_set_mem_type;
>  		xendevicemodel_inject_event;
> +		xendevicemodel_shutdown;
>  		xendevicemodel_restrict;
>  		xendevicemodel_close;
>  	local: *; /* Do not expose anything by default */

We need to have:

VERS_1.1 {
   global:
	xendevicemodel_shutdown;
} VERS_1.0;

And also bump the minor number in Makefile.

The code looks fine to me.

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

Re: [PATCH 16/22] xentoolcore, _restrict_all: Document implementation "complete"

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:53PM +0100, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  tools/libs/toolcore/include/xentoolcore.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libs/toolcore/include/xentoolcore.h b/tools/libs/toolcore/include/xentoolcore.h
> index 1210d7d..52f7aa3 100644
> --- a/tools/libs/toolcore/include/xentoolcore.h
> +++ b/tools/libs/toolcore/include/xentoolcore.h
> @@ -41,8 +41,10 @@
>   *  ====================================================================
>   *  IMPORTANT - IMPLEMENTATION STATUS
>   *
> - *  This function will be implemented insofar as it appears necessary
> - *  for the purposes of running a deprivileged qemu.
> + *  This function has been implemented insofar as it appears necessary
> + *  for the purposes of running a deprivileged qemu, and is believed to
> + *  be sufficient (subject to the caveats discussed in the appropriate
> + *  libxl documatation for this feature).

documentation


>   *
>   *  However, this function is NOT implemented for all Xen libraries.
>   *  For each use case of this function, the designer must evaluate and
> -- 
> 2.1.4
> 

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

Re: [PATCH 18/22] libxl: Rationalise calculation of user to run qemu as

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:55PM +0100, Ian Jackson wrote:
> If the config specifies a user we use that.  Otherwise:
> 
> When we are not restricting qemu, there is very little point running
> it as a different user than root.  Indeed, previously, creating the
> "magic" users would cause qemu to become slightly dysfunctional (for
> example, you can't insert a cd that the qemu user can't read).
> So, in that case, default to running it as root.
> 
> Conversely, if restriction is requested, we must insist on running
> qemu as a non-root user.
> 
> Sadly the admin is still required to create 2^16-epsilon users!
> 

I will let Stefano comment on this.

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

Re: [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:40PM +0100, Ian Jackson wrote:
> diff --git a/tools/libs/toolcore/include/xentoolcore.h b/tools/libs/toolcore/include/xentoolcore.h
> new file mode 100644
> index 0000000..1ab646e
> --- /dev/null
> +++ b/tools/libs/toolcore/include/xentoolcore.h
> @@ -0,0 +1,71 @@
> +/*
> + * xentoolcore.h
> + *
> + * Copyright (c) 2017 Citrix
> + * 
> + * Common features used/provided by all Xen tools libraries
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation;
> + * version 2.1 of the License.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef XENTOOLCORE_H
> +#define XENTOOLCORE_H
> +
> +#include <stdint.h>
> +
> +/*
> + * int xentoolcore_restrict_all(uint32_t domid);
> + *
> + * Arranges that Xen library handles (fds etc.) which are currently held
> + * by Xen libraries, can no longer be used other than to affect domid.
> + *
> + * If this cannot be achieved, returns -1 and sets errno.
> + * Idempotent if domid is always the same.
> + *
> + *  ====================================================================
> + *  IMPORTANT - IMPLEMENTATION STATUS
> + *
> + *  This function will be implemented insofar as it appears necessary
> + *  for the purposes of running a deprivileged qemu.
> + *
> + *  However, this function is NOT implemented for all Xen libraries.
> + *  For each use case of this function, the designer must evaluate and
> + *  audit whether the implementation is sufficient in their specific
> + *  context.
> + *
> + *  Of course, patches to extend the implementation are very welcome.
> + *  ====================================================================
> + *
> + * Thread safe.
> + *
> + * We expect that no callers do the following:
> + *   - in one thread call xen_somelibrary_open|close
> + *   - in another thread call fork
> + *   - in the child of the fork, before exec, call
> + *     xen_some[other]library_open|close or xentoolcore_restrict_all
> + *
> + */
> +int xentoolcore_restrict_all(uint32_t domid);
> +
> +#endif /* XENTOOLCORE_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/tools/libs/toolcore/include/xentoolcore_internal.h b/tools/libs/toolcore/include/xentoolcore_internal.h
> new file mode 100644
> index 0000000..670e29d
> --- /dev/null
> +++ b/tools/libs/toolcore/include/xentoolcore_internal.h
> @@ -0,0 +1,102 @@
> +/*
> + * xentoolcore_internal.h
> + *
> + * Interfaces of xentoolcore directed internally at other Xen libraries
> + *
> + * Copyright (c) 2017 Citrix
> + * 
> + * Common code used by all Xen tools libraries
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation;
> + * version 2.1 of the License.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef XENTOOLCORE_INTERNAL_H
> +#define XENTOOLCORE_INTERNAL_H
> +
> +#include "xentoolcore.h"
> +#include "_xentoolcore_list.h"
> +
> +/*---------- active handle registration ----------*/
> +
> +/*
> + * This is all to support xentoolcore_restrict_all
> + *
> + * Any libxl library that opens a Xen control handle of any kind which
> + * might allow manipulation of dom0, of other domains, or of the whole
> + * machine, must:
> + *   I. arrange that their own datastructure contains a
> + *          Xentoolcore__Active_Handle
> + * 
> + *   II. during the "open handle" function
> + *     1. allocate the memory for the own datastructure and initialise it
> + *     2. set Xentoolcore__Active_Handle.restrict_callback
> + *     3. call xentoolcore__register_active_handle
> + *       3a. if the open fails, call xentoolcore__deregister_active_handle
> + *     4. ONLY THEN actually open the relevant fd or whatever
> + *
> + *   III. during the "close handle" function
> + *     1. FIRST close the relevant fd or whatever
> + *     2. call xentoolcore__deregister_active_handle
> + *
> + *   IV. in the restrict_callback function
> + *     * Arrange that the fd (or other handle) can no longer by used
> + *       other than with respect to domain domid.
> + *     * Future attempts to manipulate other domains (or the whole
> + *       host) via this handle must cause an error return (and
> + *       perhaps a log message), not a crash
> + *     * If selective restriction is not possible, the handle must
> + *       be completely invalidated so that it is not useable;
> + *       subsequent manipulations may not crash
> + *     * The restrict_callback function should not normally fail
> + *       if this can be easily avoided - it is better to make the
> + *       handle nonfunction instead.
> + *     * NB that restrict_callback might be called again.  That must
> + *       work properly: if the domid is the same, it is idempotent.
> + *       If the domid is different. then either the handle must be
> + *       completely invalidated, or restrict_callback must fail.)
> + *
> + * Thread safety:
> + *    xentoolcore__[de]register_active_handle are threadsafe
> + *      but MUST NOT be called within restrict_callback
> + *
> + * Fork safety:
> + *    Libraries which use these functions do not on that account
> + *    need to take any special care over forks occurring in
> + *    other threads, provided that they obey the rules above.
> + */
> +
> +typedef struct Xentoolcore__Active_Handle Xentoolcore__Active_Handle;
> +
> +typedef int Xentoolcore__Restrict_Callback(Xentoolcore__Active_Handle*,
> +                                           uint32_t domid);
> +
> +struct Xentoolcore__Active_Handle {
> +    Xentoolcore__Restrict_Callback *restrict_callback;
> +    XENTOOLCORE_LIST_ENTRY(Xentoolcore__Active_Handle) entry;
> +};
> +
> +void xentoolcore__register_active_handle(Xentoolcore__Active_Handle*);
> +void xentoolcore__deregister_active_handle(Xentoolcore__Active_Handle*);

Why use two underscores in those function names?

Is this library supposed to be stable? We only expect this to be tech
review, right?  I think it is worth explicitly stating that if that's
the case.

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

Re: [PATCH 16/22] xentoolcore, _restrict_all: Document implementation "complete"

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 16/22] xentoolcore, _restrict_all: Document implementation "complete""):
> On Fri, Sep 15, 2017 at 07:48:53PM +0100, Ian Jackson wrote:
> > - *  This function will be implemented insofar as it appears necessary
> > - *  for the purposes of running a deprivileged qemu.
> > + *  This function has been implemented insofar as it appears necessary
> > + *  for the purposes of running a deprivileged qemu, and is believed to
> > + *  be sufficient (subject to the caveats discussed in the appropriate
> > + *  libxl documatation for this feature).
> 
> documentation

Fixed, thanks.

Ian.

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

Re: [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation"):
> On Fri, Sep 15, 2017 at 07:48:40PM +0100, Ian Jackson wrote:
...
> > +void xentoolcore__register_active_handle(Xentoolcore__Active_Handle*);
> > +void xentoolcore__deregister_active_handle(Xentoolcore__Active_Handle*);
> 
> Why use two underscores in those function names?

These functions are for use by other Xen libraries only, not by
external callers.

> Is this library supposed to be stable? We only expect this to be tech
> review, right?  I think it is worth explicitly stating that if that's
> the case.

The API/ABI to xentoolcore_restrict_all is intended to be stable.
The internal API may vary.

The head comment of xentoolcore_internal.h says
   * Interfaces of xentoolcore directed internally at other Xen libraries
but I can make this more explicit if you want.

Ian.

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

Re: [PATCH 02/22] tools: libxendevicemodel: Provide xendevicemodel_shutdown

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 02/22] tools: libxendevicemodel: Provide xendevicemodel_shutdown"):
> We need to have:
> 
> VERS_1.1 {
...
> And also bump the minor number in Makefile.

Done, thanks.

Ian.

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

Re: [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation

[Wei Liu]

On Mon, Sep 18, 2017 at 05:08:18PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation"):
> > On Fri, Sep 15, 2017 at 07:48:40PM +0100, Ian Jackson wrote:
> ...
> > > +void xentoolcore__register_active_handle(Xentoolcore__Active_Handle*);
> > > +void xentoolcore__deregister_active_handle(Xentoolcore__Active_Handle*);
> > 
> > Why use two underscores in those function names?
> 
> These functions are for use by other Xen libraries only, not by
> external callers.
> 
> > Is this library supposed to be stable? We only expect this to be tech
> > review, right?  I think it is worth explicitly stating that if that's
> > the case.
> 
> The API/ABI to xentoolcore_restrict_all is intended to be stable.
> The internal API may vary.
> 
> The head comment of xentoolcore_internal.h says
>    * Interfaces of xentoolcore directed internally at other Xen libraries
> but I can make this more explicit if you want.

There is no need for that.

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

Re: [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:40PM +0100, Ian Jackson wrote:
> +int xentoolcore_restrict_all(uint32_t domid) {
> +/* xxx lock */

This should be removed.

With this fixed:

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 04/22] tools: qemu-xen build: prepare to link against xentoolcore

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:41PM +0100, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 05/22] libxl: #include "xentoolcore_internal.h"

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:42PM +0100, Ian Jackson wrote:
> We are going to want to move something here.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 06/22] tools: move CONTAINER_OF to xentoolcore_internal.h

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:43PM +0100, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:40PM +0100, Ian Jackson wrote:
> +
> +int xentoolcore_restrict_all(uint32_t domid) {
> +/* xxx lock */
> +    int r;
> +    Xentoolcore__Active_Handle *ah;
> +
> +    lock();
> +    XENTOOLCORE_LIST_FOREACH(ah, &handles, entry) {
> +        r = ah->restrict_callback(ah, domid);

Looking at the "Implement" patches for some libraries, I think we need
to stash domid in ah and filter base on that. If not, at least in the
case of duping /dev/null, we risk closing the handles we don't wish to
close.

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

Re: [PATCH 07/22] xentoolcore_restrict_all: Implement for libxendevicemodel

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:44PM +0100, Ian Jackson wrote:
> +static int all_restrict_cb(Xentoolcore__Active_Handle *ah, uint32_t domid) {
> +    xendevicemodel_handle *dmod = CONTAINER_OF(ah, *dmod, tc_ah);
> +
> +    if (dmod->fd < 0)
> +        /* just in case */
> +        return 0;
> +
> +    return xendevicemodel_restrict(dmod, domid);

I'm not sure if this would meet the idempotent rule we lay out in the
header. Calling this function for a second time may produce a different
return code.

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

Re: [PATCH 08/22] xentoolcore_restrict_all: "Implement" for libxencall

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:45PM +0100, Ian Jackson wrote:
> +static int all_restrict_cb(Xentoolcore__Active_Handle *ah, uint32_t domid) {
> +    xencall_handle *xcall = CONTAINER_OF(ah, *xcall, tc_ah);
> +    int nullfd = -1, r;
> +
> +    if (xcall->fd < 0)
> +        /* just in case */
> +        return 0;
> +
> +    /*
> +     * We don't implement a restrict function.  We neuter the fd by
> +     * dup'ing /dev/null onto it.  This is better than closing it,
> +     * because it does not involve locking against concurrent uses
> +     * of xencall in other threads.
> +     */
> +    nullfd = open("/dev/null",O_RDONLY);

Space after "," please.

With that fixed:

Acked-by: Wei Liu <wei.liu2@citrix.com>


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

Re: [PATCH 09/22] xentoolcore_restrict: Break out xentoolcore__restrict_by_dup2_null

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:46PM +0100, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 10/22] xentoolcore_restrict_all: Implement for libxenforeignmemory

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:47PM +0100, Ian Jackson wrote:
> +static int all_restrict_cb(Xentoolcore__Active_Handle *ah, uint32_t domid) {
> +    xenforeignmemory_handle *fmem = CONTAINER_OF(ah, *fmem, tc_ah);
> +
> +    if (fmem->fd < 0)
> +        /* just in case */
> +        return 0;
> +
> +    return xenforeignmemory_restrict(fmem, domid);

Same comment for xendevicemodel_restrict on idempotent applies here.

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

Re: [PATCH 11/22] xentoolcore_restrict_all: Declare problems due to no evtchn support

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:48PM +0100, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 12/22] xentoolcore_restrict_all: "Implement" for xengnttab

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:49PM +0100, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 13/22] tools/xenstore: get_handle: use "goto err" error handling style

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:50PM +0100, Ian Jackson wrote:
> Replace the ad-hoc exit clauses with the error handling style where
>   - local variables contain either things to be freed, or sentinels
>   - all error exits go via an "err" label which frees everything
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 14/22] tools/xenstore: get_handle: Allocate struct before opening fd

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:51PM +0100, Ian Jackson wrote:
> Now we can also abolish the temporary local variable "fd" and simply
> use h->fd.
> 
> This ordering is necessary to be able to call
> xentoolcore__register_active_handle sensibly.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 15/22] xentoolcore_restrict_all: "Implement" for xenstore

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:52PM +0100, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 17/22] xl, libxl: Provide dm_restrict

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:54PM +0100, Ian Jackson wrote:
> This functionality is still quite imperfect, but it will be useful in
> certain restricted use cases.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Seeing this is mostly plumbing for QEMU and a technology preview
feature:

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 19/22] libxl: libxl__dm_runas_helper: return pwd

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:56PM +0100, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 20/22] libxl: userlookup_helper_getpwnam rename and turn into a macro

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:57PM +0100, Ian Jackson wrote:
> +/*
> + *  userlookup_helper_getpwnam(libxl__gc*, const char *user,
> + *                             struct passwd **pwd_r);
> + *
> + *  returns 1 if the user was found, 0 if it was not, -1 on error
> + */
> +#define DEFINE_USERLOOKUP_HELPER(NAME,SPEC_TYPE,STRUCTNAME,SYSCONF)          \
> +    static int userlookup_helper_##NAME(libxl__gc *gc,                  \
> +                                        SPEC_TYPE spec,                 \
> +                                           struct STRUCTNAME **out)   \
> +    {                                                                   \
> +        struct STRUCTNAME resultbuf, *resultp = NULL;                   \
> +        char *buf = NULL;                                               \
> +        long buf_size;                                                  \
> +        int ret;                                                        \
> +                                                                        \
> +        buf_size = sysconf(SYSCONF);                                    \
> +        if (buf_size < 0) {                                             \
> +            buf_size = 2048;                                            \
> +            LOG(DEBUG,                                                  \
> +    "sysconf failed, setting the initial buffer size to %ld",           \
> +                buf_size);                                              \
> +        }                                                               \
> +                                                                        \
> +        while (1) {                                                     \
> +            buf = libxl__realloc(gc, buf, buf_size);                    \
> +            ret = NAME##_r(spec, &resultbuf, buf, buf_size, &resultp); \
> +            if (ret == ERANGE) {                                        \
> +                buf_size += 128;                                        \
> +                continue;                                               \
> +            }                                                           \
> +            if (ret != 0)                                               \
> +                return ERROR_FAIL;                                      \
> +            if (resultp != NULL) {                                       \
> +                if (out) *out = resultp;                                \
> +                return 1;                                               \
> +            }                                                           \
> +            return 0;                                                   \
> +        }                                                               \
> +    }
> +

Some of the "\"'s are misaligned.

With or without aligning them:

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 22/22] RFC: tools: xentoolcore_restrict_all: use domid_t

[Wei Liu]

On Fri, Sep 15, 2017 at 07:48:59PM +0100, Ian Jackson wrote:
> This is an RFC because it does not currently compile, because not all
> the places that use xentoolcore have a definition of domid in scope!
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

If the places you mentioned are in xen.git we should probably fix them.
If they are external users like QEMU then we probably can't do much.

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

Re: [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation"):
> On Fri, Sep 15, 2017 at 07:48:40PM +0100, Ian Jackson wrote:
> > +int xentoolcore_restrict_all(uint32_t domid) {
> > +/* xxx lock */
> 
> This should be removed.

Oops, missed one indeed.

> With this fixed:
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Thanks,
Ian.

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

Re: [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation"):
> On Fri, Sep 15, 2017 at 07:48:40PM +0100, Ian Jackson wrote:
> > +int xentoolcore_restrict_all(uint32_t domid) {
> > +    int r;
> > +    Xentoolcore__Active_Handle *ah;
> > +
> > +    lock();
> > +    XENTOOLCORE_LIST_FOREACH(ah, &handles, entry) {
> > +        r = ah->restrict_callback(ah, domid);
> 
> Looking at the "Implement" patches for some libraries, I think we need
> to stash domid in ah and filter base on that. If not, at least in the
> case of duping /dev/null, we risk closing the handles we don't wish to
> close.

I don't follow.

The libraries where we dup /dev/null do not support restriction and
therefore the domid is irrelevant for them.

For the libraries where we call an actual restriction ioctl, the domid
is recorded in the kernel.  The worst case is a bug where the restrict
ioctl cannot be called more than once.  TBH if that is the case then
we can just change the docs for xentoolcore_restrict_all to say that
if you call it a 2nd time if may fail, even if given the same domid.

Ian.

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

Re: [PATCH 08/22] xentoolcore_restrict_all: "Implement" for libxencall

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 08/22] xentoolcore_restrict_all: "Implement" for libxencall"):
> On Fri, Sep 15, 2017 at 07:48:45PM +0100, Ian Jackson wrote:
> > +    nullfd = open("/dev/null",O_RDONLY);
> 
> Space after "," please.

OK.  I will keep that change when the code moves, in a moment.

> With that fixed:
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Thanks,
Ian.

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

Re: [PATCH 10/22] xentoolcore_restrict_all: Implement for libxenforeignmemory

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 10/22] xentoolcore_restrict_all: Implement for libxenforeignmemory"):
> Same comment for xendevicemodel_restrict on idempotent applies here.

Are you happy with me fixing this by changing the docs rather than the
code ?

Ian.

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

Re: [PATCH 17/22] xl, libxl: Provide dm_restrict

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 17/22] xl, libxl: Provide dm_restrict"):
> On Fri, Sep 15, 2017 at 07:48:54PM +0100, Ian Jackson wrote:
> > This functionality is still quite imperfect, but it will be useful in
> > certain restricted use cases.
...
> Seeing this is mostly plumbing for QEMU and a technology preview
> feature:

Doing a more complete job will involve more significant work which is
probably not (or at least, much of which is not) going to be ready for
4.10.

I may update things to make some additional restriction calls in qemu
but the big one is uid reuse.  I think fixing the uid reuse problem
involves adding a new fork to the domain creation and domain teardown,
since I'm not aware of a way to kill all processes with a particular
uid other than by running a process with that uid.

Ian.

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

Re: [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation

[Wei Liu]

On Tue, Sep 19, 2017 at 11:47:12AM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation"):
> > On Fri, Sep 15, 2017 at 07:48:40PM +0100, Ian Jackson wrote:
> > > +int xentoolcore_restrict_all(uint32_t domid) {
> > > +    int r;
> > > +    Xentoolcore__Active_Handle *ah;
> > > +
> > > +    lock();
> > > +    XENTOOLCORE_LIST_FOREACH(ah, &handles, entry) {
> > > +        r = ah->restrict_callback(ah, domid);
> > 
> > Looking at the "Implement" patches for some libraries, I think we need
> > to stash domid in ah and filter base on that. If not, at least in the
> > case of duping /dev/null, we risk closing the handles we don't wish to
> > close.
> 
> I don't follow.
> 
> The libraries where we dup /dev/null do not support restriction and
> therefore the domid is irrelevant for them.
> 
> For the libraries where we call an actual restriction ioctl, the domid
> is recorded in the kernel.  The worst case is a bug where the restrict
> ioctl cannot be called more than once.  TBH if that is the case then
> we can just change the docs for xentoolcore_restrict_all to say that
> if you call it a 2nd time if may fail, even if given the same domid.

The impression I get from the name and parameter from this API is that
the following use case is allowed: a device model serving multiple
domains. The device model will open two sets of handlers of various
libraries. The device model will call restrict_all(domid) to restrict
its own privileges on certain domid when it sees fit.

Without filtering, the callbacks are called for all the domains at the
same time. The code as-is, when resctrict_all(dom1) is called, makes
privileges on dom2 are also dropped sometimes -- imagine a xenstore
callback registered for dom2 is called, which makes the connection
unusable for dom2.

If the aforementioned use case is not anticipated, I think we shouldn't
accept domid parameter for the resctrict_all function.

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

Re: [PATCH 20/22] libxl: userlookup_helper_getpwnam rename and turn into a macro

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 20/22] libxl: userlookup_helper_getpwnam rename and turn into a macro"):
> On Fri, Sep 15, 2017 at 07:48:57PM +0100, Ian Jackson wrote:
> > +#define DEFINE_USERLOOKUP_HELPER(NAME,SPEC_TYPE,STRUCTNAME,SYSCONF)          \
> 
> Some of the "\"'s are misaligned.

So they are.  Fixed.

> Acked-by: Wei Liu <wei.liu2@citrix.com>

Thanks,
Ian.

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

Re: [PATCH 10/22] xentoolcore_restrict_all: Implement for libxenforeignmemory

[Wei Liu]

On Tue, Sep 19, 2017 at 11:51:06AM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH 10/22] xentoolcore_restrict_all: Implement for libxenforeignmemory"):
> > Same comment for xendevicemodel_restrict on idempotent applies here.
> 
> Are you happy with me fixing this by changing the docs rather than the
> code ?

Sure that's fine.

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

Re: [PATCH 22/22] RFC: tools: xentoolcore_restrict_all: use domid_t

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 22/22] RFC: tools: xentoolcore_restrict_all: use domid_t"):
> On Fri, Sep 15, 2017 at 07:48:59PM +0100, Ian Jackson wrote:
> > This is an RFC because it does not currently compile, because not all
> > the places that use xentoolcore have a definition of domid in scope!
...
> If the places you mentioned are in xen.git we should probably fix them.
> If they are external users like QEMU then we probably can't do much.

They are in xen.git.  The difficulty is that they don't currently
include the right headers.  Also, in general, much of the code in
xen.git doesn't always use domid_t.

(Partly because domid_t is a weird short type.)

Ian.

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

Re: [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation"):
> The impression I get from the name and parameter from this API is that
> the following use case is allowed: a device model serving multiple
> domains.

Such a device model is obviously, by necessity, unrestricted.

> The device model will open two sets of handlers of various
> libraries. The device model will call restrict_all(domid) to restrict
> its own privileges on certain domid when it sees fit.

After it has restricted its privileges to domid A, it is no longer
permitted to do things to domid B.  Attempting to call restrict_all(B)
will fail.

> Without filtering, the callbacks are called for all the domains at the
> same time. The code as-is, when resctrict_all(dom1) is called, makes
> privileges on dom2 are also dropped sometimes -- imagine a xenstore
> callback registered for dom2 is called, which makes the connection
> unusable for dom2.
> 
> If the aforementioned use case is not anticipated, I think we shouldn't
> accept domid parameter for the resctrict_all function.

But the domid is precisely the scope of the intended restriction.
After making the call, the malign influence of the calling process is
limited to the specified domid (at least, insofar as the malign
influence is exercised via already-open Xen library handles).

Ian.

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

Re: [PATCH 10/22] xentoolcore_restrict_all: Implement for libxenforeignmemory

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 10/22] xentoolcore_restrict_all: Implement for libxenforeignmemory"):
> Sure that's fine.

I have this now:

 * If called again with the same domid, it may succeed, or it may
 * fail (even though such a call is potentially meaningful).
 * (If called again with a different domid, it will necessariloy
 * fail.)
   
Ian.

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

Re: [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation

[Wei Liu]

On Tue, Sep 19, 2017 at 12:04:42PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation"):
> > The impression I get from the name and parameter from this API is that
> > the following use case is allowed: a device model serving multiple
> > domains.
> 
> Such a device model is obviously, by necessity, unrestricted.
> 
> > The device model will open two sets of handlers of various
> > libraries. The device model will call restrict_all(domid) to restrict
> > its own privileges on certain domid when it sees fit.
> 
> After it has restricted its privileges to domid A, it is no longer
> permitted to do things to domid B.  Attempting to call restrict_all(B)
> will fail.
> 
> > Without filtering, the callbacks are called for all the domains at the
> > same time. The code as-is, when resctrict_all(dom1) is called, makes
> > privileges on dom2 are also dropped sometimes -- imagine a xenstore
> > callback registered for dom2 is called, which makes the connection
> > unusable for dom2.
> > 
> > If the aforementioned use case is not anticipated, I think we shouldn't
> > accept domid parameter for the resctrict_all function.
> 
> But the domid is precisely the scope of the intended restriction.
> After making the call, the malign influence of the calling process is
> limited to the specified domid (at least, insofar as the malign
> influence is exercised via already-open Xen library handles).
> 

Ah, I know where I got myself confused.

You can have my Ack on this patch.

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

Re: [PATCH 10/22] xentoolcore_restrict_all: Implement for libxenforeignmemory

[Wei Liu]

On Tue, Sep 19, 2017 at 12:08:27PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH 10/22] xentoolcore_restrict_all: Implement for libxenforeignmemory"):
> > Sure that's fine.
> 
> I have this now:
> 
>  * If called again with the same domid, it may succeed, or it may
>  * fail (even though such a call is potentially meaningful).
>  * (If called again with a different domid, it will necessariloy
>  * fail.)

Typo "necessariloy".

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

Re: [PATCH 22/22] RFC: tools: xentoolcore_restrict_all: use domid_t

[Wei Liu]

On Tue, Sep 19, 2017 at 12:01:15PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH 22/22] RFC: tools: xentoolcore_restrict_all: use domid_t"):
> > On Fri, Sep 15, 2017 at 07:48:59PM +0100, Ian Jackson wrote:
> > > This is an RFC because it does not currently compile, because not all
> > > the places that use xentoolcore have a definition of domid in scope!
> ...
> > If the places you mentioned are in xen.git we should probably fix them.
> > If they are external users like QEMU then we probably can't do much.
> 
> They are in xen.git.  The difficulty is that they don't currently
> include the right headers.  Also, in general, much of the code in
> xen.git doesn't always use domid_t.
> 
> (Partly because domid_t is a weird short type.)

I would very much like to have domid_t in the toolcore API, just to be
consistent with xendevicemodel APIs. I think we should fix xen.git --
including the right header in the places which use toolcore APIs doesn't
sound too hard for me.

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

Re: [PATCH 10/22] xentoolcore_restrict_all: Implement for libxenforeignmemory

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 10/22] xentoolcore_restrict_all: Implement for libxenforeignmemory"):
> On Tue, Sep 19, 2017 at 12:08:27PM +0100, Ian Jackson wrote:
> > Wei Liu writes ("Re: [PATCH 10/22] xentoolcore_restrict_all: Implement for libxenforeignmemory"):
> > > Sure that's fine.
> > 
> > I have this now:
> > 
> >  * If called again with the same domid, it may succeed, or it may
> >  * fail (even though such a call is potentially meaningful).
> >  * (If called again with a different domid, it will necessariloy
> >  * fail.)
> 
> Typo "necessariloy".

Fixed, thanks.  NB that this text is in
   xentoolcore, _restrict_all: Introduce new library and implementation
not the libxenforeignmemory patch, of course.

Ian.

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