This series creates an EBOIV template that produces a skcipher
transform which passes through all operations to the skcipher, while
using the same skcipher and key to encrypt the input IV, which is
assumed to be a sector offset, although this is not enforced.
This matches dm-crypt use of EBOIV to provide BitLocker support,
and so it is proposed as a replacement in patch #3.
Replacing the dm-crypt specific EBOIV implementation to a Crypto
API based one allows us to save a memory allocation per
each request, as well as opening the way for use of compatible
alternative transform providers, one of which, based on the
Arm TrustZone CryptoCell hardware, is proposed as patch #4.
Future potential work to allow encapsulating the handling of
multiple subsequent blocks by the Crypto API may also
benefit from this work.
The code has been tested on both x86_64 virtual machine
with the dm-crypt test suite and on an arm 32 bit board
with the CryptoCell hardware.
Since no offical source for eboiv test vectors is known,
the test vectors supplied as patch #2 are derived from
sectors which are part of the dm-crypt test suite.
Gilad Ben-Yossef (4):
crypto: add eboiv as a crypto API template
crypto: add eboiv(cbc(aes)) test vectors
dm crypt: switch to EBOIV crypto API template
crypto: ccree: re-introduce ccree eboiv support
crypto/Kconfig | 21 ++
crypto/Makefile | 1 +
crypto/eboiv.c | 295 +++++++++++++++++++++++++++
crypto/tcrypt.c | 9 +
crypto/testmgr.c | 6 +
crypto/testmgr.h | 279 +++++++++++++++++++++++++
drivers/crypto/ccree/cc_cipher.c | 130 ++++++++----
drivers/crypto/ccree/cc_crypto_ctx.h | 1 +
drivers/md/Kconfig | 1 +
drivers/md/dm-crypt.c | 61 ++----
10 files changed, 725 insertions(+), 79 deletions(-)
create mode 100644 crypto/eboiv.c
--
2.28.0
BitLocker eboiv support, which was removed in
commit 1d8b41ff6991 ("crypto: ccree - remove bitlocker cipher")
is reintroduced based on the crypto API new support for
eboiv.
Signed-off-by: Gilad Ben-Yossef <[email protected]>
Fixes: 1d8b41ff6991 ("crypto: ccree - remove bitlocker cipher")
---
drivers/crypto/ccree/cc_cipher.c | 130 +++++++++++++++++++--------
drivers/crypto/ccree/cc_crypto_ctx.h | 1 +
2 files changed, 94 insertions(+), 37 deletions(-)
diff --git a/drivers/crypto/ccree/cc_cipher.c b/drivers/crypto/ccree/cc_cipher.c
index b5568de86ca4..23407063bd40 100644
--- a/drivers/crypto/ccree/cc_cipher.c
+++ b/drivers/crypto/ccree/cc_cipher.c
@@ -95,10 +95,14 @@ static int validate_keys_sizes(struct cc_cipher_ctx *ctx_p, u32 size)
case S_DIN_to_AES:
switch (size) {
case CC_AES_128_BIT_KEY_SIZE:
- case CC_AES_192_BIT_KEY_SIZE:
if (ctx_p->cipher_mode != DRV_CIPHER_XTS)
return 0;
break;
+ case CC_AES_192_BIT_KEY_SIZE:
+ if (ctx_p->cipher_mode != DRV_CIPHER_XTS &&
+ ctx_p->cipher_mode != DRV_CIPHER_BITLOCKER)
+ return 0;
+ break;
case CC_AES_256_BIT_KEY_SIZE:
return 0;
case (CC_AES_192_BIT_KEY_SIZE * 2):
@@ -141,6 +145,7 @@ static int validate_data_size(struct cc_cipher_ctx *ctx_p,
case DRV_CIPHER_ECB:
case DRV_CIPHER_CBC:
case DRV_CIPHER_ESSIV:
+ case DRV_CIPHER_BITLOCKER:
if (IS_ALIGNED(size, AES_BLOCK_SIZE))
return 0;
break;
@@ -366,7 +371,8 @@ static int cc_cipher_sethkey(struct crypto_skcipher *sktfm, const u8 *key,
}
if (ctx_p->cipher_mode == DRV_CIPHER_XTS ||
- ctx_p->cipher_mode == DRV_CIPHER_ESSIV) {
+ ctx_p->cipher_mode == DRV_CIPHER_ESSIV ||
+ ctx_p->cipher_mode == DRV_CIPHER_BITLOCKER) {
if (hki.hw_key1 == hki.hw_key2) {
dev_err(dev, "Illegal hw key numbers (%d,%d)\n",
hki.hw_key1, hki.hw_key2);
@@ -564,6 +570,7 @@ static void cc_setup_readiv_desc(struct crypto_tfm *tfm,
break;
case DRV_CIPHER_XTS:
case DRV_CIPHER_ESSIV:
+ case DRV_CIPHER_BITLOCKER:
/* IV */
hw_desc_init(&desc[*seq_size]);
set_setup_mode(&desc[*seq_size], SETUP_WRITE_STATE1);
@@ -618,6 +625,7 @@ static void cc_setup_state_desc(struct crypto_tfm *tfm,
break;
case DRV_CIPHER_XTS:
case DRV_CIPHER_ESSIV:
+ case DRV_CIPHER_BITLOCKER:
break;
default:
dev_err(dev, "Unsupported cipher mode (%d)\n", cipher_mode);
@@ -637,56 +645,68 @@ static void cc_setup_xex_state_desc(struct crypto_tfm *tfm,
int flow_mode = ctx_p->flow_mode;
int direction = req_ctx->gen_ctx.op_type;
dma_addr_t key_dma_addr = ctx_p->user.key_dma_addr;
- unsigned int key_len = (ctx_p->keylen / 2);
dma_addr_t iv_dma_addr = req_ctx->gen_ctx.iv_dma_addr;
- unsigned int key_offset = key_len;
+ unsigned int key_len;
+ unsigned int key_offset;
switch (cipher_mode) {
case DRV_CIPHER_ECB:
- break;
case DRV_CIPHER_CBC:
case DRV_CIPHER_CBC_CTS:
case DRV_CIPHER_CTR:
case DRV_CIPHER_OFB:
- break;
- case DRV_CIPHER_XTS:
- case DRV_CIPHER_ESSIV:
+ /* No secondary key for these ciphers, so just return */
+ return;
- if (cipher_mode == DRV_CIPHER_ESSIV)
- key_len = SHA256_DIGEST_SIZE;
+ case DRV_CIPHER_XTS:
+ /* Secondary key is same size as primary key and stored after primary key */
+ key_len = ctx_p->keylen / 2;
+ key_offset = key_len;
+ break;
- /* load XEX key */
- hw_desc_init(&desc[*seq_size]);
- set_cipher_mode(&desc[*seq_size], cipher_mode);
- set_cipher_config0(&desc[*seq_size], direction);
- if (cc_key_type(tfm) == CC_HW_PROTECTED_KEY) {
- set_hw_crypto_key(&desc[*seq_size],
- ctx_p->hw.key2_slot);
- } else {
- set_din_type(&desc[*seq_size], DMA_DLLI,
- (key_dma_addr + key_offset),
- key_len, NS_BIT);
- }
- set_xex_data_unit_size(&desc[*seq_size], nbytes);
- set_flow_mode(&desc[*seq_size], S_DIN_to_AES2);
- set_key_size_aes(&desc[*seq_size], key_len);
- set_setup_mode(&desc[*seq_size], SETUP_LOAD_XEX_KEY);
- (*seq_size)++;
+ case DRV_CIPHER_ESSIV:
+ /* Secondary key is a digest of primary key and stored after primary key */
+ key_len = SHA256_DIGEST_SIZE;
+ key_offset = ctx_p->keylen / 2;
+ break;
- /* Load IV */
- hw_desc_init(&desc[*seq_size]);
- set_setup_mode(&desc[*seq_size], SETUP_LOAD_STATE1);
- set_cipher_mode(&desc[*seq_size], cipher_mode);
- set_cipher_config0(&desc[*seq_size], direction);
- set_key_size_aes(&desc[*seq_size], key_len);
- set_flow_mode(&desc[*seq_size], flow_mode);
- set_din_type(&desc[*seq_size], DMA_DLLI, iv_dma_addr,
- CC_AES_BLOCK_SIZE, NS_BIT);
- (*seq_size)++;
+ case DRV_CIPHER_BITLOCKER:
+ /* Secondary key is same as primary key */
+ key_len = ctx_p->keylen;
+ key_offset = 0;
break;
+
default:
dev_err(dev, "Unsupported cipher mode (%d)\n", cipher_mode);
}
+
+ /* load XEX key */
+ hw_desc_init(&desc[*seq_size]);
+ set_cipher_mode(&desc[*seq_size], cipher_mode);
+ set_cipher_config0(&desc[*seq_size], direction);
+ if (cc_key_type(tfm) == CC_HW_PROTECTED_KEY) {
+ set_hw_crypto_key(&desc[*seq_size],
+ ctx_p->hw.key2_slot);
+ } else {
+ set_din_type(&desc[*seq_size], DMA_DLLI,
+ (key_dma_addr + key_offset),
+ key_len, NS_BIT);
+ }
+ set_xex_data_unit_size(&desc[*seq_size], nbytes);
+ set_flow_mode(&desc[*seq_size], S_DIN_to_AES2);
+ set_key_size_aes(&desc[*seq_size], key_len);
+ set_setup_mode(&desc[*seq_size], SETUP_LOAD_XEX_KEY);
+ (*seq_size)++;
+
+ /* Load IV */
+ hw_desc_init(&desc[*seq_size]);
+ set_setup_mode(&desc[*seq_size], SETUP_LOAD_STATE1);
+ set_cipher_mode(&desc[*seq_size], cipher_mode);
+ set_cipher_config0(&desc[*seq_size], direction);
+ set_key_size_aes(&desc[*seq_size], key_len);
+ set_flow_mode(&desc[*seq_size], flow_mode);
+ set_din_type(&desc[*seq_size], DMA_DLLI, iv_dma_addr, CC_AES_BLOCK_SIZE, NS_BIT);
+ (*seq_size)++;
}
static int cc_out_flow_mode(struct cc_cipher_ctx *ctx_p)
@@ -723,6 +743,7 @@ static void cc_setup_key_desc(struct crypto_tfm *tfm,
case DRV_CIPHER_CTR:
case DRV_CIPHER_OFB:
case DRV_CIPHER_ECB:
+ case DRV_CIPHER_BITLOCKER:
/* Load key */
hw_desc_init(&desc[*seq_size]);
set_cipher_mode(&desc[*seq_size], cipher_mode);
@@ -1061,6 +1082,24 @@ static const struct cc_alg_template skcipher_algs[] = {
.std_body = CC_STD_NIST,
.sec_func = true,
},
+ {
+ .name = "eboiv(cbc(paes))",
+ .driver_name = "eboiv-cbc-paes-ccree",
+ .blocksize = AES_BLOCK_SIZE,
+ .template_skcipher = {
+ .setkey = cc_cipher_sethkey,
+ .encrypt = cc_cipher_encrypt,
+ .decrypt = cc_cipher_decrypt,
+ .min_keysize = CC_HW_KEY_SIZE,
+ .max_keysize = CC_HW_KEY_SIZE,
+ .ivsize = AES_BLOCK_SIZE,
+ },
+ .cipher_mode = DRV_CIPHER_BITLOCKER,
+ .flow_mode = S_DIN_to_AES,
+ .min_hw_rev = CC_HW_REV_712,
+ .std_body = CC_STD_NIST,
+ .sec_func = true,
+ },
{
.name = "ecb(paes)",
.driver_name = "ecb-paes-ccree",
@@ -1189,6 +1228,23 @@ static const struct cc_alg_template skcipher_algs[] = {
.min_hw_rev = CC_HW_REV_712,
.std_body = CC_STD_NIST,
},
+ {
+ .name = "eboiv(cbc(aes))",
+ .driver_name = "eboiv-cbc-aes-ccree",
+ .blocksize = AES_BLOCK_SIZE,
+ .template_skcipher = {
+ .setkey = cc_cipher_setkey,
+ .encrypt = cc_cipher_encrypt,
+ .decrypt = cc_cipher_decrypt,
+ .min_keysize = AES_MIN_KEY_SIZE,
+ .max_keysize = AES_MAX_KEY_SIZE,
+ .ivsize = AES_BLOCK_SIZE,
+ },
+ .cipher_mode = DRV_CIPHER_BITLOCKER,
+ .flow_mode = S_DIN_to_AES,
+ .min_hw_rev = CC_HW_REV_712,
+ .std_body = CC_STD_NIST,
+ },
{
.name = "ecb(aes)",
.driver_name = "ecb-aes-ccree",
diff --git a/drivers/crypto/ccree/cc_crypto_ctx.h b/drivers/crypto/ccree/cc_crypto_ctx.h
index bd9a1c0896b3..ccf960a0d989 100644
--- a/drivers/crypto/ccree/cc_crypto_ctx.h
+++ b/drivers/crypto/ccree/cc_crypto_ctx.h
@@ -108,6 +108,7 @@ enum drv_cipher_mode {
DRV_CIPHER_CBC_CTS = 11,
DRV_CIPHER_GCTR = 12,
DRV_CIPHER_ESSIV = 13,
+ DRV_CIPHER_BITLOCKER = 14,
DRV_CIPHER_RESERVE32B = S32_MAX
};
--
2.28.0
Replace the explicit EBOIV handling in the dm-crypt driver with calls
into the crypto API, which now possesses the capability to perform
this processing within the crypto subsystem.
Signed-off-by: Gilad Ben-Yossef <[email protected]>
---
drivers/md/Kconfig | 1 +
drivers/md/dm-crypt.c | 61 ++++++++++++++-----------------------------
2 files changed, 20 insertions(+), 42 deletions(-)
diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index 30ba3573626c..ca6e56a72281 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -273,6 +273,7 @@ config DM_CRYPT
select CRYPTO
select CRYPTO_CBC
select CRYPTO_ESSIV
+ select CRYPTO_EBOIV
help
This device-mapper target allows you to create a device that
transparently encrypts the data on it. You'll need to activate
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 148960721254..cad8f4e3f5d9 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -716,47 +716,18 @@ static int crypt_iv_random_gen(struct crypt_config *cc, u8 *iv,
return 0;
}
-static int crypt_iv_eboiv_ctr(struct crypt_config *cc, struct dm_target *ti,
- const char *opts)
-{
- if (crypt_integrity_aead(cc)) {
- ti->error = "AEAD transforms not supported for EBOIV";
- return -EINVAL;
- }
-
- if (crypto_skcipher_blocksize(any_tfm(cc)) != cc->iv_size) {
- ti->error = "Block size of EBOIV cipher does "
- "not match IV size of block cipher";
- return -EINVAL;
- }
-
- return 0;
-}
-
static int crypt_iv_eboiv_gen(struct crypt_config *cc, u8 *iv,
struct dm_crypt_request *dmreq)
{
- u8 buf[MAX_CIPHER_BLOCKSIZE] __aligned(__alignof__(__le64));
- struct skcipher_request *req;
- struct scatterlist src, dst;
- struct crypto_wait wait;
- int err;
-
- req = skcipher_request_alloc(any_tfm(cc), GFP_NOIO);
- if (!req)
- return -ENOMEM;
-
- memset(buf, 0, cc->iv_size);
- *(__le64 *)buf = cpu_to_le64(dmreq->iv_sector * cc->sector_size);
- sg_init_one(&src, page_address(ZERO_PAGE(0)), cc->iv_size);
- sg_init_one(&dst, iv, cc->iv_size);
- skcipher_request_set_crypt(req, &src, &dst, cc->iv_size, buf);
- skcipher_request_set_callback(req, 0, crypto_req_done, &wait);
- err = crypto_wait_req(crypto_skcipher_encrypt(req), &wait);
- skcipher_request_free(req);
+ /*
+ * ESSIV encryption of the IV is handled by the crypto API,
+ * so compute and pass the sector offset here.
+ */
+ memset(iv, 0, cc->iv_size);
+ *(__le64 *)iv = cpu_to_le64(dmreq->iv_sector * cc->sector_size);
- return err;
+ return 0;
}
static void crypt_iv_elephant_dtr(struct crypt_config *cc)
@@ -777,13 +748,9 @@ static int crypt_iv_elephant_ctr(struct crypt_config *cc, struct dm_target *ti,
if (IS_ERR(elephant->tfm)) {
r = PTR_ERR(elephant->tfm);
elephant->tfm = NULL;
- return r;
}
- r = crypt_iv_eboiv_ctr(cc, ti, NULL);
- if (r)
- crypt_iv_elephant_dtr(cc);
- return r;
+ return 0;
}
static void diffuser_disk_to_cpu(u32 *d, size_t n)
@@ -1092,7 +1059,6 @@ static struct crypt_iv_operations crypt_iv_random_ops = {
};
static struct crypt_iv_operations crypt_iv_eboiv_ops = {
- .ctr = crypt_iv_eboiv_ctr,
.generator = crypt_iv_eboiv_gen
};
@@ -2739,6 +2705,15 @@ static int crypt_ctr_cipher_new(struct dm_target *ti, char *cipher_in, char *key
cipher_api = buf;
}
+ if (*ivmode && (!strcmp(*ivmode, "eboiv") || !strcmp(*ivmode, "elephant"))) {
+ ret = snprintf(buf, CRYPTO_MAX_ALG_NAME, "eboiv(%s)", cipher_api);
+ if (ret < 0 || ret >= CRYPTO_MAX_ALG_NAME) {
+ ti->error = "Cannot allocate cipher string";
+ return -ENOMEM;
+ }
+ cipher_api = buf;
+ }
+
cc->key_parts = cc->tfms_count;
/* Allocate cipher */
@@ -2817,6 +2792,8 @@ static int crypt_ctr_cipher_old(struct dm_target *ti, char *cipher_in, char *key
}
ret = snprintf(cipher_api, CRYPTO_MAX_ALG_NAME,
"essiv(%s(%s),%s)", chainmode, cipher, *ivopts);
+ } else if (*ivmode && (!strcmp(*ivmode, "eboiv") || !strcmp(*ivmode, "elephant"))) {
+ ret = snprintf(cipher_api, CRYPTO_MAX_ALG_NAME, "eboiv(%s(%s))", chainmode, cipher);
} else {
ret = snprintf(cipher_api, CRYPTO_MAX_ALG_NAME,
"%s(%s)", chainmode, cipher);
--
2.28.0
On Mon, Oct 26, 2020 at 03:04:46PM +0200, Gilad Ben-Yossef wrote:
> Replace the explicit EBOIV handling in the dm-crypt driver with calls
> into the crypto API, which now possesses the capability to perform
> this processing within the crypto subsystem.
>
> Signed-off-by: Gilad Ben-Yossef <[email protected]>
>
> ---
> drivers/md/Kconfig | 1 +
> drivers/md/dm-crypt.c | 61 ++++++++++++++-----------------------------
> 2 files changed, 20 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
> index 30ba3573626c..ca6e56a72281 100644
> --- a/drivers/md/Kconfig
> +++ b/drivers/md/Kconfig
> @@ -273,6 +273,7 @@ config DM_CRYPT
> select CRYPTO
> select CRYPTO_CBC
> select CRYPTO_ESSIV
> + select CRYPTO_EBOIV
> help
> This device-mapper target allows you to create a device that
> transparently encrypts the data on it. You'll need to activate
Can CRYPTO_EBOIV please not be selected by default? If someone really wants
Bitlocker compatibility support, they can select this option themselves.
- Eric
On 26/10/2020 18:52, Eric Biggers wrote:
> On Mon, Oct 26, 2020 at 03:04:46PM +0200, Gilad Ben-Yossef wrote:
>> Replace the explicit EBOIV handling in the dm-crypt driver with calls
>> into the crypto API, which now possesses the capability to perform
>> this processing within the crypto subsystem.
>>
>> Signed-off-by: Gilad Ben-Yossef <[email protected]>
>>
>> ---
>> drivers/md/Kconfig | 1 +
>> drivers/md/dm-crypt.c | 61 ++++++++++++++-----------------------------
>> 2 files changed, 20 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
>> index 30ba3573626c..ca6e56a72281 100644
>> --- a/drivers/md/Kconfig
>> +++ b/drivers/md/Kconfig
>> @@ -273,6 +273,7 @@ config DM_CRYPT
>> select CRYPTO
>> select CRYPTO_CBC
>> select CRYPTO_ESSIV
>> + select CRYPTO_EBOIV
>> help
>> This device-mapper target allows you to create a device that
>> transparently encrypts the data on it. You'll need to activate
>
> Can CRYPTO_EBOIV please not be selected by default? If someone really wants
> Bitlocker compatibility support, they can select this option themselves.
Please no! Until this move of IV to crypto API, we can rely on
support in dm-crypt (if it is not supported, it is just a very old kernel).
(Actually, this was the first thing I checked in this patchset - if it is
unconditionally enabled for compatibility once dmcrypt is selected.)
People already use removable devices with BitLocker.
It was the whole point that it works out-of-the-box without enabling anything.
If you insist on this to be optional, please better keep this IV inside dmcrypt.
(EBOIV has no other use than for disk encryption anyway.)
Or maybe another option would be to introduce option under dm-crypt Kconfig that
defaults to enabled (like support for foreign/legacy disk encryption schemes) and that
selects these IVs/modes.
But requiring some random switch in crypto API will only confuse users.
Milan
On Mon, Oct 26, 2020 at 07:29:57PM +0100, Milan Broz wrote:
> On 26/10/2020 18:52, Eric Biggers wrote:
> > On Mon, Oct 26, 2020 at 03:04:46PM +0200, Gilad Ben-Yossef wrote:
> >> Replace the explicit EBOIV handling in the dm-crypt driver with calls
> >> into the crypto API, which now possesses the capability to perform
> >> this processing within the crypto subsystem.
> >>
> >> Signed-off-by: Gilad Ben-Yossef <[email protected]>
> >>
> >> ---
> >> drivers/md/Kconfig | 1 +
> >> drivers/md/dm-crypt.c | 61 ++++++++++++++-----------------------------
> >> 2 files changed, 20 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
> >> index 30ba3573626c..ca6e56a72281 100644
> >> --- a/drivers/md/Kconfig
> >> +++ b/drivers/md/Kconfig
> >> @@ -273,6 +273,7 @@ config DM_CRYPT
> >> select CRYPTO
> >> select CRYPTO_CBC
> >> select CRYPTO_ESSIV
> >> + select CRYPTO_EBOIV
> >> help
> >> This device-mapper target allows you to create a device that
> >> transparently encrypts the data on it. You'll need to activate
> >
> > Can CRYPTO_EBOIV please not be selected by default? If someone really wants
> > Bitlocker compatibility support, they can select this option themselves.
>
> Please no! Until this move of IV to crypto API, we can rely on
> support in dm-crypt (if it is not supported, it is just a very old kernel).
> (Actually, this was the first thing I checked in this patchset - if it is
> unconditionally enabled for compatibility once dmcrypt is selected.)
>
> People already use removable devices with BitLocker.
> It was the whole point that it works out-of-the-box without enabling anything.
>
> If you insist on this to be optional, please better keep this IV inside dmcrypt.
> (EBOIV has no other use than for disk encryption anyway.)
>
> Or maybe another option would be to introduce option under dm-crypt Kconfig that
> defaults to enabled (like support for foreign/legacy disk encryption schemes) and that
> selects these IVs/modes.
> But requiring some random switch in crypto API will only confuse users.
CONFIG_DM_CRYPT can either select every weird combination of algorithms anyone
can ever be using, or it can select some defaults and require any other needed
algorithms to be explicitly selected.
In reality, dm-crypt has never even selected any particular block ciphers, even
AES. Nor has it ever selected XTS. So it's actually always made users (or
kernel distributors) explicitly select algorithms. Why the Bitlocker support
suddenly different?
I'd think a lot of dm-crypt users don't want to bloat their kernels with random
legacy algorithms.
- Eric
On Mon, Oct 26, 2020 at 11:39:36AM -0700, Eric Biggers wrote:
>
> CONFIG_DM_CRYPT can either select every weird combination of algorithms anyone
> can ever be using, or it can select some defaults and require any other needed
> algorithms to be explicitly selected.
>
> In reality, dm-crypt has never even selected any particular block ciphers, even
> AES. Nor has it ever selected XTS. So it's actually always made users (or
> kernel distributors) explicitly select algorithms. Why the Bitlocker support
> suddenly different?
>
> I'd think a lot of dm-crypt users don't want to bloat their kernels with random
> legacy algorithms.
The point is that people rebuilding their kernel can end up with a
broken system. Just set a default on EBOIV if dm-crypt is on.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Tue, Oct 27, 2020 at 05:41:55AM +1100, Herbert Xu wrote:
>
> The point is that people rebuilding their kernel can end up with a
> broken system. Just set a default on EBOIV if dm-crypt is on.
That's not enough as it's an existing option. So we need to
add a new Kconfig option with a default equal to dm-crypt.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On 26/10/2020 19:39, Eric Biggers wrote:
> On Mon, Oct 26, 2020 at 07:29:57PM +0100, Milan Broz wrote:
>> On 26/10/2020 18:52, Eric Biggers wrote:
>>> On Mon, Oct 26, 2020 at 03:04:46PM +0200, Gilad Ben-Yossef wrote:
>>>> Replace the explicit EBOIV handling in the dm-crypt driver with calls
>>>> into the crypto API, which now possesses the capability to perform
>>>> this processing within the crypto subsystem.
>>>>
>>>> Signed-off-by: Gilad Ben-Yossef <[email protected]>
>>>>
>>>> ---
>>>> drivers/md/Kconfig | 1 +
>>>> drivers/md/dm-crypt.c | 61 ++++++++++++++-----------------------------
>>>> 2 files changed, 20 insertions(+), 42 deletions(-)
>>>>
>>>> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
>>>> index 30ba3573626c..ca6e56a72281 100644
>>>> --- a/drivers/md/Kconfig
>>>> +++ b/drivers/md/Kconfig
>>>> @@ -273,6 +273,7 @@ config DM_CRYPT
>>>> select CRYPTO
>>>> select CRYPTO_CBC
>>>> select CRYPTO_ESSIV
>>>> + select CRYPTO_EBOIV
>>>> help
>>>> This device-mapper target allows you to create a device that
>>>> transparently encrypts the data on it. You'll need to activate
>>>
>>> Can CRYPTO_EBOIV please not be selected by default? If someone really wants
>>> Bitlocker compatibility support, they can select this option themselves.
>>
>> Please no! Until this move of IV to crypto API, we can rely on
>> support in dm-crypt (if it is not supported, it is just a very old kernel).
>> (Actually, this was the first thing I checked in this patchset - if it is
>> unconditionally enabled for compatibility once dmcrypt is selected.)
>>
>> People already use removable devices with BitLocker.
>> It was the whole point that it works out-of-the-box without enabling anything.
>>
>> If you insist on this to be optional, please better keep this IV inside dmcrypt.
>> (EBOIV has no other use than for disk encryption anyway.)
>>
>> Or maybe another option would be to introduce option under dm-crypt Kconfig that
>> defaults to enabled (like support for foreign/legacy disk encryption schemes) and that
>> selects these IVs/modes.
>> But requiring some random switch in crypto API will only confuse users.
>
> CONFIG_DM_CRYPT can either select every weird combination of algorithms anyone
> can ever be using, or it can select some defaults and require any other needed
> algorithms to be explicitly selected.
>
> In reality, dm-crypt has never even selected any particular block ciphers, even
> AES. Nor has it ever selected XTS. So it's actually always made users (or
> kernel distributors) explicitly select algorithms. Why the Bitlocker support
> suddenly different?
>
> I'd think a lot of dm-crypt users don't want to bloat their kernels with random
> legacy algorithms.
Yes, but IV is in reality not a cryptographic algorithm, it is kind-of a configuration
"option" of sector encryption mode here.
We had all of disk-IV inside dmcrypt before - but once it is partially moved into crypto API
(ESSIV, EBOIV for now), it becomes much more complicated for user to select what he needs.
I think we have no way to check that IV is available from userspace - it
will report the same error as if block cipher is not available, not helping user much
with the error.
But then I also think we should add abstract dm-crypt options here (Legacy TrueCrypt modes,
Bitlocker modes) that will select these crypto API configuration switches.
Otherwise it will be only a complicated matrix of crypto API options...
Milan
Hi Gilad,
I love your patch! Perhaps something to improve:
[auto build test WARNING on cryptodev/master]
[also build test WARNING on crypto/master]
[cannot apply to dm/for-next v5.10-rc1 next-20201026]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Gilad-Ben-Yossef/crypto-switch-to-crypto-API-for-EBOIV-generation/20201026-210817
base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
config: x86_64-randconfig-a005-20201026 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project f2c25c70791de95d2466e09b5b58fc37f6ccd7a4)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/cebe27982e51dca8b744adebe5b6f6bcb726e1c8
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Gilad-Ben-Yossef/crypto-switch-to-crypto-API-for-EBOIV-generation/20201026-210817
git checkout cebe27982e51dca8b744adebe5b6f6bcb726e1c8
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/crypto/ccree/cc_cipher.c:658:2: warning: variable 'key_len' is used uninitialized whenever switch default is taken [-Wsometimes-uninitialized]
default:
^~~~~~~
drivers/crypto/ccree/cc_cipher.c:676:37: note: uninitialized use occurs here
set_key_size_aes(&desc[*seq_size], key_len);
^~~~~~~
drivers/crypto/ccree/cc_cipher.c:628:22: note: initialize the variable 'key_len' to silence this warning
unsigned int key_len;
^
= 0
1 warning generated.
vim +/key_len +658 drivers/crypto/ccree/cc_cipher.c
613
614
615 static void cc_setup_xex_state_desc(struct crypto_tfm *tfm,
616 struct cipher_req_ctx *req_ctx,
617 unsigned int ivsize, unsigned int nbytes,
618 struct cc_hw_desc desc[],
619 unsigned int *seq_size)
620 {
621 struct cc_cipher_ctx *ctx_p = crypto_tfm_ctx(tfm);
622 struct device *dev = drvdata_to_dev(ctx_p->drvdata);
623 int cipher_mode = ctx_p->cipher_mode;
624 int flow_mode = ctx_p->flow_mode;
625 int direction = req_ctx->gen_ctx.op_type;
626 dma_addr_t key_dma_addr = ctx_p->user.key_dma_addr;
627 dma_addr_t iv_dma_addr = req_ctx->gen_ctx.iv_dma_addr;
628 unsigned int key_len;
629 unsigned int key_offset;
630
631 switch (cipher_mode) {
632 case DRV_CIPHER_ECB:
633 case DRV_CIPHER_CBC:
634 case DRV_CIPHER_CBC_CTS:
635 case DRV_CIPHER_CTR:
636 case DRV_CIPHER_OFB:
637 /* No secondary key for these ciphers, so just return */
638 return;
639
640 case DRV_CIPHER_XTS:
641 /* Secondary key is same size as primary key and stored after primary key */
642 key_len = ctx_p->keylen / 2;
643 key_offset = key_len;
644 break;
645
646 case DRV_CIPHER_ESSIV:
647 /* Secondary key is a digest of primary key and stored after primary key */
648 key_len = SHA256_DIGEST_SIZE;
649 key_offset = ctx_p->keylen / 2;
650 break;
651
652 case DRV_CIPHER_BITLOCKER:
653 /* Secondary key is same as primary key */
654 key_len = ctx_p->keylen;
655 key_offset = 0;
656 break;
657
> 658 default:
659 dev_err(dev, "Unsupported cipher mode (%d)\n", cipher_mode);
660 }
661
662 /* load XEX key */
663 hw_desc_init(&desc[*seq_size]);
664 set_cipher_mode(&desc[*seq_size], cipher_mode);
665 set_cipher_config0(&desc[*seq_size], direction);
666 if (cc_key_type(tfm) == CC_HW_PROTECTED_KEY) {
667 set_hw_crypto_key(&desc[*seq_size],
668 ctx_p->hw.key2_slot);
669 } else {
670 set_din_type(&desc[*seq_size], DMA_DLLI,
671 (key_dma_addr + key_offset),
672 key_len, NS_BIT);
673 }
674 set_xex_data_unit_size(&desc[*seq_size], nbytes);
675 set_flow_mode(&desc[*seq_size], S_DIN_to_AES2);
676 set_key_size_aes(&desc[*seq_size], key_len);
677 set_setup_mode(&desc[*seq_size], SETUP_LOAD_XEX_KEY);
678 (*seq_size)++;
679
680 /* Load IV */
681 hw_desc_init(&desc[*seq_size]);
682 set_setup_mode(&desc[*seq_size], SETUP_LOAD_STATE1);
683 set_cipher_mode(&desc[*seq_size], cipher_mode);
684 set_cipher_config0(&desc[*seq_size], direction);
685 set_key_size_aes(&desc[*seq_size], key_len);
686 set_flow_mode(&desc[*seq_size], flow_mode);
687 set_din_type(&desc[*seq_size], DMA_DLLI, iv_dma_addr, CC_AES_BLOCK_SIZE, NS_BIT);
688 (*seq_size)++;
689 }
690
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Mon, Oct 26, 2020 at 9:04 PM Milan Broz <[email protected]> wrote:
>
>
>
> On 26/10/2020 19:39, Eric Biggers wrote:
> > On Mon, Oct 26, 2020 at 07:29:57PM +0100, Milan Broz wrote:
> >> On 26/10/2020 18:52, Eric Biggers wrote:
> >>> On Mon, Oct 26, 2020 at 03:04:46PM +0200, Gilad Ben-Yossef wrote:
> >>>> Replace the explicit EBOIV handling in the dm-crypt driver with calls
> >>>> into the crypto API, which now possesses the capability to perform
> >>>> this processing within the crypto subsystem.
> >>>>
> >>>> Signed-off-by: Gilad Ben-Yossef <[email protected]>
> >>>>
> >>>> ---
> >>>> drivers/md/Kconfig | 1 +
> >>>> drivers/md/dm-crypt.c | 61 ++++++++++++++-----------------------------
> >>>> 2 files changed, 20 insertions(+), 42 deletions(-)
> >>>>
> >>>> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
> >>>> index 30ba3573626c..ca6e56a72281 100644
> >>>> --- a/drivers/md/Kconfig
> >>>> +++ b/drivers/md/Kconfig
> >>>> @@ -273,6 +273,7 @@ config DM_CRYPT
> >>>> select CRYPTO
> >>>> select CRYPTO_CBC
> >>>> select CRYPTO_ESSIV
> >>>> + select CRYPTO_EBOIV
> >>>> help
> >>>> This device-mapper target allows you to create a device that
> >>>> transparently encrypts the data on it. You'll need to activate
> >>>
> >>> Can CRYPTO_EBOIV please not be selected by default? If someone really wants
> >>> Bitlocker compatibility support, they can select this option themselves.
> >>
> >> Please no! Until this move of IV to crypto API, we can rely on
> >> support in dm-crypt (if it is not supported, it is just a very old kernel).
> >> (Actually, this was the first thing I checked in this patchset - if it is
> >> unconditionally enabled for compatibility once dmcrypt is selected.)
> >>
> >> People already use removable devices with BitLocker.
> >> It was the whole point that it works out-of-the-box without enabling anything.
> >>
> >> If you insist on this to be optional, please better keep this IV inside dmcrypt.
> >> (EBOIV has no other use than for disk encryption anyway.)
> >>
> >> Or maybe another option would be to introduce option under dm-crypt Kconfig that
> >> defaults to enabled (like support for foreign/legacy disk encryption schemes) and that
> >> selects these IVs/modes.
> >> But requiring some random switch in crypto API will only confuse users.
> >
> > CONFIG_DM_CRYPT can either select every weird combination of algorithms anyone
> > can ever be using, or it can select some defaults and require any other needed
> > algorithms to be explicitly selected.
> >
> > In reality, dm-crypt has never even selected any particular block ciphers, even
> > AES. Nor has it ever selected XTS. So it's actually always made users (or
> > kernel distributors) explicitly select algorithms. Why the Bitlocker support
> > suddenly different?
> >
> > I'd think a lot of dm-crypt users don't want to bloat their kernels with random
> > legacy algorithms.
>
> Yes, but IV is in reality not a cryptographic algorithm, it is kind-of a configuration
> "option" of sector encryption mode here.
>
> We had all of disk-IV inside dmcrypt before - but once it is partially moved into crypto API
> (ESSIV, EBOIV for now), it becomes much more complicated for user to select what he needs.
>
> I think we have no way to check that IV is available from userspace - it
> will report the same error as if block cipher is not available, not helping user much
> with the error.
>
> But then I also think we should add abstract dm-crypt options here (Legacy TrueCrypt modes,
> Bitlocker modes) that will select these crypto API configuration switches.
> Otherwise it will be only a complicated matrix of crypto API options...
hm... just thinking out loud, but maybe the right say to go is to not
have a build dependency,
but add some user assistance code in cryptosetup that parses
/proc/crypto after failures to
try and suggest the user with a way forward?
e.g. if eboiv mapping initiation fails, scan /proc/crypto and either
warn of a lack of AES
or, assuming some instance of AES is found, warn of lack of EBOIV.
It's a little messy
and heuristic code for sure, but it lives in a user space utility.
Does that sound sane?
--
Gilad Ben-Yossef
Chief Coffee Drinker
values of β will give rise to dom!
On 27/10/2020 07:59, Gilad Ben-Yossef wrote:
> On Mon, Oct 26, 2020 at 9:04 PM Milan Broz <[email protected]> wrote:
...
>> We had all of disk-IV inside dmcrypt before - but once it is partially moved into crypto API
>> (ESSIV, EBOIV for now), it becomes much more complicated for user to select what he needs.
>>
>> I think we have no way to check that IV is available from userspace - it
>> will report the same error as if block cipher is not available, not helping user much
>> with the error.
>>
>> But then I also think we should add abstract dm-crypt options here (Legacy TrueCrypt modes,
>> Bitlocker modes) that will select these crypto API configuration switches.
>> Otherwise it will be only a complicated matrix of crypto API options...
>
> hm... just thinking out loud, but maybe the right say to go is to not
> have a build dependency,
> but add some user assistance code in cryptosetup that parses
> /proc/crypto after failures to
> try and suggest the user with a way forward?
>
> e.g. if eboiv mapping initiation fails, scan /proc/crypto and either
> warn of a lack of AES
> or, assuming some instance of AES is found, warn of lack of EBOIV.
> It's a little messy
> and heuristic code for sure, but it lives in a user space utility.
>
> Does that sound sane?
Such an idea (try to parse /proc/crypto) is on my TODO list since 2009 :)
I expected userspace kernel crypto API could help here, but it seems it is not the case.
So yes, I think we need to add something like this in userspace. In combination with
the kernel and dmcrypt target version, we could have a pretty good hint matrix for the user,
instead of (literally) cryptic errors.
(There is also a problem that device-mapper targets are losing detailed error state.
We often end just with -EINVAL during table create ... and no descriptive log entry.
And leaking info about encrypted devices activation failures to syslog is not a good idea either.)
Anyway, this will not fix existing userspace that is not prepared for this kind
of EBOIV missing fail, so Herbert's solution seems like the solution for this particular
problem for now. (But I agree we should perhaps remove these build dependences in future completely...)
Milan
On Mon, Oct 26, 2020 at 8:44 PM Herbert Xu <[email protected]> wrote:
>
> On Tue, Oct 27, 2020 at 05:41:55AM +1100, Herbert Xu wrote:
> >
> > The point is that people rebuilding their kernel can end up with a
> > broken system. Just set a default on EBOIV if dm-crypt is on.
>
> That's not enough as it's an existing option. So we need to
> add a new Kconfig option with a default equal to dm-crypt.
Sorry if I'm being daft, but what did you refer to be "an existing
option"? there was no CONFIG_EBOIV before my patchset, it was simply
built as part of dm-crypt so it seems that setting CONFIG_EBOIV
default to dm-crypto Kconfig option value does solves the problem, or
have I missed something?
Thanks,
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
values of β will give rise to dom!
On Wed, Oct 28, 2020 at 01:41:28PM +0200, Gilad Ben-Yossef wrote:
>
> Sorry if I'm being daft, but what did you refer to be "an existing
> option"? there was no CONFIG_EBOIV before my patchset, it was simply
> built as part of dm-crypt so it seems that setting CONFIG_EBOIV
> default to dm-crypto Kconfig option value does solves the problem, or
> have I missed something?
Oh I'm mistaken then. I thought it was an existing option. If
it's a new option then a default depending on dm-crypt should be
sufficient.
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt