Re: [PATCH] migration: cpr socket permissions fix

[Peter Xu <peterx@redhat.com>]

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