From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kKJow-00069f-U8 for mharc-grub-devel@gnu.org; Mon, 21 Sep 2020 07:17:24 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:39624) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kKJot-00068t-Ch for grub-devel@gnu.org; Mon, 21 Sep 2020 07:17:19 -0400 Received: from aserp2120.oracle.com ([141.146.126.78]:54014) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kKJol-0007ji-JS for grub-devel@gnu.org; Mon, 21 Sep 2020 07:17:16 -0400 Received: from pps.filterd (aserp2120.oracle.com [127.0.0.1]) by aserp2120.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 08LB96XE106085; Mon, 21 Sep 2020 11:17:00 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=date : from : to : cc : subject : message-id : references : mime-version : content-type : content-transfer-encoding : in-reply-to; s=corp-2020-01-29; bh=c5oe6VC9DCZiwK7jqrhT68dVkmje2NhgbtijfZleAUo=; b=SSQ7/hJGXdb1u7XLCgcloP59Z/k0/GQo2am+bG5naQPAwFcn6nsbPHkLEyG1xWgUUnjT lMniY2LaAovoeVnxCO8KXi1d03yTowCiPjMkp3Wg+f8mxyUSFghsqaeU8CCt/H2qNUvN AKHVb8DGPb3FTW9V+uK/4uPPioHxTAeArO7qkgrl1Qzmk5FIYrIaFL72VoBS1oJ+XbjR /CQs9Hk0x5n0YcYu3mKMO63eN42pmQjqZxbMtCQkDsdF/N1x+YXvrq2/YbeNkg2k0WtH VJiXvk1gsn5BGm75mGlC1YoihtSsJRgM7NK062bz/0BQN45ldY/vWLczIEx7WcLK8hhw 8w== Received: from userp3030.oracle.com (userp3030.oracle.com [156.151.31.80]) by aserp2120.oracle.com with ESMTP id 33n9xkn0x3-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 21 Sep 2020 11:16:59 +0000 Received: from pps.filterd (userp3030.oracle.com [127.0.0.1]) by userp3030.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 08LBBI5l006070; Mon, 21 Sep 2020 11:16:59 GMT Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by userp3030.oracle.com with ESMTP id 33nuww050p-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 21 Sep 2020 11:16:59 +0000 Received: from abhmp0014.oracle.com (abhmp0014.oracle.com [141.146.116.20]) by aserv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 08LBGrEA008184; Mon, 21 Sep 2020 11:16:55 GMT Received: from tomti.i.net-space.pl (/10.175.223.167) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Mon, 21 Sep 2020 04:16:52 -0700 Date: Mon, 21 Sep 2020 13:16:48 +0200 From: Daniel Kiper To: Glenn Washburn Cc: Patrick Steinhardt , grub-devel@gnu.org, Denis GNUtoo Carikli Subject: Re: [PATCH v3 9/9] cryptodisk: Properly handle non-512 byte sized sectors Message-ID: <20200921111648.ohbrtufp2qifykfc@tomti.i.net-space.pl> References: <81b8a7f915ff1a4499bd9c2e2ca92828a887d046.1599492346.git.ps@pks.im> <20200909112155.bqrrcazvb73lrtsy@tomti.i.net-space.pl> <5593246c-37c4-4355-9c63-8475d15b32f5@efficientek.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <5593246c-37c4-4355-9c63-8475d15b32f5@efficientek.com> User-Agent: NeoMutt/20170113 (1.7.2) X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9750 signatures=668679 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxscore=0 adultscore=0 bulkscore=0 mlxlogscore=999 phishscore=0 suspectscore=2 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2006250000 definitions=main-2009210083 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9750 signatures=668679 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 lowpriorityscore=0 malwarescore=0 suspectscore=2 priorityscore=1501 adultscore=0 spamscore=0 clxscore=1015 mlxlogscore=999 bulkscore=0 mlxscore=0 phishscore=0 impostorscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2006250000 definitions=main-2009210083 Received-SPF: pass client-ip=141.146.126.78; envelope-from=daniel.kiper@oracle.com; helo=aserp2120.oracle.com X-detected-operating-system: by eggs.gnu.org: First seen = 2020/09/21 07:17:08 X-ACL-Warn: Detected OS = Linux 3.1-3.10 [fuzzy] X-Spam_score_int: -58 X-Spam_score: -5.9 X-Spam_bar: ----- X-Spam_report: (-5.9 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-1.455, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001, UNPARSEABLE_RELAY=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 11:17:20 -0000 On Mon, Sep 21, 2020 at 05:58:06AM +0000, Glenn Washburn wrote: > 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 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 cipher block in the fifth > >> 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 this 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 cipher blocks in the sector." Yeah, LGTM. > >> each sequential 512 byte block or an IV of 4 for each cipher block 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 > >> Reviewed-by: Patrick Steinhardt > >> --- > >> grub-core/disk/cryptodisk.c | 44 ++++++++++++++++++++----------------- > >> grub-core/disk/luks.c       |  5 +++-- > >> grub-core/disk/luks2.c      |  7 +++++- > >> include/grub/cryptodisk.h   |  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, > >> -        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,12 +238,13 @@ 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 = 0; i < len; i += (1U << dev->log_sector_size)) > >> +  for (i = 0; i < len; i += (1U << log_sector_size)) > >> { > >> grub_size_t sz = ((dev->cipher->cipher->blocksize > >> + sizeof (grub_uint32_t) - 1) > >> / sizeof (grub_uint32_t)); > >> grub_uint32_t iv[(GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE + 3) / 4]; > >> +      grub_uint64_t iv_calc = 0; > >> > >> if (dev->rekey) > >> { > >> @@ -270,7 +272,7 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev, > >> if (!ctx) > >> return GPG_ERR_OUT_OF_MEMORY; > >> > >> -     tmp = grub_cpu_to_le64 (sector << dev->log_sector_size); > >> +     tmp = 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: > >> -   iv[1] = grub_cpu_to_le32 (sector >> 32); > >> +   iv_calc = sector << (log_sector_size - GRUB_CRYPTODISK_IV_LOG_SIZE); > >> +   iv[1] = 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 deal 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 patch, but I'd like to point out that the "32" literal you're > commenting on is not code I've introduced. As such, the comment would > not be relevant to this patch. Given that, do you still want a comment > in this patch? If you touch this code I want it fixed this way or another, If you think comment should be added in separate patch and/or in different place I am OK with it. > >> /* FALLTHROUGH */ > >> case GRUB_CRYPTODISK_MODE_IV_PLAIN: > >> -   iv[0] = grub_cpu_to_le32 (sector & 0xFFFFFFFF); > >> +   iv_calc = sector << (log_sector_size - GRUB_CRYPTODISK_IV_LOG_SIZE); > >> +   iv[0] = grub_cpu_to_le32 (iv_calc & 0xFFFFFFFF); > >> break; > >> case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64: > >> -   iv[1] = grub_cpu_to_le32 (sector >> (32 - dev->log_sector_size)); > > > > Ditto? > > For modes other than plain and plain64, all that is being done is > substituting "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 relevant to this patch. > > Here the "32" is used to create the IV in 2 32bit chunks because the > variable "iv" is an array of 32bit uints. As above, if you think separate patch is better go for it... > >> -   iv[0] = grub_cpu_to_le32 ((sector << dev->log_sector_size) > >> +   iv[1] = grub_cpu_to_le32 (sector >> (32 - log_sector_size)); > > > > 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 *check_uuid, > >> return NULL; > >> newdev->offset = grub_be_to_cpu32 (header.payloadOffset); > >> newdev->source_disk = NULL; > >> -  newdev->log_sector_size = 9; > >> +  newdev->log_sector_size = LUKS_LOG_SECTOR_SIZE; > >> newdev->total_length = grub_disk_get_size (disk) - newdev->offset; > >> grub_memcpy (newdev->uuid, uuid, sizeof (uuid)); > >> newdev->modname = "luks"; > >> @@ -247,7 +247,8 @@ luks_recover_key (grub_disk_t source, > >> return err; > >> } > >> > >> -      gcry_err = grub_cryptodisk_decrypt (dev, split_key, length, 0); > >> +      gcry_err = grub_cryptodisk_decrypt (dev, split_key, length, 0, > >> +           LUKS_LOG_SECTOR_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; > >> } > >> > >> -  gcry_ret = grub_cryptodisk_decrypt (crypt, split_key, k->area.size, 0); > >> +  /* > >> +   * The key slots area is always encrypted in 512-byte sectors, > >> +   * regardless of encrypted data sector size. > >> +   */ > >> +  gcry_ret = grub_cryptodisk_decrypt (crypt, split_key, k->area.size, 0, > >> +             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. So > 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 natively for incrementing the > IV. Please add similar comments to places where these constants are defined. Daniel