From 1b0b732e6a9b4979fccf6a09eb6704264edf675d Mon Sep 17 00:00:00 2001

From: Eric Blake <eblake@redhat.com>

Date: Thu, 3 Feb 2022 14:25:58 -0600

Subject: [PATCH] copy: CVE-2022-0485: Fail nbdcopy if NBD read or write fails

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 code 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, as can be done with rsync or

ddrescue).

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, so as not

leave the command allocated until nbd_close.  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 calls perror() and exit() if the

callback "failed".  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; thankfully, all the

existing code paths were correct.

The added testsuite script demonstrates several scenarios, some of

which fail without the rest of this patch in place, and others which

showcase ways in which sparse images can bypass errors.

Once backports are complete, a followup patch on the main branch will

edit docs/libnbd-security.pod with the mailing list announcement of

the stable branch commit ids and release versions that incorporate

this fix.

Reported-by: Nir Soffer <nsoffer@redhat.com>

Fixes: bc896eec4d ("copy: Implement multi-conn, multiple threads, multiple requests in flight.", v1.5.6)

Fixes: https://bugzilla.redhat.com/2046194

Message-Id: <20220203202558.203013-6-eblake@redhat.com>

Acked-by: Richard W.M. Jones <rjones@redhat.com>

Acked-by: Nir Soffer <nsoffer@redhat.com>

[eblake: fix error message per Nir, tweak requires lines in unit test per Rich]

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

(cherry picked from commit 8d444b41d09a700c7ee6f9182a649f3f2d325abb)

Conflicts:

	copy/nbdcopy.h - copyright context

	copy/null-ops.c - no backport of 0b16205e "copy: Implement "null:"

 destination."

        copy/copy-nbd-error.sh - no backport of d5f65e56 ("copy: Do not use

 trim for zeroing"), so one test needed an additional error-trim-rate;

 no backport of 4ff9e62d (copy: Add --request-size option") and friends, so

 this version uses larger transactions, so change error rate of 0.5 to 1;

 no backport of 0b16205e "copy: Implement "null:" destination.", so use

 nbdkit null instead

Note that while the use of NBD_CMD_TRIM can create data corruption, it is

not as severe as what this patch fixes, since trim corruption will only

expose what had previously been on the disk, compared to this patch fixing

a potential leak of nbdcopy heap contents into the destination.

(cherry picked from commit 6c8f2f859926b82094fb5e85c446ea099700fa10)

---

 TODO                        |  1 +

 copy/Makefile.am            |  4 +-

 copy/copy-nbd-error.sh      | 81 +++++++++++++++++++++++++++++++++++++

 copy/file-ops.c             | 17 +++-----

 copy/multi-thread-copying.c | 13 ++++++

 copy/nbdcopy.h              |  7 ++--

 6 files changed, 107 insertions(+), 16 deletions(-)

 create mode 100755 copy/copy-nbd-error.sh

diff --git a/TODO b/TODO

index 510c219..19c21d4 100644

--- a/TODO

+++ b/TODO

@@ -35,6 +35,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 d318388..3406cd8 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

@@ -30,6 +30,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 \

@@ -105,6 +106,7 @@ TESTS += \

 	copy-nbd-to-sparse-file.sh \

 	copy-stdin-to-nbd.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 0000000..bba71db

--- /dev/null

+++ b/copy/copy-nbd-error.sh

@@ -0,0 +1,81 @@

+#!/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 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

+requires nbdkit --filter=noextents null --version

+requires nbdkit --filter=error pattern --version

+requires nbdkit --filter=nozero memory --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 ] [ nbdkit --exit-with-parent -v null 5M ] || 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=1 ] [ nbdkit --exit-with-parent -v null 5M ] && 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 ] [ nbdkit --exit-with-parent -v null 5M ] || 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=1 ] && 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-trim-rate=1 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 cc312b4..b19af04 100644

--- a/copy/file-ops.c

+++ b/copy/file-ops.c

@@ -162,10 +162,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

@@ -177,10 +175,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

@@ -206,10 +202,7 @@ file_asynch_zero (struct rw *rw, struct command *command,

   if (!file_synch_zero (rw, command->offset, command->slice.len))

     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 2593ff7..28749ae 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>

@@ -374,6 +375,12 @@ finished_read (void *vp, int *error)

 {

   struct command *command = vp;

+  if (*error) {

+    fprintf (stderr, "read at offset %" PRId64 " 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.

@@ -552,6 +559,12 @@ free_command (void *vp, int *error)

   struct command *command = vp;

   struct buffer *buffer = command->slice.buffer;

+  if (*error) {

+    fprintf (stderr, "write at offset %" PRId64 " 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 3dcc6df..9626a52 100644

--- a/copy/nbdcopy.h

+++ b/copy/nbdcopy.h

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

@@ -134,7 +134,8 @@ struct rw_ops {

   bool (*synch_zero) (struct rw *rw, uint64_t offset, uint64_t count);

   /* 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'.

@@ -156,7 +157,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);

-- 

2.31.1