On Thu, 02 Aug 2012 15:27:33 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> 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()?
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.
>
