1. Patchew
  2. QEMU
  3. [Qemu-devel] [RFC v10 00/24] Multifd
  4. View patch

[Qemu-devel] [PATCH v10 10/24] migration: In case of error just end the migration

Juan Quintela posted 24 patches 7 years, 7 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v10 10/24] migration: In case of error just end the migration
Posted by Juan Quintela 7 years, 7 months ago 
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/socket.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/migration/socket.c b/migration/socket.c
index 08606c665d..b12b0a462e 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -139,9 +139,8 @@ static gboolean socket_accept_incoming_migration(QIOChannel *ioc,
     sioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
                                      &err);
     if (!sioc) {
-        error_report("could not accept migration connection (%s)",
-                     error_get_pretty(err));
-        goto out;
+        migrate_set_error(migrate_get_current(), err);
+        return G_SOURCE_REMOVE;
     }
 
     trace_migration_socket_incoming_accepted();
@@ -150,7 +149,6 @@ static gboolean socket_accept_incoming_migration(QIOChannel *ioc,
     migration_channel_process_incoming(QIO_CHANNEL(sioc));
     object_unref(OBJECT(sioc));
 
-out:
     if (migration_has_all_channels()) {
         /* Close listening socket as its no longer needed */
         qio_channel_close(ioc, NULL);
-- 
2.14.3
Re: [Qemu-devel] [PATCH v10 10/24] migration: In case of error just end the migration
Posted by Daniel P. Berrangé 7 years, 7 months ago 
On Wed, Mar 07, 2018 at 11:59:56AM +0100, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/socket.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> diff --git a/migration/socket.c b/migration/socket.c
> index 08606c665d..b12b0a462e 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -139,9 +139,8 @@ static gboolean socket_accept_incoming_migration(QIOChannel *ioc,
>      sioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
>                                       &err);
>      if (!sioc) {
> -        error_report("could not accept migration connection (%s)",
> -                     error_get_pretty(err));
> -        goto out;
> +        migrate_set_error(migrate_get_current(), err);
> +        return G_SOURCE_REMOVE;

It will only return NULL if a client connected & then went away. This should
not happen with a "normal" mgmt app usage. On the flip side this allows a
malicious network attacker to inflict a denial of service on the migration
by simply connecting to target QEMU & immediately exiting.

Our "authentication" for migration relies on being able to validate the TLS
certs during TLS handshake. So in general we ought to allow repeated incoming
connections until we get a successful handshake. 

So in fact, I think a better fix here is to simply remove the original
'error_report' line, and ensure we return G_SOURCE_CONTINUE to wait for
another incoming connection from the real mgmt app.

>      }
>  
>      trace_migration_socket_incoming_accepted();
> @@ -150,7 +149,6 @@ static gboolean socket_accept_incoming_migration(QIOChannel *ioc,
>      migration_channel_process_incoming(QIO_CHANNEL(sioc));
>      object_unref(OBJECT(sioc));
>  
> -out:
>      if (migration_has_all_channels()) {
>          /* Close listening socket as its no longer needed */
>          qio_channel_close(ioc, NULL);
> -- 
> 2.14.3
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v10 10/24] migration: In case of error just end the migration
Posted by Eric Blake 7 years, 7 months ago 
On 03/07/2018 05:52 AM, Daniel P. Berrangé wrote:
> On Wed, Mar 07, 2018 at 11:59:56AM +0100, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>   migration/socket.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 

> It will only return NULL if a client connected & then went away. This should
> not happen with a "normal" mgmt app usage. On the flip side this allows a
> malicious network attacker to inflict a denial of service on the migration
> by simply connecting to target QEMU & immediately exiting.
> 
> Our "authentication" for migration relies on being able to validate the TLS
> certs during TLS handshake. So in general we ought to allow repeated incoming
> connections until we get a successful handshake.

Indeed, our NBD code had some CVE fixes last year where a rogue 'nc' 
process could cause denial of service by connecting and hanging up 
immediately, until we fixed it to retry until the first client that 
actually got past the handshake.  We don't need to repeat CVEs like that.

> 
> So in fact, I think a better fix here is to simply remove the original
> 'error_report' line, and ensure we return G_SOURCE_CONTINUE to wait for
> another incoming connection from the real mgmt app.
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v10 10/24] migration: In case of error just end the migration
Posted by Dr. David Alan Gilbert 7 years, 7 months ago 
* Juan Quintela (quintela@redhat.com) wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>

It's probably worth keeping an eye on what happens in the case
of Peter's postcopy recovery where a new incoming connection
happens later.

Dave

> ---
>  migration/socket.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/socket.c b/migration/socket.c
> index 08606c665d..b12b0a462e 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -139,9 +139,8 @@ static gboolean socket_accept_incoming_migration(QIOChannel *ioc,
>      sioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
>                                       &err);
>      if (!sioc) {
> -        error_report("could not accept migration connection (%s)",
> -                     error_get_pretty(err));
> -        goto out;
> +        migrate_set_error(migrate_get_current(), err);
> +        return G_SOURCE_REMOVE;
>      }
>  
>      trace_migration_socket_incoming_accepted();
> @@ -150,7 +149,6 @@ static gboolean socket_accept_incoming_migration(QIOChannel *ioc,
>      migration_channel_process_incoming(QIO_CHANNEL(sioc));
>      object_unref(OBJECT(sioc));
>  
> -out:
>      if (migration_has_all_channels()) {
>          /* Close listening socket as its no longer needed */
>          qio_channel_close(ioc, NULL);
> -- 
> 2.14.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.