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

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

[Pierre-Louis Bonicoli]

v2 updates:
 * grub-fs-tester: remove extraneous 'sleep' and 'tail'
 * add LUKS and LUKS2 fs tests
 * the patches have been rebased on top of the master branch


On 09/02/2022 07.20, Glenn Washburn wrote:
> Why do this here instead of leaving this function alone and doing the
> pipe and -C arg below when calling run_grubfstest?

I kept the changes within the run_it bash function because the
run_grubfstest function is called many times and each call should have
been updated. Do you prefer it changed everywhere?


Tested with:

  $ ./bootstrap && ./configure && make
  $ make luks_test luks2_test
  $ sudo ./luks_test
  $ sudo ./luks2_test

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

 .gitignore                          |  2 ++
 Makefile.util.def                   | 12 ++++++++
 grub-core/kern/disk.c               |  4 ++-
 grub-core/osdep/devmapper/getroot.c |  6 ++--
 tests/luks2_test.in                 | 23 +++++++++++++++
 tests/luks_test.in                  | 23 +++++++++++++++
 tests/util/grub-fs-tester.in        | 46 +++++++++++++++++++++++++++--
 7 files changed, 111 insertions(+), 5 deletions(-)
 create mode 100644 tests/luks2_test.in
 create mode 100644 tests/luks_test.in

-- 
2.35.1

[PATCH v2 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.
---
 .gitignore                   |  2 ++
 Makefile.util.def            | 12 ++++++++++
 tests/luks2_test.in          | 23 ++++++++++++++++++
 tests/luks_test.in           | 23 ++++++++++++++++++
 tests/util/grub-fs-tester.in | 46 ++++++++++++++++++++++++++++++++++--
 5 files changed, 104 insertions(+), 2 deletions(-)
 create mode 100644 tests/luks2_test.in
 create mode 100644 tests/luks_test.in

diff --git a/.gitignore b/.gitignore
index f6a1bd051..9f91f4cf4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -230,6 +230,8 @@ widthspec.bin
 /lib/libgcrypt-grub
 /libgrub_a_init.c
 /lzocompress_test
+/luks_test
+/luks2_test
 /m4/
 /minixfs_test
 /missing
diff --git a/Makefile.util.def b/Makefile.util.def
index d919c562c..a910faa4c 100644
--- a/Makefile.util.def
+++ b/Makefile.util.def
@@ -1213,6 +1213,18 @@ script = {
   common = tests/syslinux_test.in;
 };
 
+script = {
+  testcase = native;
+  name = luks_test;
+  common = tests/luks_test.in;
+};
+
+script = {
+  testcase = native;
+  name = luks2_test;
+  common = tests/luks2_test.in;
+};
+
 program = {
   testcase = native;
   name = example_unit_test;
diff --git a/tests/luks2_test.in b/tests/luks2_test.in
new file mode 100644
index 000000000..6a26ba626
--- /dev/null
+++ b/tests/luks2_test.in
@@ -0,0 +1,23 @@
+#!@BUILD_SHEBANG@
+
+set -e
+
+if [ "x$EUID" = "x" ] ; then
+  EUID=`id -u`
+fi
+
+if [ "$EUID" != 0 ] ; then
+   exit 99
+fi
+
+if ! which mkfs.ext2 >/dev/null 2>&1; then
+   echo "mkfs.ext2 not installed; cannot test luks2."
+   exit 99
+fi
+
+if ! which cryptsetup >/dev/null 2>&1; then
+   echo "cryptsetup not installed; cannot test luks2."
+   exit 99
+fi
+
+"@builddir@/grub-fs-tester" luks2
diff --git a/tests/luks_test.in b/tests/luks_test.in
new file mode 100644
index 000000000..1b155e29f
--- /dev/null
+++ b/tests/luks_test.in
@@ -0,0 +1,23 @@
+#!@BUILD_SHEBANG@
+
+set -e
+
+if [ "x$EUID" = "x" ] ; then
+  EUID=`id -u`
+fi
+
+if [ "$EUID" != 0 ] ; then
+   exit 99
+fi
+
+if ! which mkfs.ext2 >/dev/null 2>&1; then
+   echo "mkfs.ext2 not installed; cannot test luks."
+   exit 99
+fi
+
+if ! which cryptsetup >/dev/null 2>&1; then
+   echo "cryptsetup not installed; cannot test luks."
+   exit 99
+fi
+
+"@builddir@/grub-fs-tester" luks
diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
index 65633c7f8..709927f5c 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 -q "\"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,15 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
 		    sleep 1
 		    vgchange -a n grub_test
 		    ;;
+		xluks*)
+		    for try in $(range 0 20 1); do
+			if umount "$MNTPOINTRW" ; then
+			    break;
+			fi
+		    done
+		    UMOUNT_TIME=$(date -u "+%Y-%m-%d %H:%M:%S")
+		    cryptsetup close grub_luks_test
+		    ;;
 		xmdraid*)
 		    sleep 1
 		    for try in $(range 0 20 1); do
@@ -1117,6 +1153,9 @@ 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
+		    mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o ${MOUNTOPTS}${SELINUXOPTS}ro ;;
 		xmdraid*)
 		    mdadm --assemble /dev/md/"${fs}_$NDEVICES" $LODEVICES
 		    sleep 1
@@ -1558,6 +1597,9 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
 		    vgchange -a n grub_test
 		    sleep 1
 		    ;;
+		xluks*)
+		    cryptsetup close grub_luks_test
+		    ;;
 	    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.35.1

[PATCH v2 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)
---
 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.35.1

[PATCH v2 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
---
 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 3a42c007b..fa3177bf0 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.35.1

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

[Daniel Kiper]

Adding Glenn and Patrick...

Pierre-Louis, may I ask you to CC both of them next time?

Glenn, Patrick, could you take a look at this patch series?

Daniel

On Tue, Mar 29, 2022 at 12:31:55PM +0200, Pierre-Louis Bonicoli wrote:
> v2 updates:
>  * grub-fs-tester: remove extraneous 'sleep' and 'tail'
>  * add LUKS and LUKS2 fs tests
>  * the patches have been rebased on top of the master branch
>
>
> On 09/02/2022 07.20, Glenn Washburn wrote:
> > Why do this here instead of leaving this function alone and doing the
> > pipe and -C arg below when calling run_grubfstest?
>
> I kept the changes within the run_it bash function because the
> run_grubfstest function is called many times and each call should have
> been updated. Do you prefer it changed everywhere?
>
>
> Tested with:
>
>   $ ./bootstrap && ./configure && make
>   $ make luks_test luks2_test
>   $ sudo ./luks_test
>   $ sudo ./luks2_test
>
> 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
>
>  .gitignore                          |  2 ++
>  Makefile.util.def                   | 12 ++++++++
>  grub-core/kern/disk.c               |  4 ++-
>  grub-core/osdep/devmapper/getroot.c |  6 ++--
>  tests/luks2_test.in                 | 23 +++++++++++++++
>  tests/luks_test.in                  | 23 +++++++++++++++
>  tests/util/grub-fs-tester.in        | 46 +++++++++++++++++++++++++++--
>  7 files changed, 111 insertions(+), 5 deletions(-)
>  create mode 100644 tests/luks2_test.in
>  create mode 100644 tests/luks_test.in
>
> --
> 2.35.1

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

[Pierre-Louis Bonicoli]

On 14/04/2022 18.11, Daniel Kiper wrote:
> Adding Glenn and Patrick...
> 
> Pierre-Louis, may I ask you to CC both of them next time?

Yes, of course :) !

-- 
Pierre-Louis

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

[Glenn Washburn]

Thanks for the update, I'd like to see this get into master. However,
while this was waiting for review some changes were made to master that
make this need some extra work. I've commented below on that and other
issues.

