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

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(a)redhat.com&gt; 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.