[Qemu-devel] The unholy encrypted image key mess

[Qemu-devel] The unholy encrypted image key mess

[Markus Armbruster]

The encrypted images Saturday afternoon hack is a gift that keeps on
giving.  I wish we could rip it out and bury it deep.  Or that I could
continue to ignore it.  Unfortunately, it looks like I need to touch it
to clean up error propagation in the monitor.  So here goes.

A naive user might expect QEMU to require the password right when he
asks QEMU to open an encrypted image.  But that's not how it works.

Two bits in BlockDriverState apply: encrypted and valid_key.

Both are unset when BlockDriverState has no image, i.e. after bdrv_new()
and bdrv_close().

Opening an image sets encrypted iff the image is encrypted, in driver
method bdrv_open().  valid_key remains unset.  Two states: NOKEY and
NEEDKEY.  NEEDKEY is the troublemaker.

bdrv_set_key() sets a key.  Fails unless encrypted is set, i.e. it
requires state NEEDKEY.  Good.  Also fails if the block driver rejects
the password, but no driver does that.  Otherwise, it sets valid_key.
Call this state GOTKEY.

The user can open an image via command line, HMP commands change,
usb_add (legacy), drive_add, and QMP command blockdev-add.  They all
create BlockDriverStates in states NOKEY or NEEDKEY.

The user can set a key with HMP / QMP command block_passwd.  If it
succeeds, state goes from NEEDKEY to GOTKEY.

You can't unpause a guest while a BlockDriverState is in state NEEDKEY:

* QMP command cont fails if any BlockDriverState is in state NEEDKEY.

* HMP command cont tries to be cute then: it picks a BlockDriverState in
  state NEEDKEY and prompts for its key.  If this is the only one in
  state NEEDKEY, it unpauses the guest as ordered.  Else it silently
  doesn't.  The user is expected to know to rerun cont for the next key
  prompt.

This might make you think that the guest is protected from ever having a
block device backed by a BlockDriverState in state NEEDKEY.  Not true!
Simply open an image while the guest runs, and hot-plug a device backed
by it.  The guest will happily read encrypted garbage from it, and if it
writes anything to it, it's not so encrypted anymore.  To add insult to
injury, the "protection" kicks in next time you pause the guest.

Changing media has its wrinkles, too:

* HMP command change first closes the old image, then opens the new
  image, and if it's encrypted, it asks for a key.  Okay.

* QMP command change first opens the image, then errors out if it's
  encrypted and the password argument is missing, or it's not encrypted
  and the password argument is present.  These two errors aren't really
  failures; the change succeeds just fine.

  Clients can detect the former non-failure error (it's class
  "DeviceEncrypted"), and use block_passwd to go to state GOTKEY.

  Clients can't reliably distinguish the latter non-failure error from
  real errors, because it's class "GenericError".

In any case, callback bdrv_dev_change_media_cb() is delayed for
encrypted images until we enter state GOTKEY.  As afar as I can tell,
nothing stops the device model from reading or writing before it gets
the callback, but that would be a device model bug.

The final funny is device model usb-storage (another one that
desperately needs to be buried deep).  Its init() callback
usb_msd_initfn_storage() tries to be cute when it detects state NEEDKEY.

If it's running in monitor context (say in HMP/QMP command device_add),
it attempts to ask for a key.  In HMP context, it unplugs itself when
this fails (I think).  In QMP context, it behaves similar to change: it
works, but you get a "DeviceEncrypted" error, and the backend remains in
state NEEDKEY.

If it's not running in monitor context, it clears autostart.  No idea
why it does that, and I'm not sure it has any effect.  Opening an
encrypted image clears autostart already, in blockdev_init().

Thankfully, usb-storage is the only device model that messes with keys.

Questions:

1. Should we protect guests from state NEEDKEY?

2. If yes, how?

   Pause the guest when something enters state NEEDKEY?  I'd hate that.

   Fail device_add in state NEEDKEY?  Takes care of hot-plug, and
   cold-plug is already protected by cont.

Other bright ideas?

Re: [Qemu-devel] The unholy encrypted image key mess

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 6093 bytes --]

On 02/28/2014 02:01 PM, Markus Armbruster wrote:
> The encrypted images Saturday afternoon hack is a gift that keeps on
> giving.  I wish we could rip it out and bury it deep.  Or that I could
> continue to ignore it.  Unfortunately, it looks like I need to touch it
> to clean up error propagation in the monitor.  So here goes.
> 
> A naive user might expect QEMU to require the password right when he
> asks QEMU to open an encrypted image.  But that's not how it works.
> 

> 
> The user can open an image via command line, HMP commands change,
> usb_add (legacy), drive_add, and QMP command blockdev-add.  They all
> create BlockDriverStates in states NOKEY or NEEDKEY.

Uggh - so there's no current way to hot-plug a device in state GOTKEY
short of using a two-command sequence?  It would be nicer if hot-plug
had a way to fail to add encrypted devices unless the user also passes
the password at the same time, creating the device directly into the
GOTKEY state.

> 
> The user can set a key with HMP / QMP command block_passwd.  If it
> succeeds, state goes from NEEDKEY to GOTKEY.
> 
> You can't unpause a guest while a BlockDriverState is in state NEEDKEY:
> 
> * QMP command cont fails if any BlockDriverState is in state NEEDKEY.
> 
> * HMP command cont tries to be cute then: it picks a BlockDriverState in
>   state NEEDKEY and prompts for its key.  If this is the only one in
>   state NEEDKEY, it unpauses the guest as ordered.  Else it silently
>   doesn't.  The user is expected to know to rerun cont for the next key
>   prompt.

