Re: [Libguestfs] [libnbd PATCH v2 5/6] block_status: Fix assertion on bad 64-bit block status reply
On 9/21/23 22:58, Eric Blake wrote:
> If a server replies to a block status command with an invalid count 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. Rich's fuzzing run
> initially found this, but I was able to quickly write a one-byte patch
> on top of my pending qemu patches [1] to reproduce it:
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg05231.html
>
> | diff --git i/nbd/server.c w/nbd/server.c
> | index 898580a9b0b..bd8d46ba3c4 100644
> | --- i/nbd/server.c
> | +++ w/nbd/server.c
> | @@ -2326,7 +2326,7 @@ nbd_co_send_extents(NBDClient *client, NBDRequest
> *request, NBDExtentArray *ea,
> | iov[1].iov_base = _ext;
> | iov[1].iov_len = sizeof(meta_ext);
> | stl_be_p(_ext.context_id, context_id);
> | -stl_be_p(_ext.count, ea->count);
> | +stl_be_p(_ext.count, !ea->count);
> |
> | nbd_extent_array_convert_to_be(ea);
> | iov[2].iov_base = ea->extents;
>
> then with a just-built 'qemu-nbd -f raw -r file -t &' running, we have
> pre-patch:
>
> $ ./run nbdsh --base -u nbd://localhost -c - <<\EOF
>> def f(*k):
>> pass
>> try:
>> h.block_status(1,0,f)
>> except nbd.Error as ex:
>> print(ex.string)
>> h.shutdown()
>> EOF
> nbdsh: generator/states-reply-chunk.c:701:
> enter_STATE_REPLY_CHUNK_REPLY_FINISH: Assertion `h->payload_left == 0' failed.
> Aborted (core dumped)
>
> vs. post-patch:
>
> $ ./run nbdsh --base -u nbd://localhost -c - <<\EOF
>> def f(*k):
>> pass
>> try:
>> h.block_status(1,0,f)
>> except nbd.Error as ex:
>> print(ex.string)
>> h.shutdown()
>> EOF
> nbd_block_status: block-status: command failed: Protocol error
>
> 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 h->hlen
> modification sites show that all other chunked reads updated
> h->payload_left appropriately (often in the next statement, but
> sometimes in a later state when that made logic easier).
>
> Requires a non-compliant server, and only possible when extended
> headers are negotiated, which does not affect any stable released
> libnbd. Thus, there is no reason to create a CVE, although since I
> will already be doing a security info email about previous patches
> also addressing fuzzer findings, I can mention this at the same time.
>
> Fixes: ab992766cd ("block_status: Accept 64-bit extents during block status")
> Thanks: Richard W.M. Jones
> Signed-off-by: Eric Blake
> ---
> 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 20407d91..5a31c192 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;
>}
Right, this seems consistent with other transitions to RESYNC (we zero
out payload_left in all those locations), and after we go from RESYNC to
FINISH, FINISH asserts the zero value.
Reviewed-by: Laszlo Ersek
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH v2 5/6] block_status: Fix assertion on bad 64-bit block status reply
On Thu, Sep 21, 2023 at 03:58:04PM -0500, Eric Blake wrote:
> If a server replies to a block status command with an invalid count 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. Rich's fuzzing run
> initially found this, but I was able to quickly write a one-byte patch
> on top of my pending qemu patches [1] to reproduce it:
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg05231.html
>
> | diff --git i/nbd/server.c w/nbd/server.c
> | index 898580a9b0b..bd8d46ba3c4 100644
> | --- i/nbd/server.c
> | +++ w/nbd/server.c
> | @@ -2326,7 +2326,7 @@ nbd_co_send_extents(NBDClient *client, NBDRequest
> *request, NBDExtentArray *ea,
> | iov[1].iov_base = _ext;
> | iov[1].iov_len = sizeof(meta_ext);
> | stl_be_p(_ext.context_id, context_id);
> | -stl_be_p(_ext.count, ea->count);
> | +stl_be_p(_ext.count, !ea->count);
> |
> | nbd_extent_array_convert_to_be(ea);
> | iov[2].iov_base = ea->extents;
>
> then with a just-built 'qemu-nbd -f raw -r file -t &' running, we have
> pre-patch:
>
> $ ./run nbdsh --base -u nbd://localhost -c - <<\EOF
> > def f(*k):
> > pass
> > try:
> > h.block_status(1,0,f)
> > except nbd.Error as ex:
> > print(ex.string)
> > h.shutdown()
> > EOF
> nbdsh: generator/states-reply-chunk.c:701:
> enter_STATE_REPLY_CHUNK_REPLY_FINISH: Assertion `h->payload_left == 0' failed.
> Aborted (core dumped)
>
> vs. post-patch:
>
> $ ./run nbdsh --base -u nbd://localhost -c - <<\EOF
> > def f(*k):
> > pass
> > try:
> > h.block_status(1,0,f)
> > except nbd.Error as ex:
> > print(ex.string)
> > h.shutdown()
> > EOF
> nbd_block_status: block-status: command failed: Protocol error
>
> 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 h->hlen
> modification sites show that all other chunked reads updated
> h->payload_left appropriately (often in the next statement, but
> sometimes in a later state when that made logic easier).
>
> Requires a non-compliant server, and only possible when extended
> headers are negotiated, which does not affect any stable released
> libnbd. Thus, there is no reason to create a CVE, although since I
> will already be doing a security info email about previous patches
> also addressing fuzzer findings, I can mention this at the same time.
>
> Fixes: ab992766cd ("block_status: Accept 64-bit extents during block status")
> Thanks: Richard W.M. Jones
> Signed-off-by: Eric Blake
> ---
> 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 20407d91..5a31c192 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;
>}
Reviewed-by: Richard W.M. Jones
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [libnbd PATCH v2 5/6] block_status: Fix assertion on bad 64-bit block status reply
If a server replies to a block status command with an invalid count 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. Rich's fuzzing run
initially found this, but I was able to quickly write a one-byte patch
on top of my pending qemu patches [1] to reproduce it:
[1] https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg05231.html
| diff --git i/nbd/server.c w/nbd/server.c
| index 898580a9b0b..bd8d46ba3c4 100644
| --- i/nbd/server.c
| +++ w/nbd/server.c
| @@ -2326,7 +2326,7 @@ nbd_co_send_extents(NBDClient *client, NBDRequest
*request, NBDExtentArray *ea,
| iov[1].iov_base = _ext;
| iov[1].iov_len = sizeof(meta_ext);
| stl_be_p(_ext.context_id, context_id);
| -stl_be_p(_ext.count, ea->count);
| +stl_be_p(_ext.count, !ea->count);
|
| nbd_extent_array_convert_to_be(ea);
| iov[2].iov_base = ea->extents;
then with a just-built 'qemu-nbd -f raw -r file -t &' running, we have
pre-patch:
$ ./run nbdsh --base -u nbd://localhost -c - <<\EOF
> def f(*k):
> pass
> try:
> h.block_status(1,0,f)
> except nbd.Error as ex:
> print(ex.string)
> h.shutdown()
> EOF
nbdsh: generator/states-reply-chunk.c:701:
enter_STATE_REPLY_CHUNK_REPLY_FINISH: Assertion `h->payload_left == 0' failed.
Aborted (core dumped)
vs. post-patch:
$ ./run nbdsh --base -u nbd://localhost -c - <<\EOF
> def f(*k):
> pass
> try:
> h.block_status(1,0,f)
> except nbd.Error as ex:
> print(ex.string)
> h.shutdown()
> EOF
nbd_block_status: block-status: command failed: Protocol error
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 h->hlen
modification sites show that all other chunked reads updated
h->payload_left appropriately (often in the next statement, but
sometimes in a later state when that made logic easier).
Requires a non-compliant server, and only possible when extended
headers are negotiated, which does not affect any stable released
libnbd. Thus, there is no reason to create a CVE, although since I
will already be doing a security info email about previous patches
also addressing fuzzer findings, I can mention this at the same time.
Fixes: ab992766cd ("block_status: Accept 64-bit extents during block status")
Thanks: Richard W.M. Jones
Signed-off-by: Eric Blake
---
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 20407d91..5a31c192 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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