On Tue, 29 Mar 2022 12:31:56 +0200
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.
> ---
>  .gitignore                   |  2 ++
>  Makefile.util.def            | 12 ++++++++++
>  tests/luks2_test.in          | 23 ++++++++++++++++++
>  tests/luks_test.in           | 23 ++++++++++++++++++
>  tests/util/grub-fs-tester.in | 46 ++++++++++++++++++++++++++++++++++--
>  5 files changed, 104 insertions(+), 2 deletions(-)
>  create mode 100644 tests/luks2_test.in
>  create mode 100644 tests/luks_test.in
> 
> diff --git a/.gitignore b/.gitignore
> index f6a1bd051..9f91f4cf4 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -230,6 +230,8 @@ widthspec.bin
>  /lib/libgcrypt-grub
>  /libgrub_a_init.c
>  /lzocompress_test
> +/luks_test
> +/luks2_test
>  /m4/
>  /minixfs_test
>  /missing
> diff --git a/Makefile.util.def b/Makefile.util.def
> index d919c562c..a910faa4c 100644
> --- a/Makefile.util.def
> +++ b/Makefile.util.def
> @@ -1213,6 +1213,18 @@ script = {
>    common = tests/syslinux_test.in;
>  };
>  
> +script = {
> +  testcase = native;
> +  name = luks_test;
> +  common = tests/luks_test.in;
> +};
> +
> +script = {
> +  testcase = native;
> +  name = luks2_test;
> +  common = tests/luks2_test.in;
> +};
> +
>  program = {
>    testcase = native;
>    name = example_unit_test;
> diff --git a/tests/luks2_test.in b/tests/luks2_test.in
> new file mode 100644
> index 000000000..6a26ba626
> --- /dev/null
> +++ b/tests/luks2_test.in
> @@ -0,0 +1,23 @@
> +#!@BUILD_SHEBANG@
> +
> +set -e
> +
> +if [ "x$EUID" = "x" ] ; then
> +  EUID=`id -u`
> +fi
> +
> +if [ "$EUID" != 0 ] ; then
> +   exit 99
> +fi
> +
> +if ! which mkfs.ext2 >/dev/null 2>&1; then
> +   echo "mkfs.ext2 not installed; cannot test luks2."
> +   exit 99
> +fi
> +
> +if ! which cryptsetup >/dev/null 2>&1; then
> +   echo "cryptsetup not installed; cannot test luks2."
> +   exit 99
> +fi
> +
> +"@builddir@/grub-fs-tester" luks2
> diff --git a/tests/luks_test.in b/tests/luks_test.in
> new file mode 100644
> index 000000000..1b155e29f
> --- /dev/null
> +++ b/tests/luks_test.in
> @@ -0,0 +1,23 @@
> +#!@BUILD_SHEBANG@
> +
> +set -e
> +
> +if [ "x$EUID" = "x" ] ; then
> +  EUID=`id -u`
> +fi
> +
> +if [ "$EUID" != 0 ] ; then
> +   exit 99
> +fi
> +
> +if ! which mkfs.ext2 >/dev/null 2>&1; then
> +   echo "mkfs.ext2 not installed; cannot test luks."
> +   exit 99
> +fi
> +
> +if ! which cryptsetup >/dev/null 2>&1; then
> +   echo "cryptsetup not installed; cannot test luks."
> +   exit 99
> +fi
> +
> +"@builddir@/grub-fs-tester" luks
> diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
> index 65633c7f8..709927f5c 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 "$@";;

I wasn't looking at the totality of the changes and code when I made m
previous comment about doing this where run_grubfstest was called. So
I'll revoke the previous comment. However, I think this should be done
inside run_grubfstest() instead of here.

> +    *)
> +	LC_ALL=C "$GRUBFSTEST" "$@";;
> +    esac
>  }
>  
>  range() {
> @@ -48,6 +53,8 @@ run_grubfstest () {
>  MINLOGSECSIZE=9
>  MAXLOGSECSIZE=9
>  case x"$fs" in
> +    xluks2*)

I don't think we need the '*' here.

> +	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

Small nit, I'd like to see grub_luks_test be a shell variable defined
at the top and made reasonably unique per test run. As it is now, two
simultaneous test runs of LUKS* will use the same device, not what we
want. I'm thinking this is fine as is for now and I'll create a patch
after this is merged to do that.

> +		    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 -q "\"sector_size\":$SECSIZE"); then

Use --debug-json instead of --dump-json-metadata because the latter is
not available for cryptsetup version 2.3.7 which is what is available on
Debian 11, the test system target machine.

> +			    echo "Unexpected sector size for $LODEVICE (expected: $SECSIZE)"
> +			    exit 1

When this fails, there is no code to cleanup /dev/mapper/grub_luks_test,
the LUKS mapping. In master there is now a way to do this in the
cleanup() function near the top. Code should be added similar to ZFS
which tries to remove/close the LUKS device if it exists.

> +		    fi
> +
> +		    uuid=$(cryptsetup luksUUID $LODEVICE | tr -d '-')
> +		    probe_uuid=$(@builddir@/grub-probe --device $MOUNTDEVICE --target=cryptodisk_uuid)

These variable should be upper case to follow the (mostly) convention.
Also I would rather have @builddir@/grub-probe be $GRUBPROBE and set at
the top where GRUBFSTEST is set.

> +		    if [ x"$uuid" != x"$probe_uuid" ]; then
> +			echo "$fs probe command FAIL"

This instead should be:
  echo "UUID FAIL"
  echo "$UUID"
  echo "$PROBE_UUID"

> +			exit 1
> +		    fi
> +		    GRUBDEVICE="cryptouuid/${uuid}"

s/uuid/UUID/

> +		    ;;
>  	    esac
>  	    GRUBDIR="($GRUBDEVICE)"
>  	    case x"$fs" in
> @@ -1071,6 +1098,15 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
>  		    sleep 1
>  		    vgchange -a n grub_test
>  		    ;;
> +		xluks*)
> +		    for try in $(range 0 20 1); do
> +			if umount "$MNTPOINTRW" ; then
> +			    break;
> +			fi
> +		    done
> +		    UMOUNT_TIME=$(date -u "+%Y-%m-%d %H:%M:%S")
> +		    cryptsetup close grub_luks_test
> +		    ;;
>  		xmdraid*)
>  		    sleep 1
>  		    for try in $(range 0 20 1); do
> @@ -1117,6 +1153,9 @@ 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
> +		    mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o ${MOUNTOPTS}${SELINUXOPTS}ro ;;

After rebasing to master, there should be a line 'MOUNTS="$MOUNTS
$MNTPOINTRO"', like the cases around it.

>  		xmdraid*)
>  		    mdadm --assemble /dev/md/"${fs}_$NDEVICES" $LODEVICES
>  		    sleep 1
> @@ -1558,6 +1597,9 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
>  		    vgchange -a n grub_test
>  		    sleep 1
>  		    ;;
> +		xluks*)
> +		    cryptsetup close grub_luks_test
> +		    ;;
>  	    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

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

[Glenn Washburn]

On Tue, 29 Mar 2022 12:31:58 +0200
Pierre-Louis Bonicoli <pierre-louis.bonicoli@libregerbil.fr> wrote:

> 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
> ---
>  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 3a42c007b..fa3177bf0 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))

I don't really like this, but it gets the job done and is a work-around
for a peculiarity of the LUKS2 backend. The cheat mount code for
cryptodisk does only calls scan() and not recover_key(). For LUKS1 scan
will return a grub_cryptodisk_t with log_sector_size set, but LUKS2
will not. This is because for LUKS1 the log_sector_size is constant
(LUKS1 also sets most of the other properties of the cryptodisk device,
like crypto algorithm, because they are in the binary header). However,
for LUKS2 the sector size (along with other properties) is in the json
header, which isn't getting parsed in scan(). 

