[Top][All Lists]

[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: Glenn Washburn
Subject: Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors
Date: Fri, 6 Nov 2020 06:41:31 +0000 (UTC)

Nov 4, 2020 7:15:30 AM Daniel Kiper <>:

> On Tue, Nov 03, 2020 at 08:21:15PM +0000, Glenn Washburn wrote:
>> Oct 30, 2020 7:50:08 AM Daniel Kiper <>:
>>> On Thu, Oct 29, 2020 at 02:53:34PM -0500, Glenn Washburn wrote:
>>>> On Tue, 27 Oct 2020 20:11:56 +0100
>>>> Daniel Kiper <> 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 <> wrote:
>>>>>>> On Mon, Sep 21, 2020 at 06:28:28AM +0000, Glenn Washburn wrote:
>>>>>>>> Sep 8, 2020 7:21:31 AM Daniel Kiper <>:
>>>>>>>>> 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.

Normally you only check errno after a function that sets it fails. In this 
case, we know it sets it, but we don't know it fails. The problem with checking 
grub_errno when grub_strtoull() has not failed is that grub_errno can be 
non-zero from being by a failing function prior to grub_strtoull(). I see now 
that man strtoull suggests setting errno=0 before calling strtoull(). Wrapping 
the call in a grub_error_push/pop makes better sense here, so we can preserve 
any previous errors.

As an aside, grub_strtoull() doesn't handle strings prefixed by '-', while 
strtoull does. I think grub's makes more sense and suits the current purpose 

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

Yes, and I've written python code to do that. But it's not trivial because the 
header has a checksum and the part we'd be modifying is json. Grub doesn't 
actually verify the checksum, so we probably don't need to change it. But we 
will want a json parser. Since the build already requires python, I suppose 
there's no harm in having it as a dependency of tests too.

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

Ok, I'll combine this with grub_error_push/pop


reply via email to

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