[PATCH 0/3] grub-probe: improve support of LUKS2

[PATCH 0/3] grub-probe: improve support of LUKS2

[Pierre-Louis Bonicoli]

This patch series improves support of LUKS2 for grub-probe command.

Related:
- https://lists.gnu.org/archive/html/grub-devel/2020-05/msg00238.html
- https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00017.html

Marc Dequènes (Duck) encountered this issue with Debian Bullseye while
trying to migrate from LUKS1 to LUKS2:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1000948


Pierre-Louis Bonicoli (3):
  grub-fs-tester: add luks1 and luks2 support
  commands/probe: improve support of LUKS2 devices
  grub-core/kern/disk.c: handle LUKS2 devices

 grub-core/kern/disk.c               |  4 +-
 grub-core/osdep/devmapper/getroot.c |  6 ++-
 tests/util/grub-fs-tester.in        | 58 +++++++++++++++++++++++++++--
 3 files changed, 62 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/3] grub-fs-tester: add luks1 and luks2 support

[Pierre-Louis Bonicoli]

The logical sector size used by LUKS1 is 512 bytes. LUKS2 uses 512 to
4069 bytes.

Signed-off-by: Pierre-Louis Bonicoli <pierre-louis.bonicoli@libregerbil.fr>
---
 tests/util/grub-fs-tester.in | 58 ++++++++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 3 deletions(-)

diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
index aa72b2174..7ed275fd0 100644
--- a/tests/util/grub-fs-tester.in
+++ b/tests/util/grub-fs-tester.in
@@ -15,7 +15,12 @@ XORRISOFS_CHARSET="-input-charset UTF-8 -output-charset UTF-8"
 
 # This wrapper is to ease insertion of valgrind or time statistics
 run_it () {
-    LC_ALL=C "$GRUBFSTEST" "$@"
+    case x"$fs" in
+    xluks*)
+	LC_ALL=C echo -n pass | "$GRUBFSTEST" -C "$@";;
+    *)
+	LC_ALL=C "$GRUBFSTEST" "$@";;
+    esac
 }
 
 range() {
@@ -48,6 +53,8 @@ run_grubfstest () {
 MINLOGSECSIZE=9
 MAXLOGSECSIZE=9
 case x"$fs" in
+    xluks2*)
+	MAXLOGSECSIZE=12;;
     xntfs*)
 	MINLOGSECSIZE=8
 	MAXLOGSECSIZE=12;;
@@ -335,7 +342,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
 		    #FSLABEL="g;/_é莭莽😁кит u"
 		    ;;
 	    # FS LIMITATION: reiserfs, extN and jfs label is at most 16 UTF-8 characters
-		x"reiserfs_old" | x"reiserfs" | x"ext"* | x"lvm"* | x"mdraid"* | x"jfs" | x"jfs_caseins")
+		x"reiserfs_old" | x"reiserfs" | x"ext"* | x"lvm"* | x"luks"* | x"mdraid"* | x"jfs" | x"jfs_caseins")
 		    FSLABEL="g;/éт 莭😁";;
             # FS LIMITATION: No underscore, space, semicolon, slash or international characters in UFS* in label. Limited to 32 UTF-8 characters
 		x"ufs1" | x"ufs1_sun" | x"ufs2")
@@ -804,6 +811,12 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
 		    MOUNTDEVICE="/dev/mapper/grub_test-testvol"
 		    MOUNTFS=ext2
 		    "mkfs.ext2" -L "$FSLABEL" -q "${MOUNTDEVICE}"  ;;
+		x"luks"*)
+		    echo -n pass | cryptsetup luksFormat --type $fs --sector-size $SECSIZE --pbkdf pbkdf2 $LODEVICE
+		    echo -n pass | cryptsetup open $LODEVICE grub_luks_test
+		    MOUNTDEVICE="/dev/mapper/grub_luks_test"
+		    MOUNTFS=ext2
+		    "mkfs.ext2" -L "$FSLABEL" -q "${MOUNTDEVICE}"  ;;
 		xf2fs)
 		    "mkfs.f2fs" -l "$FSLABEL" -q "${MOUNTDEVICE}" ;;
 		xnilfs2)
