Re: [Libguestfs] [libnbd PATCH v2 4/6] block_status: Fix assertion with large server size

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 <rjones(a)redhat.com&gt; Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- 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";

As before, we never report a zero-length extent to the callback.