I have been working on trying to get qemu, when running as a Xen
device model, to _actually_ not have power equivalent to root.
I think I have achieved this, with some limitations (which are
discussed in my series against xen.git.
However, there are changes to qemu needed. In particular
* The -xen-domid-restrict option does not work properly right now.
It only restricts a small subset of the descriptors qemu has open.
I am introducing a new library call in the Xen libraries for this,
xentoolcore_restrict_all.
* We need to call a different function on domain shutdown.
* The restriction operation needs to be done at a slightly different
time, necessitating a new hook.
* Additionally, we want to be able to set aside a uid range for these
qemus to run in, and that involves being able to tell qemu to drop
privilege by numeric uid and gid.
Thanks to Anthony Perard, Ross Lagerwall, Peter Maydell, Markus
Armbruster and Daniel P. Berrange for assistance, review and testing.
m 1/8 xen: link against xentoolcore
r 2/8 xen: restrict: use xentoolcore_restrict_all
rm 3/8 xen: defer call to xen_restrict until just before
r 4/8 xen: destroy_hvm_domain: Move reason into a variable
a 5/8 xen: move xc_interface compatibility fallback further up
r 6/8 xen: destroy_hvm_domain: Try xendevicemodel_shutdown
* 7/8 os-posix: Provide new -runas <uid>.<gid> facility
8/8 configure: do_compiler: Dump some extra info under bash
m = commit message (only) changed in v5 of the series
* = patch changed in v5 of the series
r = reviewed
a = acked
Thanks,
Ian.
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 1508431916-9412-1-git-send-email-ian.jackson@eu.citrix.com
Subject: [Qemu-devel] [PATCH v5 0/8] xen: xen-domid-restrict improvements
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
b9f9672c4d configure: do_compiler: Dump some extra info under bash
74f2dc4a81 os-posix: Provide new -runas <uid>.<gid> facility
34bf5bf2fa xen: destroy_hvm_domain: Try xendevicemodel_shutdown
baa9207819 xen: move xc_interface compatibility fallback further up the file
f3cb5e6890 xen: destroy_hvm_domain: Move reason into a variable
4d0d91ed91 xen: defer call to xen_restrict until just before os_setup_post
4deada5196 xen: restrict: use xentoolcore_restrict_all
13836ce7d9 xen: link against xentoolcore
=== OUTPUT BEGIN ===
Checking PATCH 1/8: xen: link against xentoolcore...
Checking PATCH 2/8: xen: restrict: use xentoolcore_restrict_all...
Checking PATCH 3/8: xen: defer call to xen_restrict until just before os_setup_post...
Checking PATCH 4/8: xen: destroy_hvm_domain: Move reason into a variable...
Checking PATCH 5/8: xen: move xc_interface compatibility fallback further up the file...
Checking PATCH 6/8: xen: destroy_hvm_domain: Try xendevicemodel_shutdown...
Checking PATCH 7/8: os-posix: Provide new -runas <uid>.<gid> facility...
ERROR: consider using qemu_strtoul in preference to strtoul
#45: FILE: os-posix.c:142:
+ lv = strtoul(optarg, &ep, 0); /* can't qemu_strtoul, want *ep=='.' */
total: 1 errors, 0 warnings, 100 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 8/8: configure: do_compiler: Dump some extra info under bash...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
no-reply@patchew.org writes ("Re: [Qemu-devel] [PATCH v5 0/8] xen: xen-domid-restrict improvements"):
> This series seems to have some coding style problems. See output below for
> more information:
No, it doesn't have coding style problems. At least, this mail
contains only one complaint which is this:
> Checking PATCH 7/8: os-posix: Provide new -runas <uid>.<gid> facility...
> ERROR: consider using qemu_strtoul in preference to strtoul
> #45: FILE: os-posix.c:142:
> + lv = strtoul(optarg, &ep, 0); /* can't qemu_strtoul, want *ep=='.' */
This is a false positive as you can see from the comment.
Ian.
Hi Ian, The patches in this v5 appear to be the same the one from the patch series v4. -- Anthony PERARD
Anthony PERARD writes ("Re: [PATCH v5 0/8] xen: xen-domid-restrict improvements"):
> The patches in this v5 appear to be the same the one from the patch
> series v4.
Erk, so they are.
I'll post a v5.1 in reply to this email.
Ian.
On 10/20/2017 02:37 PM, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [PATCH v5 0/8] xen: xen-domid-restrict improvements"):
>> The patches in this v5 appear to be the same the one from the patch
>> series v4.
>
> Erk, so they are.
>
> I'll post a v5.1 in reply to this email.
>
What's the status of this patch series? There don't seem to be many
outstanding complaints but they haven't been pushed into master. At
least the Xen changes have all been reviewed by Anthony (except for
configure changes) so they could probably go in.
Thanks,
--
Ross Lagerwall
Ross Lagerwall writes ("Re: [PATCH v5 0/8] xen: xen-domid-restrict improvements"):
> What's the status of this patch series? There don't seem to be many
> outstanding complaints but they haven't been pushed into master. At
> least the Xen changes have all been reviewed by Anthony (except for
> configure changes) so they could probably go in.
The short answer is I don't really know; my brain was eaten by
meltdown/spectre. I'll hope to get back to this RSN.
Thanks,
Ian.
From: Anthony PERARD <anthony.perard@citrix.com>
Xen libraries in 4.10 include a new xentoolcore library. This
contains the xentoolcore_restrict_all function which we are about to
want to use.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v5: More truthful commit message.
---
configure | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/configure b/configure
index fd7e3a5..6f691df 100755
--- a/configure
+++ b/configure
@@ -2072,7 +2072,7 @@ if test "$xen" != "no" ; then
$($pkg_config --modversion xencontrol | sed 's/\./ /g') )"
xen=yes
xen_pc="xencontrol xenstore xenguest xenforeignmemory xengnttab"
- xen_pc="$xen_pc xenevtchn xendevicemodel"
+ xen_pc="$xen_pc xenevtchn xendevicemodel xentoolcore"
QEMU_CFLAGS="$QEMU_CFLAGS $($pkg_config --cflags $xen_pc)"
libs_softmmu="$($pkg_config --libs $xen_pc) $libs_softmmu"
LDFLAGS="$($pkg_config --libs $xen_pc) $LDFLAGS"
@@ -2104,18 +2104,20 @@ EOF
cat > $TMPC <<EOF &&
#undef XC_WANT_COMPAT_MAP_FOREIGN_API
#include <xenforeignmemory.h>
+#include <xentoolcore.h>
int main(void) {
xenforeignmemory_handle *xfmem;
xfmem = xenforeignmemory_open(0, 0);
xenforeignmemory_map2(xfmem, 0, 0, 0, 0, 0, 0, 0);
+ xentoolcore_restrict_all(0);
return 0;
}
EOF
- compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs"
+ compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs -lxentoolcore"
then
- xen_stable_libs="-lxendevicemodel $xen_stable_libs"
+ xen_stable_libs="-lxendevicemodel $xen_stable_libs -lxentoolcore"
xen_ctrl_version=41000
xen=yes
elif
--
2.1.4
On Fri, 20 Oct 2017, Ian Jackson wrote:
> From: Anthony PERARD <anthony.perard@citrix.com>
>
> Xen libraries in 4.10 include a new xentoolcore library. This
> contains the xentoolcore_restrict_all function which we are about to
> want to use.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
> v5: More truthful commit message.
> ---
> configure | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/configure b/configure
> index fd7e3a5..6f691df 100755
> --- a/configure
> +++ b/configure
> @@ -2072,7 +2072,7 @@ if test "$xen" != "no" ; then
> $($pkg_config --modversion xencontrol | sed 's/\./ /g') )"
> xen=yes
> xen_pc="xencontrol xenstore xenguest xenforeignmemory xengnttab"
> - xen_pc="$xen_pc xenevtchn xendevicemodel"
> + xen_pc="$xen_pc xenevtchn xendevicemodel xentoolcore"
> QEMU_CFLAGS="$QEMU_CFLAGS $($pkg_config --cflags $xen_pc)"
> libs_softmmu="$($pkg_config --libs $xen_pc) $libs_softmmu"
> LDFLAGS="$($pkg_config --libs $xen_pc) $LDFLAGS"
> @@ -2104,18 +2104,20 @@ EOF
> cat > $TMPC <<EOF &&
> #undef XC_WANT_COMPAT_MAP_FOREIGN_API
> #include <xenforeignmemory.h>
> +#include <xentoolcore.h>
> int main(void) {
> xenforeignmemory_handle *xfmem;
>
> xfmem = xenforeignmemory_open(0, 0);
> xenforeignmemory_map2(xfmem, 0, 0, 0, 0, 0, 0, 0);
> + xentoolcore_restrict_all(0);
>
> return 0;
> }
> EOF
> - compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs"
> + compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs -lxentoolcore"
> then
> - xen_stable_libs="-lxendevicemodel $xen_stable_libs"
> + xen_stable_libs="-lxendevicemodel $xen_stable_libs -lxentoolcore"
Is it on purpose that -lxentoolcore is at the end of this string rather
than before $xen_stable_libs?
In any case
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> xen_ctrl_version=41000
> xen=yes
> elif
> --
> 2.1.4
>
Stefano Stabellini writes ("Re: [PATCH v5.1 1/8] xen: link against xentoolcore"):
> On Fri, 20 Oct 2017, Ian Jackson wrote:
> > then
> > - xen_stable_libs="-lxendevicemodel $xen_stable_libs"
> > + xen_stable_libs="-lxendevicemodel $xen_stable_libs -lxentoolcore"
>
> Is it on purpose that -lxentoolcore is at the end of this string rather
> than before $xen_stable_libs?
Yes. xentoolcore is required by the other libraries, and this is
therefore the correct ordering for situations where the link order
matters.
> In any case
>
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Thanks,
Ian.
And insist that it works.
Drop individual use of xendevicemodel_restrict and
xenforeignmemory_restrict. These are not actually effective in this
version of qemu, because qemu has a large number of fds open onto
various Xen control devices.
The restriction arrangements are still not right, because the
restriction needs to be done very late - after qemu has opened all of
its control fds.
xentoolcore_restrict_all and xentoolcore.h are available in Xen 4.10
and later, only. Provide a compatibility stub. And drop the
compatibility stubs for the old functions.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
v2: Modify the compatibility code, too.
Bump this patch ahead of "defer call to xen_restrict until running"
Retain call to xentoolcore_restrict_all
---
include/hw/xen/xen_common.h | 46 +++++++++++----------------------------------
1 file changed, 11 insertions(+), 35 deletions(-)
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 86c7f26..3f44a63 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -91,6 +91,16 @@ static inline void *xenforeignmemory_map2(xenforeignmemory_handle *h,
return xenforeignmemory_map(h, dom, prot, pages, arr, err);
}
+static inline int xentoolcore_restrict_all(domid_t domid)
+{
+ errno = ENOTTY;
+ return -1;
+}
+
+#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 41000 */
+
+#include <xentoolcore.h>
+
#endif
#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
@@ -218,20 +228,6 @@ static inline int xendevicemodel_set_mem_type(
return xc_hvm_set_mem_type(dmod, domid, mem_type, first_pfn, nr);
}
-static inline int xendevicemodel_restrict(
- xendevicemodel_handle *dmod, domid_t domid)
-{
- errno = ENOTTY;
- return -1;
-}
-
-static inline int xenforeignmemory_restrict(
- xenforeignmemory_handle *fmem, domid_t domid)
-{
- errno = ENOTTY;
- return -1;
-}
-
#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
#undef XC_WANT_COMPAT_DEVICEMODEL_API
@@ -290,28 +286,8 @@ static inline int xen_modified_memory(domid_t domid, uint64_t first_pfn,
static inline int xen_restrict(domid_t domid)
{
int rc;
-
- /* Attempt to restrict devicemodel operations */
- rc = xendevicemodel_restrict(xen_dmod, domid);
+ rc = xentoolcore_restrict_all(domid);
trace_xen_domid_restrict(rc ? errno : 0);
-
- if (rc < 0) {
- /*
- * If errno is ENOTTY then restriction is not implemented so
- * there's no point in trying to restrict other types of
- * operation, but it should not be treated as a failure.
- */
- if (errno == ENOTTY) {
- return 0;
- }
-
- return rc;
- }
-
- /* Restrict foreignmemory operations */
- rc = xenforeignmemory_restrict(xen_fmem, domid);
- trace_xen_domid_restrict(rc ? errno : 0);
-
return rc;
}
--
2.1.4
On Fri, 20 Oct 2017, Ian Jackson wrote:
> And insist that it works.
>
> Drop individual use of xendevicemodel_restrict and
> xenforeignmemory_restrict. These are not actually effective in this
> version of qemu, because qemu has a large number of fds open onto
> various Xen control devices.
>
> The restriction arrangements are still not right, because the
> restriction needs to be done very late - after qemu has opened all of
> its control fds.
>
> xentoolcore_restrict_all and xentoolcore.h are available in Xen 4.10
> and later, only. Provide a compatibility stub. And drop the
> compatibility stubs for the old functions.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> v2: Modify the compatibility code, too.
> Bump this patch ahead of "defer call to xen_restrict until running"
> Retain call to xentoolcore_restrict_all
> ---
> include/hw/xen/xen_common.h | 46 +++++++++++----------------------------------
> 1 file changed, 11 insertions(+), 35 deletions(-)
>
> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> index 86c7f26..3f44a63 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -91,6 +91,16 @@ static inline void *xenforeignmemory_map2(xenforeignmemory_handle *h,
> return xenforeignmemory_map(h, dom, prot, pages, arr, err);
> }
>
> +static inline int xentoolcore_restrict_all(domid_t domid)
> +{
> + errno = ENOTTY;
> + return -1;
Wait, if the compat stub returns error, and this patch removed the code
to check for ENOTTY, doesn't it prevent any QEMU compiled against older
Xen from working?
Or am I missing something?
> +}
> +
> +#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 41000 */
> +
> +#include <xentoolcore.h>
> +
> #endif
>
> #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
> @@ -218,20 +228,6 @@ static inline int xendevicemodel_set_mem_type(
> return xc_hvm_set_mem_type(dmod, domid, mem_type, first_pfn, nr);
> }
>
> -static inline int xendevicemodel_restrict(
> - xendevicemodel_handle *dmod, domid_t domid)
> -{
> - errno = ENOTTY;
> - return -1;
> -}
> -
> -static inline int xenforeignmemory_restrict(
> - xenforeignmemory_handle *fmem, domid_t domid)
> -{
> - errno = ENOTTY;
> - return -1;
> -}
> -
> #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
>
> #undef XC_WANT_COMPAT_DEVICEMODEL_API
> @@ -290,28 +286,8 @@ static inline int xen_modified_memory(domid_t domid, uint64_t first_pfn,
> static inline int xen_restrict(domid_t domid)
> {
> int rc;
> -
> - /* Attempt to restrict devicemodel operations */
> - rc = xendevicemodel_restrict(xen_dmod, domid);
> + rc = xentoolcore_restrict_all(domid);
> trace_xen_domid_restrict(rc ? errno : 0);
> -
> - if (rc < 0) {
> - /*
> - * If errno is ENOTTY then restriction is not implemented so
> - * there's no point in trying to restrict other types of
> - * operation, but it should not be treated as a failure.
> - */
> - if (errno == ENOTTY) {
> - return 0;
> - }
> -
> - return rc;
> - }
> -
> - /* Restrict foreignmemory operations */
> - rc = xenforeignmemory_restrict(xen_fmem, domid);
> - trace_xen_domid_restrict(rc ? errno : 0);
> -
> return rc;
> }
>
> --
> 2.1.4
>
Stefano Stabellini writes ("Re: [PATCH v5.1 2/8] xen: restrict: use xentoolcore_restrict_all"):
> On Fri, 20 Oct 2017, Ian Jackson wrote:
...
> > Drop individual use of xendevicemodel_restrict and
> > xenforeignmemory_restrict. These are not actually effective in this
> > version of qemu, because qemu has a large number of fds open onto
> > various Xen control devices.
...
> Wait, if the compat stub returns error, and this patch removed the code
> to check for ENOTTY, doesn't it prevent any QEMU compiled against older
> Xen from working?
>
> Or am I missing something?
You are right, but this is intended. The paragraph I quote in the
commit message above is intended to explain.
That is: without xentoolcore_restrict_all, -xen-domid-restrict is a
booby-trap. It does not actually prevent a compromised qemu from
doing anything. So there is no reason to pass it in such a
configuration. If you do pass it it is better for the domain startup
to fail, than for it to carry on without the restriction.
The only reason I am not saying someone should be issuing an advisory
is that this feature was never supported by any of the Xen toolstacks.
Thanks,
Ian.
On Fri, 27 Oct 2017, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [PATCH v5.1 2/8] xen: restrict: use xentoolcore_restrict_all"):
> > On Fri, 20 Oct 2017, Ian Jackson wrote:
> ...
> > > Drop individual use of xendevicemodel_restrict and
> > > xenforeignmemory_restrict. These are not actually effective in this
> > > version of qemu, because qemu has a large number of fds open onto
> > > various Xen control devices.
> ...
> > Wait, if the compat stub returns error, and this patch removed the code
> > to check for ENOTTY, doesn't it prevent any QEMU compiled against older
> > Xen from working?
> >
> > Or am I missing something?
>
> You are right, but this is intended. The paragraph I quote in the
> commit message above is intended to explain.
>
> That is: without xentoolcore_restrict_all, -xen-domid-restrict is a
> booby-trap. It does not actually prevent a compromised qemu from
> doing anything. So there is no reason to pass it in such a
> configuration. If you do pass it it is better for the domain startup
> to fail, than for it to carry on without the restriction.
>
> The only reason I am not saying someone should be issuing an advisory
> is that this feature was never supported by any of the Xen toolstacks.
Ah, right. And libxl has never passed -xen-domid-restrict in previous
releases, so we are OK.
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
We need to restrict *all* the control fds that qemu opens. Looking in
/proc/PID/fd shows there are many; their allocation seems scattered
throughout Xen support code in qemu.
We must postpone the restrict call until roughly the same time as qemu
changes its uid, chroots (if applicable), and so on.
There doesn't seem to be an appropriate hook already. The RunState
change hook fires at different times depending on exactly what mode
qemu is operating in.
And it appears that no-one but the Xen code wants a hook at this phase
of execution. So, introduce a bare call to a new function
xen_setup_post, just before os_setup_post. Also provide the
appropriate stub for when Xen compilation is disabled.
We do the restriction before rather than after os_setup_post, because
xen_restrict may need to open /dev/null, and os_setup_post might have
called chroot.
Currently this does not work with migration, because when running as
the Xen device model qemu needs to signal to the toolstack that it is
ready. It currently does this using xenstore, and for incoming
migration (but not for ordinary startup) that happens after
os_setup_post.
It is correct that this happens late: we want the incoming migration
stream to be processed by a restricted qemu. The fix for this will be
to do the startup notification a different way, without using
xenstore. (QMP is probably a reasonable choice.)
So for now this restriction feature cannot be used in conjunction with
migration. (Note that this is not a regression in this patch, because
previously the -xen-restrict-domid call was, in fact, simply
ineffective!) We will revisit this in the Xen 4.11 release cycle.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
v5: Discuss problems with migration startup notification
in the commit message.
v3: Do xen_setup_post just before, not just after, os_setup_post,
to improve interaction with chroot. Thanks to Ross Lagerwall.
---
hw/i386/xen/xen-hvm.c | 8 --------
hw/xen/xen-common.c | 13 +++++++++++++
include/sysemu/sysemu.h | 2 ++
stubs/xen-hvm.c | 5 +++++
vl.c | 1 +
5 files changed, 21 insertions(+), 8 deletions(-)
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index d9ccd5d..7b60ec6 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1254,14 +1254,6 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
goto err;
}
- if (xen_domid_restrict) {
- rc = xen_restrict(xen_domid);
- if (rc < 0) {
- error_report("failed to restrict: error %d", errno);
- goto err;
- }
- }
-
xen_create_ioreq_server(xen_domid, &state->ioservid);
state->exit.notify = xen_exit_notifier;
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 632a938..4056420 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -117,6 +117,19 @@ static void xen_change_state_handler(void *opaque, int running,
}
}
+void xen_setup_post(void)
+{
+ int rc;
+
+ if (xen_domid_restrict) {
+ rc = xen_restrict(xen_domid);
+ if (rc < 0) {
+ perror("xen: failed to restrict");
+ exit(1);
+ }
+ }
+}
+
static int xen_init(MachineState *ms)
{
xen_xc = xc_interface_open(0, 0, 0);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index b213696..b064a55 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -93,6 +93,8 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify);
void qemu_announce_self(void);
+void xen_setup_post(void);
+
extern int autostart;
typedef enum {
diff --git a/stubs/xen-hvm.c b/stubs/xen-hvm.c
index 3ca6c51..9701feb 100644
--- a/stubs/xen-hvm.c
+++ b/stubs/xen-hvm.c
@@ -13,6 +13,7 @@
#include "hw/xen/xen.h"
#include "exec/memory.h"
#include "qmp-commands.h"
+#include "sysemu/sysemu.h"
int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
{
@@ -61,3 +62,7 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
{
}
+
+void xen_setup_post(void)
+{
+}
diff --git a/vl.c b/vl.c
index fb1f05b..ca06553 100644
--- a/vl.c
+++ b/vl.c
@@ -4792,6 +4792,7 @@ int main(int argc, char **argv, char **envp)
vm_start();
}
+ xen_setup_post();
os_setup_post();
main_loop();
--
2.1.4
This patch affects non-Xen components. CC'ing the relevant maintainers.
On Fri, 20 Oct 2017, Ian Jackson wrote:
> We need to restrict *all* the control fds that qemu opens. Looking in
> /proc/PID/fd shows there are many; their allocation seems scattered
> throughout Xen support code in qemu.
>
> We must postpone the restrict call until roughly the same time as qemu
> changes its uid, chroots (if applicable), and so on.
>
> There doesn't seem to be an appropriate hook already. The RunState
> change hook fires at different times depending on exactly what mode
> qemu is operating in.
>
> And it appears that no-one but the Xen code wants a hook at this phase
> of execution. So, introduce a bare call to a new function
> xen_setup_post, just before os_setup_post. Also provide the
> appropriate stub for when Xen compilation is disabled.
>
> We do the restriction before rather than after os_setup_post, because
> xen_restrict may need to open /dev/null, and os_setup_post might have
> called chroot.
>
> Currently this does not work with migration, because when running as
> the Xen device model qemu needs to signal to the toolstack that it is
> ready. It currently does this using xenstore, and for incoming
> migration (but not for ordinary startup) that happens after
> os_setup_post.
>
> It is correct that this happens late: we want the incoming migration
> stream to be processed by a restricted qemu. The fix for this will be
> to do the startup notification a different way, without using
> xenstore. (QMP is probably a reasonable choice.)
>
> So for now this restriction feature cannot be used in conjunction with
> migration. (Note that this is not a regression in this patch, because
> previously the -xen-restrict-domid call was, in fact, simply
> ineffective!) We will revisit this in the Xen 4.11 release cycle.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> v5: Discuss problems with migration startup notification
> in the commit message.
> v3: Do xen_setup_post just before, not just after, os_setup_post,
> to improve interaction with chroot. Thanks to Ross Lagerwall.
> ---
> hw/i386/xen/xen-hvm.c | 8 --------
> hw/xen/xen-common.c | 13 +++++++++++++
> include/sysemu/sysemu.h | 2 ++
> stubs/xen-hvm.c | 5 +++++
> vl.c | 1 +
> 5 files changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index d9ccd5d..7b60ec6 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -1254,14 +1254,6 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
> goto err;
> }
>
> - if (xen_domid_restrict) {
> - rc = xen_restrict(xen_domid);
> - if (rc < 0) {
> - error_report("failed to restrict: error %d", errno);
> - goto err;
> - }
> - }
> -
> xen_create_ioreq_server(xen_domid, &state->ioservid);
>
> state->exit.notify = xen_exit_notifier;
> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> index 632a938..4056420 100644
> --- a/hw/xen/xen-common.c
> +++ b/hw/xen/xen-common.c
> @@ -117,6 +117,19 @@ static void xen_change_state_handler(void *opaque, int running,
> }
> }
>
> +void xen_setup_post(void)
> +{
> + int rc;
> +
> + if (xen_domid_restrict) {
> + rc = xen_restrict(xen_domid);
> + if (rc < 0) {
> + perror("xen: failed to restrict");
> + exit(1);
> + }
> + }
> +}
> +
> static int xen_init(MachineState *ms)
> {
> xen_xc = xc_interface_open(0, 0, 0);
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index b213696..b064a55 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -93,6 +93,8 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify);
>
> void qemu_announce_self(void);
>
> +void xen_setup_post(void);
> +
> extern int autostart;
>
> typedef enum {
> diff --git a/stubs/xen-hvm.c b/stubs/xen-hvm.c
> index 3ca6c51..9701feb 100644
> --- a/stubs/xen-hvm.c
> +++ b/stubs/xen-hvm.c
> @@ -13,6 +13,7 @@
> #include "hw/xen/xen.h"
> #include "exec/memory.h"
> #include "qmp-commands.h"
> +#include "sysemu/sysemu.h"
>
> int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
> {
> @@ -61,3 +62,7 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
> void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
> {
> }
> +
> +void xen_setup_post(void)
> +{
> +}
> diff --git a/vl.c b/vl.c
> index fb1f05b..ca06553 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4792,6 +4792,7 @@ int main(int argc, char **argv, char **envp)
> vm_start();
> }
>
> + xen_setup_post();
> os_setup_post();
>
> main_loop();
> --
> 2.1.4
>
We are going to want to reuse this.
No functional change.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
hw/i386/xen/xen-hvm.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 7b60ec6..83420cd 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1387,12 +1387,13 @@ void destroy_hvm_domain(bool reboot)
xc_interface *xc_handle;
int sts;
+ unsigned int reason = reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff;
+
xc_handle = xc_interface_open(0, 0, 0);
if (xc_handle == NULL) {
fprintf(stderr, "Cannot acquire xenctrl handle\n");
} else {
- sts = xc_domain_shutdown(xc_handle, xen_domid,
- reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff);
+ sts = xc_domain_shutdown(xc_handle, xen_domid, reason);
if (sts != 0) {
fprintf(stderr, "xc_domain_shutdown failed to issue %s, "
"sts %d, %s\n", reboot ? "reboot" : "poweroff",
--
2.1.4
On Fri, 20 Oct 2017, Ian Jackson wrote:
> We are going to want to reuse this.
>
> No functional change.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> hw/i386/xen/xen-hvm.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index 7b60ec6..83420cd 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -1387,12 +1387,13 @@ void destroy_hvm_domain(bool reboot)
> xc_interface *xc_handle;
> int sts;
>
> + unsigned int reason = reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff;
> +
> xc_handle = xc_interface_open(0, 0, 0);
> if (xc_handle == NULL) {
> fprintf(stderr, "Cannot acquire xenctrl handle\n");
> } else {
> - sts = xc_domain_shutdown(xc_handle, xen_domid,
> - reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff);
> + sts = xc_domain_shutdown(xc_handle, xen_domid, reason);
> if (sts != 0) {
> fprintf(stderr, "xc_domain_shutdown failed to issue %s, "
> "sts %d, %s\n", reboot ? "reboot" : "poweroff",
> --
> 2.1.4
>
We are going to want to use the dummy xendevicemodel_handle type in
new stub functions in the CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
section. So we need to provide that definition, or (as applicable)
include the appropriate header, earlier in the file.
(Ideally the newer compatibility layers would be at the bottom of the
file, so that they can naturally benefit from the compatibility layers
for earlier version. But that's rather too much for this series.)
No functional change.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
---
v2: New patch in v2 of the series
---
include/hw/xen/xen_common.h | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 3f44a63..8efdb8a 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -78,6 +78,17 @@ static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
extern xenforeignmemory_handle *xen_fmem;
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
+
+typedef xc_interface xendevicemodel_handle;
+
+#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
+
+#undef XC_WANT_COMPAT_DEVICEMODEL_API
+#include <xendevicemodel.h>
+
+#endif
+
#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
#define XEN_COMPAT_PHYSMAP
@@ -105,8 +116,6 @@ static inline int xentoolcore_restrict_all(domid_t domid)
#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
-typedef xc_interface xendevicemodel_handle;
-
static inline xendevicemodel_handle *xendevicemodel_open(
struct xentoollog_logger *logger, unsigned int open_flags)
{
@@ -228,11 +237,6 @@ static inline int xendevicemodel_set_mem_type(
return xc_hvm_set_mem_type(dmod, domid, mem_type, first_pfn, nr);
}
-#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
-
-#undef XC_WANT_COMPAT_DEVICEMODEL_API
-#include <xendevicemodel.h>
-
#endif
extern xendevicemodel_handle *xen_dmod;
--
2.1.4
On Fri, 20 Oct 2017, Ian Jackson wrote:
> We are going to want to use the dummy xendevicemodel_handle type in
> new stub functions in the CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
> section. So we need to provide that definition, or (as applicable)
> include the appropriate header, earlier in the file.
>
> (Ideally the newer compatibility layers would be at the bottom of the
> file, so that they can naturally benefit from the compatibility layers
> for earlier version. But that's rather too much for this series.)
>
> No functional change.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> v2: New patch in v2 of the series
> ---
> include/hw/xen/xen_common.h | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> index 3f44a63..8efdb8a 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -78,6 +78,17 @@ static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
>
> extern xenforeignmemory_handle *xen_fmem;
>
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
> +
> +typedef xc_interface xendevicemodel_handle;
> +
> +#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
> +
> +#undef XC_WANT_COMPAT_DEVICEMODEL_API
> +#include <xendevicemodel.h>
> +
> +#endif
> +
> #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
>
> #define XEN_COMPAT_PHYSMAP
> @@ -105,8 +116,6 @@ static inline int xentoolcore_restrict_all(domid_t domid)
>
> #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
>
> -typedef xc_interface xendevicemodel_handle;
> -
> static inline xendevicemodel_handle *xendevicemodel_open(
> struct xentoollog_logger *logger, unsigned int open_flags)
> {
> @@ -228,11 +237,6 @@ static inline int xendevicemodel_set_mem_type(
> return xc_hvm_set_mem_type(dmod, domid, mem_type, first_pfn, nr);
> }
>
> -#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
> -
> -#undef XC_WANT_COMPAT_DEVICEMODEL_API
> -#include <xendevicemodel.h>
> -
> #endif
>
> extern xendevicemodel_handle *xen_dmod;
> --
> 2.1.4
>
xc_interface_open etc. is not going to work if we have dropped
privilege, but xendevicemodel_shutdown will if everything is new
enough.
xendevicemodel_shutdown is only availabe in Xen 4.10 and later, so
provide a stub for earlier versions.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
v2: Add compatibility stub for Xen < 4.10.
Fix coding style.
---
hw/i386/xen/xen-hvm.c | 10 ++++++++++
include/hw/xen/xen_common.h | 7 +++++++
2 files changed, 17 insertions(+)
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 83420cd..25b8b14 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1386,9 +1386,19 @@ void destroy_hvm_domain(bool reboot)
{
xc_interface *xc_handle;
int sts;
+ int rc;
unsigned int reason = reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff;
+ if (xen_dmod) {
+ rc = xendevicemodel_shutdown(xen_dmod, xen_domid, reason);
+ if (!rc) {
+ return;
+ }
+ perror("xendevicemodel_shutdown failed");
+ /* well, try the old thing then */
+ }
+
xc_handle = xc_interface_open(0, 0, 0);
if (xc_handle == NULL) {
fprintf(stderr, "Cannot acquire xenctrl handle\n");
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 8efdb8a..1d6fb57 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -108,6 +108,13 @@ static inline int xentoolcore_restrict_all(domid_t domid)
return -1;
}
+static inline int xendevicemodel_shutdown(xendevicemodel_handle *dmod,
+ domid_t domid, unsigned int reason)
+{
+ errno = ENOTTY;
+ return -1;
+}
+
#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 41000 */
#include <xentoolcore.h>
--
2.1.4
On Fri, 20 Oct 2017, Ian Jackson wrote:
> xc_interface_open etc. is not going to work if we have dropped
> privilege, but xendevicemodel_shutdown will if everything is new
> enough.
>
> xendevicemodel_shutdown is only availabe in Xen 4.10 and later, so
> provide a stub for earlier versions.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> v2: Add compatibility stub for Xen < 4.10.
> Fix coding style.
> ---
> hw/i386/xen/xen-hvm.c | 10 ++++++++++
> include/hw/xen/xen_common.h | 7 +++++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index 83420cd..25b8b14 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -1386,9 +1386,19 @@ void destroy_hvm_domain(bool reboot)
> {
> xc_interface *xc_handle;
> int sts;
> + int rc;
>
> unsigned int reason = reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff;
>
> + if (xen_dmod) {
> + rc = xendevicemodel_shutdown(xen_dmod, xen_domid, reason);
> + if (!rc) {
> + return;
> + }
> + perror("xendevicemodel_shutdown failed");
I don't think is a good idea to print an error because this is actually
a normal condition when QEMU is build and run against an older Xen.
Users might get confused when looking at the logs.
But it would be correct to print an error if errno != ENOTTY.
> + /* well, try the old thing then */
> + }
> +
> xc_handle = xc_interface_open(0, 0, 0);
> if (xc_handle == NULL) {
> fprintf(stderr, "Cannot acquire xenctrl handle\n");
> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> index 8efdb8a..1d6fb57 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -108,6 +108,13 @@ static inline int xentoolcore_restrict_all(domid_t domid)
> return -1;
> }
>
> +static inline int xendevicemodel_shutdown(xendevicemodel_handle *dmod,
> + domid_t domid, unsigned int reason)
> +{
> + errno = ENOTTY;
> + return -1;
> +}
> +
> #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 41000 */
>
> #include <xentoolcore.h>
> --
> 2.1.4
>
Stefano Stabellini writes ("Re: [PATCH v5.1 6/8] xen: destroy_hvm_domain: Try xendevicemodel_shutdown"):
> On Fri, 20 Oct 2017, Ian Jackson wrote:
> > xc_interface_open etc. is not going to work if we have dropped
> > privilege, but xendevicemodel_shutdown will if everything is new
> > enough.
> >
> > xendevicemodel_shutdown is only availabe in Xen 4.10 and later, so
> > provide a stub for earlier versions.
...
> > + if (xen_dmod) {
> > + rc = xendevicemodel_shutdown(xen_dmod, xen_domid, reason);
> > + if (!rc) {
> > + return;
> > + }
> > + perror("xendevicemodel_shutdown failed");
>
> I don't think is a good idea to print an error because this is actually
> a normal condition when QEMU is build and run against an older Xen.
> Users might get confused when looking at the logs.
Oh. Yes. I wrote this before I provided the fallback stub in
xen_common.h, and therefore before I properly understood the approach
taken to fallbacks. The fallback logic here is not correct.
> But it would be correct to print an error if errno != ENOTTY.
Indeed.
I have changed it to read like this:
if (xen_dmod) {
rc = xendevicemodel_shutdown(xen_dmod, xen_domid, reason);
if (!rc) {
return;
}
if (errno != ENOTTY /* old Xen */)
perror("xendevicemodel_shutdown failed");
/* well, try the old thing then */
}
Thanks,
Ian.
This allows the caller to specify a uid and gid to use, even if there
is no corresponding password entry. This will be useful in certain
Xen configurations.
We don't support just -runas <uid> because: (i) deprivileging without
calling setgroups would be ineffective (ii) given only a uid we don't
know what gid we ought to use (since uids may eppear in multiple
passwd file entries with different gids).
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v5: Use : rather than . to separate uid from gid
v4: Changed to reuse option -runas
v3: Error messages fixed. Thanks to Peter Maydell and Ross Lagerwall.
v2: Coding style fixes.
squash! os-posix: Provide new -runas <uid>.<gid> facility
---
os-posix.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++----------
qemu-options.hx | 3 ++-
2 files changed, 55 insertions(+), 12 deletions(-)
diff --git a/os-posix.c b/os-posix.c
index 92e9d85..f95b7bf 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -43,6 +43,8 @@
#endif
static struct passwd *user_pwd;
+static uid_t user_uid = (uid_t)-1;
+static gid_t user_gid = (gid_t)-1;
static const char *chroot_dir;
static int daemonize;
static int daemon_pipe;
@@ -128,6 +130,34 @@ void os_set_proc_name(const char *s)
#endif
}
+
+static bool os_parse_runas_uid_gid(const char *optarg)
+{
+ unsigned long lv;
+ char *ep;
+ uid_t got_uid;
+ gid_t got_gid;
+ int rc;
+
+ errno = 0;
+ lv = strtoul(optarg, &ep, 0); /* can't qemu_strtoul, want *ep==':' */
+ got_uid = lv; /* overflow here is ID in C99 */
+ if (errno || *ep != ':' || got_uid != lv || got_uid == (uid_t)-1) {
+ return false;
+ }
+
+ lv = 0;
+ rc = qemu_strtoul(ep + 1, 0, 0, &lv);
+ got_gid = lv; /* overflow here is ID in C99 */
+ if (rc || got_gid != lv || got_gid == (gid_t)-1) {
+ return false;
+ }
+
+ user_uid = got_uid;
+ user_gid = got_gid;
+ return true;
+}
+
/*
* Parse OS specific command line options.
* return 0 if option handled, -1 otherwise
@@ -145,8 +175,10 @@ void os_parse_cmd_args(int index, const char *optarg)
#endif
case QEMU_OPTION_runas:
user_pwd = getpwnam(optarg);
- if (!user_pwd) {
- fprintf(stderr, "User \"%s\" doesn't exist\n", optarg);
+ if (!user_pwd && !os_parse_runas_uid_gid(optarg)) {
+ fprintf(stderr,
+ "User \"%s\" doesn't exist (and is not <uid>.<gid>)\n",
+ optarg);
exit(1);
}
break;
@@ -166,18 +198,28 @@ void os_parse_cmd_args(int index, const char *optarg)
static void change_process_uid(void)
{
- if (user_pwd) {
- if (setgid(user_pwd->pw_gid) < 0) {
- fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
+ if (user_pwd || user_uid != (uid_t)-1) {
+ gid_t intended_gid = user_pwd ? user_pwd->pw_gid : user_gid;
+ uid_t intended_uid = user_pwd ? user_pwd->pw_uid : user_uid;
+ if (setgid(intended_gid) < 0) {
+ fprintf(stderr, "Failed to setgid(%d)\n", intended_gid);
exit(1);
}
- if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
- fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
- user_pwd->pw_name, user_pwd->pw_gid);
- exit(1);
+ if (user_pwd) {
+ if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
+ fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
+ user_pwd->pw_name, user_pwd->pw_gid);
+ exit(1);
+ }
+ } else {
+ if (setgroups(1, &user_gid) < 0) {
+ fprintf(stderr, "Failed to setgroups(1, [%d])",
+ user_gid);
+ exit(1);
+ }
}
- if (setuid(user_pwd->pw_uid) < 0) {
- fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
+ if (setuid(intended_uid) < 0) {
+ fprintf(stderr, "Failed to setuid(%d)\n", intended_uid);
exit(1);
}
if (setuid(0) != -1) {
diff --git a/qemu-options.hx b/qemu-options.hx
index 9f6e2ad..f707197e 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3958,7 +3958,8 @@ ETEXI
#ifndef _WIN32
DEF("runas", HAS_ARG, QEMU_OPTION_runas, \
- "-runas user change to user id user just before starting the VM\n",
+ "-runas user change to user id user just before starting the VM\n" \
+ " user can be numeric uid:gid instead\n",
QEMU_ARCH_ALL)
#endif
STEXI
--
2.1.4
On Fri, Oct 20, 2017 at 02:38:21PM +0100, Ian Jackson wrote:
> +static bool os_parse_runas_uid_gid(const char *optarg)
> +{
> + unsigned long lv;
> + char *ep;
> + uid_t got_uid;
> + gid_t got_gid;
> + int rc;
> +
> + errno = 0;
> + lv = strtoul(optarg, &ep, 0); /* can't qemu_strtoul, want *ep==':' */
Should strtoul base be 10? If that matter.
> + got_uid = lv; /* overflow here is ID in C99 */
> + if (errno || *ep != ':' || got_uid != lv || got_uid == (uid_t)-1) {
> + return false;
> + }
> +
> + lv = 0;
> + rc = qemu_strtoul(ep + 1, 0, 0, &lv);
> + got_gid = lv; /* overflow here is ID in C99 */
> + if (rc || got_gid != lv || got_gid == (gid_t)-1) {
> + return false;
> + }
> +
> + user_uid = got_uid;
> + user_gid = got_gid;
> + return true;
> +}
> +
> /*
> * Parse OS specific command line options.
> * return 0 if option handled, -1 otherwise
> @@ -145,8 +175,10 @@ void os_parse_cmd_args(int index, const char *optarg)
> #endif
> case QEMU_OPTION_runas:
> user_pwd = getpwnam(optarg);
> - if (!user_pwd) {
> - fprintf(stderr, "User \"%s\" doesn't exist\n", optarg);
> + if (!user_pwd && !os_parse_runas_uid_gid(optarg)) {
> + fprintf(stderr,
> + "User \"%s\" doesn't exist (and is not <uid>.<gid>)\n",
The error message have not been update, I think it should be <uid>:<gid>
> + optarg);
> exit(1);
> }
> break;
With the error message fix:
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
--
Anthony PERARD
Anthony PERARD writes ("Re: [PATCH v5.1 7/8] os-posix: Provide new -runas <uid>:<gid> facility"):
> On Fri, Oct 20, 2017 at 02:38:21PM +0100, Ian Jackson wrote:
> > +static bool os_parse_runas_uid_gid(const char *optarg)
...
> > + errno = 0;
> > + lv = strtoul(optarg, &ep, 0); /* can't qemu_strtoul, want *ep==':' */
>
> Should strtoul base be 10? If that matter.
If someone wants to write uids in hex then I don't see a reason to
stop them...
> > - if (!user_pwd) {
> > - fprintf(stderr, "User \"%s\" doesn't exist\n", optarg);
> > + if (!user_pwd && !os_parse_runas_uid_gid(optarg)) {
> > + fprintf(stderr,
> > + "User \"%s\" doesn't exist (and is not <uid>.<gid>)\n",
>
> The error message have not been update, I think it should be <uid>:<gid>
Oops.
> With the error message fix:
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Thanks,
Ian.
CC'ing the maintainers (scripts/get_maintainer.pl is your friend)
On Fri, 20 Oct 2017, Ian Jackson wrote:
> This allows the caller to specify a uid and gid to use, even if there
> is no corresponding password entry. This will be useful in certain
> Xen configurations.
>
> We don't support just -runas <uid> because: (i) deprivileging without
> calling setgroups would be ineffective (ii) given only a uid we don't
> know what gid we ought to use (since uids may eppear in multiple
> passwd file entries with different gids).
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
> v5: Use : rather than . to separate uid from gid
> v4: Changed to reuse option -runas
> v3: Error messages fixed. Thanks to Peter Maydell and Ross Lagerwall.
> v2: Coding style fixes.
>
> squash! os-posix: Provide new -runas <uid>.<gid> facility
> ---
> os-posix.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++----------
> qemu-options.hx | 3 ++-
> 2 files changed, 55 insertions(+), 12 deletions(-)
>
> diff --git a/os-posix.c b/os-posix.c
> index 92e9d85..f95b7bf 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -43,6 +43,8 @@
> #endif
>
> static struct passwd *user_pwd;
> +static uid_t user_uid = (uid_t)-1;
> +static gid_t user_gid = (gid_t)-1;
> static const char *chroot_dir;
> static int daemonize;
> static int daemon_pipe;
> @@ -128,6 +130,34 @@ void os_set_proc_name(const char *s)
> #endif
> }
>
> +
> +static bool os_parse_runas_uid_gid(const char *optarg)
> +{
> + unsigned long lv;
> + char *ep;
> + uid_t got_uid;
> + gid_t got_gid;
> + int rc;
> +
> + errno = 0;
> + lv = strtoul(optarg, &ep, 0); /* can't qemu_strtoul, want *ep==':' */
> + got_uid = lv; /* overflow here is ID in C99 */
> + if (errno || *ep != ':' || got_uid != lv || got_uid == (uid_t)-1) {
> + return false;
> + }
> +
> + lv = 0;
> + rc = qemu_strtoul(ep + 1, 0, 0, &lv);
> + got_gid = lv; /* overflow here is ID in C99 */
> + if (rc || got_gid != lv || got_gid == (gid_t)-1) {
> + return false;
> + }
> +
> + user_uid = got_uid;
> + user_gid = got_gid;
> + return true;
> +}
> +
> /*
> * Parse OS specific command line options.
> * return 0 if option handled, -1 otherwise
> @@ -145,8 +175,10 @@ void os_parse_cmd_args(int index, const char *optarg)
> #endif
> case QEMU_OPTION_runas:
> user_pwd = getpwnam(optarg);
> - if (!user_pwd) {
> - fprintf(stderr, "User \"%s\" doesn't exist\n", optarg);
> + if (!user_pwd && !os_parse_runas_uid_gid(optarg)) {
> + fprintf(stderr,
> + "User \"%s\" doesn't exist (and is not <uid>.<gid>)\n",
> + optarg);
> exit(1);
> }
> break;
> @@ -166,18 +198,28 @@ void os_parse_cmd_args(int index, const char *optarg)
>
> static void change_process_uid(void)
> {
> - if (user_pwd) {
> - if (setgid(user_pwd->pw_gid) < 0) {
> - fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
> + if (user_pwd || user_uid != (uid_t)-1) {
> + gid_t intended_gid = user_pwd ? user_pwd->pw_gid : user_gid;
> + uid_t intended_uid = user_pwd ? user_pwd->pw_uid : user_uid;
> + if (setgid(intended_gid) < 0) {
> + fprintf(stderr, "Failed to setgid(%d)\n", intended_gid);
> exit(1);
> }
> - if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
> - fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
> - user_pwd->pw_name, user_pwd->pw_gid);
> - exit(1);
> + if (user_pwd) {
> + if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
> + fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
> + user_pwd->pw_name, user_pwd->pw_gid);
> + exit(1);
> + }
> + } else {
> + if (setgroups(1, &user_gid) < 0) {
> + fprintf(stderr, "Failed to setgroups(1, [%d])",
> + user_gid);
> + exit(1);
> + }
> }
> - if (setuid(user_pwd->pw_uid) < 0) {
> - fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
> + if (setuid(intended_uid) < 0) {
> + fprintf(stderr, "Failed to setuid(%d)\n", intended_uid);
> exit(1);
> }
> if (setuid(0) != -1) {
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9f6e2ad..f707197e 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3958,7 +3958,8 @@ ETEXI
>
> #ifndef _WIN32
> DEF("runas", HAS_ARG, QEMU_OPTION_runas, \
> - "-runas user change to user id user just before starting the VM\n",
> + "-runas user change to user id user just before starting the VM\n" \
> + " user can be numeric uid:gid instead\n",
> QEMU_ARCH_ALL)
> #endif
> STEXI
> --
> 2.1.4
>
Stefano Stabellini writes ("Re: [PATCH v5.1 7/8] os-posix: Provide new -runas <uid>:<gid> facility"):
> CC'ing the maintainers (scripts/get_maintainer.pl is your friend)
I don't know what your scripts/get_maintainer.pl does, but mine
says:
get_maintainer.pl: No maintainers found, printing recent contributors.
get_maintainer.pl: Do not blindly cc: them on patches! Use common sense.
Anthony PERARD <anthony.perard@citrix.com> (commit_signer:1/2=50%)
Paolo Bonzini <pbonzini@redhat.com> (commit_signer:1/2=50%,commit_signer:11/57=19%)
Ian Jackson <Ian.Jackson@eu.citrix.com> (commit_signer:1/2=50%)
Michael Tokarev <mjt@tls.msk.ru> (commit_signer:12/57=21%)
Eric Blake <eblake@redhat.com> (commit_signer:10/57=18%)
Thomas Huth <thuth@redhat.com> (commit_signer:8/57=14%)
Markus Armbruster <armbru@redhat.com> (commit_signer:8/57=14%)
qemu-devel@nongnu.org (open list:POSIX)
I have added Paolo, Markus and Daniel Berrange to the CCs of my patch
on the basis that they have commented already...
Thanks,
Ian.
This makes it much easier to find a particular thing in config.log.
The information may be lacking in other shells, resulting in harmless
empty output. (This is why we don't use the proper ${FUNCNAME[*]}
array syntax - other shells will choke on that.)
The extra output is only printed if configure is run with bash. The
something), it is necessary to say bash ./configure to get the extra
debug info in the log.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v4: No longer tag this patch RFC.
---
configure | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/configure b/configure
index 6f691df..21a2b15 100755
--- a/configure
+++ b/configure
@@ -60,6 +60,10 @@ do_compiler() {
# is compiler binary to execute.
local compiler="$1"
shift
+ echo >>config.log "
+funcs: ${FUNCNAME}
+lines: ${BASH_LINENO}
+files: ${BASH_SOURCE}"
echo $compiler "$@" >> config.log
$compiler "$@" >> config.log 2>&1 || return $?
# Test passed. If this is an --enable-werror build, rerun
--
2.1.4
Ian Jackson writes ("[PATCH v5.1 8/8] configure: do_compiler: Dump some extra info under bash"):
> This makes it much easier to find a particular thing in config.log.
>
> The information may be lacking in other shells, resulting in harmless
> empty output. (This is why we don't use the proper ${FUNCNAME[*]}
> array syntax - other shells will choke on that.)
>
> The extra output is only printed if configure is run with bash. The
> something), it is necessary to say bash ./configure to get the extra
> debug info in the log.
Kent Spillner points out that this last sentence is garbled. The
paragraph should read:
The extra output is only printed if configure is run with bash. On
systems where /bin/sh is not bash, it is necessary to say bash
./configure to get the extra debug info in the log.
I have updated it in my branch.
Ian.
CC'ing the maintainers for this.
On Fri, 20 Oct 2017, Ian Jackson wrote:
> This makes it much easier to find a particular thing in config.log.
>
> The information may be lacking in other shells, resulting in harmless
> empty output. (This is why we don't use the proper ${FUNCNAME[*]}
> array syntax - other shells will choke on that.)
>
> The extra output is only printed if configure is run with bash. The
> something), it is necessary to say bash ./configure to get the extra
> debug info in the log.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
> v4: No longer tag this patch RFC.
> ---
> configure | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/configure b/configure
> index 6f691df..21a2b15 100755
> --- a/configure
> +++ b/configure
> @@ -60,6 +60,10 @@ do_compiler() {
> # is compiler binary to execute.
> local compiler="$1"
> shift
> + echo >>config.log "
> +funcs: ${FUNCNAME}
> +lines: ${BASH_LINENO}
> +files: ${BASH_SOURCE}"
> echo $compiler "$@" >> config.log
> $compiler "$@" >> config.log 2>&1 || return $?
> # Test passed. If this is an --enable-werror build, rerun
> --
> 2.1.4
>
Stefano Stabellini writes ("Re: [PATCH v5.1 8/8] configure: do_compiler: Dump some extra info under bash"):
> CC'ing the maintainers for this.
Thanks, but scripts/get_maintainer.pl seems to print different
information for me... (see my other mail)
Ian.
© 2016 - 2024 Red Hat, Inc.