rpms/nbdkit
7
0
Fork 0
Code Issues Pull Requests Releases Activity

vddk: Avoid reading partial chunk beyond the end of the disk

Browse Source 
resolves: RHEL-71694
This commit is contained in:
Richard W.M. Jones 2025-01-06 17:37:12 +00:00
parent a8a97713f1
commit 8243333e16
6 changed files with 364 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
0006-vddk-Include-stdbool.h.patch Normal file
View File

@ -0,0 +1,28 @@
From 47345a7a56c343e2cd559b736df685214ed75a9b Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Mon, 6 Jan 2025 15:22:05 +0000
Subject: [PATCH] vddk: Include <stdbool.h>
Since this file uses booleans.
Acked-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit fe855addae44e45e2344a33bd3857c561587f12e)
---
plugins/vddk/worker.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/plugins/vddk/worker.c b/plugins/vddk/worker.c
index 467d00ca..5982fcea 100644
--- a/plugins/vddk/worker.c
+++ b/plugins/vddk/worker.c
@@ -34,6 +34,7 @@
#include <stdio.h>
#include <stdlib.h>
+#include <stdbool.h>
#include <stdint.h>
#include <inttypes.h>
--
2.43.0

59
0007-vddk-Cache-the-disk-size-in-the-handle.patch Normal file
View File

@ -0,0 +1,59 @@
From 927cb4063da464aa2605ae87d1b1157146551a47 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Mon, 6 Jan 2025 16:47:55 +0000
Subject: [PATCH] vddk: Cache the disk size in the handle
No functional change here, we're just making sure we have the disk
size (in bytes) available in the handle.
Acked-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit 2ba76db4a048471e997e508715081a70356f94f3)
---
plugins/vddk/vddk.c | 6 +++---
plugins/vddk/vddk.h | 3 +++
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index 27d53bd6..2a787453 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -875,19 +875,19 @@ vddk_get_size (void *handle)
{
struct vddk_handle *h = handle;
VixDiskLibInfo *info;
- int64_t size;
struct command info_cmd = { .type = INFO, .ptr = &info };
if (send_command_and_wait (h, &info_cmd) == -1)
return -1;
- size = info->capacity * (int64_t)VIXDISKLIB_SECTOR_SIZE;
+ /* Compute the size and cache it into the handle. */
+ h->size = info->capacity * VIXDISKLIB_SECTOR_SIZE;
VDDK_CALL_START (VixDiskLib_FreeInfo, "info")
VixDiskLib_FreeInfo (info);
VDDK_CALL_END (VixDiskLib_FreeInfo, 0);
- return size;
+ return h->size;
}
/* Advertise most efficient block sizes. */
diff --git a/plugins/vddk/vddk.h b/plugins/vddk/vddk.h
index fb0c79a8..1d1069cc 100644
--- a/plugins/vddk/vddk.h
+++ b/plugins/vddk/vddk.h
@@ -165,6 +165,9 @@ struct vddk_handle {
command_queue commands; /* command queue */
pthread_cond_t commands_cond; /* condition (queue size 0 -> 1) */
uint64_t id; /* next command ID */
+
+ /* Cached disk size in bytes (set in get_size()). */
+ uint64_t size;
};
/* reexec.c */
--
2.43.0

34
0008-vddk-do_extents-Mark-some-local-variables-const.patch Normal file
View File

@ -0,0 +1,34 @@
From 0ba2e0d3c53efd49309d9e274e0cb6c2e6720cbd Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Mon, 6 Jan 2025 15:37:54 +0000
Subject: [PATCH] vddk: do_extents: Mark some local variables const
These are never changed in the code (they are fields copied out from
the *cmd struct), so mark them as const.
Acked-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit 24fd7df460ae31fe3f72b5100ca3dbe138bbadbe)
---
plugins/vddk/worker.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/plugins/vddk/worker.c b/plugins/vddk/worker.c
index 5982fcea..bc015d16 100644
--- a/plugins/vddk/worker.c
+++ b/plugins/vddk/worker.c
@@ -388,9 +388,9 @@ add_extent (struct nbdkit_extents *extents,
static int
do_extents (struct command *cmd, struct vddk_handle *h)
{
- uint32_t count = cmd->count;
- uint64_t offset = cmd->offset;
- bool req_one = cmd->req_one;
+ const uint32_t count = cmd->count;
+ const uint64_t offset = cmd->offset;
+ const bool req_one = cmd->req_one;
struct nbdkit_extents *extents = cmd->ptr;
uint64_t position, end, start_sector;
--
2.43.0

31
0009-vddk-do_extents-Exit-the-function-if-we-hit-req_one-.patch Normal file
View File

@ -0,0 +1,31 @@
From 6dd8b5eb14c0723764dd39597d64f62162ca3ab3 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Mon, 6 Jan 2025 16:39:51 +0000
Subject: [PATCH] vddk: do_extents: Exit the function if we hit req_one
condition
No change to the functionality, since the code previously called
'return 0' immediately following the loop.
Acked-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit 2f4d71f8f704d89d69cd635791c3239d2f44d631)
---
plugins/vddk/worker.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/plugins/vddk/worker.c b/plugins/vddk/worker.c
index bc015d16..112111e3 100644
--- a/plugins/vddk/worker.c
+++ b/plugins/vddk/worker.c
@@ -471,7 +471,7 @@ do_extents (struct command *cmd, struct vddk_handle *h)
* overlapping the original offset we're done.
*/
if (req_one && position > offset)
- break;
+ return 0;
}
return 0;
--
2.43.0

