[Libguestfs] [libnbd PATCH] copy: Fail nbdcopy if NBD read or write fails
Richard W.M. Jones
rjones at redhat.com
Thu Feb 3 17:47:04 UTC 2022
On Thu, Feb 03, 2022 at 10:41:10AM +0000, Richard W.M. Jones wrote:
> [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).
The CVE has now been assigned: CVE-2022-0485
Rich.
> > 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 at redhat.com>
> > 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 at 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
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
More information about the Libguestfs
mailing list