On Tue, Nov 03, 2020 at 08:21:15PM +0000, Glenn Washburn wrote: > Oct 30, 2020 7:50:08 AM Daniel Kiper <dki...@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 <dki...@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.ki...@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.ki...@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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