@@ -916,6 +929,20 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
 		    GRUBDEVICE="mduuid/`mdadm --detail --export $MOUNTDEVICE | grep MD_UUID=|sed 's,MD_UUID=,,g;s,:,,g'`";;
 		xlvm*)
 		    GRUBDEVICE="lvm/grub_test-testvol";;
+		xluks*)
+		    if test x"$fs" = xluks2 && ! (cryptsetup luksDump --dump-json-metadata $LODEVICE | grep "\"sector_size\":$SECSIZE"); then
+			    echo "Unexpected sector size for $LODEVICE (expected: $SECSIZE)"
+			    exit 1
+		    fi
+
+		    uuid=$(cryptsetup luksUUID $LODEVICE | tr -d '-')
+		    probe_uuid=$(@builddir@/grub-probe --device $MOUNTDEVICE --target=cryptodisk_uuid)
+		    if [ x"$uuid" != x"$probe_uuid" ]; then
+			echo "$fs probe command FAIL"
+			exit 1
+		    fi
+		    GRUBDEVICE="cryptouuid/${uuid}"
+		    ;;
 	    esac
 	    GRUBDIR="($GRUBDEVICE)"
 	    case x"$fs" in
@@ -1071,6 +1098,18 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
 		    sleep 1
 		    vgchange -a n grub_test
 		    ;;
+		xluks*)
+			sleep 1
+		    for try in $(range 0 20 1); do
+			if umount "$MNTPOINTRW" ; then
+			    break;
+			fi
+			sleep 1;
+		    done
+		    UMOUNT_TIME=$(date -u "+%Y-%m-%d %H:%M:%S")
+		    sleep 1
+		    cryptsetup close grub_luks_test
+		    ;;
 		xmdraid*)
 		    sleep 1
 		    for try in $(range 0 20 1); do
@@ -1117,6 +1156,10 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
 		    vgchange -a y grub_test
 		    sleep 1
 		    mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o ${MOUNTOPTS}${SELINUXOPTS}ro ;;
+		xluks*)
+		    echo -n pass | cryptsetup open $LODEVICE grub_luks_test
+		    sleep 1
+		    mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o ${MOUNTOPTS}${SELINUXOPTS}ro ;;
 		xmdraid*)
 		    mdadm --assemble /dev/md/"${fs}_$NDEVICES" $LODEVICES
 		    sleep 1
@@ -1281,7 +1324,12 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
 		exit 1
 	    fi
 
-	    LSOUT=`run_grubfstest ls -- -l "($GRUBDEVICE)"`
+	    case x"$fs" in
+		x"luks"*)
+		    LSOUT=`run_grubfstest ls -- -l "($GRUBDEVICE)"|tail -n 1`;;
+		*)
+		    LSOUT=`run_grubfstest ls -- -l "($GRUBDEVICE)"`;;
+	    esac
 	    if [ x"$NOFSLABEL" = xy ]; then
 		:
 	    elif echo "$LSOUT" | grep -F "Label \`$FSLABEL'" > /dev/null; then
@@ -1558,6 +1606,10 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
 		    vgchange -a n grub_test
 		    sleep 1
 		    ;;
+		xluks*)
+		    cryptsetup close grub_luks_test
+		    sleep 1
+		    ;;
 	    esac
 	    case x"$fs" in
 		x"tarfs" | x"cpio_"* | x"iso9660" | xrockridge | xjoliet | xrockridge_joliet | x"ziso9660" | x"romfs" | x"squash4_"* | x"iso9660_1999" | xrockridge_1999 | xjoliet_1999 | xrockridge_joliet_1999) ;;
-- 
2.34.1

[PATCH 2/3] commands/probe: improve support of LUKS2 devices

[Pierre-Louis Bonicoli]

