On Mon, Oct 01, 2018 at 10:22:59AM +0200, Erik Skultety wrote:
On Thu, Sep 27, 2018 at 05:02:13PM +0200, Ján Tomko wrote:We switched to opening mode='bind' sockets ourselves: commit 30fb2276d88b275dc2aad6ddd28c100d944b59a5 qemu: support passing pre-opened UNIX socket listen FD in v4.5.0-rc1~251 Then fixed qemuBuildChrChardevStr to change libvirtd's label while creating the socket: commit b0c6300fc42bbc3e5eb0b236392f7344581c5810 qemu: ensure FDs passed to QEMU for chardevs have correct SELinux labels v4.5.0-rc1~52 Also add labeling of these sockets to the DAC driver. Instead of trying to figure out which one was created by libvirt, label it if it exists. https://bugzilla.redhat.com/show_bug.cgi?id=1633389 Signed-off-by: Ján Tomko <jtomko@xxxxxxxxxx> --- src/security/security_dac.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 62442745dd..da4a6c72fe 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1308,7 +1308,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, break; case VIR_DOMAIN_CHR_TYPE_UNIX: - if (!dev_source->data.nix.listen) { + if (!dev_source->data.nix.listen || + (dev_source->data.nix.path && + virFileExists(dev_source->data.nix.path))) { + /* Also label mode='bind' sockets if they exist, + * e.g. because they were created by libvirt + * and passed via FD */ if (virSecurityDACSetOwnership(mgr, NULL, dev_source->data.nix.path, user, group) < 0)So we're labeling path even if nix.listen == false, shouldn't we check for the file's existence too? Or have we already done it and I missed this fact?
For mode='connect', the path not existing is fatal, so we can safely call virSecurityDACSetOwnership and let it fail. For mode='bind', the path not existing is valid for QEMUs which do not support FD passing for chardevs. If QEMU supports FD passing, the path should exist because libvirt created it earlier. I could not think of a nicer idea on how to pass down the logic that decides whether libvirt pre-creates it in qemuBuildChrChardevStr (which, of course, is an odd place to be creating sockets), so I opted to use the file's existence as a witness of libvirt pre-creating it. Which is what I tried to say by this commit message snippet:
Instead of trying to figure out which one was created by libvirt, label it if it exists.
Shall I reword the message? E.g.: Instead of duplicating the logic which decides whether libvirt should pre-create the socket, assume an existing path means it was created by libvirt.
Basically what I'm aiming at is that nix.path will always be set at this point,
Yes, the dev_source->data.nix.path condition might be superfluous. I was being overly cautious. As evidenced by: commit 614193fac67445a7e92bf620ffef726ed1bd6f07 conf: Fix check for chardev source path An empty path should be caught by our validation.
either explicitly by the user (most cases) or it would have been generated by us if the target type was either VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN or VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN.
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN | VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN is still VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN. ;)
I'm just simply wondering whether the condition cannot be further simplified to: if (virFileExists(foo) && virSecurityDACSetOwnership(...) < 0)
This introduces a change in behavior for mode='connect' - the DAC driver will no longer report an error for a non-existent path. I have not checked whether it's checked anywhere else (possibly the SELinux driver), but it is a legitimate error and I don't see the need to skip it. Moreover, this hides the reason for using virFileExists in the first place - it's only needed for type='listen' sockets. Hope that answers your questions. Looking forward to hearing from you. Jano
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
