Re: [Qemu-devel] [PATCH] blockdev: Refuse to open encrypted image unless paused

[Fam Zheng <famz@redhat.com>]

On Thu, 03/13 14:25, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> 
> > On Wed, 03/12 18:00, Markus Armbruster wrote:
> >> Opening an encrypted image takes an additional step: setting the key.
> >> Between open and the key set, the image must not be used.
> >> 
> >> We have some protection against accidental use in place: you can't
> >> unpause a guest while we're missing keys.  You can, however, hot-plug
> >> block devices lacking keys into a running guest just fine, or insert
> >> media lacking keys.  In the latter case, notifying the guest of the
> >> insert is delayed until the key is set, which may suffice to protect
> >> at least some guests in common usage.
> >> 
> >> This patch makes the protection apply in more cases, in a rather
> >> heavy-handed way: it doesn't let you open encrypted images unless
> >> we're in a paused state.
> >> 
> >> It doesn't extend the protection to users other than the guest (block
> >> jobs?).  Use of runstate_check() from block.c is disgusting.  Best I
> >> can do right now.
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  block.c                | 8 +++++++-
> >>  stubs/Makefile.objs    | 1 +
> >>  stubs/runstate-check.c | 6 ++++++
> >>  3 files changed, 14 insertions(+), 1 deletion(-)
> >>  create mode 100644 stubs/runstate-check.c
> >> 
> >> diff --git a/block.c b/block.c
> >> index f1ef4b0..7604881 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -1388,12 +1388,18 @@ done:
> >>          ret = -EINVAL;
> >>          goto close_and_fail;
> >>      }
> >> -    QDECREF(options);
> >>  
> >>      if (!bdrv_key_required(bs)) {
> >>          bdrv_dev_change_media_cb(bs, true);
> >> +    } else if (!runstate_check(RUN_STATE_PRELAUNCH)
> >> +            && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
> >> +        error_setg(errp,
> >> +                   "Guest must be stopped for opening of encrypted image");
> >
> > Changing error message here breaks qemu-iotests 087.
> 
> Crap.  I'm on vacation until Monday, just checking in to shepherd this
> patch...
> 
> On *master*, "make check-block" reports
> 
>     Not run: 016 052 059 064 070 077
>     Failures: 085 087
>     Failed 2 of 34 tests
> 

I didn't run it from "make check-block" (Stefan sent a patch to remove 085 and
087 from check-block.

Running command: TEST_DIR=/tmp/qemu-iotests QEMU_PROG=../../x86_64-softmmu/qemu-system-x86_64 QEMU_IMG_PROG=../../qemu-img QEMU_IO_PROG=../../qemu-io QEMU_NBD_PROG=../../qemu-nbd ./check -qcow2 -o "" 087
QEMU          -- ../../x86_64-softmmu/qemu-system-x86_64
QEMU_IMG      -- ../../qemu-img
QEMU_IO       -- ../../qemu-io
QEMU_NBD      -- ../../qemu-nbd
IMGFMT        -- qcow2 (compat=1.1)
IMGPROTO      -- file
PLATFORM      -- Linux/x86_64 T430 3.13.6-1-ARCH
SOCKET_SCM_HELPER --

087 0s ... - output mismatch (see 087.out.bad)
--- 087.out     2014-03-12 09:28:04.167060760 +0800
+++ 087.out.bad 2014-03-14 09:11:59.770326144 +0800
@@ -31,7 +31,7 @@
 Testing:
 QMP_VERSION
 {"return": {}}
-{"error": {"class": "GenericError", "desc": "blockdev-add doesn't support encrypted devices"}}
+{"error": {"class": "GenericError", "desc": "could not open disk image disk: Guest must be stopped for opening of encrypted image"}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
Failures: 087
Failed 1 of 1 tests

Thanks,
Fam