From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kaIdU-0000sU-SF for mharc-grub-devel@gnu.org; Wed, 04 Nov 2020 08:15:36 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:52576) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kaIdT-0000rm-UM for grub-devel@gnu.org; Wed, 04 Nov 2020 08:15:35 -0500 Received: from dibed.net-space.pl ([84.10.22.86]:60490) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:192) (Exim 4.90_1) (envelope-from ) id 1kaIdS-00080x-2a for grub-devel@gnu.org; Wed, 04 Nov 2020 08:15:35 -0500 Received: from router-fw.i.net-space.pl ([192.168.52.1]:59622 "EHLO tomti.i.net-space.pl") by router-fw-old.i.net-space.pl with ESMTP id S152313AbgKDNPZ (ORCPT ); Wed, 4 Nov 2020 14:15:25 +0100 X-Comment: RFC 2476 MSA function at dibed.net-space.pl logged sender identity as: dkiper Date: Wed, 4 Nov 2020 14:15:21 +0100 From: Daniel Kiper To: Glenn Washburn Cc: Daniel Kiper , Patrick Steinhardt , grub-devel@gnu.org, Denis GNUtoo Carikli Subject: Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors Message-ID: <20201104131521.2wttovsxfygbregz@tomti.i.net-space.pl> References: <20200908132119.6o4xaaubq6oe2vxh@tomti.i.net-space.pl> <94e72828-212b-49ed-8a59-e33a85f7afce@efficientek.com> <20200921112304.4wz3gbz5j3lfqhux@tomti.i.net-space.pl> <20201003004224.2e057dd4@crass-HP-ZBook-15-G2> <20201027191156.yfizbvbzjmkhjedd@tomti.i.net-space.pl> <20201029145334.2aa22c50@crass-HP-ZBook-15-G2> <20201030124926.svq2l4y7s7zdzfpj@tomti.i.net-space.pl> <25abba49-2f02-4f17-924a-64a96efdd2da@efficientek.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <25abba49-2f02-4f17-924a-64a96efdd2da@efficientek.com> User-Agent: NeoMutt/20170113 (1.7.2) Received-SPF: pass client-ip=84.10.22.86; envelope-from=dkiper@net-space.pl; helo=dibed.net-space.pl X-detected-operating-system: by eggs.gnu.org: First seen = 2020/11/04 07:44:31 X-ACL-Warn: Detected OS = Linux 2.2.x-3.x [generic] [fuzzy] X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 04 Nov 2020 13:15:36 -0000 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. > >> 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