[libvirt] [PATCH 0/3] more cgroup ACL audit improvements
Based on some feedback from Steve Grubb, Stephan Mueller, and others (unfortunately most of it on some non-public lists), I'm proposing the following patches to enhance my earlier audits for device cgroup ACLs. Pre-patch, cgroup audits looked like: type=VIRT_RESOURCE msg=audit(1298068194.479:83142): user pid=23863 uid=0 auid=500 ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 msg='resrc=cgroup reason=deny vm="fedora_12" uuid=51c6fc83-65a4-e627-b698-042b00145201 item=all: exe="/home/dummy/libvirt/daemon/.libs/lt-libvirtd" hostname=? addr=? terminal=pts/0 res=success' type=VIRT_RESOURCE msg=audit(1298068194.480:83143): user pid=23863 uid=0 auid=500 ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 msg='resrc=cgroup reason=allow vm="fedora_12" uuid=51c6fc83-65a4-e627-b698-042b00145201 item=major type="pty": exe="/home/dummy/libvirt/daemon/.libs/lt-libvirtd" hostname=? addr=? terminal=pts/0 res=success' type=VIRT_RESOURCE msg=audit(1298068194.480:83145): user pid=23863 uid=0 auid=500 ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 msg='resrc=cgroup reason=allow vm="fedora_12" uuid=51c6fc83-65a4-e627-b698-042b00145201 item=file path="/dev/null": exe="/home/dummy/libvirt/daemon/.libs/lt-libvirtd" hostname=? addr=? terminal=pts/0 res=success' Post-patch, the same three audits are modified to include cgroup controller, rdev information for files, major device number for categories, and better names so as not to collide with well-known audit field names (for example, audit libraries expect item= to match a decimal integer, so I used class= instead). type=VIRT_RESOURCE msg=audit(1299541864.111:78295): user pid=30632 uid=0 auid=500 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 msg='resrc=cgroup reason=deny vm="fedora_12" uuid=51c6fc83-65a4-e627-b698-042b00145201 cgroup="/cgroup/devices/libvirt/qemu/fedora_12/" class=all: exe="/home/dummy/libvirt/daemon/.libs/lt-libvirtd" hostname=? addr=? terminal=pts/0 res=success' type=VIRT_RESOURCE msg=audit(1299541864.112:78296): user pid=30632 uid=0 auid=500 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 msg='resrc=cgroup reason=allow vm="fedora_12" uuid=51c6fc83-65a4-e627-b698-042b00145201 cgroup="/cgroup/devices/libvirt/qemu/fedora_12/" class=major category=pty maj=88: exe="/home/dummy/libvirt/daemon/.libs/lt-libvirtd" hostname=? addr=? terminal=pts/0 res=success' type=VIRT_RESOURCE msg=audit(1299541864.112:78297): user pid=30632 uid=0 auid=500 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 msg='resrc=cgroup reason=allow vm="fedora_12" uuid=51c6fc83-65a4-e627-b698-042b00145201 cgroup="/cgroup/devices/libvirt/qemu/fedora_12/" class=path path=/dev/null rdev=01:03: exe="/home/dummy/libvirt/daemon/.libs/lt-libvirtd" hostname=? addr=? terminal=pts/0 res=success' Eric Blake (3): audit: tweak audit messages to match conventions audit: split cgroup audit types to allow more information audit: also audit cgroup controller path src/libvirt_private.syms | 1 + src/qemu/qemu_audit.c | 115 ++++++++++++++++++++++++++++++++++++++++------ src/qemu/qemu_audit.h | 14 +++++- src/qemu/qemu_cgroup.c | 29 ++++++------ src/qemu/qemu_driver.c | 8 ++-- src/util/cgroup.c | 8 ++-- src/util/cgroup.h | 5 ++ 7 files changed, 142 insertions(+), 38 deletions(-) -- 1.7.4
* src/qemu/qemu_audit.c (qemuDomainHostdevAudit): Avoid use of "type", which has a pre-defined meaning. (qemuDomainCgroupAudit): Likewise, as well as "item". --- src/qemu/qemu_audit.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_audit.c b/src/qemu/qemu_audit.c index b1948c8..0f954c0 100644 --- a/src/qemu/qemu_audit.c +++ b/src/qemu/qemu_audit.c @@ -159,7 +159,7 @@ qemuDomainHostdevAudit(virDomainObjPtr vm, } VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, - "resrc=dev reason=%s %s uuid=%s type=%s %s", + "resrc=dev reason=%s %s uuid=%s bus=%s %s", reason, vmname, uuidstr, virDomainHostdevSubsysTypeToString(hostdev->source.subsys.type), device); @@ -200,14 +200,14 @@ void qemuDomainCgroupAudit(virDomainObjPtr vm, return; } if (name && - !(detail = virAuditEncode(STREQ(item, "path") ? "path" : "type", + !(detail = virAuditEncode(STREQ(item, "path") ? "path" : "category", name))) { VIR_WARN0("OOM while encoding audit message"); goto cleanup; } VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, - "resrc=cgroup reason=%s %s uuid=%s item=%s%s%s", + "resrc=cgroup reason=%s %s uuid=%s class=%s%s%s", reason, vmname, uuidstr, item, detail ? " " : "", detail ? detail : ""); -- 1.7.4
Device names can be manipulated, so it is better to also log the major/minor device number corresponding to the cgroup ACL changes that libvirt made. This required some refactoring of the relatively new qemu cgroup audit code. * src/qemu/qemu_audit.c (qemuDomainCgroupAudit): Drop a parameter. (qemuDomainCgroupAuditMajor, qemuDomainCgroupAuditPath): New functions, to allow listing device major/minor in audit. * src/qemu/qemu_driver.c (qemudDomainSaveFlag): Adjust callers. * src/qemu/qemu_cgroup.c (qemuSetupDiskPathAllow) (qemuSetupChardevCgroup, qemuSetupHostUsbDeviceCgroup) (qemuSetupCgroup, qemuTeardownDiskPathDeny): Likewise. --- src/qemu/qemu_audit.c | 104 +++++++++++++++++++++++++++++++++++++++++------- src/qemu/qemu_audit.h | 14 ++++++- src/qemu/qemu_cgroup.c | 29 +++++++------ src/qemu/qemu_driver.c | 8 ++-- 4 files changed, 120 insertions(+), 35 deletions(-) diff --git a/src/qemu/qemu_audit.c b/src/qemu/qemu_audit.c index 0f954c0..0d85fa7 100644 --- a/src/qemu/qemu_audit.c +++ b/src/qemu/qemu_audit.c @@ -23,6 +23,9 @@ #include <config.h> +#include <sys/stat.h> +#include <sys/types.h> + #include "qemu_audit.h" #include "virtaudit.h" #include "uuid.h" @@ -176,9 +179,10 @@ cleanup: * @vm: domain making the cgroups ACL change * @cgroup: cgroup that manages the devices * @reason: either "allow" or "deny" - * @item: one of "all", "path", or "major" - * @name: NULL for @item of "all", device path for @item of "path", and - * string describing major device type for @item of "major" + * @extra: additional details, in the form "all", + * "major category=xyz maj=nn", or "path path=xyz dev=nn:mm" (the + * latter two are generated by qemuDomainCgroupAuditMajor and + * qemuDomainCgroupAuditPath). * @success: true if the cgroup operation succeeded * * Log an audit message about an attempted cgroup device ACL change. @@ -186,37 +190,107 @@ cleanup: void qemuDomainCgroupAudit(virDomainObjPtr vm, virCgroupPtr cgroup ATTRIBUTE_UNUSED, const char *reason, - const char *item, - const char *name, + const char *extra, bool success) { char uuidstr[VIR_UUID_STRING_BUFLEN]; char *vmname; - char *detail = NULL; virUUIDFormat(vm->def->uuid, uuidstr); if (!(vmname = virAuditEncode("vm", vm->def->name))) { VIR_WARN0("OOM while encoding audit message"); return; } - if (name && - !(detail = virAuditEncode(STREQ(item, "path") ? "path" : "category", - name))) { + + VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, + "resrc=cgroup reason=%s %s uuid=%s class=%s", + reason, vmname, uuidstr, extra); + + VIR_FREE(vmname); +} + +/** + * qemuDomainCgroupAuditMajor: + * @vm: domain making the cgroups ACL change + * @cgroup: cgroup that manages the devices + * @reason: either "allow" or "deny" + * @maj: the major number of the device category + * @name: a textual name for that device category, alphabetic only + * @success: true if the cgroup operation succeeded + * + * Log an audit message about an attempted cgroup device ACL change. + */ +void qemuDomainCgroupAuditMajor(virDomainObjPtr vm, + virCgroupPtr cgroup, + const char *reason, + int maj, + const char *name, + bool success) +{ + char *extra; + + if (virAsprintf(&extra, "major category=%s maj=%02X", name, maj) < 0) { + VIR_WARN0("OOM while encoding audit message"); + return; + } + + qemuDomainCgroupAudit(vm, cgroup, reason, extra, success); + + VIR_FREE(extra); +} + +/** + * qemuDomainCgroupAuditPath: + * @vm: domain making the cgroups ACL change + * @cgroup: cgroup that manages the devices + * @reason: either "allow" or "deny" + * @path: the device being adjusted + * @success: true if the cgroup operation succeeded + * + * Log an audit message about an attempted cgroup device ACL change to + * a specific device. + */ +void qemuDomainCgroupAuditPath(virDomainObjPtr vm, + virCgroupPtr cgroup, + const char *reason, + const char *path, + bool success) +{ + char *detail; + char *extra; + int rc; + + if (!(detail = virAuditEncode("path", path))) { + VIR_WARN0("OOM while encoding audit message"); + return; + } + +#if defined major && defined minor + struct stat sb; + if (stat(path, &sb) == 0 && + (S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode))) { + int maj = major(sb.st_rdev); + int min = minor(sb.st_rdev); + rc = virAsprintf(&extra, "path path=%s rdev=%02X:%02X", + path, maj, min); + } else +#endif + { + rc = virAsprintf(&extra, "path path=%s rdev=?", path); + } + + if (rc < 0) { VIR_WARN0("OOM while encoding audit message"); goto cleanup; } - VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, - "resrc=cgroup reason=%s %s uuid=%s class=%s%s%s", - reason, vmname, uuidstr, - item, detail ? " " : "", detail ? detail : ""); + qemuDomainCgroupAudit(vm, cgroup, reason, extra, success); cleanup: - VIR_FREE(vmname); + VIR_FREE(extra); VIR_FREE(detail); } - /** * qemuDomainResourceAudit: * @vm: domain making an integer resource change diff --git a/src/qemu/qemu_audit.h b/src/qemu/qemu_audit.h index 247edde..500eb98 100644 --- a/src/qemu/qemu_audit.h +++ b/src/qemu/qemu_audit.h @@ -46,9 +46,19 @@ void qemuDomainHostdevAudit(virDomainObjPtr vm, void qemuDomainCgroupAudit(virDomainObjPtr vm, virCgroupPtr group, const char *reason, - const char *item, - const char *name, + const char *extra, bool success); +void qemuDomainCgroupAuditMajor(virDomainObjPtr vm, + virCgroupPtr group, + const char *reason, + int maj, + const char *name, + bool success); +void qemuDomainCgroupAuditPath(virDomainObjPtr vm, + virCgroupPtr group, + const char *reason, + const char *path, + bool success); void qemuDomainMemoryAudit(virDomainObjPtr vm, unsigned long long oldmem, unsigned long long newmem, diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index e71d3fa..b911005 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -68,8 +68,8 @@ qemuSetupDiskPathAllow(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, /* XXX RO vs RW */ rc = virCgroupAllowDevicePath(data->cgroup, path); if (rc <= 0) - qemuDomainCgroupAudit(data->vm, data->cgroup, "allow", "path", path, - rc == 0); + qemuDomainCgroupAuditPath(data->vm, data->cgroup, "allow", path, + rc == 0); if (rc < 0) { if (rc == -EACCES) { /* Get this for root squash NFS */ VIR_DEBUG("Ignoring EACCES for %s", path); @@ -111,8 +111,8 @@ qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, /* XXX RO vs RW */ rc = virCgroupDenyDevicePath(data->cgroup, path); if (rc <= 0) - qemuDomainCgroupAudit(data->vm, data->cgroup, "deny", "path", path, - rc == 0); + qemuDomainCgroupAuditPath(data->vm, data->cgroup, "deny", path, + rc == 0); if (rc < 0) { if (rc == -EACCES) { /* Get this for root squash NFS */ VIR_DEBUG("Ignoring EACCES for %s", path); @@ -156,8 +156,8 @@ qemuSetupChardevCgroup(virDomainDefPtr def, VIR_DEBUG("Process path '%s' for disk", dev->source.data.file.path); rc = virCgroupAllowDevicePath(data->cgroup, dev->source.data.file.path); if (rc < 0) - qemuDomainCgroupAudit(data->vm, data->cgroup, "allow", "path", - dev->source.data.file.path, rc == 0); + qemuDomainCgroupAuditPath(data->vm, data->cgroup, "allow", + dev->source.data.file.path, rc == 0); if (rc < 0) { virReportSystemError(-rc, _("Unable to allow device %s for %s"), @@ -179,8 +179,8 @@ int qemuSetupHostUsbDeviceCgroup(usbDevice *dev ATTRIBUTE_UNUSED, VIR_DEBUG("Process path '%s' for USB device", path); rc = virCgroupAllowDevicePath(data->cgroup, path); if (rc <= 0) - qemuDomainCgroupAudit(data->vm, data->cgroup, "allow", "path", path, - rc == 0); + qemuDomainCgroupAuditPath(data->vm, data->cgroup, "allow", path, + rc == 0); if (rc < 0) { virReportSystemError(-rc, _("Unable to allow device %s"), @@ -216,7 +216,7 @@ int qemuSetupCgroup(struct qemud_driver *driver, if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) { qemuCgroupData data = { vm, cgroup }; rc = virCgroupDenyAllDevices(cgroup); - qemuDomainCgroupAudit(vm, cgroup, "deny", "all", NULL, rc == 0); + qemuDomainCgroupAudit(vm, cgroup, "deny", "all", rc == 0); if (rc != 0) { if (rc == -EPERM) { VIR_WARN0("Group devices ACL is not accessible, disabling whitelisting"); @@ -234,7 +234,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, } rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_PTY_MAJOR); - qemuDomainCgroupAudit(vm, cgroup, "allow", "major", "pty", rc == 0); + qemuDomainCgroupAuditMajor(vm, cgroup, "allow", DEVICE_PTY_MAJOR, + "pty", rc == 0); if (rc != 0) { virReportSystemError(-rc, "%s", _("unable to allow /dev/pts/ devices")); @@ -247,8 +248,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, driver->vncAllowHostAudio) || (vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL)))) { rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_SND_MAJOR); - qemuDomainCgroupAudit(vm, cgroup, "allow", "major", "sound", - rc == 0); + qemuDomainCgroupAuditMajor(vm, cgroup, "allow", DEVICE_SND_MAJOR, + "sound", rc == 0); if (rc != 0) { virReportSystemError(-rc, "%s", _("unable to allow /dev/snd/ devices")); @@ -259,8 +260,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, for (i = 0; deviceACL[i] != NULL ; i++) { rc = virCgroupAllowDevicePath(cgroup, deviceACL[i]); - qemuDomainCgroupAudit(vm, cgroup, "allow", "path", - deviceACL[i], rc == 0); + qemuDomainCgroupAuditPath(vm, cgroup, "allow", + deviceACL[i], rc == 0); if (rc < 0 && rc != -ENOENT) { virReportSystemError(-rc, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0f7cbad..cb5d7ef 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1965,7 +1965,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, } rc = virCgroupAllowDevicePath(cgroup, path); if (rc <= 0) - qemuDomainCgroupAudit(vm, cgroup, "allow", "path", path, rc == 0); + qemuDomainCgroupAuditPath(vm, cgroup, "allow", path, rc == 0); if (rc < 0) { virReportSystemError(-rc, _("Unable to allow device %s for %s"), @@ -2016,7 +2016,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, if (cgroup != NULL) { rc = virCgroupDenyDevicePath(cgroup, path); if (rc <= 0) - qemuDomainCgroupAudit(vm, cgroup, "deny", "path", path, rc == 0); + qemuDomainCgroupAuditPath(vm, cgroup, "deny", path, rc == 0); if (rc < 0) VIR_WARN("Unable to deny device %s for %s %d", path, vm->def->name, rc); @@ -2049,8 +2049,8 @@ endjob: if (cgroup != NULL) { rc = virCgroupDenyDevicePath(cgroup, path); if (rc <= 0) - qemuDomainCgroupAudit(vm, cgroup, "deny", "path", path, - rc == 0); + qemuDomainCgroupAuditPath(vm, cgroup, "deny", path, + rc == 0); if (rc < 0) VIR_WARN("Unable to deny device %s for %s: %d", path, vm->def->name, rc); -- 1.7.4
Although the cgroup device ACL controller path can be worked out by researching the code, it is more efficient to include that information directly in the audit message. * src/util/cgroup.h (virCgroupPathOfController): New prototype. * src/util/cgroup.c (virCgroupPathOfController): Export. * src/libvirt_private.syms: Likewise. * src/qemu/qemu_audit.c (qemuDomainCgroupAudit): Use it. --- src/libvirt_private.syms | 1 + src/qemu/qemu_audit.c | 19 ++++++++++++++++--- src/util/cgroup.c | 8 ++++---- src/util/cgroup.h | 5 +++++ 4 files changed, 26 insertions(+), 7 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index efcf3c5..c0da78e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -79,6 +79,7 @@ virCgroupKill; virCgroupKillRecursive; virCgroupKillPainfully; virCgroupMounted; +virCgroupPathOfController; virCgroupRemove; virCgroupSetBlkioWeight; virCgroupSetCpuShares; diff --git a/src/qemu/qemu_audit.c b/src/qemu/qemu_audit.c index 0d85fa7..c2402b5 100644 --- a/src/qemu/qemu_audit.c +++ b/src/qemu/qemu_audit.c @@ -188,13 +188,15 @@ cleanup: * Log an audit message about an attempted cgroup device ACL change. */ void qemuDomainCgroupAudit(virDomainObjPtr vm, - virCgroupPtr cgroup ATTRIBUTE_UNUSED, + virCgroupPtr cgroup, const char *reason, const char *extra, bool success) { char uuidstr[VIR_UUID_STRING_BUFLEN]; char *vmname; + char *controller = NULL; + char *detail; virUUIDFormat(vm->def->uuid, uuidstr); if (!(vmname = virAuditEncode("vm", vm->def->name))) { @@ -202,11 +204,22 @@ void qemuDomainCgroupAudit(virDomainObjPtr vm, return; } + virCgroupPathOfController(cgroup, VIR_CGROUP_CONTROLLER_DEVICES, + NULL, &controller); + + if (!(detail = virAuditEncode("cgroup", VIR_AUDIT_STR(controller)))) { + VIR_WARN0("OOM while encoding audit message"); + goto cleanup; + } + VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, - "resrc=cgroup reason=%s %s uuid=%s class=%s", - reason, vmname, uuidstr, extra); + "resrc=cgroup reason=%s %s uuid=%s %s class=%s", + reason, vmname, uuidstr, detail, extra); +cleanup: VIR_FREE(vmname); + VIR_FREE(controller); + VIR_FREE(detail); } /** diff --git a/src/util/cgroup.c b/src/util/cgroup.c index c5b8cdd..a16b1ab 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -254,10 +254,10 @@ static int virCgroupDetect(virCgroupPtr group) #endif -static int virCgroupPathOfController(virCgroupPtr group, - int controller, - const char *key, - char **path) +int virCgroupPathOfController(virCgroupPtr group, + int controller, + const char *key, + char **path) { if (controller == -1) { int i; diff --git a/src/util/cgroup.h b/src/util/cgroup.h index d468cb3..b3c5f27 100644 --- a/src/util/cgroup.h +++ b/src/util/cgroup.h @@ -40,6 +40,11 @@ int virCgroupForDomain(virCgroupPtr driver, virCgroupPtr *group, int create); +int virCgroupPathOfController(virCgroupPtr group, + int controller, + const char *key, + char **path); + int virCgroupAddTask(virCgroupPtr group, pid_t pid); int virCgroupSetBlkioWeight(virCgroupPtr group, unsigned int weight); -- 1.7.4
participants (1)
-
Eric Blake