Libguestfs - Sep 2020 - [RFC libnbd PATCH 0/2] Add knobs for client- vs. server-side validation

We have been inconsistent on how much we reject client-side without
even consulting the server, vs. how much we depend on the server to
detect failure (even if our request can be deemed undefined per NBD
protocol).  I'd like to change it so that by default, we reject as
much as we can client-side for less traffic, but where the user can
also change things on the fly for server-side integration testing.

My remaining question, then, is how many knobs to expose initially?
As written, I've only given two knobs, but make the case in each patch
how it could easily be split into four knobs if we think the finer
granularity is worthwhile.

A future knob for block-size validation would also make sense,
especially given the recent addition of support for querying server
minimum block size constraints (although I'd like to finish my work on
getting nbdkit to advertise block constraints to make that easier to
test...).

Eric Blake (2):
  api: Add nbd_set_strict_mode
  api: Add STRICT_BOUNDS to nbd_set_strict_mode

 lib/internal.h   |   3 +
 generator/API.ml |  67 ++++++++++-
 lib/disconnect.c |  18 +--
 lib/handle.c     |  16 +++
 lib/rw.c         | 289 ++++++++++++++++++++++++++++++-----------------
 tests/errors.c   |  61 +++++++++-
 6 files changed, 340 insertions(+), 114 deletions(-)

-- 
2.28.0

Right now, libnbd has refused to issue a command not advertised as
supported by a server, including unknown flags, mainly because the NBD
protocol does not guarantee what the server will do, and libnbd would
rather stay in sync with the server than drop the connection.
However, for integration purposes, it can be handy to coerce libnbd
into sending something to see how the server will react (whether it be
an extension libnbd has not yet learned, or an intentional bad request
to test server error handling).  Time to make this something the user
can control, by adding a new strictness mode.  A later patch will add
another knob to the mode.

---

Question: Should we split this into two knobs?  Sending unknown flags
is dangerous (in the future, a flag might be defined to alter the
on-wire layout expected by either the client or server, and get things
out-of-sync or even cause data corruption), while sending known
commands contrary to advertised flags is less risky (so far,
NBD_CMD_WRITE is the only command that alters what the client sends,
so a server can easily reject unknown commands by assuming they match
the layout of NBD_CMD_READ; and I just fixed a long-standing bug where
we were sending trim and zero requests contrary to server
advertisement).  Trying to provoke a write to a read-only server is
different than trying to experiment with an unknown flag, which is why
it might make sense to have two separate knobs for the gating done in
this patch.

---
 lib/internal.h   |   3 +
 generator/API.ml |  60 +++++++++++++-
 lib/disconnect.c |  18 ++--
 lib/handle.c     |  16 ++++
 lib/rw.c         | 208 +++++++++++++++++++++++++----------------------
 tests/errors.c   |  41 +++++++++-
 6 files changed, 239 insertions(+), 107 deletions(-)

