Re: [Qemu-devel] [PATCH 13/34] hmp: hmp_change(): don't rely on QERR_DEVICE_ENCRYPTED

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 02 Aug 2012 15:27:33 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > This commit changes the way hmp_change() checks if an encryption key
> > is required for the device to be inserted.
> >
> > Instead of checking for QERR_DEVICE_ENCRYPTED, hmp_change() now checks
> > if the device was successfully inserted, is encrypted and is missing
> > an encryption key.
> >
> > This change is needed because QERR_DEVICE_ENCRYPTED is going to be
> > dropped by a future commit.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  hmp.c | 54 ++++++++++++++++++++++++++++++++++++++----------------
> >  1 file changed, 38 insertions(+), 16 deletions(-)
> >
> > diff --git a/hmp.c b/hmp.c
> > index 1ebeb63..ea21cf7 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -783,17 +783,29 @@ static void hmp_change_read_arg(Monitor *mon, const char *password,
> >  static void cb_hmp_change_bdrv_pwd(Monitor *mon, const char *password,
> >                                     void *opaque)
> >  {
> > -    Error *encryption_err = opaque;
> > +    char *device = opaque;
> >      Error *err = NULL;
> > -    const char *device;
> > -
> > -    device = error_get_field(encryption_err, "device");
> >  
> >      qmp_block_passwd(device, password, &err);
> >      hmp_handle_error(mon, &err);
> > -    error_free(encryption_err);
> >  
> >      monitor_read_command(mon, 1);
> > +    g_free(device);
> > +}
> > +
> > +static void hmp_change_ask_user_key(Monitor *mon, const BlockInfo *binfo)
> > +{
> > +    monitor_printf(mon, "%s (%s) is encrypted.\n", binfo->device,
> > +                   binfo->inserted->file);
> > +
> > +    if (!monitor_get_rs(mon)) {
> > +        monitor_printf(mon,
> > +                "terminal does not support password prompting\n");
> > +        return;
> > +    }
> > +
> > +    readline_start(monitor_get_rs(mon), "Password: ", 1,
> > +                   cb_hmp_change_bdrv_pwd, g_strdup(binfo->device));
> 
> Why can't you use monitor_read_password() here?  Or even
> monitor_read_bdrv_key_start()?

Should be possible, I just did what the current version does.

> 
> >  }
> >  
> >  void hmp_change(Monitor *mon, const QDict *qdict)
> > @@ -801,6 +813,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
> >      const char *device = qdict_get_str(qdict, "device");
> >      const char *target = qdict_get_str(qdict, "target");
> >      const char *arg = qdict_get_try_str(qdict, "arg");
> > +    BlockInfoList *bdev_list = NULL, *bdev;
> >      Error *err = NULL;
> >  
> >      if (strcmp(device, "vnc") == 0 &&
> > @@ -813,21 +826,30 @@ void hmp_change(Monitor *mon, const QDict *qdict)
> >      }
> >  
> >      qmp_change(device, target, !!arg, arg, &err);
> > -    if (error_is_type(err, QERR_DEVICE_ENCRYPTED)) {
> > -        monitor_printf(mon, "%s (%s) is encrypted.\n",
> > -                       error_get_field(err, "device"),
> > -                       error_get_field(err, "filename"));
> > -        if (!monitor_get_rs(mon)) {
> > -            monitor_printf(mon,
> > -                    "terminal does not support password prompting\n");
> > +    if (error_is_set(&err)) {
> > +        /* qmp_change() failed. If 'device' is returned by qmp_query_block(),
> > +         * is encrypted and doesn't have a valid encryption key set, then
> > +         * either the user passed an invalid key or didn't pass one at all.
> > +         * Ask the user for the key.
> > +         */
> 
> Is it even possible to pass a key?

Looks like you can't, I think I confused myself with change vnc.

> 
> > +        bdev_list = qmp_query_block(NULL);
> > +        for (bdev = bdev_list; bdev; bdev = bdev->next) {
> > +            if (!strcmp(bdev->value->device, device) &&
> > +                blockinfo_is_encrypted(bdev->value) &&
> > +                !blockinfo_key_is_set(bdev->value)) {
> > +                hmp_change_ask_user_key(mon, bdev->value);
> > +                break;
> > +            }
> > +        }
> > +
> > +        if (bdev) {
> >              error_free(err);
> > -            return;
> > +            err = NULL;
> >          }
> > -        readline_start(monitor_get_rs(mon), "Password: ", 1,
> > -                       cb_hmp_change_bdrv_pwd, err);
> > -        return;
> >      }
> > +
> >      hmp_handle_error(mon, &err);
> > +    qapi_free_BlockInfoList(bdev_list);
> >  }
> >  
> >  void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
> 
> Ourside the scope of this patch, but here goes anyway: I wonder what
> happens when you enter an incorrect key.

I remember testing this ages ago (even before QMP if my memory is not failing
me) and no error is reported.

> If bdrv_set_key() fails, it returns
> 
> 1. -EINVAL if the image isn't encrypted
> 
> 2. -ENOMEDIUM under conditions that look like they shouldn't happen
> 
>    Possibly a bug.
> 
> 3. Whatever the block driver's bdrv_set_key() methods returned
> 
>    Both existing methods qcow2_set_key() and qcow_set_key() return -1 on
>    failure, not a negative errno.  Bug.
> 
>    I believe they can fail only when passed invalid parameters, which
>    isn't the case.  In other words, they both accept *any* key.  If it's
>    wrong, reads will produce garbage, and writes will destroy the image.
> 
> qmp_block_passwd() sets QERR_DEVICE_ENCRYPTED on -EINVAL, else
> QERR_INVALID_PASSWORD.
> 
> hmp_handle_error() prints the message.
> 
> The media change takes effect regardless of bdrv_set_key() failures!  I
> believe it takes effect first thing in qmp_bdrv_open_encrypted(), before
> we even check whether a key is required, let alone prompt for it.

Not sure I follow you, as far as I understand it the problem is that
bdrv_set_key() doesn't return an error for an invalid key.

> 
> If the guest is running, I suspect it'll happily read and write with a
> zero AES_KEY.  Reads produce garbarge, and writes destroy the image.
> 
> Problem exists both in HMP (which asks for password) and QMP (which
> doesn't even try to supply one).  change with a block device argument is
> unsafe unless the guest is stopped.
> 
> Encryption is yet another half-baked feature that shouldn't have been
> committed.
> 
> And I hate the change command, too.
>