8:30 a.m.
Relabeling tapfd right after the tap device is created.
qemuPhysIfaceConnect is common function called both for static
netdevs and for hotplug netdevs.
---
src/qemu/qemu_command.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0c0c400..b24e9b1 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -170,6 +170,11 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
vmop, driver->stateDir,
virDomainNetGetActualBandwidth(net));
if (rc >= 0) {
+ if (virSecurityManagerSetTapFDLabel(driver->securityManager,
+ def, rc) < 0) {
+ VIR_FORCE_CLOSE(rc);
+ return -1;
+ }
virDomainAuditNetDevice(def, net, res_ifname, true);
VIR_FREE(net->ifname);
net->ifname = res_ifname;
@@ -5425,10 +5430,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);
--
1.7.11.4
8:55 a.m.
On 10/18/2012 07:30 AM, Guannan Ren wrote:
Relabeling tapfd right after the tap device is created.
qemuPhysIfaceConnect is common function called both for static
netdevs and for hotplug netdevs.
---
src/qemu/qemu_command.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0c0c400..b24e9b1 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -170,6 +170,11 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
vmop, driver->stateDir,
virDomainNetGetActualBandwidth(net));
if (rc >= 0) {
+ if (virSecurityManagerSetTapFDLabel(driver->securityManager,
+ def, rc) < 0) {
+ VIR_FORCE_CLOSE(rc);
+ return -1;
+ }
virDomainAuditNetDevice(def, net, res_ifname, true);
VIR_FREE(net->ifname);
Looks like you leak net->ifname on error, not to mention that you are
skipping out on an audit point. I think you need to fix up the logic
here to avoid leaks on failure to relabel the fd.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:21 a.m.
On 10/18/2012 09:55 PM, Eric Blake wrote:
On 10/18/2012 07:30 AM, Guannan Ren wrote:
> Relabeling tapfd right after the tap device is created.
> qemuPhysIfaceConnect is common function called both for static
> netdevs and for hotplug netdevs.
> ---
> src/qemu/qemu_command.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 0c0c400..b24e9b1 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -170,6 +170,11 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
> vmop, driver->stateDir,
> virDomainNetGetActualBandwidth(net));
> if (rc >= 0) {
> + if (virSecurityManagerSetTapFDLabel(driver->securityManager,
> + def, rc) < 0) {
> + VIR_FORCE_CLOSE(rc);
> + return -1;
> + }
> virDomainAuditNetDevice(def, net, res_ifname, true);
> VIR_FREE(net->ifname);
Looks like you leak net->ifname on error, not to mention that you are
skipping out on an audit point. I think you need to fix up the logic
here to avoid leaks on failure to relabel the fd.
In case of failure of relabel, I leaked char *res_ifname.
Shall we need to do the audit on failure of tapfd relabelling?
I don't think it is necessary to do it, is it?
11:40 a.m.
On 10/18/2012 08:21 AM, Guannan Ren wrote:
>> +++ b/src/qemu/qemu_command.c
>> @@ -170,6 +170,11 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
>> vmop, driver->stateDir,
>> virDomainNetGetActualBandwidth(net));
>> if (rc >= 0) {
>> + if (virSecurityManagerSetTapFDLabel(driver->securityManager,
>> + def, rc) < 0) {
>> + VIR_FORCE_CLOSE(rc);
>> + return -1;
>> + }
>> virDomainAuditNetDevice(def, net, res_ifname, true);
>> VIR_FREE(net->ifname);
> Looks like you leak net->ifname on error, not to mention that you are
> skipping out on an audit point. I think you need to fix up the logic
> here to avoid leaks on failure to relabel the fd.
>
In case of failure of relabel, I leaked char *res_ifname.
Shall we need to do the audit on failure of tapfd relabelling?
I don't think it is necessary to do it, is it?
Looking closer, this code currently only audits on success: if rc < 0,
then it skips the audit point [I'm not sure if it should also be
auditing on failure, but fixing that should be in a separate patch]. So
for this patch, I think you can also skip the audit point if the relabel
fails, because then you are treating rc as < 0; but you DO need to clean
up the memory that got allocated along the way.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:08 a.m.
New subject: [libvirt] [PATCH v2] selinux: relabel tapfd in qemuPhysIfaceConnect
Relabeling tapfd right after the tap device is created.
qemuPhysIfaceConnect is common function called both for static
netdevs and for hotplug netdevs.
---
src/qemu/qemu_command.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0c0c400..f727cb6 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -171,11 +171,21 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
virDomainNetGetActualBandwidth(net));
if (rc >= 0) {
virDomainAuditNetDevice(def, net, res_ifname, true);
+
+ if (virSecurityManagerSetTapFDLabel(driver->securityManager,
+ def, rc) < 0)
+ goto error;
+
VIR_FREE(net->ifname);
net->ifname = res_ifname;
}
return rc;
+
+error:
+ VIR_FREE(res_ifname);
+ VIR_FORCE_CLOSE(rc);
+ return -1;
}
@@ -5425,10 +5435,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);
--
1.7.11.4
11:42 a.m.
New subject: [libvirt] [PATCH v2] selinux: relabel tapfd in qemuPhysIfaceConnect
On 10/18/2012 09:08 AM, Guannan Ren wrote:
Relabeling tapfd right after the tap device is created.
qemuPhysIfaceConnect is common function called both for static
netdevs and for hotplug netdevs.
---
src/qemu/qemu_command.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0c0c400..f727cb6 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -171,11 +171,21 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
virDomainNetGetActualBandwidth(net));
if (rc >= 0) {
virDomainAuditNetDevice(def, net, res_ifname, true);
+
+ if (virSecurityManagerSetTapFDLabel(driver->securityManager,
+ def, rc) < 0)
+ goto error;
+
Hmm. Here, we now have a misleading audit, which says we successfully
opened a net device, even though due to the relabel we are not using
that device. I think you need to leave the successful audit point after
the relabel attempt.
VIR_FREE(net->ifname);
net->ifname = res_ifname;
}
return rc;
+
+error:
+ VIR_FREE(res_ifname);
+ VIR_FORCE_CLOSE(rc);
+ return -1;
}
@@ -5425,10 +5435,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);
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:05 p.m.
New subject: [libvirt] [PATCH v2] selinux: relabel tapfd in qemuPhysIfaceConnect
On 10/19/2012 12:42 AM, Eric Blake wrote:
On 10/18/2012 09:08 AM, Guannan Ren wrote:
> Relabeling tapfd right after the tap device is created.
> qemuPhysIfaceConnect is common function called both for static
> netdevs and for hotplug netdevs.
> ---
> src/qemu/qemu_command.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 0c0c400..f727cb6 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -171,11 +171,21 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
> virDomainNetGetActualBandwidth(net));
> if (rc >= 0) {
> virDomainAuditNetDevice(def, net, res_ifname, true);
> +
> + if (virSecurityManagerSetTapFDLabel(driver->securityManager,
> + def, rc) < 0)
> + goto error;
> +
Hmm. Here, we now have a misleading audit, which says we successfully
opened a net device, even though due to the relabel we are not using
that device. I think you need to leave the successful audit point after
the relabel attempt.
Thanks for the review.
v3 sent out :)
Guannan
4419
days inactive
4419
days old
6 comments
2 participants
participants (2)
-
Eric Blake -
Guannan Ren