On 05/02/2012 05:44 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> Inserts the minimal access control checks to the QEMU driver to
> protect usage of virDomainObjPtr objects.
> ---
> src/qemu/qemu_driver.c | 626 +++++++++++++++++++++++++++++++++++++++++++--
600 lines qualifies as minimal - wow, we've got a lot of new code to add
for the other checks. I'm wondering if you can factor this for a
smaller addition:
> @@ -1267,72 +1282,90 @@ cleanup:
> static int qemuDomainIsActive(virDomainPtr dom)
> {
> struct qemud_driver *driver = dom->conn->privateData;
> - virDomainObjPtr obj;
> + virDomainObjPtr vm;
> int ret = -1;
>
> qemuDriverLock(driver);
> - obj = virDomainFindByUUID(&driver->domains, dom->uuid);
> + vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> qemuDriverUnlock(driver);
> - if (!obj) {
> + if (!vm) {
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> virUUIDFormat(dom->uuid, uuidstr);
> qemuReportError(VIR_ERR_NO_DOMAIN,
> _("no domain with matching uuid '%s'"), uuidstr);
> goto cleanup;
> }
> - ret = virDomainObjIsActive(obj);
> +
> + if (!virAccessManagerCheckDomain(driver->accessManager,
> + vm->def,
> + VIR_ACCESS_PERM_DOMAIN_READ))
> + goto cleanup;
> +
The pattern of getting a VM, followed by an access check, is pretty
common. What if we introduce a helper function:
vm = qemuDomainFindByUUIDAccess(driver, dom->uuid,
VIR_ACCESS_PERM_DOMAIN_READ);
which does both the virDOmainFindByUUID(&driver->domains, dom->uuid) and
the virAccessManagerCheckDomain(driver->accessManager, vm->def,
access_perm).
> @@ -2899,6 +3030,15 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags)
> goto cleanup;
> }
>
> + if (!virAccessManagerCheckDomain(driver->accessManager,
> + vm->def,
> + VIR_ACCESS_PERM_DOMAIN_STOP))
> + goto cleanup;
> + if (!virAccessManagerCheckDomain(driver->accessManager,
> + vm->def,
> + VIR_ACCESS_PERM_DOMAIN_HIBERNATE))
> + goto cleanup;
Hmm, code like this, where you check two separate permissions, makes me
wonder if we should allow virAccessManagerCheckDomain to take a bitmap
argument for all checks to run, as well as a simple way of generating a
bitmap for one or more access bits in one go. Or even some glue magic
to allow checking an unbounded list of permissions in what appears to be
one call:
virAccessManagerCheckDomain(driver->accessManager, vm->def,
VIR_ACCESS_PERM_DOMAIN_STOP,
VIR_ACCESS_PERM_DOMAIN_HIBERNATE)
#define virAccessManagerCheck(...) \
virAccessManagerCheckAll(__VA_ARGS__, 0)
virAccessManagerCheckAll(manager, def, ...) {
va_list list;
int perm;
va_start(list, def);
while ((perm = va_arg(list, int))
virAccessManagerCheckDomainOne(manager, def, perm);
va_end(list);
}
The bulk of the patch looks mechanical, and I didn't closely check it
for inaccuracies. But the overall idea seems worthwhile.
I'm afraid that this series won't make it into 0.9.12, though, as it is
rather invasive at this point when we already have rc1 out.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list