[PATCH v3 0/2] Have LUKS2 cryptomounts be useable with grub-probe

[Josselin Poiret]

Hello Glenn,

Thanks for your review!  I've adressed all of your comments with these
revised patches (only the second one has changed), with the caveats below:

Glenn Washburn <development@efficientek.com> writes:
>
>> +                grub_util_error (_("can't set dm task name to `%s'"), name);
>> +              if (!dm_task_run (dmt))
>> +                grub_util_error (_("can't run dm task for `%s'"), name);
>> +              dm_get_next_target (dmt, NULL, &start, &length,
>> +                                  &target_type, &params);
>
> It looks like this might return NULL on error? Or does it never error
> here?

dm_get_next_target only unpacks the result of a dm_task, it does no
error handling at all.  We only need the first (and only) target here,
hence the NULL parameter.

> [...]
>
>> +              /* This is the only hash usable by PBKDF2, so set it as such 
>> */
>> +              cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256");
>
> So does this mean that when/if argon2 support is added, that this will
> need to be detected as well? If so, is there a way to get the hash via
> devmapper? It doesn't show up with the table command, so I'm guessing
> not. On the otherhand, I don't think cryptodisk->hash is ever used in
> the user-space utilities because the cryptodisk is never attempted to
> be unlocked, so there's no password to hash. Perhaps this should be
> NULL with a comment that its not used (if I'm right about that).

Looking at the LUKS2 spec (see Key Derivation Object, section 3.2.5 of
[1]), it doesn't seem that argon2 has a way to specify different hash
functions, so I don't think we would need to detect that.  We need to
set cryptodisk->hash though so that the right abstractions are picked
up and included by grub-install in the binary (this was my motivation
for this whole thing in the first place)!

> [...]
>
>> +                          else
>> +                            sector_size_str = c;
>> +                          sector_size = grub_strtoull (sector_size_str, 
>> NULL, 10);
>
> This should check for in grub_strtoull errors as is done in commit
> ac8a37dda (net/http: Allow use of non-standard TCP/IP ports).

I didn't spot anything out of the ordinary in that commit, but note
that just after grub_strtoull, we check that sector_size is either
512, 1024, 2048 or 4096, and error if it isn't (unlikely).

In any case, I've tested this in a VM with a LUKS2 device that has a
4096 sector_size, and `-v` does indeed report the right sector_size as
well as the number of (encryption) sectors!  Installation of GNU Guix
went as expected on it, I even stacked LVM with BTRFS on top and
everything worked properly.

[1] https://gitlab.com/cryptsetup/LUKS2-docs/blob/master/luks2_doc_wip.pdf

Best,
Josselin Poiret (2):
  devmapper/getroot: Have devmapper recognize LUKS2
  devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM
    parameters

 grub-core/osdep/devmapper/getroot.c | 138 +++++++++++++++++++++++++++-
 1 file changed, 133 insertions(+), 5 deletions(-)

-- 
2.36.0