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

[Eric Blake]

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.

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