For single segment LUKS2 containers, scan() could get the sector size
from the json segment object. The LUKS2 spec says that normal LUKS2
devices are single segment[1], so this should work in the the cases the
care about (currently). scan() would not be able to fill in the other
properties, like crypto algorithm, because that depends on the keyslot
used, which needs key recovery to be determined. To avoid parsing the
json data twice, once in scan() and once in recover_key(), which should
be avoided, the parsed json object could be put in the grub_cryptodisk_t
in scan(), and used and freed in recover_key(). We'd probably also want
to add a way for grub_cryptodisk_t objects to get cleaned up by the
backend using them, so that the json object could be freed even if
recover_key() is never called.

I think the above is the real fix, a moderate amount more work, and not
something I'd expect Pierre-Louis to take up. So if we're not going to
do this to get this functionality to work, we'll need a hack to get it
working. However, I'd prefer a different one.

I've not tested this, but it seems to me that we can set the
log_sector_size field to GRUB_DISK_SECTOR_BITS _if_ it equals zero in
grub_cryptodisk_cheat_insert(). This limits the hack to only GRUB
host/user-space code.

[1] https://fossies.org/linux/cryptsetup/docs/on-disk-format-luks2.pdf,
section 3.3

>      {
>        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)

It seems better to me to not add another strncmp, but to only check for
the prefix "CRYPT-LUKS". This way when LUKS3 comes out next decade we
won't have to add another strncmp here.

If we do want to keep this, I'd like to see '||' aligned with the start
of "strncmp" of the line above, even though it will push the line past
80 chars by a few chars. At a minimum indent more than the line below.

>        && lastsubdev)
>      {
>        char *grdev = grub_util_get_grub_dev (lastsubdev);

Glenn

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

[Glenn Washburn]

On Tue, 29 Mar 2022 12:31:57 +0200
Pierre-Louis Bonicoli <pierre-louis.bonicoli@libregerbil.fr> wrote:

> 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)
> ---
>  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)

I like using the sizeof() instead of a literal. And as mentioned in
comments on patch #3, it seems better to me to not add another strncmp,
but to only check for the prefix "CRYPT-LUKS".

>      {
>        grub_free (uuid);
>        return GRUB_DEV_ABSTRACTION_LUKS;

Glenn

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

[Glenn Washburn]

On Tue, 29 Mar 2022 12:31:55 +0200
Pierre-Louis Bonicoli <pierre-louis.bonicoli@libregerbil.fr> wrote:

> v2 updates:
>  * grub-fs-tester: remove extraneous 'sleep' and 'tail'
>  * add LUKS and LUKS2 fs tests
>  * the patches have been rebased on top of the master branch
> 
> 
> On 09/02/2022 07.20, Glenn Washburn wrote:
> > Why do this here instead of leaving this function alone and doing the
> > pipe and -C arg below when calling run_grubfstest?
> 
> I kept the changes within the run_it bash function because the
> run_grubfstest function is called many times and each call should have
> been updated. Do you prefer it changed everywhere?

No, it shouldn't be changed everywhere. I've addressed this further in
my comment on the grub-fs-tester patch.

Glenn

> 
> 
> Tested with:
> 
>   $ ./bootstrap && ./configure && make
>   $ make luks_test luks2_test
>   $ sudo ./luks_test
>   $ sudo ./luks2_test
> 
> 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
> 
>  .gitignore                          |  2 ++
>  Makefile.util.def                   | 12 ++++++++
>  grub-core/kern/disk.c               |  4 ++-
>  grub-core/osdep/devmapper/getroot.c |  6 ++--
>  tests/luks2_test.in                 | 23 +++++++++++++++
>  tests/luks_test.in                  | 23 +++++++++++++++
>  tests/util/grub-fs-tester.in        | 46 +++++++++++++++++++++++++++--
>  7 files changed, 111 insertions(+), 5 deletions(-)
>  create mode 100644 tests/luks2_test.in
>  create mode 100644 tests/luks_test.in
> 

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

[Josselin Poiret]

Hello everyone,

Glenn Washburn <development@efficientek.com> writes:

> I don't really like this, but it gets the job done and is a work-around
> for a peculiarity of the LUKS2 backend. The cheat mount code for
> cryptodisk does only calls scan() and not recover_key(). For LUKS1 scan
> will return a grub_cryptodisk_t with log_sector_size set, but LUKS2
> will not. This is because for LUKS1 the log_sector_size is constant
> (LUKS1 also sets most of the other properties of the cryptodisk device,
> like crypto algorithm, because they are in the binary header). However,
> for LUKS2 the sector size (along with other properties) is in the json
> header, which isn't getting parsed in scan(). 
>
> For single segment LUKS2 containers, scan() could get the sector size
> from the json segment object. The LUKS2 spec says that normal LUKS2
> devices are single segment[1], so this should work in the the cases the
> care about (currently). scan() would not be able to fill in the other
> properties, like crypto algorithm, because that depends on the keyslot
> used, which needs key recovery to be determined. To avoid parsing the
> json data twice, once in scan() and once in recover_key(), which should
> be avoided, the parsed json object could be put in the grub_cryptodisk_t
> in scan(), and used and freed in recover_key(). We'd probably also want
> to add a way for grub_cryptodisk_t objects to get cleaned up by the
> backend using them, so that the json object could be freed even if
> recover_key() is never called.
>
> I think the above is the real fix, a moderate amount more work, and not
> something I'd expect Pierre-Louis to take up. So if we're not going to
> do this to get this functionality to work, we'll need a hack to get it
> working. However, I'd prefer a different one.
>
> I've not tested this, but it seems to me that we can set the
> log_sector_size field to GRUB_DISK_SECTOR_BITS _if_ it equals zero in
> grub_cryptodisk_cheat_insert(). This limits the hack to only GRUB
> host/user-space code.

Regarding these last lines, it's also possible to directly ask dm for
the actual sector size when cheatmounting, as well as the crypto
algorithm, bypassing the whole issue of parsing the json and finding the
right slot.  This is roughly what's done in patch 2 of [1], maybe this
workaround would be more to your liking?

I've distributed this patch to several people that were having issues on
GNU Guix and they've been happily using LUKS2 with GRUB with it.

[1] https://lists.gnu.org/archive/html/grub-devel/2021-12/msg00079.html
(20211211122945.6326-1-dev@jpoiret.xyz)

Best,
-- 
Josselin Poiret

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

[Glenn Washburn]

On Mon, 09 May 2022 22:27:30 +0200
Josselin Poiret <dev@jpoiret.xyz> wrote:

> Hello everyone,
> 
> Glenn Washburn <development@efficientek.com> writes:
> 
> > I don't really like this, but it gets the job done and is a work-around
> > for a peculiarity of the LUKS2 backend. The cheat mount code for
> > cryptodisk does only calls scan() and not recover_key(). For LUKS1 scan
> > will return a grub_cryptodisk_t with log_sector_size set, but LUKS2
> > will not. This is because for LUKS1 the log_sector_size is constant
> > (LUKS1 also sets most of the other properties of the cryptodisk device,
> > like crypto algorithm, because they are in the binary header). However,
> > for LUKS2 the sector size (along with other properties) is in the json
> > header, which isn't getting parsed in scan(). 
> >
> > For single segment LUKS2 containers, scan() could get the sector size
> > from the json segment object. The LUKS2 spec says that normal LUKS2
> > devices are single segment[1], so this should work in the the cases the
> > care about (currently). scan() would not be able to fill in the other
> > properties, like crypto algorithm, because that depends on the keyslot
> > used, which needs key recovery to be determined. To avoid parsing the
> > json data twice, once in scan() and once in recover_key(), which should
> > be avoided, the parsed json object could be put in the grub_cryptodisk_t
> > in scan(), and used and freed in recover_key(). We'd probably also want
> > to add a way for grub_cryptodisk_t objects to get cleaned up by the
> > backend using them, so that the json object could be freed even if
> > recover_key() is never called.
> >
> > I think the above is the real fix, a moderate amount more work, and not
> > something I'd expect Pierre-Louis to take up. So if we're not going to
> > do this to get this functionality to work, we'll need a hack to get it
> > working. However, I'd prefer a different one.
> >
> > I've not tested this, but it seems to me that we can set the
> > log_sector_size field to GRUB_DISK_SECTOR_BITS _if_ it equals zero in
> > grub_cryptodisk_cheat_insert(). This limits the hack to only GRUB
> > host/user-space code.
> 
> Regarding these last lines, it's also possible to directly ask dm for
> the actual sector size when cheatmounting, as well as the crypto
> algorithm, bypassing the whole issue of parsing the json and finding the
> right slot.  This is roughly what's done in patch 2 of [1], maybe this
> workaround would be more to your liking?

Hah! Yes, thanks for reminding me. I forgot I'd suggested this and its
a better approach than what I suggested above. And probably the one I'd
support, but I need to more thoroughly take a look at it.

> I've distributed this patch to several people that were having issues on
> GNU Guix and they've been happily using LUKS2 with GRUB with it.

Yes, this does work and too much of a hack as-is. Regardless, your
contribution is appreciated. I'd like to get your patch with the GRUB fs
tests merged. Do you want to make the changes I suggested and send a new
patch here? If not, at some point I'll probably make them myself and
submit it to the list.

> [1] https://lists.gnu.org/archive/html/grub-devel/2021-12/msg00079.html
> (20211211122945.6326-1-dev@jpoiret.xyz)

Glenn

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5770 bytes --]

