On Thu, May 03, 2012 at 05:32:05PM -0600, Eric Blake wrote:
> 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.
Oh I wasn't even intending it for that. This is more of an RFC to see
if people like the direction this is going, before doing more work on
it. There are still all the other drivers to add access checks to :-)
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list