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. > 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... Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