Arguably, the HMP behavior could be made smarter, to do the loop itself.

> 
> This might make you think that the guest is protected from ever having a
> block device backed by a BlockDriverState in state NEEDKEY.  Not true!
> Simply open an image while the guest runs, and hot-plug a device backed
> by it.  The guest will happily read encrypted garbage from it, and if it
> writes anything to it, it's not so encrypted anymore.  To add insult to
> injury, the "protection" kicks in next time you pause the guest.
> 
> Changing media has its wrinkles, too:
> 
> * HMP command change first closes the old image, then opens the new
>   image, and if it's encrypted, it asks for a key.  Okay.
> 
> * QMP command change first opens the image, then errors out if it's
>   encrypted and the password argument is missing, or it's not encrypted
>   and the password argument is present.  These two errors aren't really
>   failures; the change succeeds just fine.

Indeed, the 'change' command specifically documents that using 'change'
on a running guest with an encrypted image is ill-advised, because
there's no password provided along with the 'change' action, so the user
is forced to do a 2-command sequence where any window with the guest
running is hosed before the followup 'block_passwd'.

Question: when changing from one encrypted image to another, does the
second image inherit the password from the first (whether or not it
decrypts the second), or do we always clean state back to having no
known password?  The way you worded it, it sounds like the password is
inherited from the first media to the second?

> 
>   Clients can detect the former non-failure error (it's class
>   "DeviceEncrypted"), and use block_passwd to go to state GOTKEY.

This makes sense whether you go from unencrypted to encrypted media, or
if going to an encrypted media always wipes out the old password state.

> 
>   Clients can't reliably distinguish the latter non-failure error from
>   real errors, because it's class "GenericError".

It seems like this error (providing a password argument for a
non-encrypted disk) is not possible via the 'change' command.  How
exactly is this state even triggered?

> 
> In any case, callback bdrv_dev_change_media_cb() is delayed for
> encrypted images until we enter state GOTKEY.  As afar as I can tell,
> nothing stops the device model from reading or writing before it gets
> the callback, but that would be a device model bug.

And one that the qapi docs specifically warn against.

> 
> The final funny is device model usb-storage (another one that
> desperately needs to be buried deep).  Its init() callback
> usb_msd_initfn_storage() tries to be cute when it detects state NEEDKEY.
> 
> If it's running in monitor context (say in HMP/QMP command device_add),
> it attempts to ask for a key.  In HMP context, it unplugs itself when
> this fails (I think).  In QMP context, it behaves similar to change: it
> works, but you get a "DeviceEncrypted" error, and the backend remains in
> state NEEDKEY.
> 
> If it's not running in monitor context, it clears autostart.  No idea
> why it does that, and I'm not sure it has any effect.  Opening an
> encrypted image clears autostart already, in blockdev_init().
> 
> Thankfully, usb-storage is the only device model that messes with keys.
> 
> Questions:
> 
> 1. Should we protect guests from state NEEDKEY?

Ideally, yes.

> 
> 2. If yes, how?
> 
>    Pause the guest when something enters state NEEDKEY?  I'd hate that.
> 
>    Fail device_add in state NEEDKEY?  Takes care of hot-plug, and
>    cold-plug is already protected by cont.

The existing QMP 'change' command is stupid.  Add a new 'disk-change'
command that takes a password argument, and require that the new media
either be unencrypted (and error if a password was provided) or fully
reach the GOTKEY state (and error if no password was provided) for the
command to successfully change the media.  Similarly for hotplug.  That
is, enforce that hot-plug can never make a NEEDKEY block device visible
to the guest.

> Other bright ideas?

Use the fact that we are calling the next release "2.0" to actually kill
qemu disk encryption as a horribly botched feature, on the grounds that
we are doing users a favor by not letting them use broken encryption?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] The unholy encrypted image key mess

[Paolo Bonzini]

Il 28/02/2014 23:08, Eric Blake ha scritto:
> Use the fact that we are calling the next release "2.0" to actually kill
> qemu disk encryption as a horribly botched feature, on the grounds that
> we are doing users a favor by not letting them use broken encryption?

Only for qemu, of course---qemu-img would still have support for 
encryption, and there's no reason to risk stability by removing all the 
monitor code right now.

However, wouldn't we have the same problems even with a sane encrypted 
image format (based on LUKS, for example)?

Let's just open bugs (oh if only Launchpad supported tracker bugs) for now.

Paolo

Re: [Qemu-devel] The unholy encrypted image key mess

[Kevin Wolf]

Am 28.02.2014 um 22:01 hat Markus Armbruster geschrieben:
> Questions:
> 
> 1. Should we protect guests from state NEEDKEY?

Yes. An image in state NEEDKEY isn't fully initialised, so we should
make sure that it isn't used.

> 2. If yes, how?
> 
>    Pause the guest when something enters state NEEDKEY?  I'd hate that.
> 
>    Fail device_add in state NEEDKEY?  Takes care of hot-plug, and
>    cold-plug is already protected by cont.

'device_add' should refuse to accept a backend that isn't fully
initialised, so yes, I agree.

'change' is a bit trickier because it involves several low-level actions
at once, and device_add is not one of them. What we probably really need
to do is support a state where no BDS is attached to the device
emulation (a BlockBackend might still be attached, not sure about this
one), but the VM is still running. And then 'change' can detach the BDS,
bring it back to the NEEDKEY state (unrealize in QOM speech?), magic
happens and then we reattach the BDS to the guest device.