diff --git a/lib/internal.h b/lib/internal.h
index b2637bd..788a62e 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -148,6 +148,9 @@ struct nbd_handle {
   bool debug;
   nbd_debug_callback debug_callback;

+  /* How strict to be. */
+  unsigned strict;
+
   /* State machine.
    *
    * The actual current state is ‘state’.  ‘public_state’ is updated
diff --git a/generator/API.ml b/generator/API.ml
index 962b787..8811d3c 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -169,7 +169,13 @@ let handshake_flags = {
   flags = [
     "FIXED_NEWSTYLE", 1 lsl 0;
     "NO_ZEROES",      1 lsl 1;
-    ]
+  ]
+}
+let strict_flags = {
+  flag_prefix = "STRICT";
+  flags = [
+    "COMMANDS",       1 lsl 0;
+  ]
 }
 let allow_transport_flags = {
   flag_prefix = "ALLOW_TRANSPORT";
@@ -179,7 +185,8 @@ let allow_transport_flags = {
     "VSOCK", 1 lsl 2;
   ]
 }
-let all_flags = [ cmd_flags; handshake_flags; allow_transport_flags ]
+let all_flags = [ cmd_flags; handshake_flags; strict_flags;
+                  allow_transport_flags ]

 let default_call = { args = []; optargs = []; ret = RErr;
                      shortdesc = ""; longdesc = ""; example
= None;
@@ -696,6 +703,51 @@ the time of compilation.";
                 Link "aio_is_created"; Link
"aio_is_ready"];
   };

+  "set_strict_mode", {
+    default_call with
+    args = [ Flags ("flags", strict_flags) ]; ret = RErr;
+    shortdesc = "control how strictly to follow NBD protocol";
+    longdesc = "\
+By default, libnbd tries to detect requests that would trigger
+undefined behavior in the NBD protocol, and rejects them client
+side without causing any network traffic rather than risking
+undefined server behavior.  However, for integration testing, it
+can be handy to coerce libnbd into sending such requests in
+order to test the robustness of the server in dealing with such
+traffic.
+
+The C<flags> argument is a bitmask, including zero or more of the
+following strictness flags:
+
+=over 4
+
+=item C<LIBNBD_STRICT_COMMANDS> = 1
+
+If set, this flag rejects client requests that do not comply with the
+set of advertised server flags (for example, attempting a write on
+a read-only server).  If clear, this flag relies on the server to
+reject unexpected commands or unknown flags to supported commands.
+
+=back
+
+Future versions of libnbd may add further flags.
+";
+    see_also = [Link "get_strict_mode"];
+  };
+
+  "get_strict_mode", {
+    default_call with
+    args = []; ret = RUInt;
+    may_set_error = false;
+    shortdesc = "see which strictness flags are in effect";
+    longdesc = "\
+Return flags indicating which protocol strictness items are being
+enforced locally by libnbd rather than the server.  The return value
+from a newer library version may include bits that were undefined at
+the time of compilation.";
+    see_also = [Link "set_strict_mode"];
+  };
+
   "set_opt_mode", {
     default_call with
     args = [Bool "enable"]; ret = RErr;
@@ -2615,6 +2667,10 @@ let first_version = [
   "aio_opt_list", (1, 4);
   "aio_opt_info", (1, 4);

+  (* Added in 1.5.x development cycle, will be stable and supported in 1.6. *)
+  "set_strict_mode", (1, 6);
+  "get_strict_mode", (1, 6);
+
   (* These calls are proposed for a future version of libnbd, but
    * have not been added to any released version so far.
   "get_tls_certificates", (1, ??);
diff --git a/lib/disconnect.c b/lib/disconnect.c
index dcb95d8..22cd5d7 100644
--- a/lib/disconnect.c
+++ b/lib/disconnect.c
@@ -1,5 +1,5 @@
 /* NBD client library in userspace
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2020 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
@@ -29,9 +29,11 @@
 int
 nbd_unlocked_shutdown (struct nbd_handle *h, uint32_t flags)
 {
-  if (flags != 0) {
-    set_error (EINVAL, "invalid flag: %" PRIu32, flags);
-    return -1;
+  if (h->strict & LIBNBD_STRICT_COMMANDS) {
+    if (flags != 0) {
+      set_error (EINVAL, "invalid flag: %" PRIu32, flags);
+      return -1;
+    }
   }

   if (!h->disconnect_request &&
@@ -55,9 +57,11 @@ nbd_unlocked_aio_disconnect (struct nbd_handle *h, uint32_t
flags)
 {
   int64_t id;

-  if (flags != 0) {
-    set_error (EINVAL, "invalid flag: %" PRIu32, flags);
-    return -1;
+  if (h->strict & LIBNBD_STRICT_COMMANDS) {
+    if (flags != 0) {
+      set_error (EINVAL, "invalid flag: %" PRIu32, flags);
+      return -1;
+    }
   }

   id = nbd_internal_command_common (h, 0, NBD_CMD_DISC, 0, 0, NULL, NULL);
diff --git a/lib/handle.c b/lib/handle.c
index 7987d59..a1fa142 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -75,6 +75,8 @@ nbd_create (void)
   s = getenv ("LIBNBD_DEBUG");
   h->debug = s && strcmp (s, "1") == 0;

+  h->strict = LIBNBD_STRICT_COMMANDS;
+
   h->public_state = STATE_START;
   h->state = STATE_START;
   h->pid = -1;
@@ -350,6 +352,20 @@ nbd_unlocked_get_handshake_flags (struct nbd_handle *h)
   return h->gflags;
 }

+int
+nbd_unlocked_set_strict_mode (struct nbd_handle *h, uint32_t flags)
+{
+  h->strict = flags;
+  return 0;
+}
+
+/* NB: may_set_error = false. */
+unsigned
+nbd_unlocked_get_strict_mode (struct nbd_handle *h)
+{
+  return h->strict;
+}
+
 const char *
 nbd_unlocked_get_package_name (struct nbd_handle *h)
 {
diff --git a/lib/rw.c b/lib/rw.c
index 8b736d3..b5c1698 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -267,13 +267,15 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf,
 {
   struct command_cb cb = { .completion = completion };

-  /* We could silently accept flag DF, but it really only makes sense
-   * with callbacks, because otherwise there is no observable change
-   * except that the server may fail where it would otherwise succeed.
-   */
-  if (flags != 0) {
-    set_error (EINVAL, "invalid flag: %" PRIu32, flags);
-    return -1;
+  if (h->strict & LIBNBD_STRICT_COMMANDS) {
+    /* We could silently accept flag DF, but it really only makes sense
+     * with callbacks, because otherwise there is no observable change
+     * except that the server may fail where it would otherwise succeed.
+     */
+    if (flags != 0) {
+      set_error (EINVAL, "invalid flag: %" PRIu32, flags);
+      return -1;
+    }
   }

   return nbd_internal_command_common (h, 0, NBD_CMD_READ, offset, count,
@@ -290,15 +292,17 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle *h,
void *buf,
   struct command_cb cb = { .fn.chunk = chunk,
                            .completion = completion };

-  if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) {
-    set_error (EINVAL, "invalid flag: %" PRIu32, flags);
-    return -1;
-  }
+  if (h->strict & LIBNBD_STRICT_COMMANDS) {
+    if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) {
+      set_error (EINVAL, "invalid flag: %" PRIu32, flags);
+      return -1;
+    }

-  if ((flags & LIBNBD_CMD_FLAG_DF) != 0 &&
-      nbd_unlocked_can_df (h) != 1) {
-    set_error (EINVAL, "server does not support the DF flag");
-    return -1;
+    if ((flags & LIBNBD_CMD_FLAG_DF) != 0 &&
+        nbd_unlocked_can_df (h) != 1) {
+      set_error (EINVAL, "server does not support the DF flag");
+      return -1;
+    }
   }

   return nbd_internal_command_common (h, flags, NBD_CMD_READ, offset, count,
@@ -313,20 +317,22 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void
*buf,
 {
   struct command_cb cb = { .completion = completion };

-  if (nbd_unlocked_is_read_only (h) == 1) {
-    set_error (EPERM, "server does not support write operations");
-    return -1;
-  }
+  if (h->strict & LIBNBD_STRICT_COMMANDS) {
+    if (nbd_unlocked_is_read_only (h) == 1) {
+      set_error (EPERM, "server does not support write operations");
+      return -1;
+    }

-  if ((flags & ~LIBNBD_CMD_FLAG_FUA) != 0) {
-    set_error (EINVAL, "invalid flag: %" PRIu32, flags);
-    return -1;
-  }
+    if ((flags & ~LIBNBD_CMD_FLAG_FUA) != 0) {
+      set_error (EINVAL, "invalid flag: %" PRIu32, flags);
+      return -1;
+    }

-  if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 &&
-      nbd_unlocked_can_fua (h) != 1) {
-    set_error (EINVAL, "server does not support the FUA flag");
-    return -1;
+    if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 &&
+        nbd_unlocked_can_fua (h) != 1) {
+      set_error (EINVAL, "server does not support the FUA flag");
+      return -1;
+    }
   }

   return nbd_internal_command_common (h, flags, NBD_CMD_WRITE, offset, count,
@@ -340,14 +346,16 @@ nbd_unlocked_aio_flush (struct nbd_handle *h,
 {
   struct command_cb cb = { .completion = completion };

-  if (nbd_unlocked_can_flush (h) != 1) {
-    set_error (EINVAL, "server does not support flush operations");
-    return -1;
-  }
+  if (h->strict & LIBNBD_STRICT_COMMANDS) {
+    if (nbd_unlocked_can_flush (h) != 1) {
+      set_error (EINVAL, "server does not support flush operations");
+      return -1;
+    }

-  if (flags != 0) {
-    set_error (EINVAL, "invalid flag: %" PRIu32, flags);
-    return -1;
+    if (flags != 0) {
+      set_error (EINVAL, "invalid flag: %" PRIu32, flags);
+      return -1;
+    }
   }

   return nbd_internal_command_common (h, 0, NBD_CMD_FLUSH, 0, 0,
@@ -362,24 +370,26 @@ nbd_unlocked_aio_trim (struct nbd_handle *h,
 {
   struct command_cb cb = { .completion = completion };

-  if (nbd_unlocked_can_trim (h) != 1) {
-    set_error (EINVAL, "server does not support trim operations");
-    return -1;
-  }
-  if (nbd_unlocked_is_read_only (h) == 1) {
-    set_error (EPERM, "server does not support write operations");
-    return -1;
-  }
+  if (h->strict & LIBNBD_STRICT_COMMANDS) {
+    if (nbd_unlocked_can_trim (h) != 1) {
+      set_error (EINVAL, "server does not support trim operations");
+      return -1;
+    }
+    if (nbd_unlocked_is_read_only (h) == 1) {
+      set_error (EPERM, "server does not support write operations");
+      return -1;
+    }

-  if ((flags & ~LIBNBD_CMD_FLAG_FUA) != 0) {
-    set_error (EINVAL, "invalid flag: %" PRIu32, flags);
-    return -1;
-  }
+    if ((flags & ~LIBNBD_CMD_FLAG_FUA) != 0) {
+      set_error (EINVAL, "invalid flag: %" PRIu32, flags);
+      return -1;
+    }

-  if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 &&
-      nbd_unlocked_can_fua (h) != 1) {
-    set_error (EINVAL, "server does not support the FUA flag");
-    return -1;
+    if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 &&
+        nbd_unlocked_can_fua (h) != 1) {
+      set_error (EINVAL, "server does not support the FUA flag");
+      return -1;
+    }
   }

   if (count == 0) {             /* NBD protocol forbids this. */
@@ -399,18 +409,20 @@ nbd_unlocked_aio_cache (struct nbd_handle *h,
 {
   struct command_cb cb = { .completion = completion };

-  /* Actually according to the NBD protocol document, servers do exist
-   * that support NBD_CMD_CACHE but don't advertise the
-   * NBD_FLAG_SEND_CACHE bit, but we ignore those.
-   */
-  if (nbd_unlocked_can_cache (h) != 1) {
-    set_error (EINVAL, "server does not support cache operations");
-    return -1;
-  }
+  if (h->strict & LIBNBD_STRICT_COMMANDS) {
+    /* Actually according to the NBD protocol document, servers do exist
+     * that support NBD_CMD_CACHE but don't advertise the
+     * NBD_FLAG_SEND_CACHE bit, but we ignore those.
+     */
+    if (nbd_unlocked_can_cache (h) != 1) {
+      set_error (EINVAL, "server does not support cache operations");
+      return -1;
+    }

-  if (flags != 0) {
-    set_error (EINVAL, "invalid flag: %" PRIu32, flags);
-    return -1;
+    if (flags != 0) {
+      set_error (EINVAL, "invalid flag: %" PRIu32, flags);
+      return -1;
+    }
   }

   return nbd_internal_command_common (h, 0, NBD_CMD_CACHE, offset, count,
@@ -425,31 +437,33 @@ nbd_unlocked_aio_zero (struct nbd_handle *h,
 {
   struct command_cb cb = { .completion = completion };

-  if (nbd_unlocked_can_zero (h) != 1) {
-    set_error (EINVAL, "server does not support zero operations");
-    return -1;
-  }
-  if (nbd_unlocked_is_read_only (h) == 1) {
-    set_error (EPERM, "server does not support write operations");
-    return -1;
-  }
+  if (h->strict & LIBNBD_STRICT_COMMANDS) {
+    if (nbd_unlocked_can_zero (h) != 1) {
+      set_error (EINVAL, "server does not support zero operations");
+      return -1;
+    }
+    if (nbd_unlocked_is_read_only (h) == 1) {
+      set_error (EPERM, "server does not support write operations");
+      return -1;
+    }

-  if ((flags & ~(LIBNBD_CMD_FLAG_FUA | LIBNBD_CMD_FLAG_NO_HOLE |
-                 LIBNBD_CMD_FLAG_FAST_ZERO)) != 0) {
-    set_error (EINVAL, "invalid flag: %" PRIu32, flags);
-    return -1;
-  }
+    if ((flags & ~(LIBNBD_CMD_FLAG_FUA | LIBNBD_CMD_FLAG_NO_HOLE |
+                   LIBNBD_CMD_FLAG_FAST_ZERO)) != 0) {
+      set_error (EINVAL, "invalid flag: %" PRIu32, flags);
+      return -1;
+    }

-  if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 &&
-      nbd_unlocked_can_fua (h) != 1) {
-    set_error (EINVAL, "server does not support the FUA flag");
-    return -1;
-  }
+    if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 &&
+        nbd_unlocked_can_fua (h) != 1) {
+      set_error (EINVAL, "server does not support the FUA flag");
+      return -1;
+    }

-  if ((flags & LIBNBD_CMD_FLAG_FAST_ZERO) != 0 &&
-      nbd_unlocked_can_fast_zero (h) != 1) {
-    set_error (EINVAL, "server does not support the fast zero flag");
-    return -1;
+    if ((flags & LIBNBD_CMD_FLAG_FAST_ZERO) != 0 &&
+        nbd_unlocked_can_fast_zero (h) != 1) {
+      set_error (EINVAL, "server does not support the fast zero
flag");
+      return -1;
+    }
   }

   if (count == 0) {             /* NBD protocol forbids this. */
@@ -471,21 +485,23 @@ nbd_unlocked_aio_block_status (struct nbd_handle *h,
   struct command_cb cb = { .fn.extent = extent,
                            .completion = completion };

-  if (!h->structured_replies) {
-    set_error (ENOTSUP, "server does not support structured
replies");
-    return -1;
-  }
+  if (h->strict & LIBNBD_STRICT_COMMANDS) {
+    if (!h->structured_replies) {
+      set_error (ENOTSUP, "server does not support structured
replies");
+      return -1;
+    }

-  if (h->meta_contexts == NULL) {
-    set_error (ENOTSUP, "did not negotiate any metadata contexts, "
-               "either you did not call nbd_add_meta_context before "
-               "connecting or the server does not support it");
-    return -1;
-  }
+    if (h->meta_contexts == NULL) {
+      set_error (ENOTSUP, "did not negotiate any metadata contexts, "
+                 "either you did not call nbd_add_meta_context before
"
+                 "connecting or the server does not support it");
+      return -1;
+    }

-  if ((flags & ~LIBNBD_CMD_FLAG_REQ_ONE) != 0) {
-    set_error (EINVAL, "invalid flag: %" PRIu32, flags);
-    return -1;
+    if ((flags & ~LIBNBD_CMD_FLAG_REQ_ONE) != 0) {
+      set_error (EINVAL, "invalid flag: %" PRIu32, flags);
+      return -1;
+    }
   }

   if (count == 0) {             /* NBD protocol forbids this. */
diff --git a/tests/errors.c b/tests/errors.c
index ef8303e..a8e1b0c 100644
--- a/tests/errors.c
+++ b/tests/errors.c
@@ -86,6 +86,7 @@ main (int argc, char *argv[])
    */
   const char *cmd[] = { "nbdkit", "-s",
"--exit-with-parent", "sh",
                         script, NULL };
+  uint32_t strict;

   progname = argv[0];

@@ -214,7 +215,25 @@ main (int argc, char *argv[])
   }
   check (EINVAL, "nbd_pread: ");

-  /* Use unknown command flags */
+  /* Use unknown command flags, client-side */
+  strict = nbd_get_strict_mode (nbd) | LIBNBD_STRICT_COMMANDS;
+  if (nbd_set_strict_mode (nbd, strict) == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+  if (nbd_pread (nbd, NULL, 0, 0, -1) != -1) {
+    fprintf (stderr, "%s: test failed: "
+             "nbd_pread did not fail with bogus flags\n",
+             argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  check (EINVAL, "nbd_pread: ");
+  /* Use unknown command flags, server-side */
+  strict &= ~LIBNBD_STRICT_COMMANDS;
+  if (nbd_set_strict_mode (nbd, strict) == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
   if (nbd_pread (nbd, NULL, 0, 0, -1) != -1) {
     fprintf (stderr, "%s: test failed: "
              "nbd_pread did not fail with bogus flags\n",
@@ -240,7 +259,25 @@ main (int argc, char *argv[])
   }
   check (ERANGE, "nbd_aio_pwrite: ");

-  /* Use unadvertised command */
+  /* Use unadvertised command, client-side */
+  strict |= LIBNBD_STRICT_COMMANDS;
+  if (nbd_set_strict_mode (nbd, strict) == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+  if (nbd_trim (nbd, 512, 0, 0) != -1) {
+    fprintf (stderr, "%s: test failed: "
+             "unpermitted nbd_trim did not fail\n",
+             argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  check (EINVAL, "nbd_trim: ");
+  /* Use unadvertised command, server-side */
+  strict &= ~LIBNBD_STRICT_COMMANDS;
+  if (nbd_set_strict_mode (nbd, strict) == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
   if (nbd_trim (nbd, 512, 0, 0) != -1) {
     fprintf (stderr, "%s: test failed: "
              "unpermitted nbd_trim did not fail\n",
-- 
2.28.0

The NBD protocol states that a 0-length request is undefined; we were
inconsistent in that we let it through for read, write, and cache, but
blocked it for trim, zero, and block_status.  The NBD protocol also
has documented rules on handling access beyond EOF, but we are
currently wasting traffic to the server when we can give the same
answer ourselves.  Exposing this as a strictness knob gives the user
finer-grained control over what behavior they want.

---

Question: should this be split into two flags, one to prevent 0-length
requests, and the other to prevent out-of-bounds requests?
---
 generator/API.ml |  7 ++++
 lib/handle.c     |  2 +-
 lib/rw.c         | 87 +++++++++++++++++++++++++++++++++++++++++++-----
 tests/errors.c   | 20 ++++++++++-
 4 files changed, 105 insertions(+), 11 deletions(-)

diff --git a/generator/API.ml b/generator/API.ml
index 8811d3c..10d145c 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -175,6 +175,7 @@ let strict_flags = {
   flag_prefix = "STRICT";
   flags = [
     "COMMANDS",       1 lsl 0;
+    "BOUNDS",         1 lsl 1;
   ]
 }
 let allow_transport_flags = {
@@ -728,6 +729,12 @@ set of advertised server flags (for example, attempting a
write on
 a read-only server).  If clear, this flag relies on the server to
 reject unexpected commands or unknown flags to supported commands.

+=item C<LIBNBD_STRICT_BOUNDS> = 2
+
+If set, this flag rejects client requests that would exceed the export
+bounds without sending any traffic to the server.  If clear, this flag
+relies on the server to detect out-of-bounds requests.
+
 =back

 Future versions of libnbd may add further flags.
diff --git a/lib/handle.c b/lib/handle.c
index a1fa142..817fcd3 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -75,7 +75,7 @@ nbd_create (void)
   s = getenv ("LIBNBD_DEBUG");
   h->debug = s && strcmp (s, "1") == 0;

-  h->strict = LIBNBD_STRICT_COMMANDS;
+  h->strict = LIBNBD_STRICT_COMMANDS | LIBNBD_STRICT_BOUNDS;

   h->public_state = STATE_START;
   h->state = STATE_START;
diff --git a/lib/rw.c b/lib/rw.c
index b5c1698..adfb7ac 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -278,6 +278,18 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf,
     }
   }

+  if (h->strict & LIBNBD_STRICT_BOUNDS) {
+    if (count == 0) {
+      set_error (EINVAL, "count cannot be 0");
+      return -1;
+    }
+
+    if (offset > h->exportsize || offset + count > h->exportsize) {
+      set_error (EINVAL, "request out of bounds");
+      return -1;
+    }
+  }
+
   return nbd_internal_command_common (h, 0, NBD_CMD_READ, offset, count,
                                       buf, &cb);
 }
@@ -305,6 +317,18 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle *h,
void *buf,
     }
   }

+  if (h->strict & LIBNBD_STRICT_BOUNDS) {
+    if (count == 0) {
+      set_error (EINVAL, "count cannot be 0");
+      return -1;
+    }
+
+    if (offset > h->exportsize || offset + count > h->exportsize) {
+      set_error (EINVAL, "request out of bounds");
+      return -1;
+    }
+  }
+
   return nbd_internal_command_common (h, flags, NBD_CMD_READ, offset, count,
                                       buf, &cb);
 }
@@ -335,6 +359,18 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void
*buf,
     }
   }

+  if (h->strict & LIBNBD_STRICT_BOUNDS) {
+    if (count == 0) {
+      set_error (EINVAL, "count cannot be 0");
+      return -1;
+    }
+
+    if (offset > h->exportsize || offset + count > h->exportsize) {
+      set_error (ENOSPC, "request out of bounds");
+      return -1;
+    }
+  }
+
   return nbd_internal_command_common (h, flags, NBD_CMD_WRITE, offset, count,
                                       (void *) buf, &cb);
 }
@@ -392,9 +428,16 @@ nbd_unlocked_aio_trim (struct nbd_handle *h,
     }
   }

-  if (count == 0) {             /* NBD protocol forbids this. */
-    set_error (EINVAL, "count cannot be 0");
-    return -1;
+  if (h->strict & LIBNBD_STRICT_BOUNDS) {
+    if (count == 0) {             /* NBD protocol forbids this. */
+      set_error (EINVAL, "count cannot be 0");
+      return -1;
+    }
+
+    if (offset > h->exportsize || offset + count > h->exportsize) {
+      set_error (ENOSPC, "request out of bounds");
+      return -1;
+    }
   }

   return nbd_internal_command_common (h, flags, NBD_CMD_TRIM, offset, count,
@@ -425,6 +468,18 @@ nbd_unlocked_aio_cache (struct nbd_handle *h,
     }
   }

+  if (h->strict & LIBNBD_STRICT_BOUNDS) {
+    if (count == 0) {
+      set_error (EINVAL, "count cannot be 0");
+      return -1;
+    }
+
+    if (offset > h->exportsize || offset + count > h->exportsize) {
+      set_error (EINVAL, "request out of bounds");
+      return -1;
+    }
+  }
+
   return nbd_internal_command_common (h, 0, NBD_CMD_CACHE, offset, count,
                                       NULL, &cb);
 }
@@ -466,9 +521,16 @@ nbd_unlocked_aio_zero (struct nbd_handle *h,
     }
   }

-  if (count == 0) {             /* NBD protocol forbids this. */
-    set_error (EINVAL, "count cannot be 0");
-    return -1;
+  if (h->strict & LIBNBD_STRICT_BOUNDS) {
+    if (count == 0) {             /* NBD protocol forbids this. */
+      set_error (EINVAL, "count cannot be 0");
+      return -1;
+    }
+
+    if (offset > h->exportsize || offset + count > h->exportsize) {
+      set_error (ENOSPC, "request out of bounds");
+      return -1;
+    }
   }

   return nbd_internal_command_common (h, flags, NBD_CMD_WRITE_ZEROES, offset,
@@ -504,9 +566,16 @@ nbd_unlocked_aio_block_status (struct nbd_handle *h,
     }
   }

-  if (count == 0) {             /* NBD protocol forbids this. */
-    set_error (EINVAL, "count cannot be 0");
-    return -1;
+  if (h->strict & LIBNBD_STRICT_BOUNDS) {
+    if (count == 0) {             /* NBD protocol forbids this. */
+      set_error (EINVAL, "count cannot be 0");
+      return -1;
+    }
+
+    if (offset > h->exportsize || offset + count > h->exportsize) {
+      set_error (EINVAL, "request out of bounds");
+      return -1;
+    }
   }

   return nbd_internal_command_common (h, flags, NBD_CMD_BLOCK_STATUS, offset,
diff --git a/tests/errors.c b/tests/errors.c
index a8e1b0c..720033b 100644
--- a/tests/errors.c
+++ b/tests/errors.c
@@ -206,7 +206,25 @@ main (int argc, char *argv[])
   }
   check (EINVAL, "nbd_aio_command_completed: ");

-  /* Read from an invalid offset */
+  /* Read from an invalid offset, client-side */
+  strict = nbd_get_strict_mode (nbd) | LIBNBD_STRICT_BOUNDS;
+  if (nbd_set_strict_mode (nbd, strict) == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+  if (nbd_pread (nbd, NULL, 0, -1, 0) != -1) {
+    fprintf (stderr, "%s: test failed: "
+             "nbd_pread did not fail with bogus offset\n",
+             argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  check (EINVAL, "nbd_pread: ");
+  /* Read from an invalid offset, server-side */
+  strict &= ~LIBNBD_STRICT_BOUNDS;
+  if (nbd_set_strict_mode (nbd, strict) == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
   if (nbd_pread (nbd, NULL, 0, -1, 0) != -1) {
     fprintf (stderr, "%s: test failed: "
              "nbd_pread did not fail with bogus offset\n",
-- 
2.28.0

On Fri, Sep 04, 2020 at 04:18:41PM -0500, Eric Blake
wrote:
> Right now, libnbd has refused to issue a command not advertised as
> supported by a server, including unknown flags, mainly because the NBD
> protocol does not guarantee what the server will do, and libnbd would
> rather stay in sync with the server than drop the connection.
> However, for integration purposes, it can be handy to coerce libnbd
> into sending something to see how the server will react (whether it be
> an extension libnbd has not yet learned, or an intentional bad request
> to test server error handling).  Time to make this something the user
> can control, by adding a new strictness mode.  A later patch will add
> another knob to the mode.
> 
> ---
> 
> Question: Should we split this into two knobs?  Sending unknown flags
> is dangerous (in the future, a flag might be defined to alter the
> on-wire layout expected by either the client or server, and get things
> out-of-sync or even cause data corruption), while sending known
> commands contrary to advertised flags is less risky (so far,
> NBD_CMD_WRITE is the only command that alters what the client sends,
> so a server can easily reject unknown commands by assuming they match
> the layout of NBD_CMD_READ; and I just fixed a long-standing bug where
> we were sending trim and zero requests contrary to server
> advertisement).  Trying to provoke a write to a read-only server is
> different than trying to experiment with an unknown flag, which is why
> it might make sense to have two separate knobs for the gating done in
> this patch.
> 
Patch looks fine, ACK.

I've no particular opinion on whether this should be one or two flags.
Whatever you think is best.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Fri, Sep 04, 2020 at 04:18:42PM -0500, Eric Blake
wrote:
> The NBD protocol states that a 0-length request is undefined; we were
> inconsistent in that we let it through for read, write, and cache, but
> blocked it for trim, zero, and block_status.  The NBD protocol also
> has documented rules on handling access beyond EOF, but we are
> currently wasting traffic to the server when we can give the same
> answer ourselves.  Exposing this as a strictness knob gives the user
> finer-grained control over what behavior they want.
> 
> ---
> 
> Question: should this be split into two flags, one to prevent 0-length
> requests, and the other to prevent out-of-bounds requests?
Looks fine, ACK.

My only comment would be that it might be better to factor the
checking code into a new function -- it looks like the checks only
differ by one errno value.

Two flags could be better here because I can see it might be useful to
disable one check but not the other, but up to you.

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