[libvirt] [PATCH 1/2] selinux: fix wrong tapfd relablling
It should relabel tapfd of virtual network of type VIR_DOMAIN_NET_TYPE_DIRECT rather than VIR_DOMAIN_NET_TYPE_NETWORK and VIR_DOMAIN_NET_TYPE_BRIDGE (commit ae368ebfcc4923d0b32e83d4ca96a6f599625785 introduced this bug) --- src/qemu/qemu_command.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 239592c..0c0c400 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5412,10 +5412,6 @@ qemuBuildCommandLine(virConnectPtr conn, if (tapfd < 0) goto error; - if (virSecurityManagerSetTapFDLabel(driver->securityManager, - def, tapfd) < 0) - goto error; - last_good_net = i; virCommandTransferFD(cmd, tapfd); @@ -5429,6 +5425,10 @@ qemuBuildCommandLine(virConnectPtr conn, if (tapfd < 0) goto error; + if (virSecurityManagerSetTapFDLabel(driver->securityManager, + def, tapfd) < 0) + goto error; + last_good_net = i; virCommandTransferFD(cmd, tapfd); -- 1.7.11.2
--- src/security/security_selinux.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 85419ba..b5108ab 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1634,8 +1634,6 @@ virSecuritySELinuxSetSecurityDaemonSocketLabel(virSecurityManagerPtr mgr, { /* TODO: verify DOI */ virSecurityLabelDefPtr secdef; - context_t execcon = NULL; - context_t proccon = NULL; security_context_t scon = NULL; char *str = NULL; int rc = -1; @@ -1678,8 +1676,6 @@ done: if (security_getenforce() != 1) rc = 0; - if (execcon) context_free(execcon); - if (proccon) context_free(proccon); freecon(scon); VIR_FREE(str); return rc; -- 1.7.11.2
On 10/16/2012 09:32 PM, Guannan Ren wrote:
--- src/security/security_selinux.c | 4 ---- 1 file changed, 4 deletions(-)
ACK. [Meanwhile, I'm still testing 1/2 to see if it fixes the problem I ran into today...]
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 85419ba..b5108ab 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1634,8 +1634,6 @@ virSecuritySELinuxSetSecurityDaemonSocketLabel(virSecurityManagerPtr mgr, { /* TODO: verify DOI */ virSecurityLabelDefPtr secdef; - context_t execcon = NULL; - context_t proccon = NULL; security_context_t scon = NULL; char *str = NULL; int rc = -1; @@ -1678,8 +1676,6 @@ done:
if (security_getenforce() != 1) rc = 0; - if (execcon) context_free(execcon); - if (proccon) context_free(proccon); freecon(scon); VIR_FREE(str); return rc;
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/16/2012 09:32 PM, Guannan Ren wrote: In the subject: s/relablling/relabeling/
It should relabel tapfd of virtual network of type VIR_DOMAIN_NET_TYPE_DIRECT rather than VIR_DOMAIN_NET_TYPE_NETWORK and VIR_DOMAIN_NET_TYPE_BRIDGE (commit ae368ebfcc4923d0b32e83d4ca96a6f599625785 introduced this bug) --- src/qemu/qemu_command.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Tested, and fixed the breakage I saw earlier today when running with SELinux enabled. ACK. However, I might add a word of caution in the commit message alerting anyone trying to do a backport of this patch to be extremely cautious of where the patch gets applied, as the context of the two hunks is identical other than indentation. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/17/2012 11:57 AM, Eric Blake wrote:
On 10/16/2012 09:32 PM, Guannan Ren wrote:
In the subject: s/relablling/relabeling/
It should relabel tapfd of virtual network of type VIR_DOMAIN_NET_TYPE_DIRECT rather than VIR_DOMAIN_NET_TYPE_NETWORK and VIR_DOMAIN_NET_TYPE_BRIDGE (commit ae368ebfcc4923d0b32e83d4ca96a6f599625785 introduced this bug) --- src/qemu/qemu_command.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) Tested, and fixed the breakage I saw earlier today when running with SELinux enabled.
ACK.
However, I might add a word of caution in the commit message alerting anyone trying to do a backport of this patch to be extremely cautious of where the patch gets applied, as the context of the two hunks is identical other than indentation.
Okay, I will add the caution when it is pushed. Thanks for the review in so late night. Guannan
On 10/17/2012 11:32 AM, Guannan Ren wrote:
It should relabel tapfd of virtual network of type VIR_DOMAIN_NET_TYPE_DIRECT rather than VIR_DOMAIN_NET_TYPE_NETWORK and VIR_DOMAIN_NET_TYPE_BRIDGE (commit ae368ebfcc4923d0b32e83d4ca96a6f599625785 introduced this bug) --- src/qemu/qemu_command.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 239592c..0c0c400 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5412,10 +5412,6 @@ qemuBuildCommandLine(virConnectPtr conn, if (tapfd < 0) goto error;
- if (virSecurityManagerSetTapFDLabel(driver->securityManager, - def, tapfd) < 0) - goto error; - last_good_net = i; virCommandTransferFD(cmd, tapfd);
@@ -5429,6 +5425,10 @@ qemuBuildCommandLine(virConnectPtr conn, if (tapfd < 0) goto error;
+ if (virSecurityManagerSetTapFDLabel(driver->securityManager, + def, tapfd) < 0) + goto error; + last_good_net = i; virCommandTransferFD(cmd, tapfd);
The two patches are pushed. Guannan
On Wed, Oct 17, 2012 at 11:32:45AM +0800, Guannan Ren wrote:
It should relabel tapfd of virtual network of type VIR_DOMAIN_NET_TYPE_DIRECT rather than VIR_DOMAIN_NET_TYPE_NETWORK and VIR_DOMAIN_NET_TYPE_BRIDGE (commit ae368ebfcc4923d0b32e83d4ca96a6f599625785 introduced this bug)
Why ? IMHO, if we're going to trouble of relabelling TAP file descriptors with a MCS category, we should relabel *all* of them. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/17/2012 04:30 PM, Daniel P. Berrange wrote:
On Wed, Oct 17, 2012 at 11:32:45AM +0800, Guannan Ren wrote:
It should relabel tapfd of virtual network of type VIR_DOMAIN_NET_TYPE_DIRECT rather than VIR_DOMAIN_NET_TYPE_NETWORK and VIR_DOMAIN_NET_TYPE_BRIDGE (commit ae368ebfcc4923d0b32e83d4ca96a6f599625785 introduced this bug) Why ? IMHO, if we're going to trouble of relabelling TAP file descriptors with a MCS category, we should relabel *all* of them.
Daniel
Eric met a such AVC denied in this case where /dev/net/tun is relabelled with MCS. "libvirtd would now be assigning a label to /dev/net/tun, which makes it impossible for anyone else (like openvpn) to also open a tun device." type=AVC msg=audit(1350411375.542:759325): avc: denied { read } for pid=4773 comm="openvpn" path="/dev/net/tun" dev=devtmpfs ino=9557 scontext=system_u:system_r:openvpn_t:s0 tcontext=system_u:object_r:tun_tap_device_t:s0:c1,c489 tclass=chr_file
On 10/16/2012 11:32 PM, Guannan Ren wrote:
It should relabel tapfd of virtual network of type VIR_DOMAIN_NET_TYPE_DIRECT rather than VIR_DOMAIN_NET_TYPE_NETWORK and VIR_DOMAIN_NET_TYPE_BRIDGE (commit ae368ebfcc4923d0b32e83d4ca96a6f599625785 introduced this bug) --- src/qemu/qemu_command.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 239592c..0c0c400 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5412,10 +5412,6 @@ qemuBuildCommandLine(virConnectPtr conn, if (tapfd < 0) goto error;
- if (virSecurityManagerSetTapFDLabel(driver->securityManager, - def, tapfd) < 0) - goto error; - last_good_net = i; virCommandTransferFD(cmd, tapfd);
@@ -5429,6 +5425,10 @@ qemuBuildCommandLine(virConnectPtr conn, if (tapfd < 0) goto error;
+ if (virSecurityManagerSetTapFDLabel(driver->securityManager, + def, tapfd) < 0) + goto error; + last_good_net = i; virCommandTransferFD(cmd, tapfd);
(Sigh, I just have too much mail to read...) I unfortunately missed this second patch yesterday, and also missed the incorrect placement of the label change even in the original patch series. Aside from what Dan said about relabelling *all* tap devices (the fact that it happens to work for standard tap devices without any relabelling now just means that currently the race is always being won, not that there isn't a problem), also the call to relabel shouldn't be in qemuBuildCommandline - that overlooks device hotplug. Instead, you should be calling virSecurityManagerSetTapFDLabel from qemuNetworkIfaceConnect and qemuPhysIfaceConnect right after the tap device is created. Those are common functions called both for static netdevs and for hotplug netdevs.
On 10/17/2012 11:39 PM, Laine Stump wrote:
On 10/16/2012 11:32 PM, Guannan Ren wrote:
It should relabel tapfd of virtual network of type VIR_DOMAIN_NET_TYPE_DIRECT rather than VIR_DOMAIN_NET_TYPE_NETWORK and VIR_DOMAIN_NET_TYPE_BRIDGE (commit ae368ebfcc4923d0b32e83d4ca96a6f599625785 introduced this bug) --- src/qemu/qemu_command.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 239592c..0c0c400 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5412,10 +5412,6 @@ qemuBuildCommandLine(virConnectPtr conn, if (tapfd < 0) goto error;
- if (virSecurityManagerSetTapFDLabel(driver->securityManager, - def, tapfd) < 0) - goto error; - last_good_net = i; virCommandTransferFD(cmd, tapfd);
@@ -5429,6 +5425,10 @@ qemuBuildCommandLine(virConnectPtr conn, if (tapfd < 0) goto error;
+ if (virSecurityManagerSetTapFDLabel(driver->securityManager, + def, tapfd) < 0) + goto error; + last_good_net = i; virCommandTransferFD(cmd, tapfd);
(Sigh, I just have too much mail to read...)
I unfortunately missed this second patch yesterday, and also missed the incorrect placement of the label change even in the original patch series.
Aside from what Dan said about relabelling *all* tap devices (the fact that it happens to work for standard tap devices without any relabelling now just means that currently the race is always being won, not that there isn't a problem),
Agree, but there is a MCS label discussion on this, If we relabel *all* tap device, we shouldn't use MCS anyway.
also the call to relabel shouldn't be in qemuBuildCommandline - that overlooks device hotplug.
Instead, you should be calling virSecurityManagerSetTapFDLabel from qemuNetworkIfaceConnect and qemuPhysIfaceConnect right after the tap device is created. Those are common functions called both for static netdevs and for hotplug netdevs.
Agree, will do it.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Guannan Ren -
Laine Stump