On Tue, Apr 22, 2025 at 05:07:17PM -0500, Eric Blake via Libguestfs wrote:
> Latent since the introduction of the .extents callback. The loop
> intentionally truncates to 2**32-1 bytes, but condition to end the
> loop early used > while the assertion after the loop used <=, meaning
> that the assertion can fire for any plugin that returns an extent of
> 2**32 or larger.
>
> Fixes: 26455d45 ('server: protocol: Implement Block Status
> "base:allocation".', v1.11.10)
> Reported-by: Nikolay Ivanets <stena...@gmail.com>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
>
> We still need to sort out if this gets a CVE number; either way, the
> commit title needs an update, and if it does get a CVE we need to also
> post a security alert message to the list.
>
> tests/Makefile.am | 2 ++
> server/protocol.c | 2 +-
> tests/test-eval-extents.sh | 60 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 63 insertions(+), 1 deletion(-)
> create mode 100755 tests/test-eval-extents.sh
>
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 13c0457c..ffe42c78 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -881,6 +881,7 @@ TESTS += \
> test-eval.sh \
> test-eval-file.sh \
> test-eval-exports.sh \
> + test-eval-extents.sh \
> test-eval-cache.sh \
> test-eval-dump-plugin.sh \
> test-eval-disconnect.sh \
> @@ -889,6 +890,7 @@ EXTRA_DIST += \
> test-eval.sh \
> test-eval-file.sh \
> test-eval-exports.sh \
> + test-eval-extents.sh \
> test-eval-cache.sh \
> test-eval-dump-plugin.sh \
> test-eval-disconnect.sh \
> diff --git a/server/protocol.c b/server/protocol.c
> index d428bfc8..b4b1c162 100644
> --- a/server/protocol.c
> +++ b/server/protocol.c
> @@ -499,7 +499,7 @@ extents_to_block_descriptors (struct nbdkit_extents
> *extents,
> (*nr_blocks)++;
>
> pos += length;
> - if (pos > offset + count) /* this must be the last block */
> + if (pos >= offset + count) /* this must be the last block */
> break;
AIUI ...
blocks[i].length = length = MIN (e.length, UINT32_MAX);
The extent we're sending has e.length is >= 4G, which we truncate to
UINT32_MAX == 4G-1.
pos += length;
pos += 4G-1
In the previous code:
if (pos > offset + count) /* this must be the last block */
break;
branch not taken so we reach:
assert (e.length <= length);
e.length == 4G, length == 4G-1, so we assert here.
In the updated code, the assert is avoided, which means this branch
must now be taken, which means the client requested 'offset' == 0 &
'count' must be exactly 4G-1, I think? (Or any relative request)
if (pos >= offset + count) /* this must be the last block */
break;
> /* If we reach here then we must have consumed this whole
> diff --git a/tests/test-eval-extents.sh b/tests/test-eval-extents.sh
> new file mode 100755
> index 00000000..08c8d670
> --- /dev/null
> +++ b/tests/test-eval-extents.sh
> @@ -0,0 +1,60 @@
> +#!/usr/bin/env bash
> +# nbdkit
> +# Copyright Red Hat
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:
> +#
> +# * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +#
> +# * Redistributions in binary form must reproduce the above copyright
> +# notice, this list of conditions and the following disclaimer in the
> +# documentation and/or other materials provided with the distribution.
> +#
> +# * Neither the name of Red Hat nor the names of its contributors may be
> +# used to endorse or promote products derived from this software without
> +# specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND
> +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
> +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
> +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> +# SUCH DAMAGE.
> +
> +source ./functions.sh
> +set -e
> +set -x
> +
> +requires_run
> +requires_plugin eval
> +requires_nbdsh_uri
> +requires nbdsh --base-allocation
I think it's marginally better to use:
requires nbdsh --base-allocation --version
which should be the same test.
> +files="eval-extents.out"
> +rm -f $files
> +cleanup_fn rm -f $files
> +
> +# Trigger an off-by-one bug introduced in v1.11.10 and fixed in v1.43.6
> +export script='
> +def f(context, offset, extents, status):
> + print(extents)
> +
> +h.block_status(2**32-1, 0, f)
And here the requested offset == 0 & count == 4G-1, as deduced above.
> +'
> +nbdkit eval \
> + get_size='echo 5G' \
> + pread='dd if=/dev/zero count=$3 iflag=count_bytes' \
> + extents='echo 0 5G 0' \
> + --run 'nbdsh --base-allocation --uri "$uri" -c "$script"' \
> + > eval-extents.out
> +cat eval-extents.out
> +grep -E '\[4294967295, 0]' eval-extents.out
> --
It seems OK, ACK.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
_______________________________________________
Libguestfs mailing list -- guestfs@lists.libguestfs.org
To unsubscribe send an email to guestfs-le...@lists.libguestfs.org