We'll also want to protect other parts of qemu from not fully
initialised BDSes, e.g. block jobs or NBD servers shouldn't take such an
BDS as their source.

Kevin

Re: [Qemu-devel] The unholy encrypted image key mess

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/28/2014 02:01 PM, Markus Armbruster wrote:
>> The encrypted images Saturday afternoon hack is a gift that keeps on
>> giving.  I wish we could rip it out and bury it deep.  Or that I could
>> continue to ignore it.  Unfortunately, it looks like I need to touch it
>> to clean up error propagation in the monitor.  So here goes.
>> 
>> A naive user might expect QEMU to require the password right when he
>> asks QEMU to open an encrypted image.  But that's not how it works.
>> 
>> Two bits in BlockDriverState apply: encrypted and valid_key.
>> 
>> Both are unset when BlockDriverState has no image, i.e. after bdrv_new()
>> and bdrv_close().
>> 
>> Opening an image sets encrypted iff the image is encrypted, in driver
>> method bdrv_open().  valid_key remains unset.  Two states: NOKEY and
>> NEEDKEY.  NEEDKEY is the troublemaker.
>> 
>> bdrv_set_key() sets a key.  Fails unless encrypted is set, i.e. it
>> requires state NEEDKEY.  Good.  Also fails if the block driver rejects
>> the password, but no driver does that.  Otherwise, it sets valid_key.
>> Call this state GOTKEY.
>> 
>> The user can open an image via command line, HMP commands change,
>> usb_add (legacy), drive_add, and QMP command blockdev-add.  They all
>> create BlockDriverStates in states NOKEY or NEEDKEY.
>
> Uggh - so there's no current way to hot-plug a device in state GOTKEY
> short of using a two-command sequence?  It would be nicer if hot-plug
> had a way to fail to add encrypted devices unless the user also passes
> the password at the same time, creating the device directly into the
> GOTKEY state.

I can't see why QMP commands would ever want to create in state NEEDKEY.
We could easily avoid it there: give QMP commands creating
BlockDriverStates an optional password parameter, fail the command if
the BDS is encrypted and the password parameter is missing.

QMP command change is crap, and should be replaced rather than fixed.

QMP command blockdev-add is new.  Can we still fix it?  Kevin?

For HMP, we need to make up our minds how to do passwords.

The current way is to tie NEEDKEY to "guest paused".  I hate that.

Another way is to make the commands adding BDS prompt for necessary
passwords.  We still have to deal with state NEEDKEY while we're waiting
for the user's reply.  Need to take care to hide the new BDS.  Create it
anonymous, and publish it only after setting the key?

We'd have to do the same for the command line, of course.

Incompatible change, but since this stuff doesn't really work and really
shouldn't be used...

>> The user can set a key with HMP / QMP command block_passwd.  If it
>> succeeds, state goes from NEEDKEY to GOTKEY.
>> 
>> You can't unpause a guest while a BlockDriverState is in state NEEDKEY:
>> 
>> * QMP command cont fails if any BlockDriverState is in state NEEDKEY.
>> 
>> * HMP command cont tries to be cute then: it picks a BlockDriverState in
>>   state NEEDKEY and prompts for its key.  If this is the only one in
>>   state NEEDKEY, it unpauses the guest as ordered.  Else it silently
>>   doesn't.  The user is expected to know to rerun cont for the next key
>>   prompt.
>
> Arguably, the HMP behavior could be made smarter, to do the loop itself.

Yes, if we stick to the "NEEDKEY tied to guest paused" model, we should
do that.

>> This might make you think that the guest is protected from ever having a
>> block device backed by a BlockDriverState in state NEEDKEY.  Not true!
>> Simply open an image while the guest runs, and hot-plug a device backed
>> by it.  The guest will happily read encrypted garbage from it, and if it
>> writes anything to it, it's not so encrypted anymore.  To add insult to
>> injury, the "protection" kicks in next time you pause the guest.

This one is distinct from the change flaws below, and at least as
serious.

>> Changing media has its wrinkles, too:
>> 
>> * HMP command change first closes the old image, then opens the new
>>   image, and if it's encrypted, it asks for a key.  Okay.
>> 
>> * QMP command change first opens the image, then errors out if it's
>>   encrypted and the password argument is missing, or it's not encrypted
>>   and the password argument is present.  These two errors aren't really
>>   failures; the change succeeds just fine.
>
> Indeed, the 'change' command specifically documents that using 'change'
> on a running guest with an encrypted image is ill-advised, because
> there's no password provided along with the 'change' action, so the user
> is forced to do a 2-command sequence where any window with the guest
> running is hosed before the followup 'block_passwd'.

Mitigating factor: the device model (and thus the guest) is notified of
the eject right away, but it's notified of the load only when the key
gets set.  I figure this reduces, but does not eliminate the risk of
disaster.

> Question: when changing from one encrypted image to another, does the
> second image inherit the password from the first (whether or not it
> decrypts the second), or do we always clean state back to having no
> known password?  The way you worded it, it sounds like the password is
> inherited from the first media to the second?

Clean slate.  Ejecting media puts the BDS into a "has no image" state.
"Both [encrypted and valid_key] are unset when BlockDriverState has no
image".

>>   Clients can detect the former non-failure error (it's class
>>   "DeviceEncrypted"), and use block_passwd to go to state GOTKEY.
>
> This makes sense whether you go from unencrypted to encrypted media, or
> if going to an encrypted media always wipes out the old password state.

Clients need to know that DeviceEncrypted is not a failure in the sense
that changing media failed.  It succeeded, but the new media still needs
a key.

