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";
--
2.41.0
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
