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

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

Browse Source 
resolves: RHEL-71697
This commit is contained in:
Richard W.M. Jones 2025-01-06 17:37:13 +00:00
parent daa291b813
commit 0b6564875e
6 changed files with 364 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -0,0 +1,28 @@
From 9f86b51b4d8110ee82f2c67c3939c85ce0ec1ea9 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
0002-vddk-Cache-the-disk-size-in-the-handle.patch Normal file
View File

@ -0,0 +1,59 @@
From bfac699727ccf20757dcb5dc4ce1aff885025c9d 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 6d242515..7a830cf9 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -873,19 +873,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
0003-vddk-do_extents-Mark-some-local-variables-const.patch Normal file
View File

@ -0,0 +1,34 @@
From dff3fc3b97aab79f6ee168a9b9dd2dff05425439 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
0004-vddk-do_extents-Exit-the-function-if-we-hit-req_one-.patch Normal file
View File

@ -0,0 +1,31 @@
From 5f7e5399aa4b208cb6aa0c51dbea59f73fd4d5f3 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
0005-vddk-do_extents-Avoid-reading-partial-chunk-beyond-t.patch Normal file
View File

@ -0,0 +1,202 @@
From fe65a789da92e53bfd3f3814f1c93566f69591db 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 c0d1bdcc..94d4a219 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -188,7 +188,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
#----------------------------------------------------------------------
@@ -1172,6 +1173,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 \
@@ -1204,6 +1206,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

12
nbdkit.spec
View File

@ -55,7 +55,7 @@
Name: nbdkit
Version: 1.40.4
Release: 2%{?dist}
Release: 3%{?dist}
Summary: NBD server
License: BSD-3-Clause
@ -80,7 +80,11 @@ Source3: copy-patches.sh
# https://gitlab.com/nbdkit/nbdkit/-/commits/rhel-10.0/
# Patches.
#(nothing)
Patch0001: 0001-vddk-Include-stdbool.h.patch
Patch0002: 0002-vddk-Cache-the-disk-size-in-the-handle.patch
Patch0003: 0003-vddk-do_extents-Mark-some-local-variables-const.patch
Patch0004: 0004-vddk-do_extents-Exit-the-function-if-we-hit-req_one-.patch
Patch0005: 0005-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
@ -1498,6 +1502,10 @@ fi
%changelog
* Mon Jan 06 2025 Richard W.M. Jones <rjones@redhat.com> - 1.40.4-3
- vddk: Avoid reading partial chunk beyond the end of the disk
resolves: RHEL-71697
* Tue Oct 29 2024 Troy Dawson <tdawson@redhat.com> - 1.40.4-2
- Bump release for October 2024 mass rebuild:
Resolves: RHEL-64018