From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kKFJV-0006MC-7e for mharc-grub-devel@gnu.org; Mon, 21 Sep 2020 02:28:37 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:50808) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kKFJS-0006LE-OB for grub-devel@gnu.org; Mon, 21 Sep 2020 02:28:34 -0400 Received: from mail-pf1-x444.google.com ([2607:f8b0:4864:20::444]:33064) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kKFJQ-0003AB-Ep for grub-devel@gnu.org; Mon, 21 Sep 2020 02:28:34 -0400 Received: by mail-pf1-x444.google.com with SMTP id z18so8407802pfg.0 for ; Sun, 20 Sep 2020 23:28:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficientek-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:message-id:in-reply-to:references:subject :mime-version:content-transfer-encoding; bh=+Sf4jIHtsuUXtQq3p9ZNTLNkw7vxfDcjq4mwzDdoy4M=; b=ToZhO7u18zycjrCOQtYmboTtsp17sZYs6QM0Ujf/blFUzM2yZ61cWuHjYX2OaBGnwL 5xEHYgkKmuTJVXTjMcVrEBqd9ngK+a/kX37E/IgFAVjXrToQ3BLkVKu5YmeN9aVKutmh a0EpelPMWCAHnkmbuCpsFUvlNPFOOFEnbYpBWcSeBNmo7GfGa15ryiv/jjPQDiwN/2jJ TutONQozIxjk0D2lIdNPAtqdPM0+Q4x0aqaRat9RlzM/gdo/C7N0JqOB8v5OT1pWU02J 53hbuVzpNXScnzirR0kvlhtIMvHp3IAnd3+r17+TfCpD3QDCXGAgzWgHbptKcLojnnHv bp5g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:message-id:in-reply-to :references:subject:mime-version:content-transfer-encoding; bh=+Sf4jIHtsuUXtQq3p9ZNTLNkw7vxfDcjq4mwzDdoy4M=; b=hMN3G1Q0RMEQbgkmf99et3wPtPK1FHEsl9TcYKFf0295Dmr/fz3NjgdKBtiPeyteAU MY38wPSlXs0SMilmiyrsA8pmqF5dN1MqXxzchiKT9BoQfXAQqU1+4uSDhNbfp4fw9nfk dB4CTXNJ8VncQOdeTtwsi6UykL18F2dtc8cCDOZLXMmZ6HgamNax3C7EGq4THiqf9dS+ rO/yMuuzSQt7nbuYMSZXi1pIEQIPZdMFBD/5jyDhtUSqt3ARjVBA1yR6HfjE4kJmr32Y AexFkqrN0SRXoFqevDBISUGhXPKg3v1wyWj94lBT0Ss57T3hW5tSpzczDrGkhhfaln7m dloA== X-Gm-Message-State: AOAM530afTDPEDII4k/vWPBAKnm5aXIBMGv2MT29XkZtBummZMXIhEG4 SQs30/WLbtPI1SekxRLXcf8bhA== X-Google-Smtp-Source: ABdhPJyEHJnaifBn6fH8Mvhx1JccIQe02WrccuLx7sZY1jCLaXhPYpUl0gaYmSl443KpPARBIofrhw== X-Received: by 2002:aa7:989c:0:b029:142:2501:3973 with SMTP id r28-20020aa7989c0000b029014225013973mr27013131pfl.56.1600669710642; Sun, 20 Sep 2020 23:28:30 -0700 (PDT) Received: from [127.0.0.1] ([172.58.46.198]) by smtp.gmail.com with ESMTPSA id x3sm9662323pjf.42.2020.09.20.23.28.29 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sun, 20 Sep 2020 23:28:30 -0700 (PDT) Date: Mon, 21 Sep 2020 06:28:28 +0000 (UTC) From: Glenn Washburn To: Daniel Kiper Cc: Patrick Steinhardt , grub-devel@gnu.org, Denis GNUtoo Carikli Message-ID: <94e72828-212b-49ed-8a59-e33a85f7afce@efficientek.com> In-Reply-To: <20200908132119.6o4xaaubq6oe2vxh@tomti.i.net-space.pl> References: <20200908132119.6o4xaaubq6oe2vxh@tomti.i.net-space.pl> Subject: Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Correlation-ID: <94e72828-212b-49ed-8a59-e33a85f7afce@efficientek.com> Received-SPF: pass client-ip=2607:f8b0:4864:20::444; envelope-from=development@efficientek.com; helo=mail-pf1-x444.google.com X-detected-operating-system: by eggs.gnu.org: No matching host in p0f cache. That's all we know. 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, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, 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: Mon, 21 Sep 2020 06:28:35 -0000 Sep 8, 2020 7:21:31 AM Daniel Kiper : > On Mon, Sep 07, 2020 at 05:27:46PM +0200, Patrick Steinhardt wrote: >> From: Glenn Washburn >> >> The total_length field is named confusingly because length usually refer= s to >> bytes, whereas in this case its really the total number of sectors on th= e >> 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 Patri= ck 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 n= ext release. I'll include that patch again in a follow up patch series. >> 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 sector= s. >> >> Signed-off-by: Glenn Washburn >> Reviewed-by: Patrick Steinhardt >> --- >> 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 =3D NULL; >> grub_size_t saltlen =3D sizeof (salt); >> -=C2=A0 char cipher[32], *p;; >> +=C2=A0 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 =3D sizeof (unsigned int) * 8 >> - __builtin_clz ((unsigned int) segment.sector_size) - 1; >> if (grub_strcmp (segment.size, "dynamic") =3D=3D 0) >> - crypt->total_length =3D grub_disk_get_size (disk) - crypt->offset; >> + crypt->total_length =3D (grub_disk_get_size (disk) >> (crypt->log_sect= or_size - disk->log_sector_size)) >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - cr= ypt->offset; >> else >> - crypt->total_length =3D grub_strtoull (segment.size, NULL, 10); >> + crypt->total_length =3D grub_strtoull (segment.size, NULL, 10) >> cryp= t->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 ab= out grub_strtoull, while valid, don't apply to this patch and should be add= ressed in a new 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 mor= e complicated if the last partial sector is at the end of the disk. > Daniel >