[Libguestfs] [libnbd PATCH v2 5/5] api: Add STRICT_BOUNDS/ZERO_SIZE 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 these as two more strictness knobs gives the user finer-grained control over what behavior they want. --- lib/internal.h | 3 ++- generator/API.ml | 14 ++++++++++++++ lib/disconnect.c | 3 ++- lib/rw.c | 47 +++++++++++++++++++++++------------------------ tests/errors.c | 20 +++++++++++++++++++- 5 files changed, 60 insertions(+), 27 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 2a5147f..cde5dcd 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -432,7 +432,8 @@ extern const char *nbd_internal_name_of_nbd_cmd (uint16_t type); extern int64_t nbd_internal_command_common (struct nbd_handle *h, uint16_t flags, uint16_t type, uint64_t offset, uint64_t count, - void *data, struct command_cb *cb); + int count_err, void *data, + struct command_cb *cb); /* socket.c */ struct socket *nbd_internal_socket_create (int fd); diff --git a/generator/API.ml b/generator/API.ml index 4cd425b..d3b1d1b 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -189,6 +189,8 @@ let strict_flags = { flags = [ "COMMANDS", 1 lsl 0; "FLAGS", 1 lsl 1; + "BOUNDS", 1 lsl 2; + "ZERO_SIZE", 1 lsl 3; ] } let allow_transport_flags = { @@ -772,6 +774,18 @@ Note that the NBD protocol only supports 16 bits of command flags, even though the libnbd API uses C<uint32_t>; bits outside of the range permitted by the protocol are always a client-side error. +=item C<LIBNBD_STRICT_BOUNDS> = 3 + +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. + +=item C<LIBNBD_STRICT_ZERO_SIZE> = 4 + +If set, this flag rejects client requests with length 0. If clear, +this permits zero-length requests to the server, which may produce +undefined results. + =back For convenience, the constant C<LIBNBD_STRICT_MASK> is available to diff --git a/lib/disconnect.c b/lib/disconnect.c index f99fbd0..822abc1 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -64,7 +64,8 @@ nbd_unlocked_aio_disconnect (struct nbd_handle *h, uint32_t flags) { int64_t id; - id = nbd_internal_command_common (h, flags, NBD_CMD_DISC, 0, 0, NULL, NULL); + id = nbd_internal_command_common (h, flags, NBD_CMD_DISC, 0, 0, 0, + NULL, NULL); if (id == -1) return -1; h->disconnect_request = true; diff --git a/lib/rw.c b/lib/rw.c index e3e80ad..5dbc947 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -168,10 +168,11 @@ nbd_unlocked_block_status (struct nbd_handle *h, return wait_for_command (h, cookie); } +/* count_err represents the errno to return if bounds check fail */ int64_t nbd_internal_command_common (struct nbd_handle *h, uint16_t flags, uint16_t type, - uint64_t offset, uint64_t count, + uint64_t offset, uint64_t count, int count_err, void *data, struct command_cb *cb) { struct command *cmd; @@ -185,6 +186,19 @@ nbd_internal_command_common (struct nbd_handle *h, goto err; } + if (count_err) { + if ((h->strict & LIBNBD_STRICT_ZERO_SIZE) && count == 0) { + set_error (EINVAL, "count cannot be 0"); + goto err; + } + + if ((h->strict & LIBNBD_STRICT_BOUNDS) && + (offset > h->exportsize || offset + count > h->exportsize)) { + set_error (count_err, "request out of bounds"); + return -1; + } + } + switch (type) { /* Commands which send or receive data are limited to MAX_REQUEST_SIZE. */ case NBD_CMD_READ: @@ -282,7 +296,7 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf, SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_READ, offset, count, - buf, &cb); + EINVAL, buf, &cb); } int64_t @@ -306,7 +320,7 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle *h, void *buf, SET_CALLBACK_TO_NULL (*chunk); SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_READ, offset, count, - buf, &cb); + EINVAL, buf, &cb); } int64_t @@ -332,7 +346,7 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void *buf, SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_WRITE, offset, count, - (void *) buf, &cb); + ENOSPC, (void *) buf, &cb); } int64_t @@ -351,7 +365,7 @@ nbd_unlocked_aio_flush (struct nbd_handle *h, SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_FLUSH, 0, 0, - NULL, &cb); + 0, NULL, &cb); } int64_t @@ -379,14 +393,9 @@ nbd_unlocked_aio_trim (struct nbd_handle *h, } } - if (count == 0) { /* NBD protocol forbids this. */ - set_error (EINVAL, "count cannot be 0"); - return -1; - } - SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_TRIM, offset, count, - NULL, &cb); + ENOSPC, NULL, &cb); } int64_t @@ -410,7 +419,7 @@ nbd_unlocked_aio_cache (struct nbd_handle *h, SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_CACHE, offset, count, - NULL, &cb); + EINVAL, NULL, &cb); } int64_t @@ -444,14 +453,9 @@ nbd_unlocked_aio_zero (struct nbd_handle *h, } } - if (count == 0) { /* NBD protocol forbids this. */ - set_error (EINVAL, "count cannot be 0"); - return -1; - } - SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_WRITE_ZEROES, offset, - count, NULL, &cb); + count, ENOSPC, NULL, &cb); } int64_t @@ -478,13 +482,8 @@ 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; - } - SET_CALLBACK_TO_NULL (*extent); SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, flags, NBD_CMD_BLOCK_STATUS, offset, - count, NULL, &cb); + count, EINVAL, NULL, &cb); } diff --git a/tests/errors.c b/tests/errors.c index 0cfcac5..74bd17e 100644 --- a/tests/errors.c +++ b/tests/errors.c @@ -235,7 +235,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, buf, 1, -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, buf, 1, -1, 0) != -1) { fprintf (stderr, "%s: test failed: " "nbd_pread did not fail with bogus offset\n", -- 2.28.0