On Sun, 23 Aug 2020 12:59:47 +0200 Patrick Steinhardt <p...@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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
