* [PATCH v2 0/3] grub-probe: improve support of LUKS2
@ 2022-03-29 10:31 Pierre-Louis Bonicoli
2022-03-29 10:31 ` [PATCH v2 1/3] grub-fs-tester: add luks1 and luks2 support Pierre-Louis Bonicoli
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Pierre-Louis Bonicoli @ 2022-03-29 10:31 UTC (permalink / raw)
To: grub-devel
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
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v2 1/3] grub-fs-tester: add luks1 and luks2 support 2022-03-29 10:31 [PATCH v2 0/3] grub-probe: improve support of LUKS2 Pierre-Louis Bonicoli @ 2022-03-29 10:31 ` Pierre-Louis Bonicoli 2022-05-04 17:25 ` Glenn Washburn 2022-03-29 10:31 ` [PATCH v2 2/3] commands/probe: improve support of LUKS2 devices Pierre-Louis Bonicoli ` (3 subsequent siblings) 4 siblings, 1 reply; 19+ messages in thread From: Pierre-Louis Bonicoli @ 2022-03-29 10:31 UTC (permalink / raw) To: grub-devel 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/3] grub-fs-tester: add luks1 and luks2 support 2022-03-29 10:31 ` [PATCH v2 1/3] grub-fs-tester: add luks1 and luks2 support Pierre-Louis Bonicoli @ 2022-05-04 17:25 ` Glenn Washburn 0 siblings, 0 replies; 19+ messages in thread From: Glenn Washburn @ 2022-05-04 17:25 UTC (permalink / raw) To: Pierre-Louis Bonicoli; +Cc: The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 2/3] commands/probe: improve support of LUKS2 devices 2022-03-29 10:31 [PATCH v2 0/3] grub-probe: improve support of LUKS2 Pierre-Louis Bonicoli 2022-03-29 10:31 ` [PATCH v2 1/3] grub-fs-tester: add luks1 and luks2 support Pierre-Louis Bonicoli @ 2022-03-29 10:31 ` Pierre-Louis Bonicoli 2022-05-04 21:55 ` Glenn Washburn 2022-03-29 10:31 ` [PATCH v2 3/3] grub-core/kern/disk.c: handle " Pierre-Louis Bonicoli ` (2 subsequent siblings) 4 siblings, 1 reply; 19+ messages in thread From: Pierre-Louis Bonicoli @ 2022-03-29 10:31 UTC (permalink / raw) To: grub-devel 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/3] commands/probe: improve support of LUKS2 devices 2022-03-29 10:31 ` [PATCH v2 2/3] commands/probe: improve support of LUKS2 devices Pierre-Louis Bonicoli @ 2022-05-04 21:55 ` Glenn Washburn 0 siblings, 0 replies; 19+ messages in thread From: Glenn Washburn @ 2022-05-04 21:55 UTC (permalink / raw) To: Pierre-Louis Bonicoli; +Cc: The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 3/3] grub-core/kern/disk.c: handle LUKS2 devices 2022-03-29 10:31 [PATCH v2 0/3] grub-probe: improve support of LUKS2 Pierre-Louis Bonicoli 2022-03-29 10:31 ` [PATCH v2 1/3] grub-fs-tester: add luks1 and luks2 support Pierre-Louis Bonicoli 2022-03-29 10:31 ` [PATCH v2 2/3] commands/probe: improve support of LUKS2 devices Pierre-Louis Bonicoli @ 2022-03-29 10:31 ` Pierre-Louis Bonicoli 2022-05-04 21:47 ` Glenn Washburn 2022-04-14 16:11 ` [PATCH v2 0/3] grub-probe: improve support of LUKS2 Daniel Kiper 2022-05-04 21:57 ` Glenn Washburn 4 siblings, 1 reply; 19+ messages in thread From: Pierre-Louis Bonicoli @ 2022-03-29 10:31 UTC (permalink / raw) To: grub-devel 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/3] grub-core/kern/disk.c: handle LUKS2 devices 2022-03-29 10:31 ` [PATCH v2 3/3] grub-core/kern/disk.c: handle " Pierre-Louis Bonicoli @ 2022-05-04 21:47 ` Glenn Washburn 2022-05-09 20:27 ` Josselin Poiret 2022-05-29 6:58 ` Patrick Steinhardt 0 siblings, 2 replies; 19+ messages in thread From: Glenn Washburn @ 2022-05-04 21:47 UTC (permalink / raw) To: Pierre-Louis Bonicoli; +Cc: The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/3] grub-core/kern/disk.c: handle LUKS2 devices 2022-05-04 21:47 ` Glenn Washburn @ 2022-05-09 20:27 ` Josselin Poiret 2022-05-11 3:55 ` Glenn Washburn 2022-05-29 6:58 ` Patrick Steinhardt 1 sibling, 1 reply; 19+ messages in thread From: Josselin Poiret @ 2022-05-09 20:27 UTC (permalink / raw) To: Glenn Washburn, Pierre-Louis Bonicoli; +Cc: The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/3] grub-core/kern/disk.c: handle LUKS2 devices 2022-05-09 20:27 ` Josselin Poiret @ 2022-05-11 3:55 ` Glenn Washburn 2022-05-29 7:09 ` Patrick Steinhardt 0 siblings, 1 reply; 19+ messages in thread From: Glenn Washburn @ 2022-05-11 3:55 UTC (permalink / raw) To: Josselin Poiret; +Cc: Pierre-Louis Bonicoli, The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/3] grub-core/kern/disk.c: handle LUKS2 devices 2022-05-11 3:55 ` Glenn Washburn @ 2022-05-29 7:09 ` Patrick Steinhardt 2022-06-05 18:43 ` Glenn Washburn 0 siblings, 1 reply; 19+ messages in thread From: Patrick Steinhardt @ 2022-05-29 7:09 UTC (permalink / raw) To: The development of GNU GRUB Cc: Josselin Poiret, Pierre-Louis Bonicoli, Glenn Washburn, Daniel Kiper [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/3] grub-core/kern/disk.c: handle LUKS2 devices 2022-05-29 7:09 ` Patrick Steinhardt @ 2022-06-05 18:43 ` Glenn Washburn 2022-06-06 5:32 ` Patrick Steinhardt 0 siblings, 1 reply; 19+ messages in thread From: Glenn Washburn @ 2022-06-05 18:43 UTC (permalink / raw) To: Patrick Steinhardt Cc: The development of GNU GRUB, Josselin Poiret, Pierre-Louis Bonicoli, Daniel Kiper 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> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/3] grub-core/kern/disk.c: handle LUKS2 devices 2022-06-05 18:43 ` Glenn Washburn @ 2022-06-06 5:32 ` Patrick Steinhardt 2022-06-06 17:11 ` Glenn Washburn 0 siblings, 1 reply; 19+ messages in thread From: Patrick Steinhardt @ 2022-06-06 5:32 UTC (permalink / raw) To: Glenn Washburn Cc: The development of GNU GRUB, Josselin Poiret, Pierre-Louis Bonicoli, Daniel Kiper [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/3] grub-core/kern/disk.c: handle LUKS2 devices 2022-06-06 5:32 ` Patrick Steinhardt @ 2022-06-06 17:11 ` Glenn Washburn 2022-06-12 19:25 ` Patrick Steinhardt 0 siblings, 1 reply; 19+ messages in thread From: Glenn Washburn @ 2022-06-06 17:11 UTC (permalink / raw) To: Patrick Steinhardt Cc: The development of GNU GRUB, Josselin Poiret, Pierre-Louis Bonicoli, Daniel Kiper 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> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/3] grub-core/kern/disk.c: handle LUKS2 devices 2022-06-06 17:11 ` Glenn Washburn @ 2022-06-12 19:25 ` Patrick Steinhardt 0 siblings, 0 replies; 19+ messages in thread From: Patrick Steinhardt @ 2022-06-12 19:25 UTC (permalink / raw) To: Glenn Washburn Cc: The development of GNU GRUB, Josselin Poiret, Pierre-Louis Bonicoli, Daniel Kiper [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/3] grub-core/kern/disk.c: handle LUKS2 devices 2022-05-04 21:47 ` Glenn Washburn 2022-05-09 20:27 ` Josselin Poiret @ 2022-05-29 6:58 ` Patrick Steinhardt 2022-06-05 18:08 ` Glenn Washburn 1 sibling, 1 reply; 19+ messages in thread From: Patrick Steinhardt @ 2022-05-29 6:58 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Pierre-Louis Bonicoli [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/3] grub-core/kern/disk.c: handle LUKS2 devices 2022-05-29 6:58 ` Patrick Steinhardt @ 2022-06-05 18:08 ` Glenn Washburn 0 siblings, 0 replies; 19+ messages in thread From: Glenn Washburn @ 2022-06-05 18:08 UTC (permalink / raw) To: Patrick Steinhardt; +Cc: The development of GNU GRUB, Pierre-Louis Bonicoli 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] grub-probe: improve support of LUKS2 2022-03-29 10:31 [PATCH v2 0/3] grub-probe: improve support of LUKS2 Pierre-Louis Bonicoli ` (2 preceding siblings ...) 2022-03-29 10:31 ` [PATCH v2 3/3] grub-core/kern/disk.c: handle " Pierre-Louis Bonicoli @ 2022-04-14 16:11 ` Daniel Kiper 2022-04-14 16:17 ` Pierre-Louis Bonicoli 2022-05-04 21:57 ` Glenn Washburn 4 siblings, 1 reply; 19+ messages in thread From: Daniel Kiper @ 2022-04-14 16:11 UTC (permalink / raw) To: Pierre-Louis Bonicoli; +Cc: grub-devel, development, ps 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] grub-probe: improve support of LUKS2 2022-04-14 16:11 ` [PATCH v2 0/3] grub-probe: improve support of LUKS2 Daniel Kiper @ 2022-04-14 16:17 ` Pierre-Louis Bonicoli 0 siblings, 0 replies; 19+ messages in thread From: Pierre-Louis Bonicoli @ 2022-04-14 16:17 UTC (permalink / raw) To: Daniel Kiper; +Cc: grub-devel, development, ps 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/3] grub-probe: improve support of LUKS2 2022-03-29 10:31 [PATCH v2 0/3] grub-probe: improve support of LUKS2 Pierre-Louis Bonicoli ` (3 preceding siblings ...) 2022-04-14 16:11 ` [PATCH v2 0/3] grub-probe: improve support of LUKS2 Daniel Kiper @ 2022-05-04 21:57 ` Glenn Washburn 4 siblings, 0 replies; 19+ messages in thread From: Glenn Washburn @ 2022-05-04 21:57 UTC (permalink / raw) To: Pierre-Louis Bonicoli; +Cc: The development of GNU GRUB 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2022-06-12 19:25 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-03-29 10:31 [PATCH v2 0/3] grub-probe: improve support of LUKS2 Pierre-Louis Bonicoli 2022-03-29 10:31 ` [PATCH v2 1/3] grub-fs-tester: add luks1 and luks2 support Pierre-Louis Bonicoli 2022-05-04 17:25 ` Glenn Washburn 2022-03-29 10:31 ` [PATCH v2 2/3] commands/probe: improve support of LUKS2 devices Pierre-Louis Bonicoli 2022-05-04 21:55 ` Glenn Washburn 2022-03-29 10:31 ` [PATCH v2 3/3] grub-core/kern/disk.c: handle " Pierre-Louis Bonicoli 2022-05-04 21:47 ` Glenn Washburn 2022-05-09 20:27 ` Josselin Poiret 2022-05-11 3:55 ` Glenn Washburn 2022-05-29 7:09 ` Patrick Steinhardt 2022-06-05 18:43 ` Glenn Washburn 2022-06-06 5:32 ` Patrick Steinhardt 2022-06-06 17:11 ` Glenn Washburn 2022-06-12 19:25 ` Patrick Steinhardt 2022-05-29 6:58 ` Patrick Steinhardt 2022-06-05 18:08 ` Glenn Washburn 2022-04-14 16:11 ` [PATCH v2 0/3] grub-probe: improve support of LUKS2 Daniel Kiper 2022-04-14 16:17 ` Pierre-Louis Bonicoli 2022-05-04 21:57 ` Glenn Washburn
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.