Libguestfs - May 2019 - [libnbd PATCH 0/4] Various interop fixes

Some of these affect attempts to connect to older qemu-nbd versions,
some of them were triggered by manual edits to qemu-nbd source code to
provoke various other compliant (if uncommon) server behaviors.

Eric Blake (4):
  starttls: Skip error payload if falling back to unencrypted
  states: Reject payload to NBD_REP_ACK
  meta-context: Skip error payload if server lacks meta_context
  states: Add NBD_OPT_EXPORT_NAME handling

 generator/Makefile.am                         |  1 +
 generator/generator                           | 53 ++++++++++++++
 generator/states-newstyle-opt-export-name.c   | 73 +++++++++++++++++++
 generator/states-newstyle-opt-go.c            | 13 +++-
 .../states-newstyle-opt-set-meta-context.c    | 26 +++++--
 generator/states-newstyle-opt-starttls.c      | 27 ++++++-
 .../states-newstyle-opt-structured-reply.c    |  6 ++
 generator/states-newstyle.c                   | 11 ++-
 lib/internal.h                                |  1 +
 lib/nbd-protocol.h                            |  7 ++
 10 files changed, 200 insertions(+), 18 deletions(-)
 create mode 100644 generator/states-newstyle-opt-export-name.c

-- 
2.20.1

If we tried but did not require TLS, and the NBD server rejected our
attempt at NBD_OPT_STARTTLS but included an error message, then we
MUST capture (or skip) that message off the wire before proceeding on
with attempting structured replies. This requires a new state, as well
as changing the rejection of payload length to only happen on
NBD_REP_ACK.
---
 generator/generator                      |  7 ++++++
 generator/states-newstyle-opt-starttls.c | 27 +++++++++++++++++++++---
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/generator/generator b/generator/generator
index d5b5f5f..3faa272 100755
--- a/generator/generator
+++ b/generator/generator
@@ -352,6 +352,13 @@ and newstyle_opt_starttls_state_machine = [
     external_events = [ NotifyRead, "" ];
   };

