Re: [PATCH 0/9] Cryptodisk fixes for v2.06

[Glenn Washburn <development@efficientek.com>]

On Sun, 23 Aug 2020 12:59:47 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> Hi,
> 
> I've sifted through the mailing list contents of the last few months
> to cherry-pick cryptodisk bugfixes which I think should be included
> in the v2.06 release. I've found the following 9 patches from Glenn
> and me which should probably be included, separated them out from
> their respective patch series and made them play nice with each other.
> 
> This patch series shouldn't be applied as-is, but my intention is
> instead to bundle all fixes which apply to v2.06 in a single thread to
> make discussion easier and help us keep track of what needs to be
> done. I've got some comments which I've sent to the original threads
> already and added notes below.
> 
> - luks2: grub_cryptodisk_t->total_length is the max number of device
>   native sectors
> 
>     I'm not sure if this fix is correct, mostly because I think that
>     `grub_disk_get_size` is buggy already: it returns sectors for
>     partitions and the total size for disks. So I do think we need
>     another patch to fix that function, too.

Its not clear to me what "total size" means here.  However,
`grub_disk_get_size` returns sectors for non-partition disks as well.
Note, that it returns the total number of grub native sector-sized
sectors (ie 512 byte sectors).  I do think that the _size suffix is
misleading and should be named `grub_disk_get_sectors` or something
similar.

Is there something I can do to clear up the uncertainty around this
fix?  I suspect part of the confusion lies in the fact that the
total_length field is actually in cryptodisk sector-sized sectors. We
know this because in `grub_cryptodisk_open` in cryptodisk.c the
total_length field is being set unmodified to the total_sectors field of
a `grub_disk_t` and `grub_disk_get_size` assume that total_sectors
needs to be converted from disk native to grub native sector-sized
sectors.

> - cryptodisk: Incorrect calculation of start sector for grub_disk_read
>   in grub_cryptodisk_read
> 
>     The patch looks correct to me and matches what both LUKS and LUKS2
>     on-disk format say. But I'm surprised our code ever worked
> correctly without this fix, which does make me feel uncomfortable.

In case its missed, I've explained this in my response to your response
to my original patch.

> - cryptodisk: Properly handle non-512 byte sized sectors
> 
>     Should we pick this for v2.06? It definitely fixes things, but
> also feels a bit like feature-enablement.

I see you fixed the bug in my patch where IV_PLAIN IVs were not being
calculated, thanks.  Considering grubs slow release cycle, I think its
better to include this or at least make a note that non-512-byte
sectors are currently not supported and have cryptomount fail with an
appropriate error message if it detects trying to open a LUKS2 device
with sector size not equal to 512.  As it is, I think LUKS2 support
will have people thinking that non-default sector sizes are supported.

> I've added my Reviewed-by to those patches which look obviously
> correct to me.
> 
> Glenn, please let me know if this somehow interferes with your work or
> if you'd like to handle upstreaming of those fixes yourself.

I would like to get these patches in a quickly as possible and I
suspect this patchset is probably that route, so I'm pleased to have
these patches in here.  

As a reminder, my CRYPTOMOUNT-TESTS patchset can test the full patch
set (not individual patches because non-512-byte sector support
requires multiple patches in this patchset).

Glenn