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