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

[Daniel Kiper]

On Sat, Oct 03, 2020 at 12:42:55AM -0500, Glenn Washburn wrote:
> 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.

I know about these issues and I think they should be fixed at some
point. Or at least there should be a comment added why it is safe here
and there to ignore grub_strtoull() errors. Anyway, it is not an excuse
to do things in wrong way if we are touching this code here.

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

What is the worst scenario if somebody plays bad games with segment.size
string? If nothing dangerous happens I am OK with the comment explaining
why it is safe to ignore grub_strtoull() errors here.

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

I am not saying we should care about such crazy scenarios. However,
I care a lot if GRUB fails safely in cases where somebody feeds it
with invalid data. So, please add code which protects against crashes
or explain in the comments why such protections are not needed.

Daniel