Richard W.M. Jones
2022-Feb-24 10:43 UTC
[Libguestfs] [libguestfs-common PATCH 2/2] options: decrypt LUKS-on-LV devices
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. 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
Laszlo Ersek
2022-Feb-24 12:12 UTC
[Libguestfs] [libguestfs-common PATCH 2/2] options: decrypt LUKS-on-LV devices
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. 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. >