Re: [PATCH v2 1/3] nbd: CVE-XXX: Use cookie to track generation of nbd-server

[Eric Blake]

On Fri, Aug 02, 2024 at 06:00:32PM GMT, Vladimir Sementsov-Ogievskiy wrote:
> On 02.08.24 04:32, Eric Blake wrote:

> [..]
> 
> > -static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
> > +static void nbd_blockdev_client_closed(NBDClient *client, uint32_t cookie,
> > +                                       bool ignored)
> >   {
> >       nbd_client_put(client);
> > -    assert(nbd_server->connections > 0);
> > -    nbd_server->connections--;
> > -    nbd_update_server_watch(nbd_server);
> > +    /* Ignore any (late) connection made under a previous server */
> > +    if (cookie == nbd_cookie) {
> 
> creating a getter nbd_client_get_cookie(client), and use it instead of 
> passing together with client, will simplify the patch a lot. [*]

I may be able to avoid the cookie altogether if I can add an
AIO_WAIT_WHILE(, nbd_server->connections > 0) after forcefully closing
all of the client sockets (nbd_client_new _should_ progress pretty
rapidly towards eventually calling nbd_blockdev_client_closed once the
socket is closed) - but that still requires patch 2 to keep a list of
open clients.

> 
> Hmm.. don't we need some atomic accessors for nbd_cookie? and for 
> nbs_server->connections.. The function is called from client, which live in 
> coroutine and maybe in another thread? At least client code do atomic 
> accesses of client->refcount..
> 
> > +        assert(nbd_server->connections > 0);
> > +        nbd_server->connections--;
> > +        nbd_update_server_watch(nbd_server);
> > +    }
> >   }
> > 
> 
> [..]
> 
> > @@ -1621,7 +1622,7 @@ static void client_close(NBDClient *client, bool 
> > negotiated)
> > 
> >       /* Also tell the client, so that they release their reference.  */
> >       if (client->close_fn) {
> > -        client->close_fn(client, negotiated);
> > +        client->close_fn(client, client->close_cookie, negotiated);
> 
> [*] passing client->close_cokkie together with client itself looks like we 
> lack a getter for .close_cookie

Whether the cookie be a uint32_t or the void* server object itself, it
is opaque to the client, but the server needs to track something.


> 
> >       }
> >   }
> > 
> 
> [..]
> 
> 
> Hmm, instead of cookies and additional NBDConn objects in the next patch, 
> could we simply have a list of connected NBDClient objects in NBDServer and 
> link to NBDServer in NBDClient? (Ok we actually don't have NBDServer, but 
> NBDServerData in qemu, and several global variables in qemu-nbd, so some 
> refactoring is needed, to put common state to NBDServer, and add clients list 
> to it)
> 
> This way, in nbd_server_free we'll just call client_close() in a loop. And in 
> client_close we'll have nbd_server_client_detach(client->server, client), 
> instead of client->close_fn(...). And server is freed only after all clients 
> are closed. And client never try to detach from another server.
> 
> This way, we also may implement several NBD servers working simultaneously if 
> we want.

Yes, we do eventually want to get to the point of being able to open
parallel NBD servers on different ports simultaneously, at which point
having a client remember which server it is associated makes sense (so
at a bare minimum, pass in a void* instead of a uint32_t to
nbd_client_new).  And given that we can have an NBD server with more
than one export, and with exports running in different threads (if
multithread io is enabled), I probably also need to add missing
locking to protect nbd_server (whether or not it stays global or we
eventually reach the point of having parallel servers on separate
ports).

Looks like I have work cut out for me before posting a v3.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org