Libguestfs - Sep 2023 - [libnbd PATCH] generator: Fix assertion with ill-formed 64-bit block status reply

If a server replies to a block status command with an invalid length
in NBD_REPLY_TYPE_BLOCK_STATUS_EXT, we correctly detect the server's
error, but fail to mark that we've consumed enough data off the wire
to resync back to the server's next reply.  Symptoms seen during a
fuzzing run:

?  $ ./fuzzing/libnbd-fuzz-wrapper
?+id\:000001\,sig\:06\,src\:000396\,time\:16888628\,execs\:54159419\,op\:havoc\,rep\:4
?  libnbd-fuzz-wrapper: generator/states-reply-chunk.c:698:
enter_STATE_REPLY_CHUNK_REPLY_FINISH:
?+Assertion `h->payload_left == 0' failed.
?  Aborted (core dumped)
?  read: Connection reset by peer

Appears to be a casualty of rebasing: I added h->payload_left
verification fairly late in the game, then floated it earlier in the
series, and missed a spot where I added a state machine jump to RESYNC
without having updated h->payload_left.  An audit of all other
assignments to h->rlen in that file was able to find corresponding
assignments to h->payload_left (often the next statement, but
sometimes split across states based on what made the next state easier
to code).

Requires a non-compliant server, but I was able to come up with a
one-line tweak to pending qemu patches that could trigger it.  Not
creating a CVE as it only appears in unstable releases.

Fixes: ab992766cd ("block_status: Accept 64-bit extents during block
status")
Thanks: Richard W.M. Jones <rjones at redhat.com>
Signed-off-by: Eric Blake <eblake at redhat.com>
---

I'm investigating another crash that Rich sent me off-list, but I
suspect it will be a similar non-CVE situation caused by my recent
64-bit extension patches.

I'll wait to apply this one for just a bit more, in case I can get the
corpus file or two from Rich's fuzzing run to add to
fuzzing/testcase_dir.

 generator/states-reply-chunk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c
index 2cebe456..a5d3aefe 100644
--- a/generator/states-reply-chunk.c
+++ b/generator/states-reply-chunk.c
@@ -476,6 +476,7 @@  REPLY.CHUNK_REPLY.RECV_BS_HEADER:
       if (h->bs_count != be32toh (h->sbuf.reply.payload.bs_hdr_64.count))
{
         h->rbuf = NULL;
         h->rlen = h->payload_left;
+        h->payload_left = 0;
         SET_NEXT_STATE (%RESYNC);
         return 0;
       }
-- 
2.41.0

On Mon, Sep 18, 2023 at 02:31:28PM -0500, Eric Blake
wrote:
> If a server replies to a block status command with an invalid length
> in NBD_REPLY_TYPE_BLOCK_STATUS_EXT, we correctly detect the server's
> error, but fail to mark that we've consumed enough data off the wire
> to resync back to the server's next reply.  Symptoms seen during a
> fuzzing run:
> 
> ?  $ ./fuzzing/libnbd-fuzz-wrapper
>
?+id\:000001\,sig\:06\,src\:000396\,time\:16888628\,execs\:54159419\,op\:havoc\,rep\:4
> ?  libnbd-fuzz-wrapper: generator/states-reply-chunk.c:698:
enter_STATE_REPLY_CHUNK_REPLY_FINISH:
> ?+Assertion `h->payload_left == 0' failed.
> ?  Aborted (core dumped)
> ?  read: Connection reset by peer
> 
> Appears to be a casualty of rebasing: I added h->payload_left
> verification fairly late in the game, then floated it earlier in the
> series, and missed a spot where I added a state machine jump to RESYNC
> without having updated h->payload_left.  An audit of all other
> assignments to h->rlen in that file was able to find corresponding
> assignments to h->payload_left (often the next statement, but
> sometimes split across states based on what made the next state easier
> to code).
> 
> Requires a non-compliant server, but I was able to come up with a
> one-line tweak to pending qemu patches that could trigger it.  Not
> creating a CVE as it only appears in unstable releases.
> 
> Fixes: ab992766cd ("block_status: Accept 64-bit extents during block
status")
> Thanks: Richard W.M. Jones <rjones at redhat.com>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> 
> I'm investigating another crash that Rich sent me off-list, but I
> suspect it will be a similar non-CVE situation caused by my recent
> 64-bit extension patches.
> 
> I'll wait to apply this one for just a bit more, in case I can get the
> corpus file or two from Rich's fuzzing run to add to
> fuzzing/testcase_dir.
> 
>  generator/states-reply-chunk.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/generator/states-reply-chunk.c
b/generator/states-reply-chunk.c
> index 2cebe456..a5d3aefe 100644
> --- a/generator/states-reply-chunk.c
> +++ b/generator/states-reply-chunk.c
> @@ -476,6 +476,7 @@  REPLY.CHUNK_REPLY.RECV_BS_HEADER:
>        if (h->bs_count != be32toh
(h->sbuf.reply.payload.bs_hdr_64.count)) {
>          h->rbuf = NULL;
>          h->rlen = h->payload_left;
> +        h->payload_left = 0;
>          SET_NEXT_STATE (%RESYNC);
>          return 0;
>        }
ACK, thanks.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit