[PATCH v2 1/3] qemu_passt: Don't make passt transition to svirt_t/libvirt_domain on start

[Daniel P. Berrangé]

On Thu, Feb 23, 2023 at 11:40:00AM +0100, Jiri Denemark wrote:
> On Wed, Feb 22, 2023 at 14:21:29 +0100, Stefano Brivio wrote:
> > qemuSecurityCommandRun() causes an explicit domain transition of the
> > new process, but passt ships with its own SELinux policy, with
> > external interfaces for libvirtd, so we simply need to transition
> > from virtd_t to passt_t as passt is executed. The qemu type
> > enforcement rules have little to do with it.
> > 
> > That is, if we switch to svirt_t, passt will run in the security
> > context that's intended for QEMU, which allows a number of
> > operations not needed by passt. On the other hand, with a switch
> > to svirt_t, passt won't be able to create its own PID file.
> > 
> > Usage of those new interfaces is implemented by this change in
> > selinux-policy:
> >   https://github.com/fedora-selinux/selinux-policy/pull/1613
> > 
> > Replace qemuSecurityCommandRun() with virCommandRun(), and explicitly
> > set the label, preserving the correct MCS range for the given VM
> > instance. This is a temporary measure: eventually, we'll need a more
> > generic and elegant mechanism for helper binaries.
> > 
> > Fixes: a56f0168d576 ("qemu: hook up passt config to qemu domains")
> > Signed-off-by: Stefano Brivio <sbrivio at redhat.com>
> > ---
> >  src/qemu/qemu_passt.c | 33 +++++++++++++++++++++++++++------
> >  1 file changed, 27 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> > index 1217a6a087..81f48dd630 100644
> > --- a/src/qemu/qemu_passt.c
> > +++ b/src/qemu/qemu_passt.c
> > @@ -30,6 +30,11 @@
> >  #include "virlog.h"
> >  #include "virpidfile.h"
> >  
> > +#ifdef WITH_SELINUX
> > +# include <selinux/selinux.h>
> > +# include <selinux/context.h>
> > +#endif
> > +
> >  #define VIR_FROM_THIS VIR_FROM_NONE
> >  
> >  VIR_LOG_INIT("qemu.passt");
> > @@ -158,8 +163,11 @@ qemuPasstStart(virDomainObj *vm,
> >      g_autofree char *errbuf = NULL;
> >      char macaddr[VIR_MAC_STRING_BUFLEN];
> >      size_t i;
> > -    int exitstatus = 0;
> > -    int cmdret = 0;
> > +#ifdef WITH_SELINUX
> > +    virSecurityLabelDef *seclabel;
> > +    context_t s;
> > +    const char *newLabel;
> > +#endif
> >  
> >      cmd = virCommandNew(PASST);
> >  
> > @@ -271,18 +279,31 @@ qemuPasstStart(virDomainObj *vm,
> >      if (qemuExtDeviceLogCommand(driver, vm, cmd, "passt") < 0)
> >          return -1;
> >  
> > -    if (qemuSecurityCommandRun(driver, vm, cmd, -1, -1, &exitstatus, &cmdret) < 0)
> > -        goto error;
> > +#ifdef WITH_SELINUX
> > +    /* TODO: Implement a new security manager function for helper binaries,
> > +     * possibly deriving domain names from security attributes of the helper
> > +     * binary itself.
> > +     */
> > +    seclabel = virDomainDefGetSecurityLabelDef(vm->def, "selinux");
> > +    s = context_new(seclabel->label);
> > +    context_type_set(s, "passt_t");
> > +    newLabel = context_str(s);
> > +
> > +    virCommandSetSELinuxLabel(cmd, newLabel);
> > +#endif
> >  
> > -    if (cmdret < 0 || exitstatus != 0) {
> > +    if (virCommandRun(cmd, NULL)) {
> >          virReportError(VIR_ERR_INTERNAL_ERROR,
> >                         _("Could not start 'passt': %s"), NULLSTR(errbuf));
> >          goto error;
> >      }
> >  
> > +    context_free(s);
> 
> This would need to be enclosed in #ifdef too.
> 
> > +
> >      return 0;
> >  
> >   error:
> > -    qemuPasstKill(pidfile);
> > +    context_free(s);
> 
> And here as well.
> 
> > +    qemuPasstKill(pidfile, passtSocketName);
> >      return -1;
> >  }
> 
> I have to admit I'm not sure whether a proper solution via security
> manager is feasible with a reasonable short time frame, i.e., ready to
> be pushed ideally just after the release as I feel it would be too much
> of a change for the freeze (but as I said, I may be wrong, I'm not
> familiar with the details of the security manager code).
> 
> This hacky approach has its downsides apart from the easily fixed nits
> above. Due to not going through the security manager layers, this code
> would try to use SELinux even if the driver is actually disabled in
> runtime (which can happen for various reasons).
> 
> So the question is, can a proper solution be implemented fast enough or
> we need some form of this hack to have this new functionality working?
> Michal, do you have any idea about the feasibility of providing a proper
> solution to this?

This really isn't difficult to do in the security manager IMHO. It is
just a variation on the existing virSecurityManagerSetChildProcessLabel
method, which instead of using the standard QEMU svirt_t labels, queries
the label by calling getfilecon on the binary to be execd and appending
the MCS label.

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 :|