[libvirt] [PATCH] qemu: don't munge user input during block commit

Michal Privoznik mprivozn at redhat.com
Fri Mar 7 10:58:35 UTC 2014


On 07.03.2014 00:47, Eric Blake wrote:
> While investigating https://bugzilla.redhat.com/show_bug.cgi?id=1061827
> I noticed that we pass user input unscathed for block-pull, but
> always pass a canonical absolute name through for block-commit.
> [Note that we probably _ought_ to validate that the user's request
> for block-pull actually matches the backing chain, the way we already
> do for block-commit - but that's a separate issue.  Further note that
> the ability to pass user input through unscathed allows backdoors
> such as specifying a backing image that is a network URI such as
> a gluster disk, instead of forcing things to the local file system;
> which is an area still under active investigation on whether libvirt
> needs to behave differently for network disks.]
>
> Since qemu may write the name that the user passed in as the backing
> file, a user may have a reason to want a relative file name passed
> through to qemu, and always munging things to absolute prevents that.
>
> Put another way, if you have the backing chain:
>
> [A] <- [B(back=./A)] <- [C(back=./B)]
>
> and commit B into A (virsh blockcommit $dom vda --base A --top B),
> the metadata of C will have to be re-written. But should it be
> rewritten as [C(back=./A)] or as [C(back=/path/to/A)]?  Still up in
> the air is whether qemu's decision should be based on whether B
> and/or C had relative paths, or on whether the --base and/or
> --top arguments to the command were relative paths; but if we always
> pass a canonical name, we've prevented the spelling of the command
> arguments from being part of the hueristics that qemu uses.
>
> I also audited the code, and verified that we never call
> qemuMonitorBlockCommit() with a NULL base, either before or after
> the change to qemu_driver.c.
>
> * src/qemu/qemu_driver.c (qemuDomainBlockCommit): Preserve user's
> spelling, since absolute vs. relative matters to qemu.
> * src/qemu/qemu_monitor.h (qemuMonitorBlockCommit): Base is never
> null.
> * src/qemu/qemu_monitor.c (qemuMonitorBlockCommit): Likewise.
> * src/qemu/qemu_monitor_json.h (qemuMonitorJSONBlockCommit):
> Likewise.
> * src/qemu/qemu_monitor_json.c (qemuMonitorJSONBlockCommit):
> Likewise.
>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>
> I was _hoping_ that this would solve the mentioned bugzilla.  But
> even with this applied, qemu still ended up writing an absolute
> backing file name into the active file in the backing chain, so that
> bug has been reassigned back to qemu - it probably has to do with
> the fact that libvirt always spawns qemu with -drive pointing to
> an absolute name, which is unrelated to what this patch fixes.
>
> Therefore, I'm a little bit hesitant to apply this patch, but
> wanted to post it for review anyway.
>
>   src/qemu/qemu_driver.c       | 11 ++++++++---
>   src/qemu/qemu_monitor.c      |  4 ++--
>   src/qemu/qemu_monitor.h      |  3 ++-
>   src/qemu/qemu_monitor_json.c |  2 +-
>   src/qemu/qemu_monitor_json.h |  5 +++--
>   5 files changed, 16 insertions(+), 9 deletions(-)

ACK

Michal




More information about the libvir-list mailing list