[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@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@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@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 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@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@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
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@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 03, 2022 at 11:52:43AM +0100, Laszlo Ersek wrote:
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 :)
I was thinking more like ddrescue, but the idea is the same ;)
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).
Okay, I will do that as a preliminary patch.
+++ 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
Huh; I wonder if this is yet another fallout from Nir's addition of .editorconfig. Makefile.am is odd - there are places where tabs are important, but not the entire file; but I definitely agree that local consistency is important even in the places where tab vs. space doesn't affect automake/make semantics.
+# 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?
Copy-paste from other existing tests. If I clean it up, it's worth doing globally as a separate patch. Ultimately, as long as it ends up in the .log for debugging test failures, I don't care if it got there through stdout or stderr.
+ 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
Hmm, I should probably use \-newline to split this long line.
What's the reason to test writing if the read test fails first?
When running the testsuite, it's nice to uncover as many cases as possible in a single run, rather than patching one early failure just to encounter another failure on the next test run that was previously skipped because of the first failure. One way to do this is to write two separate tests (one that tests failed reads from NBD as source, a second that tests failed writes to NBD as destination), but writing two nearly-identical .sh files is more boilerplate overhead than just jamming related tests into the same .sh script.
Looks OK to me, but should be definitely reviewed by another reviewer.
Looks like Rich has given ACK as well. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 02/03/22 14:49, Eric Blake wrote:
On Thu, Feb 03, 2022 at 11:52:43AM +0100, Laszlo Ersek wrote:
What's the reason to test writing if the read test fails first?
When running the testsuite, it's nice to uncover as many cases as possible in a single run, rather than patching one early failure just to encounter another failure on the next test run that was previously skipped because of the first failure.
This is a great idea, and I wish it worked consistently *between* separate test cases too. I distinctly remember connect-tcp6 failing for me (earlier), and myself dismissing it, and assuming everything else was OK. Then, in the RHEL9 Brew environment, where connect-tcp6 was specifically disabled, I was hit by the actual test failures :/ (And there was no easy way to skip connect-tcp6.)
One way to do this is to write two separate tests (one that tests failed reads from NBD as source, a second that tests failed writes to NBD as destination), but writing two nearly-identical .sh files is more boilerplate overhead than just jamming related tests into the same .sh script.
Right, but this is exactly what I wish worked -- but IME doesn't always work.
Looks OK to me, but should be definitely reviewed by another reviewer.
Looks like Rich has given ACK as well.
Thanks! Laszlo
On Fri, Feb 04, 2022 at 09:18:07AM +0100, Laszlo Ersek wrote:
(And there was no easy way to skip connect-tcp6.)
Wwe use this in libnbd.spec: https://src.fedoraproject.org/rpms/libnbd/blob/rawhide/f/libnbd.spec#_214 In libguestfs we have a whole infrastructure for skipping tests by setting $SKIP_* variables in localenv. https://libguestfs.org/guestfs-building.1.html#local-files https://github.com/libguestfs/libguestfs/blob/0553f90462f456b1fe7100346aa1a4... For libnbd/nbdkit it might be possible to automate it in the same way with tests/functions.sh.in + $0, but it's not implemented at the moment. Of course it'd be better if the tests didn't fail :-) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On 02/08/22 14:43, Richard W.M. Jones wrote:
On Fri, Feb 04, 2022 at 09:18:07AM +0100, Laszlo Ersek wrote:
(And there was no easy way to skip connect-tcp6.)
Wwe use this in libnbd.spec:
https://src.fedoraproject.org/rpms/libnbd/blob/rawhide/f/libnbd.spec#_214
Yes (I know what the link points to, without looking :)); that's why one of my earlier patches (or patch sets) exposed the real issue in it only in Brew! :)
In libguestfs we have a whole infrastructure for skipping tests by setting $SKIP_* variables in localenv.
Ah, this is very nice. For the various v2v projects, I've created a bunch of env setup scripts, and those scripts consider the inter-project dependencies (based on the graph you gave me earlier). So they more or less "chain" the various "run" scripts in the project roots. One problem with the ignored local* files in the git worktrees could be that "git clean -ffdx" probably removes them. I have a $HOME/bin/scripts dir on my $PATH, where I keep such scripts.
https://github.com/libguestfs/libguestfs/blob/0553f90462f456b1fe7100346aa1a4...
For libnbd/nbdkit it might be possible to automate it in the same way with tests/functions.sh.in + $0, but it's not implemented at the moment. Of course it'd be better if the tests didn't fail :-)
Yes, absolutely. Thanks! Laszlo
On Thu, Feb 3, 2022 at 3:51 AM Eric Blake <eblake@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@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@redhat.com>
On Thu, Feb 03, 2022 at 02:46:27PM +0200, Nir Soffer wrote:
On Thu, Feb 3, 2022 at 3:51 AM Eric Blake <eblake@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
It was the same callback shared between nbd_aio_pwrite and nbd_aio_zero, but you are right that...
+ +# 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?
...I did not have test coverage of writing zeroes being yet another way to trigger the problem. I may further tweak the testsuite to use 'nbdcopy -- [ nbdkit ... ] [ nbdkit ...]' tuned to each failure scenario, rather than creating a pid file for a long-running process. One of the things I ran into when first testing is that a mere error-rate=0.5 is too broad (it can fails the nbd_block_status calls, which were already exiting nbdcopy), and I also had to inject --filter=noextents so that reads would not see that the source was entirely sparse (because then it skips the nbd_aio_pread).
@@ -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?
I'm not sure that the assert would buy us much; I already added the documentation contract in nbdcopy.h, and really, the only time we would even have the bug of not returning 1 would be when *error is not 0 on entry, but the file plugin never tickles that case for the assertion to detect us intrducing such a bug into the callback.
@@ -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
Good idea - we DO have the offset available as part of *command, so I'll rework it into the error message for v2.
So we would use fprintf and strerror, and setting errno would be needed although it can be used with %m (is it portable?).
%m is not portable in *printf(3). It IS portable in syslog(3), which is why glibc printf(3) supports %m as well; and in nbdkit, we use a printf wrapper on non-glibc to give unconditional %m support to nbdkit plugins. But we do not have that printf wrapper here. Using strerror() instead of perror() is doable in applications (we shouldn't do it directly in libnbd since strerror() has multi-threaded implications and we don't control what the application may be doing in other threads, but in nbdcopy we control the entire application).
For now I think this is fine, we want a minimal change for easy backport.
Yep, and since I'm the one that will be doing the backports, I'm aware of the problems of making the patch too complex ;)
+ 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.
Is your complaint that in some places we call free_command() directly, in other places, we rely on libnbd to call it as a callback? Yeah, it can get confusing, and it took me a while to convince myself that we call it exactly once on all call paths, particularly when asynch_zero returns true vs. false.
Would be good to test also writing zeroes, but otherwise the fix is complete.
Given that I will be revising the patch to add in a CVE number anyways, I'll improve the test for v2.
Reviewed-by: Nir Soffer <nsoffer@redhat.com>
Thanks. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 02/03/22 15:12, Eric Blake wrote:
On Thu, Feb 03, 2022 at 02:46:27PM +0200, Nir Soffer wrote:
So we would use fprintf and strerror, and setting errno would be needed although it can be used with %m (is it portable?).
%m is not portable in *printf(3). It IS portable in syslog(3), which is why glibc printf(3) supports %m as well; and in nbdkit, we use a printf wrapper on non-glibc to give unconditional %m support to nbdkit plugins. But we do not have that printf wrapper here. Using strerror() instead of perror() is doable in applications (we shouldn't do it directly in libnbd since strerror() has multi-threaded implications and we don't control what the application may be doing in other threads, but in nbdcopy we control the entire application).
Side topic: what do we think about strerror_r()? ... Hmmm I recall something... Yep: we now have guestfs_int_strerror() in the libguestfs-common project at least. (We cannot just copy it over I think, due to licence differences; however, with Rich having authored commit 9ec7cd8e23b5 ("utils: Fix usage of strerror_r", 2021-12-09), Red Hat seems to "own the copyright on that function" (if this statement makes any sense), so we could "relicense" it.) Thanks, Laszlo
For now I think this is fine, we want a minimal change for easy backport.
Yep, and since I'm the one that will be doing the backports, I'm aware of the problems of making the patch too complex ;)
+ 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.
Is your complaint that in some places we call free_command() directly, in other places, we rely on libnbd to call it as a callback? Yeah, it can get confusing, and it took me a while to convince myself that we call it exactly once on all call paths, particularly when asynch_zero returns true vs. false.
Would be good to test also writing zeroes, but otherwise the fix is complete.
Given that I will be revising the patch to add in a CVE number anyways, I'll improve the test for v2.
Reviewed-by: Nir Soffer <nsoffer@redhat.com>
Thanks.
On Fri, Feb 04, 2022 at 09:31:19AM +0100, Laszlo Ersek wrote:
On 02/03/22 15:12, Eric Blake wrote:
On Thu, Feb 03, 2022 at 02:46:27PM +0200, Nir Soffer wrote:
So we would use fprintf and strerror, and setting errno would be needed although it can be used with %m (is it portable?).
%m is not portable in *printf(3). It IS portable in syslog(3), which is why glibc printf(3) supports %m as well; and in nbdkit, we use a printf wrapper on non-glibc to give unconditional %m support to nbdkit plugins. But we do not have that printf wrapper here. Using strerror() instead of perror() is doable in applications (we shouldn't do it directly in libnbd since strerror() has multi-threaded implications and we don't control what the application may be doing in other threads, but in nbdcopy we control the entire application).
Side topic: what do we think about strerror_r()?
The glibc and POSIX signatures differ, making it a pain to use portably. Gnulib has LGPLv2+ wrappers that work around that, but we can't use gnulib in nbdkit, and libnbd has (so far) been able to cope without needing a dependency on gnulib. I also seem to recall that we haven't audited nbdkit for unsafe uses of strerror and strerror_r, partly because it is relatively unrewarding grunt work.
... Hmmm I recall something... Yep: we now have guestfs_int_strerror() in the libguestfs-common project at least.
(We cannot just copy it over I think, due to licence differences; however, with Rich having authored commit 9ec7cd8e23b5 ("utils: Fix usage of strerror_r", 2021-12-09), Red Hat seems to "own the copyright on that function" (if this statement makes any sense), so we could "relicense" it.)
Yes, that is an option, if we decide that it is time to perform that audit (but it is not my highest priority at the moment). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.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.
BTW: why is this a CVE? How is this exploitable for an attacker? Thanks Laszlo
On Fri, Feb 04, 2022 at 09:26:09AM +0100, Laszlo Ersek wrote:
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?
If you use nbdcopy over TCP without TLS (admittedly, that's already a security risk you are taking by not using TLS), a MitM attacker can inject a read or write failure to cause data corruption, and the end-user of nbdcopy will receive no indication that it happened. Users expect that if nbdcopy exits with status 0 and no messages, then the destination should match the source. I haven't ruled out whether an attacker could also corrupt a copy made by TLS by aborting the stream at just the right moment before the end of the transfer. On that front, nbd-ops.c looks like it will generally fail the copy if nbd_shutdown() fails to clean up because the connection was torn down prematurely, but it's difficult to prove that there is no timing window where an attacker can manage to cause the callback to nbd_aio_pread to fail while still being undetected by nbd_shutdown. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
participants (4)
-
Eric Blake -
Laszlo Ersek -
Nir Soffer -
Richard W.M. Jones