[1/2] nbd: Drop connection if broken server is detected

Submitted by Eric Blake on Aug. 11, 2017, 2:37 a.m.

Details

Message ID 20170811023759.26390-2-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake Aug. 11, 2017, 2:37 a.m.
As soon as the server is sending us garbage, we should quit
trying to send further messages to the server, and allow all
pending coroutines for any remaining replies to error out.
Failure to do so can let a malicious server cause the client
to hang, for example, if the server sends an invalid magic
number in its response.

Reported by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/nbd-client.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy Aug. 11, 2017, 7:48 a.m.
11.08.2017 05:37, Eric Blake wrote:
> As soon as the server is sending us garbage, we should quit
> trying to send further messages to the server, and allow all
> pending coroutines for any remaining replies to error out.
> Failure to do so can let a malicious server cause the client
> to hang, for example, if the server sends an invalid magic
> number in its response.
>
> Reported by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   block/nbd-client.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 25dd28406b..802d50b636 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -68,7 +68,8 @@ static void nbd_teardown_connection(BlockDriverState *bs)
>
>   static coroutine_fn void nbd_read_reply_entry(void *opaque)
>   {
> -    NBDClientSession *s = opaque;
> +    BlockDriverState *bs = opaque;
> +    NBDClientSession *s = nbd_get_client_session(bs);
>       uint64_t i;
>       int ret;
>       Error *local_err = NULL;
> @@ -107,8 +108,12 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
>           qemu_coroutine_yield();
>       }
>
> +    s->reply.handle = 0;
>       nbd_recv_coroutines_enter_all(s);
>       s->read_reply_co = NULL;
> +    if (ret < 0) {
> +        nbd_teardown_connection(bs);
> +    }

what if it happens in parallel with nbd_co_send_request? 
nbd_teardown_connectin destroys s->ioc, nbd_co_send_requests
checks s->ioc only once and then calls nbd_send_request (which is 
finally nbd_rwv and may yield). I think nbd_rwv is not
prepared to sudden destruction of ioc..

>   }
>
>   static int nbd_co_send_request(BlockDriverState *bs,
> @@ -416,7 +421,7 @@ int nbd_client_init(BlockDriverState *bs,
>       /* Now that we're connected, set the socket to be non-blocking and
>        * kick the reply mechanism.  */
>       qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
> -    client->read_reply_co = qemu_coroutine_create(nbd_read_reply_entry, client);
> +    client->read_reply_co = qemu_coroutine_create(nbd_read_reply_entry, bs);
>       nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));
>
>       logout("Established connection with NBD server\n");

Patch hide | download patch | download mbox

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 25dd28406b..802d50b636 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -68,7 +68,8 @@  static void nbd_teardown_connection(BlockDriverState *bs)

 static coroutine_fn void nbd_read_reply_entry(void *opaque)
 {
-    NBDClientSession *s = opaque;
+    BlockDriverState *bs = opaque;
+    NBDClientSession *s = nbd_get_client_session(bs);
     uint64_t i;
     int ret;
     Error *local_err = NULL;
@@ -107,8 +108,12 @@  static coroutine_fn void nbd_read_reply_entry(void *opaque)
         qemu_coroutine_yield();
     }

+    s->reply.handle = 0;
     nbd_recv_coroutines_enter_all(s);
     s->read_reply_co = NULL;
+    if (ret < 0) {
+        nbd_teardown_connection(bs);
+    }
 }

 static int nbd_co_send_request(BlockDriverState *bs,
@@ -416,7 +421,7 @@  int nbd_client_init(BlockDriverState *bs,
     /* Now that we're connected, set the socket to be non-blocking and
      * kick the reply mechanism.  */
     qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
-    client->read_reply_co = qemu_coroutine_create(nbd_read_reply_entry, client);
+    client->read_reply_co = qemu_coroutine_create(nbd_read_reply_entry, bs);
     nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));

     logout("Established connection with NBD server\n");