On 9/21/23 22:58, Eric Blake wrote:
> As mentioned in the previous commit ("api: Sanitize sizes larger than
> INT64_MAX"), the NBD spec does not (yet) prohibit a server from
> advertising a size larger than INT64_MAX. While we can't report such
> size to the user, v1.16 was at least internally consistent with the
> server's size everywhere else.
>
> But when adding code to implement 64-bit block status, I intentionally
> wanted to guarantee that the callback sees a positive int64_t length
> even when the server's wire value can be 64 bits, for ease in writing
> language bindings (OCaml in particular benefitted from that
> guarantee), even though I didn't document that in the API until now.
> That was because I had blindly assumed that the server's exportsize
> fit in 63 bits, and therefore I didn't have to worry about arithmetic
> overflow once I capped the extent length to exportsize. But the
> fuzzer quickly proved me wrong. What's more, with the same one-line
> hack to qemu as shown in the previous commit to advertise a size with
> the high-order bit set,
>
> $ ./run nbdsh --base -u nbd://localhost -c - <<\EOF
>> def f(*k):
>> pass
>> h.block_status(1,0,f)
>> EOF
> nbdsh: generator/states-reply-chunk.c:554:
> enter_STATE_REPLY_CHUNK_REPLY_RECV_BS_ENTRIES: Assertion `h->exportsize <=
> INT64_MAX' failed.
> Aborted (core dumped)
>
> even though it did not dump core in v1.16.
>
> Since my assumption was bad, rewrite the logic to increment total
> after bounds-checking rather than before, and to bounds-check based on
> INT64_MAX+1-64M rather than on the export size. As before, we never
> report a zero-length extent to the callback. Whether or not secalert
> advises us to create a CVE for the previous patch, this bug does not
> deserve its own CVE as it was only introduced in recent unstable
> releases.
>
> Fixes: e8d837d306 ("block_status: Add some sanity checking of server
> lengths", v1.17.4)
> Thanks: Richard W.M. Jones <rjo...@redhat.com>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
> generator/states-reply-chunk.c | 49 ++++++++++++++++++----------------
> generator/C.ml | 2 +-
> 2 files changed, 27 insertions(+), 24 deletions(-)
>
> diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c
> index 2cebe456..20407d91 100644
> --- a/generator/states-reply-chunk.c
> +++ b/generator/states-reply-chunk.c
> @@ -547,11 +547,16 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
> break;
> }
>
> + /* Be careful to avoid arithmetic overflow, even when the user
> + * disabled LIBNBD_STRICT_BOUNDS to pass a suspect offset, or the
> + * server returns suspect lengths or advertised exportsize larger
> + * than 63 bits. We guarantee that callbacks will not see a
> + * length exceeding INT64_MAX or the advertised h->exportsize.
> + */
> name = h->meta_contexts.ptr[i].name;
> - total = 0;
> - cap = h->exportsize - cmd->offset;
> - assert (cap <= h->exportsize);
> - assert (h->exportsize <= INT64_MAX);
> + total = cap = 0;
> + if (cmd->offset <= h->exportsize)
> + cap = h->exportsize - cmd->offset;
>
> /* Need to byte-swap the entries returned into the callback size
> * requested by the caller. The NBD protocol allows truncation as
> @@ -560,10 +565,11 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
> * don't like. We stop iterating on a zero-length extent (error
> * only if it is the first extent), on an extent beyond the
> * exportsize (unconditional error after truncating to
> - * exportsize), and on an extent exceeding a 32-bit callback (no
> - * error, and to simplify alignment, we truncate to 4G-64M); but
> - * do not diagnose issues with the server's length alignments,
> - * flag values, nor compliance with the REQ_ONE command flag.
> + * exportsize), and on an extent exceeding a callback length limit
> + * (no error, and to simplify alignment, we truncate to 64M before
> + * the limit); but we do not diagnose issues with the server's
> + * length alignments, flag values, nor compliance with the REQ_ONE
> + * command flag.
> */
> for (i = 0, stop = false; i < h->bs_count && !stop; ++i) {
> if (type == NBD_REPLY_TYPE_BLOCK_STATUS) {
> @@ -572,16 +578,12 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
> }
> else {
> orig_len = len = be64toh (h->bs_raw.wide[i].length);
> - if (len > h->exportsize) {
> - /* Since we already asserted exportsize is at most 63 bits,
> - * this ensures the extent length will appear positive even
> - * if treated as signed; treat this as an error now, rather
> - * than waiting for the comparison to cap later, to avoid
> - * arithmetic overflow.
> + if (len > INT64_MAX) {
> + /* Pick an aligned value rather than overflowing 64-bit
> + * callback; this does not require an error.
> */
> stop = true;
> - cmd->error = cmd->error ? : EPROTO;
> - len = h->exportsize;
> + len = INT64_MAX + 1ULL - MAX_REQUEST_SIZE;
> }
> if (len > UINT32_MAX && !cmd->cb.wide) {
> /* Pick an aligned value rather than overflowing 32-bit
> @@ -600,7 +602,13 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
> }
> }
>
> - total += len;
> + assert (total <= cap);
> + if (len > cap - total) {
> + /* Truncate and expose this extent as an error */
> + len = cap - total;
> + stop = true;
> + cmd->error = cmd->error ? : EPROTO;
> + }
> if (len == 0) {
> stop = true;
> if (i > 0)
> @@ -608,12 +616,7 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
> /* Expose this extent as an error; we made no progress */
> cmd->error = cmd->error ? : EPROTO;
> }
> - else if (total > cap) {
> - /* Expose this extent as an error, after truncating to make progress
> */
> - stop = true;
> - cmd->error = cmd->error ? : EPROTO;
> - len -= total - cap;
> - }
> + total += len;
> if (cmd->cb.wide) {
> h->bs_cooked.wide[i].length = len;
> h->bs_cooked.wide[i].flags = flags;
> diff --git a/generator/C.ml b/generator/C.ml
> index e5a2879b..ccaed116 100644
> --- a/generator/C.ml
> +++ b/generator/C.ml
> @@ -496,7 +496,7 @@ let
> pr "/* This is used in the callback for nbd_block_status_64.\n";
> pr " */\n";
> pr "typedef struct {\n";
> - pr " uint64_t length;\n";
> + pr " uint64_t length; /* Will not exceed INT64_MAX */\n";
> pr " uint64_t flags;\n";
> pr "} nbd_extent;\n";
> pr "\n";
I like that this patch handles (len == 0) uniformly between
(a) the server directly sending a 0-length extent, and
(b) len==0 resulting from a (series of) local truncation(s).
A consequence of that however is that -- I think -- the commit message
is inexact:
> As before, we never
> report a zero-length extent to the callback.
Suggest to append: "except when the zero-length is encountered in the
first extent, directly from the server, or as a result of client-side
truncations".
Because, both before and after the patch, we do seem to expose the
len==0 extent, in case i==0.
NB: I don't know *why* we report a zero-length extent if it is the very
first extent (both before and after the patch). But, that's not
particularly important; what's important is that the patch preserves,
and consistently extends, that behavior.
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
