Libguestfs - May 2022 - [libnbd PATCH 1/2] api: Tighter checking of structured read replies

On Tue, May 31, 2022 at 7:13 PM Eric Blake <eblake at redhat.com>
wrote:
>
> On Tue, May 31, 2022 at 06:59:25PM +0300, Nir Soffer wrote:
> > On Tue, May 31, 2022 at 5:15 PM Eric Blake <eblake at
redhat.com> wrote:
> > >
> > > Now that we allow clients to bypass buffer pre-initialization, it
> > > becomes more important to detect when a buggy server using
structured
> > > replies does not send us enough bytes to cover the requested read
> > > size.  Our check is not perfect (a server that duplicates reply
chunks
> > > for byte 0 and omits byte 1 gets past our check), but this is a
> > > tighter sanity check so that we are less likely to report a
successful
> > > read containing uninitialized memory on a buggy server.
> >
> > Nice!
> >
> > > Because we have a maximum read buffer size of 64M, and first
check
> > > that the server's chunk fits bounds, we don't have to
worry about
> > > overflowing a uint32_t, even if a server sends enough duplicate
> > > responses that an actual sum would overflow.
> > > ---
>
> > > +++ b/generator/states-reply-structured.c
> > > @@ -354,7 +354,6 @@ STATE_MACHINE {
> > >      assert (cmd); /* guaranteed by CHECK */
> > >
> > >      assert (cmd->data && cmd->type ==
NBD_CMD_READ);
> > > -    cmd->data_seen = true;
> > >
> > >      /* Length of the data following. */
> > >      length -= 8;
> > > @@ -364,6 +363,8 @@ STATE_MACHINE {
> > >        SET_NEXT_STATE (%.DEAD);
> > >        return 0;
> > >      }
> > > +    if (cmd->data_seen <= cmd->count)
> > > +      cmd->data_seen += length;
> >
> > This does not feel right. if you received more data, it should be
counted,
> > and if this causes data_seen to be bigger than cmd->count,
isn't this a fatal
> > error?
>
> cmd->count is at most 64M; it represents how much we asked the server
> to provide.  length was just validated (in the elided statement
> between these two hunks) to be <= cmd->count (otherwise, the server
is
> known-buggy for replying with more than we asked, and we've already
> moved to %.DEAD state).  cmd->data_seen is a cumulative count of all
> bytes seen in prior chunks, plus the current chunk.  If we have not
> yet passed cmd->count, then this chunk counts towards the cumulative
> limit (and there is no overflow, since 64M*2 < UINT32_MAX).  If we
> have already passed cmd->count (in a previous chunk), then we don't
> further increase cmd->count, but we already know that we will fail the
> overall read later.  In other words, we can stop incrementing
> cmd->data_seen as soon as we know it exceeds cmd->count, and by
> stopping early, we still detect server bugs without overflowing
> uint32_t.
But we can have this case:

1. ask for 32m
2. server sends 16m (data_seen increase to 16m)
3. server sends 16m (data_seen increase to 32m)
4. server sends 1m (data_seen does not increase)
5. entire request succeeds

Shouldn't we fail if server sends unexpected data?

If we detected that all data was received, and we get
unexpected data, why not fail immediately?

    cmd->data_seen += length
    if (cmd->data_seen > cmd->count)
        switch to dead state?

Nir

On Tue, May 31, 2022 at 08:40:57PM +0300, Nir Soffer
wrote:
> > > > @@ -364,6 +363,8 @@ STATE_MACHINE {
> > > >        SET_NEXT_STATE (%.DEAD);
> > > >        return 0;
> > > >      }
> > > > +    if (cmd->data_seen <= cmd->count)
> > > > +      cmd->data_seen += length;
> > >
> > > This does not feel right. if you received more data, it should be
counted,
> > > and if this causes data_seen to be bigger than cmd->count,
isn't this a fatal
> > > error?
> >
> > cmd->count is at most 64M; it represents how much we asked the
server
> > to provide.  length was just validated (in the elided statement
> > between these two hunks) to be <= cmd->count (otherwise, the
server is
> > known-buggy for replying with more than we asked, and we've
already
> > moved to %.DEAD state).  cmd->data_seen is a cumulative count of
all
> > bytes seen in prior chunks, plus the current chunk.  If we have not
> > yet passed cmd->count, then this chunk counts towards the
cumulative
> > limit (and there is no overflow, since 64M*2 < UINT32_MAX).  If we
> > have already passed cmd->count (in a previous chunk), then we
don't
> > further increase cmd->count, but we already know that we will fail
the
> > overall read later.  In other words, we can stop incrementing
> > cmd->data_seen as soon as we know it exceeds cmd->count, and by
> > stopping early, we still detect server bugs without overflowing
> > uint32_t.
> 
> But we can have this case:
> 
> 1. ask for 32m
> 2. server sends 16m (data_seen increase to 16m)
> 3. server sends 16m (data_seen increase to 32m)
> 4. server sends 1m (data_seen does not increase)
Yes it does. 32m <= cmd->count is true, so we bump data_seen to 33m.

Then, later on when retiring the command, we note that 33m != 32m and
fail the read with EIO (if it has not already failed for other
reasons).
> 5. entire request succeeds
> 
> Shouldn't we fail if server sends unexpected data?
> 
> If we detected that all data was received, and we get
> unexpected data, why not fail immediately?
> 
>     cmd->data_seen += length
>     if (cmd->data_seen > cmd->count)
>         switch to dead state?
Switching immediately to a dead state is also possible, but it's nice
to try and keep the connection alive as long as we can with a nice
diagnosis of a failed CMD_READ but still allow further commands,
rather than an abrupt disconnect that takes out all other use of the
server.

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