Good command design avoids failing after a visible state transition.

>>   Clients can't reliably distinguish the latter non-failure error from
>>   real errors, because it's class "GenericError".
>
> It seems like this error (providing a password argument for a
> non-encrypted disk) is not possible via the 'change' command.  How
> exactly is this state even triggered?

You're right; this code in qmp_bdrv_open_encrypted() is currently
unreachable.

>> In any case, callback bdrv_dev_change_media_cb() is delayed for
>> encrypted images until we enter state GOTKEY.  As afar as I can tell,
>> nothing stops the device model from reading or writing before it gets
>> the callback, but that would be a device model bug.
>
> And one that the qapi docs specifically warn against.
>
>> 
>> The final funny is device model usb-storage (another one that
>> desperately needs to be buried deep).  Its init() callback
>> usb_msd_initfn_storage() tries to be cute when it detects state NEEDKEY.
>> 
>> If it's running in monitor context (say in HMP/QMP command device_add),
>> it attempts to ask for a key.  In HMP context, it unplugs itself when
>> this fails (I think).  In QMP context, it behaves similar to change: it
>> works, but you get a "DeviceEncrypted" error, and the backend remains in
>> state NEEDKEY.
>> 
>> If it's not running in monitor context, it clears autostart.  No idea
>> why it does that, and I'm not sure it has any effect.  Opening an
>> encrypted image clears autostart already, in blockdev_init().

Can we get rid of this, and make usb-storage behave like any other
hot-pluggable block device model?

>> Thankfully, usb-storage is the only device model that messes with keys.
>> 
>> Questions:
>> 
>> 1. Should we protect guests from state NEEDKEY?
>
> Ideally, yes.
>
>> 
>> 2. If yes, how?
>> 
>>    Pause the guest when something enters state NEEDKEY?  I'd hate that.
>> 
>>    Fail device_add in state NEEDKEY?  Takes care of hot-plug, and
>>    cold-plug is already protected by cont.
>
> The existing QMP 'change' command is stupid.  Add a new 'disk-change'
> command that takes a password argument, and require that the new media
> either be unencrypted (and error if a password was provided) or fully
> reach the GOTKEY state (and error if no password was provided) for the
> command to successfully change the media.

The basic operations are "eject" (open tray, do not touch medium),
"load" (close tray, do not touch medium), "remove-medium" (tray must be
open) and "insert-medium" (likewise).

If more convenience is wanted in QMP, build a command on top that
combines the three into whatever behavior is deemed convenient.

Agree that insert-medium should fail if the BDS is encrypted and the
password argument is missing.  The command should have no effect when it
fails that way.  Easy, because insert-medium does just one thing.  Not
so easy for a command that does several things in a sequence.

I don't care for rejecting a superfluous password argument.  If we do
reject it, I again want "command has no effect when it fails".

>                                            Similarly for hotplug.  That
> is, enforce that hot-plug can never make a NEEDKEY block device visible
> to the guest.

Thus, blockdev-add should match insert-medium: optional parameter
password, fail when the BDS is encrypted and the password is missing,
fail when the BDS is unencrypted and the password is present (if we want
that), command has no effect when it fails.

>> Other bright ideas?
>
> Use the fact that we are calling the next release "2.0" to actually kill
> qemu disk encryption as a horribly botched feature, on the grounds that
> we are doing users a favor by not letting them use broken encryption?

I'd love to wipe the encrypted image slate clean and start over!

Keep encrypted images working in qemu-img, of course, so users can
salvage their "encrypted" data.

Re: [Qemu-devel] The unholy encrypted image key mess

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 28/02/2014 23:08, Eric Blake ha scritto:
>> Use the fact that we are calling the next release "2.0" to actually kill
>> qemu disk encryption as a horribly botched feature, on the grounds that
>> we are doing users a favor by not letting them use broken encryption?
>
> Only for qemu, of course---qemu-img would still have support for
> encryption, and there's no reason to risk stability by removing all
> the monitor code right now.

Right now = for 2.0?

I'm not trying to push anything into 2.0.  I'm trying to clean up
another mess (qerror, to be precise), and the encrypted images mess is
in my way.  I don't expect to complete the qerror job in time for 2.0.

> However, wouldn't we have the same problems even with a sane encrypted
> image format (based on LUKS, for example)?

Yes, and when we get that, we'll shoehorn it into the same idiotic user
interface we have now :)

> Let's just open bugs (oh if only Launchpad supported tracker bugs) for now.

Filing bugs won't help me with cleaning up qerror.  If preserving the
current idiotic encrypted image interfaces is required, I'll have no
choice but pour in the necessary work.  Sure wish I could use the time
for something more immediately useful than preserving an idiotic
interface to a feature we don't want anybody to use.

Re: [Qemu-devel] The unholy encrypted image key mess

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 28.02.2014 um 22:01 hat Markus Armbruster geschrieben:
>> Questions:
>> 
>> 1. Should we protect guests from state NEEDKEY?
>
> Yes. An image in state NEEDKEY isn't fully initialised, so we should
> make sure that it isn't used.

Consensus.

>> 2. If yes, how?
>> 
>>    Pause the guest when something enters state NEEDKEY?  I'd hate that.
>> 
>>    Fail device_add in state NEEDKEY?  Takes care of hot-plug, and
>>    cold-plug is already protected by cont.
>
> 'device_add' should refuse to accept a backend that isn't fully
> initialised, so yes, I agree.

bdrv_attach_dev() could fail then.

