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

[Daniel Kiper <dkiper@net-space.pl>]

On Thu, Oct 29, 2020 at 02:53:34PM -0500, Glenn Washburn wrote:
> On Tue, 27 Oct 2020 20:11:56 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
> > 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.
>
> Actually, I do think this is a good excuse to do things wrong _IF_ what
> is wrong is also wrong in the original code.  Are you going to reject a
> patch that _fixes_ a bug, because it does not fix a different bug around
> it? So after rejecting the patch, then _both_ bugs will continue to
> exist? This seems quite ridiculous to me.

No, this was never my goal. I am just trying to find the best solution 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.
>
> I think part of my pushback on this is I don't see a good solution. How

OK...

> do you know when grub_strtoull() errors here?  And even if it doesn't

Just check values/errors returned by it?

> error, how do you know that a segment.size of 3 or 128 won't cause a
> crash? I don't have good answers to these questions.  If grub does

This question is more difficult and I am afraid that you are right.

> crash, then a bug has been exposed in grub which should be fixed.

If possible we should prevent against the crashes but I am also aware
that it is not always feasible to predict when the GRUB will crash.

> Perhaps if we had functional testing (see my previous patch series
> implementing functional testing), we could test this.  But even then

I will try to take a look at it next week...

> there's a problem, how do we know some grub file system or disk doesn't
> have a crashing bug on too small of disks?

We can improve a situation a bit here by running some tests you are
mentioning above.

> > > > > 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
>
> Since you seem to have a clear idea of what should be done here,
> perhaps you insert a patch to your liking?  Or just tell me exactly
> what you think should be done to protect against crashes.  I can just
> add a zero check if that's what you want.  But that just adding a
> "check" to check a box and make people feel safe and comfortable that

I am not interested in adding `"check" to check a box`...

> something is being done, when in fact it may do little to fix a
> potential crash.  When you say "somebody feeds it with invalid data", I
> take you to be concerned about someone maliciously crafting data to
> exploit grub, in which case a more in-depth audit of the use of

Yep...

> total_length and offset should be done.  Perhaps a compromise would be

That would be perfect.

> a comment saying "FIXME: Verify that grub does not crash for any value
> of total_length, offset, and sector_size combination."

OK but I would be more happy if you add to the compromise a promise that
you will continue the work on the functional testing mentioned above... :-)

> Honestly, I'm frustrated at how much time this whole patch series is
> requiring of me and dragging on. I feel like this patch is being held

This is partially by my lack of time. However, I hope it will be
changing. Anyway, sorry about the delays on my side.

> hostage in order to strong-arm me in to fixing something unrelated to
> my patch.

I think it is related to some extend. Anyway, I am open to discuss any
solution to this issue except ignoring it. Though I think we are close
to the compromise... :-)

Daniel