Re: [PATCH] nbd: Don't let client send oversize strings

[Maxim Levitsky <mlevitsk@redhat.com>]

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 <eblake@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