> 'change' is a bit trickier because it involves several low-level actions
> at once, and device_add is not one of them.

The problem is that it can run while a device model is attached.

>                                             What we probably really need
> to do is support a state where no BDS is attached to the device
> emulation (a BlockBackend might still be attached, not sure about this
> one), but the VM is still running. And then 'change' can detach the BDS,
> bring it back to the NEEDKEY state (unrealize in QOM speech?), magic
> happens and then we reattach the BDS to the guest device.

In a BlockBackend world, the device model attaches to a BlockBackend,
and the BlockBackend may have a BDS[*].  No BDS means no media.
Removing media takes away the BDS and destroys it.  Inserting media
creates a new BDS and gives it to the BlockBackend.

To avoid visible NEEDKEY, delay the giving until we reach GOTKEY.

> We'll also want to protect other parts of qemu from not fully
> initialised BDSes, e.g. block jobs or NBD servers shouldn't take such an
> BDS as their source.

Good point.  Shows that the NEEDKEY problems are more pervasive than I
thought.

What shall we do about them?

We can ship yet another release with known open deathtraps around
encrypted images, only this time with a warning against it buried in the
documentation for people not to read.

Or we can do our users a favor and kill support for encrypted images
dead (except in qemu-img, of course).  Revive when we have encryption
that actually works, both in the safety and security sense.

Opinions?


[*] Actually: root of a BDS tree (or even BDS DAG?)

Re: [Qemu-devel] The unholy encrypted image key mess

[Paolo Bonzini]

Il 05/03/2014 09:24, Markus Armbruster ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> Il 28/02/2014 23:08, Eric Blake ha scritto:
>>> Use the fact that we are calling the next release "2.0" to actually kill
>>> qemu disk encryption as a horribly botched feature, on the grounds that
>>> we are doing users a favor by not letting them use broken encryption?
>>
>> Only for qemu, of course---qemu-img would still have support for
>> encryption, and there's no reason to risk stability by removing all
>> the monitor code right now.
>
> Right now = for 2.0?

Possibly:

diff --git a/block.c b/block.c
index 38bbdf3..794946c 100644
--- a/block.c
+++ b/block.c
@@ -1384,6 +1384,12 @@ done:
      }
      QDECREF(options);

+    if (bdrv_key_required(bs) && use_bdrv_whitelist) {
+        ret = -EINVAL;
+        error_setg(errp, "Encrypted images are not supported by QEMU "
+                   "anymore.\nPlease use qemu-img to unencrypt them.");
+        goto close_nad_fail;
+    }
      if (!bdrv_key_required(bs)) {
          bdrv_dev_change_media_cb(bs, true);
      }


> I'm not trying to push anything into 2.0.  I'm trying to clean up
> another mess (qerror, to be precise), and the encrypted images mess is
> in my way.  I don't expect to complete the qerror job in time for 2.0.

What are you cleaning up exactly?

>> However, wouldn't we have the same problems even with a sane encrypted
>> image format (based on LUKS, for example)?
>
> Yes, and when we get that, we'll shoehorn it into the same idiotic user
> interface we have now :)
>
>> Let's just open bugs (oh if only Launchpad supported tracker bugs) for now.
>
> Filing bugs won't help me with cleaning up qerror.  If preserving the
> current idiotic encrypted image interfaces is required, I'll have no
> choice but pour in the necessary work.  Sure wish I could use the time
> for something more immediately useful than preserving an idiotic
> interface to a feature we don't want anybody to use.

I'm not sure why it helps to kill an idiotic user interface, if we (a) 
have plans to reimplement the same feature in the future (b) don't have 
any ideas on how to have a non-idiotic user interface for the same feature.

Fixing it, at least partly---that helps.  But killing it doesn't help, 
unless we drop all plans to have sane image encryption.

Paolo

Re: [Qemu-devel] The unholy encrypted image key mess

[Paolo Bonzini]

Il 05/03/2014 09:43, Markus Armbruster ha scritto:
> Or we can do our users a favor and kill support for encrypted images
> dead (except in qemu-img, of course).  Revive when we have encryption
> that actually works, both in the safety and security sense.

I think this is okay, since the patch should hopefully be quite small.

"change" would probably leave the disk without a medium; not a big deal.

Paolo

Re: [Qemu-devel] The unholy encrypted image key mess

[Gerd Hoffmann]

  Hi,

> I can't see why QMP commands would ever want to create in state NEEDKEY.
> We could easily avoid it there: give QMP commands creating
> BlockDriverStates an optional password parameter, fail the command if
> the BDS is encrypted and the password parameter is missing.

Fully agree.

The change command kind-of makes sense for encrypted block devices
specified on the command line.  By passing the password via monitor
command it can't be captured easily (unlike for example passwords
specified on the command line which show up in ps listings).

> For HMP, we need to make up our minds how to do passwords.
> 
> The current way is to tie NEEDKEY to "guest paused".  I hate that.

I'd still tend to simply remove (or disable for 2.0, remove later on)
encryption support from qemu (keep in qemu-img).

Then worry how to do it right once we add a sane encrypted format.

> >> The final funny is device model usb-storage (another one that
> >> desperately needs to be buried deep).  Its init() callback
> >> usb_msd_initfn_storage() tries to be cute when it detects state NEEDKEY.
> >> 
> >> If it's running in monitor context (say in HMP/QMP command device_add),
> >> it attempts to ask for a key.  In HMP context, it unplugs itself when
> >> this fails (I think).  In QMP context, it behaves similar to change: it
> >> works, but you get a "DeviceEncrypted" error, and the backend remains in
> >> state NEEDKEY.
> >> 
> >> If it's not running in monitor context, it clears autostart.  No idea
> >> why it does that, and I'm not sure it has any effect.  Opening an
> >> encrypted image clears autostart already, in blockdev_init().

