From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kkrPo-00061j-8G for mharc-grub-devel@gnu.org; Thu, 03 Dec 2020 11:25:08 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:39676) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kkrPm-000601-Hk for grub-devel@gnu.org; Thu, 03 Dec 2020 11:25:06 -0500 Received: from mail-lf1-x143.google.com ([2a00:1450:4864:20::143]:40991) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kkrPk-0006xl-6N for grub-devel@gnu.org; Thu, 03 Dec 2020 11:25:06 -0500 Received: by mail-lf1-x143.google.com with SMTP id r24so3562435lfm.8 for ; Thu, 03 Dec 2020 08:25:03 -0800 (PST) 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=NC73Z+aFEJYFrWs1X2La89WMLj8ZdOGLeb1c61VxgC0=; b=WNdwl425D09btki7Z/fQJefWenmabYB4GoRuuFrECcFLfzvPvLWH2+iD+yyIXplrAU YYJ8MQmKdgZckNeaJirbiFJ5O0sFqPbltkPOwsG7O1L4pp0A9Rm7GClBzFZntorqRo8J Z9n6R8k1dYJ9rmepR2VplPFgOiPXI8X4E37g3DcS/hpy6gnmikqOLKQH7+CbRu8pM5+U OyFzpd9S2IK4gb4m6+kzPjehYUMP8oziT2wCRsLA9L+9wvrFNYqzIMCqgghSfeyGpbP5 T7Uw47Ig12OB1qblDK1hT8eaJHldvAwnPi4UmZIV/pIRuWJn2MJqooMHm8TVo/6WvVTr Itxg== 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=NC73Z+aFEJYFrWs1X2La89WMLj8ZdOGLeb1c61VxgC0=; b=fzufj6e7qHecPRKDBd7DBPsGZPv9PQNhW0oKVQap803y7QlmL/y9Z1V/S50VC/Ktwn uGTe7xgUe3lk6uv0+9IHXIT/ktIlCNVE+JNAMYvZj66l8I/wz6LIKT4rjtXiC1L4gNv1 KlB6Hd9pFWX3LryH6q8WBTsYDzOtBXN19UIJPt+q37GON5uIfrTu/8px5HVybpotK3kU zTz6demkVt2y6NE4Vt2jbRXOacVa5teYUDqnJR+HH2JiKUz54ulIvUWlE97aV7JKEqMJ fBGkNiMSHD2rGB4R8ZQKPYMXj3WAwg6MtDeMkEsSNHdKSEPkkYhUI0ZHOhDa4IzSW5i7 A/VA== X-Gm-Message-State: AOAM530akEfhXBbeRicv3gUn6e6tPg9qx9HXmsx2hmZ+Sc/kTF19ivRn s5kxB6nfcVC2epECarS+tNc1K/YYU1765w== X-Google-Smtp-Source: ABdhPJwoiy+7sTwAAsIZZSMX1iEKlBwOF6oQCqGyQfR2UzpBrM27Jtu0Ms6jOBR97yiltEuXVKAiWA== X-Received: by 2002:a19:b46:: with SMTP id 67mr1523355lfl.488.1607012702106; Thu, 03 Dec 2020 08:25:02 -0800 (PST) Received: from crass-HP-ZBook-15-G2 ([136.49.211.192]) by smtp.gmail.com with ESMTPSA id v11sm670254lfb.185.2020.12.03.08.24.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 03 Dec 2020 08:25:01 -0800 (PST) Date: Thu, 3 Dec 2020 10:24:46 -0600 From: Glenn Washburn To: Daniel Kiper Cc: grub-devel@gnu.org, Patrick Steinhardt Subject: Re: [PATCH v6 07/12] luks2: Better error handling when setting up the cryptodisk Message-ID: <20201203102446.3b7270e9@crass-HP-ZBook-15-G2> In-Reply-To: <20201203133149.2y75rrzc5yw4fqhj@tomti.i.net-space.pl> References: <20201203133149.2y75rrzc5yw4fqhj@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=US-ASCII Content-Transfer-Encoding: 7bit Received-SPF: pass client-ip=2a00:1450:4864:20::143; envelope-from=development@efficientek.com; helo=mail-lf1-x143.google.com 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: Thu, 03 Dec 2020 16:25:06 -0000 On Thu, 3 Dec 2020 14:31:49 +0100 Daniel Kiper wrote: > On Fri, Nov 27, 2020 at 03:03:39AM -0600, Glenn Washburn wrote: > > First, check to make sure that source disk has a known size. If > > not, print debug message and return error. There are 4 cases where > > GRUB_DISK_SIZE_UNKNOWN is set (biosdisk, obdisk, ofdisk, and > > uboot), and in all those cases processing continues. So this is > > probably a bit conservative. However, 3 of the cases seem > > pathological, and the other, biosdisk, happens when booting from a > > cd. Since I doubt booting from a LUKS2 volume on a cd is a big use > > case, we'll error until someone complains. > > > > Do some sanity checking on data coming from the luks header. If > > segment.size is "dynamic", > > > > Check for errors from grub_strtoull when converting segment size > > from string. If a GRUB_ERR_BAD_NUMBER error was returned, then the > > string was not a valid parsable number, so skip the key. If > > GRUB_ERR_OUT_OF_RANGE was returned, then there was an overflow in > > converting to a 64-bit unsigned integer. So this could be a very > > large disk (perhaps large raid array). In this case, we want to > > continue to try to use this key so long as the source disk's size > > is greater than this segment size. Otherwise, this is an invalid > > segment, and continue on to the next key. > > > > Signed-off-by: Glenn Washburn > > --- > > grub-core/disk/luks2.c | 80 > > ++++++++++++++++++++++++++++++++++++++++-- include/grub/disk.h | > > 16 +++++++++ 2 files changed, 93 insertions(+), 3 deletions(-) > > > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c > > index b7ed0642e..01f9608e5 100644 > > --- a/grub-core/disk/luks2.c > > +++ b/grub-core/disk/luks2.c > > @@ -597,12 +597,26 @@ luks2_recover_key (grub_disk_t source, > > goto err; > > } > > > > + if (source->total_sectors == GRUB_DISK_SIZE_UNKNOWN) > > + { > > + /* FIXME: Allow use of source disk, and maybe cause errors > > in read. */ > > + grub_dprintf ("luks2", "Source disk %s has an unknown size, " > > + "conservatively returning error\n", > > source->name); > > + ret = grub_error (GRUB_ERR_BUG, "Unknown size of luks2 > > source device"); > > + goto err; > > + } > > + > > /* Try all keyslot */ > > for (i = 0; i < size; i++) > > { > > + typeof(source->total_sectors) max_crypt_sectors = 0; > > + > > + grub_errno = GRUB_ERR_NONE; > > ret = luks2_get_keyslot (&keyslot, &digest, &segment, json, > > i); if (ret) > > goto err; > > + if (grub_errno != GRUB_ERR_NONE) > > + grub_dprintf ("luks2", "Ignoring unhandled error %d from > > luks2_get_keyslot\n", grub_errno); > > > > if (keyslot.priority == 0) > > { > > @@ -616,11 +630,71 @@ luks2_recover_key (grub_disk_t source, > > crypt->offset_sectors = grub_divmod64 (segment.offset, > > segment.sector_size, NULL); crypt->log_sector_size = sizeof > > (unsigned int) * 8 > > - __builtin_clz ((unsigned int) > > segment.sector_size) - 1; > > + /* Set to the source disk size, which is the maximum we > > allow. */ > > + max_crypt_sectors = grub_disk_convert_sector(source, > > + > > source->total_sectors, > > + > > crypt->log_sector_size); + > > + if (max_crypt_sectors < crypt->offset_sectors) > > + { > > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" > > has offset" > > + " %"PRIuGRUB_UINT64_T" which is > > greater than" > > + " source disk size > > %"PRIuGRUB_UINT64_T"," > > + " skipping\n", > > + segment.slot_key, > > crypt->offset_sectors, > > + max_crypt_sectors); > > + continue; > > + } > > + > > if (grub_strcmp (segment.size, "dynamic") == 0) > > - crypt->total_sectors = (grub_disk_get_size (source) >> > > (crypt->log_sector_size - source->log_sector_size)) > > - - crypt->offset_sectors; > > + crypt->total_sectors = max_crypt_sectors - > > crypt->offset_sectors; else > > - crypt->total_sectors = grub_strtoull (segment.size, NULL, > > 10) >> crypt->log_sector_size; > > + { > > + grub_errno = GRUB_ERR_NONE; > > + crypt->total_sectors = grub_strtoull (segment.size, > > NULL, 10) >> crypt->log_sector_size; > > I think ">>" should not happen here... Do you think this because ">>" should not operate on a value returned by a call to grub_strtoull that has errored? I don't think there's any problem because the return value is a number, so there's no harm in using ">>" on a garbage number to get another garbage number, as long as we don't use the value. (Its also not technically a garbage number, just one of two values in the error case) Also I didn't want to set total_sectors to grub_strtoull because it could be a little confusing since the return value of grub_strtoull is bytes not sectors. And yet storing in total_sectors. I could use a local variable, but was wanting to avoid that. But I'm fine with this suggestion. > > + if (grub_errno == GRUB_ERR_NONE) > > + ; > > It should happen here... Or to be exact... > crypt->total_sectors = ALIGN_UP (crypt->total_sectors, 1 << > crypt->log_sector_size); // Am I right? crypt->total_sectors >>= > crypt->log_sector_size; I don't think we need to decrypt partial blocks at the end. Cryptsetup/dm-crypt enforces disk sizes that are multiples of the sector size. So if the size is not a multiple of blocksize, there's a bug somewhere in cryptsetup/dm-crypt, and we don't need to deal with the partial block. At worse the use loses access to a partial block at the end that they should never have had access to in the first place. > > + else if(grub_errno == GRUB_ERR_BAD_NUMBER) > > Missing space before "("... Ack. > > + { > > + /* TODO: Unparsable number-string, try to use the > > whole disk */ > > + grub_dprintf ("luks2", "Segment > > \"%"PRIuGRUB_UINT64_T"\" size" > > + " \"%s\" is not a parsable > > number\n", > > + segment.slot_key, > > segment.size); > > crypt->total_sectors = max_crypt_sectors; ? Sure, I didn't think you'd want this since it potentially entails trying to read past the end of the luks data, and thus getting random data, which might crash (buggy) filesystem read code. > > + continue; > > ...and then drop continue? > > > + } > > + else if(grub_errno == GRUB_ERR_OUT_OF_RANGE) > > + { > > + /* > > + * There was an overflow in parsing segment.size, so > > disk must > > + * be very large or the string is incorrect. > > + */ > > + grub_dprintf ("luks2", "Segment > > \"%"PRIuGRUB_UINT64_T"\" size" > > + " %s overflowed 64-bit > > unsigned integer," > > + " the end of the crypto > > device will be" > > + " inaccessible\n", > > + segment.slot_key, > > segment.size); > > + if (crypt->total_sectors > max_crypt_sectors) > > + crypt->total_sectors = max_crypt_sectors; > > + } > > + } > > + > > + if (crypt->total_sectors == 0) > > + { > > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" > > has zero" > > + " sectors, skipping\n", > > + segment.slot_key); > > + continue; > > + } > > + else if (max_crypt_sectors < (crypt->offset_sectors + > > crypt->total_sectors)) > > + { > > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" > > has last" > > + " data position greater than > > source disk size," > > + " the end of the crypto device > > will be" > > + " inaccessible\n", > > + segment.slot_key); > > + /* Allow decryption up to the end of the source disk. */ > > + crypt->total_sectors = max_crypt_sectors - > > crypt->offset_sectors; > > + } > > > > ret = luks2_decrypt_key (candidate_key, source, crypt, > > &keyslot, (const grub_uint8_t *) passphrase, grub_strlen > > (passphrase)); diff --git a/include/grub/disk.h > > b/include/grub/disk.h index 132a1bb75..1a9e8fcf4 100644 > > --- a/include/grub/disk.h > > +++ b/include/grub/disk.h > > @@ -174,6 +174,22 @@ typedef struct grub_disk_memberlist > > *grub_disk_memberlist_t; /* Return value of grub_disk_get_size() in > > case disk size is unknown. */ #define GRUB_DISK_SIZE_UNKNOWN > > 0xffffffffffffffffULL > > > > +/* Convert sector number from disk sized sectors to a log-size > > sized sector. */ +static inline grub_disk_addr_t > > +grub_disk_convert_sector (grub_disk_t disk, > > + grub_disk_addr_t sector, > > + grub_size_t log_sector_size) > > +{ > > + if (disk->log_sector_size < log_sector_size) > > + { > > + /* Round up to the nearest log_sector_size sized sector. */ > > + sector += 1ULL << ((log_sector_size / disk->log_sector_size) > > - 1); > > ALIGN_UP()? Sure, I can do that and then remove the comment. > > + return sector >> (log_sector_size - disk->log_sector_size); > > + } > > + else > > + return sector << (disk->log_sector_size - log_sector_size); > > +} > > + > > /* Convert to GRUB native disk sized sector from disk sized > > sector. */ static inline grub_disk_addr_t > > grub_disk_from_native_sector (grub_disk_t disk, grub_disk_addr_t > > sector) > > Daniel