[libvirt] [RFC PATCH 0/2] audit cgroup ACL actions in qemu
I'm following up on danpb's patches to add initial audit support to qemu actions (see around commit 8dc136b in Oct 2010). Another useful thing to audit is all changes to the device ACL whitelist via the cgroup device controller - namely, any time that the qemu cgroup is altered to allow or deny access to a (set of) devices. I saw an alternate suggestion for collecting the needed audit information by using an inotify script outside of libvirt that monitors changes to the cgroup file system corresponding to each qemu instance; however, this had the drawback that it can only monitor that a change is being attempted, but not the actual change being made. It is necessary to use libvirt to provide the audit information, in order to have a rich enough set of information to make the audit worthwhile. I believe this patch series catches all instances where libvirt modifies the whitelist for a qemu instance, but I didn't do anything for when libvirt first creates a separate cgroup device whitelist as part of starting a new guest. Also, I'm not sure if the amount of information passed to the audit is adequate, or if it needs tweaking. Also, libvirt blindly passes a long list of paths through the low-level util/cgroup.c functions, including non-devices where it ignores the EINVAL failure later. But this could pollute the audit log with entries corresponding to cases where altering the actual cgroup was never even attempted. Should I tweak things to only do an audit when an actual cgroup change was attempted (perhaps by modifying util/cgroup.c to return 0 on success, 1 on skip, and -errno on failure, rather than the current -EINVAL on skip)? This is post-0.8.8 material. Eric Blake (2): audit: prepare qemu for listing vm in cgroup audits audit: add qemu hooks for auditing cgroup events src/qemu/qemu_audit.c | 48 ++++++++++++++++++++++++++++++- src/qemu/qemu_audit.h | 9 +++++- src/qemu/qemu_cgroup.c | 73 +++++++++++++++++++++++++++++++---------------- src/qemu/qemu_cgroup.h | 21 ++++++-------- src/qemu/qemu_driver.c | 12 +++++-- src/qemu/qemu_hotplug.c | 7 ++-- 6 files changed, 124 insertions(+), 46 deletions(-) -- 1.7.4
* src/qemu/qemu_cgroup.h (struct qemuCgroupData): New helper type. (qemuSetupDiskPathAllow, qemuSetupChardevCgroup) (qemuTeardownDiskPathDeny): Drop unneeded prototypes. (qemuSetupDiskCgroup, qemuTeardownDiskCgroup): Adjust prototype. * src/qemu/qemu_cgroup.c (qemuSetupDiskPathAllow, qemuSetupChardevCgroup) (qemuTeardownDiskPathDeny): Mark static and use new type. (qemuSetupHostUsbDeviceCgroup): Use new type. (qemuSetupDiskCgroup): Alter signature. (qemuSetupCgroup): Adjust caller. * src/qemu/qemu_hotplug.c (qemuDomainAttachHostUsbDevice) (qemuDomainDetachPciDiskDevice, qemuDomainDetachSCSIDiskDevice): Likewise. * src/qemu/qemu_driver.c (qemudDomainAttachDevice) (qemuDomainUpdateDeviceFlags): Likewise. --- Relatively straight-forward - all existing qemu audit entries include the vm being altered; so we need to pass vm through to the lowest-level points in qemu that alter the cgroup device whitelist. src/qemu/qemu_cgroup.c | 58 ++++++++++++++++++++++++++-------------------- src/qemu/qemu_cgroup.h | 21 +++++++--------- src/qemu/qemu_driver.c | 8 +++--- src/qemu/qemu_hotplug.c | 7 +++-- 4 files changed, 50 insertions(+), 44 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 8cd6ce9..38eacfb 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -54,18 +54,18 @@ int qemuCgroupControllerActive(struct qemud_driver *driver, return 0; } - -int qemuSetupDiskPathAllow(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, - const char *path, - size_t depth ATTRIBUTE_UNUSED, - void *opaque) +static int +qemuSetupDiskPathAllow(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, + const char *path, + size_t depth ATTRIBUTE_UNUSED, + void *opaque) { - virCgroupPtr cgroup = opaque; + qemuCgroupData *data = opaque; int rc; VIR_DEBUG("Process path %s for disk", path); /* XXX RO vs RW */ - rc = virCgroupAllowDevicePath(cgroup, path); + rc = virCgroupAllowDevicePath(data->cgroup, path); if (rc != 0) { /* Get this for non-block devices */ if (rc == -EINVAL) { @@ -84,28 +84,31 @@ int qemuSetupDiskPathAllow(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, int qemuSetupDiskCgroup(struct qemud_driver *driver, + virDomainObjPtr vm, virCgroupPtr cgroup, virDomainDiskDefPtr disk) { + qemuCgroupData data = { vm, cgroup }; return virDomainDiskDefForeachPath(disk, driver->allowDiskFormatProbing, true, qemuSetupDiskPathAllow, - cgroup); + &data); } -int qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, - const char *path, - size_t depth ATTRIBUTE_UNUSED, - void *opaque) +static int +qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, + const char *path, + size_t depth ATTRIBUTE_UNUSED, + void *opaque) { - virCgroupPtr cgroup = opaque; + qemuCgroupData *data = opaque; int rc; VIR_DEBUG("Process path %s for disk", path); /* XXX RO vs RW */ - rc = virCgroupDenyDevicePath(cgroup, path); + rc = virCgroupDenyDevicePath(data->cgroup, path); if (rc != 0) { /* Get this for non-block devices */ if (rc == -EINVAL) { @@ -124,22 +127,25 @@ int qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, int qemuTeardownDiskCgroup(struct qemud_driver *driver, + virDomainObjPtr vm, virCgroupPtr cgroup, virDomainDiskDefPtr disk) { + qemuCgroupData data = { vm, cgroup }; return virDomainDiskDefForeachPath(disk, driver->allowDiskFormatProbing, true, qemuTeardownDiskPathDeny, - cgroup); + &data); } -int qemuSetupChardevCgroup(virDomainDefPtr def, - virDomainChrDefPtr dev, - void *opaque) +static int +qemuSetupChardevCgroup(virDomainDefPtr def, + virDomainChrDefPtr dev, + void *opaque) { - virCgroupPtr cgroup = opaque; + qemuCgroupData *data = opaque; int rc; if (dev->source.type != VIR_DOMAIN_CHR_TYPE_DEV) @@ -147,7 +153,7 @@ int qemuSetupChardevCgroup(virDomainDefPtr def, VIR_DEBUG("Process path '%s' for disk", dev->source.data.file.path); - rc = virCgroupAllowDevicePath(cgroup, dev->source.data.file.path); + rc = virCgroupAllowDevicePath(data->cgroup, dev->source.data.file.path); if (rc != 0) { virReportSystemError(-rc, _("Unable to allow device %s for %s"), @@ -163,11 +169,11 @@ int qemuSetupHostUsbDeviceCgroup(usbDevice *dev ATTRIBUTE_UNUSED, const char *path, void *opaque) { - virCgroupPtr cgroup = opaque; + qemuCgroupData *data = opaque; int rc; VIR_DEBUG("Process path '%s' for USB device", path); - rc = virCgroupAllowDevicePath(cgroup, path); + rc = virCgroupAllowDevicePath(data->cgroup, path); if (rc != 0) { virReportSystemError(-rc, _("Unable to allow device %s"), @@ -201,6 +207,7 @@ int qemuSetupCgroup(struct qemud_driver *driver, } if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) { + qemuCgroupData data = { vm, cgroup }; rc = virCgroupDenyAllDevices(cgroup); if (rc != 0) { if (rc == -EPERM) { @@ -214,7 +221,7 @@ int qemuSetupCgroup(struct qemud_driver *driver, } for (i = 0; i < vm->def->ndisks ; i++) { - if (qemuSetupDiskCgroup(driver, cgroup, vm->def->disks[i]) < 0) + if (qemuSetupDiskCgroup(driver, vm, cgroup, vm->def->disks[i]) < 0) goto cleanup; } @@ -249,7 +256,7 @@ int qemuSetupCgroup(struct qemud_driver *driver, if (virDomainChrDefForeach(vm->def, true, qemuSetupChardevCgroup, - cgroup) < 0) + &data) < 0) goto cleanup; for (i = 0; i < vm->def->nhostdevs; i++) { @@ -265,7 +272,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, hostdev->source.subsys.u.usb.device)) == NULL) goto cleanup; - if (usbDeviceFileIterate(usb, qemuSetupHostUsbDeviceCgroup, cgroup) < 0 ) + if (usbDeviceFileIterate(usb, qemuSetupHostUsbDeviceCgroup, + &data) < 0) goto cleanup; } } diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 0a9692b..299bd2d 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -1,7 +1,7 @@ /* * qemu_cgroup.h: QEMU cgroup management * - * Copyright (C) 2006-2007, 2009-2010 Red Hat, Inc. + * Copyright (C) 2006-2007, 2009-2011 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -28,25 +28,22 @@ # include "domain_conf.h" # include "qemu_conf.h" +struct _qemuCgroupData { + virDomainObjPtr vm; + virCgroupPtr cgroup; +}; +typedef struct _qemuCgroupData qemuCgroupData; + int qemuCgroupControllerActive(struct qemud_driver *driver, int controller); -int qemuSetupDiskPathAllow(virDomainDiskDefPtr disk, - const char *path, - size_t depth, - void *opaque); int qemuSetupDiskCgroup(struct qemud_driver *driver, + virDomainObjPtr vm, virCgroupPtr cgroup, virDomainDiskDefPtr disk); -int qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk, - const char *path, - size_t depth, - void *opaque); int qemuTeardownDiskCgroup(struct qemud_driver *driver, + virDomainObjPtr vm, virCgroupPtr cgroup, virDomainDiskDefPtr disk); -int qemuSetupChardevCgroup(virDomainDefPtr def, - virDomainChrDefPtr dev, - void *opaque); int qemuSetupHostUsbDeviceCgroup(usbDevice *dev, const char *path, void *opaque); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 82a2210..375ad2b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6559,7 +6559,7 @@ static int qemudDomainAttachDevice(virDomainPtr dom, vm->def->name); goto endjob; } - if (qemuSetupDiskCgroup(driver, cgroup, dev->data.disk) < 0) + if (qemuSetupDiskCgroup(driver, vm, cgroup, dev->data.disk) < 0) goto endjob; } @@ -6605,7 +6605,7 @@ static int qemudDomainAttachDevice(virDomainPtr dom, /* Fallthrough */ } if (ret != 0 && cgroup) { - if (qemuTeardownDiskCgroup(driver, cgroup, dev->data.disk) < 0) + if (qemuTeardownDiskCgroup(driver, vm, cgroup, dev->data.disk) < 0) VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(dev->data.disk->src)); } @@ -6730,7 +6730,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, vm->def->name); goto endjob; } - if (qemuSetupDiskCgroup(driver, cgroup, dev->data.disk) < 0) + if (qemuSetupDiskCgroup(driver, vm, cgroup, dev->data.disk) < 0) goto endjob; } @@ -6754,7 +6754,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, } if (ret != 0 && cgroup) { - if (qemuTeardownDiskCgroup(driver, cgroup, dev->data.disk) < 0) + if (qemuTeardownDiskCgroup(driver, vm, cgroup, dev->data.disk) < 0) VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(dev->data.disk->src)); } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index fb9db5a..e959151 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -893,6 +893,7 @@ int qemuDomainAttachHostUsbDevice(struct qemud_driver *driver, if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) { virCgroupPtr cgroup = NULL; usbDevice *usb; + qemuCgroupData data = { vm, cgroup }; if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) !=0 ) { qemuReportError(VIR_ERR_INTERNAL_ERROR, @@ -905,7 +906,7 @@ int qemuDomainAttachHostUsbDevice(struct qemud_driver *driver, hostdev->source.subsys.u.usb.device)) == NULL) goto error; - if (usbDeviceFileIterate(usb, qemuSetupHostUsbDeviceCgroup, cgroup) < 0 ) + if (usbDeviceFileIterate(usb, qemuSetupHostUsbDeviceCgroup, &data) < 0) goto error; } @@ -1206,7 +1207,7 @@ int qemuDomainDetachPciDiskDevice(struct qemud_driver *driver, VIR_WARN("Unable to restore security label on %s", dev->data.disk->src); if (cgroup != NULL) { - if (qemuTeardownDiskCgroup(driver, cgroup, dev->data.disk) < 0) + if (qemuTeardownDiskCgroup(driver, vm, cgroup, dev->data.disk) < 0) VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(dev->data.disk->src)); } @@ -1284,7 +1285,7 @@ int qemuDomainDetachSCSIDiskDevice(struct qemud_driver *driver, VIR_WARN("Unable to restore security label on %s", dev->data.disk->src); if (cgroup != NULL) { - if (qemuTeardownDiskCgroup(driver, cgroup, dev->data.disk) < 0) + if (qemuTeardownDiskCgroup(driver, vm, cgroup, dev->data.disk) < 0) VIR_WARN("Failed to teardown cgroup for disk path %s", NULLSTR(dev->data.disk->src)); } -- 1.7.4
* src/qemu/qemu_audit.h (qemuDomainCgroupAudit): New prototype. * src/qemu/qemu_audit.c (qemuDomainCgroupAudit): Implement it. * src/qemu/qemu_driver.c (qemudDomainSaveFlag): Add audit. * src/qemu/qemu_cgroup.c (qemuSetupDiskPathAllow) (qemuSetupChardevCgroup, qemuSetupHostUsbDeviceCgroup) (qemuSetupCgroup, qemuTeardownDiskPathDeny): Likewise. --- Adding the actual audits. So far, this compiles, but I have no idea how to test it. Hence posting this as an RFC. Advice of where to proceed from here would be welcome! src/qemu/qemu_audit.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_audit.h | 9 ++++++++- src/qemu/qemu_cgroup.c | 15 +++++++++++++++ src/qemu/qemu_driver.c | 4 ++++ 4 files changed, 74 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_audit.c b/src/qemu/qemu_audit.c index f03f984..c76d49e 100644 --- a/src/qemu/qemu_audit.c +++ b/src/qemu/qemu_audit.c @@ -1,7 +1,7 @@ /* * qemu_audit.c: QEMU audit management * - * Copyright (C) 2006-2007, 2009-2010 Red Hat, Inc. + * Copyright (C) 2006-2007, 2009-2011 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -102,6 +102,52 @@ void qemuDomainNetAudit(virDomainObjPtr vm, } +/** + * qemuDomainCgroupAudit: + * @vm: domain making the cgroups ACL change + * @cgroup: cgroup that manages the devices + * @reason: either "allow" or "deny" + * @item: one of "all", "file", or "major" + * @name: NULL for @item of "all", path for @item of "file", and + * string describing major device type for @item of "major" + * @success: true if the cgroup operation succeeded + * + * Log an audit message about an attempted cgroup device ACL change. + */ +void qemuDomainCgroupAudit(virDomainObjPtr vm, + virCgroupPtr cgroup ATTRIBUTE_UNUSED, + const char *reason, + const char *item, + const char *name, + 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, "file") ? "path" : "type", + 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", + reason, vmname, uuidstr, + item, detail ? " " : "", detail ? detail : ""); + +cleanup: + VIR_FREE(vmname); + VIR_FREE(detail); +} + + static void qemuDomainLifecycleAudit(virDomainObjPtr vm, const char *op, const char *reason, diff --git a/src/qemu/qemu_audit.h b/src/qemu/qemu_audit.h index 5b5a5d3..40f4591 100644 --- a/src/qemu/qemu_audit.h +++ b/src/qemu/qemu_audit.h @@ -1,7 +1,7 @@ /* * qemu_audit.h: QEMU audit management * - * Copyright (C) 2006-2007, 2009-2010 Red Hat, Inc. + * Copyright (C) 2006-2007, 2009-2011 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -25,6 +25,7 @@ # define __QEMU_AUDIT_H__ # include "domain_conf.h" +# include "cgroup.h" void qemuDomainStartAudit(virDomainObjPtr vm, const char *reason, bool success); void qemuDomainStopAudit(virDomainObjPtr vm, const char *reason); @@ -38,6 +39,12 @@ void qemuDomainNetAudit(virDomainObjPtr vm, virDomainNetDefPtr newDef, const char *reason, bool success); +void qemuDomainCgroupAudit(virDomainObjPtr vm, + virCgroupPtr group, + const char *reason, + const char *item, + const char *name, + bool success); void qemuDomainSecurityLabelAudit(virDomainObjPtr vm, bool success); #endif /* __QEMU_AUDIT_H__ */ diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 38eacfb..54665fe 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -29,6 +29,7 @@ #include "memory.h" #include "virterror_internal.h" #include "util.h" +#include "qemu_audit.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -66,6 +67,8 @@ qemuSetupDiskPathAllow(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, VIR_DEBUG("Process path %s for disk", path); /* XXX RO vs RW */ rc = virCgroupAllowDevicePath(data->cgroup, path); + qemuDomainCgroupAudit(data->vm, data->cgroup, "allow", "file", path, + rc == 0); if (rc != 0) { /* Get this for non-block devices */ if (rc == -EINVAL) { @@ -109,6 +112,8 @@ qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, VIR_DEBUG("Process path %s for disk", path); /* XXX RO vs RW */ rc = virCgroupDenyDevicePath(data->cgroup, path); + qemuDomainCgroupAudit(data->vm, data->cgroup, "deny", "file", path, + rc == 0); if (rc != 0) { /* Get this for non-block devices */ if (rc == -EINVAL) { @@ -154,6 +159,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); + qemuDomainCgroupAudit(data->vm, data->cgroup, "allow", "file", + dev->source.data.file.path, rc == 0); if (rc != 0) { virReportSystemError(-rc, _("Unable to allow device %s for %s"), @@ -174,6 +181,8 @@ int qemuSetupHostUsbDeviceCgroup(usbDevice *dev ATTRIBUTE_UNUSED, VIR_DEBUG("Process path '%s' for USB device", path); rc = virCgroupAllowDevicePath(data->cgroup, path); + qemuDomainCgroupAudit(data->vm, data->cgroup, "allow", "file", path, + rc == 0); if (rc != 0) { virReportSystemError(-rc, _("Unable to allow device %s"), @@ -209,6 +218,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); if (rc != 0) { if (rc == -EPERM) { VIR_WARN0("Group devices ACL is not accessible, disabling whitelisting"); @@ -226,6 +236,7 @@ int qemuSetupCgroup(struct qemud_driver *driver, } rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_PTY_MAJOR); + qemuDomainCgroupAudit(vm, cgroup, "allow", "major", "pty", rc == 0); if (rc != 0) { virReportSystemError(-rc, "%s", _("unable to allow /dev/pts/ devices")); @@ -234,6 +245,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, if (vm->def->nsounds) { rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_SND_MAJOR); + qemuDomainCgroupAudit(vm, cgroup, "allow", "major", "sound", + rc == 0); if (rc != 0) { virReportSystemError(-rc, "%s", _("unable to allow /dev/snd/ devices")); @@ -244,6 +257,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, for (i = 0; deviceACL[i] != NULL ; i++) { rc = virCgroupAllowDevicePath(cgroup, deviceACL[i]); + qemuDomainCgroupAudit(vm, cgroup, "allow", "file", + 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 375ad2b..d89187a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4499,6 +4499,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, goto endjob; } rc = virCgroupAllowDevicePath(cgroup, path); + qemuDomainCgroupAudit(vm, cgroup, "allow", "file", path, rc == 0); if (rc != 0) { virReportSystemError(-rc, _("Unable to allow device %s for %s"), @@ -4548,6 +4549,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, if (cgroup != NULL) { rc = virCgroupDenyDevicePath(cgroup, path); + qemuDomainCgroupAudit(vm, cgroup, "deny", "file", path, rc == 0); if (rc != 0) VIR_WARN("Unable to deny device %s for %s %d", path, vm->def->name, rc); @@ -4579,6 +4581,8 @@ endjob: if (cgroup != NULL) { rc = virCgroupDenyDevicePath(cgroup, path); + qemuDomainCgroupAudit(vm, cgroup, "deny", "file", path, + rc == 0); if (rc != 0) VIR_WARN("Unable to deny device %s for %s: %d", path, vm->def->name, rc); -- 1.7.4
On Tuesday, February 15, 2011 09:59:59 pm Eric Blake wrote:
I'm following up on danpb's patches to add initial audit support to qemu actions (see around commit 8dc136b in Oct 2010). Another useful thing to audit is all changes to the device ACL whitelist via the cgroup device controller - namely, any time that the qemu cgroup is altered to allow or deny access to a (set of) devices.
I saw an alternate suggestion for collecting the needed audit information by using an inotify script outside of libvirt that monitors changes to the cgroup file system corresponding to each qemu instance; however, this had the drawback that it can only monitor that a change is being attempted, but not the actual change being made. It is necessary to use libvirt to provide the audit information, in order to have a rich enough set of information to make the audit worthwhile.
Does the cgroup auditing belong in the kernel? Or is there a syscall that would catch this?
I believe this patch series catches all instances where libvirt modifies the whitelist for a qemu instance, but I didn't do anything for when libvirt first creates a separate cgroup device whitelist as part of starting a new guest. Also, I'm not sure if the amount of information passed to the audit is adequate, or if it needs tweaking.
We would need to see some sample events. This patch should also be reviewed on the rhel6-cc-external-list so that Stephan can weigh in on whether you have everything he's expecting.
Also, libvirt blindly passes a long list of paths through the low-level util/cgroup.c functions, including non-devices where it ignores the EINVAL failure later. But this could pollute the audit log with entries corresponding to cases where altering the actual cgroup was never even attempted.
The audit event log needs to be accurate. You should never put events in there that aren't true.
Should I tweak things to only do an audit when an actual cgroup change was attempted (perhaps by modifying util/cgroup.c to return 0 on success, 1 on skip, and -errno on failure, rather than the current -EINVAL on skip)?
Probably. But also be careful about skipping too many things because there may be things with -EPERM/EACCES that we might want. I think I'd discuss this on the rhel6-cc list for clarification. -Steve
* src/qemu/qemu_audit.h (qemuDomainMemoryAudit) (qemuDomainVcpuAudit): New prototypes. * src/qemu/qemu_audit.c (qemuDomainResourceAudit) (qemuDomainMemoryAudit, qemuDomainVcpuAudit): New functions. (qemuDomainStartAudit): Call as appropriate. * src/qemu/qemu_driver.c (qemudDomainSetMemory) (qemudDomainHotplugVcpus): Likewise. --- Another round of useful audit points, for tracking hotplug changes to vcpus and memory allocations. Again, I'm not sure if this is the correct amount of information, hence including this as a continuation of my RFC patches. I'm not sure if qemudDomainSetMemory is doing the right thing - it doesn't update vm->def->mem.cur_balloon on success; on the other hand, dumpXML always recalculates the cur_balloon from a monitor query, and I don't know if any other API exposes a stale cur_balloon value from the vm definition. src/qemu/qemu_audit.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_audit.h | 10 ++++++++ src/qemu/qemu_driver.c | 7 +++++- 3 files changed, 72 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_audit.c b/src/qemu/qemu_audit.c index c76d49e..6ea31c9 100644 --- a/src/qemu/qemu_audit.c +++ b/src/qemu/qemu_audit.c @@ -148,6 +148,59 @@ cleanup: } +/** + * qemuDomainResourceAudit: + * @vm: domain making an integer resource change + * @resource: name of the resource: "mem" or "vcpu" + * @oldval: the old value of the resource + * @newval: the new value of the resource + * @reason: either "start" or "update" + * @success: true if the resource change succeeded + * + * Log an audit message about an attempted resource change. + */ +static void +qemuDomainResourceAudit(virDomainObjPtr vm, + const char *resource, + unsigned long long oldval, + unsigned long long newval, + const char *reason, + bool success) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + char *vmname; + + virUUIDFormat(vm->def->uuid, uuidstr); + if (!(vmname = virAuditEncode("vm", vm->def->name))) { + VIR_WARN0("OOM while encoding audit message"); + return; + } + + VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success, + "resrc=%s reason=%s %s uuid=%s old-%s=%lld new-%s=%lld", + resource, reason, vmname, uuidstr, + resource, oldval, resource, newval); + + VIR_FREE(vmname); +} + +void +qemuDomainMemoryAudit(virDomainObjPtr vm, + unsigned long long oldmem, unsigned long long newmem, + const char *reason, bool success) +{ + return qemuDomainResourceAudit(vm, "mem", oldmem, newmem, reason, success); +} + +void +qemuDomainVcpuAudit(virDomainObjPtr vm, + unsigned int oldvcpu, unsigned int newvcpu, + const char *reason, bool success) +{ + return qemuDomainResourceAudit(vm, "vcpu", oldvcpu, newvcpu, reason, + success); +} + static void qemuDomainLifecycleAudit(virDomainObjPtr vm, const char *op, const char *reason, @@ -185,6 +238,9 @@ void qemuDomainStartAudit(virDomainObjPtr vm, const char *reason, bool success) qemuDomainNetAudit(vm, NULL, net, "start", true); } + qemuDomainMemoryAudit(vm, 0, vm->def->mem.cur_balloon, "start", true); + qemuDomainVcpuAudit(vm, 0, vm->def->vcpus, "start", true); + qemuDomainLifecycleAudit(vm, "start", reason, success); } diff --git a/src/qemu/qemu_audit.h b/src/qemu/qemu_audit.h index 40f4591..cdbb957 100644 --- a/src/qemu/qemu_audit.h +++ b/src/qemu/qemu_audit.h @@ -45,6 +45,16 @@ void qemuDomainCgroupAudit(virDomainObjPtr vm, const char *item, const char *name, bool success); +void qemuDomainMemoryAudit(virDomainObjPtr vm, + unsigned long long oldmem, + unsigned long long newmem, + const char *reason, + bool success); +void qemuDomainVcpuAudit(virDomainObjPtr vm, + unsigned int oldvcpu, + unsigned int newvcpu, + const char *reason, + bool success); void qemuDomainSecurityLabelAudit(virDomainObjPtr vm, bool success); #endif /* __QEMU_AUDIT_H__ */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a98ceb4..ddd7728 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1605,6 +1605,8 @@ static int qemudDomainSetMemory(virDomainPtr dom, unsigned long newmem) { qemuDomainObjEnterMonitor(vm); r = qemuMonitorSetBalloon(priv->mon, newmem); qemuDomainObjExitMonitor(vm); + qemuDomainMemoryAudit(vm, vm->def->mem.cur_balloon, newmem, "update", + r <= 0); if (r < 0) goto endjob; @@ -1615,6 +1617,7 @@ static int qemudDomainSetMemory(virDomainPtr dom, unsigned long newmem) { goto endjob; } + /* XXX update vm->def->mem.cur_balloon? */ ret = 0; endjob: if (qemuDomainObjEndJob(vm) == 0) @@ -2518,8 +2521,9 @@ static void processWatchdogEvent(void *data, void *opaque) static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int nvcpus) { qemuDomainObjPrivatePtr priv = vm->privateData; - int i, rc; + int i, rc = 1; int ret = -1; + int oldvcpus = vm->def->vcpus; qemuDomainObjEnterMonitor(vm); @@ -2554,6 +2558,7 @@ static int qemudDomainHotplugVcpus(virDomainObjPtr vm, unsigned int nvcpus) cleanup: qemuDomainObjExitMonitor(vm); + qemuDomainVcpuAudit(vm, oldvcpus, nvcpus, "update", rc <= 0); return ret; unsupported: -- 1.7.4
participants (2)
-
Eric Blake -
Steve Grubb