Laszlo Ersek
2022-Feb-25 10:59 UTC
[Libguestfs] [libguestfs-common PATCH 2/2] options: decrypt LUKS-on-LV devices
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 at redhat.com> >>> >>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126 >>> Signed-off-by: Laszlo Ersek <lersek at 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. >> >
Richard W.M. Jones
2022-Feb-28 09:00 UTC
[Libguestfs] [libguestfs-common PATCH 2/2] options: decrypt LUKS-on-LV devices
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 at redhat.com> > >>> > >>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1658126 > >>> Signed-off-by: Laszlo Ersek <lersek at 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. 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