On Wed, May 04, 2022 at 04:47:08PM -0500, Glenn Washburn wrote:
> On Tue, 29 Mar 2022 12:31:58 +0200
> Pierre-Louis Bonicoli <pierre-louis.bonicoli@libregerbil.fr> wrote:
> 
> > 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
> > ---
> >  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 3a42c007b..fa3177bf0 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))
> 
> I don't really like this, but it gets the job done and is a work-around
> for a peculiarity of the LUKS2 backend. The cheat mount code for
> cryptodisk does only calls scan() and not recover_key(). For LUKS1 scan
> will return a grub_cryptodisk_t with log_sector_size set, but LUKS2
> will not. This is because for LUKS1 the log_sector_size is constant
> (LUKS1 also sets most of the other properties of the cryptodisk device,
> like crypto algorithm, because they are in the binary header). However,
> for LUKS2 the sector size (along with other properties) is in the json
> header, which isn't getting parsed in scan(). 
> 
> For single segment LUKS2 containers, scan() could get the sector size
> from the json segment object. The LUKS2 spec says that normal LUKS2
> devices are single segment[1], so this should work in the the cases the
> care about (currently). scan() would not be able to fill in the other
> properties, like crypto algorithm, because that depends on the keyslot
> used, which needs key recovery to be determined. To avoid parsing the
> json data twice, once in scan() and once in recover_key(), which should
> be avoided, the parsed json object could be put in the grub_cryptodisk_t
> in scan(), and used and freed in recover_key(). We'd probably also want
> to add a way for grub_cryptodisk_t objects to get cleaned up by the
> backend using them, so that the json object could be freed even if
> recover_key() is never called.
> 
> I think the above is the real fix, a moderate amount more work, and not
> something I'd expect Pierre-Louis to take up. So if we're not going to
> do this to get this functionality to work, we'll need a hack to get it
> working. However, I'd prefer a different one.
> 
> I've not tested this, but it seems to me that we can set the
> log_sector_size field to GRUB_DISK_SECTOR_BITS _if_ it equals zero in
> grub_cryptodisk_cheat_insert(). This limits the hack to only GRUB
> host/user-space code.
> 
> [1] https://fossies.org/linux/cryptsetup/docs/on-disk-format-luks2.pdf,
> section 3.3
> 
> >      {
> >        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)
> 
> It seems better to me to not add another strncmp, but to only check for
> the prefix "CRYPT-LUKS". This way when LUKS3 comes out next decade we
> won't have to add another strncmp here.

I'd actually argue the other way round: I'd rather be defensive and not
pretend that we can handle LUKS3, because chances are high that we won't
handle it correctly.

Patrick

> If we do want to keep this, I'd like to see '||' aligned with the start
> of "strncmp" of the line above, even though it will push the line past
> 80 chars by a few chars. At a minimum indent more than the line below.
> 
> >        && lastsubdev)
> >      {
> >        char *grdev = grub_util_get_grub_dev (lastsubdev);
> 
> Glenn
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4961 bytes --]

On Tue, May 10, 2022 at 10:55:52PM -0500, Glenn Washburn wrote:
> On Mon, 09 May 2022 22:27:30 +0200
> Josselin Poiret <dev@jpoiret.xyz> wrote:
> 
> > Hello everyone,
> > 
> > Glenn Washburn <development@efficientek.com> writes:
> > 
> > > I don't really like this, but it gets the job done and is a work-around
> > > for a peculiarity of the LUKS2 backend. The cheat mount code for
> > > cryptodisk does only calls scan() and not recover_key(). For LUKS1 scan
> > > will return a grub_cryptodisk_t with log_sector_size set, but LUKS2
> > > will not. This is because for LUKS1 the log_sector_size is constant
> > > (LUKS1 also sets most of the other properties of the cryptodisk device,
> > > like crypto algorithm, because they are in the binary header). However,
> > > for LUKS2 the sector size (along with other properties) is in the json
> > > header, which isn't getting parsed in scan(). 
> > >
> > > For single segment LUKS2 containers, scan() could get the sector size
> > > from the json segment object. The LUKS2 spec says that normal LUKS2
> > > devices are single segment[1], so this should work in the the cases the
> > > care about (currently). scan() would not be able to fill in the other
> > > properties, like crypto algorithm, because that depends on the keyslot
> > > used, which needs key recovery to be determined. To avoid parsing the
> > > json data twice, once in scan() and once in recover_key(), which should
> > > be avoided, the parsed json object could be put in the grub_cryptodisk_t
> > > in scan(), and used and freed in recover_key(). We'd probably also want
> > > to add a way for grub_cryptodisk_t objects to get cleaned up by the
> > > backend using them, so that the json object could be freed even if
> > > recover_key() is never called.
> > >
> > > I think the above is the real fix, a moderate amount more work, and not
> > > something I'd expect Pierre-Louis to take up. So if we're not going to
> > > do this to get this functionality to work, we'll need a hack to get it
> > > working. However, I'd prefer a different one.
> > >
> > > I've not tested this, but it seems to me that we can set the
> > > log_sector_size field to GRUB_DISK_SECTOR_BITS _if_ it equals zero in
> > > grub_cryptodisk_cheat_insert(). This limits the hack to only GRUB
> > > host/user-space code.
> > 
> > Regarding these last lines, it's also possible to directly ask dm for
> > the actual sector size when cheatmounting, as well as the crypto
> > algorithm, bypassing the whole issue of parsing the json and finding the
> > right slot.  This is roughly what's done in patch 2 of [1], maybe this
> > workaround would be more to your liking?
> 
> Hah! Yes, thanks for reminding me. I forgot I'd suggested this and its
> a better approach than what I suggested above. And probably the one I'd
> support, but I need to more thoroughly take a look at it.
> 
> > I've distributed this patch to several people that were having issues on
> > GNU Guix and they've been happily using LUKS2 with GRUB with it.
> 
> Yes, this does work and too much of a hack as-is. Regardless, your
> contribution is appreciated. I'd like to get your patch with the GRUB fs
> tests merged. Do you want to make the changes I suggested and send a new
> patch here? If not, at some point I'll probably make them myself and
> submit it to the list.
> 
> > [1] https://lists.gnu.org/archive/html/grub-devel/2021-12/msg00079.html
> > (20211211122945.6326-1-dev@jpoiret.xyz)
> 
> Glenn

