On Sun, 23 Aug 2020 12:39:03 +0200
Patrick Steinhardt <p...@pks.im> wrote:
> On Fri, Jul 31, 2020 at 07:01:44AM -0500, Glenn Washburn wrote:
> > Here dev is a grub_cryptodisk_t and dev->offset is offset in
> > sectors of size native to the cryptodisk device. The sector is
> > correctly transformed into native grub sector size, but then added
> > to dev->offset which is not transformed. It would be nice if the
> > type system would help us with this.
> >
> > Signed-off-by: Glenn Washburn <developm...@efficientek.com>
> > ---
> > grub-core/disk/cryptodisk.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c
> > b/grub-core/disk/cryptodisk.c index d8f66e9ef..2791a4870 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -757,8 +757,8 @@ grub_cryptodisk_read (grub_disk_t disk,
> > grub_disk_addr_t sector, size, sector, dev->offset);
> >
> > err = grub_disk_read (dev->source_disk,
> > - (sector << (disk->log_sector_size
> > - - GRUB_DISK_SECTOR_BITS)) +
> > dev->offset, 0,
> > + ((sector + dev->offset) <<
> > (disk->log_sector_size
> > + -
> > GRUB_DISK_SECTOR_BITS)), 0, size << disk->log_sector_size, buf);
> > if (err)
> > {
>
> So for LUKS2, we initialize `crypt->offset` with `crypt->offset =
> grub_divmod64 (segment.offset, segment.sector_size, NULL)` where
> `segment.offset` is the offset in bytes and `segment.sector_size` is
> the sector size. So yup, it's in sectors.
>
> For LUKS1, we do `newdev->offset = grub_be_to_cpu32
> (header.payloadOffset)`, which also is in sectors of 512 bytes.
>
> So the fix does seem correct to me, but I think it's incomplete as
> we'd have to do the same for `grub_cryptodisk_write`.
Yep, good catch. I didn't even think to check for grub_cryptodisk_write
since I tend to think of grub as read-only. I'm actually not sure how
to trigger a disk write in grub. The only thing that comes to mind is
I think there's a way to set some partition flags.
> Out of curiosity: did you test these changes?
Yes, these changes have definitely been tested. In fact, I found these
bugs precisely because I was trying to get grub to decrypt my LUKS2
device which has a 4096 byte sector size. Feeling like I wasn't
getting much traction on my patches, I wrote the CRYPTOMOUNT-TEST patch
series to allow independent verification of the bugs and my fixes.
There are a series of tests for block sizes 1024, 2048, and 4096 which
all are successful for me (all the sector size fixes are needed).
Now that I'm looking at grub_cryptodisk_read again, it looks like the
GRUB_UTIL part doesn't even take in to account the offset. So that's
probably not working either. I'm not exactly sure how to test that
code right yet either.
> The offset should always
> be a positive number, except with a detached header. So wouldn't we
> always have hit this bug if this behaviour was indeed wrong?
Yes, offset can only be zero in a detached header scenario. The key
here to why this issue was never hit is because LUKS1 and grub
native disks have a hardcoded 512-byte sector size (ie.
disk->log_sector_size == GRUB_DISK_SECTOR_BITS == 9 ). So
(disk->log_sector_size - GRUB_DISK_SECTOR_BITS) == 0 and x << 0 == x.
Effectively, the bit shift was always a noop and still is for 512-byte
sector cryptodisks, which is why the LUKS2 support also works in the
default sector-size case of 512-bytes.
Glenn
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