+  State {
+    default_state with
+    name = "SKIP_REPLY_PAYLOAD";
+    comment = "Skip newstyle NBD_OPT_STARTTLS reply payload";
+    external_events = [ NotifyRead, "" ];
+  };
+
   State {
     default_state with
     name = "CHECK_REPLY";
diff --git a/generator/states-newstyle-opt-starttls.c
b/generator/states-newstyle-opt-starttls.c
index f44b654..f56553c 100644
--- a/generator/states-newstyle-opt-starttls.c
+++ b/generator/states-newstyle-opt-starttls.c
@@ -45,9 +45,23 @@
   return 0;

  NEWSTYLE.OPT_STARTTLS.RECV_REPLY:
+  uint32_t len;
+
   switch (recv_into_rbuf (conn)) {
   case -1: SET_NEXT_STATE (%.DEAD); return -1;
-  case 0: SET_NEXT_STATE (%CHECK_REPLY);
+  case 0:
+    /* Discard the payload if there is one. */
+    len = be32toh (conn->sbuf.or.option_reply.replylen);
+    conn->rbuf = NULL;
+    conn->rlen = len;
+    SET_NEXT_STATE (%SKIP_REPLY_PAYLOAD);
+  }
+  return 0;
+
+ NEWSTYLE.OPT_STARTTLS.SKIP_REPLY_PAYLOAD:
+  switch (recv_into_rbuf (conn)) {
+  case -1: SET_NEXT_STATE (%.DEAD); return -1;
+  case 0:  SET_NEXT_STATE (%CHECK_REPLY);
   }
   return 0;

@@ -62,13 +76,19 @@
   option = be32toh (conn->sbuf.or.option_reply.option);
   reply = be32toh (conn->sbuf.or.option_reply.reply);
   len = be32toh (conn->sbuf.or.option_reply.replylen);
-  if (magic != NBD_REP_MAGIC || option != NBD_OPT_STARTTLS || len != 0) {
+  if (magic != NBD_REP_MAGIC || option != NBD_OPT_STARTTLS) {
     SET_NEXT_STATE (%.DEAD);
-    set_error (0, "handshake: invalid option reply magic, option or
length");
+    set_error (0, "handshake: invalid option reply magic or option");
     return -1;
   }
   switch (reply) {
   case NBD_REP_ACK:
+    if (len != 0) {
+      SET_NEXT_STATE (%.DEAD);
+      set_error (0, "handshake: invalid option reply length");
+      return -1;
+    }
+
     new_sock = nbd_internal_crypto_create_session (conn, conn->sock);
     if (new_sock == NULL) {
       SET_NEXT_STATE (%.DEAD);
@@ -99,6 +119,7 @@
     debug (conn->h,
            "server refused TLS (%s), continuing with unencrypted
connection",
            reply == NBD_REP_ERR_POLICY ? "policy" : "not
supported");
+    /* XXX: capture instead of skip server's payload to NBD_REP_ERR*? */
     SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
     return 0;
   }
-- 
2.20.1

The protocol says NBD_REP_ACK should be sent without payload. Enforce
this.
---
 generator/states-newstyle-opt-go.c               | 5 +++++
 generator/states-newstyle-opt-set-meta-context.c | 5 +++++
 generator/states-newstyle-opt-structured-reply.c | 6 ++++++
 3 files changed, 16 insertions(+)

diff --git a/generator/states-newstyle-opt-go.c
b/generator/states-newstyle-opt-go.c
index 6a7b3af..200d16f 100644
--- a/generator/states-newstyle-opt-go.c
+++ b/generator/states-newstyle-opt-go.c
@@ -118,6 +118,11 @@
   }
   switch (reply) {
   case NBD_REP_ACK:
+    if (len != 0) {
+      SET_NEXT_STATE (%.DEAD);
+      set_error (0, "handshake: invalid option reply length");
+      return -1;
+    }
     SET_NEXT_STATE (%.READY);
     return 0;
   case NBD_REP_INFO:
diff --git a/generator/states-newstyle-opt-set-meta-context.c
b/generator/states-newstyle-opt-set-meta-context.c
index fdc0500..5a445b2 100644
--- a/generator/states-newstyle-opt-set-meta-context.c
+++ b/generator/states-newstyle-opt-set-meta-context.c
@@ -133,6 +133,11 @@ const char base_allocation[] = "base:allocation";
     }
     switch (reply) {
     case NBD_REP_ACK:           /* End of list of replies. */
+      if (len != 0) {
+	SET_NEXT_STATE (%.DEAD);
+	set_error (0, "handshake: invalid option reply length");
+	return -1;
+      }
       SET_NEXT_STATE (%FINISH);
       break;
     case NBD_REP_META_CONTEXT:  /* A context. */
diff --git a/generator/states-newstyle-opt-structured-reply.c
b/generator/states-newstyle-opt-structured-reply.c
index 56335fd..91acdcc 100644
--- a/generator/states-newstyle-opt-structured-reply.c
+++ b/generator/states-newstyle-opt-structured-reply.c
@@ -74,10 +74,16 @@
   }
   switch (reply) {
   case NBD_REP_ACK:
+    if (conn->sbuf.or.option_reply.replylen != 0) {
+      SET_NEXT_STATE (%.DEAD);
+      set_error (0, "handshake: invalid option reply length");
+      return -1;
+    }
     debug (conn->h, "negotiated structured replies on this
connection");
     conn->structured_replies = true;
     break;
   default:
+    /* XXX: capture instead of skip server's payload to NBD_REP_ERR*? */
     debug (conn->h, "structured replies are not supported by this
server");
     conn->structured_replies = false;
     break;
-- 
2.20.1

If we tried NBD_OPT_SET_META_CONTEXT, and the NBD server rejected our
attempt with an error message, then we should capture (or skip) that
message off the wire in order to go on without base:allocation, the
same as if the server had not sent a message.  This requires a new
state.
---
 generator/generator                           |  7 +++++++
 .../states-newstyle-opt-set-meta-context.c    | 21 ++++++++++++-------
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/generator/generator b/generator/generator
index 3faa272..4d42185 100755
--- a/generator/generator
+++ b/generator/generator
@@ -491,6 +491,13 @@ and newstyle_opt_set_meta_context_state_machine = [
     external_events = [ NotifyRead, "" ];
   };

+  State {
+    default_state with
+    name = "RECV_SKIP_PAYLOAD";
+    comment = "Ignore newstyle NBD_OPT_SET_META_CONTEXT option reply
payload";
+    external_events = [ NotifyRead, "" ];
+  };
+
   State {
     default_state with
     name = "FINISH";
diff --git a/generator/states-newstyle-opt-set-meta-context.c
b/generator/states-newstyle-opt-set-meta-context.c
index 5a445b2..b815701 100644
--- a/generator/states-newstyle-opt-set-meta-context.c
+++ b/generator/states-newstyle-opt-set-meta-context.c
@@ -150,14 +150,11 @@ const char base_allocation[] =
"base:allocation";
       SET_NEXT_STATE (%RECV_REPLY_PAYLOAD);
       break;
     default:
-      /* Anything else is an error, ignore it if the len == 0. */
-      if (len != 0) {
-        /* We could probably recover from this with some effort. */
-        SET_NEXT_STATE (%.DEAD);
-        set_error (0, "handshake: unknown option reply (%" PRIu32
")", reply);
-        return -1;
-      }
-      SET_NEXT_STATE (%FINISH);
+      /* Anything else is an error, ignore it */
+      debug (conn->h, "base:allocation is not supported by this
server");
+      conn->rbuf = NULL;
+      conn->rlen = len;
+      SET_NEXT_STATE (%RECV_SKIP_PAYLOAD);
     }
   }
   return 0;
@@ -181,6 +178,14 @@ const char base_allocation[] = "base:allocation";
   }
   return 0;

+ NEWSTYLE.OPT_SET_META_CONTEXT.RECV_SKIP_PAYLOAD:
+  switch (recv_into_rbuf (conn)) {
+  case -1: SET_NEXT_STATE (%.DEAD); return -1;
+  case 0:  SET_NEXT_STATE (%FINISH);
+    /* XXX: capture instead of skip server's payload to NBD_REP_ERR*? */
+  }
+  return 0;
+
  NEWSTYLE.OPT_SET_META_CONTEXT.FINISH:
   /* Jump to the next option. */
   SET_NEXT_STATE (%^OPT_GO.START);
-- 
2.20.1

Used both for servers that are not fixed newstyle, and for servers
that don't understand NBD_OPT_GO.
---
 generator/Makefile.am                       |  1 +
 generator/generator                         | 39 +++++++++++
 generator/states-newstyle-opt-export-name.c | 73 +++++++++++++++++++++
 generator/states-newstyle-opt-go.c          |  8 +--
 generator/states-newstyle.c                 | 11 +++-
 lib/internal.h                              |  1 +
 lib/nbd-protocol.h                          |  7 ++
 7 files changed, 133 insertions(+), 7 deletions(-)
 create mode 100644 generator/states-newstyle-opt-export-name.c

diff --git a/generator/Makefile.am b/generator/Makefile.am
index 8262d7c..f4c127b 100644
--- a/generator/Makefile.am
+++ b/generator/Makefile.am
@@ -26,6 +26,7 @@ states_code = \
 	states-connect.c \
 	states-issue-command.c \
 	states-magic.c \
+	states-newstyle-opt-export-name.c \
 	states-newstyle-opt-go.c \
 	states-newstyle-opt-set-meta-context.c \
 	states-newstyle-opt-starttls.c \
diff --git a/generator/generator b/generator/generator
index 4d42185..60e4c55 100755
--- a/generator/generator
+++ b/generator/generator
@@ -327,6 +327,7 @@ and newstyle_state_machine = [
   Group ("OPT_STRUCTURED_REPLY",
newstyle_opt_structured_reply_state_machine);
   Group ("OPT_SET_META_CONTEXT",
newstyle_opt_set_meta_context_state_machine);
   Group ("OPT_GO", newstyle_opt_go_state_machine);
+  Group ("OPT_EXPORT_NAME", newstyle_opt_export_name_state_machine);
 ]

 (* Fixed newstyle NBD_OPT_STARTTLS option. *)
@@ -565,6 +566,44 @@ and newstyle_opt_go_state_machine = [
   };
 ]

+(* Newstyle NBD_OPT_EXPORT_NAME option. *)
+and newstyle_opt_export_name_state_machine = [
+  State {
+    default_state with
+    name = "START";
+    comment = "Try to send newstyle NBD_OPT_EXPORT_NAME to end
handshake";
+    external_events = [];
+  };
+
+  State {
+    default_state with
+    name = "SEND";
+    comment = "Send newstyle NBD_OPT_EXPORT_NAME to end handshake";
+    external_events = [ NotifyWrite, "" ];
+  };
+
+  State {
+    default_state with
+    name = "SEND_EXPORT";
+    comment = "Send newstyle NBD_OPT_EXPORT_NAME export name";
+    external_events = [ NotifyWrite, "" ];
+  };
+
+  State {
+    default_state with
+    name = "RECV_REPLY";
+    comment = "Receive newstyle NBD_OPT_EXPORT_NAME reply";
+    external_events = [ NotifyRead, "" ];
+  };
+
+  State {
+    default_state with
+    name = "CHECK_REPLY";
+    comment = "Check newstyle NBD_OPT_EXPORT_NAME reply";
+    external_events = [];
+  };
+]
+
 (* Sending a command to the server. *)
 and issue_command_state_machine = [
   State {
diff --git a/generator/states-newstyle-opt-export-name.c
b/generator/states-newstyle-opt-export-name.c
new file mode 100644
index 0000000..6c6bce4
--- /dev/null
+++ b/generator/states-newstyle-opt-export-name.c
@@ -0,0 +1,73 @@
+/* nbd client library in userspace: state machine
+ * Copyright (C) 2013-2019 Red Hat Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/* State machine for ending newstyle handshake with NBD_OPT_EXPORT_NAME. */
+
+/* STATE MACHINE */ {
+ NEWSTYLE.OPT_EXPORT_NAME.START:
+  conn->sbuf.option.version = htobe64 (NBD_NEW_VERSION);
+  conn->sbuf.option.option = htobe32 (NBD_OPT_EXPORT_NAME);
+  conn->sbuf.option.optlen = strlen (h->export_name);
+  conn->wbuf = &conn->sbuf;
+  conn->wlen = sizeof conn->sbuf.option;
+  SET_NEXT_STATE (%SEND);
+  return 0;
+
+ NEWSTYLE.OPT_EXPORT_NAME.SEND:
+  switch (send_from_wbuf (conn)) {
+  case -1: SET_NEXT_STATE (%.DEAD); return -1;
+  case 0:
+    conn->wbuf = h->export_name;
+    conn->wlen = strlen (h->export_name);
+    SET_NEXT_STATE (%SEND_EXPORT);
+  }
+  return 0;
+
+ NEWSTYLE.OPT_EXPORT_NAME.SEND_EXPORT:
+  switch (send_from_wbuf (conn)) {
+  case -1: SET_NEXT_STATE (%.DEAD); return -1;
+  case 0:
+    conn->rbuf = &conn->sbuf;
+    conn->rlen = sizeof conn->sbuf.export_name_reply;
+    if ((conn->gflags & NBD_FLAG_NO_ZEROES) != 0)
+      conn->rlen -= sizeof conn->sbuf.export_name_reply.zeroes;
+    SET_NEXT_STATE (%RECV_REPLY);
+  }
+  return 0;
+
+ NEWSTYLE.OPT_EXPORT_NAME.RECV_REPLY:
+  switch (recv_into_rbuf (conn)) {
+  case -1: SET_NEXT_STATE (%.DEAD); return -1;
+  case 0:  SET_NEXT_STATE (%CHECK_REPLY);
+  }
+  return 0;
+
+ NEWSTYLE.OPT_EXPORT_NAME.CHECK_REPLY:
+  conn->h->exportsize = be64toh
(conn->sbuf.export_name_reply.exportsize);
+  conn->h->eflags = be16toh (conn->sbuf.export_name_reply.eflags);
+  debug (conn->h, "exportsize: %" PRIu64 " eflags: 0x%"
PRIx16,
+	 conn->h->exportsize, conn->h->eflags);
+  if (conn->h->eflags == 0) {
+    SET_NEXT_STATE (%.DEAD);
+    set_error (EINVAL, "handshake: invalid eflags == 0 from server");
+    return -1;
+  }
+  SET_NEXT_STATE (%.READY);
+  return 0;
+
+} /* END STATE MACHINE */
diff --git a/generator/states-newstyle-opt-go.c
b/generator/states-newstyle-opt-go.c
index 200d16f..42bae8a 100644
--- a/generator/states-newstyle-opt-go.c
+++ b/generator/states-newstyle-opt-go.c
@@ -149,10 +149,10 @@
     SET_NEXT_STATE (%RECV_REPLY);
     return 0;
   case NBD_REP_ERR_UNSUP:
-    /* XXX fall back to NBD_OPT_EXPORT_NAME */
-    SET_NEXT_STATE (%.DEAD);
-    set_error (0, "handshake: server does not support NBD_OPT_GO");
-    return -1;
+    debug (conn->h,
+	   "server is confused by NBD_OPT_GO, continuing anyway");
+    SET_NEXT_STATE (%^OPT_EXPORT_NAME.START);
+    return 0;
   default:
     SET_NEXT_STATE (%.DEAD);
     set_error (0, "handshake: unknown reply from NBD_OPT_GO: 0x%"
PRIx32,
diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c
index f3a07f7..d108244 100644
--- a/generator/states-newstyle.c
+++ b/generator/states-newstyle.c
@@ -36,9 +36,11 @@
   uint32_t cflags;

   conn->gflags = be16toh (conn->gflags);
-  if ((conn->gflags & NBD_FLAG_FIXED_NEWSTYLE) == 0) {
+  if ((conn->gflags & NBD_FLAG_FIXED_NEWSTYLE) == 0 &&
+      h->tls == 2) {
     SET_NEXT_STATE (%.DEAD);
-    set_error (0, "handshake: server is not a fixed newstyle NBD
server");
+    set_error (ENOTSUP, "handshake: server is not fixed newstyle, "
+	       "but handle TLS setting is require (2)");
     return -1;
   }

@@ -54,7 +56,10 @@
   case -1: SET_NEXT_STATE (%.DEAD); return -1;
   case 0:
     /* Start sending options. */
-    SET_NEXT_STATE (%OPT_STARTTLS.START);
+    if ((conn->gflags & NBD_FLAG_FIXED_NEWSTYLE) == 0)
+      SET_NEXT_STATE (%OPT_EXPORT_NAME.START);
+    else
+      SET_NEXT_STATE (%OPT_STARTTLS.START);
   }
   return 0;

diff --git a/lib/internal.h b/lib/internal.h
index f154a7b..852158a 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -127,6 +127,7 @@ struct nbd_connection {
         }  __attribute__((packed)) context;
       } payload;
     }  __attribute__((packed)) or;
+    struct nbd_export_name_option_reply export_name_reply;
     struct nbd_request request;
     struct nbd_simple_reply simple_reply;
     struct {
diff --git a/lib/nbd-protocol.h b/lib/nbd-protocol.h
index 398403a..4bda056 100644
--- a/lib/nbd-protocol.h
+++ b/lib/nbd-protocol.h
@@ -74,6 +74,13 @@ struct nbd_new_option {
   /* option data follows */
 } __attribute__((packed));

+/* Newstyle handshake OPT_EXPORT_NAME reply message. */
+struct nbd_export_name_option_reply {
+  uint64_t exportsize;          /* size of export */
+  uint16_t eflags;              /* per-export flags */
+  char zeroes[124];             /* optional zeroes */
+} __attribute__((packed));;
+
 /* Fixed newstyle handshake reply message. */
 struct nbd_fixed_new_option_reply {
   uint64_t magic;             /* NBD_REP_MAGIC */
-- 
2.20.1

Thanks - I have pushed this.

I also added #line directives to lib/states.c which should hopefully
make debugging a bit easier :-)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org