Re: [CRYPTO-LUKS v1 03/19] cryptodisk: Incorrect calculation of start sector for grub_disk_read in grub_cryptodisk_read.

[Glenn Washburn]

On Sun, 23 Aug 2020 12:39:03 +0200
Patrick Steinhardt <ps@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 <development@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