Blame SOURCES/0007-copy-CVE-2022-0485-Fail-nbdcopy-if-NBD-read-or-write.patch

a81d13
From 1b0b732e6a9b4979fccf6a09eb6704264edf675d Mon Sep 17 00:00:00 2001
a81d13
From: Eric Blake <eblake@redhat.com>
a81d13
Date: Thu, 3 Feb 2022 14:25:58 -0600
a81d13
Subject: [PATCH] copy: CVE-2022-0485: Fail nbdcopy if NBD read or write fails
a81d13
a81d13
nbdcopy has a nasty bug when performing multi-threaded copies using
a81d13
asynchronous nbd calls - it was blindly treating the completion of an
a81d13
asynchronous command as successful, rather than checking the *error
a81d13
parameter.  This can result in the silent creation of a corrupted
a81d13
image in two different ways: when a read fails, we blindly wrote
a81d13
garbage to the destination; when a write fails, we did not flag that
a81d13
the destination was not written.
a81d13
a81d13
Since nbdcopy already calls exit() on a synchronous read or write
a81d13
failure to a file, doing the same for an asynchronous op to an NBD
a81d13
server is the simplest solution.  A nicer solution, but more invasive
a81d13
to code and thus not done here, might be to allow up to N retries of
a81d13
the transaction (in case the read or write failure was transient), or
a81d13
even having a mode where as much data is copied as possible (portions
a81d13
of the copy that failed would be logged on stderr, and nbdcopy would
a81d13
still fail with a non-zero exit status, but this would copy more than
a81d13
just stopping at the first error, as can be done with rsync or
a81d13
ddrescue).
a81d13
a81d13
Note that since we rely on auto-retiring and do NOT call
a81d13
nbd_aio_command_completed, our completion callbacks must always return
a81d13
1 (if they do not exit() first), even when acting on *error, so as not
a81d13
leave the command allocated until nbd_close.  As such, there is no
a81d13
sane way to return an error to a manual caller of the callback, and
a81d13
therefore we can drop dead code that calls perror() and exit() if the
a81d13
callback "failed".  It is also worth documenting the contract on when
a81d13
we must manually call the callback during the asynch_zero callback, so
a81d13
that we do not leak or double-free the command; thankfully, all the
a81d13
existing code paths were correct.
a81d13
a81d13
The added testsuite script demonstrates several scenarios, some of
a81d13
which fail without the rest of this patch in place, and others which
a81d13
showcase ways in which sparse images can bypass errors.
a81d13
a81d13
Once backports are complete, a followup patch on the main branch will
a81d13
edit docs/libnbd-security.pod with the mailing list announcement of
a81d13
the stable branch commit ids and release versions that incorporate
a81d13
this fix.
a81d13
a81d13
Reported-by: Nir Soffer <nsoffer@redhat.com>
a81d13
Fixes: bc896eec4d ("copy: Implement multi-conn, multiple threads, multiple requests in flight.", v1.5.6)
a81d13
Fixes: https://bugzilla.redhat.com/2046194
a81d13
Message-Id: <20220203202558.203013-6-eblake@redhat.com>
a81d13
Acked-by: Richard W.M. Jones <rjones@redhat.com>
a81d13
Acked-by: Nir Soffer <nsoffer@redhat.com>
a81d13
[eblake: fix error message per Nir, tweak requires lines in unit test per Rich]
a81d13
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
a81d13
a81d13
(cherry picked from commit 8d444b41d09a700c7ee6f9182a649f3f2d325abb)
a81d13
Conflicts:
a81d13
	copy/nbdcopy.h - copyright context
a81d13
	copy/null-ops.c - no backport of 0b16205e "copy: Implement "null:"
a81d13
 destination."
