On Mon, Aug 14, 2017 at 04:34:26PM -0500, Eric Blake wrote:
> When we switched NBD to use coroutines for qemu 2.9 (in particular,
> commit a12a712a), we introduced a regression: if a server sends us
> garbage (such as a corrupted magic number), we quit the read loop
> but do not stop sending further queued commands, resulting in the
> client hanging when it never reads the response to those additional
> commands. In qemu 2.8, we properly detected that the server is no
> longer reliable, and cancelled all existing pending commands with
> EIO, then tore down the socket so that all further command attempts
> get EPIPE.
>
> Restore the proper behavior of quitting (almost) all communication
> with a broken server: Once we know we are out of sync or otherwise
> can't trust the server, we must assume that any further incoming
> data is unreliable and therefore end all pending commands with EIO,
> and quit trying to send any further commands. As an exception, we
> still (try to) send NBD_CMD_DISC to let the server know we are going
> away (in part, because it is easier to do that than to further
> refactor nbd_teardown_connection, and in part because it is the
> only command where we do not have to wait for a reply).
>
> Based on a patch by Vladimir Sementsov-Ogievskiy.
>
> A malicious server can be created with the following hack,
> followed by setting NBD_SERVER_DEBUG to a non-zero value in the
> environment when running qemu-nbd:
>
> | --- a/nbd/server.c
> | +++ b/nbd/server.c
> | @@ -919,6 +919,17 @@ static int nbd_send_reply(QIOChannel *ioc, NBDReply
> *reply, Error **errp)
> | stl_be_p(buf + 4, reply->error);
> | stq_be_p(buf + 8, reply->handle);
> |
> | + static int debug;
> | + static int count;
> | + if (!count++) {
> | + const char *str = getenv("NBD_SERVER_DEBUG");
> | + if (str) {
> | + debug = atoi(str);
> | + }
> | + }
> | + if (debug && !(count % debug)) {
> | + buf[0] = 0;
> | + }
> | return nbd_write(ioc, buf, sizeof(buf), errp);
> | }
>
> Reported-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
>
> Supercedes both Vladimir and my earlier attempts:
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02131.html
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01501.html
>
> block/nbd-client.h | 1 +
> block/nbd-client.c | 14 ++++++++++----
> 2 files changed, 11 insertions(+), 4 deletions(-)Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
signature.asc
Description: PGP signature
