From: Manuel Bouyer <bouyer@netbsd.org>

NetBSD doesn't have setresuid(). Add a configure check for it,
and use plain setuid() if !HAVE_SETRESUID

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/configure             | 13 +++++++++++++
 tools/configure.ac          |  3 +++
 tools/libs/light/libxl_dm.c | 10 ++++++++++
 3 files changed, 26 insertions(+)

diff --git a/tools/configure b/tools/configure
index 131112c41e..5e3793709e 100755
--- a/tools/configure
+++ b/tools/configure
@@ -9299,6 +9299,19 @@ _ACEOF
 
 esac
 
+# NetBSD doesnt have setresuid (yet)
+for ac_func in setresuid
+do :
+  ac_fn_c_check_func "$LINENO" "setresuid" "ac_cv_func_setresuid"
+if test "x$ac_cv_func_setresuid" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_SETRESUID 1
+_ACEOF
+
+fi
+done
+
+
 # Checks for header files.
 for ac_header in yajl/yajl_version.h sys/eventfd.h valgrind/memcheck.h utmp.h
 do :
diff --git a/tools/configure.ac b/tools/configure.ac
index ee8ba5ff24..04f78bf21d 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -457,6 +457,9 @@ AC_CHECK_DECLS([fdt_first_subnode, fdt_next_subnode],,,[#include <libfdt.h>])
 AC_CHECK_DECLS([fdt_property_u32],,,[#include <libfdt.h>])
 esac
 
+# NetBSD doesnt have setresuid (yet)
+AC_CHECK_FUNCS([setresuid])
+
 # Checks for header files.
 AC_CHECK_HEADERS([yajl/yajl_version.h sys/eventfd.h valgrind/memcheck.h utmp.h])
 
diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 8866c3f5ad..7651429b9f 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -3653,6 +3653,7 @@ static int kill_device_model_uid_child(libxl__destroy_devicemodel_state *ddms,
     assert(reaper_uid);
     assert(dm_kill_uid);
 
+#if HAVE_SETRESUID
     LOGD(DEBUG, domid, "DM reaper: calling setresuid(%d, %d, 0)",
          reaper_uid, dm_kill_uid);
     r = setresuid(reaper_uid, dm_kill_uid, 0);
@@ -3662,6 +3663,15 @@ static int kill_device_model_uid_child(libxl__destroy_devicemodel_state *ddms,
         rc = rc ?: ERROR_FAIL;
         goto out;
     }
+#else /* HAVE_SETRESUID */
+    LOGD(DEBUG, domid, "DM reaper: calling setuid(%d)", dm_kill_uid);
+    r = setuid(dm_kill_uid);
+    if (r) {
+        LOGED(ERROR, domid, "setuid to %d", dm_kill_uid);
+        rc = rc ?: ERROR_FAIL;
+        goto out;
+    }
+#endif /* HAVE_SETRESUID */
 
     /*
      * And kill everyone but me.
-- 
2.29.2

On Tue, Jan 12, 2021 at 07:12:36PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> NetBSD doesn't have setresuid(). Add a configure check for it,
> and use plain setuid() if !HAVE_SETRESUID
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

LGTM from a code PoV, but I think George/Ian should take a look, since
they know exactly what this is supposed to do, and I would bet there
are some reasons why setresuid is used instead of setuid, which should
likely be taken into account in the commit message to justify why
using setuid in it's place it's fine.

> ---
>  tools/configure             | 13 +++++++++++++
>  tools/configure.ac          |  3 +++
>  tools/libs/light/libxl_dm.c | 10 ++++++++++
>  3 files changed, 26 insertions(+)
> 
> diff --git a/tools/configure b/tools/configure
> index 131112c41e..5e3793709e 100755
> --- a/tools/configure
> +++ b/tools/configure
> @@ -9299,6 +9299,19 @@ _ACEOF
>  
>  esac
>  
> +# NetBSD doesnt have setresuid (yet)
> +for ac_func in setresuid
> +do :
> +  ac_fn_c_check_func "$LINENO" "setresuid" "ac_cv_func_setresuid"
> +if test "x$ac_cv_func_setresuid" = xyes; then :
> +  cat >>confdefs.h <<_ACEOF
> +#define HAVE_SETRESUID 1
> +_ACEOF
> +
> +fi
> +done
> +

We usually leave the changes to the generated configure script out of
the patch since it's usually a PITA to run the same autoconf version
in order to not generate tons of unrelated changes. You have managed
to run the same version, so I guess it's fine.

Just so that you know that you can ask commtters to re-run autoconf for
you by adding a note to the commit message to that effect.

Thanks, Roger.

Roger Pau Monné writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> On Tue, Jan 12, 2021 at 07:12:36PM +0100, Manuel Bouyer wrote:
> > From: Manuel Bouyer <bouyer@netbsd.org>
> > 
> > NetBSD doesn't have setresuid(). Add a configure check for it,
> > and use plain setuid() if !HAVE_SETRESUID
...
> LGTM from a code PoV, but I think George/Ian should take a look, since
> they know exactly what this is supposed to do, and I would bet there
> are some reasons why setresuid is used instead of setuid, which should
> likely be taken into account in the commit message to justify why
> using setuid in it's place it's fine.

There is indeed a reason for using setresuid here.  See the comments
at the top of kill_device_model_uid_child and the commit messages for
87f9458e3400 and 0c653574d39c.  This is all quite complex:

https://xenproject.org/2018/08/01/killing-processes-that-dont-want-to-be-killed/

https://marc.info/?l=xen-devel&m=152215770803468
 (search in that message for "libxl UID cleanup")

I wrote a message to George in 2018 proving that the desired set of
IDs cannot be made without setresuid.  I'll c&p the relevant part below.

I don't think setuid is safe - at least, if we are trying to restrict
the dm.  Since I think after the libxl child is forked, and has called
setuid, it might be traceable (by NetBSD's equivalent of ptrace) by
the dm.  The dm could puppet it into pretending it had succeeded, but
then hang around until the domid is reused.

At the very least, this patch needs an argument, in detail, why this
is OK.

Also, why oh why does NetBSD not have setresuid ??  It's at least 20
years old !

Sorry,
Ian.

PS there is a long discussion of the history of saved set-ids, real vs
effective uids, etc., here
  https://pubs.opengroup.org/onlinepubs/9699919799/functions/setuid.html
but sadly it does not discuss setresuid.


From me to George etc. in 2018.

George emailed me a draft post:
> # No POSIX-compliant mousetraps?
> 
> Although `setresuid` is implemented by both Linux and FreeBSD, it is
> not in the [current POSIX
> specification](http://pubs.opengroup.org/onlinepubs/9699919799/).
> Looking at the official list of POSIX system interfaces, it's not
> clear how to get a process to have the required tuple using only POSIX
> interfaces (namely `setuid` and `setreuid`, without recourse to
> `setresuid` or Linux's `CAP_SETUID`); the assumption seems to be that
> `euid` must always be set to either `ruid` or `suid`.

Proof that this can't be simulated by proper use of setuid, seteuid
and setreuid:

					ruid	euid	suid

The desired state is:			reaper	target	reaper

If the final call is seteuid:

   seteuid(target);			reaper	target	reaper

For this to be permitted, and nontrivial, euid was 0:
   
Penultimate status			reaper	0	reaper

This state cannot be generated by setuid either euid==0 previously and
setuid would have set all of the ids; or the old euid was not 0, in
which case setuid() would have set only the euid, and required that
one of the other ids was 0, which can see that it can't have been.

This penultimate state cannot be generated by seteuid from any
different state.

So it must have been generated by setreuid.  We must avoid setreuid
setting the suid to the same as the new euid (0), which means that our
setreuid call did not change the ruid either.  That form of setreuid
is just like euid for our purposes, and not useful.

So the desired state could not be made by seteuid.

Let's consider setreuid.  Well, either setreuid sets the suid to the
same as the new euid, or it only changes the euid.  Ie, it would only
do something we could have done with seteuid and the argument above
applies.

What abouit setuid ?  Well, either setuid sets all three uids to the
same thing, or it, again, sets only the euid.

Ian.

On Wed, Jan 20, 2021 at 02:52:06PM +0000, Ian Jackson wrote:
> Roger Pau Monné writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> > On Tue, Jan 12, 2021 at 07:12:36PM +0100, Manuel Bouyer wrote:
> > > From: Manuel Bouyer <bouyer@netbsd.org>
> > > 
> > > NetBSD doesn't have setresuid(). Add a configure check for it,
> > > and use plain setuid() if !HAVE_SETRESUID
> ...
> > LGTM from a code PoV, but I think George/Ian should take a look, since
> > they know exactly what this is supposed to do, and I would bet there
> > are some reasons why setresuid is used instead of setuid, which should
> > likely be taken into account in the commit message to justify why
> > using setuid in it's place it's fine.
> 
> There is indeed a reason for using setresuid here.  See the comments
> at the top of kill_device_model_uid_child and the commit messages for
> 87f9458e3400 and 0c653574d39c.  This is all quite complex:
> 
> https://xenproject.org/2018/08/01/killing-processes-that-dont-want-to-be-killed/
> 
> https://marc.info/?l=xen-devel&m=152215770803468
>  (search in that message for "libxl UID cleanup")
> 
> I wrote a message to George in 2018 proving that the desired set of
> IDs cannot be made without setresuid.  I'll c&p the relevant part below.
> 
> I don't think setuid is safe - at least, if we are trying to restrict
> the dm.  Since I think after the libxl child is forked, and has called

What is the dm in this case ? qemu ? On NetBSD qemu runs as root AFAIK,
so there isn't much to protect.

> setuid, it might be traceable (by NetBSD's equivalent of ptrace) by
> the dm.  The dm could puppet it into pretending it had succeeded, but
> then hang around until the domid is reused.

I don't understand. We're talking about a simple kill(2) syscall here.

> 
> At the very least, this patch needs an argument, in detail, why this
> is OK.
> 
> Also, why oh why does NetBSD not have setresuid ??  It's at least 20
> years old !

It's not because it's old that it's good.



> 
> Sorry,

OK so if I understand properly, you say Xen should not be used on NetBSD ?

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> On Wed, Jan 20, 2021 at 02:52:06PM +0000, Ian Jackson wrote:
> > I don't think setuid is safe - at least, if we are trying to restrict
> > the dm.  Since I think after the libxl child is forked, and has called
> 
> What is the dm in this case ? qemu ? On NetBSD qemu runs as root AFAIK,
> so there isn't much to protect.

Yes, the dm is qemu.  If qemu restriction is not supported, that makes
a big difference.  The complex situation here is to do with trying to
kill a possibly hostile qemu.

> > setuid, it might be traceable (by NetBSD's equivalent of ptrace) by
> > the dm.  The dm could puppet it into pretending it had succeeded, but
> > then hang around until the domid is reused.
> 
> I don't understand. We're talking about a simple kill(2) syscall here.

If we're not trying to restrict qemu's privilege at all, then I think
the setuid is fine.  There are then only two remaining concerns I have
with this patch:

Firstly, we try to avoid #ifdefs like this.  It tends to make the code
rather tangled, especially over time.  Instead we prefer to move the
non-portable code into its own file, eg *_linux.c.

Secondly, I think we should check that dm_restrict is not enabled.
I think an assert would do since I think we believe this is already
prevented elsewhere ?

(One option for making this work would be to simply disable the
killing by uid on NetBSD.  But I don't think that's a good answer
because killing by uid after eg setuid is more reliable even if it is
not 100% bulletproof.  So switching to setuid or maybe setreuid is the
right answer.)

> OK so if I understand properly, you say Xen should not be used on NetBSD ?

I'm sorry to have offended and discouraged you.  That was not my
intention.  My apologies for sending an off-putting message.  For the
avoidance of any doubt, definitely don't think that.  We should make
this work properly.

Would you be willing to look into the two points I mention above and
send a revised version of the patch ?  If you find the refactoring
awkward I or Roger can help.

Regards,
Ian.

On Wed, Jan 20, 2021 at 03:32:29PM +0000, Ian Jackson wrote:
> Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> > On Wed, Jan 20, 2021 at 02:52:06PM +0000, Ian Jackson wrote:
> > > I don't think setuid is safe - at least, if we are trying to restrict
> > > the dm.  Since I think after the libxl child is forked, and has called
> > 
> > What is the dm in this case ? qemu ? On NetBSD qemu runs as root AFAIK,
> > so there isn't much to protect.
> 
> Yes, the dm is qemu.  If qemu restriction is not supported, that makes
> a big difference.  The complex situation here is to do with trying to
> kill a possibly hostile qemu.

Hum, I'll have to check this (how to check, BTW ?).
I assumed qemu was running as root but it may not be completely true.
Especially as I notice, now that I'm re-reading the patch, that
we're doing a kill to -1. If we were doing so as root, user processes
would be killed.

> 
> > > setuid, it might be traceable (by NetBSD's equivalent of ptrace) by
> > > the dm.  The dm could puppet it into pretending it had succeeded, but
> > > then hang around until the domid is reused.
> > 
> > I don't understand. We're talking about a simple kill(2) syscall here.
> 
> If we're not trying to restrict qemu's privilege at all, then I think
> the setuid is fine.
> There are then only two remaining concerns I have
> with this patch:
> 
> Firstly, we try to avoid #ifdefs like this.  It tends to make the code
> rather tangled, especially over time.  Instead we prefer to move the
> non-portable code into its own file, eg *_linux.c.
> 
> Secondly, I think we should check that dm_restrict is not enabled.
> I think an assert would do since I think we believe this is already
> prevented elsewhere ?
> 
> (One option for making this work would be to simply disable the
> killing by uid on NetBSD.  But I don't think that's a good answer
> because killing by uid after eg setuid is more reliable even if it is
> not 100% bulletproof.  So switching to setuid or maybe setreuid is the
> right answer.)

This would have to be checked, but I don't think a non-root process
can ptrace a process whose saved-user-id is root.

Actually I think I could mimic the setresuid() with setreuid() and seteuid().

> 
> > OK so if I understand properly, you say Xen should not be used on NetBSD ?
> 
> I'm sorry to have offended and discouraged you.  That was not my
> intention.  My apologies for sending an off-putting message.  For the
> avoidance of any doubt, definitely don't think that.  We should make
> this work properly.
> 
> Would you be willing to look into the two points I mention above and
> send a revised version of the patch ?  If you find the refactoring
> awkward I or Roger can help.

Actually I don't see how I could split this in a different file, without
lot of duplicate code (even in just kill_device_model_uid_child(),
we're talking of about 7 lines of code out of 75). So some guidance here
would be welcome.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> On Wed, Jan 20, 2021 at 03:32:29PM +0000, Ian Jackson wrote:
> > Yes, the dm is qemu.  If qemu restriction is not supported, that makes
> > a big difference.  The complex situation here is to do with trying to
> > kill a possibly hostile qemu.
> 
> Hum, I'll have to check this (how to check, BTW ?).
> I assumed qemu was running as root but it may not be completely true.
> Especially as I notice, now that I'm re-reading the patch, that
> we're doing a kill to -1. If we were doing so as root, user processes
> would be killed.

It may well be that this whole piece of code won't be executed on
NetBSD becauwe dm restriction will be off.

The background: dm restriction is a set of arrangements for trying to
run qemu without given it any more privilege than it needs, and
certainly not ultimate privilege over the host.  This is quite
complicated and includes running it as a non-root user, chroot, and so
on.

On Linux it's run in its own network namespace, so that a qemu
compromised by the guest cannot access host daemons.  IDK what
facilities one might want to use on NetBSD to try to contain qemu.

This seems to me all a matter for future work.  I'm sorry that code
for a feature you're not going to be benefiting from is getting in
your way.

> > (One option for making this work would be to simply disable the
> > killing by uid on NetBSD.  But I don't think that's a good answer
> > because killing by uid after eg setuid is more reliable even if it is
> > not 100% bulletproof.  So switching to setuid or maybe setreuid is the
> > right answer.)
> 
> This would have to be checked, but I don't think a non-root process
> can ptrace a process whose saved-user-id is root.

If I remember rightly the saved-set-id is reset by setuid.  But I
could be wrong.  This stuff is all quite complex :-/.

> Actually I think I could mimic the setresuid() with setreuid() and seteuid().

My last mail had in it a thing that claims to be a proof that this is
not possible.

But I'm hoping we can avoid this.

> > > OK so if I understand properly, you say Xen should not be used on NetBSD ?
> > 
> > I'm sorry to have offended and discouraged you.  That was not my
> > intention.  My apologies for sending an off-putting message.  For the
> > avoidance of any doubt, definitely don't think that.  We should make
> > this work properly.
> > 
> > Would you be willing to look into the two points I mention above and
> > send a revised version of the patch ?  If you find the refactoring
> > awkward I or Roger can help.
> 
> Actually I don't see how I could split this in a different file, without
> lot of duplicate code (even in just kill_device_model_uid_child(),
> we're talking of about 7 lines of code out of 75). So some guidance here
> would be welcome.

I think splitting it out at precisely the function needed is probably
better.

Can you try this experiment: what happens if you replace the call to
setresuid with abort() ?  I think you may find it all works, because
you're not using that code path.

If so then I suggest introducing

  int libxl__setresuid(uid_t ruid, uid_t euid, uid_t suid);

which would call setresuid on Linux and on NetBSD would do this

  assert(!"setresuid is not available on NetBSD, and dm restrction is not supported, so this code path should not have been reached")

What do you think ?

Ian.

On Wed, Jan 20, 2021 at 05:10:36PM +0000, Ian Jackson wrote:
> Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> > On Wed, Jan 20, 2021 at 03:32:29PM +0000, Ian Jackson wrote:
> > > Yes, the dm is qemu.  If qemu restriction is not supported, that makes
> > > a big difference.  The complex situation here is to do with trying to
> > > kill a possibly hostile qemu.
> > 
> > Hum, I'll have to check this (how to check, BTW ?).
> > I assumed qemu was running as root but it may not be completely true.
> > Especially as I notice, now that I'm re-reading the patch, that
> > we're doing a kill to -1. If we were doing so as root, user processes
> > would be killed.
> 
> It may well be that this whole piece of code won't be executed on
> NetBSD becauwe dm restriction will be off.
> 
> The background: dm restriction is a set of arrangements for trying to
> run qemu without given it any more privilege than it needs, and
> certainly not ultimate privilege over the host.  This is quite
> complicated and includes running it as a non-root user, chroot, and so
> on.
> 
> On Linux it's run in its own network namespace, so that a qemu
> compromised by the guest cannot access host daemons.  IDK what
> facilities one might want to use on NetBSD to try to contain qemu.
> 
> This seems to me all a matter for future work.  I'm sorry that code
> for a feature you're not going to be benefiting from is getting in
> your way.

On NetBSD we could start with a different uid and a chroot. This would
limit damages.

> > > right answer.)
> > 
> > This would have to be checked, but I don't think a non-root process
> > can ptrace a process whose saved-user-id is root.
> 
> If I remember rightly the saved-set-id is reset by setuid.  But I
> could be wrong.  This stuff is all quite complex :-/.

yes, setuid() resets the saved-user-id

> 
> > Actually I think I could mimic the setresuid() with setreuid() and seteuid().
> 
> My last mail had in it a thing that claims to be a proof that this is
> not possible.

This code:
        if (setreuid(375,0) < 0) {
                err(1, "setreuid");
        }
        if (seteuid(374) < 0) {
                err(1, "seteuid");
        }
        if (kill(-1, 9)) {
                err(1, "kill");
        }
        printf("kill done\n");
        if (seteuid(0) < 0) {
                err(1, "setreuid2");
        }
        exit(0);

actually works on NetBSD. processes from 375 are killed, and the
seteuid(0) call succeeds (showing that the saved used id is still 0).

> > 
> > Actually I don't see how I could split this in a different file, without
> > lot of duplicate code (even in just kill_device_model_uid_child(),
> > we're talking of about 7 lines of code out of 75). So some guidance here
> > would be welcome.
> 
> I think splitting it out at precisely the function needed is probably
> better.
> 
> Can you try this experiment: what happens if you replace the call to
> setresuid with abort() ?  I think you may find it all works, because
> you're not using that code path.
> 
> If so then I suggest introducing
> 
>   int libxl__setresuid(uid_t ruid, uid_t euid, uid_t suid);
> 
> which would call setresuid on Linux and on NetBSD would do this
> 
>   assert(!"setresuid is not available on NetBSD, and dm restrction is not supported, so this code path should not have been reached")
> 
> What do you think ?

As this is supported by Xen, I hope I can make at last run qemu with a
non-zero uid.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> On Wed, Jan 20, 2021 at 05:10:36PM +0000, Ian Jackson wrote:
> > My last mail had in it a thing that claims to be a proof that this is
> > not possible.
> 
> This code:
>         if (setreuid(375,0) < 0) {
>                 err(1, "setreuid");
>         }
>         if (seteuid(374) < 0) {
>                 err(1, "seteuid");
>         }
>         if (kill(-1, 9)) {
>                 err(1, "kill");
>         }
>         printf("kill done\n");
>         if (seteuid(0) < 0) {
>                 err(1, "setreuid2");
>         }
>         exit(0);
> 
> actually works on NetBSD. processes from 375 are killed, and the
> seteuid(0) call succeeds (showing that the saved used id is still 0).

I guess I must have been wrong.

> > What do you think ?
> 
> As this is supported by Xen, I hope I can make at last run qemu with a
> non-zero uid.

The logic for deciding what user to run qemu as, and whether to kill
by uid or by pid, is in libxl_dm.c, in the function
libxl__domain_get_device_model_uid.

The dm_restrict flag turns on various other things too.

Ian.

Ian Jackson writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> > On Wed, Jan 20, 2021 at 05:10:36PM +0000, Ian Jackson wrote:
> > > My last mail had in it a thing that claims to be a proof that this is
> > > not possible.
> > 
> > This code:
...
> > actually works on NetBSD. processes from 375 are killed, and the
> > seteuid(0) call succeeds (showing that the saved used id is still 0).
> 
> I guess I must have been wrong.
> 
> > > What do you think ?
> > 
> > As this is supported by Xen, I hope I can make at last run qemu with a
> > non-zero uid.
> 
> The logic for deciding what user to run qemu as, and whether to kill
> by uid or by pid, is in libxl_dm.c, in the function
> libxl__domain_get_device_model_uid.
> 
> The dm_restrict flag turns on various other things too.

I think I have lost track of where we are with this patch.  I would
like to see all this properly sorted in Xen 4.15.

How about I write a patch splitting the relevant part up into a
version for systems with setresuid and systems without ?  Then you
could fill in the missing part.

Should I expect the non-setresuid OS to provide effectively the whole
orf kill_device_model_uid_child, or just a replacement for the
setresuid call and surrounding logging, something like
  kill_device_model_uid_child_setresuid
?

Ian.

On Wed, Jan 27, 2021 at 04:03:04PM +0000, Ian Jackson wrote:
> Ian Jackson writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> > Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> > > On Wed, Jan 20, 2021 at 05:10:36PM +0000, Ian Jackson wrote:
> > > > My last mail had in it a thing that claims to be a proof that this is
> > > > not possible.
> > > 
> > > This code:
> ...
> > > actually works on NetBSD. processes from 375 are killed, and the
> > > seteuid(0) call succeeds (showing that the saved used id is still 0).
> > 
> > I guess I must have been wrong.
> > 
> > > > What do you think ?
> > > 
> > > As this is supported by Xen, I hope I can make at last run qemu with a
> > > non-zero uid.
> > 
> > The logic for deciding what user to run qemu as, and whether to kill
> > by uid or by pid, is in libxl_dm.c, in the function
> > libxl__domain_get_device_model_uid.
> > 
> > The dm_restrict flag turns on various other things too.
> 
> I think I have lost track of where we are with this patch.  I would
> like to see all this properly sorted in Xen 4.15.
> 
> How about I write a patch splitting the relevant part up into a
> version for systems with setresuid and systems without ?  Then you
> could fill in the missing part.

Yesterday I sent a v2 with the rewriting you suggested. But I'm fine
with you doing the rewrite.

> 
> Should I expect the non-setresuid OS to provide effectively the whole
> orf kill_device_model_uid_child, or just a replacement for the
> setresuid call and surrounding logging, something like
>   kill_device_model_uid_child_setresuid

As far as I'm concerned, kill_device_model_uid_child_setresuid() is enough.

Unfortunably I don't think I'll have time to work on dm restriction
for NetBSD before 4.15

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> On Wed, Jan 27, 2021 at 04:03:04PM +0000, Ian Jackson wrote:
> > How about I write a patch splitting the relevant part up into a
> > version for systems with setresuid and systems without ?  Then you
> > could fill in the missing part.
> 
> Yesterday I sent a v2 with the rewriting you suggested. But I'm fine
> with you doing the rewrite.

OK.

> > Should I expect the non-setresuid OS to provide effectively the whole
> > orf kill_device_model_uid_child, or just a replacement for the
> > setresuid call and surrounding logging, something like
> >   kill_device_model_uid_child_setresuid
> 
> As far as I'm concerned, kill_device_model_uid_child_setresuid() is enough.
> 
> Unfortunably I don't think I'll have time to work on dm restriction
> for NetBSD before 4.15

No problem.


Taking a step back, I think this series is very close to going in, if
not actually ready.  Do you have a git branch version of this ?

I find the series of un-numbered patches you have posted make it hard
to see the wood for the trees, and collect all the relevant pieces.  A
git branch version would make the life of the committer considerably
easier.

If you don't have a favourite public git server you can post me a link
to, you could use gitlab.com where we have a mirror of xen.git.
   https://gitlab.com/xen-project/xen

(If you need instructions mail me privately.)

Regards,
Ian.

On Thu, Jan 28, 2021 at 11:39:03AM +0000, Ian Jackson wrote:
> [...]
> Taking a step back, I think this series is very close to going in, if
> not actually ready.  Do you have a git branch version of this ?

Actually no. I'm not used to git, and I find it quite hard to use
(and is a large part of the time I spend to get the serie of patches ready).

The only thing I'm doing right now it a git clone from the xen repo,
commit my patches and do a git format-patch
When doing a new version that need code change I start again from scratch,
because I didn't find how to do incremental commits and then
have git format-patch output something sensible.

I nerver understood how git branches and merge were supposed to work
(for example syncing a local repo with upstream, while keeping local
changes).

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--