a81d13
        copy/copy-nbd-error.sh - no backport of d5f65e56 ("copy: Do not use
a81d13
 trim for zeroing"), so one test needed an additional error-trim-rate;
a81d13
 no backport of 4ff9e62d (copy: Add --request-size option") and friends, so
a81d13
 this version uses larger transactions, so change error rate of 0.5 to 1;
a81d13
 no backport of 0b16205e "copy: Implement "null:" destination.", so use
a81d13
 nbdkit null instead
a81d13
Note that while the use of NBD_CMD_TRIM can create data corruption, it is
a81d13
not as severe as what this patch fixes, since trim corruption will only
a81d13
expose what had previously been on the disk, compared to this patch fixing
a81d13
a potential leak of nbdcopy heap contents into the destination.
a81d13
(cherry picked from commit 6c8f2f859926b82094fb5e85c446ea099700fa10)
a81d13
---
a81d13
 TODO                        |  1 +
a81d13
 copy/Makefile.am            |  4 +-
a81d13
 copy/copy-nbd-error.sh      | 81 +++++++++++++++++++++++++++++++++++++
a81d13
 copy/file-ops.c             | 17 +++-----
a81d13
 copy/multi-thread-copying.c | 13 ++++++
a81d13
 copy/nbdcopy.h              |  7 ++--
a81d13
 6 files changed, 107 insertions(+), 16 deletions(-)
a81d13
 create mode 100755 copy/copy-nbd-error.sh
a81d13
a81d13
diff --git a/TODO b/TODO
a81d13
index 510c219..19c21d4 100644
a81d13
--- a/TODO
a81d13
+++ b/TODO
a81d13
@@ -35,6 +35,7 @@ nbdcopy:
a81d13
  - Better page cache usage, see nbdkit-file-plugin options
a81d13
    fadvise=sequential cache=none.
a81d13
  - Consider io_uring if there are performance bottlenecks.
a81d13
+ - Configurable retries in response to read or write failures.
a81d13
 
a81d13
 nbdfuse:
a81d13
  - If you write beyond the end of the virtual file, it returns EIO.
a81d13
diff --git a/copy/Makefile.am b/copy/Makefile.am
a81d13
index d318388..3406cd8 100644
a81d13
--- a/copy/Makefile.am
a81d13
+++ b/copy/Makefile.am
a81d13
@@ -1,5 +1,5 @@
a81d13
 # nbd client library in userspace
a81d13
-# Copyright (C) 2020 Red Hat Inc.
a81d13
+# Copyright (C) 2020-2022 Red Hat Inc.
a81d13
 #
a81d13
 # This library is free software; you can redistribute it and/or
a81d13
 # modify it under the terms of the GNU Lesser General Public
a81d13
@@ -30,6 +30,7 @@ EXTRA_DIST = \
a81d13
 	copy-nbd-to-small-nbd-error.sh \
a81d13
 	copy-nbd-to-sparse-file.sh \
a81d13
 	copy-nbd-to-stdout.sh \
a81d13
+	copy-nbd-error.sh \
a81d13
 	copy-progress-bar.sh \
a81d13
 	copy-sparse.sh \
a81d13
 	copy-sparse-allocated.sh \
a81d13
@@ -105,6 +106,7 @@ TESTS += \
a81d13
 	copy-nbd-to-sparse-file.sh \
a81d13
 	copy-stdin-to-nbd.sh \
a81d13
 	copy-nbd-to-stdout.sh \
a81d13
+	copy-nbd-error.sh \
a81d13
 	copy-progress-bar.sh \
a81d13
 	copy-sparse.sh \
a81d13
 	copy-sparse-allocated.sh \
a81d13
diff --git a/copy/copy-nbd-error.sh b/copy/copy-nbd-error.sh
a81d13
new file mode 100755
a81d13
index 0000000..bba71db
a81d13
--- /dev/null
a81d13
+++ b/copy/copy-nbd-error.sh
a81d13
@@ -0,0 +1,81 @@
a81d13
+#!/usr/bin/env bash
a81d13
+# nbd client library in userspace
a81d13
+# Copyright (C) 2022 Red Hat Inc.
a81d13
+#
a81d13
+# This library is free software; you can redistribute it and/or
a81d13
+# modify it under the terms of the GNU Lesser General Public
a81d13
+# License as published by the Free Software Foundation; either
a81d13
+# version 2 of the License, or (at your option) any later version.
a81d13
+#
a81d13
+# This library is distributed in the hope that it will be useful,
a81d13
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
a81d13
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
a81d13
+# Lesser General Public License for more details.
a81d13
+#
a81d13
+# You should have received a copy of the GNU Lesser General Public
a81d13
+# License along with this library; if not, write to the Free Software
a81d13
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
a81d13
+
a81d13
+# Tests several scenarios of handling NBD server errors
a81d13
+# Serves as a regression test for the CVE-2022-0485 fix.
a81d13
+
a81d13
+. ../tests/functions.sh
a81d13
+
a81d13
+set -e
a81d13
+set -x
a81d13
+
a81d13
+requires nbdkit --exit-with-parent --version
a81d13
+requires nbdkit --filter=noextents null --version
a81d13
+requires nbdkit --filter=error pattern --version
a81d13
+requires nbdkit --filter=nozero memory --version
a81d13
+
a81d13
+fail=0
a81d13
+
a81d13
+# Failure to get block status should not be fatal, but merely downgrade to
a81d13
+# reading the entire image as if data
a81d13
+echo "Testing extents failures on source"
a81d13
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error pattern 5M \
a81d13
+    error-extents-rate=1 ] [ nbdkit --exit-with-parent -v null 5M ] || fail=1
a81d13
+
a81d13
+# Failure to read should be fatal
a81d13
+echo "Testing read failures on non-sparse source"
a81d13
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error pattern 5M \
a81d13
+    error-pread-rate=1 ] [ nbdkit --exit-with-parent -v null 5M ] && fail=1
a81d13
+
a81d13
+# However, reliable block status on a sparse image can avoid the need to read
a81d13
+echo "Testing read failures on sparse source"
a81d13
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error null 5M \
a81d13
+    error-pread-rate=1 ] [ nbdkit --exit-with-parent -v null 5M ] || fail=1
a81d13
+
a81d13
+# Failure to write data should be fatal
a81d13
+echo "Testing write data failures on arbitrary destination"
a81d13
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v pattern 5M ] \
a81d13
+    [ nbdkit --exit-with-parent -v --filter=error --filter=noextents \
a81d13
+        memory 5M error-pwrite-rate=1 ] && fail=1
a81d13
+
a81d13
+# However, writing zeroes can bypass the need for normal writes
a81d13
+echo "Testing write data failures from sparse source"
a81d13
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v null 5M ] \
a81d13
+    [ nbdkit --exit-with-parent -v --filter=error --filter=noextents \
a81d13
+        memory 5M error-pwrite-rate=1 ] || fail=1
a81d13
+
a81d13
+# Failure to write zeroes should be fatal
a81d13
+echo "Testing write zero failures on arbitrary destination"
a81d13
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v null 5M ] \
a81d13
+    [ nbdkit --exit-with-parent -v --filter=error memory 5M \
a81d13
+        error-trim-rate=1 error-zero-rate=1 ] && fail=1
a81d13
+
a81d13
+# However, assuming/learning destination is zero can skip need to write
a81d13
+echo "Testing write failures on pre-zeroed destination"
a81d13
+$VG nbdcopy --destination-is-zero -- \
a81d13
+    [ nbdkit --exit-with-parent -v null 5M ] \
a81d13
+    [ nbdkit --exit-with-parent -v --filter=error memory 5M \
a81d13
+        error-pwrite-rate=1 error-zero-rate=1 ] || fail=1
a81d13
+
a81d13
+# Likewise, when write zero is not advertised, fallback to normal write works
a81d13
+echo "Testing write zeroes to destination without zero support"
a81d13
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v null 5M ] \
a81d13
+    [ nbdkit --exit-with-parent -v --filter=nozero --filter=error memory 5M \
a81d13
+        error-zero-rate=1 ] || fail=1
a81d13
+
a81d13
+exit $fail
a81d13
diff --git a/copy/file-ops.c b/copy/file-ops.c
a81d13
index cc312b4..b19af04 100644
a81d13
--- a/copy/file-ops.c
a81d13
+++ b/copy/file-ops.c
a81d13
@@ -162,10 +162,8 @@ file_asynch_read (struct rw *rw,
a81d13
 
a81d13
   file_synch_read (rw, slice_ptr (command->slice),
a81d13
                    command->slice.len, command->offset);
a81d13
-  if (cb.callback (cb.user_data, &dummy) == -1) {
a81d13
-    perror (rw->name);
a81d13
-    exit (EXIT_FAILURE);
a81d13
-  }
a81d13
+  /* file_synch_read called exit() on error */
a81d13
+  cb.callback (cb.user_data, &dummy);
a81d13
 }