202
0010-vddk-do_extents-Avoid-reading-partial-chunk-beyond-t.patch Normal file
View File

@ -0,0 +1,202 @@
From fdc2a6a9818aad25ee606118d5f6a32fa0739912 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Mon, 6 Jan 2025 15:45:35 +0000
Subject: [PATCH] vddk: do_extents: Avoid reading partial chunk beyond the end
of the disk
The QueryAllocatedBlocks API has (another) frustrating feature. It
can only query whole "chunks" (128 sectors). If the disk size is not
aligned to the chunk size (say the size was 129 sectors) then there's
a bit at the end which cannot be queried. Furthermore, the API gives
an error in this case instead of being helpful:
VixDiskLib_QueryAllocatedBlocks: One of the parameters was invalid
Fixes: https://issues.redhat.com/browse/RHEL-71694
Reported-by: Ming Xie <mxie@redhat.com>
Acked-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit fd918f3d1a185fd996999766c75acb9d6e22395d)
---
plugins/vddk/worker.c | 29 ++++++++-
tests/Makefile.am | 5 +-
tests/test-vddk-real-unaligned-chunk.sh | 82 +++++++++++++++++++++++++
3 files changed, 113 insertions(+), 3 deletions(-)
create mode 100755 tests/test-vddk-real-unaligned-chunk.sh
diff --git a/plugins/vddk/worker.c b/plugins/vddk/worker.c
index 112111e3..8a91250a 100644
--- a/plugins/vddk/worker.c
+++ b/plugins/vddk/worker.c
@@ -392,10 +392,9 @@ do_extents (struct command *cmd, struct vddk_handle *h)
const uint64_t offset = cmd->offset;
const bool req_one = cmd->req_one;
struct nbdkit_extents *extents = cmd->ptr;
- uint64_t position, end, start_sector;
+ uint64_t position, start_sector, size_sectors, last_queryable_sector, end;
position = offset;
- end = offset + count;
/* We can only query whole chunks. Therefore start with the
* first chunk before offset.
@@ -403,6 +402,21 @@ do_extents (struct command *cmd, struct vddk_handle *h)
start_sector =
ROUND_DOWN (offset, VIXDISKLIB_MIN_CHUNK_SIZE * VIXDISKLIB_SECTOR_SIZE)
/ VIXDISKLIB_SECTOR_SIZE;
+
+ /* Calculate the end byte + 1 that we're going to query, normally
+ * this is offset + count.
+ *
+ * However since chunks are larger than sectors, for a disk which
+ * has size which is not aligned to the chunk size there is a part
+ * of the disk at the end that we can never query. Reduce 'end' to
+ * the maximum possible queryable part of the disk, and we'll deal
+ * with the unaligned bit after the loop (RHEL-71694).
+ */
+ end = offset + count;
+ size_sectors = h->size / VIXDISKLIB_SECTOR_SIZE;
+ last_queryable_sector = ROUND_DOWN (size_sectors, VIXDISKLIB_MIN_CHUNK_SIZE);
+ end = MIN (end, last_queryable_sector * VIXDISKLIB_SECTOR_SIZE);
+
while (start_sector * VIXDISKLIB_SECTOR_SIZE < end) {
VixError err;
uint32_t i;
@@ -474,6 +488,17 @@ do_extents (struct command *cmd, struct vddk_handle *h)
return 0;
}
+ /* If 'end' spanned beyond the last chunk of the disk, then we
+ * reduced it above to avoid reading a chunk that extends beyond the
+ * end of the underlying disk. We have to synthesize an allocated
+ * block here, which is what VDDK's example code does
+ * (doc/samples/diskLib/vixDiskLibSample.cpp: DoGetAllocatedBlocks).
+ */
+ if (end < offset + count) {
+ if (add_extent (extents, &position, offset + count, false) == -1)
+ return -1;
+ }
+
return 0;
}
diff --git a/tests/Makefile.am b/tests/Makefile.am
index d510807c..0aa36846 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -178,7 +178,8 @@ if HAVE_VDDK
check-vddk:
$(MAKE) check TESTS="test-vddk-real.sh \
test-vddk-real-dump-plugin.sh \
- test-vddk-real-create.sh"
+ test-vddk-real-create.sh \
+ test-vddk-real-unaligned-chunk.sh"
endif HAVE_VDDK
#----------------------------------------------------------------------
@@ -1145,6 +1146,7 @@ TESTS += \
test-vddk-password-interactive.sh \
test-vddk-real-create.sh \
test-vddk-real-dump-plugin.sh \
+ test-vddk-real-unaligned-chunk.sh \
test-vddk-real.sh \
test-vddk-reexec.sh \
test-vddk-run.sh \
@@ -1177,6 +1179,7 @@ EXTRA_DIST += \
test-vddk-password-interactive.sh \
test-vddk-real-create.sh \
test-vddk-real-dump-plugin.sh \
+ test-vddk-real-unaligned-chunk.sh \
test-vddk-real.sh \
test-vddk-reexec.sh \
test-vddk-run.sh \
diff --git a/tests/test-vddk-real-unaligned-chunk.sh b/tests/test-vddk-real-unaligned-chunk.sh
new file mode 100755
index 00000000..28fccd6c
--- /dev/null
+++ b/tests/test-vddk-real-unaligned-chunk.sh
@@ -0,0 +1,82 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright Red Hat
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+# Regression test for https://issues.redhat.com/browse/RHEL-71694
+
+source ./functions.sh
+set -e
+set -x
+
+requires_run
+requires test "x$vddkdir" != "x"
+requires test -d "$vddkdir"
+requires test -f "$vddkdir/lib64/libvixDiskLib.so"
+requires qemu-img --version
+requires_nbdinfo
+requires $TRUNCATE --version
+requires dd --version
+requires test -r /dev/urandom
+skip_if_valgrind "because setting LD_LIBRARY_PATH breaks valgrind"
+
+# VDDK > 5.1.1 only supports x86_64.
+if [ `uname -m` != "x86_64" ]; then
+ echo "$0: unsupported architecture"
+ exit 77
+fi
+
+d=vddk-real-unaligned-chunk.d
+cleanup_fn rm -rf $d
+rm -rf $d
+mkdir $d
+
+# Create a vmdk disk which is partially sparse and the size is NOT
+# aligned to 128 sectors (chunk size).
+dd if=/dev/urandom of=$d/test.raw bs=512 count=$(( 3*128 ))
+$TRUNCATE -s $(( (4*128 + 3) * 512)) $d/test.raw
+qemu-img convert -f raw $d/test.raw -O vmdk $d/test.vmdk
+
+# Read the map using VDDK.
+export d
+nbdkit -rfv vddk libdir="$vddkdir" \
+ $PWD/$d/test.vmdk \
+ --run 'nbdinfo --map "$uri" > $d/map'
+cat $d/map
+
+# Note a few features of the expected map. The first 3 chunks (3*128
+# sectors) are allocated, followed by a single hole chunk. Then the
+# last 3 unaligned sectors appear allocated (even though they are not)
+# because we could not read them using the QueryAllocatedBlocks API so
+# we had to assume allocated.
+test "$(cat $d/map)" = "\
+ 0 196608 0 data
+ 196608 65536 3 hole,zero
+ 262144 1536 0 data"
--
2.43.0

