[PATCH] migration: cpr socket permissions fix

[PATCH] migration: cpr socket permissions fix

[Ben Chaney]

Fix an issue where cpr transfer fails when running with the
-run-with user=$USERID with a permission denied error. This issue
occurs because the destination host creates the transfer sockets before
it drops permissions while the source host tries to connect after
dropping permissions. Fix this by changing the ownership of the cpr
socket to the lower level so it is accessible to both parties.

Resolves: https://lore.kernel.org/all/3D32B62F-29E2-4470-86A5-9A2B3B29E371@akamai.com/
Signed-off-by: Ben Chaney <bchaney@akamai.com>
---
 include/system/os-posix.h |  1 +
 include/system/os-wasm.h  |  1 +
 include/system/os-win32.h |  1 +
 os-posix.c                | 12 ++++++++++++
 stubs/meson.build         |  1 +
 stubs/os-file.c           |  6 ++++++
 util/qemu-sockets.c       |  6 ++++++
 7 files changed, 28 insertions(+)
 create mode 100644 stubs/os-file.c

diff --git a/include/system/os-posix.h b/include/system/os-posix.h
index ce5b3bccf8..e4b69fc316 100644
--- a/include/system/os-posix.h
+++ b/include/system/os-posix.h
@@ -54,6 +54,7 @@ void os_set_chroot(const char *path);
 void os_setup_limits(void);
 void os_setup_post(void);
 int os_mlock(bool on_fault);
+void os_set_socket_permissions(const char *path);
 
 /**
  * qemu_alloc_stack:
diff --git a/include/system/os-wasm.h b/include/system/os-wasm.h
index 3abb3aaa03..eeac092183 100644
--- a/include/system/os-wasm.h
+++ b/include/system/os-wasm.h
@@ -57,6 +57,7 @@ static inline int os_set_daemonize(bool d)
 };
 bool is_daemonized(void);
 static inline void os_daemonize(void) {}
+static inline void os_set_socket_permissions(const char *dummy) {};
 
 /**
  * qemu_alloc_stack:
diff --git a/include/system/os-win32.h b/include/system/os-win32.h
index 22d72babdf..79e42ec297 100644
--- a/include/system/os-win32.h
+++ b/include/system/os-win32.h
@@ -103,6 +103,7 @@ static inline void os_setup_post(void) {}
 static inline void os_set_proc_name(const char *dummy) {}
 void os_set_line_buffering(void);
 void os_setup_early_signal_handling(void);
+static inline void os_set_socket_permissions(const char *dummy) {};
 
 int getpagesize(void);
 
diff --git a/os-posix.c b/os-posix.c
index 52925c23d3..bbd17ff2b9 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -94,6 +94,18 @@ static struct passwd *user_pwd;    /*   NULL   non-NULL   NULL   */
 static uid_t user_uid = (uid_t)-1; /*   -1      -1        >=0    */
 static gid_t user_gid = (gid_t)-1; /*   -1      -1        >=0    */
 
