Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors

[Glenn Washburn <development@efficientek.com>]

On Mon, 21 Sep 2020 13:23:04 +0200
Daniel Kiper <daniel.kiper@oracle.com> wrote:

> On Mon, Sep 21, 2020 at 06:28:28AM +0000, Glenn Washburn wrote:
> > Sep 8, 2020 7:21:31 AM Daniel Kiper <daniel.kiper@oracle.com>:
> > > On Mon, Sep 07, 2020 at 05:27:46PM +0200, Patrick Steinhardt
> > > wrote:
> > >> From: Glenn Washburn <development@efficientek.com>
> > >>
> > >> The total_length field is named confusingly because length
> > >> usually refers to bytes, whereas in this case its really the
> > >> total number of sectors on the device. Also counter-intuitively,
> > >> grub_disk_get_size returns the total
> > >
> > > Could we change total_length name? Or should it stay as is
> > > because this name is used in other implementations too?
> >
> > I sent a patch which renamed total_length to total_sectors. I
> > believe Patrick chose not to include it because I did not fix a bug
> > in the code and this patch series was only patches he thought
> > essential to be included in the next release. I'll include that
> > patch again in a follow up patch series.
> 
> Please do. I want to have this fixed before 2.06 release...
> 
> > >> number of device native sectors sectors. We need to convert the
> > >> sectors from the size of the underlying device to the cryptodisk
> > >> sector size. And segment.size is in bytes which need to be
> > >> converted to cryptodisk sectors.
> > >>
> > >> Signed-off-by: Glenn Washburn <development@efficientek.com>
> > >> Reviewed-by: Patrick Steinhardt <ps@pks.im>
> > >> ---
> > >> grub-core/disk/luks2.c | 7 ++++---
> > >> 1 file changed, 4 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > >> index c4c6ac90c..5f15a4d2c 100644
> > >> --- a/grub-core/disk/luks2.c
> > >> +++ b/grub-core/disk/luks2.c
> > >> @@ -417,7 +417,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> > >> grub_uint8_t salt[GRUB_CRYPTODISK_MAX_KEYLEN];
> > >> grub_uint8_t *split_key = NULL;
> > >> grub_size_t saltlen = sizeof (salt);
> > >> -  char cipher[32], *p;;
> > >> +  char cipher[32], *p;
> > >
> > > I am OK with changes like that but they should be mentioned
> > > shortly in the commit message.
> >
> > Noted, I'll put update the commit message.
> >
> > >> const gcry_md_spec_t *hash;
> > >> gcry_err_code_t gcry_ret;
> > >> grub_err_t ret;
> > >> @@ -603,9 +603,10 @@ luks2_recover_key (grub_disk_t disk,
> > >> crypt->log_sector_size = sizeof (unsigned int) * 8
> > >> - __builtin_clz ((unsigned int) segment.sector_size) - 1;
> > >> if (grub_strcmp (segment.size, "dynamic") == 0)
> > >> - crypt->total_length = grub_disk_get_size (disk) -
> > >> crypt->offset;
> > >> + crypt->total_length = (grub_disk_get_size (disk) >>
> > >> (crypt->log_sector_size - disk->log_sector_size))
> > >> +            - crypt->offset;
> > >> else
> > >> - crypt->total_length = grub_strtoull (segment.size, NULL, 10);
> > >> + crypt->total_length = grub_strtoull (segment.size, NULL, 10)
> > >> >> crypt->log_sector_size;
> > >
> > > I do not like that you ignore grub_strtoull() errors.
> > > Additionally, what will happen if segment.size is smaller than
> > > LUKS2 sector size? Should not you round segment.size up to the
> > > nearest multiple of LUKS2 sector size first? I think the same
> > > applies to the earlier change too.
> >
> > Again, I was making a minimal set of changes for this fix. Your
> > comments about grub_strtoull, while valid, don't apply to this patch
> > and should be addressed in a new patch.
> 
> OK, please fix it then in separate patch.

I've now looked in this more and feel that ignoring grub_strtoull()
errors is not a bad idea.  There are two error states where the
return value is either 0 if the first character is not a valid digit or
(1<<64)-1 in the case of overflow (actually could be more depending on
size of long long type). If grub_strtoull() returns 0 as an error, then
the segment size string is not compliant with the specification.  If
grub_strtoull() returns an error because of overflow, then the segment
size is greater than 16 exbibytes or 16777216 tebibytes.  If someone
has that size storage capacity, I'll wager they are not booting grub
off that storage.  And even if I'm wrong, its an even more
astronomically improbable that they would need to read past the 16th
exbibyte.  As is, this error case would still allow decrypting LUKS
sectors up to the 16th exbibyte.

Also, I looked at grub_strtoull() in hdparm.c, acpi.c, xnu_uuid.c, and
iorw.c and none of those do any error checking of grub_strtoull()
errors.  In fact, this could have serious implications for a typo in
iorw.

My professional conclusion is that I see no reason to do any error
checking.  Do you have a suggestion on how you would like the
grub_strtoull() errors handled?

> > Your concern about rounding segment.size up, is also valid and
> > pertinent to this patch, I'll update that in a following patch
> > series. This may get more complicated if the last partial sector is
> > at the end of the disk.
> 
> Yeah, but please try to fix it somehow...

On second thought, this is an edge case for a nonexistent problem.  If
segment.size is smaller than the LUKS2 sector size, then you have a
segment size of less than 4K, the current max supported sector size.
And having a filesystem smaller than 4K is pathological and I dare say
not supported by any filesystem supported by linux.

There's a more general problem of a segment size that is not a multiple
of the sector size.  In this case, there could be unreadable (by grub)
data at the end of the device.  But again, this is not something we
should worry about.  The cryptsetup program will refuse to create LUKS2
devices where the disk size is not a multiple of the sector size. It
will give the error: "Device size is not aligned to requested sector
size." The only ways I can think of where the segment size is not a
multiple of sector size is if the segment size string is corrupted or
set incorrectly.  In either case, reading the last partial sector isn't
going to matter.  The same logic holds for the case where sector size
is "dynamic".

So currently, I do not think we should support reading partial LUKS2
sectors at the end of a LUKS2 device.  And regardless, whether or not
we support reading partial sectors should not be something that
prevents this patch, which fixes a bug, from being merged.  Do you
disagree?

Glenn