On Fri, 2019-09-27 at 23:13 -0500, Eric Blake wrote:
> Qemu as server currently won't accept export names larger than 256
> bytes, so most uses of qemu as client have no reason to get anywhere
> near the NBD spec maximum of a 4k limit per string. However, we
> didn't actually have any code that prevented the client from violating
> the protocol, which, while useful for testing corner-case server
> reactions, is probably not ideal.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
> include/block/nbd.h | 1 +
> nbd/client.c | 8 ++++++++
> 2 files changed, 9 insertions(+)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 316fd705a9e4..fcabdf0f37c3 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -232,6 +232,7 @@ enum {
> * going larger would require an audit of more code to make sure we
> * aren't overflowing some other buffer. */
> #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/nbd/client.c b/nbd/client.c
> index f6733962b49b..3f21722dd914 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -648,6 +648,10 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t
> opt,
> if (query) {
> query_len = strlen(query);
> data_len += sizeof(query_len) + query_len;
> + if (query_len > NBD_MAX_STRING_SIZE) {
> + error_setg(errp, "x_dirty_bitmap query too long to send to
> server");
Is there a way not to do this here? I don't know nbd well to be honest,
and it looks like this code currently is only called for x_dirty_bitmap but
there could be more cases in the future.
nbd_negotiate_simple_meta_context which seems to be the caller of this, already
mentions
a 'hack' about this :-(
Of course if you think that this is not worth the time, you can leave this as
is.
> + return -1;
> + }
> } else {
> assert(opt == NBD_OPT_LIST_META_CONTEXT);
> }
> @@ -1010,6 +1014,10 @@ int nbd_receive_negotiate(AioContext *aio_context,
> QIOChannel *ioc,
> bool base_allocation = info->base_allocation;
>
> assert(info->name);
> + if (strlen(info->name) > NBD_MAX_STRING_SIZE) {
> + error_setg(errp, "name too long to send to server");
Maybe 'export name'?
> + return -EINVAL;
> + }
> trace_nbd_receive_negotiate_name(info->name);
>
> result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname,
> outioc,
Why not to do the export name check when info->name is set, that is in
nbd_client_connect?
Best regards,
Maxim Levitsky
