Luiz Capitulino <lcapitul...@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 <lcapitul...@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()?
> }
>
> 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.