+void os_set_socket_permissions(const char *path)
+{
+    uid_t uid = user_pwd ? user_pwd->pw_uid : user_uid;
+    gid_t gid = user_pwd ? user_pwd->pw_gid : user_gid;
+
+    if (chown(path, uid, gid) < 0) {
+        error_report("Failed to chown socket %s: %s", path, strerror(errno));
+        exit(1);
+    }
+}
+
+
 /*
  * Prepare to change user ID. user_id can be one of 3 forms:
  *   - a username, in which case user ID will be changed to its uid,
diff --git a/stubs/meson.build b/stubs/meson.build
index 0b2778c568..4a4342344b 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -24,6 +24,7 @@ if have_block
   stub_ss.add(files('ram-block.c'))
   stub_ss.add(files('runstate-check.c'))
   stub_ss.add(files('uuid.c'))
+  stub_ss.add(files('os-file.c'))
 endif
 
 if have_block or have_ga
diff --git a/stubs/os-file.c b/stubs/os-file.c
new file mode 100644
index 0000000000..c32cbc7efa
--- /dev/null
+++ b/stubs/os-file.c
@@ -0,0 +1,6 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#include "qemu/osdep.h"
+
+void os_set_socket_permissions(const char *dummy)
+{
+}
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 4773755fd5..77b2d9287b 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1026,6 +1026,12 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
         error_setg_errno(errp, errno, "Failed to bind socket to %s", path);
         goto err;
     }
+    /*
+     * Change the permissions on the socket to match the permission of the
+     * counterparty process
+     */
+    os_set_socket_permissions(un.sun_path);
+
     if (listen(sock, num) < 0) {
         error_setg_errno(errp, errno, "Failed to listen on socket");
         goto err;
-- 
2.34.1

Re: [PATCH] migration: cpr socket permissions fix

[Peter Xu]

On Thu, Nov 20, 2025 at 01:57:33PM -0500, Ben Chaney wrote:
> Fix an issue where cpr transfer fails when running with the
> -run-with user=$USERID with a permission denied error. This issue
> occurs because the destination host creates the transfer sockets before
> it drops permissions while the source host tries to connect after
> dropping permissions. Fix this by changing the ownership of the cpr
> socket to the lower level so it is accessible to both parties.
> 
> Resolves: https://lore.kernel.org/all/3D32B62F-29E2-4470-86A5-9A2B3B29E371@akamai.com/
> Signed-off-by: Ben Chaney <bchaney@akamai.com>
> ---
>  include/system/os-posix.h |  1 +
>  include/system/os-wasm.h  |  1 +
>  include/system/os-win32.h |  1 +
>  os-posix.c                | 12 ++++++++++++
>  stubs/meson.build         |  1 +
>  stubs/os-file.c           |  6 ++++++
>  util/qemu-sockets.c       |  6 ++++++
>  7 files changed, 28 insertions(+)
>  create mode 100644 stubs/os-file.c
> 
> diff --git a/include/system/os-posix.h b/include/system/os-posix.h
> index ce5b3bccf8..e4b69fc316 100644
> --- a/include/system/os-posix.h
> +++ b/include/system/os-posix.h
> @@ -54,6 +54,7 @@ void os_set_chroot(const char *path);
>  void os_setup_limits(void);
>  void os_setup_post(void);
>  int os_mlock(bool on_fault);
> +void os_set_socket_permissions(const char *path);
>  
>  /**
>   * qemu_alloc_stack:
> diff --git a/include/system/os-wasm.h b/include/system/os-wasm.h
> index 3abb3aaa03..eeac092183 100644
> --- a/include/system/os-wasm.h
> +++ b/include/system/os-wasm.h
> @@ -57,6 +57,7 @@ static inline int os_set_daemonize(bool d)
>  };
>  bool is_daemonized(void);
>  static inline void os_daemonize(void) {}
> +static inline void os_set_socket_permissions(const char *dummy) {};
>  
>  /**
>   * qemu_alloc_stack:
> diff --git a/include/system/os-win32.h b/include/system/os-win32.h
> index 22d72babdf..79e42ec297 100644
> --- a/include/system/os-win32.h
> +++ b/include/system/os-win32.h
> @@ -103,6 +103,7 @@ static inline void os_setup_post(void) {}
>  static inline void os_set_proc_name(const char *dummy) {}
>  void os_set_line_buffering(void);
>  void os_setup_early_signal_handling(void);
> +static inline void os_set_socket_permissions(const char *dummy) {};
>  
>  int getpagesize(void);
>  
> diff --git a/os-posix.c b/os-posix.c
> index 52925c23d3..bbd17ff2b9 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -94,6 +94,18 @@ static struct passwd *user_pwd;    /*   NULL   non-NULL   NULL   */
>  static uid_t user_uid = (uid_t)-1; /*   -1      -1        >=0    */
>  static gid_t user_gid = (gid_t)-1; /*   -1      -1        >=0    */
>  
> +void os_set_socket_permissions(const char *path)
> +{
> +    uid_t uid = user_pwd ? user_pwd->pw_uid : user_uid;
> +    gid_t gid = user_pwd ? user_pwd->pw_gid : user_gid;
> +
> +    if (chown(path, uid, gid) < 0) {
> +        error_report("Failed to chown socket %s: %s", path, strerror(errno));
> +        exit(1);
> +    }
> +}
> +
> +
>  /*
>   * Prepare to change user ID. user_id can be one of 3 forms:
>   *   - a username, in which case user ID will be changed to its uid,
> diff --git a/stubs/meson.build b/stubs/meson.build
> index 0b2778c568..4a4342344b 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -24,6 +24,7 @@ if have_block
>    stub_ss.add(files('ram-block.c'))
>    stub_ss.add(files('runstate-check.c'))
>    stub_ss.add(files('uuid.c'))
> +  stub_ss.add(files('os-file.c'))
>  endif
>  
>  if have_block or have_ga
> diff --git a/stubs/os-file.c b/stubs/os-file.c
> new file mode 100644
> index 0000000000..c32cbc7efa
> --- /dev/null
> +++ b/stubs/os-file.c
> @@ -0,0 +1,6 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#include "qemu/osdep.h"
> +
> +void os_set_socket_permissions(const char *dummy)
> +{
> +}
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 4773755fd5..77b2d9287b 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1026,6 +1026,12 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
>          error_setg_errno(errp, errno, "Failed to bind socket to %s", path);
>          goto err;
>      }
> +    /*
> +     * Change the permissions on the socket to match the permission of the
> +     * counterparty process
> +     */
> +    os_set_socket_permissions(un.sun_path);

QEMU's -run-with described user= as:

  user=username or user=uid:gid can be used to drop root privileges before
  starting guest execution. QEMU will use the setuid and setgid system
  calls to switch to the specified identity...

So it explicitly mentioned "before starting guest execution", hence at
least from that doc it's hard to say if unix socket should be created with
the privilege or after dropped..  Here I believe cpr socket should be the
case for latter, however when it's a generic change to unix listening
ports, I wonder if there's other unwanted side effects.

Considering unix socket itself doesn't really have a UID attached to it,
it's only the unix path that needs a chmod(), meanwhile the mgmt of course
knows both the right UID (as specified in -run-with) and the path, would it
make sense if the mgmt chmod() after it starts dest QEMU?  That'll reduce
the scope of impact to minimum.

Thanks,

> +
>      if (listen(sock, num) < 0) {
>          error_setg_errno(errp, errno, "Failed to listen on socket");
>          goto err;
> -- 
> 2.34.1
> 

-- 
Peter Xu

Re: [PATCH] migration: cpr socket permissions fix

[Daniel P. Berrangé]

On Fri, Nov 21, 2025 at 11:07:48AM -0500, Peter Xu wrote:
> On Thu, Nov 20, 2025 at 01:57:33PM -0500, Ben Chaney wrote:
> > Fix an issue where cpr transfer fails when running with the
> > -run-with user=$USERID with a permission denied error. This issue
> > occurs because the destination host creates the transfer sockets before
> > it drops permissions while the source host tries to connect after
> > dropping permissions. Fix this by changing the ownership of the cpr
> > socket to the lower level so it is accessible to both parties.
> > 
> > Resolves: https://lore.kernel.org/all/3D32B62F-29E2-4470-86A5-9A2B3B29E371@akamai.com/
> > Signed-off-by: Ben Chaney <bchaney@akamai.com>
> > ---
> >  include/system/os-posix.h |  1 +
> >  include/system/os-wasm.h  |  1 +
> >  include/system/os-win32.h |  1 +
> >  os-posix.c                | 12 ++++++++++++
> >  stubs/meson.build         |  1 +
> >  stubs/os-file.c           |  6 ++++++
> >  util/qemu-sockets.c       |  6 ++++++
> >  7 files changed, 28 insertions(+)
> >  create mode 100644 stubs/os-file.c
> > 
> > diff --git a/include/system/os-posix.h b/include/system/os-posix.h
> > index ce5b3bccf8..e4b69fc316 100644
> > --- a/include/system/os-posix.h
> > +++ b/include/system/os-posix.h
> > @@ -54,6 +54,7 @@ void os_set_chroot(const char *path);
> >  void os_setup_limits(void);
> >  void os_setup_post(void);
> >  int os_mlock(bool on_fault);
> > +void os_set_socket_permissions(const char *path);
> >  
> >  /**
> >   * qemu_alloc_stack:
> > diff --git a/include/system/os-wasm.h b/include/system/os-wasm.h
> > index 3abb3aaa03..eeac092183 100644
> > --- a/include/system/os-wasm.h
> > +++ b/include/system/os-wasm.h
> > @@ -57,6 +57,7 @@ static inline int os_set_daemonize(bool d)
> >  };
> >  bool is_daemonized(void);
> >  static inline void os_daemonize(void) {}
> > +static inline void os_set_socket_permissions(const char *dummy) {};
> >  
> >  /**
> >   * qemu_alloc_stack:
> > diff --git a/include/system/os-win32.h b/include/system/os-win32.h
> > index 22d72babdf..79e42ec297 100644
> > --- a/include/system/os-win32.h
> > +++ b/include/system/os-win32.h
> > @@ -103,6 +103,7 @@ static inline void os_setup_post(void) {}
> >  static inline void os_set_proc_name(const char *dummy) {}
> >  void os_set_line_buffering(void);
> >  void os_setup_early_signal_handling(void);
> > +static inline void os_set_socket_permissions(const char *dummy) {};
> >  
> >  int getpagesize(void);
> >  
> > diff --git a/os-posix.c b/os-posix.c
> > index 52925c23d3..bbd17ff2b9 100644
> > --- a/os-posix.c
> > +++ b/os-posix.c
> > @@ -94,6 +94,18 @@ static struct passwd *user_pwd;    /*   NULL   non-NULL   NULL   */
> >  static uid_t user_uid = (uid_t)-1; /*   -1      -1        >=0    */
> >  static gid_t user_gid = (gid_t)-1; /*   -1      -1        >=0    */
> >  
> > +void os_set_socket_permissions(const char *path)
> > +{
> > +    uid_t uid = user_pwd ? user_pwd->pw_uid : user_uid;
> > +    gid_t gid = user_pwd ? user_pwd->pw_gid : user_gid;
> > +
> > +    if (chown(path, uid, gid) < 0) {
> > +        error_report("Failed to chown socket %s: %s", path, strerror(errno));
> > +        exit(1);
> > +    }
> > +}
> > +
> > +
> >  /*
> >   * Prepare to change user ID. user_id can be one of 3 forms:
> >   *   - a username, in which case user ID will be changed to its uid,
> > diff --git a/stubs/meson.build b/stubs/meson.build
> > index 0b2778c568..4a4342344b 100644
> > --- a/stubs/meson.build
> > +++ b/stubs/meson.build
> > @@ -24,6 +24,7 @@ if have_block
> >    stub_ss.add(files('ram-block.c'))
> >    stub_ss.add(files('runstate-check.c'))
> >    stub_ss.add(files('uuid.c'))
> > +  stub_ss.add(files('os-file.c'))
> >  endif
> >  
> >  if have_block or have_ga
> > diff --git a/stubs/os-file.c b/stubs/os-file.c
> > new file mode 100644
> > index 0000000000..c32cbc7efa
> > --- /dev/null
> > +++ b/stubs/os-file.c
> > @@ -0,0 +1,6 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +#include "qemu/osdep.h"
> > +
> > +void os_set_socket_permissions(const char *dummy)
> > +{
> > +}
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > index 4773755fd5..77b2d9287b 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -1026,6 +1026,12 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
> >          error_setg_errno(errp, errno, "Failed to bind socket to %s", path);
> >          goto err;
> >      }
> > +    /*
> > +     * Change the permissions on the socket to match the permission of the
> > +     * counterparty process
> > +     */
> > +    os_set_socket_permissions(un.sun_path);
> 
> QEMU's -run-with described user= as:
> 
>   user=username or user=uid:gid can be used to drop root privileges before
>   starting guest execution. QEMU will use the setuid and setgid system
>   calls to switch to the specified identity...
> 
> So it explicitly mentioned "before starting guest execution", hence at
> least from that doc it's hard to say if unix socket should be created with
> the privilege or after dropped..  Here I believe cpr socket should be the
> case for latter, however when it's a generic change to unix listening
> ports, I wonder if there's other unwanted side effects.
> 
> Considering unix socket itself doesn't really have a UID attached to it,
> it's only the unix path that needs a chmod(), meanwhile the mgmt of course
> knows both the right UID (as specified in -run-with) and the path, would it
> make sense if the mgmt chmod() after it starts dest QEMU?  That'll reduce
> the scope of impact to minimum.

My guiding principal is that the behaviour of cold-plugging a backend
on the CLI and hot-plugging a backend via QMP should be the same, since
we want to eventually get to a point where everything can be configured
via QMP and zero CLI except for basic process related things.

hot-plugging will honour -run-with setting, therefore cold-plugging
should do the same.

Having said that, the run-with setting impl is very clearcut however
that it is intended to run *after* all cold-plug setup is complete.
IOW, it is inherantly intending to have differnt behaviour for cold
plug vs hotplug.

There's a very real risk we'll cause regressions if we change how
permissions are handled implicitly on any backends.

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