| Message ID | 20191114024635.11363-4-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Better NBD string length handling | expand |
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
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; >
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.
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;
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(-)