IIRC to allow the user specify the password before starting the guest.
And, yes, if blockdev_init does that anyway it most likely is not
needed.

> Can we get rid of this, and make usb-storage behave like any other
> hot-pluggable block device model?

The dance is needed for the legacy usb_add monitor command, which
creates blockdev + usb-storage in one go.

If we go zap encryption altogether we can obviously simply remove that.
Otherwise it should be possible to move that logic into the legacy usb
init function (usb_msd_init) so it will never ever kick in with -device.

cheers,
  Gerd

Re: [Qemu-devel] The unholy encrypted image key mess

[Andreas Färber]

Am 05.03.2014 09:43, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> 'change' is a bit trickier because it involves several low-level actions
>> at once, and device_add is not one of them.
> 
> The problem is that it can run while a device model is attached.

What exactly is the problem here? 'change' is for floppy and CD, right?
The device stays realized/connected and guest-visible during that time,
and the guest should be able to live with no media while the new drive
is not yet decrypted. Are we just lacking a QMP command to enter the
password and complete the change while the guest is executing?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] The unholy encrypted image key mess

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 05/03/2014 09:24, Markus Armbruster ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Il 28/02/2014 23:08, Eric Blake ha scritto:
>>>> Use the fact that we are calling the next release "2.0" to actually kill
>>>> qemu disk encryption as a horribly botched feature, on the grounds that
>>>> we are doing users a favor by not letting them use broken encryption?
>>>
>>> Only for qemu, of course---qemu-img would still have support for
>>> encryption, and there's no reason to risk stability by removing all
>>> the monitor code right now.
>>
>> Right now = for 2.0?
>
> Possibly:
>
> diff --git a/block.c b/block.c
> index 38bbdf3..794946c 100644
> --- a/block.c
> +++ b/block.c
> @@ -1384,6 +1384,12 @@ done:
>      }
>      QDECREF(options);
>
> +    if (bdrv_key_required(bs) && use_bdrv_whitelist) {
> +        ret = -EINVAL;
> +        error_setg(errp, "Encrypted images are not supported by QEMU "
> +                   "anymore.\nPlease use qemu-img to unencrypt them.");
> +        goto close_nad_fail;
> +    }
>      if (!bdrv_key_required(bs)) {
>          bdrv_dev_change_media_cb(bs, true);
>      }

Okay, I guess that's simple enough to be considered for 2.0.

>> I'm not trying to push anything into 2.0.  I'm trying to clean up
>> another mess (qerror, to be precise), and the encrypted images mess is
>> in my way.  I don't expect to complete the qerror job in time for 2.0.
>
> What are you cleaning up exactly?

I want to get rid of qerror_report() & friends.

qerror_report() is a transitional interface to help with converting
existing HMP commands to QMP.  It should not be used elsewhere.
However, uses have crept into many places, and they keep creeping.

Ever since we added the latest error infrastructure, we planned to get
rid of qerror_report() "eventually".  I'm working on turning
"eventually" into "actually".  Got stuck in the messy code dealing with
encryption keys, and had to figure out how it actually works.  Where
"works" is strictly in the sense of "what it does", because what it does
doesn't actually work, at least not reliably.

>>> However, wouldn't we have the same problems even with a sane encrypted
>>> image format (based on LUKS, for example)?
>>
>> Yes, and when we get that, we'll shoehorn it into the same idiotic user
>> interface we have now :)
>>
>>> Let's just open bugs (oh if only Launchpad supported tracker bugs) for now.
>>
>> Filing bugs won't help me with cleaning up qerror.  If preserving the
>> current idiotic encrypted image interfaces is required, I'll have no
>> choice but pour in the necessary work.  Sure wish I could use the time
>> for something more immediately useful than preserving an idiotic
>> interface to a feature we don't want anybody to use.
>
> I'm not sure why it helps to kill an idiotic user interface, if we (a)
> have plans to reimplement the same feature in the future (b) don't
> have any ideas on how to have a non-idiotic user interface for the
> same feature.

I think this thread has proven (b) false.  We may not have a complete
plan, but what we have is certainly more than not having any ideas.

> Fixing it, at least partly---that helps.  But killing it doesn't help,
> unless we drop all plans to have sane image encryption.

Fixing helps if the current state can be evolved into the desired state,
and reaching the desired state by evolution from the current state is a
reasonably efficient way to get there.

Killing helps if the current state is a dead end.

I think as far as interfaces are concerned, we should start over.  The
existing interfaces are too badly designed to serve as a basis.

Once we have sane interfaces designed, we can find out whether the
existing code can be productively evolved to implement them, and what it
would take to preserve the existing idiotic interfaces for backward
compatibility.  Then decide whether we want to do either.

Doing all this work feels pointless to me without a use case, and a real
encryption machinery to interface to.

Before we work on the latter, I'd like to see some of the former.  LUKS
works in guests, too.

Re: [Qemu-devel] The unholy encrypted image key mess

[Kevin Wolf]

