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: > > > > > >> From: Glenn Washburn <developm...@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 <developm...@efficientek.com> > > > > > >> Reviewed-by: Patrick Steinhardt <p...@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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
