grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max num


From: Daniel Kiper
Subject: Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors
Date: Wed, 4 Nov 2020 14:15:21 +0100
User-agent: NeoMutt/20170113 (1.7.2)

On Tue, Nov 03, 2020 at 08:21:15PM +0000, Glenn Washburn wrote:
> Oct 30, 2020 7:50:08 AM 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:

[...]

> >>> 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?
>
> There are two error values returned by grub_strtoull(), 0 and ~0ULL
> for unrecognized number and overflow. However, these are _both_ valid
> non-error return values. So was it an overflow condition or a valid
> return when ~0ULL is returned? Same for 0. In the case of 0 while it
> may be valid, it wouldn't reflect a usable segment, so we can filter
> out those.

That is why you should check grub_errno too. In general "man strtoull"
is your friend. However, I agree that this does not prevent against
overflows later. So, I think we should have error checks for
grub_strtoull() to detect incorrect input and further some other checks
for overflows in math if needed. Potentially we can use only the latter
if we put these protection properly.

> >> 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.
>
> It would be good to detect where grub is crashing because there might
> be other ways to trigger such a crash (perhaps through loopback?)

Yeah, but I think we get back to tests here...

[...]

> >> 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... :-)
>
> Hmm, let me look into it before making a promise. The part with the
> most work will be adding the ability to create LUKS2 devices that
> cryptsetup does not currently allow (eg. One with a zero length
> segment or something grub_strtoull() will error on).

Could not you create correct image and break it later by overwriting
some parts of it?

> >> 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... :-)
>
> I think a good compromise would be to error on segments where offset >
> segment.size and crypt->total_length == 0. And to add a  fixme comment
> to handle the overflow case for grub_strtoull() better. Overflow won't
> cause a crash, just the area larger than the overflow amount to be
> inaccessible. And I don't think we need the previously mentioned
> fixme, but I'm not opposed to adding it.

Make sense for me.

Daniel



reply via email to

[Prev in Thread] Current Thread [Next in Thread]