| Message ID | 1343869374-23417-14-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
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()? > } > > 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.
Il 02/08/2012 15:27, Markus Armbruster ha scritto: >> > + 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; >> > + } >> > + } Is this anything that an external application can reproduce? Perhaps we need to keep QERR_DEVICE_ENCRYPTED even if libvirt does not use it, or at least provide an alternative mechanism (e.g. an event) to realize its effect. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 02/08/2012 15:27, Markus Armbruster ha scritto: >>> > + 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; >>> > + } >>> > + } > > Is this anything that an external application can reproduce? > > Perhaps we need to keep QERR_DEVICE_ENCRYPTED even if libvirt does not > use it, or at least provide an alternative mechanism (e.g. an event) to > realize its effect. Not sure I get you. External applications should use QMP. A sane way to change media in QMP needs to provide the key as argument. If the key is wrong, fail the change cleanly. In particular, don't eject then.
Il 02/08/2012 15:53, Markus Armbruster ha scritto: >> > Is this anything that an external application can reproduce? >> > >> > Perhaps we need to keep QERR_DEVICE_ENCRYPTED even if libvirt does not >> > use it, or at least provide an alternative mechanism (e.g. an event) to >> > realize its effect. > Not sure I get you. > > External applications should use QMP. Yes. Is there a conceivable scenario in which a QMP client needs to look at QERR_DEVICE_ENCRYPTED? > A sane way to change media in QMP needs to provide the key as argument. How does the QMP client know that the device is encrypted, and thus they have to ask for the key and retry? Same for hmp_cont, by the way. > If the key is wrong, fail the change cleanly. In particular, don't > eject then. Agreed on not ejecting, but this is orthogonal. Paolo
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. >
On Thu, 02 Aug 2012 15:46:48 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 02/08/2012 15:27, Markus Armbruster ha scritto: > >> > + 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; > >> > + } > >> > + } > > Is this anything that an external application can reproduce? Yes, that should be possible. But thinking a bit more about this, the real question is whether we want them to do it. I guess not, as the fact that qmp_bdrv_open_encrypted() doesn't close the bs on error is probably a bug. > Perhaps we need to keep QERR_DEVICE_ENCRYPTED even if libvirt does not > use it, or at least provide an alternative mechanism (e.g. an event) to > realize its effect. Yes, maybe that's better. hmp_cont() wouldn't change much though, as it still needs to figure out which device needs a key.
On Thu, 02 Aug 2012 15:57:24 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 02/08/2012 15:53, Markus Armbruster ha scritto: > >> > Is this anything that an external application can reproduce? > >> > > >> > Perhaps we need to keep QERR_DEVICE_ENCRYPTED even if libvirt does not > >> > use it, or at least provide an alternative mechanism (e.g. an event) to > >> > realize its effect. > > Not sure I get you. > > > > External applications should use QMP. > > Yes. Is there a conceivable scenario in which a QMP client needs to > look at QERR_DEVICE_ENCRYPTED? > > > A sane way to change media in QMP needs to provide the key as argument. > > How does the QMP client know that the device is encrypted, and thus they > have to ask for the key and retry? It would be interesting to know how libvirt handles this. > > Same for hmp_cont, by the way. > > > If the key is wrong, fail the change cleanly. In particular, don't > > eject then. > > Agreed on not ejecting, but this is orthogonal. > > Paolo >
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)); } 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. + */ + 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)
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(-)