The --target=drive option of the probe command doesn't handle LUKS2
devices:

  # dd if=/dev/zero of=data count=10 bs=1M
  # losetup --show -f data
  /dev/loop4
  # echo -n pass | cryptsetup luksFormat -v --type luks2 /dev/loop4
  Key slot 0 created.
  Command successful.
  # echo -n pass | cryptsetup -v open /dev/loop4 test
  No usable token is available.
  Key slot 0 unlocked.
  Command successful.
  # grub-probe --device /dev/mapper/test --target=drive
  (hostdisk//dev/mapper/test)

The updated output:

  # grub-probe --device /dev/mapper/test --target=drive
  (cryptouuid/be6f4e00637148a9a40453fe9cb7f0a5)

Signed-off-by: Pierre-Louis Bonicoli <pierre-louis.bonicoli@libregerbil.fr>
---
 grub-core/osdep/devmapper/getroot.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index 9ba5c9865..96781714c 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -138,7 +138,8 @@ grub_util_get_dm_abstraction (const char *os_dev)
       grub_free (uuid);
       return GRUB_DEV_ABSTRACTION_LVM;
     }
-  if (strncmp (uuid, "CRYPT-LUKS1-", 12) == 0)
+  if (strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - 1) == 0
+      || strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
     {
       grub_free (uuid);
       return GRUB_DEV_ABSTRACTION_LUKS;
-- 
2.34.1

[PATCH 3/3] grub-core/kern/disk.c: handle LUKS2 devices

[Pierre-Louis Bonicoli]

Unlike LUKS1, the sector size of LUKS2 devices isn't hardcoded.

Regarding the probe command, the following values of --target switch
are affected: abstraction, arc_hints, baremetal_hints, bios_hints,
cryptodisk_uuid, drive, efi_hints, hints_string, ieee1275_hints,
zero_check.

For example using the --target=drive option:

  # dd if=/dev/zero of=data count=10 bs=1M
  # losetup --show -f data
  /dev/loop4
  # echo -n pass | cryptsetup luksFormat -v --type luks2 /dev/loop4
  Key slot 0 created.
  Command successful.
  # echo -n pass | cryptsetup -v open /dev/loop4 test
  No usable token is available.
  Key slot 0 unlocked.
  Command successful.
  # grub-probe --device /dev/mapper/test --target=cryptodisk_uuid
  grub-probe: error: disk `cryptouuid/f353c0f04a6a4c08bc53a0896130910f' not found.

The updated output:

  # grub-probe --device /dev/mapper/test --target=cryptodisk_uuid
  f353c0f04a6a4c08bc53a0896130910f

Signed-off-by: Pierre-Louis Bonicoli <pierre-louis.bonicoli@libregerbil.fr>
---
 grub-core/kern/disk.c               | 4 +++-
 grub-core/osdep/devmapper/getroot.c | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c
index e1b0e073e..b042ceb63 100644
--- a/grub-core/kern/disk.c
+++ b/grub-core/kern/disk.c
@@ -237,8 +237,10 @@ grub_disk_open (const char *name)
 		  name);
       goto fail;
     }
-  if (disk->log_sector_size > GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS
+  if ((disk->log_sector_size > GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS
       || disk->log_sector_size < GRUB_DISK_SECTOR_BITS)
+      /* log_sector_size is unset for LUKS2 and that's ok */
+      && !(disk->log_sector_size == 0 && dev->id == GRUB_DISK_DEVICE_CRYPTODISK_ID))
     {
       grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
 		  "sector sizes of %d bytes aren't supported yet",
diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index 96781714c..4f51c113c 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -180,7 +180,8 @@ grub_util_pull_devmapper (const char *os_dev)
 	  grub_util_pull_device (subdev);
 	}
     }
-  if (uuid && strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - 1) == 0
+  if (uuid && (strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - 1) == 0
+      || strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
       && lastsubdev)
     {
       char *grdev = grub_util_get_grub_dev (lastsubdev);
-- 
2.34.1

Re: [PATCH 1/3] grub-fs-tester: add luks1 and luks2 support

[Glenn Washburn]

On Thu,  3 Feb 2022 18:21:03 +0100
Pierre-Louis Bonicoli <pierre-louis.bonicoli@libregerbil.fr> wrote:

> The logical sector size used by LUKS1 is 512 bytes. LUKS2 uses 512 to
> 4069 bytes.

I like that this has been added here. However there's no test that
actually runs this new code. Please add another two fs tests one for
LUKS and one for LUKS2.

> 
> Signed-off-by: Pierre-Louis Bonicoli <pierre-louis.bonicoli@libregerbil.fr>
> ---
>  tests/util/grub-fs-tester.in | 58 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 55 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
> index aa72b2174..7ed275fd0 100644
> --- a/tests/util/grub-fs-tester.in
> +++ b/tests/util/grub-fs-tester.in
> @@ -15,7 +15,12 @@ XORRISOFS_CHARSET="-input-charset UTF-8 -output-charset UTF-8"
>  
>  # This wrapper is to ease insertion of valgrind or time statistics
>  run_it () {
> -    LC_ALL=C "$GRUBFSTEST" "$@"
> +    case x"$fs" in
> +    xluks*)
> +	LC_ALL=C echo -n pass | "$GRUBFSTEST" -C "$@";;

Why do this here instead of leaving this function alone and doing the
pipe and -C arg below when calling run_grubfstest?

> +    *)
> +	LC_ALL=C "$GRUBFSTEST" "$@";;
> +    esac
>  }
>  
>  range() {
> @@ -48,6 +53,8 @@ run_grubfstest () {
>  MINLOGSECSIZE=9
>  MAXLOGSECSIZE=9
>  case x"$fs" in
> +    xluks2*)
> +	MAXLOGSECSIZE=12;;
>      xntfs*)
>  	MINLOGSECSIZE=8
>  	MAXLOGSECSIZE=12;;
> @@ -335,7 +342,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
>  		    #FSLABEL="g;/_é莭莽😁кит u"
>  		    ;;
>  	    # FS LIMITATION: reiserfs, extN and jfs label is at most 16 UTF-8 characters
> -		x"reiserfs_old" | x"reiserfs" | x"ext"* | x"lvm"* | x"mdraid"* | x"jfs" | x"jfs_caseins")
> +		x"reiserfs_old" | x"reiserfs" | x"ext"* | x"lvm"* | x"luks"* | x"mdraid"* | x"jfs" | x"jfs_caseins")
>  		    FSLABEL="g;/éт 莭😁";;
>              # FS LIMITATION: No underscore, space, semicolon, slash or international characters in UFS* in label. Limited to 32 UTF-8 characters
>  		x"ufs1" | x"ufs1_sun" | x"ufs2")
> @@ -804,6 +811,12 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
>  		    MOUNTDEVICE="/dev/mapper/grub_test-testvol"
>  		    MOUNTFS=ext2
>  		    "mkfs.ext2" -L "$FSLABEL" -q "${MOUNTDEVICE}"  ;;
> +		x"luks"*)
> +		    echo -n pass | cryptsetup luksFormat --type $fs --sector-size $SECSIZE --pbkdf pbkdf2 $LODEVICE
> +		    echo -n pass | cryptsetup open $LODEVICE grub_luks_test
> +		    MOUNTDEVICE="/dev/mapper/grub_luks_test"
> +		    MOUNTFS=ext2
> +		    "mkfs.ext2" -L "$FSLABEL" -q "${MOUNTDEVICE}"  ;;
>  		xf2fs)
>  		    "mkfs.f2fs" -l "$FSLABEL" -q "${MOUNTDEVICE}" ;;
>  		xnilfs2)
> @@ -916,6 +929,20 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
>  		    GRUBDEVICE="mduuid/`mdadm --detail --export $MOUNTDEVICE | grep MD_UUID=|sed 's,MD_UUID=,,g;s,:,,g'`";;
>  		xlvm*)
>  		    GRUBDEVICE="lvm/grub_test-testvol";;
> +		xluks*)
> +		    if test x"$fs" = xluks2 && ! (cryptsetup luksDump --dump-json-metadata $LODEVICE | grep "\"sector_size\":$SECSIZE"); then
> +			    echo "Unexpected sector size for $LODEVICE (expected: $SECSIZE)"
> +			    exit 1
> +		    fi
> +
> +		    uuid=$(cryptsetup luksUUID $LODEVICE | tr -d '-')
> +		    probe_uuid=$(@builddir@/grub-probe --device $MOUNTDEVICE --target=cryptodisk_uuid)
> +		    if [ x"$uuid" != x"$probe_uuid" ]; then
> +			echo "$fs probe command FAIL"
> +			exit 1
> +		    fi
> +		    GRUBDEVICE="cryptouuid/${uuid}"
> +		    ;;
>  	    esac
>  	    GRUBDIR="($GRUBDEVICE)"
>  	    case x"$fs" in
> @@ -1071,6 +1098,18 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
>  		    sleep 1
>  		    vgchange -a n grub_test
>  		    ;;
> +		xluks*)
> +			sleep 1

Bad indention. Why is this sleep desirable if there's a sleep in the
loop below?

> +		    for try in $(range 0 20 1); do
> +			if umount "$MNTPOINTRW" ; then
> +			    break;
> +			fi
> +			sleep 1;
> +		    done
> +		    UMOUNT_TIME=$(date -u "+%Y-%m-%d %H:%M:%S")
> +		    sleep 1

Why sleeping here again? We already slept in the loop above. We need
more time?

> +		    cryptsetup close grub_luks_test
> +		    ;;
>  		xmdraid*)
>  		    sleep 1
>  		    for try in $(range 0 20 1); do
> @@ -1117,6 +1156,10 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
>  		    vgchange -a y grub_test
>  		    sleep 1
>  		    mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o ${MOUNTOPTS}${SELINUXOPTS}ro ;;
> +		xluks*)
> +		    echo -n pass | cryptsetup open $LODEVICE grub_luks_test
> +		    sleep 1

Is this sleep really needed? Seems like you're trying to avoid a race,
but is the dm device ever never fully setup after cryptsetup returns?

> +		    mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o ${MOUNTOPTS}${SELINUXOPTS}ro ;;
>  		xmdraid*)
>  		    mdadm --assemble /dev/md/"${fs}_$NDEVICES" $LODEVICES
>  		    sleep 1
> @@ -1281,7 +1324,12 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
>  		exit 1
>  	    fi
>  
> -	    LSOUT=`run_grubfstest ls -- -l "($GRUBDEVICE)"`
> +	    case x"$fs" in
> +		x"luks"*)
> +		    LSOUT=`run_grubfstest ls -- -l "($GRUBDEVICE)"|tail -n 1`;;

Here is where I'm think if would be better to do the pipe and add -C.
And why is this tail needed?

> +		*)
> +		    LSOUT=`run_grubfstest ls -- -l "($GRUBDEVICE)"`;;
> +	    esac
>  	    if [ x"$NOFSLABEL" = xy ]; then
>  		:
>  	    elif echo "$LSOUT" | grep -F "Label \`$FSLABEL'" > /dev/null; then
> @@ -1558,6 +1606,10 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
>  		    vgchange -a n grub_test
>  		    sleep 1
>  		    ;;
> +		xluks*)
> +		    cryptsetup close grub_luks_test
> +		    sleep 1
> +		    ;;
>  	    esac
>  	    case x"$fs" in
>  		x"tarfs" | x"cpio_"* | x"iso9660" | xrockridge | xjoliet | xrockridge_joliet | x"ziso9660" | x"romfs" | x"squash4_"* | x"iso9660_1999" | xrockridge_1999 | xjoliet_1999 | xrockridge_joliet_1999) ;;

Glenn