| Message ID | 20170811023759.26390-2-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
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");
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");
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(-)