* [PATCH 0/4] Probing support for LUKS2 @ 2020-05-30 12:25 Patrick Steinhardt 2020-05-30 12:25 ` [PATCH 1/4] luks: fix out-of-bounds copy of UUID Patrick Steinhardt ` (3 more replies) 0 siblings, 4 replies; 21+ messages in thread From: Patrick Steinhardt @ 2020-05-30 12:25 UTC (permalink / raw) To: grub-devel [-- Attachment #1: Type: text/plain, Size: 1992 bytes --] Hi, while basic LUKS2 support is there already, there is currently no support yet for auto-detection of LUKS2 for of grub-probe, grub-install and companions. As a result, users have to manually configure GRUB to include required modules. This series is a first step towards auto-detection and implements probing support for LUKS2: $ grub-probe -d /dev/mapper/luks2 -t cryptodisk_uuid b2e7039b5dd0bdd4d476f4467c1f7168 Noticably missing is auto-detection of required cryptographic modules, but this will require some refactoring of the cryptodisk code as the current assumption is that there will be always exactly one cipher, KDF and hash, which doesn't hold true for LUKS2. I'll thus do this as a follow up at a later point. The first two patches make sense on their own and are worthwhile to be included in GRUB 2.06. The first one is an out-of-bounds read in LUKS code, while the second one adjusts the internal UUID format of the cryptodisk to match the dash-less format that we currently use for LUKS1 disks. As such, it breaks current configs using the dashed format, so including it pre-2.06 would make sense from my point of view. The latter two patches are required to implement probing. I'm fine with deferring them until after 2.06. @Daniel: please let me know if you want me to split up this series into two. I didn't think it necessary as you can just apply the first two patches separately. Patrick Patrick Steinhardt (4): luks: fix out-of-bounds copy of UUID luks2: strip dashes off of the UUID luks2: set up dummy sector size during scan osdep: detect LUKS2-encrypted devices grub-core/disk/luks.c | 2 +- grub-core/disk/luks2.c | 21 ++++++++++++++++++--- grub-core/osdep/devmapper/getroot.c | 23 +++++++++++++++++++++-- include/grub/emu/getroot.h | 1 + util/getroot.c | 1 + 5 files changed, 42 insertions(+), 6 deletions(-) -- 2.26.2 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/4] luks: fix out-of-bounds copy of UUID 2020-05-30 12:25 [PATCH 0/4] Probing support for LUKS2 Patrick Steinhardt @ 2020-05-30 12:25 ` Patrick Steinhardt 2020-06-06 23:32 ` Petr Vorel 2020-05-30 12:25 ` [PATCH 2/4] luks2: strip dashes off of the UUID Patrick Steinhardt ` (2 subsequent siblings) 3 siblings, 1 reply; 21+ messages in thread From: Patrick Steinhardt @ 2020-05-30 12:25 UTC (permalink / raw) To: grub-devel [-- Attachment #1: Type: text/plain, Size: 1337 bytes --] When configuring a LUKS disk, we copy over the UUID from the LUKS header into the new `grub_cryptodisk_t` structure via `grub_memcpy ()`. As size we mistakenly use the size of the `grub_cryptodisk_t` UUID field, which is guaranteed to be strictly bigger than the LUKS UUID field we're copying. As a result, the copy always goes out-of-bounds and copies some garbage from other surrounding fields. During runtime, this isn't noticed due to the fact that we always NUL-terminate the UUID and thus never hit the trailing garbage. Fix the issue by using the size of the local stripped UUID field. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- grub-core/disk/luks.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c index 410cd6f84..2c730d072 100644 --- a/grub-core/disk/luks.c +++ b/grub-core/disk/luks.c @@ -125,7 +125,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid, newdev->source_disk = NULL; newdev->log_sector_size = 9; newdev->total_length = grub_disk_get_size (disk) - newdev->offset; - grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid)); + grub_memcpy (newdev->uuid, uuid, sizeof (uuid)); newdev->modname = "luks"; /* Configure the hash used for the AF splitter and HMAC. */ -- 2.26.2 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] luks: fix out-of-bounds copy of UUID 2020-05-30 12:25 ` [PATCH 1/4] luks: fix out-of-bounds copy of UUID Patrick Steinhardt @ 2020-06-06 23:32 ` Petr Vorel 0 siblings, 0 replies; 21+ messages in thread From: Petr Vorel @ 2020-06-06 23:32 UTC (permalink / raw) To: Patrick Steinhardt, Daniel Kiper; +Cc: grub-devel Hi, Reviewed-by: Petr Vorel <petr.vorel@gmail.com> Kind regards, Petr ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/4] luks2: strip dashes off of the UUID 2020-05-30 12:25 [PATCH 0/4] Probing support for LUKS2 Patrick Steinhardt 2020-05-30 12:25 ` [PATCH 1/4] luks: fix out-of-bounds copy of UUID Patrick Steinhardt @ 2020-05-30 12:25 ` Patrick Steinhardt 2020-09-15 14:30 ` Daniel Kiper 2020-05-30 12:25 ` [PATCH 3/4] luks2: set up dummy sector size during scan Patrick Steinhardt 2020-05-30 12:25 ` [PATCH 4/4] osdep: detect LUKS2-encrypted devices Patrick Steinhardt 3 siblings, 1 reply; 21+ messages in thread From: Patrick Steinhardt @ 2020-05-30 12:25 UTC (permalink / raw) To: grub-devel [-- Attachment #1: Type: text/plain, Size: 1865 bytes --] The UUID header for LUKS2 uses a format with dashes, same as for LUKS(1). But while we strip these dashes for the latter, we don't for the former. This isn't wrong per se, but it's definitely inconsistent for users as they need to use the dashed format for LUKS2 and the non-dashed format for LUKS when e.g. calling `cryptomount -u $UUID`. Fix this inconsistency by stripping dashes off of the LUKS2 UUID. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- grub-core/disk/luks2.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c index e3ff7c83d..c847b4ac4 100644 --- a/grub-core/disk/luks2.c +++ b/grub-core/disk/luks2.c @@ -346,6 +346,8 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot) { grub_cryptodisk_t cryptodisk; grub_luks2_header_t header; + char uuid[sizeof (header.uuid) + 1]; + grub_size_t i, j; if (check_boot) return NULL; @@ -356,16 +358,21 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot) return NULL; } - if (check_uuid && grub_strcasecmp (check_uuid, header.uuid) != 0) + for (i = 0, j = 0; i < sizeof (header.uuid); i++) + if (header.uuid[i] != '-') + uuid[j++] = header.uuid[i]; + uuid[j] = '\0'; + + if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0) return NULL; cryptodisk = grub_zalloc (sizeof (*cryptodisk)); if (!cryptodisk) return NULL; - COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >= sizeof (header.uuid)); + COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >= sizeof (uuid)); + grub_memcpy (cryptodisk->uuid, uuid, sizeof (uuid)); - grub_memcpy (cryptodisk->uuid, header.uuid, sizeof (header.uuid)); cryptodisk->modname = "luks2"; return cryptodisk; } -- 2.26.2 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/4] luks2: strip dashes off of the UUID 2020-05-30 12:25 ` [PATCH 2/4] luks2: strip dashes off of the UUID Patrick Steinhardt @ 2020-09-15 14:30 ` Daniel Kiper 0 siblings, 0 replies; 21+ messages in thread From: Daniel Kiper @ 2020-09-15 14:30 UTC (permalink / raw) To: Patrick Steinhardt; +Cc: grub-devel On Sat, May 30, 2020 at 02:25:11PM +0200, Patrick Steinhardt wrote: > The UUID header for LUKS2 uses a format with dashes, same as for > LUKS(1). But while we strip these dashes for the latter, we don't for > the former. This isn't wrong per se, but it's definitely inconsistent > for users as they need to use the dashed format for LUKS2 and the > non-dashed format for LUKS when e.g. calling `cryptomount -u $UUID`. > > Fix this inconsistency by stripping dashes off of the LUKS2 UUID. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> Daniel ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/4] luks2: set up dummy sector size during scan 2020-05-30 12:25 [PATCH 0/4] Probing support for LUKS2 Patrick Steinhardt 2020-05-30 12:25 ` [PATCH 1/4] luks: fix out-of-bounds copy of UUID Patrick Steinhardt 2020-05-30 12:25 ` [PATCH 2/4] luks2: strip dashes off of the UUID Patrick Steinhardt @ 2020-05-30 12:25 ` Patrick Steinhardt 2021-08-06 4:51 ` Michael Chang 2020-05-30 12:25 ` [PATCH 4/4] osdep: detect LUKS2-encrypted devices Patrick Steinhardt 3 siblings, 1 reply; 21+ messages in thread From: Patrick Steinhardt @ 2020-05-30 12:25 UTC (permalink / raw) To: grub-devel [-- Attachment #1: Type: text/plain, Size: 1687 bytes --] GRUB currently only supports disk sector sizes of at least 9 bits. While not a problem when using decrypted LUKS2 disks, where we configure the sector size after we have decrypted the disk, it will cause failure as soon as we implement support for probing of LUKS2 encrypted disks: we only cheat-mount devices there and don't perform a real decryption, and thus the sector size will remain "0", causing errors at a later point. The problem here is that we can only determine the sector size as soon as we have decrypted a key slot, as key slots may refer to different segments, where each segment in turn may have a different sector size. As we don't really need the sector size during cheat-mounting anyway, let's just specify the minimum value as dummy to fix such errors. This patch is in preparation for probing support. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- grub-core/disk/luks2.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c index c847b4ac4..5c00d9775 100644 --- a/grub-core/disk/luks2.c +++ b/grub-core/disk/luks2.c @@ -374,6 +374,14 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot) grub_memcpy (cryptodisk->uuid, uuid, sizeof (uuid)); cryptodisk->modname = "luks2"; + /* + * This dummy value is required when cheat-mounting and is overridden by + * `luks2_verify_key ()`. We can't determine it here yet, as its value + * depends on which disk sector we're going to open, which in turn depends on + * the keyslot. + */ + cryptodisk->log_sector_size = GRUB_DISK_SECTOR_BITS; + return cryptodisk; } -- 2.26.2 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] luks2: set up dummy sector size during scan 2020-05-30 12:25 ` [PATCH 3/4] luks2: set up dummy sector size during scan Patrick Steinhardt @ 2021-08-06 4:51 ` Michael Chang 2021-08-08 14:20 ` Patrick Steinhardt 0 siblings, 1 reply; 21+ messages in thread From: Michael Chang @ 2021-08-06 4:51 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Fabian Vogt Hi, Enclosed herewith please find the revised patch from openSUSE that could also fix this very same problem. According to Fabian, the author of this patch, the reason for having this patch is that he found some problem in the posted one. I have added him to the CC list so that he could provide more in detail later. Thanks, Michael From: Fabian Vogt <fvogt@suse.de> Date: Wed, 4 Aug 2021 14:56:16 +0200 Subject: [PATCH 1/2] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device When using grub-probe with cryptodisk, the mapped block device from the host is used directly instead of decrypting the source device in GRUB code. In that case, the sector size and count of the host device needs to be used. This is especially important when using luks2, which does not assign total_sectors and log_sector_size when scanning, but only later when the segments in the JSON area are evaluated. With an unset log_sector_size, grub_open_device complains. This fixes grub-probe failing with "error: sector sizes of 1 bytes aren't supported yet." Signed-off-by: Fabian Vogt <fvogt@suse.de> --- grub-core/disk/cryptodisk.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c index 90f82b2d3..c2bb2b6eb 100644 --- a/grub-core/disk/cryptodisk.c +++ b/grub-core/disk/cryptodisk.c @@ -1040,6 +1040,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat) grub_cryptodisk_t dev; grub_cryptodisk_dev_t cr; grub_disk_t source; + unsigned int cheat_sector_size; /* Try to open disk. */ source = grub_disk_open (sourcedev); @@ -1062,6 +1063,25 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat) if (!dev) continue; + /* Use the sector size and count of the cheat device */ + dev->cheat_fd = grub_util_fd_open (cheat, GRUB_UTIL_FD_O_RDONLY); + if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd)) + { + grub_free (dev); + return grub_errno; + } + dev->total_sectors = grub_util_get_fd_size (dev->cheat_fd, cheat, &cheat_sector_size); + if (dev->total_sectors == -1) + { + grub_util_fd_close (dev->cheat_fd); + grub_free (dev); + return grub_errno; + } + dev->log_sector_size = cheat_sector_size; + dev->total_sectors >>= dev->log_sector_size; + grub_util_fd_close (dev->cheat_fd); + dev->cheat_fd = GRUB_UTIL_FD_INVALID; + grub_util_info ("cheatmounted %s (%s) at %s", sourcedev, dev->modname, cheat); err = grub_cryptodisk_cheat_insert (dev, sourcedev, source, cheat); -- 2.32.0 On Sat, May 30, 2020 at 02:25:17PM +0200, Patrick Steinhardt wrote: > GRUB currently only supports disk sector sizes of at least 9 bits. While > not a problem when using decrypted LUKS2 disks, where we configure the > sector size after we have decrypted the disk, it will cause failure as > soon as we implement support for probing of LUKS2 encrypted disks: we > only cheat-mount devices there and don't perform a real decryption, and > thus the sector size will remain "0", causing errors at a later point. > > The problem here is that we can only determine the sector size as soon > as we have decrypted a key slot, as key slots may refer to different > segments, where each segment in turn may have a different sector size. > As we don't really need the sector size during cheat-mounting anyway, > let's just specify the minimum value as dummy to fix such errors. > > This patch is in preparation for probing support. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > grub-core/disk/luks2.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c > index c847b4ac4..5c00d9775 100644 > --- a/grub-core/disk/luks2.c > +++ b/grub-core/disk/luks2.c > @@ -374,6 +374,14 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot) > grub_memcpy (cryptodisk->uuid, uuid, sizeof (uuid)); > > cryptodisk->modname = "luks2"; > + /* > + * This dummy value is required when cheat-mounting and is overridden by > + * `luks2_verify_key ()`. We can't determine it here yet, as its value > + * depends on which disk sector we're going to open, which in turn depends on > + * the keyslot. > + */ > + cryptodisk->log_sector_size = GRUB_DISK_SECTOR_BITS; > + > return cryptodisk; > } > > -- > 2.26.2 > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] luks2: set up dummy sector size during scan 2021-08-06 4:51 ` Michael Chang @ 2021-08-08 14:20 ` Patrick Steinhardt 2021-12-16 15:52 ` Fabian Vogt 0 siblings, 1 reply; 21+ messages in thread From: Patrick Steinhardt @ 2021-08-08 14:20 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Michael Chang, Fabian Vogt [-- Attachment #1: Type: text/plain, Size: 2053 bytes --] On Fri, Aug 06, 2021 at 12:51:10PM +0800, Michael Chang via Grub-devel wrote: [snip] > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > index 90f82b2d3..c2bb2b6eb 100644 > --- a/grub-core/disk/cryptodisk.c > +++ b/grub-core/disk/cryptodisk.c > @@ -1040,6 +1040,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat) > grub_cryptodisk_t dev; > grub_cryptodisk_dev_t cr; > grub_disk_t source; > + unsigned int cheat_sector_size; > > /* Try to open disk. */ > source = grub_disk_open (sourcedev); > @@ -1062,6 +1063,25 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat) > if (!dev) > continue; > > + /* Use the sector size and count of the cheat device */ > + dev->cheat_fd = grub_util_fd_open (cheat, GRUB_UTIL_FD_O_RDONLY); > + if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd)) > + { > + grub_free (dev); > + return grub_errno; > + } > + dev->total_sectors = grub_util_get_fd_size (dev->cheat_fd, cheat, &cheat_sector_size); > + if (dev->total_sectors == -1) > + { > + grub_util_fd_close (dev->cheat_fd); > + grub_free (dev); > + return grub_errno; > + } We may want to print error messages for both error cases. Otherwise it's hard to tell why cheat mounting might've failed. > + dev->log_sector_size = cheat_sector_size; > + dev->total_sectors >>= dev->log_sector_size; > + grub_util_fd_close (dev->cheat_fd); > + dev->cheat_fd = GRUB_UTIL_FD_INVALID; Wouldn't it make more sense to just use a separate variable for the FD? Feels kind of weird to put it into `dev->cheat_fd`, only to clone and unset the member variable immediately after we're done again. In any case, thanks for the patch. It does look a lot more sensible compared to what I'd been doing. Do you want to keep owning the patch and try to upstream it, or shall I pick it up as part of this patch series? I'd of course retain author information. Patrick [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] luks2: set up dummy sector size during scan 2021-08-08 14:20 ` Patrick Steinhardt @ 2021-12-16 15:52 ` Fabian Vogt 2021-12-22 18:17 ` Josselin Poiret 0 siblings, 1 reply; 21+ messages in thread From: Fabian Vogt @ 2021-12-16 15:52 UTC (permalink / raw) To: The development of GNU GRUB, Patrick Steinhardt Cc: Michael Chang, Josselin Poiret Hi, (I *finally* got to this topic again...) Am Sonntag, 8. August 2021, 16:20:12 CET schrieb Patrick Steinhardt: > On Fri, Aug 06, 2021 at 12:51:10PM +0800, Michael Chang via Grub-devel wrote: > [snip] > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > > index 90f82b2d3..c2bb2b6eb 100644 > > --- a/grub-core/disk/cryptodisk.c > > +++ b/grub-core/disk/cryptodisk.c > > @@ -1040,6 +1040,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat) > > grub_cryptodisk_t dev; > > grub_cryptodisk_dev_t cr; > > grub_disk_t source; > > + unsigned int cheat_sector_size; > > > > /* Try to open disk. */ > > source = grub_disk_open (sourcedev); > > @@ -1062,6 +1063,25 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat) > > if (!dev) > > continue; > > > > + /* Use the sector size and count of the cheat device */ > > + dev->cheat_fd = grub_util_fd_open (cheat, GRUB_UTIL_FD_O_RDONLY); > > + if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd)) > > + { > > + grub_free (dev); > > + return grub_errno; > > + } > > + dev->total_sectors = grub_util_get_fd_size (dev->cheat_fd, cheat, &cheat_sector_size); > > + if (dev->total_sectors == -1) > > + { > > + grub_util_fd_close (dev->cheat_fd); > > + grub_free (dev); > > + return grub_errno; > > + } > > We may want to print error messages for both error cases. Otherwise it's > hard to tell why cheat mounting might've failed. Agreed. > > + dev->log_sector_size = cheat_sector_size; > > + dev->total_sectors >>= dev->log_sector_size; > > + grub_util_fd_close (dev->cheat_fd); > > + dev->cheat_fd = GRUB_UTIL_FD_INVALID; > > Wouldn't it make more sense to just use a separate variable for the FD? > Feels kind of weird to put it into `dev->cheat_fd`, only to clone and > unset the member variable immediately after we're done again. A bit, yes, but the alternative is having a variable "int cheat_fd" which makes it more confusing IMO. I'm fine with either option. > In any case, thanks for the patch. It does look a lot more sensible > compared to what I'd been doing. Do you want to keep owning the patch > and try to upstream it, or shall I pick it up as part of this patch > series? I'd of course retain author information. Feel free to incorporate it, or I can refine it and resend as v2. (I'll most likely get to the latter only in the next year though) It looks like we have a third patch (series) for this feature meanwhile: [PATCH 0/2] Have LUKS2 cryptomounts be useable with grub-probe I CC'd the author, let's try to coordinate. Thanks, Fabian > Patrick ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] luks2: set up dummy sector size during scan 2021-12-16 15:52 ` Fabian Vogt @ 2021-12-22 18:17 ` Josselin Poiret 2022-02-04 15:46 ` Fabian Vogt 0 siblings, 1 reply; 21+ messages in thread From: Josselin Poiret @ 2021-12-22 18:17 UTC (permalink / raw) To: Fabian Vogt, The development of GNU GRUB, Patrick Steinhardt Cc: Michael Chang Hello everyone, Fabian Vogt <fvogt@suse.de> writes: > It looks like we have a third patch (series) for this feature meanwhile: > [PATCH 0/2] Have LUKS2 cryptomounts be useable with grub-probe > > I CC'd the author, let's try to coordinate. > > Thanks, > Fabian Let me just say that I had not found this patch series while searching beforehand. Let me just recap what my patches do differently (in relation to patches 3 and 4 of this series): Because cheat-mounting cryptodisks only happens (from my understanding) when pulling devmapper devices, we can simply ask the kernel for the dm and dm-crypt parameters that it's opened with, and populate our cheat-mounted device from that. This completely circumvents the multiple segments issue, as this will always yield the parameters corresponding to the user-specified mountpoint of `grub-probe` or `grub-install`. I also opted not to add a GRUB_DEV_ABSTRACTION_LUKS2 abstraction, so as to reuse all existing code that supports LUKS1, although that can be confusing. We could simply rename GRUB_DEV_ABSTRACTION_LUKS1 to GRUB_DEV_ABSTRACTION_CRYTODISK, as LUKS1 and LUKS2 only differ in how they're unlocked, not in underlying algorithms. What do you think? -- Josselin Poiret ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] luks2: set up dummy sector size during scan 2021-12-22 18:17 ` Josselin Poiret @ 2022-02-04 15:46 ` Fabian Vogt 2022-02-07 13:15 ` Josselin Poiret 0 siblings, 1 reply; 21+ messages in thread From: Fabian Vogt @ 2022-02-04 15:46 UTC (permalink / raw) To: The development of GNU GRUB, Patrick Steinhardt Cc: Josselin Poiret, Michael Chang, Josselin Poiret via Grub-devel, Pierre-Louis Bonicoli Hi, Am Mittwoch, 22. Dezember 2021, 19:17:37 CET schrieb Josselin Poiret via Grub-devel: > Hello everyone, > > Fabian Vogt <fvogt@suse.de> writes: > > It looks like we have a third patch (series) for this feature meanwhile: > > [PATCH 0/2] Have LUKS2 cryptomounts be useable with grub-probe > > > > I CC'd the author, let's try to coordinate. And there's a forth one now (author CC'd)! ("[PATCH 3/3] grub-core/kern/disk.c: handle LUKS2 devices") So we have: "[PATCH 3/4] luks2: set up dummy sector size during scan", which hardcodes 512, "[PATCH 1/2] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device", which queries the sector size of the underlying host device, "[PATCH v2 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters", which parses the DM table to get the sector_size, and now "[PATCH 3/3] grub-core/kern/disk.c: handle LUKS2 devices", which changes the grub core code to accept a sector size of 0 for LUKS2 devices. Should be enough options to pick a good one ;-) > > Thanks, > > Fabian > > Let me just say that I had not found this patch series while searching > beforehand. Let me just recap what my patches do differently (in > relation to patches 3 and 4 of this series): > > Because cheat-mounting cryptodisks only happens (from my understanding) > when pulling devmapper devices, we can simply ask the kernel for the dm > and dm-crypt parameters that it's opened with, and populate our > cheat-mounted device from that. This completely circumvents the multiple > segments issue, as this will always yield the parameters corresponding > to the user-specified mountpoint of `grub-probe` or `grub-install`. Yup. Did you have a look at my approach? That effectively does the same, but using a single ioctl instead of anything complex with DM directly. > I also opted not to add a GRUB_DEV_ABSTRACTION_LUKS2 abstraction, so as > to reuse all existing code that supports LUKS1, although that can be > confusing. We could simply rename GRUB_DEV_ABSTRACTION_LUKS1 to > GRUB_DEV_ABSTRACTION_CRYTODISK, as LUKS1 and LUKS2 only differ in how > they're unlocked, not in underlying algorithms. > > What do you think? Sounds good to me, though I'd count that as a separate refactoring step for the future. Cheers, Fabian ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] luks2: set up dummy sector size during scan 2022-02-04 15:46 ` Fabian Vogt @ 2022-02-07 13:15 ` Josselin Poiret 2022-05-21 0:13 ` Glenn Washburn 0 siblings, 1 reply; 21+ messages in thread From: Josselin Poiret @ 2022-02-07 13:15 UTC (permalink / raw) To: Fabian Vogt, The development of GNU GRUB, Patrick Steinhardt Cc: Michael Chang, Josselin Poiret via Grub-devel, Pierre-Louis Bonicoli Hi Fabian, Fabian Vogt <fvogt@suse.de> writes: > Did you have a look at my approach? That effectively does the same, but using a > single ioctl instead of anything complex with DM directly. I agree that it's sufficient for sector_size, but we still need the cryptodisk algorithm so that grub-install will know which crypto modules to include. libdm is actually a helper library around dm-specific ioctls, since they are apparently annoying to use, so in the end we're still using the same interface :) As for the 4 patches, since the actual sector_size is available to us, I think using 512 in all cases or adding a specific 0 sector_size is something we should avoid. Best, -- Josselin Poiret ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] luks2: set up dummy sector size during scan 2022-02-07 13:15 ` Josselin Poiret @ 2022-05-21 0:13 ` Glenn Washburn 2022-05-21 10:53 ` Fabian Vogt 2022-06-13 14:29 ` [PATCH v2] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device Fabian Vogt 0 siblings, 2 replies; 21+ messages in thread From: Glenn Washburn @ 2022-05-21 0:13 UTC (permalink / raw) To: Josselin Poiret via Grub-devel, Fabian Vogt, Josselin Poiret Cc: Patrick Steinhardt, Michael Chang, Pierre-Louis Bonicoli On Mon, 07 Feb 2022 14:15:07 +0100 Josselin Poiret via Grub-devel <grub-devel@gnu.org> wrote: > Hi Fabian, > > Fabian Vogt <fvogt@suse.de> writes: > > > Did you have a look at my approach? That effectively does the same, but using a > > single ioctl instead of anything complex with DM directly. I skipped this thread because I didn't really care about probing for LUKS2 (I don't use it). However, things change and now I'm evaluating the different patch series that implement this. I had missed Fabian's patch and I'm just now seeing it and this thread. Now that I've taken a look at Fabian's patch, I think its the better way to go for getting the sector size. The reason is that it uses grub_util_get_fd_size() to get the size and sector size of the cheat mount device. That function's implementation is platform dependant. So, for instance, if FreeBSD has a LUKS* implementation that is not libdevicemapper based, we'll correctly get the number of sectors and sector size. But yes, its very unlikely that FreeBSD will support LUKS. However, this same code should allow us to get the correct sector size for GELI devices also. I don't believe GRUB has GELI probe support, so ths would make things easier for whoever wants to develop it. The same goes for future cryptodisk backends. > I agree that it's sufficient for sector_size, but we still need the > cryptodisk algorithm so that grub-install will know which crypto modules > to include. libdm is actually a helper library around dm-specific I'm now thinking that GRUB should use Fabian's patch to get the sector size and number of sectors and Josselin's method of querying device mapper to get the other properies. We could have Josselin's patch first check if log_sector_size is set and if not then try to get it from DM. However, I think that's overkill and not really useful because grub_util_get_fd_size() should always give us the correct information, otherwise we've found a kernel bug. Also, by not having to parse out the sector size from the dm-crypt params string, we avoid much the complexity of that code, which has been the source of several bugs in reviewing a couple iterations of that patch. > ioctls, since they are apparently annoying to use, so in the end we're > still using the same interface :) Technically different interface, the code Fabian is using is not using device mapper related ioctls, they are generic block device ioctls (on Linux systems). > As for the 4 patches, since the actual sector_size is available to us, I > think using 512 in all cases or adding a specific 0 sector_size is > something we should avoid. I agree. Fabian, would you be able to send an updated patch with Patrick's suggestions in the near future? Glenn ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] luks2: set up dummy sector size during scan 2022-05-21 0:13 ` Glenn Washburn @ 2022-05-21 10:53 ` Fabian Vogt 2022-06-13 14:29 ` [PATCH v2] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device Fabian Vogt 1 sibling, 0 replies; 21+ messages in thread From: Fabian Vogt @ 2022-05-21 10:53 UTC (permalink / raw) To: Josselin Poiret via Grub-devel, Josselin Poiret, development Cc: Patrick Steinhardt, Michael Chang, Pierre-Louis Bonicoli Hi, Am Samstag, 21. Mai 2022, 02:13:53 CEST schrieb Glenn Washburn: > On Mon, 07 Feb 2022 14:15:07 +0100 > Josselin Poiret via Grub-devel <grub-devel@gnu.org> wrote: > > > Hi Fabian, > > > > Fabian Vogt <fvogt@suse.de> writes: > > > > > Did you have a look at my approach? That effectively does the same, but using a > > > single ioctl instead of anything complex with DM directly. > > I skipped this thread because I didn't really care about probing for > LUKS2 (I don't use it). However, things change and now I'm evaluating > the different patch series that implement this. I had missed Fabian's > patch and I'm just now seeing it and this thread. > > Now that I've taken a look at Fabian's patch, I think its the better > way to go for getting the sector size. The reason is that it uses > grub_util_get_fd_size() to get the size and sector size of the cheat > mount device. That function's implementation is platform dependant. So, > for instance, if FreeBSD has a LUKS* implementation that is not > libdevicemapper based, we'll correctly get the number of sectors and > sector size. But yes, its very unlikely that FreeBSD will support LUKS. > However, this same code should allow us to get the correct sector size > for GELI devices also. I don't believe GRUB has GELI probe support, so > ths would make things easier for whoever wants to develop it. The same > goes for future cryptodisk backends. > > > I agree that it's sufficient for sector_size, but we still need the > > cryptodisk algorithm so that grub-install will know which crypto modules > > to include. libdm is actually a helper library around dm-specific > > I'm now thinking that GRUB should use Fabian's patch to get the sector > size and number of sectors and Josselin's method of querying device > mapper to get the other properies. We could have Josselin's patch first > check if log_sector_size is set and if not then try to get it from DM. > However, I think that's overkill and not really useful because > grub_util_get_fd_size() should always give us the correct information, > otherwise we've found a kernel bug. Also, by not having to parse out > the sector size from the dm-crypt params string, we avoid much the > complexity of that code, which has been the source of several bugs in > reviewing a couple iterations of that patch. > > > ioctls, since they are apparently annoying to use, so in the end we're > > still using the same interface :) > > Technically different interface, the code Fabian is using is not using > device mapper related ioctls, they are generic block device ioctls (on > Linux systems). > > > As for the 4 patches, since the actual sector_size is available to us, I > > think using 512 in all cases or adding a specific 0 sector_size is > > something we should avoid. > > I agree. > > Fabian, would you be able to send an updated patch with Patrick's > suggestions in the near future? Sure! I'll be back from vacation after end of May, I hope that still counts. Cheers, Fabian > Glenn ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device 2022-05-21 0:13 ` Glenn Washburn 2022-05-21 10:53 ` Fabian Vogt @ 2022-06-13 14:29 ` Fabian Vogt 2022-06-14 2:19 ` Glenn Washburn 1 sibling, 1 reply; 21+ messages in thread From: Fabian Vogt @ 2022-06-13 14:29 UTC (permalink / raw) To: grub-devel Cc: Josselin Poiret, Glenn Washburn, Patrick Steinhardt, Michael Chang, Pierre-Louis Bonicoli, aplanas When using grub-probe with cryptodisk, the mapped block device from the host is used directly instead of decrypting the source device in GRUB code. In that case, the sector size and count of the host device needs to be used. This is especially important when using luks2, which does not assign total_sectors and log_sector_size when scanning, but only later when the segments in the JSON area are evaluated. With an unset log_sector_size, grub_open_device complains. This fixes grub-probe failing with "error: sector sizes of 1 bytes aren't supported yet." Signed-off-by: Fabian Vogt <fvogt@suse.de> --- v2: Moved new code from grub_cryptodisk_cheat_mount to grub_cryptodisk_open, which allowed to simplify the code a bit. Also improved error handling. grub-core/disk/cryptodisk.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c index c80cf9907..14e105b84 100644 --- a/grub-core/disk/cryptodisk.c +++ b/grub-core/disk/cryptodisk.c @@ -686,6 +686,11 @@ static grub_err_t grub_cryptodisk_open (const char *name, grub_disk_t disk) { grub_cryptodisk_t dev; +#ifdef GRUB_UTIL + grub_uint64_t cheat_dev_size; + unsigned int cheat_log_sector_size; + const char *errmsg; +#endif if (grub_memcmp (name, "crypto", sizeof ("crypto") - 1) != 0) return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "No such device"); @@ -709,8 +714,6 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk) if (!dev) return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "No such device"); - disk->log_sector_size = dev->log_sector_size; - #ifdef GRUB_UTIL if (dev->cheat) { @@ -719,6 +722,20 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk) if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd)) return grub_error (GRUB_ERR_IO, N_("cannot open `%s': %s"), dev->cheat, grub_util_fd_strerror ()); + + /* Use the sector size and count of the cheat device */ + cheat_dev_size = grub_util_get_fd_size (dev->cheat_fd, dev->cheat, &cheat_log_sector_size); + if (cheat_dev_size == -1) + { + errmsg = grub_util_fd_strerror (); + grub_util_fd_close (dev->cheat_fd); + dev->cheat_fd = GRUB_UTIL_FD_INVALID; + return grub_error (GRUB_ERR_IO, N_("failed to query size of device `%s': %s"), + dev->cheat, errmsg); + } + + dev->log_sector_size = cheat_log_sector_size; + dev->total_sectors = cheat_dev_size >> dev->log_sector_size; } #endif @@ -732,6 +749,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk) } disk->data = dev; + disk->log_sector_size = dev->log_sector_size; disk->total_sectors = dev->total_sectors; disk->max_agglomerate = GRUB_DISK_MAX_MAX_AGGLOMERATE; disk->id = dev->id; -- 2.36.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device 2022-06-13 14:29 ` [PATCH v2] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device Fabian Vogt @ 2022-06-14 2:19 ` Glenn Washburn 2022-06-14 13:55 ` [PATCH v3] " Fabian Vogt 0 siblings, 1 reply; 21+ messages in thread From: Glenn Washburn @ 2022-06-14 2:19 UTC (permalink / raw) To: Fabian Vogt Cc: grub-devel, Josselin Poiret, Patrick Steinhardt, Michael Chang, Pierre-Louis Bonicoli, aplanas On Mon, 13 Jun 2022 16:29:48 +0200 Fabian Vogt <fvogt@suse.de> wrote: > When using grub-probe with cryptodisk, the mapped block device from the host > is used directly instead of decrypting the source device in GRUB code. > In that case, the sector size and count of the host device needs to be used. > This is especially important when using luks2, which does not assign > total_sectors and log_sector_size when scanning, but only later when the > segments in the JSON area are evaluated. With an unset log_sector_size, > grub_open_device complains. > > This fixes grub-probe failing with > "error: sector sizes of 1 bytes aren't supported yet." > > Signed-off-by: Fabian Vogt <fvogt@suse.de> > --- > v2: Moved new code from grub_cryptodisk_cheat_mount to grub_cryptodisk_open, > which allowed to simplify the code a bit. Also improved error handling. Yes, I like this patch even better. > > grub-core/disk/cryptodisk.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > index c80cf9907..14e105b84 100644 > --- a/grub-core/disk/cryptodisk.c > +++ b/grub-core/disk/cryptodisk.c > @@ -686,6 +686,11 @@ static grub_err_t > grub_cryptodisk_open (const char *name, grub_disk_t disk) > { > grub_cryptodisk_t dev; > +#ifdef GRUB_UTIL > + grub_uint64_t cheat_dev_size; > + unsigned int cheat_log_sector_size; > + const char *errmsg; > +#endif I think these variable declarations would be better put ... > > if (grub_memcmp (name, "crypto", sizeof ("crypto") - 1) != 0) > return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "No such device"); > @@ -709,8 +714,6 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk) > if (!dev) > return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "No such device"); > > - disk->log_sector_size = dev->log_sector_size; > - > #ifdef GRUB_UTIL > if (dev->cheat) > { ... here. > @@ -719,6 +722,20 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk) > if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd)) > return grub_error (GRUB_ERR_IO, N_("cannot open `%s': %s"), > dev->cheat, grub_util_fd_strerror ()); > + > + /* Use the sector size and count of the cheat device */ > + cheat_dev_size = grub_util_get_fd_size (dev->cheat_fd, dev->cheat, &cheat_log_sector_size); > + if (cheat_dev_size == -1) > + { > + errmsg = grub_util_fd_strerror (); > + grub_util_fd_close (dev->cheat_fd); > + dev->cheat_fd = GRUB_UTIL_FD_INVALID; > + return grub_error (GRUB_ERR_IO, N_("failed to query size of device `%s': %s"), > + dev->cheat, errmsg); > + } > + > + dev->log_sector_size = cheat_log_sector_size; > + dev->total_sectors = cheat_dev_size >> dev->log_sector_size; Small nit, I think it reads better to have "cheat_dev_size >> cheat_log_sector_size". > } > #endif > > @@ -732,6 +749,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk) > } > > disk->data = dev; > + disk->log_sector_size = dev->log_sector_size; > disk->total_sectors = dev->total_sectors; > disk->max_agglomerate = GRUB_DISK_MAX_MAX_AGGLOMERATE; > disk->id = dev->id; Regardless whether the suggestions above are made: Reviewed-by: Glenn Washburn <development@efficientek.com> Glenn ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device 2022-06-14 2:19 ` Glenn Washburn @ 2022-06-14 13:55 ` Fabian Vogt 2022-06-14 18:18 ` Glenn Washburn ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Fabian Vogt @ 2022-06-14 13:55 UTC (permalink / raw) To: grub-devel, Josselin Poiret Cc: Patrick Steinhardt, Michael Chang, Pierre-Louis Bonicoli, aplanas, Glenn Washburn When using grub-probe with cryptodisk, the mapped block device from the host is used directly instead of decrypting the source device in GRUB code. In that case, the sector size and count of the host device needs to be used. This is especially important when using luks2, which does not assign total_sectors and log_sector_size when scanning, but only later when the segments in the JSON area are evaluated. With an unset log_sector_size, grub_open_device complains. This fixes grub-probe failing with "error: sector sizes of 1 bytes aren't supported yet." Signed-off-by: Fabian Vogt <fvogt@suse.de> --- v2: Moved new code from grub_cryptodisk_cheat_mount to grub_cryptodisk_open, which allowed to simplify the code a bit. Also improved error handling. v3: More liberal placement of variable declarations. grub-core/disk/cryptodisk.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c index c80cf9907..b3c76ef57 100644 --- a/grub-core/disk/cryptodisk.c +++ b/grub-core/disk/cryptodisk.c @@ -709,16 +709,31 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk) if (!dev) return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "No such device"); - disk->log_sector_size = dev->log_sector_size; - #ifdef GRUB_UTIL if (dev->cheat) { + grub_uint64_t cheat_dev_size; + unsigned int cheat_log_sector_size; + if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd)) dev->cheat_fd = grub_util_fd_open (dev->cheat, GRUB_UTIL_FD_O_RDONLY); if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd)) return grub_error (GRUB_ERR_IO, N_("cannot open `%s': %s"), dev->cheat, grub_util_fd_strerror ()); + + /* Use the sector size and count of the cheat device */ + cheat_dev_size = grub_util_get_fd_size (dev->cheat_fd, dev->cheat, &cheat_log_sector_size); + if (cheat_dev_size == -1) + { + const char *errmsg = grub_util_fd_strerror (); + grub_util_fd_close (dev->cheat_fd); + dev->cheat_fd = GRUB_UTIL_FD_INVALID; + return grub_error (GRUB_ERR_IO, N_("failed to query size of device `%s': %s"), + dev->cheat, errmsg); + } + + dev->log_sector_size = cheat_log_sector_size; + dev->total_sectors = cheat_dev_size >> cheat_log_sector_size; } #endif @@ -732,6 +747,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk) } disk->data = dev; + disk->log_sector_size = dev->log_sector_size; disk->total_sectors = dev->total_sectors; disk->max_agglomerate = GRUB_DISK_MAX_MAX_AGGLOMERATE; disk->id = dev->id; -- 2.36.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device 2022-06-14 13:55 ` [PATCH v3] " Fabian Vogt @ 2022-06-14 18:18 ` Glenn Washburn 2022-06-21 15:40 ` Patrick Steinhardt 2022-08-11 18:22 ` Glenn Washburn 2 siblings, 0 replies; 21+ messages in thread From: Glenn Washburn @ 2022-06-14 18:18 UTC (permalink / raw) To: Fabian Vogt Cc: grub-devel, Josselin Poiret, Patrick Steinhardt, Michael Chang, Pierre-Louis Bonicoli, aplanas, Daniel Kiper On Tue, 14 Jun 2022 15:55:21 +0200 Fabian Vogt <fvogt@suse.de> wrote: > When using grub-probe with cryptodisk, the mapped block device from the host > is used directly instead of decrypting the source device in GRUB code. > In that case, the sector size and count of the host device needs to be used. > This is especially important when using luks2, which does not assign > total_sectors and log_sector_size when scanning, but only later when the > segments in the JSON area are evaluated. With an unset log_sector_size, > grub_open_device complains. > > This fixes grub-probe failing with > "error: sector sizes of 1 bytes aren't supported yet." > > Signed-off-by: Fabian Vogt <fvogt@suse.de> Reviewed-by: Glenn Washburn <development@efficientek.com> Glenn > --- > v2: Moved new code from grub_cryptodisk_cheat_mount to grub_cryptodisk_open, > which allowed to simplify the code a bit. Also improved error handling. > v3: More liberal placement of variable declarations. > > grub-core/disk/cryptodisk.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > index c80cf9907..b3c76ef57 100644 > --- a/grub-core/disk/cryptodisk.c > +++ b/grub-core/disk/cryptodisk.c > @@ -709,16 +709,31 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk) > if (!dev) > return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "No such device"); > > - disk->log_sector_size = dev->log_sector_size; > - > #ifdef GRUB_UTIL > if (dev->cheat) > { > + grub_uint64_t cheat_dev_size; > + unsigned int cheat_log_sector_size; > + > if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd)) > dev->cheat_fd = grub_util_fd_open (dev->cheat, GRUB_UTIL_FD_O_RDONLY); > if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd)) > return grub_error (GRUB_ERR_IO, N_("cannot open `%s': %s"), > dev->cheat, grub_util_fd_strerror ()); > + > + /* Use the sector size and count of the cheat device */ > + cheat_dev_size = grub_util_get_fd_size (dev->cheat_fd, dev->cheat, &cheat_log_sector_size); > + if (cheat_dev_size == -1) > + { > + const char *errmsg = grub_util_fd_strerror (); > + grub_util_fd_close (dev->cheat_fd); > + dev->cheat_fd = GRUB_UTIL_FD_INVALID; > + return grub_error (GRUB_ERR_IO, N_("failed to query size of device `%s': %s"), > + dev->cheat, errmsg); > + } > + > + dev->log_sector_size = cheat_log_sector_size; > + dev->total_sectors = cheat_dev_size >> cheat_log_sector_size; > } > #endif > > @@ -732,6 +747,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk) > } > > disk->data = dev; > + disk->log_sector_size = dev->log_sector_size; > disk->total_sectors = dev->total_sectors; > disk->max_agglomerate = GRUB_DISK_MAX_MAX_AGGLOMERATE; > disk->id = dev->id; ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device 2022-06-14 13:55 ` [PATCH v3] " Fabian Vogt 2022-06-14 18:18 ` Glenn Washburn @ 2022-06-21 15:40 ` Patrick Steinhardt 2022-08-11 18:22 ` Glenn Washburn 2 siblings, 0 replies; 21+ messages in thread From: Patrick Steinhardt @ 2022-06-21 15:40 UTC (permalink / raw) To: Fabian Vogt Cc: grub-devel, Josselin Poiret, Michael Chang, Pierre-Louis Bonicoli, aplanas, Glenn Washburn [-- Attachment #1: Type: text/plain, Size: 2995 bytes --] On Tue, Jun 14, 2022 at 03:55:21PM +0200, Fabian Vogt wrote: > When using grub-probe with cryptodisk, the mapped block device from the host > is used directly instead of decrypting the source device in GRUB code. > In that case, the sector size and count of the host device needs to be used. > This is especially important when using luks2, which does not assign > total_sectors and log_sector_size when scanning, but only later when the > segments in the JSON area are evaluated. With an unset log_sector_size, > grub_open_device complains. > > This fixes grub-probe failing with > "error: sector sizes of 1 bytes aren't supported yet." > > Signed-off-by: Fabian Vogt <fvogt@suse.de> Reviewed-by: Patrick Steinhardt <ps@pks.im> > --- > v2: Moved new code from grub_cryptodisk_cheat_mount to grub_cryptodisk_open, > which allowed to simplify the code a bit. Also improved error handling. > v3: More liberal placement of variable declarations. > > grub-core/disk/cryptodisk.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > index c80cf9907..b3c76ef57 100644 > --- a/grub-core/disk/cryptodisk.c > +++ b/grub-core/disk/cryptodisk.c > @@ -709,16 +709,31 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk) > if (!dev) > return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "No such device"); > > - disk->log_sector_size = dev->log_sector_size; > - > #ifdef GRUB_UTIL > if (dev->cheat) > { > + grub_uint64_t cheat_dev_size; > + unsigned int cheat_log_sector_size; > + > if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd)) > dev->cheat_fd = grub_util_fd_open (dev->cheat, GRUB_UTIL_FD_O_RDONLY); > if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd)) > return grub_error (GRUB_ERR_IO, N_("cannot open `%s': %s"), > dev->cheat, grub_util_fd_strerror ()); > + > + /* Use the sector size and count of the cheat device */ > + cheat_dev_size = grub_util_get_fd_size (dev->cheat_fd, dev->cheat, &cheat_log_sector_size); > + if (cheat_dev_size == -1) > + { > + const char *errmsg = grub_util_fd_strerror (); > + grub_util_fd_close (dev->cheat_fd); > + dev->cheat_fd = GRUB_UTIL_FD_INVALID; > + return grub_error (GRUB_ERR_IO, N_("failed to query size of device `%s': %s"), > + dev->cheat, errmsg); > + } > + > + dev->log_sector_size = cheat_log_sector_size; > + dev->total_sectors = cheat_dev_size >> cheat_log_sector_size; > } > #endif > > @@ -732,6 +747,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk) > } > > disk->data = dev; > + disk->log_sector_size = dev->log_sector_size; > disk->total_sectors = dev->total_sectors; > disk->max_agglomerate = GRUB_DISK_MAX_MAX_AGGLOMERATE; > disk->id = dev->id; > -- > 2.36.1 > > > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device 2022-06-14 13:55 ` [PATCH v3] " Fabian Vogt 2022-06-14 18:18 ` Glenn Washburn 2022-06-21 15:40 ` Patrick Steinhardt @ 2022-08-11 18:22 ` Glenn Washburn 2 siblings, 0 replies; 21+ messages in thread From: Glenn Washburn @ 2022-08-11 18:22 UTC (permalink / raw) To: Fabian Vogt, Daniel Kiper Cc: grub-devel, Josselin Poiret, Patrick Steinhardt, Michael Chang, Pierre-Louis Bonicoli, aplanas Adding Daniel to this email. Glenn On Tue, 14 Jun 2022 15:55:21 +0200 Fabian Vogt <fvogt@suse.de> wrote: > When using grub-probe with cryptodisk, the mapped block device from the host > is used directly instead of decrypting the source device in GRUB code. > In that case, the sector size and count of the host device needs to be used. > This is especially important when using luks2, which does not assign > total_sectors and log_sector_size when scanning, but only later when the > segments in the JSON area are evaluated. With an unset log_sector_size, > grub_open_device complains. > > This fixes grub-probe failing with > "error: sector sizes of 1 bytes aren't supported yet." > > Signed-off-by: Fabian Vogt <fvogt@suse.de> > --- > v2: Moved new code from grub_cryptodisk_cheat_mount to grub_cryptodisk_open, > which allowed to simplify the code a bit. Also improved error handling. > v3: More liberal placement of variable declarations. > > grub-core/disk/cryptodisk.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > index c80cf9907..b3c76ef57 100644 > --- a/grub-core/disk/cryptodisk.c > +++ b/grub-core/disk/cryptodisk.c > @@ -709,16 +709,31 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk) > if (!dev) > return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "No such device"); > > - disk->log_sector_size = dev->log_sector_size; > - > #ifdef GRUB_UTIL > if (dev->cheat) > { > + grub_uint64_t cheat_dev_size; > + unsigned int cheat_log_sector_size; > + > if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd)) > dev->cheat_fd = grub_util_fd_open (dev->cheat, GRUB_UTIL_FD_O_RDONLY); > if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd)) > return grub_error (GRUB_ERR_IO, N_("cannot open `%s': %s"), > dev->cheat, grub_util_fd_strerror ()); > + > + /* Use the sector size and count of the cheat device */ > + cheat_dev_size = grub_util_get_fd_size (dev->cheat_fd, dev->cheat, &cheat_log_sector_size); > + if (cheat_dev_size == -1) > + { > + const char *errmsg = grub_util_fd_strerror (); > + grub_util_fd_close (dev->cheat_fd); > + dev->cheat_fd = GRUB_UTIL_FD_INVALID; > + return grub_error (GRUB_ERR_IO, N_("failed to query size of device `%s': %s"), > + dev->cheat, errmsg); > + } > + > + dev->log_sector_size = cheat_log_sector_size; > + dev->total_sectors = cheat_dev_size >> cheat_log_sector_size; > } > #endif > > @@ -732,6 +747,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk) > } > > disk->data = dev; > + disk->log_sector_size = dev->log_sector_size; > disk->total_sectors = dev->total_sectors; > disk->max_agglomerate = GRUB_DISK_MAX_MAX_AGGLOMERATE; > disk->id = dev->id; ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/4] osdep: detect LUKS2-encrypted devices 2020-05-30 12:25 [PATCH 0/4] Probing support for LUKS2 Patrick Steinhardt ` (2 preceding siblings ...) 2020-05-30 12:25 ` [PATCH 3/4] luks2: set up dummy sector size during scan Patrick Steinhardt @ 2020-05-30 12:25 ` Patrick Steinhardt 3 siblings, 0 replies; 21+ messages in thread From: Patrick Steinhardt @ 2020-05-30 12:25 UTC (permalink / raw) To: grub-devel [-- Attachment #1: Type: text/plain, Size: 3695 bytes --] While support for LUKS2 has landed already, grub-install(1) doesn't yet detect it as an installation target. Users of grub-install(1) may thus end up with a bootloader that cannot read the encrypted disk, rendering it unusable. As a first step towards auto-detection, this patch implements detection for device-mappers LUKS2 signature. As it's mostly similar to LUKS' original signature except for the incremented version number, detection mostly echoes what we have for LUKS already. Note that this doesn't yet implement auto-detection of required cryptographic modules. This is due to some limitations in the current implementation, where the assumption is that there's exactly one encrypted segment and KDF for a given crypto disk. Existing implementations for LUKS and Geli thus set up ciphers, hashes and KDFs during the scanning phase, which isn't possible for LUKS2 as there may be multiple ones. As a result, auto-detecting required modules will require additional refactoring and is thus postponed to a later point. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- grub-core/osdep/devmapper/getroot.c | 23 +++++++++++++++++++++-- include/grub/emu/getroot.h | 1 + util/getroot.c | 1 + 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c index a13a39c96..74401fd5a 100644 --- a/grub-core/osdep/devmapper/getroot.c +++ b/grub-core/osdep/devmapper/getroot.c @@ -148,6 +148,11 @@ grub_util_get_dm_abstraction (const char *os_dev) grub_free (uuid); return GRUB_DEV_ABSTRACTION_LUKS; } + if (strncmp (uuid, "CRYPT-LUKS2-", 12) == 0) + { + grub_free (uuid); + return GRUB_DEV_ABSTRACTION_LUKS2; + } grub_free (uuid); return GRUB_DEV_ABSTRACTION_NONE; @@ -184,8 +189,9 @@ grub_util_pull_devmapper (const char *os_dev) grub_util_pull_device (subdev); } } - if (uuid && strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - 1) == 0 - && lastsubdev) + if (uuid && lastsubdev + && (strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - 1) == 0 + || strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)) { char *grdev = grub_util_get_grub_dev (lastsubdev); dm_tree_free (tree); @@ -267,6 +273,19 @@ grub_util_get_devmapper_grub_dev (const char *os_dev) return grub_dev; } + case GRUB_DEV_ABSTRACTION_LUKS2: + { + char *dash; + + dash = grub_strchr (uuid + sizeof ("CRYPT-LUKS2-") - 1, '-'); + if (dash) + *dash = 0; + grub_dev = grub_xasprintf ("cryptouuid/%s", + uuid + sizeof ("CRYPT-LUKS2-") - 1); + grub_free (uuid); + return grub_dev; + } + default: grub_free (uuid); return NULL; diff --git a/include/grub/emu/getroot.h b/include/grub/emu/getroot.h index 73fa2d34a..be3faf500 100644 --- a/include/grub/emu/getroot.h +++ b/include/grub/emu/getroot.h @@ -30,6 +30,7 @@ enum grub_dev_abstraction_types { GRUB_DEV_ABSTRACTION_LVM, GRUB_DEV_ABSTRACTION_RAID, GRUB_DEV_ABSTRACTION_LUKS, + GRUB_DEV_ABSTRACTION_LUKS2, GRUB_DEV_ABSTRACTION_GELI, }; diff --git a/util/getroot.c b/util/getroot.c index 847406fba..07ad92317 100644 --- a/util/getroot.c +++ b/util/getroot.c @@ -101,6 +101,7 @@ grub_util_pull_device (const char *os_dev) grub_util_pull_lvm_by_command (os_dev); /* Fallthrough - in case that lvm-tools are unavailable. */ case GRUB_DEV_ABSTRACTION_LUKS: + case GRUB_DEV_ABSTRACTION_LUKS2: grub_util_pull_devmapper (os_dev); return; -- 2.26.2 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply related [flat|nested] 21+ messages in thread
end of thread, other threads:[~2022-08-11 18:22 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-30 12:25 [PATCH 0/4] Probing support for LUKS2 Patrick Steinhardt 2020-05-30 12:25 ` [PATCH 1/4] luks: fix out-of-bounds copy of UUID Patrick Steinhardt 2020-06-06 23:32 ` Petr Vorel 2020-05-30 12:25 ` [PATCH 2/4] luks2: strip dashes off of the UUID Patrick Steinhardt 2020-09-15 14:30 ` Daniel Kiper 2020-05-30 12:25 ` [PATCH 3/4] luks2: set up dummy sector size during scan Patrick Steinhardt 2021-08-06 4:51 ` Michael Chang 2021-08-08 14:20 ` Patrick Steinhardt 2021-12-16 15:52 ` Fabian Vogt 2021-12-22 18:17 ` Josselin Poiret 2022-02-04 15:46 ` Fabian Vogt 2022-02-07 13:15 ` Josselin Poiret 2022-05-21 0:13 ` Glenn Washburn 2022-05-21 10:53 ` Fabian Vogt 2022-06-13 14:29 ` [PATCH v2] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device Fabian Vogt 2022-06-14 2:19 ` Glenn Washburn 2022-06-14 13:55 ` [PATCH v3] " Fabian Vogt 2022-06-14 18:18 ` Glenn Washburn 2022-06-21 15:40 ` Patrick Steinhardt 2022-08-11 18:22 ` Glenn Washburn 2020-05-30 12:25 ` [PATCH 4/4] osdep: detect LUKS2-encrypted devices Patrick Steinhardt
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.