From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kFdZ7-0006OX-KU for mharc-grub-devel@gnu.org; Tue, 08 Sep 2020 09:21:41 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:49692) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kFdZ1-0006NX-LA for grub-devel@gnu.org; Tue, 08 Sep 2020 09:21:35 -0400 Received: from aserp2120.oracle.com ([141.146.126.78]:35934) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kFdYz-0006ZO-GT for grub-devel@gnu.org; Tue, 08 Sep 2020 09:21:35 -0400 Received: from pps.filterd (aserp2120.oracle.com [127.0.0.1]) by aserp2120.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 088DKcGS064858; Tue, 8 Sep 2020 13:21:27 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 : in-reply-to; s=corp-2020-01-29; bh=32TLN8btqcj1T77VYHhqiTMO2ELDS6Oevzh2xIW272Y=; b=dU5ybPfhMojpykbh1LfXDj0+Dfz1/eUyTjaVPB822FOlth5M37249bdLDhzmJ5hOksuR eA/s1oL7/SBl8lLIfys8prERHtZYujFVCw6EpzO3tbSpL5ewT6pAE9IegkDCfDwXRiCM 6xWDLsY8cC32xZfYhP0iik1GsXrsgwMr31OqMYBGsy8CyRjJ3rg9Dg3osM0nQpTw19bg vpfSucTlWjxSCBxck/9oKk6vxxT6kVgD4Ddjm9MPLJmUOcmzkMvFwPA2iwE/gnkAj6V9 CN3mj08zOfP9oUXFCpqaLWKwRHr+EB8XLuak38MUehbKeaQ0KdzYktV4xLvX47W0LFhg CA== Received: from aserp3020.oracle.com (aserp3020.oracle.com [141.146.126.70]) by aserp2120.oracle.com with ESMTP id 33c2mkue0x-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 08 Sep 2020 13:21:27 +0000 Received: from pps.filterd (aserp3020.oracle.com [127.0.0.1]) by aserp3020.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 088DF1W0172079; Tue, 8 Sep 2020 13:21:27 GMT Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by aserp3020.oracle.com with ESMTP id 33cmk3dmen-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 08 Sep 2020 13:21:27 +0000 Received: from abhmp0018.oracle.com (abhmp0018.oracle.com [141.146.116.24]) by aserv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 088DLO1n009308; Tue, 8 Sep 2020 13:21:25 GMT Received: from tomti.i.net-space.pl (/10.175.199.180) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Tue, 08 Sep 2020 06:21:23 -0700 Date: Tue, 8 Sep 2020 15:21:19 +0200 From: Daniel Kiper To: Patrick Steinhardt Cc: grub-devel@gnu.org, Denis GNUtoo Carikli , Glenn Washburn Subject: Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors Message-ID: <20200908132119.6o4xaaubq6oe2vxh@tomti.i.net-space.pl> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9737 signatures=668679 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 malwarescore=0 phishscore=0 mlxlogscore=999 bulkscore=0 adultscore=0 mlxscore=0 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2006250000 definitions=main-2009080126 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9737 signatures=668679 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 priorityscore=1501 phishscore=0 adultscore=0 bulkscore=0 clxscore=1015 mlxlogscore=999 malwarescore=0 suspectscore=0 lowpriorityscore=0 spamscore=0 impostorscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2006250000 definitions=main-2009080126 Received-SPF: pass client-ip=141.146.126.78; envelope-from=daniel.kiper@oracle.com; helo=aserp2120.oracle.com X-detected-operating-system: by eggs.gnu.org: First seen = 2020/09/08 09:21:32 X-ACL-Warn: Detected OS = Linux 3.1-3.10 [fuzzy] X-Spam_score_int: -43 X-Spam_score: -4.4 X-Spam_bar: ---- X-Spam_report: (-4.4 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, 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: Tue, 08 Sep 2020 13:21:36 -0000 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? > 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. > 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. Daniel