Re: [Libguestfs] [libnbd PATCH v2 5/5] copy: CVE-2022-0485: Fail nbdcopy if NBD read or write fails

+# Tests several scenarios of handling NBD server errors +# Serves as a regression test for the CVE-2022-0485 fix. + +. ../tests/functions.sh + +set -e +set -x + +requires nbdkit --exit-with-parent --version

+fail=0 + +# Failure to get block status should not be fatal, but merely downgrade to +# reading the entire image as if data +echo "Testing extents failures on source" +$VG nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error pattern 5M \ + error-extents-rate=1 ] null: || fail=1 + +# Failure to read should be fatal +echo "Testing read failures on non-sparse source" +$VG nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error pattern 5M \ + error-pread-rate=0.5 ] null: && fail=1 + +# However, reliable block status on a sparse image can avoid the need to read +echo "Testing read failures on sparse source" +$VG nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error null 5M \ + error-pread-rate=1 ] null: || fail=1 + +# Failure to write data should be fatal +echo "Testing write data failures on arbitrary destination" +$VG nbdcopy -- [ nbdkit --exit-with-parent -v pattern 5M ] \ + [ nbdkit --exit-with-parent -v --filter=error --filter=noextents \ + memory 5M error-pwrite-rate=0.5 ] && fail=1 + +# However, writing zeroes can bypass the need for normal writes +echo "Testing write data failures from sparse source" +$VG nbdcopy -- [ nbdkit --exit-with-parent -v null 5M ] \ + [ nbdkit --exit-with-parent -v --filter=error --filter=noextents \ + memory 5M error-pwrite-rate=1 ] || fail=1 + +# Failure to write zeroes should be fatal +echo "Testing write zero failures on arbitrary destination" +$VG nbdcopy -- [ nbdkit --exit-with-parent -v null 5M ] \ + [ nbdkit --exit-with-parent -v --filter=error memory 5M \ + error-zero-rate=1 ] && fail=1 + +# However, assuming/learning destination is zero can skip need to write +echo "Testing write failures on pre-zeroed destination" +$VG nbdcopy --destination-is-zero -- \ + [ nbdkit --exit-with-parent -v null 5M ] \ + [ nbdkit --exit-with-parent -v --filter=error memory 5M \ + error-pwrite-rate=1 error-zero-rate=1 ] || fail=1 + +# Likewise, when write zero is not advertised, fallback to normal write works +echo "Testing write zeroes to destination without zero support" +$VG nbdcopy -- [ nbdkit --exit-with-parent -v null 5M ] \ + [ nbdkit --exit-with-parent -v --filter=nozero --filter=error memory 5M \ + error-zero-rate=1 ] || fail=1 + +exit $fail diff --git a/copy/file-ops.c b/copy/file-ops.c index e37a5014..aaf04ade 100644 --- a/copy/file-ops.c +++ b/copy/file-ops.c @@ -591,10 +591,8 @@ file_asynch_read (struct rw *rw, file_synch_read (rw, slice_ptr (command->slice), command->slice.len, command->offset); - if (cb.callback (cb.user_data, &dummy) == -1) { - perror (rw->name); - exit (EXIT_FAILURE); - } + /* file_synch_read called exit() on error */ + cb.callback (cb.user_data, &dummy); } static void @@ -606,10 +604,8 @@ file_asynch_write (struct rw *rw, file_synch_write (rw, slice_ptr (command->slice), command->slice.len, command->offset); - if (cb.callback (cb.user_data, &dummy) == -1) { - perror (rw->name); - exit (EXIT_FAILURE); - } + /* file_synch_write called exit() on error */ + cb.callback (cb.user_data, &dummy); } static bool @@ -620,10 +616,7 @@ file_asynch_zero (struct rw *rw, struct command *command, if (!file_synch_zero (rw, command->offset, command->slice.len, allocate)) return false; - if (cb.callback (cb.user_data, &dummy) == -1) { - perror (rw->name); - exit (EXIT_FAILURE); - } + cb.callback (cb.user_data, &dummy); return true; } diff --git a/copy/multi-thread-copying.c b/copy/multi-thread-copying.c index 815b8a02..2aa74d63 100644 --- a/copy/multi-thread-copying.c +++ b/copy/multi-thread-copying.c @@ -28,6 +28,7 @@ #include <errno.h> #include <assert.h> #include <sys/stat.h> +#include <inttypes.h> #include <pthread.h> @@ -376,6 +377,13 @@ finished_read (void *vp, int *error) { struct command *command = vp; + /* XXX - is it worth retrying a failed command? */

+ if (*error) { + fprintf (stderr, "read at offset 0x%" PRIx64 "failed: %s\n", + command->offset, strerror (*error)); + exit (EXIT_FAILURE); + } + if (allocated || sparse_size == 0) { /* If sparseness detection (see below) is turned off then we write * the whole command. @@ -548,6 +556,13 @@ free_command (void *vp, int *error) struct command *command = vp; struct buffer *buffer = command->slice.buffer; + /* XXX - is it worth retrying a failed command? */ + if (*error) { + fprintf (stderr, "write at offset 0x%" PRIx64 "failed: %s\n", + command->offset, strerror (*error)); + exit (EXIT_FAILURE); + } + if (buffer != NULL) { if (--buffer->refs == 0) { free (buffer->data); diff --git a/copy/nbdcopy.h b/copy/nbdcopy.h index e7fe1eab..c070f8d7 100644 --- a/copy/nbdcopy.h +++ b/copy/nbdcopy.h @@ -1,5 +1,5 @@ /* NBD client library in userspace. - * Copyright (C) 2020-2021 Red Hat Inc. + * Copyright (C) 2020-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -157,7 +157,8 @@ struct rw_ops { bool allocate); /* Asynchronous I/O operations. These start the operation and call - * 'cb' on completion. + * 'cb' on completion. 'cb' will return 1, for auto-retiring with + * asynchronous libnbd calls. * * The file_ops versions are actually implemented synchronously, but * still call 'cb'. @@ -173,7 +174,7 @@ struct rw_ops { nbd_completion_callback cb); /* Asynchronously zero. command->slice.buffer is not used. If not possible, - * returns false. + * returns false. 'cb' must be called only if returning true. */ bool (*asynch_zero) (struct rw *rw, struct command *command, nbd_completion_callback cb, bool allocate); diff --git a/copy/null-ops.c b/copy/null-ops.c index 11cd5116..5f1fda50 100644 --- a/copy/null-ops.c +++ b/copy/null-ops.c @@ -119,10 +119,7 @@ null_asynch_write (struct rw *rw, { int dummy = 0; - if (cb.callback (cb.user_data, &dummy) == -1) { - perror (rw->name); - exit (EXIT_FAILURE); - } + cb.callback (cb.user_data, &dummy); } static bool @@ -131,10 +128,7 @@ null_asynch_zero (struct rw *rw, struct command *command, { int dummy = 0; - if (cb.callback (cb.user_data, &dummy) == -1) { - perror (rw->name); - exit (EXIT_FAILURE); - } + cb.callback (cb.user_data, &dummy); return true; }