From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kKEq6-00070O-Ei for mharc-grub-devel@gnu.org; Mon, 21 Sep 2020 01:58:14 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:45332) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kKEq5-00070F-Ca for grub-devel@gnu.org; Mon, 21 Sep 2020 01:58:13 -0400 Received: from mail-pf1-x442.google.com ([2607:f8b0:4864:20::442]:37262) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kKEq2-00087Q-QC for grub-devel@gnu.org; Mon, 21 Sep 2020 01:58:12 -0400 Received: by mail-pf1-x442.google.com with SMTP id w7so8339229pfi.4 for ; Sun, 20 Sep 2020 22:58:10 -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:message-id:in-reply-to:references:subject :mime-version:content-transfer-encoding; bh=fHdoBVZRizARLDGwHr++D5FcISXz6ywtxmvnT4vJDfk=; b=B9qh2UpHyu9kr8rwBhYeXipFz/srMqKhoL3haiCPPlAGSRV7ZLqLrfRoa3BRO5O0IR dS7OG7WLUMD3JBWvYcn07NlTWsZpP6fjNZ27StzIx60T5W+FL71GC2yKIvPT0xRqpWUH 2mXeHal554XSTcSTccGgK5mmcr5JFbSuMJ7dKaMLSXO21R5dZa/ozaA/qkqFS/BeRarC 6SElM+VqMERMGXRF5P/rpmW2uKRvlRcQanItskDJ/Iqwxn3sa3d/64+YLE/60WRELvOg RveJZH/n/NBOpZXviearHagY5lkFTnfcIHn8IxqYXbIv+pQem5zZzfjiBiiyPo/7erp0 JqZw== 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:message-id:in-reply-to :references:subject:mime-version:content-transfer-encoding; bh=fHdoBVZRizARLDGwHr++D5FcISXz6ywtxmvnT4vJDfk=; b=i12oOERaW+KGACDUux7UObftizSf23XOWQHB2hLJDVcyboVLDW0RiCd6Bd87g/oGnw jQOgLx1jaD3kW0q7+SKDw4eOKRF7DdP2WO+eoX3HnwsYN0uM/5sCas+kn6yzVSJBjxun 8TEXQ7q8pVAoAUjABvyzsudqj1bgc1Tx+WX0/rtCX5QuZqYUOg/gBLjj4ZbeZAr6t9Si Y/7EjbyaI7LyLF9mSGYCr1/dttIrXiqevyiCuuqVWumHX2XgRsABGzYttYCaxNg/Ry1i 00KdmDcwsTtqk8XxWsCwbFnQb4fls96Pb1DjZZZaUsP60AHaIIygYTwaSM1K5JmT3BC1 g9Fg== X-Gm-Message-State: AOAM532Y13KXp7fzVe2JLP3UqCrinGK4V/zlxeEcw1NJtuiplIH87095 mHcEZZd7HC13Ggb1Q1rNCkxZFA== X-Google-Smtp-Source: ABdhPJwm/VO5enR+b1vBEwwhx3SJ/ITuuS6MNVKUAGF+wCshJT7E1F3CqJtkgJAstWjv64cP0bKdig== X-Received: by 2002:a62:7616:0:b029:150:a14b:6bd3 with SMTP id r22-20020a6276160000b0290150a14b6bd3mr3453720pfc.75.1600667889038; Sun, 20 Sep 2020 22:58:09 -0700 (PDT) Received: from [127.0.0.1] ([172.58.46.198]) by smtp.gmail.com with ESMTPSA id f28sm10913720pfq.191.2020.09.20.22.58.08 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sun, 20 Sep 2020 22:58:08 -0700 (PDT) Date: Mon, 21 Sep 2020 05:58:06 +0000 (UTC) From: Glenn Washburn To: Daniel Kiper Cc: Patrick Steinhardt , grub-devel@gnu.org, Denis GNUtoo Carikli Message-ID: <5593246c-37c4-4355-9c63-8475d15b32f5@efficientek.com> In-Reply-To: <20200909112155.bqrrcazvb73lrtsy@tomti.i.net-space.pl> References: <81b8a7f915ff1a4499bd9c2e2ca92828a887d046.1599492346.git.ps@pks.im> <20200909112155.bqrrcazvb73lrtsy@tomti.i.net-space.pl> Subject: Re: [PATCH v3 9/9] cryptodisk: Properly handle non-512 byte sized sectors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Correlation-ID: <5593246c-37c4-4355-9c63-8475d15b32f5@efficientek.com> Received-SPF: pass client-ip=2607:f8b0:4864:20::442; envelope-from=development@efficientek.com; helo=mail-pf1-x442.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, 21 Sep 2020 05:58:13 -0000 Sep 9, 2020 5:22:11 AM Daniel Kiper : > On Mon, Sep 07, 2020 at 05:28:08PM +0200, Patrick Steinhardt wrote: >> From: Glenn Washburn >> >> By default, dm-crypt internally uses an IV that corresponds to 512-byte >> sectors, even when a larger sector size is specified. What this means is >> that when using a larger sector size, the IV is incremented every sector= . >> However, the amount the IV is incremented is the number of 512 byte bloc= ks >> in a sector (ie 8 for 4K sectors). Confusingly the IV does not corespond= to >> the number of, for example, 4K sectors. So each cipher block in the fift= h >> 4K sector will be encrypted with an IV equal to 32, as opposed to 32-39 = for > > s/32-39/32-9/? I'm reading this and realizing it's worded badly and confusing. Perhaps thi= s sentence is better? "So each 512 byte cipher block in a sector will be encrypted with the same = IV and the IV will be incremented afterwards by the number of 512 byte ciph= er blocks in the sector." >> each sequential 512 byte block or an IV of 4 for each cipher block in th= e >> sector. >> >> There are some encryption utilities which do it the intuitive way and ha= ve >> the IV equal to the sector number regardless of sector size (ie. the fif= th >> sector would have an IV of 4 for each cipher block). And this is support= ed >> by dm-crypt with the iv_large_sectors option and also cryptsetup as of 2= .3.3 >> with the --iv-large-sectors, though not with LUKS headers (only with --t= ype >> plain). However, support for this has not been included as grub does not >> support plain devices right now. >> >> One gotcha here is that the encrypted split keys are encrypted with a ha= rd- >> coded 512-byte sector size. So even if your data is encrypted with 4K se= ctor >> sizes, the split key encrypted area must be decrypted with a block size = of >> 512 (ie the IV increments every 512 bytes). This made these changes less >> aestetically pleasing than desired. >> >> Signed-off-by: Glenn Washburn >> Reviewed-by: Patrick Steinhardt >> --- >> grub-core/disk/cryptodisk.c | 44 ++++++++++++++++++++----------------- >> grub-core/disk/luks.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 5 +++-= - >> grub-core/disk/luks2.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 7 +++++- >> include/grub/cryptodisk.h=C2=A0=C2=A0 |=C2=A0 9 +++++++- >> 4 files changed, 41 insertions(+), 24 deletions(-) >> >> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c >> index 0b63b7d96..6319f3164 100644 >> --- a/grub-core/disk/cryptodisk.c >> +++ b/grub-core/disk/cryptodisk.c >> @@ -224,7 +224,8 @@ lrw_xor (const struct lrw_sector *sec, >> static gcry_err_code_t >> grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev, >> grub_uint8_t * data, grub_size_t len, >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 grub_disk_addr_t sector, int= do_encrypt) >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 grub_disk_addr_t sector, gru= b_size_t log_sector_size, >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 int do_encrypt) >> { >> grub_size_t i; >> gcry_err_code_t err; >> @@ -237,12 +238,13 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk = *dev, >> return (do_encrypt ? grub_crypto_ecb_encrypt (dev->cipher, data, data, l= en) >> : grub_crypto_ecb_decrypt (dev->cipher, data, data, len)); >> >> -=C2=A0 for (i =3D 0; i < len; i +=3D (1U << dev->log_sector_size)) >> +=C2=A0 for (i =3D 0; i < len; i +=3D (1U << log_sector_size)) >> { >> grub_size_t sz =3D ((dev->cipher->cipher->blocksize >> + sizeof (grub_uint32_t) - 1) >> / sizeof (grub_uint32_t)); >> grub_uint32_t iv[(GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE + 3) / 4]; >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 grub_uint64_t iv_calc =3D 0; >> >> if (dev->rekey) >> { >> @@ -270,7 +272,7 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *d= ev, >> if (!ctx) >> return GPG_ERR_OUT_OF_MEMORY; >> >> -=C2=A0=C2=A0=C2=A0=C2=A0 tmp =3D grub_cpu_to_le64 (sector << dev->log_s= ector_size); >> +=C2=A0=C2=A0=C2=A0=C2=A0 tmp =3D grub_cpu_to_le64 (sector << log_sector= _size); >> dev->iv_hash->init (ctx); >> dev->iv_hash->write (ctx, dev->iv_prefix, dev->iv_prefix_len); >> dev->iv_hash->write (ctx, &tmp, sizeof (tmp)); >> @@ -281,14 +283,16 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk = *dev, >> } >> break; >> case GRUB_CRYPTODISK_MODE_IV_PLAIN64: >> -=C2=A0=C2=A0 iv[1] =3D grub_cpu_to_le32 (sector >> 32); >> +=C2=A0=C2=A0 iv_calc =3D sector << (log_sector_size - GRUB_CRYPTODISK_I= V_LOG_SIZE); >> +=C2=A0=C2=A0 iv[1] =3D grub_cpu_to_le32 (iv_calc >> 32); > > Why 32? Could you use a constant or add a comment here? Plain mode uses a 32 bit IV and plain64 uses a 64 bit IV. So in the plain64= case only deal with the non-plain (ie not lower 32 bits) IV bits and we de= al with the plain case in the fall through. I don't think a constant is warranted and I can add in a comment in this pa= tch, but I'd like to point out that the "32" literal you're commenting on i= s not code I've introduced. As such, the comment would not be relevant to t= his patch. Given that, do you still want a comment in this patch? >> /* FALLTHROUGH */ >> case GRUB_CRYPTODISK_MODE_IV_PLAIN: >> -=C2=A0=C2=A0 iv[0] =3D grub_cpu_to_le32 (sector & 0xFFFFFFFF); >> +=C2=A0=C2=A0 iv_calc =3D sector << (log_sector_size - GRUB_CRYPTODISK_I= V_LOG_SIZE); >> +=C2=A0=C2=A0 iv[0] =3D grub_cpu_to_le32 (iv_calc & 0xFFFFFFFF); >> break; >> case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64: >> -=C2=A0=C2=A0 iv[1] =3D grub_cpu_to_le32 (sector >> (32 - dev->log_secto= r_size)); > > Ditto? For modes other than plain and plain64, all that is being done is substitut= ing "dev->log_sector_size" for "log_sector_size". Again, those constant "32= " integers are not code that I've added and I don't think comments are rele= vant to this patch. Here the "32" is used to create the IV in 2 32bit chunks because the variab= le "iv" is an array of 32bit uints. >> -=C2=A0=C2=A0 iv[0] =3D grub_cpu_to_le32 ((sector << dev->log_sector_siz= e) >> +=C2=A0=C2=A0 iv[1] =3D grub_cpu_to_le32 (sector >> (32 - log_sector_siz= e)); > > Ditto? > > [...] > >> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c >> index 59702067a..2e1347b13 100644 >> --- a/grub-core/disk/luks.c >> +++ b/grub-core/disk/luks.c >> @@ -124,7 +124,7 @@ configure_ciphers (grub_disk_t disk, const char *che= ck_uuid, >> return NULL; >> newdev->offset =3D grub_be_to_cpu32 (header.payloadOffset); >> newdev->source_disk =3D NULL; >> -=C2=A0 newdev->log_sector_size =3D 9; >> +=C2=A0 newdev->log_sector_size =3D LUKS_LOG_SECTOR_SIZE; >> newdev->total_length =3D grub_disk_get_size (disk) - newdev->offset; >> grub_memcpy (newdev->uuid, uuid, sizeof (uuid)); >> newdev->modname =3D "luks"; >> @@ -247,7 +247,8 @@ luks_recover_key (grub_disk_t source, >> return err; >> } >> >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 gcry_err =3D grub_cryptodisk_decrypt (de= v, split_key, length, 0); >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 gcry_err =3D grub_cryptodisk_decrypt (de= v, split_key, length, 0, >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 LUKS_LOG_S= ECTOR_SIZE); >> if (gcry_err) >> { >> grub_free (split_key); >> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c >> index 26e1126b1..eb64a0596 100644 >> --- a/grub-core/disk/luks2.c >> +++ b/grub-core/disk/luks2.c >> @@ -492,7 +492,12 @@ luks2_decrypt_key (grub_uint8_t *out_key, >> goto err; >> } >> >> -=C2=A0 gcry_ret =3D grub_cryptodisk_decrypt (crypt, split_key, k->area.= size, 0); >> +=C2=A0 /* >> +=C2=A0=C2=A0 * The key slots area is always encrypted in 512-byte secto= rs, >> +=C2=A0=C2=A0 * regardless of encrypted data sector size. >> +=C2=A0=C2=A0 */ >> +=C2=A0 gcry_ret =3D grub_cryptodisk_decrypt (crypt, split_key, k->area.= size, 0, >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 LUKS_LOG_SECTOR_SIZE); > > s/LUKS_LOG_SECTOR_SIZE/GRUB_CRYPTODISK_IV_LOG_SIZE/? LUKS_LOG_SECTOR_SIZE and GRUB_CRYPTODISK_IV_LOG_SIZE are the same number, but they represent different things. According to the = luks2 spec the key slot area is decrypted as specified in the luks1 spec. S= o perhaps the constant should be renamed to LUKS1_LOG_SECTOR_SIZE, because = luks2 does not have a constant sector size. The constant GRUB_CRYPTODISK_IV_LOG_SIZE is meant to represent the log of the sector size that dm-crypt uses nativel= y for incrementing the IV. >> if (gcry_ret) >> { >> ret =3D grub_crypto_gcry_error (gcry_ret); >> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h >> index e1b21e785..ecb37ba43 100644 >> --- a/include/grub/cryptodisk.h >> +++ b/include/grub/cryptodisk.h >> @@ -48,6 +48,13 @@ typedef enum >> >> #define GRUB_CRYPTODISK_MAX_UUID_LENGTH 71 >> >> +#define LUKS_LOG_SECTOR_SIZE 9 >> + >> +/* For the purposes of IV incrementing the sector size is 512 bytes, un= less >> + * otherwise specified. >> + */ >> +#define GRUB_CRYPTODISK_IV_LOG_SIZE 9 >> + >> #define GRUB_CRYPTODISK_GF_LOG_SIZE 7 >> #define GRUB_CRYPTODISK_GF_SIZE (1U << GRUB_CRYPTODISK_GF_LOG_SIZE) >> #define GRUB_CRYPTODISK_GF_LOG_BYTES (GRUB_CRYPTODISK_GF_LOG_SIZE - 3) >> @@ -139,7 +146,7 @@ grub_cryptodisk_setkey (grub_cryptodisk_t dev, >> gcry_err_code_t >> grub_cryptodisk_decrypt (struct grub_cryptodisk *dev, >> grub_uint8_t * data, grub_size_t len, >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 grub_disk_addr_t sector); >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 grub_disk_addr_t sector, grub_size_t log= _sector_size); >> grub_err_t >> grub_cryptodisk_insert (grub_cryptodisk_t newdev, const char *name, >> grub_disk_t source); > > Daniel > Glenn