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:
> > -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. [*]
> 
> 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);

The client code already jumps through hoops to make sure
nbd_blockdev_client_closed() is the last thing called, and that
nbd_client_put() is only reached from the main thread (any coroutines
fired off a one-shot bh); but I added asserts in v3 to make it clear
I'm relying on the synchronous nature of coroutines yielding only at
known points and the code executing only in the main thread as the
reason why we don't need explicit locking here.

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