Re: [Libguestfs] [libnbd PATCH] copy: Fail nbdcopy if NBD read or write fails

[Adding some other distro maintainers for awareness. Note the patch is not complete and upstream yet] On Wed, Feb 02, 2022 at 07:50:55PM -0600, Eric Blake wrote: > FIXME: This is CVE-2022-XXXXX (still awaiting assignment of the CVE number).

> nbdcopy has a nasty bug when performing multi-threaded copies using > asynchronous nbd calls - it was blindly treating the completion of an > asynchronous command as successful, rather than checking the *error > parameter. This can result in the silent creation of a corrupted > image in two different ways: when a read fails, we blindly wrote > garbage to the destination; when a write fails, we did not flag that > the destination was not written. > > Since nbdcopy already calls exit() on a synchronous read or write > failure to a file, doing the same for an asynchronous op to an NBD > server is the simplest solution. A nicer solution, but more invasive > to write and thus not done here, might be to allow up to N retries of > the transaction (in case the read or write failure was transient), or > even having a mode where as much data is copied as possible (portions > of the copy that failed would be logged on stderr, and nbdcopy would > still fail with a non-zero exit status, but this would copy more than > just stopping at the first error). > > Note that since we rely on auto-retiring and do NOT call > nbd_aio_command_completed, our completion callbacks must always return > 1 (if they do not exit() first), even when acting on *error. As such, > there is no sane way to return an error to a manual caller of the > callback, and therefore we can drop dead code that exit()s if the > callback "fails". It is also worth documenting the contract on when > we must manually call the callback during the asynch_zero callback, so > that we do not leak or double-free the command. > > And since we are now exit()ing in the callback if *error is set, we > must be careful that we do not leak an unknown value of errno on paths > where we did not encounter a failure. > > Reported-by: Nir Soffer <nsoffer(a)redhat.com&gt; > Fixes: bc896eec4d ("copy: Implement multi-conn, multiple threads, multiple requests in flight.", v1.5.6) > Fixes: https://bugzilla.redhat.com/2046194 > --- > > Once the CVE is assigned, I will also update docs/libnbd-security.pod, > and backport this to affected stable branches. > > I'm not sure if I like passing &errno in the manual calls to > cb.callback(); better might be 'int dummy = 0; cb.callback(..., > &dummy)', but changing that could be a separate patch. > > I verified that the copy-nbd-error.sh test fails (in both the read and > write directions, by modifying the filter arguments) without this > patch. > > TODO | 1 + > copy/Makefile.am | 4 ++- > copy/copy-nbd-error.sh | 54 +++++++++++++++++++++++++++++++++++++ > copy/file-ops.c | 21 +++++---------- > copy/multi-thread-copying.c | 18 ++++++++++++- > copy/nbdcopy.h | 7 ++--- > copy/null-ops.c | 12 +++------ > 7 files changed, 88 insertions(+), 29 deletions(-) > create mode 100755 copy/copy-nbd-error.sh > > diff --git a/TODO b/TODO > index da157942..7c9c15e2 100644 > --- a/TODO > +++ b/TODO > @@ -33,6 +33,7 @@ nbdcopy: > - Better page cache usage, see nbdkit-file-plugin options > fadvise=sequential cache=none. > - Consider io_uring if there are performance bottlenecks. > + - Configurable retries in response to read or write failures. > > nbdfuse: > - If you write beyond the end of the virtual file, it returns EIO. > diff --git a/copy/Makefile.am b/copy/Makefile.am > index f2100853..85989798 100644 > --- a/copy/Makefile.am > +++ b/copy/Makefile.am > @@ -1,5 +1,5 @@ > # nbd client library in userspace > -# Copyright (C) 2020 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 > @@ -33,6 +33,7 @@ EXTRA_DIST = \ > copy-nbd-to-small-nbd-error.sh \ > copy-nbd-to-sparse-file.sh \ > copy-nbd-to-stdout.sh \ > + copy-nbd-error.sh \ > copy-progress-bar.sh \ > copy-sparse.sh \ > copy-sparse-allocated.sh \ > @@ -124,6 +125,7 @@ TESTS += \ > copy-stdin-to-nbd.sh \ > copy-stdin-to-null.sh \ > copy-nbd-to-stdout.sh \ > + copy-nbd-error.sh \ > copy-progress-bar.sh \ > copy-sparse.sh \ > copy-sparse-allocated.sh \ > diff --git a/copy/copy-nbd-error.sh b/copy/copy-nbd-error.sh > new file mode 100755 > index 00000000..50723195 > --- /dev/null > +++ b/copy/copy-nbd-error.sh > @@ -0,0 +1,54 @@ > +#!/usr/bin/env bash > +# nbd client library in userspace > +# Copyright (C) 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 > +# License as published by the Free Software Foundation; either > +# version 2 of the License, or (at your option) any later version. > +# > +# This library is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > +# Lesser General Public License for more details. > +# > +# You should have received a copy of the GNU Lesser General Public > +# License along with this library; if not, write to the Free Software > +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > + > +. ../tests/functions.sh > + > +set -e > +set -x > + > +requires nbdkit --exit-with-parent --version > + > +pidfile=copy-nbd-error.pid > +sock=$(mktemp -u /tmp/libnbd-test-copy.XXXXXX) > +cleanup_fn rm -f $pidfile $sock > + > +# Run an nbdkit server that randomly fails. > +nbdkit --exit-with-parent -f -v -P $pidfile -U $sock \ > + --filter=error --filter=noextents \ > + memory size=5M error-pread-rate=0.5 error-pwrite-rate=0.5 & > +# Wait for the pidfile to appear. > +for i in {1..60}; do > + if test -f $pidfile; then > + break > + fi > + sleep 1 > +done > +if ! test -f $pidfile; then > + echo "$0: nbdkit did not start up" > + exit 1 > +fi > + > +fail=0 > + > +# Failure to read should be fatal > +$VG nbdcopy -- "nbd+unix:///?socket=$sock" null: && fail=1 > + > +# Failure to write should be fatal > +$VG nbdcopy -- [ nbdkit --exit-with-parent -v pattern 5M ] "nbd+unix:///?socket=$sock" && fail=1 > + > +exit $fail > diff --git a/copy/file-ops.c b/copy/file-ops.c > index 84704341..eb30f924 100644 > --- a/copy/file-ops.c > +++ b/copy/file-ops.c > @@ -1,5 +1,5 @@ > /* NBD client library in userspace. > - * Copyright (C) 2020 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 > @@ -589,11 +589,8 @@ file_asynch_read (struct rw *rw, > { > file_synch_read (rw, slice_ptr (command->slice), > command->slice.len, command->offset); > - errno = 0; > - if (cb.callback (cb.user_data, &errno) == -1) { > - perror (rw->name); > - exit (EXIT_FAILURE); > - } > + errno = 0; /* safe, since file_synch_read exits on error */ > + cb.callback (cb.user_data, &errno); > } > > static void > @@ -603,11 +600,8 @@ file_asynch_write (struct rw *rw, > { > file_synch_write (rw, slice_ptr (command->slice), > command->slice.len, command->offset); > - errno = 0; > - if (cb.callback (cb.user_data, &errno) == -1) { > - perror (rw->name); > - exit (EXIT_FAILURE); > - } > + errno = 0; /* safe, since file_synch_write exits on error */ > + cb.callback (cb.user_data, &errno); > } > > static bool > @@ -617,10 +611,7 @@ file_asynch_zero (struct rw *rw, struct command *command, > if (!file_synch_zero (rw, command->offset, command->slice.len, allocate)) > return false; > errno = 0; > - if (cb.callback (cb.user_data, &errno) == -1) { > - perror (rw->name); > - exit (EXIT_FAILURE); > - } > + cb.callback (cb.user_data, &errno); > return true; > } > > diff --git a/copy/multi-thread-copying.c b/copy/multi-thread-copying.c > index b17ca598..c578c4bc 100644 > --- a/copy/multi-thread-copying.c > +++ b/copy/multi-thread-copying.c > @@ -1,5 +1,5 @@ > /* NBD client library in userspace. > - * Copyright (C) 2020 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 > @@ -376,6 +376,13 @@ finished_read (void *vp, int *error) > { > struct command *command = vp; > > + /* XXX - is it worth retrying a failed command? */ > + if (*error) { > + errno = *error; > + perror("read"); > + exit (EXIT_FAILURE); > + } > + > if (allocated || sparse_size == 0) { > /* If sparseness detection (see below) is turned off then we write > * the whole command. > @@ -475,6 +482,7 @@ finished_read (void *vp, int *error) > /* Free the original command since it has been split into > * subcommands and the original is no longer needed. > */ > + errno = 0; > free_command (command, &errno); > } > > @@ -537,6 +545,7 @@ fill_dst_range_with_zeroes (struct command *command) > free (data); > > free_and_return: > + errno = 0; > free_command (command, &errno); > } > > @@ -546,6 +555,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) { > + errno = *error; > + perror("write"); > + 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 a38666d6..924eaf1e 100644 > --- a/copy/null-ops.c > +++ b/copy/null-ops.c > @@ -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 > @@ -118,10 +118,7 @@ null_asynch_write (struct rw *rw, > nbd_completion_callback cb) > { > errno = 0; > - if (cb.callback (cb.user_data, &errno) == -1) { > - perror (rw->name); > - exit (EXIT_FAILURE); > - } > + cb.callback (cb.user_data, &errno); > } > > static bool > @@ -129,10 +126,7 @@ null_asynch_zero (struct rw *rw, struct command *command, > nbd_completion_callback cb, bool allocate) > { > errno = 0; > - if (cb.callback (cb.user_data, &errno) == -1) { > - perror (rw->name); > - exit (EXIT_FAILURE); > - } > + cb.callback (cb.user_data, &errno); > return true; > } > > -- > 2.34.1 > > _______________________________________________ > Libguestfs mailing list > Libguestfs(a)redhat.com > https://listman.redhat.com/mailman/listinfo/libguestfs -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org