diff mbox series

[v6,4/7] md: dm-crypt: switch to ESSIV crypto API template

Message ID 20190628152112.914-5-ard.biesheuvel@linaro.org
State Superseded
Headers show
Series crypto: switch to crypto API for ESSIV generation | expand

Commit Message

Ard Biesheuvel June 28, 2019, 3:21 p.m. UTC
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 | 200 ++++----------------
 2 files changed, 36 insertions(+), 165 deletions(-)

-- 
2.20.1

Comments

Milan Broz July 1, 2019, 8:59 a.m. UTC | #1
On 28/06/2019 17:21, 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/dm-crypt.c | 200 ++++----------------


...

> -/* Wipe salt and reset key derived from volume key */

> -static int crypt_iv_essiv_wipe(struct crypt_config *cc)


Do I understand it correctly, that this is now called inside the whole cipher
set key in wipe command (in crypt_wipe_key())?

(Wipe message is meant to suspend the device and wipe all key material
from memory without actually destroying the device.)

> -{

> -	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;

> -}


...

> @@ -2435,9 +2281,19 @@ static int crypt_ctr_cipher_new(struct dm_target *ti, char *cipher_in, char *key

>  	}

>  

>  	ret = crypt_ctr_blkdev_cipher(cc, cipher_api);

> -	if (ret < 0) {

> -		ti->error = "Cannot allocate cipher string";

> -		return -ENOMEM;

> +	if (ret < 0)

> +		goto bad_mem;

> +

> +	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,%s)",

> +			       cipher_api, cc->cipher, *ivopts);

> +		if (ret < 0 || ret >= CRYPTO_MAX_ALG_NAME)

> +			goto bad_mem;


Hm, nitpicking, but goto from only one place while we have another -ENOMEM above...

Just place this here without goto?

> +	ti->error = "Cannot allocate cipher string";

> +	return -ENOMEM;


Otherwise

Reviewed-by: Milan Broz <gmazyland@gmail.com>


Thanks,
Milan
Ard Biesheuvel July 2, 2019, 8:07 a.m. UTC | #2
On Mon, 1 Jul 2019 at 10:59, Milan Broz <gmazyland@gmail.com> wrote:
>

> On 28/06/2019 17:21, 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/dm-crypt.c | 200 ++++----------------

>

> ...

>

> > -/* Wipe salt and reset key derived from volume key */

> > -static int crypt_iv_essiv_wipe(struct crypt_config *cc)

>

> Do I understand it correctly, that this is now called inside the whole cipher

> set key in wipe command (in crypt_wipe_key())?

>

> (Wipe message is meant to suspend the device and wipe all key material

> from memory without actually destroying the device.)

>


Yes, setting the random key in wipe() triggers the SHA256 operation as
normal, which is slightly wasteful but not a big deal imo.

> > -{

> > -     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;

> > -}

>

> ...

>

> > @@ -2435,9 +2281,19 @@ static int crypt_ctr_cipher_new(struct dm_target *ti, char *cipher_in, char *key

> >       }

> >

> >       ret = crypt_ctr_blkdev_cipher(cc, cipher_api);

> > -     if (ret < 0) {

> > -             ti->error = "Cannot allocate cipher string";

> > -             return -ENOMEM;

> > +     if (ret < 0)

> > +             goto bad_mem;

> > +

> > +     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,%s)",

> > +                            cipher_api, cc->cipher, *ivopts);

> > +             if (ret < 0 || ret >= CRYPTO_MAX_ALG_NAME)

> > +                     goto bad_mem;

>

> Hm, nitpicking, but goto from only one place while we have another -ENOMEM above...

>

> Just place this here without goto?

>


OK

> > +     ti->error = "Cannot allocate cipher string";

> > +     return -ENOMEM;

>

> Otherwise

>

> Reviewed-by: Milan Broz <gmazyland@gmail.com>

>

> Thanks,

> Milan
Ard Biesheuvel July 2, 2019, 4:30 p.m. UTC | #3
On Mon, 1 Jul 2019 at 10:59, Milan Broz <gmazyland@gmail.com> wrote:
>

> On 28/06/2019 17:21, 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/dm-crypt.c | 200 ++++----------------

>

> ...

>

> > -/* Wipe salt and reset key derived from volume key */

> > -static int crypt_iv_essiv_wipe(struct crypt_config *cc)

>

> Do I understand it correctly, that this is now called inside the whole cipher

> set key in wipe command (in crypt_wipe_key())?

>

> (Wipe message is meant to suspend the device and wipe all key material

> from memory without actually destroying the device.)

>

> > -{

> > -     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;

> > -}

>

> ...

>

> > @@ -2435,9 +2281,19 @@ static int crypt_ctr_cipher_new(struct dm_target *ti, char *cipher_in, char *key

> >       }

> >

> >       ret = crypt_ctr_blkdev_cipher(cc, cipher_api);

> > -     if (ret < 0) {

> > -             ti->error = "Cannot allocate cipher string";

> > -             return -ENOMEM;

> > +     if (ret < 0)

> > +             goto bad_mem;

> > +

> > +     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,%s)",

> > +                            cipher_api, cc->cipher, *ivopts);

> > +             if (ret < 0 || ret >= CRYPTO_MAX_ALG_NAME)

> > +                     goto bad_mem;

>

> Hm, nitpicking, but goto from only one place while we have another -ENOMEM above...

>

> Just place this here without goto?

>


Actually, the bad_mem label is used 10 lines up as well.
So I'll keep this goto in the next revision.

> > +     ti->error = "Cannot allocate cipher string";

> > +     return -ENOMEM;

>

> Otherwise

>

> Reviewed-by: Milan Broz <gmazyland@gmail.com>

>

> Thanks,

> Milan
diff mbox series

Patch

diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index 45254b3ef715..30ca87cf25db 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 f001f1104cb5..e26a09c2d1a6 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;
@@ -323,157 +315,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;
 }
@@ -854,10 +704,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
 };
 
@@ -2397,7 +2243,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;
@@ -2435,9 +2281,19 @@  static int crypt_ctr_cipher_new(struct dm_target *ti, char *cipher_in, char *key
 	}
 
 	ret = crypt_ctr_blkdev_cipher(cc, cipher_api);
-	if (ret < 0) {
-		ti->error = "Cannot allocate cipher string";
-		return -ENOMEM;
+	if (ret < 0)
+		goto bad_mem;
+
+	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,%s)",
+			       cipher_api, cc->cipher, *ivopts);
+		if (ret < 0 || ret >= CRYPTO_MAX_ALG_NAME)
+			goto bad_mem;
+		cipher_api = buf;
 	}
 
 	cc->key_parts = cc->tfms_count;
@@ -2456,6 +2312,9 @@  static int crypt_ctr_cipher_new(struct dm_target *ti, char *cipher_in, char *key
 		cc->iv_size = crypto_skcipher_ivsize(any_tfm(cc));
 
 	return 0;
+bad_mem:
+	ti->error = "Cannot allocate cipher string";
+	return -ENOMEM;
 }
 
 static int crypt_ctr_cipher_old(struct dm_target *ti, char *cipher_in, char *key,
@@ -2515,9 +2374,20 @@  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,%s)", chainmode, cipher,
+			       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;
 	}