Re: [PATCH v2 1/2] nbd: Don't send oversize strings

[Maxim Levitsky <mlevitsk@redhat.com>]

On Tue, 2019-10-15 at 16:16 +0000, Vladimir Sementsov-Ogievskiy wrote:
> 15.10.2019 18:07, Eric Blake wrote:
> > On 10/11/19 2:32 AM, Vladimir Sementsov-Ogievskiy wrote:
> > > 11.10.2019 0:00, 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
> > > > 
> > > > Signed-off-by: Eric Blake <eblake@redhat.com>
> > > > ---
> > > > +++ 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. */
> > > 
> > > This comment says, that we restrict export name to 256...
> > 
> > Yes, because we still stack-allocate the name in places, but 4k is too large for stack allocation.  But we're inconsistent on where we use the smaller 256-limit; the server won't serve an image
> > that large, but doesn't prevent a client from requesting a 4k name export (even though that export will not be present).
> > 
> > 
> > > > +++ 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;
> > > > +    }
> > > 
> > > Hmmm, no, so here we restrict to 4096, but, we will not allow client to request more than
> > > 256. Seems, to correctly update server-part, we should drop NBD_MAX_NAME_SIZE and do the
> > > audit mentioned in the comment above its definition.
> > 
> > Yeah, I guess it's time to just get rid of NBD_MAX_NAME_SIZE, and move away from stack allocations.  Should I do that as a followup to this patch, or spin a v3?
> 
> Hmm. It's OK too.
> 
> With
>   - fixed mem-leak in nbd_process_options
>   - s/x_dirty_bitmap/x-dirty-bitmap in nbd_process_options in error message
>   - following yours new wordings
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> However, this patch introduces possible crash point, asserting on bitmap name below, so it would better
> be fixed before this patch or immediately after it.. Still, it's unlikely to have a bitmap with name
> longer than 4k..
> 
> > 
> > > > +++ 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 list name length");
> > > 
> > > New wording made me go above and read the comment, what functions does. Comment is good, but without
> > > it, it sounds like name of the list for me...
> > 
> > Maybe:
> > 
> > incorrect name length in server's list response
> 
> Yes, this is better, thanks
> 
> > 
> > > 
> > > >            nbd_send_opt_abort(ioc);
> > > >            return -1;
> > > >        }
> > > > @@ -303,6 +303,11 @@ 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 list description length");
> > 
> > and
> > 
> > incorrect description length in server's list response
> > 
> > 
> > > > @@ -648,6 +657,7 @@ static int nbd_send_meta_query(QIOChannel *ioc, uint32_t opt,
> > > >        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);
> > > >        }
> > > 
> > > you may assert export_len as well..
> > 
> > It was asserted earlier, but doing it again might not hurt, especially if I do the followup patch getting rid of NBD_MAX_NAME_SIZE
> > 
> > 
> > > > @@ -1561,6 +1569,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
> > > >            exp->export_bitmap = bm;
> > > >            exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
> > > >                                                         bitmap);
> > > > +        /* See BME_MAX_NAME_SIZE in block/qcow2-bitmap.c */
> > > 
> > > Hmm. BME_MAX_NAME_SIZE is checked only when creating persistent bitmaps. But for non-persistent
> > > name length is actually unlimited. So, we should either limit all bitmap names to 1023 (hope,
> > > this will not break existing scenarios) or error out here (or earlier) instead of assertion.
> > 
> > I'm leaning towards limiting ALL bitmaps to the same length (as we've already debated the idea of being able to convert an existing bitmap from transient to persistent).
> 
> Agreed, but ..
> 
> > 
> > > 
> > > We also may want QEMU_BUILD_BUG_ON(NBD_MAX_STRING_SIZE < BME_MAX_NAME_SIZE + sizeof("qemu:dirty-bitmap:") - 1)
> > 
> > Except that BME_MAX_NAME_SIZE is not (currently) in a public .h file.
> > 
> 
> .. I think, than it should be new BLOCK_DIRTY_BITMAP_MAX_NAME_SIZE.. And we'll have to note it in qapi doc..
> Should this change go through deprecation? Or we consider non-persistent bitmaps as something not really useful?
> 
> -- 
> Best regards,
> Vladimir

I followed upon the new patch and the comments, and it seems ok now to me, (including the comments that were already made) but I haven't
checked if there are more cases of missing length checks.

Best regards,
	Maxim Levitsky