On Mon, Feb 03, 2020 at 07:36:09PM +0100, Ján Tomko wrote:
[adding virtiofs list]
On Mon, Feb 03, 2020 at 04:43:51PM +0000, Daniel P. Berrangé wrote:
On Thu, Jan 30, 2020 at 06:06:26PM +0100, Ján Tomko wrote:
Start virtiofsd for each <filesystem> device using it.
Pre-create the socket for communication with QEMU and pass it to virtiofsd.
Note that virtiofsd needs to run as root.
So we're not able to use virtiofsd with the session libvirtd which runs completely unprivileged ?
Not with the version of virtiofsd currently merged in the QEMU tree.
I can understand the need to run as root if we want to support chown() of files, or DAC_OVERRIDE, but I'm surprised it isn't possible to run virtiofsd unprivileged & simply have a reduced featureset where it can only create files as that one user.
Apart from the possibly missing features (I don't know how well virtiofsd internals are ready for those), current version of the daemon sets up namespaces and the seccomp sandbox.
+int +qemuVirtioFSStart(virLogManagerPtr logManager, + virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainFSDefPtr fs) +{ + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virCommand) cmd = NULL; + g_autofree char *socket_path = NULL; + g_autofree char *pidfile = NULL; + g_autofree char *logpath = NULL; + pid_t pid = (pid_t) -1; + VIR_AUTOCLOSE fd = -1; + VIR_AUTOCLOSE logfd = -1; + int ret = -1; + int rc;
+ + if (!(pidfile = qemuVirtioFSCreatePidFilename(cfg, vm->def, fs->info.alias))) + goto cleanup; + + if (!(socket_path = qemuVirtioFSCreateSocketFilename(vm, fs->info.alias))) + goto cleanup; + + if ((fd = qemuVirtioFSOpenChardev(driver, vm, socket_path)) < 0) + goto cleanup; + + logpath = qemuVirtioFSCreateLogFilename(cfg, vm->def, fs->info.alias); + + if (cfg->stdioLogD) { + if ((logfd = virLogManagerDomainOpenLogFile(logManager, + "qemu", + vm->def->uuid, + vm->def->name, + logpath, + 0, + NULL, NULL)) < 0) + goto cleanup; + } else { + if ((logfd = open(logpath, O_WRONLY | O_CREAT | O_APPEND, S_IRUSR | S_IWUSR)) < 0) { + virReportSystemError(errno, _("failed to create logfile %s"), + logpath); + goto cleanup; + } + if (virSetCloseExec(logfd) < 0) { + virReportSystemError(errno, _("failed to set close-on-exec flag on %s"), + logpath); + goto error; + } + } + + if (!(cmd = qemuVirtioFSBuildCommandLine(cfg, fs, &fd))) + goto cleanup; + + virCommandSetPidFile(cmd, pidfile); + virCommandSetOutputFD(cmd, &logfd); + virCommandSetErrorFD(cmd, &logfd); + virCommandNonblockingFDs(cmd); + virCommandDaemonize(cmd);
We're not mandating "root" here, it is just inheriting the user that libvirtd runs as. So IIUC ,this will run virtofsd as non-root when used with session libvirtd, unless there's a check somewhere else that prevents this scenario ?
I'll add a check.
I'm also wondering about cgroups placement in this method, and any use of SELinux
Placing it into a cgroup should be easy, AFAIK it does not need to access any devices.
As for SELinux, I don't think there's anything to be done other than updating the selinux-policy. Recursively relabeling the whole directory feels intrusive.
Even if we don't do relabelling, we'll still need more than just policy work I believe. If we assume a new "virtiofsd_t" SELinux type. Ff QEMU is launched svirt_t:s0:c123,c532, we will need to explicitly spawn virtiofsd with the matching MCS category eg virtiofsd_t:s0:c123,c532. The policy should be written such that the UNIX domain socket used for comms between QEMU and virtiofsd requires this matching MCS category label. This ensures that QEMU Guest-A can't connect to a virtiofsd used by QEMU Guest-B and vica-verca. 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 :|