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.