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

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(a)redhat.com&gt; > --- > 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?