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 <developm...@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, ¶ms);
>
> 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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
