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

[Markus Armbruster]

Luiz Capitulino <address@hidden> 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 <address@hidden>
> ---
>  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()?

>  }
>  
>  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?

> +        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.

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.

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.