Am 05.03.2014 um 09:15 hat Markus Armbruster geschrieben:
> Eric Blake <eblake@redhat.com> writes:
> > Uggh - so there's no current way to hot-plug a device in state GOTKEY
> > short of using a two-command sequence?  It would be nicer if hot-plug
> > had a way to fail to add encrypted devices unless the user also passes
> > the password at the same time, creating the device directly into the
> > GOTKEY state.
> 
> I can't see why QMP commands would ever want to create in state NEEDKEY.
> We could easily avoid it there: give QMP commands creating
> BlockDriverStates an optional password parameter, fail the command if
> the BDS is encrypted and the password parameter is missing.

Yes.

> For HMP, we need to make up our minds how to do passwords.
> 
> The current way is to tie NEEDKEY to "guest paused".  I hate that.
> 
> Another way is to make the commands adding BDS prompt for necessary
> passwords.  We still have to deal with state NEEDKEY while we're waiting
> for the user's reply.  Need to take care to hide the new BDS.  Create it
> anonymous, and publish it only after setting the key?

HMP is just a QMP user, so if QMP never creates images in a NEEDKEY
state, HMP doesn't either. And that's fine: Let QMP return an error
("this needs a password and you didn't specify one") and then ask the
user for a password and retry.

Solves the whole problem with the NEEDKEY state by eliminating it. I'm
not entirely sure if NEEDKEY is the only state of not fully initialised
BDSes, but perhaps it really is.

> We'd have to do the same for the command line, of course.

This one could become a bit trickier because you'd have to ask for the
password not only before you let the VM run, but even before you create
the virtual disk devices.

> Incompatible change, but since this stuff doesn't really work and really
> shouldn't be used...

I'm not even sure if it is incompatible on the external interface. HMP
would be similar enough, and QMP already returns errors for encrypted
disks in qmp_change_blockdev() and has no pre-2.0 interface for
hotplugging disks.

Oh right, that error is a non-error and the block device is created
anyway. This is stupid, we'd have to make an incompatible change to
change this into a real error.

We also need to make sure that blockdev-add fails on encrypted images if
no password is given, this may still be missing.

Kevin

Re: [Qemu-devel] The unholy encrypted image key mess

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 05.03.2014 09:43, schrieb Markus Armbruster:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>>> 'change' is a bit trickier because it involves several low-level actions
>>> at once, and device_add is not one of them.
>> 
>> The problem is that it can run while a device model is attached.
>
> What exactly is the problem here? 'change' is for floppy and CD, right?
> The device stays realized/connected and guest-visible during that time,
> and the guest should be able to live with no media while the new drive
> is not yet decrypted. Are we just lacking a QMP command to enter the
> password and complete the change while the guest is executing?

Here's how change works (both HMP and QMP):

1. bdrv_close(); now we have no medium.

2. If a device model is attached, tell it the medium has been ejected.
   A device model supporting notification (anything but floppy, I guess)
   in turn notifies the guest.

3. bdrv_open(); now we again have a medium, we're in state NOKEY if the
   image is unencrypted, and in state NEEDKEY if it's encrypted.

4. Only in state NOKEY: if a device model is attached, tell it a medium
   has been loaded.  A device model supporting notification in turn
   notifies the guest.

5. Only in state NEEDKEY:

   If QMP, fail the command with error DeviceEncrypted.

   If HMP, prompt user for password.

In QMP, the client needs to follow up with a block_passwd command.  In
HMP, the user neeeds to enter a password.  In both cases, time passes,
guest can run, and even other monitor commands can be executed, possibly
in other monitors.  When the password arrives:

7. Set the key, state is now GOTKEY.

8. If a device model is attached, tell it a medium has been loaded.  A
   device model supporting notification in turn notifies the guest.

Shit happens for encrypted images if anything reads or writes between
3. and 8.

Re: [Qemu-devel] The unholy encrypted image key mess

[Paolo Bonzini]

Il 05/03/2014 11:36, Markus Armbruster ha scritto:
> Andreas Färber <afaerber@suse.de> writes:
>
>> Am 05.03.2014 09:43, schrieb Markus Armbruster:
>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>
>>>> 'change' is a bit trickier because it involves several low-level actions
>>>> at once, and device_add is not one of them.
>>>
>>> The problem is that it can run while a device model is attached.
>>
>> What exactly is the problem here? 'change' is for floppy and CD, right?
>> The device stays realized/connected and guest-visible during that time,
>> and the guest should be able to live with no media while the new drive
>> is not yet decrypted. Are we just lacking a QMP command to enter the
>> password and complete the change while the guest is executing?
>
> Here's how change works (both HMP and QMP):
>
> 1. bdrv_close(); now we have no medium.
>
> 2. If a device model is attached, tell it the medium has been ejected.
>    A device model supporting notification (anything but floppy, I guess)
>    in turn notifies the guest.
>
> 3. bdrv_open(); now we again have a medium, we're in state NOKEY if the
>    image is unencrypted, and in state NEEDKEY if it's encrypted.
>
> 4. Only in state NOKEY: if a device model is attached, tell it a medium
>    has been loaded.  A device model supporting notification in turn
>    notifies the guest.
>
> 5. Only in state NEEDKEY:
>
>    If QMP, fail the command with error DeviceEncrypted.
>
>    If HMP, prompt user for password.
>
> In QMP, the client needs to follow up with a block_passwd command.  In
> HMP, the user neeeds to enter a password.  In both cases, time passes,
> guest can run, and even other monitor commands can be executed, possibly
> in other monitors.  When the password arrives:
>
> 7. Set the key, state is now GOTKEY.
>
> 8. If a device model is attached, tell it a medium has been loaded.  A
>    device model supporting notification in turn notifies the guest.
>
> Shit happens for encrypted images if anything reads or writes between
> 3. and 8.