a81d13
 
a81d13
 static void
a81d13
@@ -177,10 +175,8 @@ file_asynch_write (struct rw *rw,
a81d13
 
a81d13
   file_synch_write (rw, slice_ptr (command->slice),
a81d13
                     command->slice.len, command->offset);
a81d13
-  if (cb.callback (cb.user_data, &dummy) == -1) {
a81d13
-    perror (rw->name);
a81d13
-    exit (EXIT_FAILURE);
a81d13
-  }
a81d13
+  /* file_synch_write called exit() on error */
a81d13
+  cb.callback (cb.user_data, &dummy);
a81d13
 }
a81d13
 
a81d13
 static bool
a81d13
@@ -206,10 +202,7 @@ file_asynch_zero (struct rw *rw, struct command *command,
a81d13
 
a81d13
   if (!file_synch_zero (rw, command->offset, command->slice.len))
a81d13
     return false;
a81d13
-  if (cb.callback (cb.user_data, &dummy) == -1) {
a81d13
-    perror (rw->name);
a81d13
-    exit (EXIT_FAILURE);
a81d13
-  }
a81d13
+  cb.callback (cb.user_data, &dummy);
a81d13
   return true;
a81d13
 }
a81d13
 
a81d13
diff --git a/copy/multi-thread-copying.c b/copy/multi-thread-copying.c
a81d13
index 2593ff7..28749ae 100644
a81d13
--- a/copy/multi-thread-copying.c
a81d13
+++ b/copy/multi-thread-copying.c
a81d13
@@ -28,6 +28,7 @@
a81d13
 #include <errno.h>
