Libguestfs - Feb 2022 - [libnbd PATCH] copy: Fail nbdcopy if NBD read or write fails

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 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

[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 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

On 02/03/22 02:50, 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).
That seems more like a libnbd-backed rsync :)
> 
> 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 agree, a patch should likely be prepended for updating that; the
callback may not expect that *error alias errno (IIUC).
> 
> 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 \
TABs vs spaces
>  	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 \
TABs vs spaces
>  	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"
redirect to stderr perhaps?
> +    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
What's the reason to test writing if the read test fails first?
> +
> +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;
>  }
> 
Looks OK to me, but should be definitely reviewed by another reviewer.

Thanks
Laszlo

On Thu, Feb 3, 2022 at 3:51 AM Eric Blake <eblake at redhat.com>
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.
Writing zeroes is third failure case
>
> 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).
I think failing on errors is good enough, and we need minimal change
that will be easy to backport.
>
> 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.
That's a nice side effect of the fix.
> 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 agree.
> 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 &
What about write_zeroes errors?
> +# 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);
Much nicer - should we assert that the callback returns 1?

file-ops does not care about the return value but it will help
to detect errors in code using nbd. We cannot assert about this
in nbd-ops since the call returns to libnbd.
>  }
>
>  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);
> +  }
Setting errno to use perror is clever, but I think in future we would
like to exit with a more detailed message, like:

    nbdcopy: read failed at offset 44040192: Input output error

So we would use fprintf and strerror, and setting errno would be needed
although it can be used with %m (is it portable?).

For now I think this is fine, we want a minimal change for easy backport.
> +
>    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);
Using free_command as a callback is ugly, but good enough for now.
>    }
>
> @@ -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
Would be good to test also writing zeroes, but otherwise the fix is complete.

Reviewed-by: Nir Soffer <nsoffer at redhat.com>

On 02/03/22 02:50, 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.
BTW: why is this a CVE? How is this exploitable for an attacker?

Thanks
Laszlo