[PATCH v3 for-4.2 0/4] Better NBD string length handling

[PATCH v3 for-4.2 0/4] Better NBD string length handling

[Eric Blake]

This series was originally posted before soft freeze, but then KVM
Forum interfered. I think that patches 1-3 are bug fixes still
appropriate for -rc2 if they get good reviews, but patch 4 is a new
feature and now only appropriate for 5.0.

Since v2:
- Patch 1, 2: new [Vladimir]
- Patch 3: improve error messages and fix a memleak [Vladimir]
- Patch 3: bump name length from 256 to 4k (R-b dropped)
- Patch 4: add R-b, but tweak to defer to 5.0

Eric Blake (4):
  nbd/server: Prefer heap over stack for parsing client names
  bitmap: Enforce maximum bitmap name length
  nbd: Don't send oversize strings
  nbd: Allow description when creating NBD blockdev

 qapi/block-core.json         |  2 +-
 qapi/block.json              |  9 +++++---
 include/block/dirty-bitmap.h |  2 ++
 include/block/nbd.h          | 12 +++++-----
 block/dirty-bitmap.c         | 12 +++++++---
 block/nbd.c                  | 10 +++++++++
 block/qcow2-bitmap.c         |  2 ++
 blockdev-nbd.c               | 14 +++++++++++-
 monitor/hmp-cmds.c           |  4 ++--
 nbd/client.c                 | 18 ++++++++++++---
 nbd/server.c                 | 43 ++++++++++++++++++++++++------------
 qemu-nbd.c                   |  9 ++++++++
 tests/qemu-iotests/223       |  2 +-
 tests/qemu-iotests/223.out   |  1 +
 14 files changed, 106 insertions(+), 34 deletions(-)

-- 
2.21.0

[PATCH v3 1/4] nbd/server: Prefer heap over stack for parsing client names

[Eric Blake]

