The inspect_do_decrypt() function interates over the list of partitions. For each partition, it fetches the potentially matching keys, and tries each key in turn. If no key unlocks the partition, the program exits with an error message. If at least one partition is unlocked, then LVM is rescanned. Decouple "partition" from the above logic, replacing it with "mountable". Call the new function decrypt_mountables(). As a part of the extraction, clean up the following readability warts: - CLEANUP_FREE* and other variable declarations intermixed with code, - a "goto" statement that is not used on an error path, - needless conditions on a one-shot "is_bitlocker" helper variable, - nondescript array indices, - guestfs_int_count_strings() called for counting the length of the full string list, only to check if the string list is non-empty, - a comment about GUESTFS_CRYPTSETUP_OPEN_READONLY placed outside of GUESTFS_HAVE_CRYPTSETUP_OPEN. Regression-checked with: - libguestfs/tests/luks/test-key-option-inspect.sh - guestfs-tools/inspector/test-virt-inspector-luks.sh Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- options/decrypt.c | 128 +++++++++++++++++++++++++++------------------- 1 file changed, 75 insertions(+), 53 deletions(-) diff --git a/options/decrypt.c b/options/decrypt.c index 434b7d58c2a7..9141cf5193ad 100644 --- a/options/decrypt.c +++ b/options/decrypt.c @@ -65,6 +65,78 @@ make_mapname (const char *device, char *mapname, size_t len) *mapname = '\0'; } +static bool +decrypt_mountables (guestfs_h *g, const char * const *mountables, + struct key_store *ks) +{ + bool decrypted_some = false; + const char * const *mnt_scan = mountables; + const char *mountable; + + while ((mountable = *mnt_scan++) != NULL) { + CLEANUP_FREE char *type = NULL; + CLEANUP_FREE char *uuid = NULL; + CLEANUP_FREE_STRING_LIST char **keys = NULL; + char mapname[32]; + const char * const *key_scan; + const char *key; + + type = guestfs_vfs_type (g, mountable); + if (type == NULL) + continue; + + /* "cryptsetup luksUUID" cannot read a UUID on Windows BitLocker disks + * (unclear if this is a limitation of the format or cryptsetup). + */ + if (STREQ (type, "crypto_LUKS")) { +#ifdef GUESTFS_HAVE_LUKS_UUID + uuid = guestfs_luks_uuid (g, mountable); +#endif + } else if (STRNEQ (type, "BitLocker")) + continue; + + /* Grab the keys that we should try with this device, based on device name, + * or UUID (if any). + */ + keys = get_keys (ks, mountable, uuid); + assert (keys[0] != NULL); + + /* Generate a node name for the plaintext (decrypted) device node. */ + make_mapname (mountable, mapname, sizeof mapname); + + /* Try each key in turn. */ + key_scan = (const char * const *)keys; + while ((key = *key_scan++) != NULL) { + int r; + + guestfs_push_error_handler (g, NULL, NULL); +#ifdef GUESTFS_HAVE_CRYPTSETUP_OPEN + /* XXX Should we set GUESTFS_CRYPTSETUP_OPEN_READONLY if readonly is + * set? This might break 'mount_ro'. + */ + r = guestfs_cryptsetup_open (g, mountable, key, mapname, -1); +#else + r = guestfs_luks_open (g, mountable, key, mapname); +#endif + guestfs_pop_error_handler (g); + + if (r == 0) + break; + } + + if (key == NULL) + error (EXIT_FAILURE, 0, + _("could not find key to open LUKS encrypted %s.\n\n" + "Try using --key on the command line.\n\n" + "Original error: %s (%d)"), + mountable, guestfs_last_error (g), guestfs_last_errno (g)); + + decrypted_some = true; + } + + return decrypted_some; +} + /** * Simple implementation of decryption: look for any encrypted * partitions and decrypt them, then rescan for VGs. @@ -73,62 +145,12 @@ void inspect_do_decrypt (guestfs_h *g, struct key_store *ks) { CLEANUP_FREE_STRING_LIST char **partitions = guestfs_list_partitions (g); + bool need_rescan; + if (partitions == NULL) exit (EXIT_FAILURE); - int need_rescan = 0, r; - size_t i, j; - - for (i = 0; partitions[i] != NULL; ++i) { - CLEANUP_FREE char *type = guestfs_vfs_type (g, partitions[i]); - if (type && - (STREQ (type, "crypto_LUKS") || STREQ (type, "BitLocker"))) { - bool is_bitlocker = STREQ (type, "BitLocker"); - char mapname[32]; - make_mapname (partitions[i], mapname, sizeof mapname); - -#ifdef GUESTFS_HAVE_LUKS_UUID - CLEANUP_FREE char *uuid = NULL; - - /* This fails for Windows BitLocker disks because cryptsetup - * luksUUID cannot read a UUID (unclear if this is a limitation - * of the format or cryptsetup). - */ - if (!is_bitlocker) - uuid = guestfs_luks_uuid (g, partitions[i]); -#else - const char *uuid = NULL; -#endif - - CLEANUP_FREE_STRING_LIST char **keys = get_keys (ks, partitions[i], uuid); - assert (guestfs_int_count_strings (keys) > 0); - - /* Try each key in turn. */ - for (j = 0; keys[j] != NULL; ++j) { - /* XXX Should we set GUESTFS_CRYPTSETUP_OPEN_READONLY if readonly - * is set? This might break 'mount_ro'. - */ - guestfs_push_error_handler (g, NULL, NULL); -#ifdef GUESTFS_HAVE_CRYPTSETUP_OPEN - r = guestfs_cryptsetup_open (g, partitions[i], keys[j], mapname, -1); -#else - r = guestfs_luks_open (g, partitions[i], keys[j], mapname); -#endif - guestfs_pop_error_handler (g); - if (r == 0) - goto opened; - } - error (EXIT_FAILURE, 0, - _("could not find key to open LUKS encrypted %s.\n\n" - "Try using --key on the command line.\n\n" - "Original error: %s (%d)"), - partitions[i], guestfs_last_error (g), - guestfs_last_errno (g)); - - opened: - need_rescan = 1; - } - } + need_rescan = decrypt_mountables (g, (const char * const *)partitions, ks); if (need_rescan) { if (guestfs_lvm_scan (g, 1) == -1) -- 2.19.1.3.g30247aa5d201

On 02/24/22 11:35, Richard W.M. Jones wrote:

On Wed, Feb 23, 2022 at 05:19:14PM +0100, Laszlo Ersek wrote:

...

The inspect_do_decrypt() function interates over the list of partitions. For each partition, it fetches the potentially matching keys, and tries each key in turn. If no key unlocks the partition, the program exits with an error message. If at least one partition is unlocked, then LVM is rescanned.

Decouple "partition" from the above logic, replacing it with "mountable". Call the new function decrypt_mountables(). As a part of the extraction, clean up the following readability warts:

- CLEANUP_FREE* and other variable declarations intermixed with code,

- a "goto" statement that is not used on an error path,

- needless conditions on a one-shot "is_bitlocker" helper variable,

- nondescript array indices,

- guestfs_int_count_strings() called for counting the length of the full string list, only to check if the string list is non-empty,

- a comment about GUESTFS_CRYPTSETUP_OPEN_READONLY placed outside of GUESTFS_HAVE_CRYPTSETUP_OPEN.

Regression-checked with: - libguestfs/tests/luks/test-key-option-inspect.sh - guestfs-tools/inspector/test-virt-inspector-luks.sh

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- options/decrypt.c | 128 +++++++++++++++++++++++++++------------------- 1 file changed, 75 insertions(+), 53 deletions(-)

diff --git a/options/decrypt.c b/options/decrypt.c index 434b7d58c2a7..9141cf5193ad 100644 --- a/options/decrypt.c +++ b/options/decrypt.c @@ -65,6 +65,78 @@ make_mapname (const char *device, char *mapname, size_t len) *mapname = '\0'; }

+static bool +decrypt_mountables (guestfs_h *g, const char * const *mountables, + struct key_store *ks) +{ + bool decrypted_some = false; + const char * const *mnt_scan = mountables; + const char *mountable; + + while ((mountable = *mnt_scan++) != NULL) { + CLEANUP_FREE char *type = NULL; + CLEANUP_FREE char *uuid = NULL; + CLEANUP_FREE_STRING_LIST char **keys = NULL; + char mapname[32]; + const char * const *key_scan; + const char *key; + + type = guestfs_vfs_type (g, mountable); + if (type == NULL) + continue; + + /* "cryptsetup luksUUID" cannot read a UUID on Windows BitLocker disks + * (unclear if this is a limitation of the format or cryptsetup). + */ + if (STREQ (type, "crypto_LUKS")) { +#ifdef GUESTFS_HAVE_LUKS_UUID + uuid = guestfs_luks_uuid (g, mountable); +#endif

As a separate patch after this one, I don't mind getting rid of this #ifdef conditional, and also the GUESTFS_HAVE_CRYPTSETUP_OPEN conditional below.

guestfs_luks_uuid was added in libguestfs 1.42 (released March 2020). guestfs_cryptsetup_open was added in libguestfs 1.44 (Jan 2021).

guestfs-tools already requires libguestfs >= 1.44 (see guestfs-tools.git/m4/guestfs-libraries.m4). So that's OK.

virt-v2v in theory requires only libguestfs >= 1.40, but I don't think there's any problem with requiring 1.44. It already requires a very recent libnbd. virt-v2v.git/m4/guestfs-libraries.m4 would need to be changed.

Thanks, those #ifdefs did look stale to me. The cleanups should go into a separate patch; I'll look into that.

...

+ } else if (STRNEQ (type, "BitLocker")) + continue; + + /* Grab the keys that we should try with this device, based on device name, + * or UUID (if any). + */ + keys = get_keys (ks, mountable, uuid); + assert (keys[0] != NULL); + + /* Generate a node name for the plaintext (decrypted) device node. */ + make_mapname (mountable, mapname, sizeof mapname); + + /* Try each key in turn. */ + key_scan = (const char * const *)keys; + while ((key = *key_scan++) != NULL) { + int r; + + guestfs_push_error_handler (g, NULL, NULL); +#ifdef GUESTFS_HAVE_CRYPTSETUP_OPEN + /* XXX Should we set GUESTFS_CRYPTSETUP_OPEN_READONLY if readonly is + * set? This might break 'mount_ro'. + */

This XXX comment (copied from the old code) is bogus. It would definitely be wrong to use OPEN_READONLY. In r/o mode we defend against modifying the underlying devices using a qcow2 overlay (lib/drives.c:create_overlay). Writable guest devices are required to mount filesystems with journals, even when mounting with -o ro.

So you might drop it, maybe as a separate commit.

Will do (separate commit indeed).

...

+ r = guestfs_cryptsetup_open (g, mountable, key, mapname, -1); +#else + r = guestfs_luks_open (g, mountable, key, mapname); +#endif + guestfs_pop_error_handler (g); + + if (r == 0) + break; + } + + if (key == NULL) + error (EXIT_FAILURE, 0, + _("could not find key to open LUKS encrypted %s.\n\n" + "Try using --key on the command line.\n\n" + "Original error: %s (%d)"), + mountable, guestfs_last_error (g), guestfs_last_errno (g)); + + decrypted_some = true; + } + + return decrypted_some; +} + /** * Simple implementation of decryption: look for any encrypted * partitions and decrypt them, then rescan for VGs. @@ -73,62 +145,12 @@ void inspect_do_decrypt (guestfs_h *g, struct key_store *ks) { CLEANUP_FREE_STRING_LIST char **partitions = guestfs_list_partitions (g); + bool need_rescan; + if (partitions == NULL) exit (EXIT_FAILURE);

- int need_rescan = 0, r; - size_t i, j; - - for (i = 0; partitions[i] != NULL; ++i) { - CLEANUP_FREE char *type = guestfs_vfs_type (g, partitions[i]); - if (type && - (STREQ (type, "crypto_LUKS") || STREQ (type, "BitLocker"))) { - bool is_bitlocker = STREQ (type, "BitLocker"); - char mapname[32]; - make_mapname (partitions[i], mapname, sizeof mapname); - -#ifdef GUESTFS_HAVE_LUKS_UUID - CLEANUP_FREE char *uuid = NULL; - - /* This fails for Windows BitLocker disks because cryptsetup - * luksUUID cannot read a UUID (unclear if this is a limitation - * of the format or cryptsetup). - */ - if (!is_bitlocker) - uuid = guestfs_luks_uuid (g, partitions[i]); -#else - const char *uuid = NULL; -#endif - - CLEANUP_FREE_STRING_LIST char **keys = get_keys (ks, partitions[i], uuid); - assert (guestfs_int_count_strings (keys) > 0); - - /* Try each key in turn. */ - for (j = 0; keys[j] != NULL; ++j) { - /* XXX Should we set GUESTFS_CRYPTSETUP_OPEN_READONLY if readonly - * is set? This might break 'mount_ro'. - */ - guestfs_push_error_handler (g, NULL, NULL); -#ifdef GUESTFS_HAVE_CRYPTSETUP_OPEN - r = guestfs_cryptsetup_open (g, partitions[i], keys[j], mapname, -1); -#else - r = guestfs_luks_open (g, partitions[i], keys[j], mapname); -#endif - guestfs_pop_error_handler (g); - if (r == 0) - goto opened; - } - error (EXIT_FAILURE, 0, - _("could not find key to open LUKS encrypted %s.\n\n" - "Try using --key on the command line.\n\n" - "Original error: %s (%d)"), - partitions[i], guestfs_last_error (g), - guestfs_last_errno (g)); - - opened: - need_rescan = 1; - } - } + need_rescan = decrypt_mountables (g, (const char * const *)partitions, ks);

if (need_rescan) { if (guestfs_lvm_scan (g, 1) == -1)

Patch looks fine, ACK.

Thanks! Laszlo

On Wed, Feb 23, 2022 at 05:19:15PM +0100, Laszlo Ersek wrote:

Using the previously extracted function decrypt_mountables(), look for LUKS devices on logical volumes (LVs) too.

In the LVM-on-LUKS scheme, the names of the plaintext (decrypted) block devices don't matter, as these devices host Physical Volumes, and LVM enumerates PVs automatically -- there are no references to these decrypted block devices in "/etc/fstab", for example. For naming such decrypted devices, continue calling make_mapname().

In the LUKS-on-LVM scheme however, the decrypted devices are supposed to hold filesystems, and "/etc/fstab" may refer to them. Such decrypted devices are commonly called /dev/mapper/luks-<UUID>, where <UUID> is the UUID inside the LUKS header. Employ this naming when decrypting Logical Volumes. Reuse make_mapname() as a fallback in this case.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126 Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- options/decrypt.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/options/decrypt.c b/options/decrypt.c index 9141cf5193ad..f7c1d876b3ed 100644 --- a/options/decrypt.c +++ b/options/decrypt.c @@ -38,8 +38,11 @@ #include "options.h"

/** - * Make a LUKS map name from the partition name, - * eg. C<"/dev/vda2" =E<gt> "cryptvda2"> + * Make a LUKS map name from the partition or logical volume name, eg. + * C<"/dev/vda2" =E<gt> "cryptvda2">, or C<"/dev/vg-ssd/lv-root7" =E<gt> + * "cryptvgssdlvroot7">. Note that, in logical volume device names, + * c_isalnum() eliminates the "/" separator from between the VG and the LV, so + * this mapping is not unique; but for our purposes, it will do. */ static void make_mapname (const char *device, char *mapname, size_t len) @@ -67,7 +70,7 @@ make_mapname (const char *device, char *mapname, size_t len)

static bool decrypt_mountables (guestfs_h *g, const char * const *mountables, - struct key_store *ks) + struct key_store *ks, bool name_decrypted_by_uuid) { bool decrypted_some = false; const char * const *mnt_scan = mountables; @@ -77,7 +80,7 @@ decrypt_mountables (guestfs_h *g, const char * const *mountables, CLEANUP_FREE char *type = NULL; CLEANUP_FREE char *uuid = NULL; CLEANUP_FREE_STRING_LIST char **keys = NULL; - char mapname[32]; + char mapname[512]; const char * const *key_scan; const char *key;

@@ -102,7 +105,9 @@ decrypt_mountables (guestfs_h *g, const char * const *mountables, assert (keys[0] != NULL);

/* Generate a node name for the plaintext (decrypted) device node. */ - make_mapname (mountable, mapname, sizeof mapname); + if (!name_decrypted_by_uuid || uuid == NULL || + snprintf (mapname, sizeof mapname, "luks-%s", uuid) < 0) + make_mapname (mountable, mapname, sizeof mapname);

This should all really be using asprintf, and not stack-allocating large arrays. Although it's kind of not related to this change, this change does make the stack frame larger. Saying that, when removing gnulib (libguestfs commit 0f54df53d26), I dropped the warning about large stack frames: -dnl Warn about large stack frames, including estimates for alloca -dnl and variable length arrays. -gl_WARN_ADD([-Wstack-usage=10000]) I really need to add that back ...

/* Try each key in turn. */ key_scan = (const char * const *)keys; @@ -145,15 +150,22 @@ void inspect_do_decrypt (guestfs_h *g, struct key_store *ks) { CLEANUP_FREE_STRING_LIST char **partitions = guestfs_list_partitions (g); + CLEANUP_FREE_STRING_LIST char **lvs = NULL; bool need_rescan;

if (partitions == NULL) exit (EXIT_FAILURE);

- need_rescan = decrypt_mountables (g, (const char * const *)partitions, ks); + need_rescan = decrypt_mountables (g, (const char * const *)partitions, ks, + false);

if (need_rescan) { if (guestfs_lvm_scan (g, 1) == -1) exit (EXIT_FAILURE); } + + lvs = guestfs_lvs (g); + if (lvs == NULL) + exit (EXIT_FAILURE); + decrypt_mountables (g, (const char * const *)lvs, ks, true);

ACK Rich. -- 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/24/22 13:12, Laszlo Ersek wrote:

On 02/24/22 11:43, Richard W.M. Jones wrote:

...

On Wed, Feb 23, 2022 at 05:19:15PM +0100, Laszlo Ersek wrote:

...

Using the previously extracted function decrypt_mountables(), look for LUKS devices on logical volumes (LVs) too.

In the LVM-on-LUKS scheme, the names of the plaintext (decrypted) block devices don't matter, as these devices host Physical Volumes, and LVM enumerates PVs automatically -- there are no references to these decrypted block devices in "/etc/fstab", for example. For naming such decrypted devices, continue calling make_mapname().

In the LUKS-on-LVM scheme however, the decrypted devices are supposed to hold filesystems, and "/etc/fstab" may refer to them. Such decrypted devices are commonly called /dev/mapper/luks-<UUID>, where <UUID> is the UUID inside the LUKS header. Employ this naming when decrypting Logical Volumes. Reuse make_mapname() as a fallback in this case.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126 Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- options/decrypt.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/options/decrypt.c b/options/decrypt.c index 9141cf5193ad..f7c1d876b3ed 100644 --- a/options/decrypt.c +++ b/options/decrypt.c @@ -38,8 +38,11 @@ #include "options.h"

/** - * Make a LUKS map name from the partition name, - * eg. C<"/dev/vda2" =E<gt> "cryptvda2"> + * Make a LUKS map name from the partition or logical volume name, eg. + * C<"/dev/vda2" =E<gt> "cryptvda2">, or C<"/dev/vg-ssd/lv-root7" =E<gt> + * "cryptvgssdlvroot7">. Note that, in logical volume device names, + * c_isalnum() eliminates the "/" separator from between the VG and the LV, so + * this mapping is not unique; but for our purposes, it will do. */ static void make_mapname (const char *device, char *mapname, size_t len) @@ -67,7 +70,7 @@ make_mapname (const char *device, char *mapname, size_t len)

static bool decrypt_mountables (guestfs_h *g, const char * const *mountables, - struct key_store *ks) + struct key_store *ks, bool name_decrypted_by_uuid) { bool decrypted_some = false; const char * const *mnt_scan = mountables; @@ -77,7 +80,7 @@ decrypt_mountables (guestfs_h *g, const char * const *mountables, CLEANUP_FREE char *type = NULL; CLEANUP_FREE char *uuid = NULL; CLEANUP_FREE_STRING_LIST char **keys = NULL; - char mapname[32]; + char mapname[512]; const char * const *key_scan; const char *key;

@@ -102,7 +105,9 @@ decrypt_mountables (guestfs_h *g, const char * const *mountables, assert (keys[0] != NULL);

/* Generate a node name for the plaintext (decrypted) device node. */ - make_mapname (mountable, mapname, sizeof mapname); + if (!name_decrypted_by_uuid || uuid == NULL || + snprintf (mapname, sizeof mapname, "luks-%s", uuid) < 0) + make_mapname (mountable, mapname, sizeof mapname);

This should all really be using asprintf, and not stack-allocating large arrays. Although it's kind of not related to this change, this change does make the stack frame larger.

asprintf() was my first idea, but reworking make_mapname() would then require measuring the input string lenght ;)

OK, I'll insert a patch between #1 and #2 for converting make_mapname() to asprintf(), then I'll redo this patch with asprintf() too.

On a second thought, if it's OK with you: I'd prefer merging this series as-is, and then posting additional patches on top: - assume GUESTFS_HAVE_LUKS_UUID - assume GUESTFS_HAVE_CRYPTSETUP_OPEN - bump libguestfs version requirement in virt-v2v.git/m4/guestfs-libraries.m4 to 1.44 - remove "XXX Should we set GUESTFS_CRYPTSETUP_OPEN_READONLY" - allocate "mapname" with asprintf() on all paths. The reason I'd like to do these afterwards is that none of these warts are introduced/created by this series, and I had put lots of testing into the series (not just at the end, but mid-series too). If I rebased and needed to resolved conflicts, all that testing would have to be redone. I'd really like to avoid that. Thanks! Laszlo

Thanks Laszlo

...

Saying that, when removing gnulib (libguestfs commit 0f54df53d26), I dropped the warning about large stack frames:

-dnl Warn about large stack frames, including estimates for alloca -dnl and variable length arrays. -gl_WARN_ADD([-Wstack-usage=10000])

I really need to add that back ...

...

/* Try each key in turn. */ key_scan = (const char * const *)keys; @@ -145,15 +150,22 @@ void inspect_do_decrypt (guestfs_h *g, struct key_store *ks) { CLEANUP_FREE_STRING_LIST char **partitions = guestfs_list_partitions (g); + CLEANUP_FREE_STRING_LIST char **lvs = NULL; bool need_rescan;

if (partitions == NULL) exit (EXIT_FAILURE);

- need_rescan = decrypt_mountables (g, (const char * const *)partitions, ks); + need_rescan = decrypt_mountables (g, (const char * const *)partitions, ks, + false);

if (need_rescan) { if (guestfs_lvm_scan (g, 1) == -1) exit (EXIT_FAILURE); } + + lvs = guestfs_lvs (g); + if (lvs == NULL) + exit (EXIT_FAILURE); + decrypt_mountables (g, (const char * const *)lvs, ks, true);

ACK

Rich.

On 02/28/22 10:00, Richard W.M. Jones wrote:

On Fri, Feb 25, 2022 at 11:59:37AM +0100, Laszlo Ersek wrote:

...

On 02/24/22 13:12, Laszlo Ersek wrote:

...

On 02/24/22 11:43, Richard W.M. Jones wrote:

...

On Wed, Feb 23, 2022 at 05:19:15PM +0100, Laszlo Ersek wrote:

...

Using the previously extracted function decrypt_mountables(), look for LUKS devices on logical volumes (LVs) too.

In the LVM-on-LUKS scheme, the names of the plaintext (decrypted) block devices don't matter, as these devices host Physical Volumes, and LVM enumerates PVs automatically -- there are no references to these decrypted block devices in "/etc/fstab", for example. For naming such decrypted devices, continue calling make_mapname().

In the LUKS-on-LVM scheme however, the decrypted devices are supposed to hold filesystems, and "/etc/fstab" may refer to them. Such decrypted devices are commonly called /dev/mapper/luks-<UUID>, where <UUID> is the UUID inside the LUKS header. Employ this naming when decrypting Logical Volumes. Reuse make_mapname() as a fallback in this case.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126 Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- options/decrypt.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/options/decrypt.c b/options/decrypt.c index 9141cf5193ad..f7c1d876b3ed 100644 --- a/options/decrypt.c +++ b/options/decrypt.c @@ -38,8 +38,11 @@ #include "options.h"

/** - * Make a LUKS map name from the partition name, - * eg. C<"/dev/vda2" =E<gt> "cryptvda2"> + * Make a LUKS map name from the partition or logical volume name, eg. + * C<"/dev/vda2" =E<gt> "cryptvda2">, or C<"/dev/vg-ssd/lv-root7" =E<gt> + * "cryptvgssdlvroot7">. Note that, in logical volume device names, + * c_isalnum() eliminates the "/" separator from between the VG and the LV, so + * this mapping is not unique; but for our purposes, it will do. */ static void make_mapname (const char *device, char *mapname, size_t len) @@ -67,7 +70,7 @@ make_mapname (const char *device, char *mapname, size_t len)

static bool decrypt_mountables (guestfs_h *g, const char * const *mountables, - struct key_store *ks) + struct key_store *ks, bool name_decrypted_by_uuid) { bool decrypted_some = false; const char * const *mnt_scan = mountables; @@ -77,7 +80,7 @@ decrypt_mountables (guestfs_h *g, const char * const *mountables, CLEANUP_FREE char *type = NULL; CLEANUP_FREE char *uuid = NULL; CLEANUP_FREE_STRING_LIST char **keys = NULL; - char mapname[32]; + char mapname[512]; const char * const *key_scan; const char *key;

@@ -102,7 +105,9 @@ decrypt_mountables (guestfs_h *g, const char * const *mountables, assert (keys[0] != NULL);

/* Generate a node name for the plaintext (decrypted) device node. */ - make_mapname (mountable, mapname, sizeof mapname); + if (!name_decrypted_by_uuid || uuid == NULL || + snprintf (mapname, sizeof mapname, "luks-%s", uuid) < 0) + make_mapname (mountable, mapname, sizeof mapname);

This should all really be using asprintf, and not stack-allocating large arrays. Although it's kind of not related to this change, this change does make the stack frame larger.

asprintf() was my first idea, but reworking make_mapname() would then require measuring the input string lenght ;)

OK, I'll insert a patch between #1 and #2 for converting make_mapname() to asprintf(), then I'll redo this patch with asprintf() too.

On a second thought, if it's OK with you:

I'd prefer merging this series as-is, and then posting additional patches on top:

- assume GUESTFS_HAVE_LUKS_UUID - assume GUESTFS_HAVE_CRYPTSETUP_OPEN - bump libguestfs version requirement in virt-v2v.git/m4/guestfs-libraries.m4 to 1.44 - remove "XXX Should we set GUESTFS_CRYPTSETUP_OPEN_READONLY" - allocate "mapname" with asprintf() on all paths.

The reason I'd like to do these afterwards is that none of these warts are introduced/created by this series, and I had put lots of testing into the series (not just at the end, but mid-series too). If I rebased and needed to resolved conflicts, all that testing would have to be redone. I'd really like to avoid that.

That's fine.

Thanks! Commit range 41126802097f..2d8c0f8d40b5 (for now). Thanks, Laszlo

Clarify that our current usage of "luks" stands for "lvm-on-luks" (IOW, that the decrypted LUKS devices are Physical Volumes for LVM). Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- test-data/phony-guests/Makefile.am | 8 ++++---- tests/Makefile.am | 4 ++-- .gitignore | 2 +- test-data/phony-guests/guests.xml.in | 4 ++-- test-data/phony-guests/make-fedora-img.pl | 8 ++++---- tests/luks/{test-key-option-inspect.sh => test-key-option-inspect-lvm-on-luks.sh} | 4 ++-- 6 files changed, 15 insertions(+), 15 deletions(-) diff --git a/test-data/phony-guests/Makefile.am b/test-data/phony-guests/Makefile.am index 60313548af32..0114d10bb170 100644 --- a/test-data/phony-guests/Makefile.am +++ b/test-data/phony-guests/Makefile.am @@ -49,7 +49,7 @@ disk_images = \ fedora-md1.img \ fedora-md2.img \ fedora-btrfs.img \ - fedora-luks.img \ + fedora-lvm-on-luks.img \ ubuntu.img \ archlinux.img \ coreos.img \ @@ -96,11 +96,11 @@ fedora-btrfs.img: make-fedora-img.pl \ fedora.db SRCDIR=$(srcdir) LAYOUT=btrfs $(top_builddir)/run --test ./$< -# Make a (dummy) Fedora image with LVM encrypted with LUKS. -fedora-luks.img: make-fedora-img.pl \ +# Make a (dummy) Fedora image with LVM-on-LUKS. +fedora-lvm-on-luks.img: make-fedora-img.pl \ fedora-journal.tar.xz \ fedora.db - SRCDIR=$(srcdir) LAYOUT=lvm-luks $(top_builddir)/run --test ./$< + SRCDIR=$(srcdir) LAYOUT=lvm-on-luks $(top_builddir)/run --test ./$< # Make a (dummy) Debian image. debian.img: make-debian-img.sh diff --git a/tests/Makefile.am b/tests/Makefile.am index 690e09b5e603..9112726750a7 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -464,12 +464,12 @@ TESTS += \ luks/test-luks.sh \ luks/test-luks-list.sh \ luks/test-key-option.sh \ - luks/test-key-option-inspect.sh + luks/test-key-option-inspect-lvm-on-luks.sh EXTRA_DIST += \ luks/test-luks.sh \ luks/test-luks-list.sh \ luks/test-key-option.sh \ - luks/test-key-option-inspect.sh + luks/test-key-option-inspect-lvm-on-luks.sh TESTS += \ lvm/test-lvm-filtering.sh \ diff --git a/.gitignore b/.gitignore index ebb67850c6c6..8fb61aa7bb66 100644 --- a/.gitignore +++ b/.gitignore @@ -452,7 +452,7 @@ Makefile.in /test-data/phony-guests/debian.img /test-data/phony-guests/fedora.img /test-data/phony-guests/fedora-btrfs.img -/test-data/phony-guests/fedora-luks.img +/test-data/phony-guests/fedora-lvm-on-luks.img /test-data/phony-guests/fedora-md1.img /test-data/phony-guests/fedora-md2.img /test-data/phony-guests/fedora.db diff --git a/test-data/phony-guests/guests.xml.in b/test-data/phony-guests/guests.xml.in index 4139d04f689f..3af6b27af073 100644 --- a/test-data/phony-guests/guests.xml.in +++ b/test-data/phony-guests/guests.xml.in @@ -185,7 +185,7 @@ <!-- LUKS password is 'FEDORA' --> <domain type='test'> - <name>fedora-luks</name> + <name>fedora-lvm-on-luks</name> <memory>1048576</memory> <os> <type>hvm</type> @@ -194,7 +194,7 @@ <devices> <disk type='file' device='disk'> <driver name='qemu' type='raw'/> - <source file='@abs_builddir@/fedora-luks.img'/> + <source file='@abs_builddir@/fedora-lvm-on-luks.img'/> <target dev='vda' bus='virtio'/> </disk> </devices> diff --git a/test-data/phony-guests/make-fedora-img.pl b/test-data/phony-guests/make-fedora-img.pl index 27b154f9d71d..4cd6ef9575eb 100755 --- a/test-data/phony-guests/make-fedora-img.pl +++ b/test-data/phony-guests/make-fedora-img.pl @@ -171,8 +171,8 @@ EOF } } -elsif ($ENV{LAYOUT} eq 'lvm-luks') { - push (@images, "fedora-luks.img-t"); +elsif ($ENV{LAYOUT} eq 'lvm-on-luks') { + push (@images, "fedora-lvm-on-luks.img-t"); open (my $fstab, '>', "fedora.fstab") or die; print $fstab <<EOF; @@ -183,9 +183,9 @@ EOF $bootdev = '/dev/sda1'; - $g->disk_create ("fedora-luks.img-t", "raw", $IMAGE_SIZE); + $g->disk_create ("fedora-lvm-on-luks.img-t", "raw", $IMAGE_SIZE); - $g->add_drive ("fedora-luks.img-t", format => "raw"); + $g->add_drive ("fedora-lvm-on-luks.img-t", format => "raw"); $g->launch (); $g->part_init ('/dev/sda', 'mbr'); diff --git a/tests/luks/test-key-option-inspect.sh b/tests/luks/test-key-option-inspect-lvm-on-luks.sh similarity index 94% rename from tests/luks/test-key-option-inspect.sh rename to tests/luks/test-key-option-inspect-lvm-on-luks.sh index 78128db84b52..3d2bb9e6c843 100755 --- a/tests/luks/test-key-option-inspect.sh +++ b/tests/luks/test-key-option-inspect-lvm-on-luks.sh @@ -24,9 +24,9 @@ set -e $TEST_FUNCTIONS skip_if_skipped skip_unless_feature_available luks -skip_unless_phony_guest fedora-luks.img +skip_unless_phony_guest fedora-lvm-on-luks.img -disk=../test-data/phony-guests/fedora-luks.img +disk=../test-data/phony-guests/fedora-lvm-on-luks.img device=/dev/sda2 # Get the UUID of the LUKS device. -- 2.19.1.3.g30247aa5d201

LUKS support used to work best if the LUKS device resided on a partition, and contained a Physical Volume for an LVM Volume Group. This scheme, also called LVM-on-LUKS, is commonly created by installers of various Linux distributions. (See RHBZ#1451665.) Libguestfs now also supports the scheme wherein the LUKS device resides on an LVM Logical Volume, and contains a filesystem. This is called LUKS-on-LVM, it is the inverse of the above scheme, and is created by installers of other Linux distributions. (See RHBZ#1658126.) Both schemes are now decrypted by libguestfs-based utilities when inspection is enabled (such as in "guestfish -i", virt-inspector, virt-v2v), through the inspect_mount() function in utilities written in C, and through the "inspect_decrypt" function in ones written in OCaml. We don't seem to need an API like "list-luks-devices", as "list-dm-devices" returns decrypted (i.e., opened) LUKS devices too; for example, in the LUKS-on-LVM case:

...

<fs> list-dm-devices /dev/mapper/luks-0d619854-ccd5-43b1-8883-991fec5ef713 /dev/mapper/luks-4e9e7a6f-a68c-42fd-92b4-8f4f2579a389

Thus, the subject TODO section is now out of date, and it's unclear what remains "to do" there; let's just remove the section. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- TODO | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/TODO b/TODO index 064386ac2dce..6fb11d5ecdd4 100644 --- a/TODO +++ b/TODO @@ -161,16 +161,6 @@ Could we make guestfish interactive if commands are used without params? Image name? disk.img Size of image? 10M -Better support for encrypted devices ------------------------------------- - -Currently LUKS support only works if the device contains volume -groups. If it contains, eg., partitions, you cannot access them. -We would like to add: - - - Direct access to the /dev/mapper device (eg. if it contains - anything apart from VGs). - Display image as PS ------------------- -- 2.19.1.3.g30247aa5d201

From: "Richard W.M. Jones" <rjones@redhat.com> The two calls are identical, so this simply avoids a deprecation warning. (Picked from libguestfs commit 0b223a287711 ("test-data: Replace deprecated luks_open with cryptsetup_open.", 2021-05-27), synchronizing "make-fedora-img.pl" in guestfs-tools with the same file in libguestfs @ commit 0b223a287711.) Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- test-data/phony-guests/make-fedora-img.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-data/phony-guests/make-fedora-img.pl b/test-data/phony-guests/make-fedora-img.pl index 90492b814e2b..27b154f9d71d 100755 --- a/test-data/phony-guests/make-fedora-img.pl +++ b/test-data/phony-guests/make-fedora-img.pl @@ -195,7 +195,7 @@ EOF # Put LUKS on the second partition. $g->luks_format ('/dev/sda2', 'FEDORA', 0); - $g->luks_open ('/dev/sda2', 'FEDORA', 'luks'); + $g->cryptsetup_open ('/dev/sda2', 'FEDORA', 'luks'); init_lvm_root ('/dev/mapper/luks'); } -- 2.19.1.3.g30247aa5d201

Port libguestfs patch 'tests: add LUKS-on-LVM test' to virt-inspector. (This patch is best formatted with "--find-copies-harder".) Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- inspector/Makefile.am | 5 +- test-data/phony-guests/Makefile.am | 7 +++ .gitignore | 1 + inspector/{expected-fedora.img.xml => expected-fedora-luks-on-lvm.img.xml} | 6 +-- inspector/{test-virt-inspector-lvm-on-luks.sh => test-virt-inspector-luks-on-lvm.sh} | 15 ++++-- test-data/phony-guests/guests.xml.in | 18 +++++++ test-data/phony-guests/make-fedora-img.pl | 54 ++++++++++++++++++++ 7 files changed, 97 insertions(+), 9 deletions(-) diff --git a/inspector/Makefile.am b/inspector/Makefile.am index 961498e81788..297887bae3a4 100644 --- a/inspector/Makefile.am +++ b/inspector/Makefile.am @@ -27,12 +27,14 @@ example_xml = \ EXTRA_DIST = \ expected-debian.img.xml \ expected-fedora.img.xml \ + expected-fedora-luks-on-lvm.img.xml \ expected-fedora-lvm-on-luks.img.xml \ expected-ubuntu.img.xml \ expected-archlinux.img.xml \ expected-coreos.img.xml \ expected-windows.img.xml \ test-virt-inspector.sh \ + test-virt-inspector-luks-on-lvm.sh \ test-virt-inspector-lvm-on-luks.sh \ test-virt-inspector-docs.sh \ test-xmllint.sh \ @@ -95,6 +97,7 @@ TESTS_ENVIRONMENT = $(top_builddir)/run --test TESTS = \ test-virt-inspector-docs.sh \ test-virt-inspector.sh \ + test-virt-inspector-luks-on-lvm.sh \ test-virt-inspector-lvm-on-luks.sh if HAVE_XMLLINT @@ -102,7 +105,7 @@ TESTS += test-xmllint.sh endif check-valgrind: - $(MAKE) TESTS="test-virt-inspector.sh test-virt-inspector-lvm-on-luks.sh" VG="@VG@" check + $(MAKE) TESTS="test-virt-inspector.sh test-virt-inspector-luks-on-lvm.sh test-virt-inspector-lvm-on-luks.sh" VG="@VG@" check check-valgrind-local-guests: for g in $(GUESTS); do \ diff --git a/test-data/phony-guests/Makefile.am b/test-data/phony-guests/Makefile.am index 0114d10bb170..0a41f168bdfc 100644 --- a/test-data/phony-guests/Makefile.am +++ b/test-data/phony-guests/Makefile.am @@ -49,6 +49,7 @@ disk_images = \ fedora-md1.img \ fedora-md2.img \ fedora-btrfs.img \ + fedora-luks-on-lvm.img \ fedora-lvm-on-luks.img \ ubuntu.img \ archlinux.img \ @@ -96,6 +97,12 @@ fedora-btrfs.img: make-fedora-img.pl \ fedora.db SRCDIR=$(srcdir) LAYOUT=btrfs $(top_builddir)/run --test ./$< +# Make a (dummy) Fedora image with LUKS-on-LVM. +fedora-luks-on-lvm.img: make-fedora-img.pl \ + fedora-journal.tar.xz \ + fedora.db + SRCDIR=$(srcdir) LAYOUT=luks-on-lvm $(top_builddir)/run --test ./$< + # Make a (dummy) Fedora image with LVM-on-LUKS. fedora-lvm-on-luks.img: make-fedora-img.pl \ fedora-journal.tar.xz \ diff --git a/.gitignore b/.gitignore index 5489c0cd2a5d..87040a727a7c 100644 --- a/.gitignore +++ b/.gitignore @@ -151,6 +151,7 @@ Makefile.in /test-data/phony-guests/debian.img /test-data/phony-guests/fedora.img /test-data/phony-guests/fedora-btrfs.img +/test-data/phony-guests/fedora-luks-on-lvm.img /test-data/phony-guests/fedora-lvm-on-luks.img /test-data/phony-guests/fedora-md1.img /test-data/phony-guests/fedora-md2.img diff --git a/inspector/expected-fedora.img.xml b/inspector/expected-fedora-luks-on-lvm.img.xml similarity index 98% copy from inspector/expected-fedora.img.xml copy to inspector/expected-fedora-luks-on-lvm.img.xml index 72cddaf8816d..6e21591d8066 100644 --- a/inspector/expected-fedora.img.xml +++ b/inspector/expected-fedora-luks-on-lvm.img.xml @@ -1,7 +1,7 @@ <?xml version="1.0"?> <operatingsystems> <operatingsystem> - <root>/dev/VG/Root</root> + <root>/dev/mapper/luks-ROOTUUID</root> <name>linux</name> <arch>x86_64</arch> <distro>fedora</distro> @@ -13,11 +13,11 @@ <hostname>fedora.invalid</hostname> <osinfo>fedora14</osinfo> <mountpoints> - <mountpoint dev="/dev/VG/Root">/</mountpoint> + <mountpoint dev="/dev/mapper/luks-ROOTUUID">/</mountpoint> <mountpoint dev="/dev/sda1">/boot</mountpoint> </mountpoints> <filesystems> - <filesystem dev="/dev/VG/Root"> + <filesystem dev="/dev/mapper/luks-ROOTUUID"> <type>ext2</type> <label>ROOT</label> <uuid>01234567-0123-0123-0123-012345678902</uuid> diff --git a/inspector/test-virt-inspector-lvm-on-luks.sh b/inspector/test-virt-inspector-luks-on-lvm.sh similarity index 73% copy from inspector/test-virt-inspector-lvm-on-luks.sh copy to inspector/test-virt-inspector-luks-on-lvm.sh index 955ac6316946..b2e10232c05d 100755 --- a/inspector/test-virt-inspector-lvm-on-luks.sh +++ b/inspector/test-virt-inspector-luks-on-lvm.sh @@ -19,7 +19,7 @@ # Test that virt-inspector can work on encrypted images when the # right password is supplied. # -# Regression test for https://bugzilla.redhat.com/show_bug.cgi?id=1602353 +# Regression test for https://bugzilla.redhat.com/show_bug.cgi?id=1658126 set -e set -x @@ -27,16 +27,21 @@ set -x $TEST_FUNCTIONS skip_if_skipped -f=../test-data/phony-guests/fedora-lvm-on-luks.img +f=../test-data/phony-guests/fedora-luks-on-lvm.img +keys=(--key /dev/VG/Root:key:FEDORA-Root + --key /dev/VG/LV1:key:FEDORA-LV1 + --key /dev/VG/LV2:key:FEDORA-LV2 + --key /dev/VG/LV3:key:FEDORA-LV3) # Ignore zero-sized file. if [ -s "$f" ]; then + uuid_root=$(guestfish --ro -i -a "$f" "${keys[@]}" luks-uuid /dev/VG/Root) b=$(basename "$f") - echo FEDORA | - $VG virt-inspector --keys-from-stdin --format=raw -a "$f" > "actual-$b.xml" + $VG virt-inspector "${keys[@]}" --format=raw -a "$f" > "actual-$b.xml" # Check the generated output validate the schema. $XMLLINT --noout --relaxng "$srcdir/virt-inspector.rng" "actual-$b.xml" # This 'diff' command will fail (because of -e option) if there # are any differences. - diff -ur $diff_ignore "$srcdir/expected-$b.xml" "actual-$b.xml" + sed -e "s/ROOTUUID/$uuid_root/" < "$srcdir/expected-$b.xml" \ + | diff -u - "actual-$b.xml" fi diff --git a/test-data/phony-guests/guests.xml.in b/test-data/phony-guests/guests.xml.in index 3af6b27af073..dfcccc3d4cff 100644 --- a/test-data/phony-guests/guests.xml.in +++ b/test-data/phony-guests/guests.xml.in @@ -183,6 +183,24 @@ </devices> </domain> + <!-- LUKS passwords are 'FEDORA-Root', 'FEDORA-LV1', 'FEDORA-LV2', + 'FEDORA-LV3' --> + <domain type='test'> + <name>fedora-luks-on-lvm</name> + <memory>1048576</memory> + <os> + <type>hvm</type> + <boot dev='hd'/> + </os> + <devices> + <disk type='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='@abs_builddir@/fedora-luks-on-lvm.img'/> + <target dev='vda' bus='virtio'/> + </disk> + </devices> + </domain> + <!-- LUKS password is 'FEDORA' --> <domain type='test'> <name>fedora-lvm-on-luks</name> diff --git a/test-data/phony-guests/make-fedora-img.pl b/test-data/phony-guests/make-fedora-img.pl index 4cd6ef9575eb..84d4742e9000 100755 --- a/test-data/phony-guests/make-fedora-img.pl +++ b/test-data/phony-guests/make-fedora-img.pl @@ -200,6 +200,60 @@ EOF init_lvm_root ('/dev/mapper/luks'); } +elsif ($ENV{LAYOUT} eq 'luks-on-lvm') { + push (@images, "fedora-luks-on-lvm.img-t"); + + open (my $fstab, '>', "fedora.fstab") or die; + print $fstab <<EOF; +LABEL=BOOT /boot ext2 default 0 0 +LABEL=ROOT / ext2 default 0 0 +EOF + close ($fstab) or die; + + $bootdev = '/dev/sda1'; + + $g->disk_create ("fedora-luks-on-lvm.img-t", "raw", $IMAGE_SIZE); + + $g->add_drive ("fedora-luks-on-lvm.img-t", format => "raw"); + $g->launch (); + + $g->part_init ('/dev/sda', 'mbr'); + foreach my $p (@PARTITIONS) { + $g->part_add('/dev/sda', @$p); + } + + # Create the Volume Group on /dev/sda2. + $g->pvcreate ('/dev/sda2'); + $g->vgcreate ('VG', ['/dev/sda2']); + $g->lvcreate ('Root', 'VG', 32); + $g->lvcreate ('LV1', 'VG', 32); + $g->lvcreate ('LV2', 'VG', 32); + $g->lvcreate ('LV3', 'VG', 64); + + # Format each Logical Group as a LUKS device, with a different password. + $g->luks_format ('/dev/VG/Root', 'FEDORA-Root', 0); + $g->luks_format ('/dev/VG/LV1', 'FEDORA-LV1', 0); + $g->luks_format ('/dev/VG/LV2', 'FEDORA-LV2', 0); + $g->luks_format ('/dev/VG/LV3', 'FEDORA-LV3', 0); + + # Open the LUKS devices. This creates nodes like /dev/mapper/*-luks. + $g->cryptsetup_open ('/dev/VG/Root', 'FEDORA-Root', 'Root-luks'); + $g->cryptsetup_open ('/dev/VG/LV1', 'FEDORA-LV1', 'LV1-luks'); + $g->cryptsetup_open ('/dev/VG/LV2', 'FEDORA-LV2', 'LV2-luks'); + $g->cryptsetup_open ('/dev/VG/LV3', 'FEDORA-LV3', 'LV3-luks'); + + # Phony root filesystem. + $g->mkfs ('ext2', '/dev/mapper/Root-luks', blocksize => 4096, label => 'ROOT'); + $g->set_uuid ('/dev/mapper/Root-luks', '01234567-0123-0123-0123-012345678902'); + + # Other filesystems, just for testing findfs-label. + $g->mkfs ('ext2', '/dev/mapper/LV1-luks', blocksize => 4096, label => 'LV1'); + $g->mkfs ('ext2', '/dev/mapper/LV2-luks', blocksize => 1024, label => 'LV2'); + $g->mkfs ('ext2', '/dev/mapper/LV3-luks', blocksize => 2048, label => 'LV3'); + + $g->mount ('/dev/mapper/Root-luks', '/'); +} + else { print STDERR "$0: Unknown LAYOUT: ",$ENV{LAYOUT},"\n"; exit 1; -- 2.19.1.3.g30247aa5d201

On Wed, Feb 23, 2022 at 05:23:08PM +0100, Laszlo Ersek wrote:

The conversion may change the CPU model of the domain, which in turn can lead to the disappearance of various CPU flags/features, such as "aes" (the AES-NI instruction set).

In RHEL7, the hw-accelerated AES module "aesni_intel" does not depend on the "xts" module, whereas the built-in, non-accelerated AES module does depend on the (external) "xts" module.

As a result of the conversion, the target domain may therefore become dependent on the "xts" module, for decrypting LUKS block devices that employ the "aes-xts-plain64" cipher (which is the default LUKS cipher nowadays). When dracut runs in the appliance (in my case: dracut-055-6.fc35.x86_64), it does not seem to notice this dependency. Thus we need to explicitly include "xts" in the rebuilt initrd, if the original kernel provides that module.

https://bugzilla.redhat.com/show_bug.cgi?id=1658126 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- convert/convert_linux.ml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml index 45ce069ad9bf..116d9e06f792 100644 --- a/convert/convert_linux.ml +++ b/convert/convert_linux.ml @@ -595,6 +595,7 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ = | None -> () | Some initrd -> (* Enable the basic virtio modules in the kernel. *) + (* Also forcibly include the "xts" module; see RHBZ#1658126. *) let modules = let modules = (* The order of modules here is deliberately the same as the @@ -605,7 +606,7 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ = *) List.filter (fun m -> List.mem m kernel.ki_modules) [ "virtio"; "virtio_ring"; "virtio_blk"; - "virtio_scsi"; "virtio_net"; "virtio_pci" ] in + "virtio_scsi"; "virtio_net"; "virtio_pci"; "xts" ] in if modules <> [] then modules else (* Fallback copied from old virt-v2v. XXX Why not "ide"? *) -- 2.19.1.3.g30247aa5d201

ACK (-ed already) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v

On 02/24/22 11:48, Richard W.M. Jones wrote:

On Wed, Feb 23, 2022 at 05:23:08PM +0100, Laszlo Ersek wrote:

...

The conversion may change the CPU model of the domain, which in turn can lead to the disappearance of various CPU flags/features, such as "aes" (the AES-NI instruction set).

In RHEL7, the hw-accelerated AES module "aesni_intel" does not depend on the "xts" module, whereas the built-in, non-accelerated AES module does depend on the (external) "xts" module.

As a result of the conversion, the target domain may therefore become dependent on the "xts" module, for decrypting LUKS block devices that employ the "aes-xts-plain64" cipher (which is the default LUKS cipher nowadays). When dracut runs in the appliance (in my case: dracut-055-6.fc35.x86_64), it does not seem to notice this dependency. Thus we need to explicitly include "xts" in the rebuilt initrd, if the original kernel provides that module.

https://bugzilla.redhat.com/show_bug.cgi?id=1658126 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- convert/convert_linux.ml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml index 45ce069ad9bf..116d9e06f792 100644 --- a/convert/convert_linux.ml +++ b/convert/convert_linux.ml @@ -595,6 +595,7 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ = | None -> () | Some initrd -> (* Enable the basic virtio modules in the kernel. *) + (* Also forcibly include the "xts" module; see RHBZ#1658126. *) let modules = let modules = (* The order of modules here is deliberately the same as the @@ -605,7 +606,7 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ = *) List.filter (fun m -> List.mem m kernel.ki_modules) [ "virtio"; "virtio_ring"; "virtio_blk"; - "virtio_scsi"; "virtio_net"; "virtio_pci" ] in + "virtio_scsi"; "virtio_net"; "virtio_pci"; "xts" ] in if modules <> [] then modules else (* Fallback copied from old virt-v2v. XXX Why not "ide"? *) -- 2.19.1.3.g30247aa5d201

ACK (-ed already)

1 8ab4493faab8 Update common submodule 2 6711106145f9 convert_linux: include the "xts" module in the initrd Thanks! Laszlo