From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kA5sE-00016o-B2 for mharc-grub-devel@gnu.org; Mon, 24 Aug 2020 02:22:30 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:33598) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kA5sC-00015z-Iy for grub-devel@gnu.org; Mon, 24 Aug 2020 02:22:28 -0400 Received: from mail-io1-xd44.google.com ([2607:f8b0:4864:20::d44]:45383) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kA5sA-0006Hn-HU for grub-devel@gnu.org; Mon, 24 Aug 2020 02:22:28 -0400 Received: by mail-io1-xd44.google.com with SMTP id u126so7479650iod.12 for ; Sun, 23 Aug 2020 23:22:25 -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=BTi+Zy5YIazYOCuk6yxbKFEUZQ+vMIVH9p+LmpaC8Zs=; b=lTr4hKQG5zapp7K5ThLDFXQXLcB7s5w924ivjkxt/my9gQUEzBu+Nd/4+QbC7548rG RzGWoGvidXgnUyX+vCB8PUAI3A4NFfwpcYKAyInHKazpkjkul+qe+oXnCUfg6I8pEHzM 3HAiXoTDjksvJaMcRVT4ACGawM2G7VWMbqbbRm8TjhmUdZ5bqioDO+Gbkp7lmGJFjHuK WM6e41p/aaqhlwsv7jl7ag40RWClozRlTdDucY/cm+gqDThktxzmhM1UgcxeaXi6QtQG Z4m4wVf5BJqIFcxqIqinMCfZv3mmi5QZ/4m2IepDgco6b62n10KNfO0Afqm3QdDsbZNf 45+g== 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=BTi+Zy5YIazYOCuk6yxbKFEUZQ+vMIVH9p+LmpaC8Zs=; b=bprMAKCRpQlzJ1+idAFYpe0Zc0zz+2fgyC765VPUeBSbVJiJKM6c3mQ3Lozwaho5Wc c16gRjwMKSfLkReeOcvubBtWhp6YviN8eVYts4PSPb0AWRYm65tnD48rjoZyjaDE60K/ teR3nImRSQHqDKQroLdMg4JlhK6tH6m1OStHXzC8nCpMxJwHN39HiG63ZKYNajNHsafu 6hHEOg5+IOsQ9E4Rf/dC99/1s4WnP2x/de7DitPDWIpaPCSHeAypX5Rk3bDK1LluoPRz 24KqOrush2I4Vg6uqUeR01GBS/yPwQu6hvpKFs9igSJUCvxZ2eFOIe6zmWq3d/E2Gl/I seCQ== X-Gm-Message-State: AOAM533kUHvWgndJW5VKWO1w2zjpkcrnHco7wpiYnSidARLVWAdAABuR vsl9Ox6uW5mqwKHY9fA638wf2g== X-Google-Smtp-Source: ABdhPJxZKWkvFNYON5uQHmektF55VX6EnTd1ZmmYV8l92+VAwSL2VJGl76jLQwOHa3HTZQsahb1U7Q== X-Received: by 2002:a05:6602:1495:: with SMTP id a21mr3664188iow.46.1598250145008; Sun, 23 Aug 2020 23:22:25 -0700 (PDT) Received: from crass-HP-ZBook-15-G2 ([136.49.44.103]) by smtp.gmail.com with ESMTPSA id x185sm6256961iof.41.2020.08.23.23.22.24 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 23 Aug 2020 23:22:24 -0700 (PDT) Date: Mon, 24 Aug 2020 01:22:20 -0500 From: Glenn Washburn To: Patrick Steinhardt Cc: grub-devel@gnu.org, Denis 'GNUtoo' Carikli , Daniel Kiper Subject: Re: [PATCH 0/9] Cryptodisk fixes for v2.06 Message-ID: <20200824012220.0fb938b7@crass-HP-ZBook-15-G2> In-Reply-To: References: 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=US-ASCII Content-Transfer-Encoding: 7bit Received-SPF: pass client-ip=2607:f8b0:4864:20::d44; envelope-from=development@efficientek.com; helo=mail-io1-xd44.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, 24 Aug 2020 06:22:28 -0000 On Sun, 23 Aug 2020 12:59:47 +0200 Patrick Steinhardt wrote: > Hi, > > I've sifted through the mailing list contents of the last few months > to cherry-pick cryptodisk bugfixes which I think should be included > in the v2.06 release. I've found the following 9 patches from Glenn > and me which should probably be included, separated them out from > their respective patch series and made them play nice with each other. > > This patch series shouldn't be applied as-is, but my intention is > instead to bundle all fixes which apply to v2.06 in a single thread to > make discussion easier and help us keep track of what needs to be > done. I've got some comments which I've sent to the original threads > already and added notes below. > > - luks2: grub_cryptodisk_t->total_length is the max number of device > native sectors > > I'm not sure if this fix is correct, mostly because I think that > `grub_disk_get_size` is buggy already: it returns sectors for > partitions and the total size for disks. So I do think we need > another patch to fix that function, too. Its not clear to me what "total size" means here. However, `grub_disk_get_size` returns sectors for non-partition disks as well. Note, that it returns the total number of grub native sector-sized sectors (ie 512 byte sectors). I do think that the _size suffix is misleading and should be named `grub_disk_get_sectors` or something similar. Is there something I can do to clear up the uncertainty around this fix? I suspect part of the confusion lies in the fact that the total_length field is actually in cryptodisk sector-sized sectors. We know this because in `grub_cryptodisk_open` in cryptodisk.c the total_length field is being set unmodified to the total_sectors field of a `grub_disk_t` and `grub_disk_get_size` assume that total_sectors needs to be converted from disk native to grub native sector-sized sectors. > - cryptodisk: Incorrect calculation of start sector for grub_disk_read > in grub_cryptodisk_read > > The patch looks correct to me and matches what both LUKS and LUKS2 > on-disk format say. But I'm surprised our code ever worked > correctly without this fix, which does make me feel uncomfortable. In case its missed, I've explained this in my response to your response to my original patch. > - cryptodisk: Properly handle non-512 byte sized sectors > > Should we pick this for v2.06? It definitely fixes things, but > also feels a bit like feature-enablement. I see you fixed the bug in my patch where IV_PLAIN IVs were not being calculated, thanks. Considering grubs slow release cycle, I think its better to include this or at least make a note that non-512-byte sectors are currently not supported and have cryptomount fail with an appropriate error message if it detects trying to open a LUKS2 device with sector size not equal to 512. As it is, I think LUKS2 support will have people thinking that non-default sector sizes are supported. > I've added my Reviewed-by to those patches which look obviously > correct to me. > > Glenn, please let me know if this somehow interferes with your work or > if you'd like to handle upstreaming of those fixes yourself. I would like to get these patches in a quickly as possible and I suspect this patchset is probably that route, so I'm pleased to have these patches in here. As a reminder, my CRYPTOMOUNT-TESTS patchset can test the full patch set (not individual patches because non-512-byte sector support requires multiple patches in this patchset). Glenn