Re: [Libguestfs] [libnbd PATCH v2 4/6] block_status: Fix assertion with large server size
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
> Signed-off-by: Eric Blake
> ---
> 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;
> -
Re: [Libguestfs] [libnbd PATCH v2 4/6] block_status: Fix assertion with large server size
On Thu, Sep 21, 2023 at 03:58:03PM -0500, 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
> Signed-off-by: Eric Blake
> ---
> 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 =
[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
Signed-off-by: Eric Blake
---
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
