From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kOaK6-0002cR-Ve for mharc-grub-devel@gnu.org; Sat, 03 Oct 2020 01:43:11 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:33026) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kOaK2-0002cC-8R for grub-devel@gnu.org; Sat, 03 Oct 2020 01:43:07 -0400 Received: from mail-qv1-xf42.google.com ([2607:f8b0:4864:20::f42]:33867) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kOaJy-0001MJ-Km for grub-devel@gnu.org; Sat, 03 Oct 2020 01:43:05 -0400 Received: by mail-qv1-xf42.google.com with SMTP id q10so2860630qvs.1 for ; Fri, 02 Oct 2020 22:43:01 -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:subject:message-id:in-reply-to:references:reply-to :mime-version:content-transfer-encoding; bh=6pPsLO6cxKyZtvFDcLiqAP18u3K6e1qytZ35ZfmtLDA=; b=qoGkXZt3mM5v89RB1xAAXsH7aivpaOt9VVTUs2R/37msrV13k/jNFh8M0mEA73DewY VKpLL2ZjNwMeswrlZtfheYSNv/ECNhx7R2pF7FAOmKiSH8Q27EGeQtl6rIU2kDVvNLum g+76CnFSTOiwIiDS1yQWUww5iBUzmYvW4wTvgS2mYdy1OLaNchuszczrxNqlp2f8ziQ6 eo6unJVX/aufgi7K8DRGv5KV+7xTW78TNMr3gIvV2I1+jHUtCL1iUehtrKoSdFfKv1/p zL3PZ724diL5sO1mt7f/f8pKtS2YKz8cN5QsLB84YUZCBVyok38ujgMsCCVWkpA91naa 8fKQ== 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:subject:message-id:in-reply-to :references:reply-to:mime-version:content-transfer-encoding; bh=6pPsLO6cxKyZtvFDcLiqAP18u3K6e1qytZ35ZfmtLDA=; b=s3jyzXan3qfzsviHxNszOqgqpR/KzQTicqnGuQh74z75lHJ8UTeTOvYw5V2zZFyP8q 48hiuzK84eYhAzM4rmEZBEYElXack09LKnYJwMZaC7oq0jBymJsF0jSEqDA1gVmhAaNp cMSSrKYch2E5636jNvVkaa1yWuAB8CeWuH6t/FyAIjT1Lg+CUqiVSEqkwXsN267ldAgl Dw3/NRpJvOxf2hxokKor7P29hs07bTDSOc9kWE9Zwd1jwvcTpYQ/RwPoJeXhtP/94Xoa H6SpaKoLoOxXY9NfhvS9Qcb7E38IPE94+QYOk7b8NMLQwxp+tMz9K445/mAQqskmiTgG hTmQ== X-Gm-Message-State: AOAM533H33rtvGBSpVY5hB6/GmNY2trSAo0vbJ4H2sUg6KA7r83VdBg5 mDwB+UtbhGOY1L/H8cixZBlivQ== X-Google-Smtp-Source: ABdhPJyV3r1/DRVb7MltnUP1k8o7wHuRJDXNn79zf+W653jD9ZwpWZevcBu9N37MiQKjMLusSoDJDA== X-Received: by 2002:a0c:b39a:: with SMTP id t26mr5373937qve.19.1601703780742; Fri, 02 Oct 2020 22:43:00 -0700 (PDT) Received: from crass-HP-ZBook-15-G2 ([136.49.44.103]) by smtp.gmail.com with ESMTPSA id j11sm2702148qko.111.2020.10.02.22.43.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 02 Oct 2020 22:43:00 -0700 (PDT) Date: Sat, 3 Oct 2020 00:42:55 -0500 From: Glenn Washburn To: Daniel Kiper 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: <20201003004224.2e057dd4@crass-HP-ZBook-15-G2> In-Reply-To: <20200921112304.4wz3gbz5j3lfqhux@tomti.i.net-space.pl> References: <20200908132119.6o4xaaubq6oe2vxh@tomti.i.net-space.pl> <94e72828-212b-49ed-8a59-e33a85f7afce@efficientek.com> <20200921112304.4wz3gbz5j3lfqhux@tomti.i.net-space.pl> Reply-To: development@efficientek.com X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Received-SPF: pass client-ip=2607:f8b0:4864:20::f42; envelope-from=development@efficientek.com; helo=mail-qv1-xf42.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: Sat, 03 Oct 2020 05:43:08 -0000 On Mon, 21 Sep 2020 13:23:04 +0200 Daniel Kiper wrote: > 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. >=20 > Please do. I want to have this fixed before 2.06 release... >=20 > > >> 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 =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_sector_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 = - crypt->offset; > > >> else > > >> - crypt->total_length =3D grub_strtoull (segment.size, NULL, 10); > > >> + crypt->total_length =3D 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. >=20 > 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. 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? > > 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. >=20 > 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? Glenn