You could use bdrv_new before step 3, to open the new medium on a new 
temporary BDS.  Then before step 8 you use bdrv_swap and delete the 
temporary BDS.

Paolo

Re: [Qemu-devel] The unholy encrypted image key mess

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 05.03.2014 um 09:15 hat Markus Armbruster geschrieben:
>> Eric Blake <eblake@redhat.com> writes:
>> > Uggh - so there's no current way to hot-plug a device in state GOTKEY
>> > short of using a two-command sequence?  It would be nicer if hot-plug
>> > had a way to fail to add encrypted devices unless the user also passes
>> > the password at the same time, creating the device directly into the
>> > GOTKEY state.
>> 
>> I can't see why QMP commands would ever want to create in state NEEDKEY.
>> We could easily avoid it there: give QMP commands creating
>> BlockDriverStates an optional password parameter, fail the command if
>> the BDS is encrypted and the password parameter is missing.
>
> Yes.
>
>> For HMP, we need to make up our minds how to do passwords.
>> 
>> The current way is to tie NEEDKEY to "guest paused".  I hate that.
>> 
>> Another way is to make the commands adding BDS prompt for necessary
>> passwords.  We still have to deal with state NEEDKEY while we're waiting
>> for the user's reply.  Need to take care to hide the new BDS.  Create it
>> anonymous, and publish it only after setting the key?
>
> HMP is just a QMP user, so if QMP never creates images in a NEEDKEY
> state, HMP doesn't either. And that's fine: Let QMP return an error
> ("this needs a password and you didn't specify one") and then ask the
> user for a password and retry.
>
> Solves the whole problem with the NEEDKEY state by eliminating it. I'm
> not entirely sure if NEEDKEY is the only state of not fully initialised
> BDSes, but perhaps it really is.

The qmp command needs to be fixed to truly fail when a password is
missing, not create a BDS in state NEEDKEY and fail.  But I think we
want that change anyway, incompatible or not.

>> We'd have to do the same for the command line, of course.
>
> This one could become a bit trickier because you'd have to ask for the
> password not only before you let the VM run, but even before you create
> the virtual disk devices.

Yes.  Ask right away, and right on stdout/stdin.  Improvement for human
users, because then they don't have to hunt for the monitor (which may
not be on stdout/stdin).

Possible show-stopper for tools.  Right now, the way to start a guest
with encrypted images is with -S, supply all passwords via QMP, then
cont.

Not sure how to best reconcile the needs of humans and tools here.

>> Incompatible change, but since this stuff doesn't really work and really
>> shouldn't be used...
>
> I'm not even sure if it is incompatible on the external interface. HMP
> would be similar enough, and QMP already returns errors for encrypted
> disks in qmp_change_blockdev() and has no pre-2.0 interface for
> hotplugging disks.
>
> Oh right, that error is a non-error and the block device is created
> anyway. This is stupid, we'd have to make an incompatible change to
> change this into a real error.

Yes.

> We also need to make sure that blockdev-add fails on encrypted images if
> no password is given, this may still be missing.

Yes.

Re: [Qemu-devel] The unholy encrypted image key mess

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 05/03/2014 11:36, Markus Armbruster ha scritto:
>> Andreas Färber <afaerber@suse.de> writes:
>>
>>> Am 05.03.2014 09:43, schrieb Markus Armbruster:
>>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>>
>>>>> 'change' is a bit trickier because it involves several low-level actions
>>>>> at once, and device_add is not one of them.
>>>>
>>>> The problem is that it can run while a device model is attached.
>>>
>>> What exactly is the problem here? 'change' is for floppy and CD, right?
>>> The device stays realized/connected and guest-visible during that time,
>>> and the guest should be able to live with no media while the new drive
>>> is not yet decrypted. Are we just lacking a QMP command to enter the
>>> password and complete the change while the guest is executing?
>>
>> Here's how change works (both HMP and QMP):
>>
>> 1. bdrv_close(); now we have no medium.
>>
>> 2. If a device model is attached, tell it the medium has been ejected.
>>    A device model supporting notification (anything but floppy, I guess)
>>    in turn notifies the guest.
>>
>> 3. bdrv_open(); now we again have a medium, we're in state NOKEY if the
>>    image is unencrypted, and in state NEEDKEY if it's encrypted.
>>
>> 4. Only in state NOKEY: if a device model is attached, tell it a medium
>>    has been loaded.  A device model supporting notification in turn
>>    notifies the guest.
>>
>> 5. Only in state NEEDKEY:
>>
>>    If QMP, fail the command with error DeviceEncrypted.
>>
>>    If HMP, prompt user for password.
>>
>> In QMP, the client needs to follow up with a block_passwd command.  In
>> HMP, the user neeeds to enter a password.  In both cases, time passes,
>> guest can run, and even other monitor commands can be executed, possibly
>> in other monitors.  When the password arrives:
>>
>> 7. Set the key, state is now GOTKEY.
>>
>> 8. If a device model is attached, tell it a medium has been loaded.  A
>>    device model supporting notification in turn notifies the guest.
>>
>> Shit happens for encrypted images if anything reads or writes between
>> 3. and 8.
>
> You could use bdrv_new before step 3, to open the new medium on a new
> temporary BDS.  Then before step 8 you use bdrv_swap and delete the
> temporary BDS.

If we can't get rid of state NEEDKEY entirely, then we need to hide BDSs
in this state from anything but the QMP command to set the key.

As long as we don't have BlockBackend, we need to do something like you
described to keep NEEDKEY hidden from the device model even though the
device model has a direct reference to its backing BDS.