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

[Markus Armbruster]

Fam Zheng <address@hidden> writes:

> On Thu, 03/13 14:25, Markus Armbruster wrote:
>> Fam Zheng <address@hidden> 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 <address@hidden>
>> >> ---
>> >>  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

I'll update this test to use -S, and add another one for the new failure
mode.  Thanks!