On 10/17/2012 06:30 PM, Eric Blake wrote:
> I finally have all the pieces in place to perform a block-commit with
> SELinux enforcing. There's still missing cleanup work when the commit
> completes, but doing that requires tracking both the backing chain and
> the base and top files within that chain in domain XML across libvirtd
> restarts. Furthermore, from a security standpoint, once you have
> granted access, you must assume any damage that can be done will be
> done; later revoking access is nice to minimize the window of damage,
> but less important as it does not affect the fact that damage can be
> done in the first place. Therefore, saving the revoke efforts until
> we have better XML tracking of what chain operations are in effect,
> including across a libvirtd restart, is reasonable.
Umm. I don't know that I agree with your statement about the relative
importance of undoing all the labelling when you're done, but if the
alternative to this is to require turning off selinux, then it's
definitely a win.
>
> * src/qemu/qemu_driver.c (qemuDomainBlockCommit): Label disks as
> needed.
> (qemuDomainPrepareDiskChainElement): Cast away const.
> ---
>
> v3: enhance commit message
> src/qemu/qemu_driver.c | 42 +++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 072ec17..3829a89 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -10469,7 +10469,7 @@ qemuDomainPrepareDiskChainElement(struct qemud_driver *driver,
> virDomainObjPtr vm,
> virCgroupPtr cgroup,
> virDomainDiskDefPtr disk,
> - char *file,
> + const char *file,
> qemuDomainDiskChainMode mode)
> {
> /* The easiest way to label a single file with the same
> @@ -10481,7 +10481,7 @@ qemuDomainPrepareDiskChainElement(struct qemud_driver *driver,
> bool origreadonly = disk->readonly;
> int ret = -1;
>
> - disk->src = file;
> + disk->src = (char *) file; /* casting away const is safe here */
... since you're misusing the diskdef anyway :-)
> disk->format = VIR_STORAGE_FILE_RAW;
> disk->backingChain = NULL;
> disk->readonly = mode == VIR_DISK_CHAIN_READ_ONLY;
> @@ -12699,6 +12699,7 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base,
> virStorageFileMetadataPtr top_meta = NULL;
> const char *top_parent = NULL;
> const char *base_canon = NULL;
> + virCgroupPtr cgroup = NULL;
>
> virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, -1);
>
> @@ -12774,15 +12775,46 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base,
> goto endjob;
> }
>
> - /* XXX We need to be changing the SELinux label on both 'base' and
> - * the parent of 'top', so that qemu can open(O_RDWR) those files
> - * for the duration of the commit. */
> + /* For the commit to succeed, we must allow qemu to open both the
> + * 'base' image and the parent of 'top' as read/write; 'top' might
> + * not have a parent, or might already be read-write. XXX It
> + * would also be nice to revert 'base' to read-only, as well as
> + * revoke access to files removed from the chain, when the commit
> + * operation succeeds, but doing that requires tracking the
> + * operation in XML across libvirtd restarts. */
> + if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES) &&
> + virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0)) {
It looks to me like everything in cgroup.c returns < 0 ( -errno in most,
if not all cases). Any idea why all its callers are testing for != 0
instead of the more standard < 0 ?
(you're just following the convention, so no complaints on your patch,
but it looks a bit odd.)
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to find cgroup for %s"),
> + vm->def->name);
> + goto endjob;
> + }
> + if (qemuDomainPrepareDiskChainElement(driver, vm, cgroup, disk, base_canon,
> + VIR_DISK_CHAIN_READ_WRITE) < 0 ||
> + (top_parent && top_parent != disk->src &&
> + qemuDomainPrepareDiskChainElement(driver, vm, cgroup, disk,
> + top_parent,
> + VIR_DISK_CHAIN_READ_WRITE) < 0))
> + goto endjob;
> +
> + /* Start the commit operation. */
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorBlockCommit(priv->mon, device, top_canon, base_canon,
> bandwidth);
> qemuDomainObjExitMonitor(driver, vm);
>
> endjob:
> + if (ret < 0) {
> + /* Revert access to read-only, if possible. */
> + qemuDomainPrepareDiskChainElement(driver, vm, cgroup, disk, base_canon,
> + VIR_DISK_CHAIN_READ_ONLY);
> + if (top_parent && top_parent != disk->src)
> + qemuDomainPrepareDiskChainElement(driver, vm, cgroup, disk,
> + top_parent,
> + VIR_DISK_CHAIN_READ_ONLY);
Is it always the case that it was previously read-only, or is this just
a naive assumption?
> + }
> + if (cgroup)
> + virCgroupFree(&cgroup);
> if (qemuDomainObjEndJob(driver, vm) == 0) {
> vm = NULL;
> goto cleanup;
ACK.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list