[libvirt] [PATCHv3 0/2] more audit patches - audit network device fds
I hope this closes out my audit series. As promised in https://www.redhat.com/archives/libvir-list/2011-March/msg00415.html, here's the updated and tested network device auditing patches. This time, I've completed testing: in virt-manager, I attached a hypervisor default (non-virtio, so no /dev/vhost-net), then detached it, then attached a virtio interface in its place, and got the following audit messages: type=VIRT_RESOURCE msg=audit(1299702937.924:81114): user pid=499 uid=0 auid=500 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 msg='resrc=net reason=open vm="fedora_12" uuid=51c6fc83-65a4-e627-b698-042b00145201 net='52:54:00:80:C6:06' path="/dev/net/tun" rdev=0A:C8: exe="/home/dummy/libvirt/daemon/.libs/lt-libvirtd" hostname=? addr=? terminal=pts/0 res=success' type=VIRT_RESOURCE msg=audit(1299702937.928:81115): user pid=499 uid=0 auid=500 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 msg='resrc=net reason=attach vm="fedora_12" uuid=51c6fc83-65a4-e627-b698-042b00145201 old-net='?' new-net='52:54:00:80:C6:06': exe="/home/dummy/libvirt/daemon/.libs/lt-libvirtd" hostname=? addr=? terminal=pts/0 res=success' type=VIRT_RESOURCE msg=audit(1299702995.378:81117): user pid=499 uid=0 auid=500 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 msg='resrc=net reason=detach vm="fedora_12" uuid=51c6fc83-65a4-e627-b698-042b00145201 old-net='52:54:00:80:C6:06' new-net='?': exe="/home/dummy/libvirt/daemon/.libs/lt-libvirtd" hostname=? addr=? terminal=pts/0 res=success' type=VIRT_RESOURCE msg=audit(1299703012.919:81119): user pid=499 uid=0 auid=500 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 msg='resrc=net reason=open vm="fedora_12" uuid=51c6fc83-65a4-e627-b698-042b00145201 net='52:54:00:31:26:F9' path="/dev/net/tun" rdev=0A:C8: exe="/home/dummy/libvirt/daemon/.libs/lt-libvirtd" hostname=? addr=? terminal=pts/0 res=success' type=VIRT_RESOURCE msg=audit(1299703012.919:81120): user pid=499 uid=0 auid=500 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 msg='resrc=net reason=open vm="fedora_12" uuid=51c6fc83-65a4-e627-b698-042b00145201 net='52:54:00:31:26:F9' path="/dev/vhost-net" rdev=0A:39: exe="/home/dummy/libvirt/daemon/.libs/lt-libvirtd" hostname=? addr=? terminal=pts/0 res=success' type=VIRT_RESOURCE msg=audit(1299703013.002:81121): user pid=499 uid=0 auid=500 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 msg='resrc=net reason=attach vm="fedora_12" uuid=51c6fc83-65a4-e627-b698-042b00145201 old-net='?' new-net='52:54:00:31:26:F9': exe="/home/dummy/libvirt/daemon/.libs/lt-libvirtd" hostname=? addr=? terminal=pts/0 res=success' Changes in v3: rename the audit method to qemuAuditNetDevice, and insert audit points after all attempts to open a network device that might later be passed to a qemu -netdev; document why I didn't audit closeout of said fds Eric Blake (2): qemu: support vhost in attach-interface audit: audit use of /dev/net/tun, /dev/tapN, /dev/vhost-net src/qemu/qemu_audit.c | 41 ++++++++++++++++++++++++++++++++ src/qemu/qemu_audit.h | 5 ++++ src/qemu/qemu_command.c | 43 ++++++++++++++++----------------- src/qemu/qemu_command.h | 14 ++++++++--- src/qemu/qemu_hotplug.c | 60 ++++++++++++++++++++++++++++++++++++++++------ 5 files changed, 129 insertions(+), 34 deletions(-) -- 1.7.4
* src/qemu/qemu_hotplug.c (qemuDomainAttachNetDevice): Honor vhost designations, similar to qemu_command code paths. * src/qemu/qemu_command.h (qemuOpenVhostNet): New prototype. * src/qemu/qemu_command.c (qemuOpenVhostNet): Export. --- Hmm, I just realized that it might be nice to have a --driver-name flag in the virsh attach-interface wrapper command. Oh well, that can be a separate patch. src/qemu/qemu_command.c | 3 +- src/qemu/qemu_command.h | 4 +++ src/qemu/qemu_hotplug.c | 54 ++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 54 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 198a4e2..8cf1737 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -303,12 +303,11 @@ cleanup: } -static int +int qemuOpenVhostNet(virDomainNetDefPtr net, virBitmapPtr qemuCaps, int *vhostfd) { - *vhostfd = -1; /* assume we won't use vhost */ /* If the config says explicitly to not use vhost, return now */ diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1902472..2ae364c 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -129,6 +129,10 @@ int qemuPhysIfaceConnect(virConnectPtr conn, const unsigned char *vmuuid, enum virVMOperationType vmop); +int qemuOpenVhostNet(virDomainNetDefPtr net, + virBitmapPtr qemuCaps, + int *vhostfd); + int qemudCanonicalizeMachine(struct qemud_driver *driver, virDomainDefPtr def); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7895062..e8567ad 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -552,6 +552,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, qemuDomainObjPrivatePtr priv = vm->privateData; char *tapfd_name = NULL; int tapfd = -1; + char *vhostfd_name = NULL; + int vhostfd = -1; char *nicstr = NULL; char *netstr = NULL; int ret = -1; @@ -592,6 +594,24 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, return -1; } + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || + net->type == VIR_DOMAIN_NET_TYPE_BRIDGE || + net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { + /* Attempt to use vhost-net mode for these types of + network device */ + if (qemuOpenVhostNet(net, qemuCaps, &vhostfd) < 0) + goto cleanup; + + if (vhostfd >= 0 && + priv->monConfig->type != VIR_DOMAIN_CHR_TYPE_UNIX) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("network device type '%s' cannot be attached: " + "qemu is not using a unix socket monitor"), + virDomainNetTypeToString(net->type)); + goto cleanup; + } + } + if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets+1) < 0) goto no_memory; @@ -636,15 +656,32 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, } } - /* FIXME - need to support vhost-net here (5th arg) */ + if (vhostfd != -1) { + if (virAsprintf(&vhostfd_name, "vhostfd-%s", net->info.alias) < 0) + goto no_memory; + + qemuDomainObjEnterMonitorWithDriver(driver, vm); + if (qemuMonitorSendFileHandle(priv->mon, vhostfd_name, vhostfd) < 0) { + qemuDomainObjExitMonitorWithDriver(driver, vm); + goto try_tapfd_close; + } + qemuDomainObjExitMonitorWithDriver(driver, vm); + + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto cleanup; + } + } + if (qemuCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { if (!(netstr = qemuBuildHostNetStr(net, ',', - -1, tapfd_name, 0))) + -1, tapfd_name, vhostfd_name))) goto try_tapfd_close; } else { if (!(netstr = qemuBuildHostNetStr(net, ' ', - vlan, tapfd_name, 0))) + vlan, tapfd_name, vhostfd_name))) goto try_tapfd_close; } @@ -666,6 +703,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, qemuDomainObjExitMonitorWithDriver(driver, vm); VIR_FORCE_CLOSE(tapfd); + VIR_FORCE_CLOSE(vhostfd); if (!virDomainObjIsActive(vm)) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -720,6 +758,8 @@ cleanup: VIR_FREE(netstr); VIR_FREE(tapfd_name); VIR_FORCE_CLOSE(tapfd); + VIR_FREE(vhostfd_name); + VIR_FORCE_CLOSE(vhostfd); return ret; @@ -759,10 +799,14 @@ try_tapfd_close: if (!virDomainObjIsActive(vm)) goto cleanup; - if (tapfd_name) { + if (tapfd_name || vhostfd_name) { qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (qemuMonitorCloseFileHandle(priv->mon, tapfd_name) < 0) + if (tapfd_name && + qemuMonitorCloseFileHandle(priv->mon, tapfd_name) < 0) VIR_WARN("Failed to close tapfd with '%s'", tapfd_name); + if (vhostfd_name && + qemuMonitorCloseFileHandle(priv->mon, vhostfd_name) < 0) + VIR_WARN("Failed to close vhostfd with '%s'", vhostfd_name); qemuDomainObjExitMonitorWithDriver(driver, vm); } -- 1.7.4
On 03/09/2011 03:42 PM, Eric Blake wrote:
* src/qemu/qemu_hotplug.c (qemuDomainAttachNetDevice): Honor vhost designations, similar to qemu_command code paths. * src/qemu/qemu_command.h (qemuOpenVhostNet): New prototype. * src/qemu/qemu_command.c (qemuOpenVhostNet): Export. ---
Hmm, I just realized that it might be nice to have a --driver-name flag in the virsh attach-interface wrapper command. Oh well, that can be a separate patch.
src/qemu/qemu_command.c | 3 +- src/qemu/qemu_command.h | 4 +++ src/qemu/qemu_hotplug.c | 54 ++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 54 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 198a4e2..8cf1737 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -303,12 +303,11 @@ cleanup: }
-static int +int qemuOpenVhostNet(virDomainNetDefPtr net, virBitmapPtr qemuCaps, int *vhostfd) { - *vhostfd = -1; /* assume we won't use vhost */
/* If the config says explicitly to not use vhost, return now */ diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1902472..2ae364c 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -129,6 +129,10 @@ int qemuPhysIfaceConnect(virConnectPtr conn, const unsigned char *vmuuid, enum virVMOperationType vmop);
+int qemuOpenVhostNet(virDomainNetDefPtr net, + virBitmapPtr qemuCaps, + int *vhostfd); + int qemudCanonicalizeMachine(struct qemud_driver *driver, virDomainDefPtr def);
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7895062..e8567ad 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -552,6 +552,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, qemuDomainObjPrivatePtr priv = vm->privateData; char *tapfd_name = NULL; int tapfd = -1; + char *vhostfd_name = NULL; + int vhostfd = -1; char *nicstr = NULL; char *netstr = NULL; int ret = -1; @@ -592,6 +594,24 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, return -1; }
+ if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || + net->type == VIR_DOMAIN_NET_TYPE_BRIDGE || + net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { + /* Attempt to use vhost-net mode for these types of + network device */ + if (qemuOpenVhostNet(net, qemuCaps,&vhostfd)< 0) + goto cleanup; + + if (vhostfd>= 0&& + priv->monConfig->type != VIR_DOMAIN_CHR_TYPE_UNIX) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("network device type '%s' cannot be attached: " + "qemu is not using a unix socket monitor"), + virDomainNetTypeToString(net->type)); + goto cleanup; + }
The above check has already been handled by a combination of the checks in the if { } else if { } block above, so it's superfluous here. ACK with this extra check for monConfig-type removed.
On 03/09/2011 09:54 PM, Laine Stump wrote:
On 03/09/2011 03:42 PM, Eric Blake wrote:
* src/qemu/qemu_hotplug.c (qemuDomainAttachNetDevice): Honor vhost designations, similar to qemu_command code paths. * src/qemu/qemu_command.h (qemuOpenVhostNet): New prototype. * src/qemu/qemu_command.c (qemuOpenVhostNet): Export. ---
+ if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || + net->type == VIR_DOMAIN_NET_TYPE_BRIDGE || + net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { + /* Attempt to use vhost-net mode for these types of + network device */ + if (qemuOpenVhostNet(net, qemuCaps,&vhostfd)< 0) + goto cleanup; + + if (vhostfd>= 0&& + priv->monConfig->type != VIR_DOMAIN_CHR_TYPE_UNIX) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("network device type '%s' cannot be attached: " + "qemu is not using a unix socket monitor"), + virDomainNetTypeToString(net->type)); + goto cleanup; + }
The above check has already been handled by a combination of the checks in the if { } else if { } block above, so it's superfluous here.
ACK with this extra check for monConfig-type removed.
Indeed. I simplified as follows, then pushed. diff --git c/src/qemu/qemu_hotplug.c i/src/qemu/qemu_hotplug.c index e8567ad..540939a 100644 --- c/src/qemu/qemu_hotplug.c +++ i/src/qemu/qemu_hotplug.c @@ -578,6 +578,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if ((tapfd = qemuNetworkIfaceConnect(conn, driver, net, qemuCaps)) < 0) return -1; + if (qemuOpenVhostNet(net, qemuCaps, &vhostfd) < 0) + goto cleanup; } else if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { if (priv->monConfig->type != VIR_DOMAIN_CHR_TYPE_UNIX) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -592,24 +594,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, vm->def->uuid, VIR_VM_OP_CREATE)) < 0) return -1; - } - - if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || - net->type == VIR_DOMAIN_NET_TYPE_BRIDGE || - net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { - /* Attempt to use vhost-net mode for these types of - network device */ if (qemuOpenVhostNet(net, qemuCaps, &vhostfd) < 0) goto cleanup; - - if (vhostfd >= 0 && - priv->monConfig->type != VIR_DOMAIN_CHR_TYPE_UNIX) { - qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("network device type '%s' cannot be attached: " - "qemu is not using a unix socket monitor"), - virDomainNetTypeToString(net->type)); - goto cleanup; - } } if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets+1) < 0) -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Opening raw network devices with the intent of passing those fds to qemu is worth an audit point. This makes a multi-part audit: first, we audit the device(s) that libvirt opens on behalf of the MAC address of a to-be-created interface (which can independently succeed or fail), then we audit whether qemu actually started the network device with the same MAC (so searching backwards for successful audits with the same MAC will show which fd(s) qemu is actually using). Note that it is possible for the fd to be successfully opened but no attempt made to pass the fd to qemu (for example, because intermediate nwfilter operations failed) - no interface start audit will occur in that case; so the audit for a successful opened fd does not imply rights given to qemu unless there is a followup audit about the attempt to start a new interface. Likewise, when a network device is hot-unplugged, there is only one audit message about the MAC being discontinued; again, searching back to the earlier device open audits will show which fds that qemu quits using (and yes, I checked via /proc/<qemu-pid>/fd that qemu _does_ close out the fds associated with an interface on hot-unplug). The code would require much more refactoring to be able to definitively state which device(s) were discontinued at that point, since we currently don't record anywhere in the XML whether /dev/vhost-net was opened for a given interface. * src/qemu/qemu_audit.h (qemuAuditNetDevice): New prototype. * src/qemu/qemu_audit.c (qemuAuditNetDevice): New function. * src/qemu/qemu_command.h (qemuNetworkIfaceConnect) (qemuPhysIfaceConnect, qemuOpenVhostNet): Adjust prototype. * src/qemu/qemu_command.c (qemuNetworkIfaceConnect) (qemuPhysIfaceConnect, qemuOpenVhostNet): Add audit points and adjust parameters. (qemuBuildCommandLine): Adjust caller. * src/qemu/qemu_hotplug.c (qemuDomainAttachNetDevice): Likewise. --- src/qemu/qemu_audit.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_audit.h | 5 +++++ src/qemu/qemu_command.c | 40 ++++++++++++++++++++-------------------- src/qemu/qemu_command.h | 12 +++++++----- src/qemu/qemu_hotplug.c | 8 ++++---- 5 files changed, 77 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_audit.c b/src/qemu/qemu_audit.c index 5bdf655..40b68ff 100644 --- a/src/qemu/qemu_audit.c +++ b/src/qemu/qemu_audit.c @@ -127,6 +127,47 @@ qemuAuditNet(virDomainObjPtr vm, VIR_FREE(vmname); } +/** + * qemuAuditNetDevice: + * @vm: domain opening a network-related device + * @def: details of network device that fd will be tied to + * @device: device being opened (such as /dev/vhost-net, + * /dev/net/tun, /dev/tanN). Note that merely opening a device + * does not mean that qemu owns it; a followup qemuAuditNet + * shows whether the fd was passed on. + * @success: true if the device was opened + * + * Log an audit message about an attempted network device open. + */ +void +qemuAuditNetDevice(virDomainDefPtr vmDef, virDomainNetDefPtr netDef, + const char *device, bool success) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + char macstr[VIR_MAC_STRING_BUFLEN]; + char *vmname; + char *devname; + char *rdev; + + virUUIDFormat(vmDef->uuid, uuidstr); + virFormatMacAddr(netDef->mac, macstr); + rdev = qemuAuditGetRdev(device); + + if (!(vmname = virAuditEncode("vm", vmDef->name)) || + !(devname = virAuditEncode("path", device))) { + VIR_WARN0("OOM while encoding audit message"); + goto cleanup; + } + + VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, + "resrc=net reason=open %s uuid=%s net='%s' %s rdev=%s", + vmname, uuidstr, macstr, devname, VIR_AUDIT_STR(rdev)); + +cleanup: + VIR_FREE(vmname); + VIR_FREE(devname); + VIR_FREE(rdev); +} /** * qemuAuditHostdev: diff --git a/src/qemu/qemu_audit.h b/src/qemu/qemu_audit.h index a2fbe11..14c7da5 100644 --- a/src/qemu/qemu_audit.h +++ b/src/qemu/qemu_audit.h @@ -46,6 +46,11 @@ void qemuAuditNet(virDomainObjPtr vm, const char *reason, bool success) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); +void qemuAuditNetDevice(virDomainDefPtr vmDef, + virDomainNetDefPtr netDef, + const char *device, + bool success) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); void qemuAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr def, const char *reason, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8cf1737..0fc466c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -35,6 +35,7 @@ #include "uuid.h" #include "c-ctype.h" #include "domain_nwfilter.h" +#include "qemu_audit.h" #include <sys/utsname.h> #include <sys/stat.h> @@ -97,20 +98,20 @@ uname_normalize (struct utsname *ut) /** * qemuPhysIfaceConnect: + * @def: the definition of the VM (needed by 802.1Qbh and audit) * @conn: pointer to virConnect object * @driver: pointer to the qemud_driver * @net: pointer to he VM's interface description with direct device type * @qemuCaps: flags for qemu - * @vmuuid: The UUID of the VM (needed by 802.1Qbh) * * Returns a filedescriptor on success or -1 in case of error. */ int -qemuPhysIfaceConnect(virConnectPtr conn, +qemuPhysIfaceConnect(virDomainDefPtr def, + virConnectPtr conn, struct qemud_driver *driver, virDomainNetDefPtr net, virBitmapPtr qemuCaps, - const unsigned char *vmuuid, enum virVMOperationType vmop) { int rc; @@ -124,9 +125,10 @@ qemuPhysIfaceConnect(virConnectPtr conn, vnet_hdr = 1; rc = openMacvtapTap(net->ifname, net->mac, net->data.direct.linkdev, - net->data.direct.mode, vnet_hdr, vmuuid, + net->data.direct.mode, vnet_hdr, def->uuid, &net->data.direct.virtPortProfile, &res_ifname, vmop); + qemuAuditNetDevice(def, net, res_ifname, rc >= 0); if (rc >= 0) { VIR_FREE(net->ifname); net->ifname = res_ifname; @@ -152,11 +154,11 @@ qemuPhysIfaceConnect(virConnectPtr conn, } } #else + (void)def; (void)conn; (void)net; (void)qemuCaps; (void)driver; - (void)vmuuid; (void)vmop; qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No support for macvtap device")); @@ -167,7 +169,8 @@ qemuPhysIfaceConnect(virConnectPtr conn, int -qemuNetworkIfaceConnect(virConnectPtr conn, +qemuNetworkIfaceConnect(virDomainDefPtr def, + virConnectPtr conn, struct qemud_driver *driver, virDomainNetDefPtr net, virBitmapPtr qemuCaps) @@ -247,13 +250,10 @@ qemuNetworkIfaceConnect(virConnectPtr conn, memcpy(tapmac, net->mac, VIR_MAC_BUFLEN); tapmac[0] = 0xFE; /* Discourage bridge from using TAP dev MAC */ - if ((err = brAddTap(driver->brctl, - brname, - &net->ifname, - tapmac, - vnet_hdr, - true, - &tapfd))) { + err = brAddTap(driver->brctl, brname, &net->ifname, tapmac, + vnet_hdr, true, &tapfd); + qemuAuditNetDevice(def, net, "/dev/net/tun", tapfd >= 0); + if (err) { if (err == ENOTSUP) { /* In this particular case, give a better diagnostic. */ qemuReportError(VIR_ERR_INTERNAL_ERROR, @@ -304,7 +304,8 @@ cleanup: int -qemuOpenVhostNet(virDomainNetDefPtr net, +qemuOpenVhostNet(virDomainDefPtr def, + virDomainNetDefPtr net, virBitmapPtr qemuCaps, int *vhostfd) { @@ -342,6 +343,7 @@ qemuOpenVhostNet(virDomainNetDefPtr net, } *vhostfd = open("/dev/vhost-net", O_RDWR); + qemuAuditNetDevice(def, net, "/dev/vhost-net", *vhostfd >= 0); /* If the config says explicitly to use vhost and we couldn't open it, * report an error. @@ -3460,7 +3462,7 @@ qemuBuildCommandLine(virConnectPtr conn, if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { - int tapfd = qemuNetworkIfaceConnect(conn, driver, net, + int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, qemuCaps); if (tapfd < 0) goto error; @@ -3472,10 +3474,8 @@ qemuBuildCommandLine(virConnectPtr conn, tapfd) >= sizeof(tapfd_name)) goto no_memory; } else if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { - int tapfd = qemuPhysIfaceConnect(conn, driver, net, - qemuCaps, - def->uuid, - vmop); + int tapfd = qemuPhysIfaceConnect(def, conn, driver, net, + qemuCaps, vmop); if (tapfd < 0) goto error; @@ -3494,7 +3494,7 @@ qemuBuildCommandLine(virConnectPtr conn, network device */ int vhostfd; - if (qemuOpenVhostNet(net, qemuCaps, &vhostfd) < 0) + if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd) < 0) goto error; if (vhostfd >= 0) { virCommandTransferFD(cmd, vhostfd); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 2ae364c..528031d 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -116,20 +116,22 @@ char * qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev); -int qemuNetworkIfaceConnect(virConnectPtr conn, +int qemuNetworkIfaceConnect(virDomainDefPtr def, + virConnectPtr conn, struct qemud_driver *driver, virDomainNetDefPtr net, virBitmapPtr qemuCaps) - ATTRIBUTE_NONNULL(1); + ATTRIBUTE_NONNULL(2); -int qemuPhysIfaceConnect(virConnectPtr conn, +int qemuPhysIfaceConnect(virDomainDefPtr def, + virConnectPtr conn, struct qemud_driver *driver, virDomainNetDefPtr net, virBitmapPtr qemuCaps, - const unsigned char *vmuuid, enum virVMOperationType vmop); -int qemuOpenVhostNet(virDomainNetDefPtr net, +int qemuOpenVhostNet(virDomainDefPtr def, + virDomainNetDefPtr net, virBitmapPtr qemuCaps, int *vhostfd); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e8567ad..4646fd7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -576,7 +576,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, return -1; } - if ((tapfd = qemuNetworkIfaceConnect(conn, driver, net, qemuCaps)) < 0) + if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net, + qemuCaps)) < 0) return -1; } else if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { if (priv->monConfig->type != VIR_DOMAIN_CHR_TYPE_UNIX) { @@ -587,9 +588,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, return -1; } - if ((tapfd = qemuPhysIfaceConnect(conn, driver, net, + if ((tapfd = qemuPhysIfaceConnect(vm->def, conn, driver, net, qemuCaps, - vm->def->uuid, VIR_VM_OP_CREATE)) < 0) return -1; } @@ -599,7 +599,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { /* Attempt to use vhost-net mode for these types of network device */ - if (qemuOpenVhostNet(net, qemuCaps, &vhostfd) < 0) + if (qemuOpenVhostNet(vm->def, net, qemuCaps, &vhostfd) < 0) goto cleanup; if (vhostfd >= 0 && -- 1.7.4
On 03/09/2011 03:42 PM, Eric Blake wrote:
Opening raw network devices with the intent of passing those fds to qemu is worth an audit point. This makes a multi-part audit: first, we audit the device(s) that libvirt opens on behalf of the MAC address of a to-be-created interface (which can independently succeed or fail), then we audit whether qemu actually started the network device with the same MAC (so searching backwards for successful audits with the same MAC will show which fd(s) qemu is actually using). Note that it is possible for the fd to be successfully opened but no attempt made to pass the fd to qemu (for example, because intermediate nwfilter operations failed) - no interface start audit will occur in that case; so the audit for a successful opened fd does not imply rights given to qemu unless there is a followup audit about the attempt to start a new interface.
Likewise, when a network device is hot-unplugged, there is only one audit message about the MAC being discontinued; again, searching back to the earlier device open audits will show which fds that qemu quits using (and yes, I checked via /proc/<qemu-pid>/fd that qemu _does_ close out the fds associated with an interface on hot-unplug). The code would require much more refactoring to be able to definitively state which device(s) were discontinued at that point, since we currently don't record anywhere in the XML whether /dev/vhost-net was opened for a given interface.
* src/qemu/qemu_audit.h (qemuAuditNetDevice): New prototype. * src/qemu/qemu_audit.c (qemuAuditNetDevice): New function. * src/qemu/qemu_command.h (qemuNetworkIfaceConnect) (qemuPhysIfaceConnect, qemuOpenVhostNet): Adjust prototype. * src/qemu/qemu_command.c (qemuNetworkIfaceConnect) (qemuPhysIfaceConnect, qemuOpenVhostNet): Add audit points and adjust parameters. (qemuBuildCommandLine): Adjust caller. * src/qemu/qemu_hotplug.c (qemuDomainAttachNetDevice): Likewise. --- src/qemu/qemu_audit.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_audit.h | 5 +++++ src/qemu/qemu_command.c | 40 ++++++++++++++++++++-------------------- src/qemu/qemu_command.h | 12 +++++++----- src/qemu/qemu_hotplug.c | 8 ++++---- 5 files changed, 77 insertions(+), 29 deletions(-)
diff --git a/src/qemu/qemu_audit.c b/src/qemu/qemu_audit.c index 5bdf655..40b68ff 100644 --- a/src/qemu/qemu_audit.c +++ b/src/qemu/qemu_audit.c @@ -127,6 +127,47 @@ qemuAuditNet(virDomainObjPtr vm, VIR_FREE(vmname); }
+/** + * qemuAuditNetDevice: + * @vm: domain opening a network-related device + * @def: details of network device that fd will be tied to + * @device: device being opened (such as /dev/vhost-net, + * /dev/net/tun, /dev/tanN). Note that merely opening a device + * does not mean that qemu owns it; a followup qemuAuditNet + * shows whether the fd was passed on. + * @success: true if the device was opened + * + * Log an audit message about an attempted network device open. + */ +void +qemuAuditNetDevice(virDomainDefPtr vmDef, virDomainNetDefPtr netDef, + const char *device, bool success) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + char macstr[VIR_MAC_STRING_BUFLEN]; + char *vmname; + char *devname; + char *rdev; + + virUUIDFormat(vmDef->uuid, uuidstr); + virFormatMacAddr(netDef->mac, macstr); + rdev = qemuAuditGetRdev(device); + + if (!(vmname = virAuditEncode("vm", vmDef->name)) || + !(devname = virAuditEncode("path", device))) { + VIR_WARN0("OOM while encoding audit message"); + goto cleanup; + } + + VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, + "resrc=net reason=open %s uuid=%s net='%s' %s rdev=%s", + vmname, uuidstr, macstr, devname, VIR_AUDIT_STR(rdev)); + +cleanup: + VIR_FREE(vmname); + VIR_FREE(devname); + VIR_FREE(rdev); +}
/** * qemuAuditHostdev: diff --git a/src/qemu/qemu_audit.h b/src/qemu/qemu_audit.h index a2fbe11..14c7da5 100644 --- a/src/qemu/qemu_audit.h +++ b/src/qemu/qemu_audit.h @@ -46,6 +46,11 @@ void qemuAuditNet(virDomainObjPtr vm, const char *reason, bool success) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); +void qemuAuditNetDevice(virDomainDefPtr vmDef, + virDomainNetDefPtr netDef, + const char *device, + bool success) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); void qemuAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr def, const char *reason, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8cf1737..0fc466c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -35,6 +35,7 @@ #include "uuid.h" #include "c-ctype.h" #include "domain_nwfilter.h" +#include "qemu_audit.h"
#include<sys/utsname.h> #include<sys/stat.h> @@ -97,20 +98,20 @@ uname_normalize (struct utsname *ut)
/** * qemuPhysIfaceConnect: + * @def: the definition of the VM (needed by 802.1Qbh and audit) * @conn: pointer to virConnect object * @driver: pointer to the qemud_driver * @net: pointer to he VM's interface description with direct device type * @qemuCaps: flags for qemu - * @vmuuid: The UUID of the VM (needed by 802.1Qbh) * * Returns a filedescriptor on success or -1 in case of error. */ int -qemuPhysIfaceConnect(virConnectPtr conn, +qemuPhysIfaceConnect(virDomainDefPtr def, + virConnectPtr conn, struct qemud_driver *driver, virDomainNetDefPtr net, virBitmapPtr qemuCaps, - const unsigned char *vmuuid, enum virVMOperationType vmop) { int rc; @@ -124,9 +125,10 @@ qemuPhysIfaceConnect(virConnectPtr conn, vnet_hdr = 1;
rc = openMacvtapTap(net->ifname, net->mac, net->data.direct.linkdev, - net->data.direct.mode, vnet_hdr, vmuuid, + net->data.direct.mode, vnet_hdr, def->uuid, &net->data.direct.virtPortProfile,&res_ifname, vmop); + qemuAuditNetDevice(def, net, res_ifname, rc>= 0); if (rc>= 0) { VIR_FREE(net->ifname); net->ifname = res_ifname; @@ -152,11 +154,11 @@ qemuPhysIfaceConnect(virConnectPtr conn, } } #else + (void)def; (void)conn; (void)net; (void)qemuCaps; (void)driver; - (void)vmuuid; (void)vmop; qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No support for macvtap device")); @@ -167,7 +169,8 @@ qemuPhysIfaceConnect(virConnectPtr conn,
int -qemuNetworkIfaceConnect(virConnectPtr conn, +qemuNetworkIfaceConnect(virDomainDefPtr def, + virConnectPtr conn, struct qemud_driver *driver, virDomainNetDefPtr net, virBitmapPtr qemuCaps) @@ -247,13 +250,10 @@ qemuNetworkIfaceConnect(virConnectPtr conn,
memcpy(tapmac, net->mac, VIR_MAC_BUFLEN); tapmac[0] = 0xFE; /* Discourage bridge from using TAP dev MAC */ - if ((err = brAddTap(driver->brctl, - brname, -&net->ifname, - tapmac, - vnet_hdr, - true, -&tapfd))) { + err = brAddTap(driver->brctl, brname,&net->ifname, tapmac, + vnet_hdr, true,&tapfd); + qemuAuditNetDevice(def, net, "/dev/net/tun", tapfd>= 0); + if (err) { if (err == ENOTSUP) { /* In this particular case, give a better diagnostic. */ qemuReportError(VIR_ERR_INTERNAL_ERROR, @@ -304,7 +304,8 @@ cleanup:
int -qemuOpenVhostNet(virDomainNetDefPtr net, +qemuOpenVhostNet(virDomainDefPtr def, + virDomainNetDefPtr net, virBitmapPtr qemuCaps, int *vhostfd) { @@ -342,6 +343,7 @@ qemuOpenVhostNet(virDomainNetDefPtr net, }
*vhostfd = open("/dev/vhost-net", O_RDWR); + qemuAuditNetDevice(def, net, "/dev/vhost-net", *vhostfd>= 0);
/* If the config says explicitly to use vhost and we couldn't open it, * report an error. @@ -3460,7 +3462,7 @@ qemuBuildCommandLine(virConnectPtr conn,
if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { - int tapfd = qemuNetworkIfaceConnect(conn, driver, net, + int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, qemuCaps); if (tapfd< 0) goto error; @@ -3472,10 +3474,8 @@ qemuBuildCommandLine(virConnectPtr conn, tapfd)>= sizeof(tapfd_name)) goto no_memory; } else if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { - int tapfd = qemuPhysIfaceConnect(conn, driver, net, - qemuCaps, - def->uuid, - vmop); + int tapfd = qemuPhysIfaceConnect(def, conn, driver, net, + qemuCaps, vmop); if (tapfd< 0) goto error;
@@ -3494,7 +3494,7 @@ qemuBuildCommandLine(virConnectPtr conn, network device */ int vhostfd;
- if (qemuOpenVhostNet(net, qemuCaps,&vhostfd)< 0) + if (qemuOpenVhostNet(def, net, qemuCaps,&vhostfd)< 0) goto error; if (vhostfd>= 0) { virCommandTransferFD(cmd, vhostfd); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 2ae364c..528031d 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -116,20 +116,22 @@ char * qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev);
-int qemuNetworkIfaceConnect(virConnectPtr conn, +int qemuNetworkIfaceConnect(virDomainDefPtr def, + virConnectPtr conn, struct qemud_driver *driver, virDomainNetDefPtr net, virBitmapPtr qemuCaps) - ATTRIBUTE_NONNULL(1); + ATTRIBUTE_NONNULL(2);
-int qemuPhysIfaceConnect(virConnectPtr conn, +int qemuPhysIfaceConnect(virDomainDefPtr def, + virConnectPtr conn, struct qemud_driver *driver, virDomainNetDefPtr net, virBitmapPtr qemuCaps, - const unsigned char *vmuuid, enum virVMOperationType vmop);
-int qemuOpenVhostNet(virDomainNetDefPtr net, +int qemuOpenVhostNet(virDomainDefPtr def, + virDomainNetDefPtr net, virBitmapPtr qemuCaps, int *vhostfd);
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e8567ad..4646fd7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -576,7 +576,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, return -1; }
- if ((tapfd = qemuNetworkIfaceConnect(conn, driver, net, qemuCaps))< 0) + if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net, + qemuCaps))< 0) return -1; } else if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { if (priv->monConfig->type != VIR_DOMAIN_CHR_TYPE_UNIX) { @@ -587,9 +588,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, return -1; }
- if ((tapfd = qemuPhysIfaceConnect(conn, driver, net, + if ((tapfd = qemuPhysIfaceConnect(vm->def, conn, driver, net, qemuCaps, - vm->def->uuid, VIR_VM_OP_CREATE))< 0) return -1; } @@ -599,7 +599,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { /* Attempt to use vhost-net mode for these types of network device */ - if (qemuOpenVhostNet(net, qemuCaps,&vhostfd)< 0) + if (qemuOpenVhostNet(vm->def, net, qemuCaps,&vhostfd)< 0) goto cleanup;
if (vhostfd>= 0&&
ACK
On 03/10/2011 09:16 AM, Laine Stump wrote:
On 03/09/2011 03:42 PM, Eric Blake wrote:
Opening raw network devices with the intent of passing those fds to qemu is worth an audit point.
<long commit message> Hmm, we really ought to have some sort of documentation that describes all possible audit messages, but that can be a separate patch.
+++ b/src/qemu/qemu_hotplug.c @@ -576,7 +576,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, return -1; }
- if ((tapfd = qemuNetworkIfaceConnect(conn, driver, net, qemuCaps))< 0) + if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net, + qemuCaps))< 0)
This part needed some tweaks to rebase on top of the modified 1/2. Pretty mechanical, though.
ACK
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Since libvirt always passes /dev/net/tun to qemu via fd, we should never trigger the cases where qemu tries to directly open the device. Therefore, it is safer to deny the cgroup device ACL. * src/qemu/qemu_cgroup.c (defaultDeviceACL): Remove /dev/net/tun. * src/qemu/qemu.conf (cgroup_device_acl): Reflect this change. --- Might as well fix this in the process of audit cleanups. I tested that I was still able to access the network through a virtio connection with cgroup ACL enforcing in the host after this change. And it matches the fact that we did not have a cgroup ACL allow for /dev/tapN devices (also passed via fd). src/qemu/qemu.conf | 2 +- src/qemu/qemu_cgroup.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 8c6b996..364f555 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -182,7 +182,7 @@ # "/dev/null", "/dev/full", "/dev/zero", # "/dev/random", "/dev/urandom", # "/dev/ptmx", "/dev/kvm", "/dev/kqemu", -# "/dev/rtc", "/dev/hpet", "/dev/net/tun", +# "/dev/rtc", "/dev/hpet", #] diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 9a7d42f..8c3eee3 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -37,7 +37,7 @@ static const char *const defaultDeviceACL[] = { "/dev/null", "/dev/full", "/dev/zero", "/dev/random", "/dev/urandom", "/dev/ptmx", "/dev/kvm", "/dev/kqemu", - "/dev/rtc", "/dev/hpet", "/dev/net/tun", + "/dev/rtc", "/dev/hpet", NULL, }; #define DEVICE_PTY_MAJOR 136 -- 1.7.4
On 03/09/2011 05:12 PM, Eric Blake wrote:
Since libvirt always passes /dev/net/tun to qemu via fd, we should never trigger the cases where qemu tries to directly open the device. Therefore, it is safer to deny the cgroup device ACL.
* src/qemu/qemu_cgroup.c (defaultDeviceACL): Remove /dev/net/tun. * src/qemu/qemu.conf (cgroup_device_acl): Reflect this change. ---
Might as well fix this in the process of audit cleanups. I tested that I was still able to access the network through a virtio connection with cgroup ACL enforcing in the host after this change. And it matches the fact that we did not have a cgroup ACL allow for /dev/tapN devices (also passed via fd).
src/qemu/qemu.conf | 2 +- src/qemu/qemu_cgroup.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 8c6b996..364f555 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -182,7 +182,7 @@ # "/dev/null", "/dev/full", "/dev/zero", # "/dev/random", "/dev/urandom", # "/dev/ptmx", "/dev/kvm", "/dev/kqemu", -# "/dev/rtc", "/dev/hpet", "/dev/net/tun", +# "/dev/rtc", "/dev/hpet", #]
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 9a7d42f..8c3eee3 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -37,7 +37,7 @@ static const char *const defaultDeviceACL[] = { "/dev/null", "/dev/full", "/dev/zero", "/dev/random", "/dev/urandom", "/dev/ptmx", "/dev/kvm", "/dev/kqemu", - "/dev/rtc", "/dev/hpet", "/dev/net/tun", + "/dev/rtc", "/dev/hpet", NULL, }; #define DEVICE_PTY_MAJOR 136
ACK.
On 03/10/2011 09:27 AM, Laine Stump wrote:
On 03/09/2011 05:12 PM, Eric Blake wrote:
Since libvirt always passes /dev/net/tun to qemu via fd, we should never trigger the cases where qemu tries to directly open the device. Therefore, it is safer to deny the cgroup device ACL.
* src/qemu/qemu_cgroup.c (defaultDeviceACL): Remove /dev/net/tun. * src/qemu/qemu.conf (cgroup_device_acl): Reflect this change.
- "/dev/rtc", "/dev/hpet", "/dev/net/tun", + "/dev/rtc", "/dev/hpet", NULL, }; #define DEVICE_PTY_MAJOR 136
ACK.
Thanks; pushed (actually, I pushed this prior to 2/2). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Laine Stump