As long as we limit NBD names to 256 bytes (the bare minimum permitted
by the standard), stack-allocation works for parsing a name received
from the client.  But as mentioned in a comment, we eventually want to
permit up to the 4k maximum of the NBD standard, which is too large
for stack allocation; so switch everything in the server to use heap
allocation.  For now, there is no change in actually supported name
length.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h | 10 +++++-----
 nbd/server.c        | 25 +++++++++++++++----------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 316fd705a9e4..c306423dc85c 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -226,11 +226,11 @@ enum {
 /* Maximum size of a single READ/WRITE data buffer */
 #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)

-/* Maximum size of an export name. The NBD spec requires 256 and
- * suggests that servers support up to 4096, but we stick to only the
- * required size so that we can stack-allocate the names, and because
- * going larger would require an audit of more code to make sure we
- * aren't overflowing some other buffer. */
+/*
+ * Maximum size of an export name. The NBD spec requires a minimum of
+ * 256 and recommends that servers support up to 4096; all users use
+ * malloc so we can bump this constant without worry.
+ */
 #define NBD_MAX_NAME_SIZE 256

 /* Two types of reply structures */
diff --git a/nbd/server.c b/nbd/server.c
index d8d1e6245532..c63b76b22735 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -324,18 +324,20 @@ static int nbd_opt_skip(NBDClient *client, size_t size, Error **errp)
  *   uint32_t len     (<= NBD_MAX_NAME_SIZE)
  *   len bytes string (not 0-terminated)
  *
- * @name should be enough to store NBD_MAX_NAME_SIZE+1.
+ * On success, @name will be allocated.
  * If @length is non-null, it will be set to the actual string length.
  *
  * Return -errno on I/O error, 0 if option was completely handled by
  * sending a reply about inconsistent lengths, or 1 on success.
  */
-static int nbd_opt_read_name(NBDClient *client, char *name, uint32_t *length,
+static int nbd_opt_read_name(NBDClient *client, char **name, uint32_t *length,
                              Error **errp)
 {
     int ret;
     uint32_t len;
+    g_autofree char *local_name = NULL;

+    *name = NULL;
     ret = nbd_opt_read(client, &len, sizeof(len), errp);
     if (ret <= 0) {
         return ret;
@@ -347,15 +349,17 @@ static int nbd_opt_read_name(NBDClient *client, char *name, uint32_t *length,
                                "Invalid name length: %" PRIu32, len);
     }

-    ret = nbd_opt_read(client, name, len, errp);
+    local_name = g_malloc(len + 1);
+    ret = nbd_opt_read(client, local_name, len, errp);
     if (ret <= 0) {
         return ret;
     }
-    name[len] = '\0';
+    local_name[len] = '\0';

     if (length) {
         *length = len;
     }
+    *name = g_steal_pointer(&local_name);

     return 1;
 }
@@ -427,7 +431,7 @@ static void nbd_check_meta_export(NBDClient *client)
 static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
                                             Error **errp)
 {
-    char name[NBD_MAX_NAME_SIZE + 1];
+    g_autofree char *name;
     char buf[NBD_REPLY_EXPORT_NAME_SIZE] = "";
     size_t len;
     int ret;
@@ -441,10 +445,11 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
         [10 .. 133]   reserved     (0) [unless no_zeroes]
      */
     trace_nbd_negotiate_handle_export_name();
-    if (client->optlen >= sizeof(name)) {
+    if (client->optlen > NBD_MAX_NAME_SIZE) {
         error_setg(errp, "Bad length received");
         return -EINVAL;
     }
+    name = g_malloc(client->optlen + 1);
     if (nbd_read(client->ioc, name, client->optlen, "export name", errp) < 0) {
         return -EIO;
     }
@@ -533,7 +538,7 @@ static int nbd_reject_length(NBDClient *client, bool fatal, Error **errp)
 static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
 {
     int rc;
-    char name[NBD_MAX_NAME_SIZE + 1];
+    g_autofree char *name = NULL;
     NBDExport *exp;
     uint16_t requests;
     uint16_t request;
@@ -551,7 +556,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
         2 bytes: N, number of requests (can be 0)
         N * 2 bytes: N requests
     */
-    rc = nbd_opt_read_name(client, name, &namelen, errp);
+    rc = nbd_opt_read_name(client, &name, &namelen, errp);
     if (rc <= 0) {
         return rc;
     }
@@ -957,7 +962,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
                                       NBDExportMetaContexts *meta, Error **errp)
 {
     int ret;
-    char export_name[NBD_MAX_NAME_SIZE + 1];
+    g_autofree char *export_name = NULL;
     NBDExportMetaContexts local_meta;
     uint32_t nb_queries;
     int i;
@@ -976,7 +981,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client,

     memset(meta, 0, sizeof(*meta));

-    ret = nbd_opt_read_name(client, export_name, NULL, errp);
+    ret = nbd_opt_read_name(client, &export_name, NULL, errp);
     if (ret <= 0) {
         return ret;
     }
-- 
2.21.0

[PATCH v3 2/4] bitmap: Enforce maximum bitmap name length

[Eric Blake]

We document that for qcow2 persistent bitmaps, the name cannot exceed
1023 bytes.  It is inconsistent if transient bitmaps do not have to
abide by the same limit, and it is unlikely that any existing client
even cares about using bitmap names this long.  It's time to codify
that ALL bitmaps managed by qemu (whether persistent in qcow2 or not)
have a documented maximum length.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qapi/block-core.json         |  2 +-
 include/block/dirty-bitmap.h |  2 ++
 block/dirty-bitmap.c         | 12 +++++++++---
 block/qcow2-bitmap.c         |  2 ++
 4 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index aa97ee264112..0cf68fea1450 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2042,7 +2042,7 @@
 #
 # @node: name of device/node which the bitmap is tracking
 #
-# @name: name of the dirty bitmap
+# @name: name of the dirty bitmap (must be less than 1024 bytes)
 #
 # @granularity: the bitmap granularity, default is 64k for
 #               block-dirty-bitmap-add
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 958e7474fb51..e2b20ecab9a3 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -14,6 +14,8 @@ typedef enum BitmapCheckFlags {
                              BDRV_BITMAP_INCONSISTENT)
 #define BDRV_BITMAP_ALLOW_RO (BDRV_BITMAP_BUSY | BDRV_BITMAP_INCONSISTENT)

+#define BDRV_BITMAP_MAX_NAME_SIZE 1023
+
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
                                           uint32_t granularity,
                                           const char *name,
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 4bbb251b2c9c..7039e8252009 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -104,9 +104,15 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,

     assert(is_power_of_2(granularity) && granularity >= BDRV_SECTOR_SIZE);

-    if (name && bdrv_find_dirty_bitmap(bs, name)) {
-        error_setg(errp, "Bitmap already exists: %s", name);
-        return NULL;
+    if (name) {
+        if (bdrv_find_dirty_bitmap(bs, name)) {
+            error_setg(errp, "Bitmap already exists: %s", name);
+            return NULL;
+        }
+        if (strlen(name) > BDRV_BITMAP_MAX_NAME_SIZE) {
+            error_setg(errp, "Bitmap name too long: %s", name);
+            return NULL;
+        }
     }
     bitmap_size = bdrv_getlength(bs);
     if (bitmap_size < 0) {
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index ef9ef628a0d0..809bbc5d20c8 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -42,6 +42,8 @@
 #define BME_MIN_GRANULARITY_BITS 9
 #define BME_MAX_NAME_SIZE 1023

+QEMU_BUILD_BUG_ON(BME_MAX_NAME_SIZE != BDRV_BITMAP_MAX_NAME_SIZE);
+
 #if BME_MAX_TABLE_SIZE * 8ULL > INT_MAX
 #error In the code bitmap table physical size assumed to fit into int
 #endif
-- 
2.21.0

[PATCH v3 3/4] nbd: Don't send oversize strings

[Eric Blake]

Qemu as server currently won't accept export names larger than 256
bytes, nor create dirty bitmap names longer than 1023 bytes, so most
uses of qemu as client or server have no reason to get anywhere near
the NBD spec maximum of a 4k limit per string.

However, we weren't actually enforcing things, ignoring when the
remote side violates the protocol on input, and also having several
code paths where we send oversize strings on output (for example,
qemu-nbd --description could easily send more than 4k).  Tighten
things up as follows:

client:
- Perform bounds check on export name and dirty bitmap request prior
  to handing it to server
- Validate that copied server replies are not too long (ignoring
  NBD_INFO_* replies that are not copied is not too bad)
server:
- Perform bounds check on export name and description prior to
  advertising it to client
- Reject client name or metadata query that is too long
- Adjust things to allow full 4k name limit rather than previous
  256 byte limit

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h |  8 ++++----
 block/nbd.c         | 10 ++++++++++
 blockdev-nbd.c      |  5 +++++
 nbd/client.c        | 18 +++++++++++++++---
 nbd/server.c        | 20 +++++++++++++++-----
 qemu-nbd.c          |  9 +++++++++
 6 files changed, 58 insertions(+), 12 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index c306423dc85c..7f46932d80f1 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -227,11 +227,11 @@ enum {
 #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)

 /*
- * Maximum size of an export name. The NBD spec requires a minimum of
- * 256 and recommends that servers support up to 4096; all users use
- * malloc so we can bump this constant without worry.
+ * Maximum size of a protocol string (export name, meta context name,
+ * etc.).  Use malloc rather than stack allocation for storage of a
+ * string.
  */
-#define NBD_MAX_NAME_SIZE 256
+#define NBD_MAX_STRING_SIZE 4096

 /* Two types of reply structures */
 #define NBD_SIMPLE_REPLY_MAGIC      0x67446698
diff --git a/block/nbd.c b/block/nbd.c
index 123976171cf4..5f18f78a9471 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1832,6 +1832,10 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options,
     }

     s->export = g_strdup(qemu_opt_get(opts, "export"));
+    if (s->export && strlen(s->export) > NBD_MAX_STRING_SIZE) {
+        error_setg(errp, "export name too long to send to server");
+        goto error;
+    }

     s->tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds"));
     if (s->tlscredsid) {
@@ -1849,6 +1853,11 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options,
     }

     s->x_dirty_bitmap = g_strdup(qemu_opt_get(opts, "x-dirty-bitmap"));
+    if (s->x_dirty_bitmap && strlen(s->x_dirty_bitmap) > NBD_MAX_STRING_SIZE) {
+        error_setg(errp, "x-dirty-bitmap query too long to send to server");
+        goto error;
+    }
+
     s->reconnect_delay = qemu_opt_get_number(opts, "reconnect-delay", 0);

     ret = 0;
@@ -1859,6 +1868,7 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options,
         qapi_free_SocketAddress(s->saddr);
         g_free(s->export);
         g_free(s->tlscredsid);
+        g_free(s->x_dirty_bitmap);
     }
     qemu_opts_del(opts);
     return ret;
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 6a8b206e1d74..8c20baa4a4b9 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -162,6 +162,11 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
         name = device;
     }

+    if (strlen(name) > NBD_MAX_STRING_SIZE) {
+        error_setg(errp, "export name '%s' too long", name);
+        return;
+    }
+
     if (nbd_export_find(name)) {
         error_setg(errp, "NBD server already has export named '%s'", name);
         return;
diff --git a/nbd/client.c b/nbd/client.c
index f6733962b49b..ba173108baab 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -289,8 +289,8 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,
         return -1;
     }
     len -= sizeof(namelen);
-    if (len < namelen) {
-        error_setg(errp, "incorrect option name length");
+    if (len < namelen || namelen > NBD_MAX_STRING_SIZE) {
+        error_setg(errp, "incorrect name length in server's list response");
         nbd_send_opt_abort(ioc);
         return -1;
     }
@@ -303,6 +303,12 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,
     local_name[namelen] = '\0';
     len -= namelen;
     if (len) {
+        if (len > NBD_MAX_STRING_SIZE) {
+            error_setg(errp, "incorrect description length in server's "
+                       "list response");
+            nbd_send_opt_abort(ioc);
+            return -1;
+        }
         local_desc = g_malloc(len + 1);
         if (nbd_read(ioc, local_desc, len, "export description", errp) < 0) {
             nbd_send_opt_abort(ioc);
@@ -479,6 +485,10 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
             break;

         default:
+            /*
+             * Not worth the bother to check if NBD_INFO_NAME or
+             * NBD_INFO_DESCRIPTION exceed NBD_MAX_STRING_SIZE.
+             */
             trace_nbd_opt_info_unknown(type, nbd_info_lookup(type));
             if (nbd_drop(ioc, len, errp) < 0) {
                 error_prepend(errp, "Failed to read info payload: ");
@@ -645,9 +655,11 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t opt,
     char *p;

     data_len = sizeof(export_len) + export_len + sizeof(queries);
+    assert(export_len <= NBD_MAX_STRING_SIZE);
     if (query) {
         query_len = strlen(query);
         data_len += sizeof(query_len) + query_len;
+        assert(query_len <= NBD_MAX_STRING_SIZE);
     } else {
         assert(opt == NBD_OPT_LIST_META_CONTEXT);
     }
@@ -1009,7 +1021,7 @@ int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc,
     bool zeroes;
     bool base_allocation = info->base_allocation;

-    assert(info->name);
+    assert(info->name && strlen(info->name) <= NBD_MAX_STRING_SIZE);
     trace_nbd_receive_negotiate_name(info->name);

     result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname, outioc,
diff --git a/nbd/server.c b/nbd/server.c
index c63b76b22735..d28123c562be 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -321,7 +321,7 @@ static int nbd_opt_skip(NBDClient *client, size_t size, Error **errp)
 /* nbd_opt_read_name
  *
  * Read a string with the format:
- *   uint32_t len     (<= NBD_MAX_NAME_SIZE)
+ *   uint32_t len     (<= NBD_MAX_STRING_SIZE)
  *   len bytes string (not 0-terminated)
  *
  * On success, @name will be allocated.
@@ -344,7 +344,7 @@ static int nbd_opt_read_name(NBDClient *client, char **name, uint32_t *length,
     }
     len = cpu_to_be32(len);

-    if (len > NBD_MAX_NAME_SIZE) {
+    if (len > NBD_MAX_STRING_SIZE) {
         return nbd_opt_invalid(client, errp,
                                "Invalid name length: %" PRIu32, len);
     }
@@ -379,6 +379,7 @@ static int nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp,
     trace_nbd_negotiate_send_rep_list(name, desc);
     name_len = strlen(name);
     desc_len = strlen(desc);
+    assert(name_len <= NBD_MAX_STRING_SIZE && desc_len <= NBD_MAX_STRING_SIZE);
     len = name_len + desc_len + sizeof(len);
     ret = nbd_negotiate_send_rep_len(client, NBD_REP_SERVER, len, errp);
     if (ret < 0) {
@@ -445,7 +446,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
         [10 .. 133]   reserved     (0) [unless no_zeroes]
      */
     trace_nbd_negotiate_handle_export_name();
-    if (client->optlen > NBD_MAX_NAME_SIZE) {
+    if (client->optlen > NBD_MAX_STRING_SIZE) {
         error_setg(errp, "Bad length received");
         return -EINVAL;
     }
@@ -613,6 +614,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
     if (exp->description) {
         size_t len = strlen(exp->description);

+        assert(len <= NBD_MAX_STRING_SIZE);
         rc = nbd_negotiate_send_info(client, NBD_INFO_DESCRIPTION,
                                      len, exp->description, errp);
         if (rc < 0) {
@@ -757,6 +759,7 @@ static int nbd_negotiate_send_meta_context(NBDClient *client,
         {.iov_base = (void *)context, .iov_len = strlen(context)}
     };

+    assert(iov[1].iov_len <= NBD_MAX_STRING_SIZE);
     if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
         context_id = 0;
     }
@@ -905,7 +908,7 @@ static int nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta,
  * Parse namespace name and call corresponding function to parse body of the
  * query.
  *
- * The only supported namespace now is 'base'.
+ * The only supported namespaces are 'base' and 'qemu'.
  *
  * The function aims not wasting time and memory to read long unknown namespace
  * names.
@@ -931,6 +934,10 @@ static int nbd_negotiate_meta_query(NBDClient *client,
     }
     len = cpu_to_be32(len);

+    if (len > NBD_MAX_STRING_SIZE) {
+        trace_nbd_negotiate_meta_query_skip("length too long");
+        return nbd_opt_skip(client, len, errp);
+    }
     if (len < ns_len) {
         trace_nbd_negotiate_meta_query_skip("length too short");
         return nbd_opt_skip(client, len, errp);
@@ -1492,7 +1499,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
      * access since the export could be available before migration handover.
      * ctx was acquired in the caller.
      */
-    assert(name);
+    assert(name && strlen(name) <= NBD_MAX_STRING_SIZE);
     ctx = bdrv_get_aio_context(bs);
     bdrv_invalidate_cache(bs, NULL);

@@ -1518,6 +1525,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
     assert(dev_offset <= INT64_MAX);
     exp->dev_offset = dev_offset;
     exp->name = g_strdup(name);
+    assert(!desc || strlen(desc) <= NBD_MAX_STRING_SIZE);
     exp->description = g_strdup(desc);
     exp->nbdflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_FLUSH |
                      NBD_FLAG_SEND_FUA | NBD_FLAG_SEND_CACHE);
@@ -1564,8 +1572,10 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,

         bdrv_dirty_bitmap_set_busy(bm, true);
         exp->export_bitmap = bm;
+        assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
         exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
                                                      bitmap);
+        assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
     }

     exp->close = close;
diff --git a/qemu-nbd.c b/qemu-nbd.c
index caacf0ed7379..108a51f7eb01 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -833,9 +833,18 @@ int main(int argc, char **argv)
             break;
         case 'x':
             export_name = optarg;
+            if (strlen(export_name) > NBD_MAX_STRING_SIZE) {
+                error_report("export name '%s' too long", export_name);
+                exit(EXIT_FAILURE);
+            }
             break;
         case 'D':
             export_description = optarg;
+            if (strlen(export_description) > NBD_MAX_STRING_SIZE) {
+                error_report("export description '%s' too long",
+                             export_description);
+                exit(EXIT_FAILURE);
+            }
             break;
         case 'v':
             verbose = 1;
-- 
2.21.0

[PATCH v3 for-5.0 4/4] nbd: Allow description when creating NBD blockdev

[Eric Blake]

Allow blockdevs to match the feature already present in qemu-nbd -D.
Enhance iotest 223 to cover it.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block.json            | 9 ++++++---
 blockdev-nbd.c             | 9 ++++++++-
 monitor/hmp-cmds.c         | 4 ++--
 tests/qemu-iotests/223     | 2 +-
 tests/qemu-iotests/223.out | 1 +
 5 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/qapi/block.json b/qapi/block.json
index 145c268bb646..7898104dae42 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -250,9 +250,12 @@
 # @name: Export name. If unspecified, the @device parameter is used as the
 #        export name. (Since 2.12)
 #
+# @description: Free-form description of the export, up to 4096 bytes.
+#               (Since 5.0)
+#
 # @writable: Whether clients should be able to write to the device via the
 #     NBD connection (default false).
-
+#
 # @bitmap: Also export the dirty bitmap reachable from @device, so the
 #          NBD client can use NBD_OPT_SET_META_CONTEXT with
 #          "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0)
@@ -263,8 +266,8 @@
 # Since: 1.3.0
 ##
 { 'command': 'nbd-server-add',
-  'data': {'device': 'str', '*name': 'str', '*writable': 'bool',
-           '*bitmap': 'str' } }
+  'data': {'device': 'str', '*name': 'str', '*description': 'str',
+           '*writable': 'bool', '*bitmap': 'str' } }

 ##
 # @NbdServerRemoveMode:
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 8c20baa4a4b9..de2f2ff71320 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -144,6 +144,7 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
 }

 void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
+                        bool has_description, const char *description,
                         bool has_writable, bool writable,
                         bool has_bitmap, const char *bitmap, Error **errp)
 {
@@ -167,6 +168,11 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
         return;
     }

+    if (has_description && strlen(description) > NBD_MAX_STRING_SIZE) {
+        error_setg(errp, "description '%s' too long", description);
+        return;
+    }
+
     if (nbd_export_find(name)) {
         error_setg(errp, "NBD server already has export named '%s'", name);
         return;
@@ -195,7 +201,8 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
         writable = false;
     }

-    exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, !writable, !writable,
+    exp = nbd_export_new(bs, 0, len, name, description, bitmap,
+                         !writable, !writable,
                          NULL, false, on_eject_blk, errp);
     if (!exp) {
         goto out;
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index b2551c16d129..574c6321c9d0 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2352,7 +2352,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
             continue;
         }

-        qmp_nbd_server_add(info->value->device, false, NULL,
+        qmp_nbd_server_add(info->value->device, false, NULL, false, NULL,
                            true, writable, false, NULL, &local_err);

         if (local_err != NULL) {
@@ -2374,7 +2374,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
     bool writable = qdict_get_try_bool(qdict, "writable", false);
     Error *local_err = NULL;

-    qmp_nbd_server_add(device, !!name, name, true, writable,
+    qmp_nbd_server_add(device, !!name, name, false, NULL, true, writable,
                        false, NULL, &local_err);
     hmp_handle_error(mon, &local_err);
 }
diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index b5a80e50bbc1..c708e479325e 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -144,7 +144,7 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
   "bitmap":"b3"}}' "error" # Missing bitmap
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
   "arguments":{"device":"n", "name":"n2", "writable":true,
-  "bitmap":"b2"}}' "return"
+  "description":"some text", "bitmap":"b2"}}' "return"
 $QEMU_NBD_PROG -L -k "$SOCK_DIR/nbd"

 echo
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 23b34fcd202e..16d597585b4f 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -49,6 +49,7 @@ exports available: 2
    base:allocation
    qemu:dirty-bitmap:b
  export: 'n2'
+  description: some text
   size:  4194304
   flags: 0xced ( flush fua trim zeroes df cache fast-zero )
   min block: 1
-- 
2.21.0

Re: [PATCH v3 for-4.2 0/4] Better NBD string length handling

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191114024635.11363-1-eblake@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

                 from /tmp/qemu-test/src/include/qemu/osdep.h:140,
                 from /tmp/qemu-test/src/nbd/server.c:20:
/tmp/qemu-test/src/nbd/server.c: In function 'nbd_negotiate_handle_export_name':
/usr/include/glib-2.0/glib/glib-autocleanups.h:28:10: error: 'name' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   g_free (*pp);
          ^
/tmp/qemu-test/src/nbd/server.c:435:22: note: 'name' was declared here
     g_autofree char *name;
                      ^
cc1: all warnings being treated as errors
make: *** [nbd/server.o] Error 1
make: *** Waiting for unfinished jobs....
  CC      chardev/char-file.o
  CC      chardev/char-io.o
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=9620816161004b4c84746eaba688af5a', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-gsxzfw9n/src/docker-src.2019-11-13-21.55.21.14727:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=9620816161004b4c84746eaba688af5a
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-gsxzfw9n/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    2m9.489s
user    0m7.589s


The full log is available at
http://patchew.org/logs/20191114024635.11363-1-eblake@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v3 1/4] nbd/server: Prefer heap over stack for parsing client names

[Eric Blake]

On 11/13/19 8:46 PM, Eric Blake wrote:
> As long as we limit NBD names to 256 bytes (the bare minimum permitted
> by the standard), stack-allocation works for parsing a name received
> from the client.  But as mentioned in a comment, we eventually want to
> permit up to the 4k maximum of the NBD standard, which is too large
> for stack allocation; so switch everything in the server to use heap
> allocation.  For now, there is no change in actually supported name
> length.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   include/block/nbd.h | 10 +++++-----
>   nbd/server.c        | 25 +++++++++++++++----------
>   2 files changed, 20 insertions(+), 15 deletions(-)

> @@ -427,7 +431,7 @@ static void nbd_check_meta_export(NBDClient *client)
>   static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
>                                               Error **errp)
>   {
> -    char name[NBD_MAX_NAME_SIZE + 1];
> +    g_autofree char *name;

This needs to be:

g_autofree char *name = NULL;

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v3 for-4.2 0/4] Better NBD string length handling

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191114024635.11363-1-eblake@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

                 from /tmp/qemu-test/src/include/qemu/osdep.h:140,
                 from /tmp/qemu-test/src/nbd/server.c:20:
/tmp/qemu-test/src/nbd/server.c: In function 'nbd_negotiate_handle_export_name':
/usr/x86_64-w64-mingw32/sys-root/mingw/include/glib-2.0/glib/glib-autocleanups.h:28:3: error: 'name' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   g_free (*pp);
   ^~~~~~~~~~~~
/tmp/qemu-test/src/nbd/server.c:435:22: note: 'name' was declared here
     g_autofree char *name;
                      ^~~~
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: nbd/server.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 662, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=960f572775044fbdbd74adcaff59712e', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-07kxi3y_/src/docker-src.2019-11-13-21.57.58.24536:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=960f572775044fbdbd74adcaff59712e
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-07kxi3y_/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    2m33.836s
user    0m7.592s


The full log is available at
http://patchew.org/logs/20191114024635.11363-1-eblake@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v3 for-4.2 0/4] Better NBD string length handling

[Eric Blake]

On 11/13/19 9:00 PM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20191114024635.11363-1-eblake@redhat.com/
> 

> 
>                   from /tmp/qemu-test/src/include/qemu/osdep.h:140,
>                   from /tmp/qemu-test/src/nbd/server.c:20:
> /tmp/qemu-test/src/nbd/server.c: In function 'nbd_negotiate_handle_export_name':
> /usr/x86_64-w64-mingw32/sys-root/mingw/include/glib-2.0/glib/glib-autocleanups.h:28:3: error: 'name' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>     g_free (*pp);
>     ^~~~~~~~~~~~
> /tmp/qemu-test/src/nbd/server.c:435:22: note: 'name' was declared here
>       g_autofree char *name;
>                        ^~~~

Ha - I posted the fix to that one minute before patchew flagged it. 
Still, I'm not sure why my gcc didn't flag this locally, while the mingw 
builder did.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v3 1/4] nbd/server: Prefer heap over stack for parsing client names

