From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kKJuj-0000Ol-2m for mharc-grub-devel@gnu.org; Mon, 21 Sep 2020 07:23:21 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:40992) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kKJuh-0000OC-OL for grub-devel@gnu.org; Mon, 21 Sep 2020 07:23:19 -0400 Received: from userp2130.oracle.com ([156.151.31.86]:47994) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kKJuf-0008M5-MY for grub-devel@gnu.org; Mon, 21 Sep 2020 07:23:19 -0400 Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 08LBK1uT142270; Mon, 21 Sep 2020 11:23:12 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=date : from : to : cc : subject : message-id : references : mime-version : content-type : content-transfer-encoding : in-reply-to; s=corp-2020-01-29; bh=rcfQzp3lqo1iXIVGXo4oYmP4M2rXUFaIETLhJb0HDF8=; b=eJ80mksmlyaStdtnG/elhUyO0uEHy9M+GzLY+BdCHuLMQXntimt154BBwPHskZKEGML7 KnyFPJiRxwhUoyujAc8m2RSgos5Zt5s5B2JpWlbkSO4VQaHaOAz78I/4zoUSJ+Q5bgqG n5Pk18tKJzCutE2VDoxR4g06OaQdV1jU7aQMFZCOKX8FIgx14IEVms98WWjSqtHJwGQP kGWca6VXEKYMlOpLgVjOxrVlczk9ObBorw9djmDKUARy5zU4ZbnPKu8UJ+S9oYA6pjRY 7iNOcjXy0lJzp3xDe5tE8d0tZb8CZgHt8v28h2zdbyLQW4cqY1YvTdSBT/VOFNvi3TBR NQ== Received: from aserp3030.oracle.com (aserp3030.oracle.com [141.146.126.71]) by userp2130.oracle.com with ESMTP id 33n9dqw2wb-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 21 Sep 2020 11:23:12 +0000 Received: from pps.filterd (aserp3030.oracle.com [127.0.0.1]) by aserp3030.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 08LBKINd026084; Mon, 21 Sep 2020 11:23:11 GMT Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by aserp3030.oracle.com with ESMTP id 33nujkhyaq-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 21 Sep 2020 11:23:11 +0000 Received: from abhmp0013.oracle.com (abhmp0013.oracle.com [141.146.116.19]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id 08LBN86U032471; Mon, 21 Sep 2020 11:23:09 GMT Received: from tomti.i.net-space.pl (/10.175.223.167) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Mon, 21 Sep 2020 04:23:08 -0700 Date: Mon, 21 Sep 2020 13:23:04 +0200 From: Daniel Kiper To: Glenn Washburn Cc: 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: <20200921112304.4wz3gbz5j3lfqhux@tomti.i.net-space.pl> References: <20200908132119.6o4xaaubq6oe2vxh@tomti.i.net-space.pl> <94e72828-212b-49ed-8a59-e33a85f7afce@efficientek.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <94e72828-212b-49ed-8a59-e33a85f7afce@efficientek.com> User-Agent: NeoMutt/20170113 (1.7.2) X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9750 signatures=668679 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 malwarescore=0 mlxlogscore=999 phishscore=0 adultscore=0 spamscore=0 suspectscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2006250000 definitions=main-2009210084 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9750 signatures=668679 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 mlxlogscore=999 phishscore=0 priorityscore=1501 malwarescore=0 mlxscore=0 impostorscore=0 clxscore=1015 lowpriorityscore=0 suspectscore=0 spamscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2006250000 definitions=main-2009210084 Received-SPF: pass client-ip=156.151.31.86; envelope-from=daniel.kiper@oracle.com; helo=userp2130.oracle.com X-detected-operating-system: by eggs.gnu.org: First seen = 2020/09/21 07:23:16 X-ACL-Warn: Detected OS = Linux 3.1-3.10 [fuzzy] X-Spam_score_int: -58 X-Spam_score: -5.9 X-Spam_bar: ----- X-Spam_report: (-5.9 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-1.455, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001, UNPARSEABLE_RELAY=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 11:23:19 -0000 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: > >> From: Glenn Washburn > >> > >> 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 > >> 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 = 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