I very much agree that we should land the test-patches regardless of
what happens to the rest: they cover an important test gap.

Other than that the patches look sane to me. The biggest question to me
is which of the three patch series we want to include in the end:

    - Yours has the extra benefit of added tests, but these can go in
      independently.

    - Josselin's patches [1] have the benefit that they try to derive a
      "proper" sector size via device-mapper.

    - My own patches [2] include two additional patches: one to strip
      dashes of the UUID so that findfs is easier to use and the same
      across LUKS and LUKS2. And one out-of-bounds copy of the UUID in
      LUKS. Both are kind of orthogonal though. One more thing I like
      better about this patch series is that it clearly discerns LUKS
      and LUKS2 devices.

So ultimately it feels like all of the patch series have their own
advantages, but they should be combinable. The tests and my own
orthogonal patches can be split out. And if we combined the approach in
Josselin's patches to use DM to get the sector size with a proper
conceptual split of LUKS and LUKS2 as in my own patches then I'd be more
than happy.

I may very well be biased here though given that one of the patch series
is my own.

Patrick

[1]: <20220520182039.21654-1-dev@jpoiret.xyz>
[2]: <cover.1590840835.git.ps@pks.im>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Glenn Washburn]

On Sun, 29 May 2022 08:58:20 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Wed, May 04, 2022 at 04:47:08PM -0500, Glenn Washburn wrote:
> > On Tue, 29 Mar 2022 12:31:58 +0200
> > Pierre-Louis Bonicoli <pierre-louis.bonicoli@libregerbil.fr> wrote:
> > 
> > > 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
> > > ---
> > >  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 3a42c007b..fa3177bf0 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))
> > 
> > I don't really like this, but it gets the job done and is a work-around
> > for a peculiarity of the LUKS2 backend. The cheat mount code for
> > cryptodisk does only calls scan() and not recover_key(). For LUKS1 scan
> > will return a grub_cryptodisk_t with log_sector_size set, but LUKS2
> > will not. This is because for LUKS1 the log_sector_size is constant
> > (LUKS1 also sets most of the other properties of the cryptodisk device,
> > like crypto algorithm, because they are in the binary header). However,
> > for LUKS2 the sector size (along with other properties) is in the json
> > header, which isn't getting parsed in scan(). 
> > 
> > For single segment LUKS2 containers, scan() could get the sector size
> > from the json segment object. The LUKS2 spec says that normal LUKS2
> > devices are single segment[1], so this should work in the the cases the
> > care about (currently). scan() would not be able to fill in the other
> > properties, like crypto algorithm, because that depends on the keyslot
> > used, which needs key recovery to be determined. To avoid parsing the
> > json data twice, once in scan() and once in recover_key(), which should
> > be avoided, the parsed json object could be put in the grub_cryptodisk_t
> > in scan(), and used and freed in recover_key(). We'd probably also want
> > to add a way for grub_cryptodisk_t objects to get cleaned up by the
> > backend using them, so that the json object could be freed even if
> > recover_key() is never called.
> > 
> > I think the above is the real fix, a moderate amount more work, and not
> > something I'd expect Pierre-Louis to take up. So if we're not going to
> > do this to get this functionality to work, we'll need a hack to get it
> > working. However, I'd prefer a different one.
> > 
> > I've not tested this, but it seems to me that we can set the
> > log_sector_size field to GRUB_DISK_SECTOR_BITS _if_ it equals zero in
> > grub_cryptodisk_cheat_insert(). This limits the hack to only GRUB
> > host/user-space code.
> > 
> > [1] https://fossies.org/linux/cryptsetup/docs/on-disk-format-luks2.pdf,
> > section 3.3
> > 
> > >      {
> > >        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)
> > 
> > It seems better to me to not add another strncmp, but to only check for
> > the prefix "CRYPT-LUKS". This way when LUKS3 comes out next decade we
> > won't have to add another strncmp here.
> 
> I'd actually argue the other way round: I'd rather be defensive and not
> pretend that we can handle LUKS3, because chances are high that we won't
> handle it correctly.

I think its fine because there should be a failure or non-detection
elsewhere. If a user has a LUKS3 volume and runs grub-probe with an
installed GRUB that does not have support for LUKS3, then
grub_cryptodisk_cheat_mount() will not call
grub_cryptodisk_cheat_insert() because no cryptodisk backend modules
are successfully scan the LUKS3 volume. But, I won't press this issue.

Glenn

> 
> Patrick
> 
> > If we do want to keep this, I'd like to see '||' aligned with the start
> > of "strncmp" of the line above, even though it will push the line past
> > 80 chars by a few chars. At a minimum indent more than the line below.
> > 
> > >        && lastsubdev)
> > >      {
> > >        char *grdev = grub_util_get_grub_dev (lastsubdev);
> > 
> > Glenn
> > 
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel

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

[Glenn Washburn]

On Sun, 29 May 2022 09:09:38 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Tue, May 10, 2022 at 10:55:52PM -0500, Glenn Washburn wrote:
> > On Mon, 09 May 2022 22:27:30 +0200
> > Josselin Poiret <dev@jpoiret.xyz> wrote:
> > 
> > > Hello everyone,
> > > 
> > > Glenn Washburn <development@efficientek.com> writes:
> > > 
> > > > I don't really like this, but it gets the job done and is a work-around
> > > > for a peculiarity of the LUKS2 backend. The cheat mount code for
> > > > cryptodisk does only calls scan() and not recover_key(). For LUKS1 scan
> > > > will return a grub_cryptodisk_t with log_sector_size set, but LUKS2
> > > > will not. This is because for LUKS1 the log_sector_size is constant
> > > > (LUKS1 also sets most of the other properties of the cryptodisk device,
> > > > like crypto algorithm, because they are in the binary header). However,
> > > > for LUKS2 the sector size (along with other properties) is in the json
> > > > header, which isn't getting parsed in scan(). 
> > > >
> > > > For single segment LUKS2 containers, scan() could get the sector size
> > > > from the json segment object. The LUKS2 spec says that normal LUKS2
> > > > devices are single segment[1], so this should work in the the cases the
> > > > care about (currently). scan() would not be able to fill in the other
> > > > properties, like crypto algorithm, because that depends on the keyslot
> > > > used, which needs key recovery to be determined. To avoid parsing the
> > > > json data twice, once in scan() and once in recover_key(), which should
> > > > be avoided, the parsed json object could be put in the grub_cryptodisk_t
> > > > in scan(), and used and freed in recover_key(). We'd probably also want
> > > > to add a way for grub_cryptodisk_t objects to get cleaned up by the
> > > > backend using them, so that the json object could be freed even if
> > > > recover_key() is never called.
> > > >
> > > > I think the above is the real fix, a moderate amount more work, and not
> > > > something I'd expect Pierre-Louis to take up. So if we're not going to
> > > > do this to get this functionality to work, we'll need a hack to get it
> > > > working. However, I'd prefer a different one.
> > > >
> > > > I've not tested this, but it seems to me that we can set the
> > > > log_sector_size field to GRUB_DISK_SECTOR_BITS _if_ it equals zero in
> > > > grub_cryptodisk_cheat_insert(). This limits the hack to only GRUB
> > > > host/user-space code.
> > > 
> > > Regarding these last lines, it's also possible to directly ask dm for
> > > the actual sector size when cheatmounting, as well as the crypto
> > > algorithm, bypassing the whole issue of parsing the json and finding the
> > > right slot.  This is roughly what's done in patch 2 of [1], maybe this
> > > workaround would be more to your liking?
> > 
> > Hah! Yes, thanks for reminding me. I forgot I'd suggested this and its
> > a better approach than what I suggested above. And probably the one I'd
> > support, but I need to more thoroughly take a look at it.
> > 
> > > I've distributed this patch to several people that were having issues on
> > > GNU Guix and they've been happily using LUKS2 with GRUB with it.
> > 
> > Yes, this does work and too much of a hack as-is. Regardless, your
> > contribution is appreciated. I'd like to get your patch with the GRUB fs
> > tests merged. Do you want to make the changes I suggested and send a new
> > patch here? If not, at some point I'll probably make them myself and
> > submit it to the list.
> > 
> > > [1] https://lists.gnu.org/archive/html/grub-devel/2021-12/msg00079.html
> > > (20211211122945.6326-1-dev@jpoiret.xyz)
> > 
> > Glenn
> 
> I very much agree that we should land the test-patches regardless of
> what happens to the rest: they cover an important test gap.
> 
> Other than that the patches look sane to me. The biggest question to me
> is which of the three patch series we want to include in the end:
> 
>     - Yours has the extra benefit of added tests, but these can go in
>       independently.
> 
>     - Josselin's patches [1] have the benefit that they try to derive a
>       "proper" sector size via device-mapper.
> 
>     - My own patches [2] include two additional patches: one to strip
>       dashes of the UUID so that findfs is easier to use and the same
>       across LUKS and LUKS2. And one out-of-bounds copy of the UUID in
>       LUKS. Both are kind of orthogonal though. One more thing I like
>       better about this patch series is that it clearly discerns LUKS
>       and LUKS2 devices.
> 
> So ultimately it feels like all of the patch series have their own
> advantages, but they should be combinable. The tests and my own
> orthogonal patches can be split out. And if we combined the approach in
> Josselin's patches to use DM to get the sector size with a proper
> conceptual split of LUKS and LUKS2 as in my own patches then I'd be more
> than happy.

I think[1] Fabian's patch[2] for getting sector size is actually better
and more general than using DM. My preference is for a combination of
Fabian's and Josselin's patches.

Your response made me question how we could get the correct luks module
to put in the grub config if we don't have a separate
GRUB_DEV_ABSTRACTION_LUKS2. But actually that code uses the command
"grub-probe --device $@ --target=abstraction, which calls
probe_abstraction() -> grub_util_cryptodisk_get_abstraction(), which
then uses the "modname" member of the grub_cryptodisk_t device. So
GRUB_DEV_ABSTRACTION_* is not used, nor needed in this case. When you
do make use of GRUB_DEV_ABSTRACTION_LUKS2, its just to duplicate
existing code. I think its best to not have a separate
GRUB_DEV_ABSTRACTION_LUKS2.

Glenn

[1] https://lists.gnu.org/archive/html/grub-devel/2022-05/msg00184.html
[2] https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00017.html

> 
> I may very well be biased here though given that one of the patch series
> is my own.
> 
> Patrick
> 
> [1]: <20220520182039.21654-1-dev@jpoiret.xyz>
> [2]: <cover.1590840835.git.ps@pks.im>

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6713 bytes --]

