On Mon, Jan 29, 2024 at 7:53 PM Daniel P. Berrangé <berra...@redhat.com> wrote: > > diff --git a/meson.build b/meson.build > > index d0329966f1b4..93fc233b0891 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -4015,6 +4015,11 @@ if have_tools > > dependencies: [authz, crypto, io, qom, qemuutil, > > libcap_ng, mpathpersist], > > install: true) > > + > > + executable('qemu-vmsr-helper', files('tools/i386/qemu-vmsr-helper.c'), > > I'd suggest 'tools/x86/' since this works fine on 64-bit too
QEMU tends to use i386 in the source to mean both 32- and 64-bit. Either is fine by me though. > > + dependencies: [authz, crypto, io, qom, qemuutil, > > + libcap_ng, mpathpersist], > > + install: true) > > Shouldn't this executable() call be conditional though, so this > is only built for x86 host targets. Yes. Also should be 32- and 64-bit (careful because Meson uses 'x86' for 32-bit). > > +static void compute_default_paths(void) > > +{ > > + socket_path = g_build_filename("/run", "qemu-vmsr-helper.sock", NULL); > > + pidfile = g_build_filename("/run", "qemu-vmsr-helper.pid", NULL); > > +} > > We shouldn't be hardcoding /run, we need to honour --prefix and > --localstatedir args given to configure. /var/run is a symlink > to /run so the end result ends up the same AFAIK Indeed; just copy from scsi/qemu-pr-helper.c. > You never answered my question from the previous posting of this > > This check is merely validating the the thread ID in the message > is a child of the process ID connected to the socket. Any process > on the entire host can satisfy this requirement. > > I don't see what is limiting this to only QEMU as claimed by the > commit message, unless you're expecting the UNIX socket permissions > to be such that only processes under the qemu:qemu user:group pair > can access to the socket ? That would be a libvirt based permissions > assumption though. Yes, this is why the systemd socket uses 600, like contrib/systemd/qemu-pr-helper.socket. The socket can be passed via SCM_RIGHTS by libvirt, or its permissions can be changed (e.g. 660 and root:kvm would make sense on a Debian system), or a separate helper can be started by libvirt. Either way, the policy is left to the user rather than embedding it in the provided systemd unit. > > + if (capng_update(CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED, > > + CAP_SYS_RAWIO) < 0) { > > + return -1; > > + } > > + > > + /* > > + * Change user/group id, retaining the capabilities. > > + * Because file descriptors are passed via SCM_RIGHTS, > > + * we don't need supplementary groups (and in fact the helper > > + * can run as "nobody"). > > + */ > > + if (capng_change_id(uid != -1 ? uid : getuid(), > > + gid != -1 ? gid : getgid(), > > + CAPNG_DROP_SUPP_GRP | CAPNG_CLEAR_BOUNDING)) { > > + return -1; > > + } > > Does this actually work ? IIUC, the file that requires privileges > is /dev/cpu/%u/msr, and we're opening that fresh on every request, > so how can this run as anything other than root ? Agreed, the capabilities can be dropped but the uid and gid cannot. Paolo