[Libguestfs] [libnbd PATCH v2 5/5] copy: CVE-2022-0485: Fail nbdcopy if NBD read or write fails
Richard W.M. Jones
rjones at redhat.com
Thu Feb 3 21:48:02 UTC 2022
On Thu, Feb 03, 2022 at 02:25:58PM -0600, Eric Blake wrote:
...
> +# 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
It's not very likely but in theory the pattern plugin or the error
filter might not be installed. These additional tests would catch
that and skip the test, although we don't do that in other tests yet ...
requires nbdkit pattern --version
requires nbdkit --filter=error null --version
(https://libguestfs.org/nbdkit-probing.1.html)
> +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? */
I would say no. nbdkit can use the retry or retry-request filter if
retrying is desirable, and other Unix commands (eg. cp) don't have
automatic retry behaviour by default.
> + 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;
> }
ACK series, thanks!
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
More information about the Libguestfs
mailing list