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 <ebl...@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 <vsement...@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
