10:32 p.m.
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
10:32 p.m.
New subject: [libvirt] [PATCH 2/2] selinux: remove unused variables in socket labelling
---
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
10:45 p.m.
New subject: [libvirt] [PATCH 2/2] selinux: remove unused variables in socket labelling
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:57 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:52 p.m.
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
12:37 a.m.
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
3:30 a.m.
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 :|
6:26 a.m.
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
10:39 a.m.
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.
11:12 a.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
4420
days inactive
4420
days old
9 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Guannan Ren -
Laine Stump