From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kfypC-0006CT-7i for mharc-grub-devel@gnu.org; Fri, 20 Nov 2020 00:19:10 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:38128) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kfypA-0006CH-Rv for grub-devel@gnu.org; Fri, 20 Nov 2020 00:19:09 -0500 Received: from mail-wm1-x344.google.com ([2a00:1450:4864:20::344]:52667) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kfyp3-0003ZY-Kx for grub-devel@gnu.org; Fri, 20 Nov 2020 00:19:08 -0500 Received: by mail-wm1-x344.google.com with SMTP id 10so9195694wml.2 for ; Thu, 19 Nov 2020 21:19:00 -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=eg8vpQv8EjIihDE74WcFYJ3IoG6LMo7KLzWrWU8M5z4=; b=Ue+UfZbPs05ojYxTKhmB0ul1kbP93DEDy38k0tKby/qM0AJ6LbLsIDHo4aRydHR2HS xMwJ+PCVY4wL/OOVHBrnoZZXREaQTTgoIJAaw/b2Q/7Q1E0kiEYziCAWBkIy0u/Xp1fK STqo89ZShyZXWz1lmif4emV1q8wJE+d0kwW/r4nDhZ3iOUEKMIqiJGp6ZrZBLDjajbAs WRTU9acI5yN/ICooi1BAm6rGACbOe6P0ASQnbFNLu5+GiMzaFLGaJHJVLZHScxstPrHU Axw9EnbXnWPrIhNQV2SM4HNFr6QI4qT6vx3vVArFW1ynI+ifVESB7TrizJFpyVpoVGU/ pp2g== 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=eg8vpQv8EjIihDE74WcFYJ3IoG6LMo7KLzWrWU8M5z4=; b=e18HXB2ENEt15QgL+Umwnzl5FoaLREVo5b0qXj6qc5T+qNWOTnKLM9iYHPEYmXX1Vg mMi3Ey1OZcGByngWz3+kV/1mtsbyG66AbTeMiBKbHGuha+pb0CizEDaqWIheGBemkLSA fankwj2OMQtEFODLI+yOJYc5dsAfxmZLzABHStguQhc5q19jn/lNEy8h/cjez0DqLmbH XhPKwHBDnaS+NzvdNEDCowxW060NHz/G1kMXJ8ghtVHeiZvDGJVfPJOGAuVCpOiuA47D 0c/JXLG8h7t19r9g9ZRAyAbe4mMkRy1P1g2LNxEikO8vBsmKp6wWzQWbH4VDF4HTxzS5 EADA== X-Gm-Message-State: AOAM532OfAQ386KfAptpIiD0jo2pLUB61iNVGikiwg/HG4kolTDVPBl3 M9MN16fjgFYBgxiTB9Vv8y3UgbX9XEv/HQ== X-Google-Smtp-Source: ABdhPJwPfSTGMCB1wvs+Qk8tumR8IfXFA3ympI22nFnjPMAHncShLjdA+HatHL5U7RzySoExE2z0Cg== X-Received: by 2002:a1c:6456:: with SMTP id y83mr8217069wmb.59.1605849539288; Thu, 19 Nov 2020 21:18:59 -0800 (PST) Received: from crass-HP-ZBook-15-G2 ([136.49.211.192]) by smtp.gmail.com with ESMTPSA id b14sm3385587wrx.35.2020.11.19.21.18.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 19 Nov 2020 21:18:59 -0800 (PST) Date: Thu, 19 Nov 2020 23:18:34 -0600 From: Glenn Washburn To: Daniel Kiper Cc: grub-devel@gnu.org, Patrick Steinhardt Subject: Re: [PATCH v3 06/10] cryptodisk: Properly handle non-512 byte sized sectors. Message-ID: <20201119231834.12028cda@crass-HP-ZBook-15-G2> In-Reply-To: <20201119142533.uw64vnl25zaafn4g@tomti.i.net-space.pl> References: <20201009100122.GH2088@tanuki> <6fe26ba56862040d53b03d3d83f393e89156505b.1603148099.git.development@efficientek.com> <20201030204714.dslalmta4ayavnk7@tomti.i.net-space.pl> <20201106130810.54c48e31@crass-HP-ZBook-15-G2> <20201119142533.uw64vnl25zaafn4g@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=2a00:1450:4864:20::344; envelope-from=development@efficientek.com; helo=mail-wm1-x344.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: Fri, 20 Nov 2020 05:19:09 -0000 On Thu, 19 Nov 2020 15:25:33 +0100 Daniel Kiper wrote: > On Fri, Nov 06, 2020 at 01:08:28PM -0600, Glenn Washburn wrote: > > On Fri, 30 Oct 2020 21:47:14 +0100 > > Daniel Kiper wrote: > > > > > On Mon, Oct 19, 2020 at 06:09:54PM -0500, Glenn Washburn wrote: > > > > 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 blocks in a sector (ie 8 > > > > for 4K sectors). Confusingly the IV does not corespond to the > > > > number of, for example, 4K sectors. 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 cipher > > > > blocks in the sector. > > > > > > > > There are some encryption utilities which do it the intuitive > > > > way and have the IV equal to the sector number regardless of > > > > sector size (ie. the fifth sector would have an IV of 4 for > > > > each cipher block). And this is supported 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 > > > > --type 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 hard- coded 512-byte sector size. So even if your data is > > > > encrypted with 4K sector 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 > > > > --- > > > > grub-core/disk/cryptodisk.c | 52 > > > > ++++++++++++++++++++++--------------- grub-core/disk/luks.c > > > > | 5 ++-- grub-core/disk/luks2.c | 7 ++++- > > > > include/grub/cryptodisk.h | 8 +++++- > > > > 4 files changed, 47 insertions(+), 25 deletions(-) > > > > > > > > diff --git a/grub-core/disk/cryptodisk.c > > > > b/grub-core/disk/cryptodisk.c index a3d672f68..623f0f396 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, > > > > - grub_disk_addr_t sector, int > > > > do_encrypt) > > > > + grub_disk_addr_t sector, grub_size_t > > > > log_sector_size, > > > > + int do_encrypt) > > > > { > > > > grub_size_t i; > > > > gcry_err_code_t err; > > > > @@ -237,7 +238,7 @@ grub_cryptodisk_endecrypt (struct > > > > grub_cryptodisk *dev, return (do_encrypt ? > > > > grub_crypto_ecb_encrypt (dev->cipher, data, data, len) : > > > > grub_crypto_ecb_decrypt (dev->cipher, data, data, len)); > > > > > > > > - for (i =3D 0; i < len; i +=3D (1U << dev->log_sector_size)) > > > > + 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) > > > > @@ -270,7 +271,7 @@ grub_cryptodisk_endecrypt (struct > > > > grub_cryptodisk *dev, if (!ctx) > > > > return GPG_ERR_OUT_OF_MEMORY; > > > > > > > > - tmp =3D grub_cpu_to_le64 (sector << > > > > dev->log_sector_size); > > > > + 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 +282,23 @@ grub_cryptodisk_endecrypt (struct > > > > grub_cryptodisk *dev, } > > > > break; > > > > case GRUB_CRYPTODISK_MODE_IV_PLAIN64: > > > > - iv[1] =3D grub_cpu_to_le32 (sector >> 32); > > > > - /* FALLTHROUGH */ > > > > case GRUB_CRYPTODISK_MODE_IV_PLAIN: > > > > - iv[0] =3D grub_cpu_to_le32 (sector & 0xFFFFFFFF); > > > > + /* > > > > + * The IV is a 32 or 64 bit value of the dm-crypt > > > > native sector > > > > + * number. If using 32 bit IV mode, zero out the most > > > > significant > > > > + * 32 bits. > > > > + */ > > > > + { > > > > + grub_uint64_t *iv64 =3D (grub_uint64_t *)iv; > > > > > > ./configure --target=3Darm-linux-gnueabihf --with-platform=3Dcoreboot > > > ... make > > > > > > ...and you get this: > > > > > > disk/cryptodisk.c: In function =E2=80=98grub_cryptodisk_endecrypt= =E2=80=99: > > > disk/cryptodisk.c:292:28: error: cast increases required > > > alignment of target type [-Werror=3Dcast-align] grub_uint64_t *iv64 > > > =3D (grub_uint64_t *)iv; ^ > > > cc1: all warnings being treated as errors > > > > > > I think every 32-bit build will/may fail. It seems to me that > > > (grub_uint64_t *)(void *) iv; > > > or > > > (grub_uint64_t *)(grub_addr_t) iv; > > > should help. > > > > I'm having issues building for arm-linux-gnueabihf, so I can't > > effectively test this. This code does compile for > > --target=3Di386 --with-platform=3Dpc, which I believe is 32-bit. > > > > Since I can't test your suggestion, I can't verify that it works. > > However, I suspect it may get rid of the compiler problem, but not > > solve the underlying issue, which is that iv may not be 8-byte > > aligned. My guess is that if the compiler message goes away you > > could get a crash at the next line if iv is not 8-byte aligned. > > Can you see if the warning disappears if you define iv like this: > > > > grub_uint32_t iv[(GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE + 3) / 4] > > __attribute__((aligned(sizeof(grub_uint64_t)))); > > > > I'm not sure the best way to handle this the "grub way". Do you see > > a better way to do this? >=20 > Below you can find all fixes which are needed to properly build > the GRUB with your patches on all platforms. If you have any > questions drop me a line. >=20 > Daniel Great, thanks for the help. I'm confused as to why the changes to compiler-rt* are needed now. I assume that they are needed because of the use of __builtin_clzll. My v4 changes include a patch that moves the use of __builtin_clzll in luks2.c into a macro in misc.h, but I don't think that would have necessitated this change because the only code that uses the macro is in luks2.c. So the compiler should see no difference from current master. Could it be that mips targets are not properly building with current master due to the use of __builtin_clzll? Or am I totally off? I'll add these in the forth coming patch series. For now, I'll assume that I should put the compiler-rt* changes in a separate patch and merge in the cryptodisk.c changes. Glenn >=20 > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > index 61f8e57f4..b62835acc 100644 > --- a/grub-core/disk/cryptodisk.c > +++ b/grub-core/disk/cryptodisk.c > @@ -243,7 +243,7 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk > *dev, 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] > __attribute__((aligned (sizeof (grub_uint64_t)))); > + grub_uint32_t iv[(GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE + 3) / 4]; >=20 > if (dev->rekey) > { > @@ -289,9 +289,11 @@ grub_cryptodisk_endecrypt (struct > grub_cryptodisk *dev, > * 32 bits. > */ > { > - grub_uint64_t *iv64 =3D (grub_uint64_t *) iv; > - *iv64 =3D grub_cpu_to_le64 (sector << (log_sector_size > + grub_uint64_t iv64; > + > + iv64 =3D grub_cpu_to_le64 (sector << (log_sector_size > - > GRUB_CRYPTODISK_IV_LOG_SIZE)); > + grub_set_unaligned64 (iv, iv64); > if (dev->mode_iv =3D=3D GRUB_CRYPTODISK_MODE_IV_PLAIN) > iv[1] =3D 0; > } > diff --git a/grub-core/kern/compiler-rt.c > b/grub-core/kern/compiler-rt.c index a464200c6..2057c2e0c 100644 > --- a/grub-core/kern/compiler-rt.c > +++ b/grub-core/kern/compiler-rt.c > @@ -448,7 +448,7 @@ __clzsi2 (grub_uint32_t val) > } > #endif >=20 > -#if defined(__riscv) || defined(__sparc__) > +#if defined(__mips__) || defined(__riscv) || defined(__sparc__) > int > __clzdi2 (grub_uint64_t val) > { > diff --git a/include/grub/compiler-rt.h b/include/grub/compiler-rt.h > index 7591980b4..17828b322 100644 > --- a/include/grub/compiler-rt.h > +++ b/include/grub/compiler-rt.h > @@ -115,7 +115,7 @@ int > EXPORT_FUNC (__clzsi2) (grub_uint32_t val); > #endif >=20 > -#if defined(__riscv) || defined(__sparc__) > +#if defined(__mips__) || defined(__riscv) || defined(__sparc__) > int > EXPORT_FUNC (__clzdi2) (grub_uint64_t val); > #endif