[Maxim Levitsky]

On Wed, 2019-11-13 at 20:46 -0600, Eric Blake wrote:
> As long as we limit NBD names to 256 bytes (the bare minimum permitted
> by the standard), stack-allocation works for parsing a name received
> from the client.  But as mentioned in a comment, we eventually want to
> permit up to the 4k maximum of the NBD standard, which is too large
> for stack allocation; so switch everything in the server to use heap
> allocation.  For now, there is no change in actually supported name
> length.

I am just curios, why is this so?
I know that kernel uses 8K stacks due to historical limitation
of 1:1 physical memory mapping which creates fragmentation,
but in the userspace stacks shouldn't really be limited and grow on demand.
Some gcc security option limits this?

> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/nbd.h | 10 +++++-----
>  nbd/server.c        | 25 +++++++++++++++----------
>  2 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 316fd705a9e4..c306423dc85c 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -226,11 +226,11 @@ enum {
>  /* Maximum size of a single READ/WRITE data buffer */
>  #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)
> 
> -/* Maximum size of an export name. The NBD spec requires 256 and
> - * suggests that servers support up to 4096, but we stick to only the
> - * required size so that we can stack-allocate the names, and because
> - * going larger would require an audit of more code to make sure we
> - * aren't overflowing some other buffer. */
> +/*
> + * Maximum size of an export name. The NBD spec requires a minimum of
> + * 256 and recommends that servers support up to 4096; all users use
> + * malloc so we can bump this constant without worry.
> + */
>  #define NBD_MAX_NAME_SIZE 256
> 
>  /* Two types of reply structures */
> diff --git a/nbd/server.c b/nbd/server.c
> index d8d1e6245532..c63b76b22735 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -324,18 +324,20 @@ static int nbd_opt_skip(NBDClient *client, size_t size, Error **errp)
>   *   uint32_t len     (<= NBD_MAX_NAME_SIZE)
>   *   len bytes string (not 0-terminated)
>   *
> - * @name should be enough to store NBD_MAX_NAME_SIZE+1.
> + * On success, @name will be allocated.
>   * If @length is non-null, it will be set to the actual string length.
>   *
>   * Return -errno on I/O error, 0 if option was completely handled by
>   * sending a reply about inconsistent lengths, or 1 on success.
>   */
> -static int nbd_opt_read_name(NBDClient *client, char *name, uint32_t *length,
> +static int nbd_opt_read_name(NBDClient *client, char **name, uint32_t *length,
>                               Error **errp)
>  {
>      int ret;
>      uint32_t len;
> +    g_autofree char *local_name = NULL;
> 
> +    *name = NULL;
>      ret = nbd_opt_read(client, &len, sizeof(len), errp);
>      if (ret <= 0) {
>          return ret;
> @@ -347,15 +349,17 @@ static int nbd_opt_read_name(NBDClient *client, char *name, uint32_t *length,
>                                 "Invalid name length: %" PRIu32, len);
>      }
> 
> -    ret = nbd_opt_read(client, name, len, errp);
> +    local_name = g_malloc(len + 1);
> +    ret = nbd_opt_read(client, local_name, len, errp);
>      if (ret <= 0) {
>          return ret;
>      }
> -    name[len] = '\0';
> +    local_name[len] = '\0';
> 
>      if (length) {
>          *length = len;
>      }
> +    *name = g_steal_pointer(&local_name);
> 
>      return 1;
>  }
> @@ -427,7 +431,7 @@ static void nbd_check_meta_export(NBDClient *client)
>  static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
>                                              Error **errp)
>  {
> -    char name[NBD_MAX_NAME_SIZE + 1];
> +    g_autofree char *name;

That is what patchew complained about I think.

Isn't it wonderful how g_autofree fixes one issue
and introduces another. I mean 'name' isn't really
used here prior to allocation according to plain C,
but due to g_autofree, it can be now on any error
path. Nothing against g_autofree though, just noting this.

>      char buf[NBD_REPLY_EXPORT_NAME_SIZE] = "";
>      size_t len;
>      int ret;
> @@ -441,10 +445,11 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
>          [10 .. 133]   reserved     (0) [unless no_zeroes]
>       */
>      trace_nbd_negotiate_handle_export_name();
> -    if (client->optlen >= sizeof(name)) {
> +    if (client->optlen > NBD_MAX_NAME_SIZE) {
>          error_setg(errp, "Bad length received");
>          return -EINVAL;
>      }
> +    name = g_malloc(client->optlen + 1);
>      if (nbd_read(client->ioc, name, client->optlen, "export name", errp) < 0) {
>          return -EIO;
>      }
> @@ -533,7 +538,7 @@ static int nbd_reject_length(NBDClient *client, bool fatal, Error **errp)
>  static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
>  {
>      int rc;
> -    char name[NBD_MAX_NAME_SIZE + 1];
> +    g_autofree char *name = NULL;
>      NBDExport *exp;
>      uint16_t requests;
>      uint16_t request;
> @@ -551,7 +556,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
>          2 bytes: N, number of requests (can be 0)
>          N * 2 bytes: N requests
>      */
> -    rc = nbd_opt_read_name(client, name, &namelen, errp);
> +    rc = nbd_opt_read_name(client, &name, &namelen, errp);
>      if (rc <= 0) {
>          return rc;
>      }
> @@ -957,7 +962,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
>                                        NBDExportMetaContexts *meta, Error **errp)
>  {
>      int ret;
> -    char export_name[NBD_MAX_NAME_SIZE + 1];
> +    g_autofree char *export_name = NULL;
>      NBDExportMetaContexts local_meta;
>      uint32_t nb_queries;
>      int i;
> @@ -976,7 +981,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
> 
>      memset(meta, 0, sizeof(*meta));
> 
> -    ret = nbd_opt_read_name(client, export_name, NULL, errp);
> +    ret = nbd_opt_read_name(client, &export_name, NULL, errp);
>      if (ret <= 0) {
>          return ret;
>      }

Looks correct, but I might have missed something.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [PATCH v3 2/4] bitmap: Enforce maximum bitmap name length

[Maxim Levitsky]

On Wed, 2019-11-13 at 20:46 -0600, Eric Blake wrote:
> We document that for qcow2 persistent bitmaps, the name cannot exceed
> 1023 bytes.  It is inconsistent if transient bitmaps do not have to
> abide by the same limit, and it is unlikely that any existing client
> even cares about using bitmap names this long.  It's time to codify
> that ALL bitmaps managed by qemu (whether persistent in qcow2 or not)
> have a documented maximum length.

Strange a bit that 1023 was choosen, I guess implementation
uses a 1024 zero terminated string for storing the names
in memory.

> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qapi/block-core.json         |  2 +-
>  include/block/dirty-bitmap.h |  2 ++
>  block/dirty-bitmap.c         | 12 +++++++++---
>  block/qcow2-bitmap.c         |  2 ++
>  4 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index aa97ee264112..0cf68fea1450 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2042,7 +2042,7 @@
>  #
>  # @node: name of device/node which the bitmap is tracking
>  #
> -# @name: name of the dirty bitmap
> +# @name: name of the dirty bitmap (must be less than 1024 bytes)
>  #
>  # @granularity: the bitmap granularity, default is 64k for
>  #               block-dirty-bitmap-add
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 958e7474fb51..e2b20ecab9a3 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -14,6 +14,8 @@ typedef enum BitmapCheckFlags {
>                               BDRV_BITMAP_INCONSISTENT)
>  #define BDRV_BITMAP_ALLOW_RO (BDRV_BITMAP_BUSY | BDRV_BITMAP_INCONSISTENT)
> 
> +#define BDRV_BITMAP_MAX_NAME_SIZE 1023
> +
>  BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>                                            uint32_t granularity,
>                                            const char *name,
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 4bbb251b2c9c..7039e8252009 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -104,9 +104,15 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> 
>      assert(is_power_of_2(granularity) && granularity >= BDRV_SECTOR_SIZE);
> 
> -    if (name && bdrv_find_dirty_bitmap(bs, name)) {
> -        error_setg(errp, "Bitmap already exists: %s", name);
> -        return NULL;
> +    if (name) {
> +        if (bdrv_find_dirty_bitmap(bs, name)) {
> +            error_setg(errp, "Bitmap already exists: %s", name);
> +            return NULL;
> +        }
> +        if (strlen(name) > BDRV_BITMAP_MAX_NAME_SIZE) {
> +            error_setg(errp, "Bitmap name too long: %s", name);
> +            return NULL;
> +        }
>      }
>      bitmap_size = bdrv_getlength(bs);
>      if (bitmap_size < 0) {
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index ef9ef628a0d0..809bbc5d20c8 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -42,6 +42,8 @@
>  #define BME_MIN_GRANULARITY_BITS 9
>  #define BME_MAX_NAME_SIZE 1023
> 
> +QEMU_BUILD_BUG_ON(BME_MAX_NAME_SIZE != BDRV_BITMAP_MAX_NAME_SIZE);
> +
>  #if BME_MAX_TABLE_SIZE * 8ULL > INT_MAX
>  #error In the code bitmap table physical size assumed to fit into int
>  #endif


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [PATCH v3 3/4] nbd: Don't send oversize strings

[Maxim Levitsky]

On Wed, 2019-11-13 at 20:46 -0600, Eric Blake wrote:
> Qemu as server currently won't accept export names larger than 256
> bytes, nor create dirty bitmap names longer than 1023 bytes, so most
> uses of qemu as client or server have no reason to get anywhere near
> the NBD spec maximum of a 4k limit per string.
> 
> However, we weren't actually enforcing things, ignoring when the
> remote side violates the protocol on input, and also having several
> code paths where we send oversize strings on output (for example,
> qemu-nbd --description could easily send more than 4k).  Tighten
> things up as follows:
> 
> client:
> - Perform bounds check on export name and dirty bitmap request prior
>   to handing it to server
> - Validate that copied server replies are not too long (ignoring
>   NBD_INFO_* replies that are not copied is not too bad)
> server:
> - Perform bounds check on export name and description prior to
>   advertising it to client
> - Reject client name or metadata query that is too long
> - Adjust things to allow full 4k name limit rather than previous
>   256 byte limit
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/nbd.h |  8 ++++----
>  block/nbd.c         | 10 ++++++++++
>  blockdev-nbd.c      |  5 +++++
>  nbd/client.c        | 18 +++++++++++++++---
>  nbd/server.c        | 20 +++++++++++++++-----
>  qemu-nbd.c          |  9 +++++++++
>  6 files changed, 58 insertions(+), 12 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index c306423dc85c..7f46932d80f1 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -227,11 +227,11 @@ enum {
>  #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)
> 
>  /*
> - * Maximum size of an export name. The NBD spec requires a minimum of
> - * 256 and recommends that servers support up to 4096; all users use
> - * malloc so we can bump this constant without worry.
> + * Maximum size of a protocol string (export name, meta context name,
> + * etc.).  Use malloc rather than stack allocation for storage of a
> + * string.
>   */
> -#define NBD_MAX_NAME_SIZE 256
> +#define NBD_MAX_STRING_SIZE 4096
> 
>  /* Two types of reply structures */
>  #define NBD_SIMPLE_REPLY_MAGIC      0x67446698
> diff --git a/block/nbd.c b/block/nbd.c
> index 123976171cf4..5f18f78a9471 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1832,6 +1832,10 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options,
>      }
> 
>      s->export = g_strdup(qemu_opt_get(opts, "export"));
> +    if (s->export && strlen(s->export) > NBD_MAX_STRING_SIZE) {
> +        error_setg(errp, "export name too long to send to server");
> +        goto error;
> +    }
> 
>      s->tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds"));
>      if (s->tlscredsid) {
> @@ -1849,6 +1853,11 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options,
>      }
> 
>      s->x_dirty_bitmap = g_strdup(qemu_opt_get(opts, "x-dirty-bitmap"));
> +    if (s->x_dirty_bitmap && strlen(s->x_dirty_bitmap) > NBD_MAX_STRING_SIZE) {
> +        error_setg(errp, "x-dirty-bitmap query too long to send to server");
> +        goto error;
> +    }
> +
>      s->reconnect_delay = qemu_opt_get_number(opts, "reconnect-delay", 0);
> 
>      ret = 0;
> @@ -1859,6 +1868,7 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options,
>          qapi_free_SocketAddress(s->saddr);
>          g_free(s->export);
>          g_free(s->tlscredsid);
> +        g_free(s->x_dirty_bitmap);
>      }
>      qemu_opts_del(opts);
>      return ret;
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 6a8b206e1d74..8c20baa4a4b9 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -162,6 +162,11 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
>          name = device;
>      }
> 
> +    if (strlen(name) > NBD_MAX_STRING_SIZE) {
> +        error_setg(errp, "export name '%s' too long", name);
> +        return;
> +    }
> +
>      if (nbd_export_find(name)) {
>          error_setg(errp, "NBD server already has export named '%s'", name);
>          return;
> diff --git a/nbd/client.c b/nbd/client.c
> index f6733962b49b..ba173108baab 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -289,8 +289,8 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,
>          return -1;
>      }
>      len -= sizeof(namelen);
> -    if (len < namelen) {
> -        error_setg(errp, "incorrect option name length");
> +    if (len < namelen || namelen > NBD_MAX_STRING_SIZE) {
> +        error_setg(errp, "incorrect name length in server's list response");
>          nbd_send_opt_abort(ioc);
>          return -1;
>      }
> @@ -303,6 +303,12 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,
>      local_name[namelen] = '\0';
>      len -= namelen;
>      if (len) {
> +        if (len > NBD_MAX_STRING_SIZE) {
> +            error_setg(errp, "incorrect description length in server's "
> +                       "list response");
> +            nbd_send_opt_abort(ioc);
> +            return -1;
> +        }
>          local_desc = g_malloc(len + 1);
>          if (nbd_read(ioc, local_desc, len, "export description", errp) < 0) {
>              nbd_send_opt_abort(ioc);
> @@ -479,6 +485,10 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
>              break;
> 
>          default:
> +            /*
> +             * Not worth the bother to check if NBD_INFO_NAME or
> +             * NBD_INFO_DESCRIPTION exceed NBD_MAX_STRING_SIZE.
> +             */
>              trace_nbd_opt_info_unknown(type, nbd_info_lookup(type));
>              if (nbd_drop(ioc, len, errp) < 0) {
>                  error_prepend(errp, "Failed to read info payload: ");
> @@ -645,9 +655,11 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t opt,
>      char *p;
> 
>      data_len = sizeof(export_len) + export_len + sizeof(queries);
> +    assert(export_len <= NBD_MAX_STRING_SIZE);
>      if (query) {
>          query_len = strlen(query);
>          data_len += sizeof(query_len) + query_len;
> +        assert(query_len <= NBD_MAX_STRING_SIZE);
>      } else {
>          assert(opt == NBD_OPT_LIST_META_CONTEXT);
>      }
> @@ -1009,7 +1021,7 @@ int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc,
>      bool zeroes;
>      bool base_allocation = info->base_allocation;
> 
> -    assert(info->name);
> +    assert(info->name && strlen(info->name) <= NBD_MAX_STRING_SIZE);
>      trace_nbd_receive_negotiate_name(info->name);
> 
>      result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname, outioc,
> diff --git a/nbd/server.c b/nbd/server.c
> index c63b76b22735..d28123c562be 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -321,7 +321,7 @@ static int nbd_opt_skip(NBDClient *client, size_t size, Error **errp)
>  /* nbd_opt_read_name
>   *
>   * Read a string with the format:
> - *   uint32_t len     (<= NBD_MAX_NAME_SIZE)
> + *   uint32_t len     (<= NBD_MAX_STRING_SIZE)
>   *   len bytes string (not 0-terminated)
>   *
>   * On success, @name will be allocated.
> @@ -344,7 +344,7 @@ static int nbd_opt_read_name(NBDClient *client, char **name, uint32_t *length,
>      }
>      len = cpu_to_be32(len);
> 
> -    if (len > NBD_MAX_NAME_SIZE) {
> +    if (len > NBD_MAX_STRING_SIZE) {
>          return nbd_opt_invalid(client, errp,
>                                 "Invalid name length: %" PRIu32, len);
>      }
> @@ -379,6 +379,7 @@ static int nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp,
>      trace_nbd_negotiate_send_rep_list(name, desc);
>      name_len = strlen(name);
>      desc_len = strlen(desc);
> +    assert(name_len <= NBD_MAX_STRING_SIZE && desc_len <= NBD_MAX_STRING_SIZE);
>      len = name_len + desc_len + sizeof(len);
>      ret = nbd_negotiate_send_rep_len(client, NBD_REP_SERVER, len, errp);
>      if (ret < 0) {
> @@ -445,7 +446,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
>          [10 .. 133]   reserved     (0) [unless no_zeroes]
>       */
>      trace_nbd_negotiate_handle_export_name();
> -    if (client->optlen > NBD_MAX_NAME_SIZE) {
> +    if (client->optlen > NBD_MAX_STRING_SIZE) {
>          error_setg(errp, "Bad length received");
>          return -EINVAL;
>      }
> @@ -613,6 +614,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
>      if (exp->description) {
>          size_t len = strlen(exp->description);
> 
> +        assert(len <= NBD_MAX_STRING_SIZE);
>          rc = nbd_negotiate_send_info(client, NBD_INFO_DESCRIPTION,
>                                       len, exp->description, errp);
>          if (rc < 0) {
> @@ -757,6 +759,7 @@ static int nbd_negotiate_send_meta_context(NBDClient *client,
>          {.iov_base = (void *)context, .iov_len = strlen(context)}
>      };
> 
> +    assert(iov[1].iov_len <= NBD_MAX_STRING_SIZE);
>      if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
>          context_id = 0;
>      }
> @@ -905,7 +908,7 @@ static int nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta,
>   * Parse namespace name and call corresponding function to parse body of the
>   * query.
>   *
> - * The only supported namespace now is 'base'.
> + * The only supported namespaces are 'base' and 'qemu'.
>   *
>   * The function aims not wasting time and memory to read long unknown namespace
>   * names.
> @@ -931,6 +934,10 @@ static int nbd_negotiate_meta_query(NBDClient *client,
>      }
>      len = cpu_to_be32(len);
> 
> +    if (len > NBD_MAX_STRING_SIZE) {
> +        trace_nbd_negotiate_meta_query_skip("length too long");
> +        return nbd_opt_skip(client, len, errp);
> +    }
>      if (len < ns_len) {
>          trace_nbd_negotiate_meta_query_skip("length too short");
>          return nbd_opt_skip(client, len, errp);
> @@ -1492,7 +1499,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>       * access since the export could be available before migration handover.
>       * ctx was acquired in the caller.
>       */
> -    assert(name);
> +    assert(name && strlen(name) <= NBD_MAX_STRING_SIZE);
>      ctx = bdrv_get_aio_context(bs);
>      bdrv_invalidate_cache(bs, NULL);
> 
> @@ -1518,6 +1525,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>      assert(dev_offset <= INT64_MAX);
>      exp->dev_offset = dev_offset;
>      exp->name = g_strdup(name);
> +    assert(!desc || strlen(desc) <= NBD_MAX_STRING_SIZE);
>      exp->description = g_strdup(desc);
>      exp->nbdflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_FLUSH |
>                       NBD_FLAG_SEND_FUA | NBD_FLAG_SEND_CACHE);
> @@ -1564,8 +1572,10 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
> 
>          bdrv_dirty_bitmap_set_busy(bm, true);
>          exp->export_bitmap = bm;
> +        assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
>          exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
>                                                       bitmap);
> +        assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
>      }
> 
>      exp->close = close;
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index caacf0ed7379..108a51f7eb01 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -833,9 +833,18 @@ int main(int argc, char **argv)
>              break;
>          case 'x':
>              export_name = optarg;
> +            if (strlen(export_name) > NBD_MAX_STRING_SIZE) {
> +                error_report("export name '%s' too long", export_name);
> +                exit(EXIT_FAILURE);
> +            }
>              break;
>          case 'D':
>              export_description = optarg;
> +            if (strlen(export_description) > NBD_MAX_STRING_SIZE) {
> +                error_report("export description '%s' too long",
> +                             export_description);
> +                exit(EXIT_FAILURE);
> +            }
>              break;
>          case 'v':
>              verbose = 1;

Maybe I would split that patch into server side checks,
then client side checks, and then patch that adds bunch
of asserts, which are true after client side checks are added.


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [PATCH v3 1/4] nbd/server: Prefer heap over stack for parsing client names

[Eric Blake]

On 11/14/19 4:04 AM, Maxim Levitsky wrote:
> On Wed, 2019-11-13 at 20:46 -0600, Eric Blake wrote:
>> As long as we limit NBD names to 256 bytes (the bare minimum permitted
>> by the standard), stack-allocation works for parsing a name received
>> from the client.  But as mentioned in a comment, we eventually want to
>> permit up to the 4k maximum of the NBD standard, which is too large
>> for stack allocation; so switch everything in the server to use heap
>> allocation.  For now, there is no change in actually supported name
>> length.
> 
> I am just curios, why is this so?
> I know that kernel uses 8K stacks due to historical limitation
> of 1:1 physical memory mapping which creates fragmentation,
> but in the userspace stacks shouldn't really be limited and grow on demand.

Actually, 4k rather than 8k stack overflow guard pages are typical on 
some OS.  The problem with stack-allocating anything larger than the 
guard page size is that you can end up overshooting the guard page, and 
then the OS is unable to catch stack overflow in the normal manner of 
sending SIGSEGV.  Also, when using coroutines, it is very common to have 
limited stack size in the first place, where large stack allocations can 
run into issues.  So in general, it's a good rule of thumb to never 
stack-allocate something if it can be larger than 4k.

> Some gcc security option limits this?

Not by default, but you can compile with -Wframe-larger-than=4096 (or 
even smaller) to catch instances where stack allocation is likely to run 
into trouble.


>> @@ -427,7 +431,7 @@ static void nbd_check_meta_export(NBDClient *client)
>>   static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
>>                                               Error **errp)
>>   {
>> -    char name[NBD_MAX_NAME_SIZE + 1];
>> +    g_autofree char *name;
> 
> That is what patchew complained about I think.

Yes, and I've already fixed the missing initializer.

> 
> Isn't it wonderful how g_autofree fixes one issue
> and introduces another. I mean 'name' isn't really
> used here prior to allocation according to plain C,
> but due to g_autofree, it can be now on any error
> path. Nothing against g_autofree though, just noting this.

Yes, and our documentation for g_auto* reminds that all such variables 
with automatic cleanup must have an initializer or be set prior to any 
exit path.  I think I see why I didn't catch it beforehand - I'm 
compiling with --enable-debug, which passes CFLAGS=-g, while the 
compiler warning occurs when -O2 is in effect; but it is rather annoying 
that gcc doesn't catch the bug when not optimizing.

> 
> Looks correct, but I might have missed something.
> 
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> 

Thanks, and assuming that's with my initializer fix squashed in.

> Best regards,
> 	Maxim Levitsky
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v3 1/4] nbd/server: Prefer heap over stack for parsing client names

[Vladimir Sementsov-Ogievskiy]

14.11.2019 5:46, Eric Blake wrote:
> As long as we limit NBD names to 256 bytes (the bare minimum permitted
> by the standard), stack-allocation works for parsing a name received
> from the client.  But as mentioned in a comment, we eventually want to
> permit up to the 4k maximum of the NBD standard, which is too large
> for stack allocation; so switch everything in the server to use heap
> allocation.  For now, there is no change in actually supported name
> length.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

with mentioned fix:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

PS: Great, how using g_autofree simplifies reviewing! I don't need to check
that something is leaked at some return point.


-- 
Best regards,
Vladimir

Re: [PATCH v3 2/4] bitmap: Enforce maximum bitmap name length

[Vladimir Sementsov-Ogievskiy]

14.11.2019 5:46, Eric Blake wrote:
> We document that for qcow2 persistent bitmaps, the name cannot exceed
> 1023 bytes.  It is inconsistent if transient bitmaps do not have to
> abide by the same limit, and it is unlikely that any existing client
> even cares about using bitmap names this long.  It's time to codify
> that ALL bitmaps managed by qemu (whether persistent in qcow2 or not)
> have a documented maximum length.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>



-- 
Best regards,
Vladimir

Re: [PATCH v3 1/4] nbd/server: Prefer heap over stack for parsing client names

[Maxim Levitsky]

On Thu, 2019-11-14 at 07:33 -0600, Eric Blake wrote:
> On 11/14/19 4:04 AM, Maxim Levitsky wrote:
> > On Wed, 2019-11-13 at 20:46 -0600, Eric Blake wrote:
> > > As long as we limit NBD names to 256 bytes (the bare minimum permitted
> > > by the standard), stack-allocation works for parsing a name received
> > > from the client.  But as mentioned in a comment, we eventually want to
> > > permit up to the 4k maximum of the NBD standard, which is too large
> > > for stack allocation; so switch everything in the server to use heap
> > > allocation.  For now, there is no change in actually supported name
> > > length.
> > 
> > I am just curios, why is this so?
> > I know that kernel uses 8K stacks due to historical limitation
> > of 1:1 physical memory mapping which creates fragmentation,
> > but in the userspace stacks shouldn't really be limited and grow on demand.
> 
> Actually, 4k rather than 8k stack overflow guard pages are typical on 
> some OS.  
I was talking about the kernel stacks. These are limited to 8K with
no growing and it is a pain point there. Userspace stacks on the
other hand should be able to grow to an reasonable size.


> The problem with stack-allocating anything larger than the 
> guard page size is that you can end up overshooting the guard page, and 
> then the OS is unable to catch stack overflow in the normal manner of 
> sending SIGSEGV.  Also, when using coroutines, it is very common to have 
> limited stack size in the first place, where large stack allocations can 
> run into issues.  So in general, it's a good rule of thumb to never 
> stack-allocate something if it can be larger than 4k.

Doh! I know how the guard pages work, but never thought
about them in this way. I guess I don't after all.
Thanks for the explanation!


> 
> > Some gcc security option limits this?
> 
> Not by default, but you can compile with -Wframe-larger-than=4096 (or 
> even smaller) to catch instances where stack allocation is likely to run 
> into trouble.
> 
> 
> > > @@ -427,7 +431,7 @@ static void nbd_check_meta_export(NBDClient *client)
> > >   static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
> > >                                               Error **errp)
> > >   {
> > > -    char name[NBD_MAX_NAME_SIZE + 1];
> > > +    g_autofree char *name;
> > 
> > That is what patchew complained about I think.
> 
> Yes, and I've already fixed the missing initializer.
> 
> > 
> > Isn't it wonderful how g_autofree fixes one issue
> > and introduces another. I mean 'name' isn't really
> > used here prior to allocation according to plain C,
> > but due to g_autofree, it can be now on any error
> > path. Nothing against g_autofree though, just noting this.
> 
> Yes, and our documentation for g_auto* reminds that all such variables 
> with automatic cleanup must have an initializer or be set prior to any 
> exit path.  I think I see why I didn't catch it beforehand - I'm 
> compiling with --enable-debug, which passes CFLAGS=-g, while the 
> compiler warning occurs when -O2 is in effect; but it is rather annoying 
> that gcc doesn't catch the bug when not optimizing.
> 
> > 
> > Looks correct, but I might have missed something.
> > 
> > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> > 
> 
> Thanks, and assuming that's with my initializer fix squashed in.
Of course.

Best regards,
	Maxim Levitsky

Re: [PATCH v3 2/4] bitmap: Enforce maximum bitmap name length

[Vladimir Sementsov-Ogievskiy]

15.11.2019 18:03, Vladimir Sementsov-Ogievskiy wrote:
> 14.11.2019 5:46, Eric Blake wrote:
>> We document that for qcow2 persistent bitmaps, the name cannot exceed
>> 1023 bytes.  It is inconsistent if transient bitmaps do not have to
>> abide by the same limit, and it is unlikely that any existing client
>> even cares about using bitmap names this long.  It's time to codify
>> that ALL bitmaps managed by qemu (whether persistent in qcow2 or not)
>> have a documented maximum length.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> 

One doubt:

Is it good idea to include string larger than 4K into error message
(in next patch too)? I doubt that such message would be
readable, and I think that most possible source of such message is
some kind of memory corruption, so the whole message would be garbage,
which may contain special symbols which may look bad or even break
output.

-- 
Best regards,
Vladimir

Re: [PATCH v3 2/4] bitmap: Enforce maximum bitmap name length

[Eric Blake]

On 11/15/19 9:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> 15.11.2019 18:03, Vladimir Sementsov-Ogievskiy wrote:
>> 14.11.2019 5:46, Eric Blake wrote:
>>> We document that for qcow2 persistent bitmaps, the name cannot exceed
>>> 1023 bytes.  It is inconsistent if transient bitmaps do not have to
>>> abide by the same limit, and it is unlikely that any existing client
>>> even cares about using bitmap names this long.  It's time to codify
>>> that ALL bitmaps managed by qemu (whether persistent in qcow2 or not)
>>> have a documented maximum length.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>>
> 
> One doubt:
> 
> Is it good idea to include string larger than 4K into error message
> (in next patch too)? I doubt that such message would be
> readable, and I think that most possible source of such message is
> some kind of memory corruption, so the whole message would be garbage,
> which may contain special symbols which may look bad or even break
> output.

The string was provided by the user. You are correct that it results in 
a lot of output on stderr, but it is no more garbage than what the user 
provided in the first place. If we wanted, we could truncate (list only 
the first 256 or so bytes and then output "..."), but it's such a corner 
case error that I don't think it's worth the effort to worry about it.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v3 3/4] nbd: Don't send oversize strings

[Vladimir Sementsov-Ogievskiy]

14.11.2019 5:46, Eric Blake wrote:
> Qemu as server currently won't accept export names larger than 256
> bytes, nor create dirty bitmap names longer than 1023 bytes, so most
> uses of qemu as client or server have no reason to get anywhere near
> the NBD spec maximum of a 4k limit per string.
> 
> However, we weren't actually enforcing things, ignoring when the
> remote side violates the protocol on input, and also having several
> code paths where we send oversize strings on output (for example,
> qemu-nbd --description could easily send more than 4k).  Tighten
> things up as follows:
> 
> client:
> - Perform bounds check on export name and dirty bitmap request prior
>    to handing it to server
> - Validate that copied server replies are not too long (ignoring
>    NBD_INFO_* replies that are not copied is not too bad)
> server:
> - Perform bounds check on export name and description prior to
>    advertising it to client
> - Reject client name or metadata query that is too long
> - Adjust things to allow full 4k name limit rather than previous
>    256 byte limit
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Still, same doubt about putting over-sized strings into error messages. If you
decide to drop them, keep my r-b.

======

Not very simple to review, as I need to check that all assertions will not fire.
Hope you don't mind me doing it here)

1.
nbd_send_meta_query checks export and query

    nbd_negotiate_simple_meta_context ^ info->name, info->x_dirty_bitmap/"base:allocation"

       nbd_receive_negotiate ^ info=info
           (info->name proved by assert)

          nbd_client_connect ^ info=s->info                                                        OK
              (s->info.x_dirty_bitmap = g_strdup(s->x_dirty_bitmap);
               s->info.name = g_strdup(s->export ?: "");
               s->export is set only in nbd_process_options() and checked in it
               s->x_dirty_bitmap too)

          nbd_client_thread & info= local info, with name = "" and x_dirty_bitmap = NULL           OK


    nbd_list_meta_contexts ^ info->name, NULL/"qemu:"

       nbd_receive_export_list ^ info=&array[i]
           (array[count - 1].name = name;, name comes from nbd_receive_list, where it is checked)  OK

2.
nbd_receive_negotiate check info->name
     (see 1.)

3.
nbd_negotiate_send_rep_list check exp->name and exp->description

    nbd_negotiate_handle_list                                                                      OK
        (does QTAILQ_FOREACH(exp, &exports, next)
         new exports comes from nbd_export_new() which checks name and desc (by assertions))

4.
nbd_negotiate_handle_info checks exp->description                                                 OK
     (exp comes from nbd_export_find(name), from global exports, so it's OK)


5.
nbd_negotiate_send_meta_context checks  context

     nbd_negotiate_meta_queries ^ context = "base:allocation"/meta->exp->export_bitmap_context     OK
     (meta is filled inside the function, meta->exp = nbd_export_find(export_name),

     and export_bitmap_context is set and asserted in  nbd_export_new

6.
nbd_export_new checks name and desc and bitmap (export_bitmap_context is obvious)
     bitmap is found before assertion (which proves that name is shorter than 1024)

     qmp_nbd_server_add ^ name NULL bitmap                                                         OK
         name is checked

     main in qemu-nbd.c ^ export_name export_description bitmap                                    OK
         both export_name and export_description are checked.

> ---
>   include/block/nbd.h |  8 ++++----
>   block/nbd.c         | 10 ++++++++++
>   blockdev-nbd.c      |  5 +++++
>   nbd/client.c        | 18 +++++++++++++++---
>   nbd/server.c        | 20 +++++++++++++++-----
>   qemu-nbd.c          |  9 +++++++++
>   6 files changed, 58 insertions(+), 12 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index c306423dc85c..7f46932d80f1 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -227,11 +227,11 @@ enum {
>   #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)
> 
>   /*
> - * Maximum size of an export name. The NBD spec requires a minimum of
> - * 256 and recommends that servers support up to 4096; all users use
> - * malloc so we can bump this constant without worry.
> + * Maximum size of a protocol string (export name, meta context name,
> + * etc.).  Use malloc rather than stack allocation for storage of a
> + * string.
>    */
> -#define NBD_MAX_NAME_SIZE 256
> +#define NBD_MAX_STRING_SIZE 4096
> 
>   /* Two types of reply structures */
>   #define NBD_SIMPLE_REPLY_MAGIC      0x67446698
> diff --git a/block/nbd.c b/block/nbd.c
> index 123976171cf4..5f18f78a9471 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1832,6 +1832,10 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options,
>       }
> 
>       s->export = g_strdup(qemu_opt_get(opts, "export"));
> +    if (s->export && strlen(s->export) > NBD_MAX_STRING_SIZE) {
> +        error_setg(errp, "export name too long to send to server");
> +        goto error;
> +    }
> 
>       s->tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds"));
>       if (s->tlscredsid) {
> @@ -1849,6 +1853,11 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options,
>       }
> 
>       s->x_dirty_bitmap = g_strdup(qemu_opt_get(opts, "x-dirty-bitmap"));
> +    if (s->x_dirty_bitmap && strlen(s->x_dirty_bitmap) > NBD_MAX_STRING_SIZE) {
> +        error_setg(errp, "x-dirty-bitmap query too long to send to server");
> +        goto error;
> +    }
> +
>       s->reconnect_delay = qemu_opt_get_number(opts, "reconnect-delay", 0);
> 
>       ret = 0;
> @@ -1859,6 +1868,7 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options,
>           qapi_free_SocketAddress(s->saddr);
>           g_free(s->export);
>           g_free(s->tlscredsid);
> +        g_free(s->x_dirty_bitmap);
>       }
>       qemu_opts_del(opts);
>       return ret;
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 6a8b206e1d74..8c20baa4a4b9 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -162,6 +162,11 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
>           name = device;
>       }
> 
> +    if (strlen(name) > NBD_MAX_STRING_SIZE) {
> +        error_setg(errp, "export name '%s' too long", name);
> +        return;
> +    }
> +
>       if (nbd_export_find(name)) {
>           error_setg(errp, "NBD server already has export named '%s'", name);
>           return;
> diff --git a/nbd/client.c b/nbd/client.c
> index f6733962b49b..ba173108baab 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -289,8 +289,8 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,
>           return -1;
>       }
>       len -= sizeof(namelen);
> -    if (len < namelen) {
> -        error_setg(errp, "incorrect option name length");
> +    if (len < namelen || namelen > NBD_MAX_STRING_SIZE) {
> +        error_setg(errp, "incorrect name length in server's list response");
>           nbd_send_opt_abort(ioc);
>           return -1;
>       }
> @@ -303,6 +303,12 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,
>       local_name[namelen] = '\0';
>       len -= namelen;
>       if (len) {
> +        if (len > NBD_MAX_STRING_SIZE) {
> +            error_setg(errp, "incorrect description length in server's "
> +                       "list response");
> +            nbd_send_opt_abort(ioc);
> +            return -1;
> +        }
>           local_desc = g_malloc(len + 1);
>           if (nbd_read(ioc, local_desc, len, "export description", errp) < 0) {
>               nbd_send_opt_abort(ioc);
> @@ -479,6 +485,10 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
>               break;
> 
>           default:
> +            /*
> +             * Not worth the bother to check if NBD_INFO_NAME or
> +             * NBD_INFO_DESCRIPTION exceed NBD_MAX_STRING_SIZE.
> +             */
>               trace_nbd_opt_info_unknown(type, nbd_info_lookup(type));
>               if (nbd_drop(ioc, len, errp) < 0) {
>                   error_prepend(errp, "Failed to read info payload: ");
> @@ -645,9 +655,11 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t opt,
>       char *p;
> 
>       data_len = sizeof(export_len) + export_len + sizeof(queries);
> +    assert(export_len <= NBD_MAX_STRING_SIZE);
>       if (query) {
>           query_len = strlen(query);
>           data_len += sizeof(query_len) + query_len;
> +        assert(query_len <= NBD_MAX_STRING_SIZE);
>       } else {
>           assert(opt == NBD_OPT_LIST_META_CONTEXT);
>       }
> @@ -1009,7 +1021,7 @@ int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc,
>       bool zeroes;
>       bool base_allocation = info->base_allocation;
> 
> -    assert(info->name);
> +    assert(info->name && strlen(info->name) <= NBD_MAX_STRING_SIZE);
>       trace_nbd_receive_negotiate_name(info->name);
> 
>       result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname, outioc,
> diff --git a/nbd/server.c b/nbd/server.c
> index c63b76b22735..d28123c562be 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -321,7 +321,7 @@ static int nbd_opt_skip(NBDClient *client, size_t size, Error **errp)
>   /* nbd_opt_read_name
>    *
>    * Read a string with the format:
> - *   uint32_t len     (<= NBD_MAX_NAME_SIZE)
> + *   uint32_t len     (<= NBD_MAX_STRING_SIZE)
>    *   len bytes string (not 0-terminated)
>    *
>    * On success, @name will be allocated.
> @@ -344,7 +344,7 @@ static int nbd_opt_read_name(NBDClient *client, char **name, uint32_t *length,
>       }
>       len = cpu_to_be32(len);
> 
> -    if (len > NBD_MAX_NAME_SIZE) {
> +    if (len > NBD_MAX_STRING_SIZE) {
>           return nbd_opt_invalid(client, errp,
>                                  "Invalid name length: %" PRIu32, len);
>       }
> @@ -379,6 +379,7 @@ static int nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp,
>       trace_nbd_negotiate_send_rep_list(name, desc);
>       name_len = strlen(name);
>       desc_len = strlen(desc);
> +    assert(name_len <= NBD_MAX_STRING_SIZE && desc_len <= NBD_MAX_STRING_SIZE);
>       len = name_len + desc_len + sizeof(len);
>       ret = nbd_negotiate_send_rep_len(client, NBD_REP_SERVER, len, errp);
>       if (ret < 0) {
> @@ -445,7 +446,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
>           [10 .. 133]   reserved     (0) [unless no_zeroes]
>        */
>       trace_nbd_negotiate_handle_export_name();
> -    if (client->optlen > NBD_MAX_NAME_SIZE) {
> +    if (client->optlen > NBD_MAX_STRING_SIZE) {
>           error_setg(errp, "Bad length received");
>           return -EINVAL;
>       }
> @@ -613,6 +614,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
>       if (exp->description) {
>           size_t len = strlen(exp->description);
> 
> +        assert(len <= NBD_MAX_STRING_SIZE);
>           rc = nbd_negotiate_send_info(client, NBD_INFO_DESCRIPTION,
>                                        len, exp->description, errp);
>           if (rc < 0) {
> @@ -757,6 +759,7 @@ static int nbd_negotiate_send_meta_context(NBDClient *client,
>           {.iov_base = (void *)context, .iov_len = strlen(context)}
>       };
> 
> +    assert(iov[1].iov_len <= NBD_MAX_STRING_SIZE);
>       if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
>           context_id = 0;
>       }
> @@ -905,7 +908,7 @@ static int nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta,
>    * Parse namespace name and call corresponding function to parse body of the
>    * query.
>    *
> - * The only supported namespace now is 'base'.
> + * The only supported namespaces are 'base' and 'qemu'.
>    *
>    * The function aims not wasting time and memory to read long unknown namespace
>    * names.
> @@ -931,6 +934,10 @@ static int nbd_negotiate_meta_query(NBDClient *client,
>       }
>       len = cpu_to_be32(len);
> 
> +    if (len > NBD_MAX_STRING_SIZE) {
> +        trace_nbd_negotiate_meta_query_skip("length too long");
> +        return nbd_opt_skip(client, len, errp);
> +    }
>       if (len < ns_len) {
>           trace_nbd_negotiate_meta_query_skip("length too short");
>           return nbd_opt_skip(client, len, errp);
> @@ -1492,7 +1499,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>        * access since the export could be available before migration handover.
>        * ctx was acquired in the caller.
>        */
> -    assert(name);
> +    assert(name && strlen(name) <= NBD_MAX_STRING_SIZE);
>       ctx = bdrv_get_aio_context(bs);
>       bdrv_invalidate_cache(bs, NULL);
> 
> @@ -1518,6 +1525,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>       assert(dev_offset <= INT64_MAX);
>       exp->dev_offset = dev_offset;
>       exp->name = g_strdup(name);
> +    assert(!desc || strlen(desc) <= NBD_MAX_STRING_SIZE);
>       exp->description = g_strdup(desc);
>       exp->nbdflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_FLUSH |
>                        NBD_FLAG_SEND_FUA | NBD_FLAG_SEND_CACHE);
> @@ -1564,8 +1572,10 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
> 
>           bdrv_dirty_bitmap_set_busy(bm, true);
>           exp->export_bitmap = bm;
> +        assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE);
>           exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
>                                                        bitmap);
> +        assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE);
>       }
> 
>       exp->close = close;
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index caacf0ed7379..108a51f7eb01 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -833,9 +833,18 @@ int main(int argc, char **argv)
>               break;
>           case 'x':
>               export_name = optarg;
> +            if (strlen(export_name) > NBD_MAX_STRING_SIZE) {
> +                error_report("export name '%s' too long", export_name);
> +                exit(EXIT_FAILURE);
> +            }
>               break;
>           case 'D':
>               export_description = optarg;
> +            if (strlen(export_description) > NBD_MAX_STRING_SIZE) {
> +                error_report("export description '%s' too long",
> +                             export_description);
> +                exit(EXIT_FAILURE);
> +            }
>               break;
>           case 'v':
>               verbose = 1;
> 


-- 
Best regards,
Vladimir

Re: [PATCH v3 2/4] bitmap: Enforce maximum bitmap name length

[Vladimir Sementsov-Ogievskiy]

15.11.2019 19:33, Eric Blake wrote:
> On 11/15/19 9:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 15.11.2019 18:03, Vladimir Sementsov-Ogievskiy wrote:
>>> 14.11.2019 5:46, Eric Blake wrote:
>>>> We document that for qcow2 persistent bitmaps, the name cannot exceed
>>>> 1023 bytes.  It is inconsistent if transient bitmaps do not have to
>>>> abide by the same limit, and it is unlikely that any existing client
>>>> even cares about using bitmap names this long.  It's time to codify
>>>> that ALL bitmaps managed by qemu (whether persistent in qcow2 or not)
>>>> have a documented maximum length.
>>>>
>>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>>
>>
>> One doubt:
>>
>> Is it good idea to include string larger than 4K into error message
>> (in next patch too)? I doubt that such message would be
>> readable, and I think that most possible source of such message is
>> some kind of memory corruption, so the whole message would be garbage,
>> which may contain special symbols which may look bad or even break
>> output.
> 
> The string was provided by the user. You are correct that it results in a lot of output on stderr, but it is no more garbage than what the user provided in the first place. If we wanted, we could truncate (list only the first 256 or so bytes and then output "..."), but it's such a corner case error that I don't think it's worth the effort to worry about it.
> 

OK

-- 
Best regards,
Vladimir

Re: [PATCH v3 3/4] nbd: Don't send oversize strings

[Eric Blake]

On 11/15/19 11:08 AM, Vladimir Sementsov-Ogievskiy wrote:
> 14.11.2019 5:46, Eric Blake wrote:
>> Qemu as server currently won't accept export names larger than 256
>> bytes, nor create dirty bitmap names longer than 1023 bytes, so most
>> uses of qemu as client or server have no reason to get anywhere near
>> the NBD spec maximum of a 4k limit per string.
>>
>> However, we weren't actually enforcing things, ignoring when the
>> remote side violates the protocol on input, and also having several
>> code paths where we send oversize strings on output (for example,
>> qemu-nbd --description could easily send more than 4k).  Tighten
>> things up as follows:
>>
>> client:
>> - Perform bounds check on export name and dirty bitmap request prior
>>     to handing it to server
>> - Validate that copied server replies are not too long (ignoring
>>     NBD_INFO_* replies that are not copied is not too bad)
>> server:
>> - Perform bounds check on export name and description prior to
>>     advertising it to client
>> - Reject client name or metadata query that is too long
>> - Adjust things to allow full 4k name limit rather than previous
>>     256 byte limit
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks; I'm queuing 1-3 in my NBD tree for -rc2.

> 
> Still, same doubt about putting over-sized strings into error messages. If you
> decide to drop them, keep my r-b.
> 
> ======
> 
> Not very simple to review, as I need to check that all assertions will not fire.
> Hope you don't mind me doing it here)

Not at all; your thorough review is appreciated.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org