[Libguestfs] [libnbd PATCH 2/2] api: Add STRICT_BOUNDS to nbd_set_strict_mode

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