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

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
>>> ---
> 
>>> +++ 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 <address@hidden>

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