[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
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
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1

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 
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: %" 
                      ", 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...


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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]