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

[Eric Blake]

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