11
nbdkit.spec
View File

@ -56,7 +56,7 @@
Name: nbdkit
Version: 1.38.5
Release: 1%{?dist}
Release: 2%{?dist}
Summary: NBD server
License: BSD-3-Clause
@ -86,6 +86,11 @@ Patch0002: 0002-server-Rename-threadlocal_-set-get-_error-to-._errno.patch
Patch0003: 0003-server-Introduce-threadlocal_-set-get-_last_error.patch
Patch0004: 0004-server-Take-a-thread-local-copy-of-the-last-call-to-.patch
Patch0005: 0005-server-Send-the-last-error-to-the-NBD-client.patch
Patch0006: 0006-vddk-Include-stdbool.h.patch
Patch0007: 0007-vddk-Cache-the-disk-size-in-the-handle.patch
Patch0008: 0008-vddk-do_extents-Mark-some-local-variables-const.patch
Patch0009: 0009-vddk-do_extents-Exit-the-function-if-we-hit-req_one-.patch
Patch0010: 0010-vddk-do_extents-Avoid-reading-partial-chunk-beyond-t.patch
# For automatic RPM Provides generation.
# See: https://rpm-software-management.github.io/rpm/manual/dependency_generators.html
@ -1504,6 +1509,10 @@ fi
%changelog
* Mon Jan 06 2025 Richard W.M. Jones <rjones@redhat.com> - 1.38.5-2
- vddk: Avoid reading partial chunk beyond the end of the disk
resolves: RHEL-71694
* Sat Sep 28 2024 Richard W.M. Jones <rjones@redhat.com> - 1.38.5-1
- Rebase to 1.38.5 (along stable branch)