[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 0/9] Cryptodisk fixes for v2.06
From: |
Patrick Steinhardt |
Subject: |
Re: [PATCH 0/9] Cryptodisk fixes for v2.06 |
Date: |
Mon, 24 Aug 2020 08:31:47 +0200 |
On Mon, Aug 24, 2020 at 01:22:20AM -0500, Glenn Washburn wrote:
> 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.
Oh, yeah. I've misunderstood the translation from disk sector size to
GRUB-native sector size.
I heavily agree with you that there should be some code cleanup after
v2.06 which does s/grub_disk_get_size/grub_disk_get_sectors/ for this
and other related code. It's just too easy to shoot yourself in the foot
here if you're not knowing a 100% what you're doing.
> 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.
No, I've read it again and will add my Reviewed-by for this 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 should've added a note in here about my fixup. And let's do
include it then.
> > 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).
I'll have a look at that patchset soonish.
Patrick
signature.asc
Description: PGP signature
- [PATCH 3/9] luks2: Fix use of incorrect index and some error messages, (continued)
- [PATCH 3/9] luks2: Fix use of incorrect index and some error messages, Patrick Steinhardt, 2020/08/23
- [PATCH 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors, Patrick Steinhardt, 2020/08/23
- [PATCH 5/9] luks2: Improve error reporting when decrypting/verifying key, Patrick Steinhardt, 2020/08/23
- [PATCH 6/9] cryptodisk: Unregister cryptomount command when removing module, Patrick Steinhardt, 2020/08/23
- [PATCH 7/9] cryptodisk: Incorrect calculation of start sector for grub_disk_read in grub_cryptodisk_read, Patrick Steinhardt, 2020/08/23
- [PATCH 8/9] cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain', Patrick Steinhardt, 2020/08/23
- [PATCH 9/9] cryptodisk: Properly handle non-512 byte sized sectors, Patrick Steinhardt, 2020/08/23
- Re: [PATCH 0/9] Cryptodisk fixes for v2.06, Glenn Washburn, 2020/08/24
- Re: [PATCH 0/9] Cryptodisk fixes for v2.06,
Patrick Steinhardt <=
- [PATCH v2 0/9] Cryptodisk fixes for v2.06, Patrick Steinhardt, 2020/08/26
- [PATCH v2 1/9] json: Remove invalid typedef redefinition, Patrick Steinhardt, 2020/08/26
- [PATCH v2 2/9] luks: Fix out-of-bounds copy of UUID, Patrick Steinhardt, 2020/08/26
- [PATCH v2 3/9] luks2: Fix use of incorrect index and some error messages, Patrick Steinhardt, 2020/08/26
- [PATCH v2 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors, Patrick Steinhardt, 2020/08/26
- [PATCH v2 5/9] luks2: Improve error reporting when decrypting/verifying key, Patrick Steinhardt, 2020/08/26
- [PATCH v2 6/9] cryptodisk: Unregister cryptomount command when removing module, Patrick Steinhardt, 2020/08/26