On Sun, Jun 05, 2022 at 01:43:18PM -0500, Glenn Washburn wrote:
> On Sun, 29 May 2022 09:09:38 +0200
> Patrick Steinhardt <ps@pks.im> wrote:
> 
> > On Tue, May 10, 2022 at 10:55:52PM -0500, Glenn Washburn wrote:
> > > On Mon, 09 May 2022 22:27:30 +0200
> > > Josselin Poiret <dev@jpoiret.xyz> wrote:
> > > 
> > > > Hello everyone,
> > > > 
> > > > Glenn Washburn <development@efficientek.com> writes:
> > > > 
> > > > > I don't really like this, but it gets the job done and is a work-around
> > > > > for a peculiarity of the LUKS2 backend. The cheat mount code for
> > > > > cryptodisk does only calls scan() and not recover_key(). For LUKS1 scan
> > > > > will return a grub_cryptodisk_t with log_sector_size set, but LUKS2
> > > > > will not. This is because for LUKS1 the log_sector_size is constant
> > > > > (LUKS1 also sets most of the other properties of the cryptodisk device,
> > > > > like crypto algorithm, because they are in the binary header). However,
> > > > > for LUKS2 the sector size (along with other properties) is in the json
> > > > > header, which isn't getting parsed in scan(). 
> > > > >
> > > > > For single segment LUKS2 containers, scan() could get the sector size
> > > > > from the json segment object. The LUKS2 spec says that normal LUKS2
> > > > > devices are single segment[1], so this should work in the the cases the
> > > > > care about (currently). scan() would not be able to fill in the other
> > > > > properties, like crypto algorithm, because that depends on the keyslot
> > > > > used, which needs key recovery to be determined. To avoid parsing the
> > > > > json data twice, once in scan() and once in recover_key(), which should
> > > > > be avoided, the parsed json object could be put in the grub_cryptodisk_t
> > > > > in scan(), and used and freed in recover_key(). We'd probably also want
> > > > > to add a way for grub_cryptodisk_t objects to get cleaned up by the
> > > > > backend using them, so that the json object could be freed even if
> > > > > recover_key() is never called.
> > > > >
> > > > > I think the above is the real fix, a moderate amount more work, and not
> > > > > something I'd expect Pierre-Louis to take up. So if we're not going to
> > > > > do this to get this functionality to work, we'll need a hack to get it
> > > > > working. However, I'd prefer a different one.
> > > > >
> > > > > I've not tested this, but it seems to me that we can set the
> > > > > log_sector_size field to GRUB_DISK_SECTOR_BITS _if_ it equals zero in
> > > > > grub_cryptodisk_cheat_insert(). This limits the hack to only GRUB
> > > > > host/user-space code.
> > > > 
> > > > Regarding these last lines, it's also possible to directly ask dm for
> > > > the actual sector size when cheatmounting, as well as the crypto
> > > > algorithm, bypassing the whole issue of parsing the json and finding the
> > > > right slot.  This is roughly what's done in patch 2 of [1], maybe this
> > > > workaround would be more to your liking?
> > > 
> > > Hah! Yes, thanks for reminding me. I forgot I'd suggested this and its
> > > a better approach than what I suggested above. And probably the one I'd
> > > support, but I need to more thoroughly take a look at it.
> > > 
> > > > I've distributed this patch to several people that were having issues on
> > > > GNU Guix and they've been happily using LUKS2 with GRUB with it.
> > > 
> > > Yes, this does work and too much of a hack as-is. Regardless, your
> > > contribution is appreciated. I'd like to get your patch with the GRUB fs
> > > tests merged. Do you want to make the changes I suggested and send a new
> > > patch here? If not, at some point I'll probably make them myself and
> > > submit it to the list.
> > > 
> > > > [1] https://lists.gnu.org/archive/html/grub-devel/2021-12/msg00079.html
> > > > (20211211122945.6326-1-dev@jpoiret.xyz)
> > > 
> > > Glenn
> > 
> > I very much agree that we should land the test-patches regardless of
> > what happens to the rest: they cover an important test gap.
> > 
> > Other than that the patches look sane to me. The biggest question to me
> > is which of the three patch series we want to include in the end:
> > 
> >     - Yours has the extra benefit of added tests, but these can go in
> >       independently.
> > 
> >     - Josselin's patches [1] have the benefit that they try to derive a
> >       "proper" sector size via device-mapper.
> > 
> >     - My own patches [2] include two additional patches: one to strip
> >       dashes of the UUID so that findfs is easier to use and the same
> >       across LUKS and LUKS2. And one out-of-bounds copy of the UUID in
> >       LUKS. Both are kind of orthogonal though. One more thing I like
> >       better about this patch series is that it clearly discerns LUKS
> >       and LUKS2 devices.
> > 
> > So ultimately it feels like all of the patch series have their own
> > advantages, but they should be combinable. The tests and my own
> > orthogonal patches can be split out. And if we combined the approach in
> > Josselin's patches to use DM to get the sector size with a proper
> > conceptual split of LUKS and LUKS2 as in my own patches then I'd be more
> > than happy.
> 
> I think[1] Fabian's patch[2] for getting sector size is actually better
> and more general than using DM. My preference is for a combination of
> Fabian's and Josselin's patches.

