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

[Eric Blake]

On 9/29/19 1:49 PM, Maxim Levitsky wrote:
> 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 <address@hidden>
> > ---
> >   include/block/nbd.h | 1 +
> >   nbd/client.c        | 8 ++++++++
> >   2 files changed, 9 insertions(+)

> > +++ 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.

I could make this an assert, and fix the callers to pass in valid 
lengths (callers pass in either "base:allocation" which fits, or a 
user-supplied x_dirty_bitmap, so validating at the point that hack is 
assigned is reasoanble).

> 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'?

Sure.

> > +        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?

I'll spin up a v2.

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