[libvirt] [PATCH 12/12] Insert access control checks for virDomainObjPtr into QEMU driver

Eric Blake eblake at redhat.com
Thu May 3 23:32:05 UTC 2012


On 05/02/2012 05:44 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> 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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120503/466bcfbf/attachment-0001.sig>


More information about the libvir-list mailing list