I also noted that my own two orthogonal patches to fix the out-of-bounds
copy and dash-stripping have landed in `master` already. So I agree that
a combination of the other two series would be best.

Patrick

> Your response made me question how we could get the correct luks module
> to put in the grub config if we don't have a separate
> GRUB_DEV_ABSTRACTION_LUKS2. But actually that code uses the command
> "grub-probe --device $@ --target=abstraction, which calls
> probe_abstraction() -> grub_util_cryptodisk_get_abstraction(), which
> then uses the "modname" member of the grub_cryptodisk_t device. So
> GRUB_DEV_ABSTRACTION_* is not used, nor needed in this case. When you
> do make use of GRUB_DEV_ABSTRACTION_LUKS2, its just to duplicate
> existing code. I think its best to not have a separate
> GRUB_DEV_ABSTRACTION_LUKS2.
> 
> Glenn
> 
> [1] https://lists.gnu.org/archive/html/grub-devel/2022-05/msg00184.html
> [2] https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00017.html
> 
> > 
> > I may very well be biased here though given that one of the patch series
> > is my own.
> > 
> > Patrick
> > 
> > [1]: <20220520182039.21654-1-dev@jpoiret.xyz>
> > [2]: <cover.1590840835.git.ps@pks.im>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Glenn Washburn]

On Mon, 6 Jun 2022 07:32:40 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Sun, Jun 05, 2022 at 01:43:18PM -0500, Glenn Washburn wrote:
> > On Sun, 29 May 2022 09:09:38 +0200
> > Patrick Steinhardt <ps@pks.im> wrote:
> > 
> > > On Tue, May 10, 2022 at 10:55:52PM -0500, Glenn Washburn wrote:
> > > > On Mon, 09 May 2022 22:27:30 +0200
> > > > Josselin Poiret <dev@jpoiret.xyz> wrote:
> > > > 
> > > > > Hello everyone,
> > > > > 
> > > > > Glenn Washburn <development@efficientek.com> writes:
> > > > > 
> > > > > > I don't really like this, but it gets the job done and is a work-around
> > > > > > for a peculiarity of the LUKS2 backend. The cheat mount code for
> > > > > > cryptodisk does only calls scan() and not recover_key(). For LUKS1 scan
> > > > > > will return a grub_cryptodisk_t with log_sector_size set, but LUKS2
> > > > > > will not. This is because for LUKS1 the log_sector_size is constant
> > > > > > (LUKS1 also sets most of the other properties of the cryptodisk device,
> > > > > > like crypto algorithm, because they are in the binary header). However,
> > > > > > for LUKS2 the sector size (along with other properties) is in the json
> > > > > > header, which isn't getting parsed in scan(). 
> > > > > >
> > > > > > For single segment LUKS2 containers, scan() could get the sector size
> > > > > > from the json segment object. The LUKS2 spec says that normal LUKS2
> > > > > > devices are single segment[1], so this should work in the the cases the
> > > > > > care about (currently). scan() would not be able to fill in the other
> > > > > > properties, like crypto algorithm, because that depends on the keyslot
> > > > > > used, which needs key recovery to be determined. To avoid parsing the
> > > > > > json data twice, once in scan() and once in recover_key(), which should
> > > > > > be avoided, the parsed json object could be put in the grub_cryptodisk_t
> > > > > > in scan(), and used and freed in recover_key(). We'd probably also want
> > > > > > to add a way for grub_cryptodisk_t objects to get cleaned up by the
> > > > > > backend using them, so that the json object could be freed even if
> > > > > > recover_key() is never called.
> > > > > >
> > > > > > I think the above is the real fix, a moderate amount more work, and not
> > > > > > something I'd expect Pierre-Louis to take up. So if we're not going to
> > > > > > do this to get this functionality to work, we'll need a hack to get it
> > > > > > working. However, I'd prefer a different one.
> > > > > >
> > > > > > I've not tested this, but it seems to me that we can set the
> > > > > > log_sector_size field to GRUB_DISK_SECTOR_BITS _if_ it equals zero in
> > > > > > grub_cryptodisk_cheat_insert(). This limits the hack to only GRUB
> > > > > > host/user-space code.
> > > > > 
> > > > > Regarding these last lines, it's also possible to directly ask dm for
> > > > > the actual sector size when cheatmounting, as well as the crypto
> > > > > algorithm, bypassing the whole issue of parsing the json and finding the
> > > > > right slot.  This is roughly what's done in patch 2 of [1], maybe this
> > > > > workaround would be more to your liking?
> > > > 
> > > > Hah! Yes, thanks for reminding me. I forgot I'd suggested this and its
> > > > a better approach than what I suggested above. And probably the one I'd
> > > > support, but I need to more thoroughly take a look at it.
> > > > 
> > > > > I've distributed this patch to several people that were having issues on
> > > > > GNU Guix and they've been happily using LUKS2 with GRUB with it.
> > > > 
> > > > Yes, this does work and too much of a hack as-is. Regardless, your
> > > > contribution is appreciated. I'd like to get your patch with the GRUB fs
> > > > tests merged. Do you want to make the changes I suggested and send a new
> > > > patch here? If not, at some point I'll probably make them myself and
> > > > submit it to the list.
> > > > 
> > > > > [1] https://lists.gnu.org/archive/html/grub-devel/2021-12/msg00079.html
> > > > > (20211211122945.6326-1-dev@jpoiret.xyz)
> > > > 
> > > > Glenn
> > > 
> > > I very much agree that we should land the test-patches regardless of
> > > what happens to the rest: they cover an important test gap.
> > > 
> > > Other than that the patches look sane to me. The biggest question to me
> > > is which of the three patch series we want to include in the end:
> > > 
> > >     - Yours has the extra benefit of added tests, but these can go in
> > >       independently.
> > > 
> > >     - Josselin's patches [1] have the benefit that they try to derive a
> > >       "proper" sector size via device-mapper.
> > > 
> > >     - My own patches [2] include two additional patches: one to strip
> > >       dashes of the UUID so that findfs is easier to use and the same
> > >       across LUKS and LUKS2. And one out-of-bounds copy of the UUID in
> > >       LUKS. Both are kind of orthogonal though. One more thing I like
> > >       better about this patch series is that it clearly discerns LUKS
> > >       and LUKS2 devices.
> > > 
> > > So ultimately it feels like all of the patch series have their own
> > > advantages, but they should be combinable. The tests and my own
> > > orthogonal patches can be split out. And if we combined the approach in
> > > Josselin's patches to use DM to get the sector size with a proper
> > > conceptual split of LUKS and LUKS2 as in my own patches then I'd be more
> > > than happy.
> > 
> > I think[1] Fabian's patch[2] for getting sector size is actually better
> > and more general than using DM. My preference is for a combination of
> > Fabian's and Josselin's patches.
> 
> I also noted that my own two orthogonal patches to fix the out-of-bounds
> copy and dash-stripping have landed in `master` already. So I agree that
> a combination of the other two series would be best.

I think those should be a different series, since they are orthogonal.
I prefer my patch series for handling uuids[1], which allows searching
with dashes. This would allow the user to search using uuids that
contain dashes (or without dashes for backwards compatibility), which
would be consistent with filesystem uuid searching.

Glenn

[1] https://lists.gnu.org/archive/html/grub-devel/2022-02/msg00101.html

