Re: [PATCH v3 05/10] block/dirty-bitmap: switch _next_dirty_area and _next_zero to int64_t

From: Eric Blake <eblake@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	Max Reitz <mreitz@redhat.com>,
	"qemu-block@nongnu.org" <qemu-block@nongnu.org>
Cc: "kwolf@redhat.com" <kwolf@redhat.com>,
	"jsnow@redhat.com" <jsnow@redhat.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	Denis Lunev <den@virtuozzo.com>
Subject: Re: [PATCH v3 05/10] block/dirty-bitmap: switch _next_dirty_area and _next_zero to int64_t
Date: Mon, 20 Jan 2020 13:56:23 -0600	[thread overview]
Message-ID: <a9d1c3a2-5121-75dd-3682-3c02a510b494@redhat.com> (raw)
In-Reply-To: <c2f78255-c001-01a3-487a-f7cf224f86f8@virtuozzo.com>

On 1/20/20 6:28 AM, Vladimir Sementsov-Ogievskiy wrote:

>> As far as I can see, NBD just passes NBDRequest.from (which is a
>> uint64_t) to this function (on NBD_CMD_BLOCK_STATUS).  Would this allow
>> a malicious client to send a value > INT64_MAX, thus provoking an
>> overflow and killing the server with this new assertion?
> 
> 
> in nbd_co_receive_request() we have
> 
> 
>       if (request->from > client->exp->size ||
>           request->len > client->exp->size - request->from) {
> 
> 
> So, we check that from is <= exp->size. and exp->size cant be greater than INT64_MAX,
> as it derived from bdrv_getlength, which returns int64_t.
> 
> 
> 
> Interesting, should we be more strict in server:?

I think we're okay based on the existing bounds checks.

> 
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2178,7 +2178,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
>            error_setg(errp, "Export is read-only");
>            return -EROFS;
>        }
> -    if (request->from > client->exp->size ||
> +    if (request->from >= client->exp->size ||
>            request->len > client->exp->size - request->from) {
>            error_setg(errp, "operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
>                       ", Size: %" PRIu64, request->from, request->len,
> 
> Or is it intentional? Looking through NBD spec I found only
> 
>      client MUST NOT use a length ... or which, when added to offset, would exceed the export size.
> 
> So, formally pair offset=<export size>, len=0 is valid...

Except that the spec also says that len=0 is generally unspecified 
behavior (whether it is a no-op, or means special handling, or whatever 
else, is up to the server, but clients shouldn't be sending it - thus a 
server that rejects it instead of handling it as a no-op is no worse for 
the wear).

> 
>>
>> On second thought, we have this problem already everywhere in
>> nbd_handle_request().  I don’t see it or its caller ever checking
>> whether the received values are in bounds, it just passes them to all
>> kind of block layer functions that sometimes even just accept plain
>> ints.  Well, I suppose all other functions just error out, so it
>> probably isn’t an actual problem in practice so far...
>>
>> Max
>>
> 
> 

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