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

[Patrick Steinhardt]

On Sun, Aug 23, 2020 at 11:31:46PM -0500, Glenn Washburn wrote:
> 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.  

I'd assume that things like the grub.env file get updated based e.g. on
the last chosen boot entry. Which would require support to write to
disk. But I honestly don't now and have never used write-functionality
in GRUB.

Do you want to update your patch to also fix the write part? I'd then
pull it into a v2 of my patch series for v2.06 fixes.

> > 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).

I'll have to have a look at them. Improved testability is definitely
something I'm interested in, mostly because I found the manual
compile-boot-test cycle quite unwieldy, even with virtualization.

> 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.

Cool, thanks a lot for explaining the issue in more depth!

Patrick

signature.asc
Description: PGP signature