Re: [PATCH v3 2/3] tools: build qemu-vmsr-helper

[Paolo Bonzini]

On Mon, Jan 29, 2024 at 7:53 PM Daniel P. Berrangé <berrange@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