> 
> Patrick
> 
> > Your response made me question how we could get the correct luks module
> > to put in the grub config if we don't have a separate
> > GRUB_DEV_ABSTRACTION_LUKS2. But actually that code uses the command
> > "grub-probe --device $@ --target=abstraction, which calls
> > probe_abstraction() -> grub_util_cryptodisk_get_abstraction(), which
> > then uses the "modname" member of the grub_cryptodisk_t device. So
> > GRUB_DEV_ABSTRACTION_* is not used, nor needed in this case. When you
> > do make use of GRUB_DEV_ABSTRACTION_LUKS2, its just to duplicate
> > existing code. I think its best to not have a separate
> > GRUB_DEV_ABSTRACTION_LUKS2.
> > 
> > Glenn
> > 
> > [1] https://lists.gnu.org/archive/html/grub-devel/2022-05/msg00184.html
> > [2] https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00017.html
> > 
> > > 
> > > I may very well be biased here though given that one of the patch series
> > > is my own.
> > > 
> > > Patrick
> > > 
> > > [1]: <20220520182039.21654-1-dev@jpoiret.xyz>
> > > [2]: <cover.1590840835.git.ps@pks.im>

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6960 bytes --]

On Mon, Jun 06, 2022 at 12:11:39PM -0500, Glenn Washburn wrote:
> On Mon, 6 Jun 2022 07:32:40 +0200
> Patrick Steinhardt <ps@pks.im> wrote:
> 
> > On Sun, Jun 05, 2022 at 01:43:18PM -0500, Glenn Washburn wrote:
> > > On Sun, 29 May 2022 09:09:38 +0200
> > > Patrick Steinhardt <ps@pks.im> wrote:
> > > 
> > > > On Tue, May 10, 2022 at 10:55:52PM -0500, Glenn Washburn wrote:
> > > > > On Mon, 09 May 2022 22:27:30 +0200
> > > > > Josselin Poiret <dev@jpoiret.xyz> wrote:
> > > > > 
> > > > > > Hello everyone,
> > > > > > 
> > > > > > Glenn Washburn <development@efficientek.com> writes:
> > > > > > 
> > > > > > > I don't really like this, but it gets the job done and is a work-around
> > > > > > > for a peculiarity of the LUKS2 backend. The cheat mount code for
> > > > > > > cryptodisk does only calls scan() and not recover_key(). For LUKS1 scan
> > > > > > > will return a grub_cryptodisk_t with log_sector_size set, but LUKS2
> > > > > > > will not. This is because for LUKS1 the log_sector_size is constant
> > > > > > > (LUKS1 also sets most of the other properties of the cryptodisk device,
> > > > > > > like crypto algorithm, because they are in the binary header). However,
> > > > > > > for LUKS2 the sector size (along with other properties) is in the json
> > > > > > > header, which isn't getting parsed in scan(). 
> > > > > > >
> > > > > > > For single segment LUKS2 containers, scan() could get the sector size
> > > > > > > from the json segment object. The LUKS2 spec says that normal LUKS2
> > > > > > > devices are single segment[1], so this should work in the the cases the
> > > > > > > care about (currently). scan() would not be able to fill in the other
> > > > > > > properties, like crypto algorithm, because that depends on the keyslot
> > > > > > > used, which needs key recovery to be determined. To avoid parsing the
> > > > > > > json data twice, once in scan() and once in recover_key(), which should
> > > > > > > be avoided, the parsed json object could be put in the grub_cryptodisk_t
> > > > > > > in scan(), and used and freed in recover_key(). We'd probably also want
> > > > > > > to add a way for grub_cryptodisk_t objects to get cleaned up by the
> > > > > > > backend using them, so that the json object could be freed even if
> > > > > > > recover_key() is never called.
> > > > > > >
> > > > > > > I think the above is the real fix, a moderate amount more work, and not
> > > > > > > something I'd expect Pierre-Louis to take up. So if we're not going to
> > > > > > > do this to get this functionality to work, we'll need a hack to get it
> > > > > > > working. However, I'd prefer a different one.
> > > > > > >
> > > > > > > I've not tested this, but it seems to me that we can set the
> > > > > > > log_sector_size field to GRUB_DISK_SECTOR_BITS _if_ it equals zero in
> > > > > > > grub_cryptodisk_cheat_insert(). This limits the hack to only GRUB
> > > > > > > host/user-space code.
> > > > > > 
> > > > > > Regarding these last lines, it's also possible to directly ask dm for
> > > > > > the actual sector size when cheatmounting, as well as the crypto
> > > > > > algorithm, bypassing the whole issue of parsing the json and finding the
> > > > > > right slot.  This is roughly what's done in patch 2 of [1], maybe this
> > > > > > workaround would be more to your liking?
> > > > > 
> > > > > Hah! Yes, thanks for reminding me. I forgot I'd suggested this and its
> > > > > a better approach than what I suggested above. And probably the one I'd
> > > > > support, but I need to more thoroughly take a look at it.
> > > > > 
> > > > > > I've distributed this patch to several people that were having issues on
> > > > > > GNU Guix and they've been happily using LUKS2 with GRUB with it.
> > > > > 
> > > > > Yes, this does work and too much of a hack as-is. Regardless, your
> > > > > contribution is appreciated. I'd like to get your patch with the GRUB fs
> > > > > tests merged. Do you want to make the changes I suggested and send a new
> > > > > patch here? If not, at some point I'll probably make them myself and
> > > > > submit it to the list.
> > > > > 
> > > > > > [1] https://lists.gnu.org/archive/html/grub-devel/2021-12/msg00079.html
> > > > > > (20211211122945.6326-1-dev@jpoiret.xyz)
> > > > > 
> > > > > Glenn
> > > > 
> > > > I very much agree that we should land the test-patches regardless of
> > > > what happens to the rest: they cover an important test gap.
> > > > 
> > > > Other than that the patches look sane to me. The biggest question to me
> > > > is which of the three patch series we want to include in the end:
> > > > 
> > > >     - Yours has the extra benefit of added tests, but these can go in
> > > >       independently.
> > > > 
> > > >     - Josselin's patches [1] have the benefit that they try to derive a
> > > >       "proper" sector size via device-mapper.
> > > > 
> > > >     - My own patches [2] include two additional patches: one to strip
> > > >       dashes of the UUID so that findfs is easier to use and the same
> > > >       across LUKS and LUKS2. And one out-of-bounds copy of the UUID in
> > > >       LUKS. Both are kind of orthogonal though. One more thing I like
> > > >       better about this patch series is that it clearly discerns LUKS
> > > >       and LUKS2 devices.
> > > > 
> > > > So ultimately it feels like all of the patch series have their own
> > > > advantages, but they should be combinable. The tests and my own
> > > > orthogonal patches can be split out. And if we combined the approach in
> > > > Josselin's patches to use DM to get the sector size with a proper
> > > > conceptual split of LUKS and LUKS2 as in my own patches then I'd be more
> > > > than happy.
> > > 
> > > I think[1] Fabian's patch[2] for getting sector size is actually better
> > > and more general than using DM. My preference is for a combination of
> > > Fabian's and Josselin's patches.
> > 
> > I also noted that my own two orthogonal patches to fix the out-of-bounds
> > copy and dash-stripping have landed in `master` already. So I agree that
> > a combination of the other two series would be best.
> 
> I think those should be a different series, since they are orthogonal.
> I prefer my patch series for handling uuids[1], which allows searching
> with dashes. This would allow the user to search using uuids that
> contain dashes (or without dashes for backwards compatibility), which
> would be consistent with filesystem uuid searching.
> 
> Glenn
> 
> [1] https://lists.gnu.org/archive/html/grub-devel/2022-02/msg00101.html

I think we misunderstood. The two patches already _are_ in the `master`
branch:

    - ee12785f7 (luks2: Strip dashes off of the UUID, 2020-05-30)
    - 1066336dc (luks: Fix out-of-bounds copy of UUID, 2020-09-07)

So there's nothing that needs to be done here anymore.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]