a81d13
 #include <assert.h>
a81d13
 #include <sys/stat.h>
a81d13
+#include <inttypes.h>
a81d13
 
a81d13
 #include <pthread.h>
a81d13
 
a81d13
@@ -374,6 +375,12 @@ finished_read (void *vp, int *error)
a81d13
 {
a81d13
   struct command *command = vp;
a81d13
 
a81d13
+  if (*error) {
a81d13
+    fprintf (stderr, "read at offset %" PRId64 " failed: %s\n",
a81d13
+             command->offset, strerror (*error));
a81d13
+    exit (EXIT_FAILURE);
a81d13
+  }
a81d13
+
a81d13
   if (allocated || sparse_size == 0) {
a81d13
     /* If sparseness detection (see below) is turned off then we write
a81d13
      * the whole command.
a81d13
@@ -552,6 +559,12 @@ free_command (void *vp, int *error)
a81d13
   struct command *command = vp;
a81d13
   struct buffer *buffer = command->slice.buffer;
a81d13
 
a81d13
+  if (*error) {
a81d13
+    fprintf (stderr, "write at offset %" PRId64 " failed: %s\n",
a81d13
+             command->offset, strerror (*error));
a81d13
+    exit (EXIT_FAILURE);
a81d13
+  }
a81d13
+
a81d13
   if (buffer != NULL) {
a81d13
     if (--buffer->refs == 0) {
a81d13
       free (buffer->data);
a81d13
diff --git a/copy/nbdcopy.h b/copy/nbdcopy.h
a81d13
index 3dcc6df..9626a52 100644
a81d13
--- a/copy/nbdcopy.h
a81d13
+++ b/copy/nbdcopy.h
a81d13
@@ -1,5 +1,5 @@
a81d13
 /* NBD client library in userspace.
a81d13
- * Copyright (C) 2020 Red Hat Inc.
a81d13
+ * Copyright (C) 2020-2022 Red Hat Inc.
a81d13
  *
a81d13
  * This library is free software; you can redistribute it and/or
a81d13
  * modify it under the terms of the GNU Lesser General Public
a81d13
@@ -134,7 +134,8 @@ struct rw_ops {
a81d13
   bool (*synch_zero) (struct rw *rw, uint64_t offset, uint64_t count);
a81d13
 
a81d13
   /* Asynchronous I/O operations.  These start the operation and call
a81d13
-   * 'cb' on completion.
a81d13
+   * 'cb' on completion.  'cb' will return 1, for auto-retiring with
a81d13
+   * asynchronous libnbd calls.
a81d13
    *
a81d13
    * The file_ops versions are actually implemented synchronously, but
a81d13
    * still call 'cb'.
a81d13
@@ -156,7 +157,7 @@ struct rw_ops {
a81d13
                        nbd_completion_callback cb);
a81d13
 
a81d13
   /* Asynchronously zero.  command->slice.buffer is not used.  If not possible,
a81d13
-   * returns false.
a81d13
+   * returns false.  'cb' must be called only if returning true.
a81d13
    */
a81d13
   bool (*asynch_zero) (struct rw *rw, struct command *command,
a81d13
                        nbd_completion_callback cb);
a81d13
-- 
a81d13
2.31.1
a81d13