| Message ID | 20190810094053.7423-4-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: switch to crypto API for ESSIV generation | expand |
Hi, On 10/08/2019 11:40, Ard Biesheuvel wrote: > Replace the explicit ESSIV 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: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > drivers/md/Kconfig | 1 + > drivers/md/dm-crypt.c | 194 ++++---------------- > 2 files changed, 33 insertions(+), 162 deletions(-) > > diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig > index 3834332f4963..b727e8f15264 100644 > --- a/drivers/md/Kconfig > +++ b/drivers/md/Kconfig ... > @@ -2493,6 +2339,20 @@ static int crypt_ctr_cipher_new(struct dm_target *ti, char *cipher_in, char *key > if (*ivmode && !strcmp(*ivmode, "lmk")) > cc->tfms_count = 64; > > + if (*ivmode && !strcmp(*ivmode, "essiv")) { > + if (!*ivopts) { > + ti->error = "Digest algorithm missing for ESSIV mode"; > + return -EINVAL; > + } > + ret = snprintf(buf, CRYPTO_MAX_ALG_NAME, "essiv(%s,%s)", > + cipher_api, *ivopts); This is wrong. It works only in length-preserving modes, not in AEAD modes. Try for example # cryptsetup luksFormat /dev/sdc -c aes-cbc-essiv:sha256 --integrity hmac-sha256 -q -i1 It should produce Crypto API string authenc(hmac(sha256),essiv(cbc(aes),sha256)) while it produces essiv(authenc(hmac(sha256),cbc(aes)),sha256) (and fails). You can run "luks2-integrity-test" from cryptsetup test suite to detect it. Just the test does not fail, it prints N/A for ESSIV use cases - we need to deal with older kernels... I can probable change it to fail unconditionally though. ... > @@ -2579,9 +2439,19 @@ static int crypt_ctr_cipher_old(struct dm_target *ti, char *cipher_in, char *key > if (!cipher_api) > goto bad_mem; > > - ret = snprintf(cipher_api, CRYPTO_MAX_ALG_NAME, > - "%s(%s)", chainmode, cipher); > - if (ret < 0) { > + if (*ivmode && !strcmp(*ivmode, "essiv")) { > + if (!*ivopts) { > + ti->error = "Digest algorithm missing for ESSIV mode"; > + kfree(cipher_api); > + return -EINVAL; > + } > + ret = snprintf(cipher_api, CRYPTO_MAX_ALG_NAME, > + "essiv(%s(%s),%s)", chainmode, cipher, *ivopts); I guess here it is ok, because old forma cannot use AEAD. Thanks, Milan
On Mon, 12 Aug 2019 at 09:33, Milan Broz <gmazyland@gmail.com> wrote: > > Hi, > > On 10/08/2019 11:40, Ard Biesheuvel wrote: > > Replace the explicit ESSIV 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: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > --- > > drivers/md/Kconfig | 1 + > > drivers/md/dm-crypt.c | 194 ++++---------------- > > 2 files changed, 33 insertions(+), 162 deletions(-) > > > > diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig > > index 3834332f4963..b727e8f15264 100644 > > --- a/drivers/md/Kconfig > > +++ b/drivers/md/Kconfig > ... > > @@ -2493,6 +2339,20 @@ static int crypt_ctr_cipher_new(struct dm_target *ti, char *cipher_in, char *key > > if (*ivmode && !strcmp(*ivmode, "lmk")) > > cc->tfms_count = 64; > > > > + if (*ivmode && !strcmp(*ivmode, "essiv")) { > > + if (!*ivopts) { > > + ti->error = "Digest algorithm missing for ESSIV mode"; > > + return -EINVAL; > > + } > > + ret = snprintf(buf, CRYPTO_MAX_ALG_NAME, "essiv(%s,%s)", > > + cipher_api, *ivopts); > > This is wrong. It works only in length-preserving modes, not in AEAD modes. > > Try for example > # cryptsetup luksFormat /dev/sdc -c aes-cbc-essiv:sha256 --integrity hmac-sha256 -q -i1 > > It should produce Crypto API string > authenc(hmac(sha256),essiv(cbc(aes),sha256)) > while it produces > essiv(authenc(hmac(sha256),cbc(aes)),sha256) > (and fails). > No. I don't know why it fails, but the latter is actually the correct string. The essiv template is instantiated either as a skcipher or as an aead, and it encapsulates the entire transformation. (This is necessary considering that the IV is passed via the AAD and so the ESSIV handling needs to touch that as well) This code worked fine in my testing: I could instantiate essiv(authenc(hmac(sha256),cbc(aes)),sha256) essiv(authenc(hmac(sha1),cbc(aes)),sha256) where the former worked as expected (including fuzz testing of the arm64 implementation), and the second got instantiated as well, but with a complaint about a missing test case. So I'm not sure why this is failing, I will try to check once I have access to my ordinary development environment. > You can run "luks2-integrity-test" from cryptsetup test suite to detect it. > > Just the test does not fail, it prints N/A for ESSIV use cases - we need to deal with older kernels... > I can probable change it to fail unconditionally though. > > ... > > @@ -2579,9 +2439,19 @@ static int crypt_ctr_cipher_old(struct dm_target *ti, char *cipher_in, char *key > > if (!cipher_api) > > goto bad_mem; > > > > - ret = snprintf(cipher_api, CRYPTO_MAX_ALG_NAME, > > - "%s(%s)", chainmode, cipher); > > - if (ret < 0) { > > + if (*ivmode && !strcmp(*ivmode, "essiv")) { > > + if (!*ivopts) { > > + ti->error = "Digest algorithm missing for ESSIV mode"; > > + kfree(cipher_api); > > + return -EINVAL; > > + } > > + ret = snprintf(cipher_api, CRYPTO_MAX_ALG_NAME, > > + "essiv(%s(%s),%s)", chainmode, cipher, *ivopts); > > I guess here it is ok, because old forma cannot use AEAD. > > Thanks, > Milan
On 12/08/2019 08:54, Ard Biesheuvel wrote: > On Mon, 12 Aug 2019 at 09:33, Milan Broz <gmazyland@gmail.com> wrote: >> Try for example >> # cryptsetup luksFormat /dev/sdc -c aes-cbc-essiv:sha256 --integrity hmac-sha256 -q -i1 >> >> It should produce Crypto API string >> authenc(hmac(sha256),essiv(cbc(aes),sha256)) >> while it produces >> essiv(authenc(hmac(sha256),cbc(aes)),sha256) >> (and fails). >> > > No. I don't know why it fails, but the latter is actually the correct > string. The essiv template is instantiated either as a skcipher or as > an aead, and it encapsulates the entire transformation. (This is > necessary considering that the IV is passed via the AAD and so the > ESSIV handling needs to touch that as well) Hm. Constructing these strings seems to be more confusing than dmcrypt mode combinations :-) But you are right, I actually tried the former string (authenc(hmac(sha256),essiv(cbc(aes),sha256))) and it worked, but I guess the authenticated IV (AAD) was actually the input to IV (plain sector number) not the output of ESSIV? Do I understand it correctly now? Milan
On Mon, 12 Aug 2019 at 10:44, Milan Broz <gmazyland@gmail.com> wrote: > > On 12/08/2019 08:54, Ard Biesheuvel wrote: > > On Mon, 12 Aug 2019 at 09:33, Milan Broz <gmazyland@gmail.com> wrote: > >> Try for example > >> # cryptsetup luksFormat /dev/sdc -c aes-cbc-essiv:sha256 --integrity hmac-sha256 -q -i1 > >> > >> It should produce Crypto API string > >> authenc(hmac(sha256),essiv(cbc(aes),sha256)) > >> while it produces > >> essiv(authenc(hmac(sha256),cbc(aes)),sha256) > >> (and fails). > >> > > > > No. I don't know why it fails, but the latter is actually the correct > > string. The essiv template is instantiated either as a skcipher or as > > an aead, and it encapsulates the entire transformation. (This is > > necessary considering that the IV is passed via the AAD and so the > > ESSIV handling needs to touch that as well) > > Hm. Constructing these strings seems to be more confusing than dmcrypt mode combinations :-) > > But you are right, I actually tried the former string (authenc(hmac(sha256),essiv(cbc(aes),sha256))) > and it worked, but I guess the authenticated IV (AAD) was actually the input to IV (plain sector number) > not the output of ESSIV? Do I understand it correctly now? > Indeed. The former string instantiates the skcipher version of the ESSIV template, and so the AAD handling is omitted, and we end up using the plain IV in the authentication rather than the encrypted IV. So when using the latter string, does it produce any error messages when it fails?
On 12/08/2019 09:50, Ard Biesheuvel wrote: > On Mon, 12 Aug 2019 at 10:44, Milan Broz <gmazyland@gmail.com> wrote: >> >> On 12/08/2019 08:54, Ard Biesheuvel wrote: >>> On Mon, 12 Aug 2019 at 09:33, Milan Broz <gmazyland@gmail.com> wrote: >>>> Try for example >>>> # cryptsetup luksFormat /dev/sdc -c aes-cbc-essiv:sha256 --integrity hmac-sha256 -q -i1 >>>> >>>> It should produce Crypto API string >>>> authenc(hmac(sha256),essiv(cbc(aes),sha256)) >>>> while it produces >>>> essiv(authenc(hmac(sha256),cbc(aes)),sha256) >>>> (and fails). >>>> >>> >>> No. I don't know why it fails, but the latter is actually the correct >>> string. The essiv template is instantiated either as a skcipher or as >>> an aead, and it encapsulates the entire transformation. (This is >>> necessary considering that the IV is passed via the AAD and so the >>> ESSIV handling needs to touch that as well) >> >> Hm. Constructing these strings seems to be more confusing than dmcrypt mode combinations :-) >> >> But you are right, I actually tried the former string (authenc(hmac(sha256),essiv(cbc(aes),sha256))) >> and it worked, but I guess the authenticated IV (AAD) was actually the input to IV (plain sector number) >> not the output of ESSIV? Do I understand it correctly now? >> > > Indeed. The former string instantiates the skcipher version of the > ESSIV template, and so the AAD handling is omitted, and we end up > using the plain IV in the authentication rather than the encrypted IV. > > So when using the latter string, does it produce any error messages > when it fails? The error is table: 253:1: crypt: Error decoding and setting key and it is failing in crypt_setkey() int this crypto_aead_setkey(); And it is because it now wrongly calculates MAC key length. (We have two keys here - one for length-preserving CBC-ESSIV encryption and one for HMAC.) This super-ugly hotfix helps here... I guess it can be done better :-) diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c index e9a0093c88ee..7b06d975a2e1 100644 --- a/drivers/md/dm-crypt.c +++ b/drivers/md/dm-crypt.c @@ -2342,6 +2342,9 @@ static int crypt_ctr_auth_cipher(struct crypt_config *cc, char *cipher_api) char *start, *end, *mac_alg = NULL; struct crypto_ahash *mac; + if (strstarts(cipher_api, "essiv(authenc(")) + cipher_api += strlen("essiv("); + if (!strstarts(cipher_api, "authenc(")) return 0; Milan
On Mon, 12 Aug 2019 at 16:51, Milan Broz <gmazyland@gmail.com> wrote: > > On 12/08/2019 09:50, Ard Biesheuvel wrote: > > On Mon, 12 Aug 2019 at 10:44, Milan Broz <gmazyland@gmail.com> wrote: > >> > >> On 12/08/2019 08:54, Ard Biesheuvel wrote: > >>> On Mon, 12 Aug 2019 at 09:33, Milan Broz <gmazyland@gmail.com> wrote: > >>>> Try for example > >>>> # cryptsetup luksFormat /dev/sdc -c aes-cbc-essiv:sha256 --integrity hmac-sha256 -q -i1 > >>>> > >>>> It should produce Crypto API string > >>>> authenc(hmac(sha256),essiv(cbc(aes),sha256)) > >>>> while it produces > >>>> essiv(authenc(hmac(sha256),cbc(aes)),sha256) > >>>> (and fails). > >>>> > >>> > >>> No. I don't know why it fails, but the latter is actually the correct > >>> string. The essiv template is instantiated either as a skcipher or as > >>> an aead, and it encapsulates the entire transformation. (This is > >>> necessary considering that the IV is passed via the AAD and so the > >>> ESSIV handling needs to touch that as well) > >> > >> Hm. Constructing these strings seems to be more confusing than dmcrypt mode combinations :-) > >> > >> But you are right, I actually tried the former string (authenc(hmac(sha256),essiv(cbc(aes),sha256))) > >> and it worked, but I guess the authenticated IV (AAD) was actually the input to IV (plain sector number) > >> not the output of ESSIV? Do I understand it correctly now? > >> > > > > Indeed. The former string instantiates the skcipher version of the > > ESSIV template, and so the AAD handling is omitted, and we end up > > using the plain IV in the authentication rather than the encrypted IV. > > > > So when using the latter string, does it produce any error messages > > when it fails? > > The error is > table: 253:1: crypt: Error decoding and setting key > > and it is failing in crypt_setkey() int this crypto_aead_setkey(); > > And it is because it now wrongly calculates MAC key length. > (We have two keys here - one for length-preserving CBC-ESSIV encryption > and one for HMAC.) > > This super-ugly hotfix helps here... I guess it can be done better :-) > Weird. It did work fine before, but now that I have dropped the 'md: dm-crypt: infer ESSIV block cipher from cipher string directly' patch, we are probably taking a different code path and hitting this error. I'll try to fix this cleanly. Thanks for doing the diagnosis. > diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c > index e9a0093c88ee..7b06d975a2e1 100644 > --- a/drivers/md/dm-crypt.c > +++ b/drivers/md/dm-crypt.c > @@ -2342,6 +2342,9 @@ static int crypt_ctr_auth_cipher(struct crypt_config *cc, char *cipher_api) > char *start, *end, *mac_alg = NULL; > struct crypto_ahash *mac; > > + if (strstarts(cipher_api, "essiv(authenc(")) > + cipher_api += strlen("essiv("); > + > if (!strstarts(cipher_api, "authenc(")) > return 0; > > Milan
diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig index 3834332f4963..b727e8f15264 100644 --- a/drivers/md/Kconfig +++ b/drivers/md/Kconfig @@ -271,6 +271,7 @@ config DM_CRYPT depends on BLK_DEV_DM select CRYPTO select CRYPTO_CBC + select CRYPTO_ESSIV ---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 48cd76c88d77..d3f2634f41a8 100644 --- a/drivers/md/dm-crypt.c +++ b/drivers/md/dm-crypt.c @@ -98,11 +98,6 @@ struct crypt_iv_operations { struct dm_crypt_request *dmreq); }; -struct iv_essiv_private { - struct crypto_shash *hash_tfm; - u8 *salt; -}; - struct iv_benbi_private { int shift; }; @@ -155,7 +150,6 @@ struct crypt_config { const struct crypt_iv_operations *iv_gen_ops; union { - struct iv_essiv_private essiv; struct iv_benbi_private benbi; struct iv_lmk_private lmk; struct iv_tcw_private tcw; @@ -165,8 +159,6 @@ struct crypt_config { unsigned short int sector_size; unsigned char sector_shift; - /* ESSIV: struct crypto_cipher *essiv_tfm */ - void *iv_private; union { struct crypto_skcipher **tfms; struct crypto_aead **tfms_aead; @@ -324,157 +316,15 @@ static int crypt_iv_plain64be_gen(struct crypt_config *cc, u8 *iv, return 0; } -/* Initialise ESSIV - compute salt but no local memory allocations */ -static int crypt_iv_essiv_init(struct crypt_config *cc) -{ - struct iv_essiv_private *essiv = &cc->iv_gen_private.essiv; - SHASH_DESC_ON_STACK(desc, essiv->hash_tfm); - struct crypto_cipher *essiv_tfm; - int err; - - desc->tfm = essiv->hash_tfm; - - err = crypto_shash_digest(desc, cc->key, cc->key_size, essiv->salt); - shash_desc_zero(desc); - if (err) - return err; - - essiv_tfm = cc->iv_private; - - err = crypto_cipher_setkey(essiv_tfm, essiv->salt, - crypto_shash_digestsize(essiv->hash_tfm)); - if (err) - return err; - - return 0; -} - -/* Wipe salt and reset key derived from volume key */ -static int crypt_iv_essiv_wipe(struct crypt_config *cc) -{ - struct iv_essiv_private *essiv = &cc->iv_gen_private.essiv; - unsigned salt_size = crypto_shash_digestsize(essiv->hash_tfm); - struct crypto_cipher *essiv_tfm; - int r, err = 0; - - memset(essiv->salt, 0, salt_size); - - essiv_tfm = cc->iv_private; - r = crypto_cipher_setkey(essiv_tfm, essiv->salt, salt_size); - if (r) - err = r; - - return err; -} - -/* Allocate the cipher for ESSIV */ -static struct crypto_cipher *alloc_essiv_cipher(struct crypt_config *cc, - struct dm_target *ti, - const u8 *salt, - unsigned int saltsize) -{ - struct crypto_cipher *essiv_tfm; - int err; - - /* Setup the essiv_tfm with the given salt */ - essiv_tfm = crypto_alloc_cipher(cc->cipher, 0, 0); - if (IS_ERR(essiv_tfm)) { - ti->error = "Error allocating crypto tfm for ESSIV"; - return essiv_tfm; - } - - if (crypto_cipher_blocksize(essiv_tfm) != cc->iv_size) { - ti->error = "Block size of ESSIV cipher does " - "not match IV size of block cipher"; - crypto_free_cipher(essiv_tfm); - return ERR_PTR(-EINVAL); - } - - err = crypto_cipher_setkey(essiv_tfm, salt, saltsize); - if (err) { - ti->error = "Failed to set key for ESSIV cipher"; - crypto_free_cipher(essiv_tfm); - return ERR_PTR(err); - } - - return essiv_tfm; -} - -static void crypt_iv_essiv_dtr(struct crypt_config *cc) -{ - struct crypto_cipher *essiv_tfm; - struct iv_essiv_private *essiv = &cc->iv_gen_private.essiv; - - crypto_free_shash(essiv->hash_tfm); - essiv->hash_tfm = NULL; - - kzfree(essiv->salt); - essiv->salt = NULL; - - essiv_tfm = cc->iv_private; - - if (essiv_tfm) - crypto_free_cipher(essiv_tfm); - - cc->iv_private = NULL; -} - -static int crypt_iv_essiv_ctr(struct crypt_config *cc, struct dm_target *ti, - const char *opts) -{ - struct crypto_cipher *essiv_tfm = NULL; - struct crypto_shash *hash_tfm = NULL; - u8 *salt = NULL; - int err; - - if (!opts) { - ti->error = "Digest algorithm missing for ESSIV mode"; - return -EINVAL; - } - - /* Allocate hash algorithm */ - hash_tfm = crypto_alloc_shash(opts, 0, 0); - if (IS_ERR(hash_tfm)) { - ti->error = "Error initializing ESSIV hash"; - err = PTR_ERR(hash_tfm); - goto bad; - } - - salt = kzalloc(crypto_shash_digestsize(hash_tfm), GFP_KERNEL); - if (!salt) { - ti->error = "Error kmallocing salt storage in ESSIV"; - err = -ENOMEM; - goto bad; - } - - cc->iv_gen_private.essiv.salt = salt; - cc->iv_gen_private.essiv.hash_tfm = hash_tfm; - - essiv_tfm = alloc_essiv_cipher(cc, ti, salt, - crypto_shash_digestsize(hash_tfm)); - if (IS_ERR(essiv_tfm)) { - crypt_iv_essiv_dtr(cc); - return PTR_ERR(essiv_tfm); - } - cc->iv_private = essiv_tfm; - - return 0; - -bad: - if (hash_tfm && !IS_ERR(hash_tfm)) - crypto_free_shash(hash_tfm); - kfree(salt); - return err; -} - static int crypt_iv_essiv_gen(struct crypt_config *cc, u8 *iv, struct dm_crypt_request *dmreq) { - struct crypto_cipher *essiv_tfm = cc->iv_private; - + /* + * ESSIV encryption of the IV is now handled by the crypto API, + * so just pass the plain sector number here. + */ memset(iv, 0, cc->iv_size); *(__le64 *)iv = cpu_to_le64(dmreq->iv_sector); - crypto_cipher_encrypt_one(essiv_tfm, iv, iv); return 0; } @@ -898,10 +748,6 @@ static const struct crypt_iv_operations crypt_iv_plain64be_ops = { }; static const struct crypt_iv_operations crypt_iv_essiv_ops = { - .ctr = crypt_iv_essiv_ctr, - .dtr = crypt_iv_essiv_dtr, - .init = crypt_iv_essiv_init, - .wipe = crypt_iv_essiv_wipe, .generator = crypt_iv_essiv_gen }; @@ -2464,7 +2310,7 @@ static int crypt_ctr_cipher_new(struct dm_target *ti, char *cipher_in, char *key char **ivmode, char **ivopts) { struct crypt_config *cc = ti->private; - char *tmp, *cipher_api; + char *tmp, *cipher_api, buf[CRYPTO_MAX_ALG_NAME]; int ret = -EINVAL; cc->tfms_count = 1; @@ -2493,6 +2339,20 @@ static int crypt_ctr_cipher_new(struct dm_target *ti, char *cipher_in, char *key if (*ivmode && !strcmp(*ivmode, "lmk")) cc->tfms_count = 64; + if (*ivmode && !strcmp(*ivmode, "essiv")) { + if (!*ivopts) { + ti->error = "Digest algorithm missing for ESSIV mode"; + return -EINVAL; + } + ret = snprintf(buf, CRYPTO_MAX_ALG_NAME, "essiv(%s,%s)", + cipher_api, *ivopts); + 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 */ @@ -2579,9 +2439,19 @@ static int crypt_ctr_cipher_old(struct dm_target *ti, char *cipher_in, char *key if (!cipher_api) goto bad_mem; - ret = snprintf(cipher_api, CRYPTO_MAX_ALG_NAME, - "%s(%s)", chainmode, cipher); - if (ret < 0) { + if (*ivmode && !strcmp(*ivmode, "essiv")) { + if (!*ivopts) { + ti->error = "Digest algorithm missing for ESSIV mode"; + kfree(cipher_api); + return -EINVAL; + } + ret = snprintf(cipher_api, CRYPTO_MAX_ALG_NAME, + "essiv(%s(%s),%s)", chainmode, cipher, *ivopts); + } else { + ret = snprintf(cipher_api, CRYPTO_MAX_ALG_NAME, + "%s(%s)", chainmode, cipher); + } + if (ret < 0 || ret >= CRYPTO_MAX_ALG_NAME) { kfree(cipher_api); goto bad_mem; }
Replace the explicit ESSIV 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: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- drivers/md/Kconfig | 1 + drivers/md/dm-crypt.c | 194 ++++---------------- 2 files changed, 33 insertions(+), 162 deletions(-)