diff mbox series

[1/4] crypto: introduce CRYPTO_ALG_ALLOCATES_MEMORY

Message ID alpine.LRH.2.02.2006161101080.28052@file01.intranet.prod.int.rdu2.redhat.com (mailing list archive)
State Not Applicable, archived
Delegated to: Mike Snitzer
Headers show
Series [1/4] crypto: introduce CRYPTO_ALG_ALLOCATES_MEMORY | expand

Commit Message

Mikulas Patocka June 16, 2020, 3:01 p.m. UTC 
Introduce a new flag CRYPTO_ALG_ALLOCATES_MEMORY and modify dm-crypt, so
that it uses only drivers without this flag.

If the flag is set, then the crypto driver allocates memory in its request
routine. Such drivers are not suitable for disk encryption because
GFP_ATOMIC allocation can fail anytime (causing random I/O errors) and
GFP_KERNEL allocation can recurse into the block layer, causing a
deadlock.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Comments

Eric Biggers June 16, 2020, 5:36 p.m. UTC | #1 
On Tue, Jun 16, 2020 at 11:01:31AM -0400, Mikulas Patocka wrote:
> Introduce a new flag CRYPTO_ALG_ALLOCATES_MEMORY and modify dm-crypt, so
> that it uses only drivers without this flag.
> 
> If the flag is set, then the crypto driver allocates memory in its request
> routine. Such drivers are not suitable for disk encryption because
> GFP_ATOMIC allocation can fail anytime (causing random I/O errors) and
> GFP_KERNEL allocation can recurse into the block layer, causing a
> deadlock.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> Index: linux-2.6/include/linux/crypto.h
> ===================================================================
> --- linux-2.6.orig/include/linux/crypto.h
> +++ linux-2.6/include/linux/crypto.h
> @@ -97,9 +97,15 @@
>  #define CRYPTO_ALG_OPTIONAL_KEY		0x00004000
>  
>  /*
> + * The driver is allocating emmory in its encrypt or decrypt callback,
> + * so it should not be used to encrypt block devices.
> + */

"is allocating emmory" => "may allocate memory"

"so it should not be used to encrypt block devices" =>
"so it shouldn't be used in cases where memory allocation failures aren't
 acceptable, such as during block device encryption".

Also, which types of algorithms does this flag apply to?  E.g. if it applies to
hash algorithms too, it's not sufficient to say "encrypt or decrypt callback".

How about:

 /*
  * The driver may allocate memory during request processing, so it shouldn't be
  * used in cases where memory allocation failures aren't acceptable, such as
  * during block device encryption.
  */

> +#define CRYPTO_ALG_ALLOCATES_MEMORY	0x00008000
> +
> +/*
>   * Don't trigger module loading
>   */
> -#define CRYPTO_NOLOAD			0x00008000
> +#define CRYPTO_NOLOAD			0x00010000
>  
>  /*
>   * Transform masks and values (for crt_flags).
> Index: linux-2.6/drivers/md/dm-crypt.c
> ===================================================================

This would better belong as two separate patches: one to introduce
CRYPTO_ALG_ALLOCATES_MEMORY, and one to make dm-crypt use it.

> --- linux-2.6.orig/drivers/md/dm-crypt.c
> +++ linux-2.6/drivers/md/dm-crypt.c
> @@ -419,7 +419,7 @@ static int crypt_iv_lmk_ctr(struct crypt
>  		return -EINVAL;
>  	}
>  
> -	lmk->hash_tfm = crypto_alloc_shash("md5", 0, 0);
> +	lmk->hash_tfm = crypto_alloc_shash("md5", 0, CRYPTO_ALG_ALLOCATES_MEMORY);
>  	if (IS_ERR(lmk->hash_tfm)) {
>  		ti->error = "Error initializing LMK hash";
>  		return PTR_ERR(lmk->hash_tfm);
> @@ -581,7 +581,7 @@ static int crypt_iv_tcw_ctr(struct crypt
>  		return -EINVAL;
>  	}
>  
> -	tcw->crc32_tfm = crypto_alloc_shash("crc32", 0, 0);
> +	tcw->crc32_tfm = crypto_alloc_shash("crc32", 0, CRYPTO_ALG_ALLOCATES_MEMORY);
>  	if (IS_ERR(tcw->crc32_tfm)) {
>  		ti->error = "Error initializing CRC32 in TCW";
>  		return PTR_ERR(tcw->crc32_tfm);
> @@ -768,7 +768,7 @@ static int crypt_iv_elephant_ctr(struct
>  	struct iv_elephant_private *elephant = &cc->iv_gen_private.elephant;
>  	int r;
>  
> -	elephant->tfm = crypto_alloc_skcipher("ecb(aes)", 0, 0);
> +	elephant->tfm = crypto_alloc_skcipher("ecb(aes)", 0, CRYPTO_ALG_ALLOCATES_MEMORY);
>  	if (IS_ERR(elephant->tfm)) {
>  		r = PTR_ERR(elephant->tfm);
>  		elephant->tfm = NULL;
> @@ -2088,7 +2088,7 @@ static int crypt_alloc_tfms_skcipher(str
>  		return -ENOMEM;
>  
>  	for (i = 0; i < cc->tfms_count; i++) {
> -		cc->cipher_tfm.tfms[i] = crypto_alloc_skcipher(ciphermode, 0, 0);
> +		cc->cipher_tfm.tfms[i] = crypto_alloc_skcipher(ciphermode, 0, CRYPTO_ALG_ALLOCATES_MEMORY);

Despite the recent relaxation in rules, the preferred length of a line is still
80 columns.

- Eric

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Mikulas Patocka June 17, 2020, 3:08 p.m. UTC | #2 
I'm resending the patches with your comments resolved...

Mikulas



On Tue, 16 Jun 2020, Eric Biggers wrote:

> On Tue, Jun 16, 2020 at 11:01:31AM -0400, Mikulas Patocka wrote:
> > Introduce a new flag CRYPTO_ALG_ALLOCATES_MEMORY and modify dm-crypt, so
> > that it uses only drivers without this flag.
> > 
> > If the flag is set, then the crypto driver allocates memory in its request
> > routine. Such drivers are not suitable for disk encryption because
> > GFP_ATOMIC allocation can fail anytime (causing random I/O errors) and
> > GFP_KERNEL allocation can recurse into the block layer, causing a
> > deadlock.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > Index: linux-2.6/include/linux/crypto.h
> > ===================================================================
> > --- linux-2.6.orig/include/linux/crypto.h
> > +++ linux-2.6/include/linux/crypto.h
> > @@ -97,9 +97,15 @@
> >  #define CRYPTO_ALG_OPTIONAL_KEY		0x00004000
> >  
> >  /*
> > + * The driver is allocating emmory in its encrypt or decrypt callback,
> > + * so it should not be used to encrypt block devices.
> > + */
> 
> "is allocating emmory" => "may allocate memory"
> 
> "so it should not be used to encrypt block devices" =>
> "so it shouldn't be used in cases where memory allocation failures aren't
>  acceptable, such as during block device encryption".
> 
> Also, which types of algorithms does this flag apply to?  E.g. if it applies to
> hash algorithms too, it's not sufficient to say "encrypt or decrypt callback".
> 
> How about:
> 
>  /*
>   * The driver may allocate memory during request processing, so it shouldn't be
>   * used in cases where memory allocation failures aren't acceptable, such as
>   * during block device encryption.
>   */
> 
> > +#define CRYPTO_ALG_ALLOCATES_MEMORY	0x00008000
> > +
> > +/*
> >   * Don't trigger module loading
> >   */
> > -#define CRYPTO_NOLOAD			0x00008000
> > +#define CRYPTO_NOLOAD			0x00010000
> >  
> >  /*
> >   * Transform masks and values (for crt_flags).
> > Index: linux-2.6/drivers/md/dm-crypt.c
> > ===================================================================
> 
> This would better belong as two separate patches: one to introduce
> CRYPTO_ALG_ALLOCATES_MEMORY, and one to make dm-crypt use it.
> 
> > --- linux-2.6.orig/drivers/md/dm-crypt.c
> > +++ linux-2.6/drivers/md/dm-crypt.c
> > @@ -419,7 +419,7 @@ static int crypt_iv_lmk_ctr(struct crypt
> >  		return -EINVAL;
> >  	}
> >  
> > -	lmk->hash_tfm = crypto_alloc_shash("md5", 0, 0);
> > +	lmk->hash_tfm = crypto_alloc_shash("md5", 0, CRYPTO_ALG_ALLOCATES_MEMORY);
> >  	if (IS_ERR(lmk->hash_tfm)) {
> >  		ti->error = "Error initializing LMK hash";
> >  		return PTR_ERR(lmk->hash_tfm);
> > @@ -581,7 +581,7 @@ static int crypt_iv_tcw_ctr(struct crypt
> >  		return -EINVAL;
> >  	}
> >  
> > -	tcw->crc32_tfm = crypto_alloc_shash("crc32", 0, 0);
> > +	tcw->crc32_tfm = crypto_alloc_shash("crc32", 0, CRYPTO_ALG_ALLOCATES_MEMORY);
> >  	if (IS_ERR(tcw->crc32_tfm)) {
> >  		ti->error = "Error initializing CRC32 in TCW";
> >  		return PTR_ERR(tcw->crc32_tfm);
> > @@ -768,7 +768,7 @@ static int crypt_iv_elephant_ctr(struct
> >  	struct iv_elephant_private *elephant = &cc->iv_gen_private.elephant;
> >  	int r;
> >  
> > -	elephant->tfm = crypto_alloc_skcipher("ecb(aes)", 0, 0);
> > +	elephant->tfm = crypto_alloc_skcipher("ecb(aes)", 0, CRYPTO_ALG_ALLOCATES_MEMORY);
> >  	if (IS_ERR(elephant->tfm)) {
> >  		r = PTR_ERR(elephant->tfm);
> >  		elephant->tfm = NULL;
> > @@ -2088,7 +2088,7 @@ static int crypt_alloc_tfms_skcipher(str
> >  		return -ENOMEM;
> >  
> >  	for (i = 0; i < cc->tfms_count; i++) {
> > -		cc->cipher_tfm.tfms[i] = crypto_alloc_skcipher(ciphermode, 0, 0);
> > +		cc->cipher_tfm.tfms[i] = crypto_alloc_skcipher(ciphermode, 0, CRYPTO_ALG_ALLOCATES_MEMORY);
> 
> Despite the recent relaxation in rules, the preferred length of a line is still
> 80 columns.
> 
> - Eric
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
diff mbox series

Patch

Index: linux-2.6/include/linux/crypto.h
===================================================================
--- linux-2.6.orig/include/linux/crypto.h
+++ linux-2.6/include/linux/crypto.h
@@ -97,9 +97,15 @@ 
 #define CRYPTO_ALG_OPTIONAL_KEY		0x00004000
 
 /*
+ * The driver is allocating emmory in its encrypt or decrypt callback,
+ * so it should not be used to encrypt block devices.
+ */
+#define CRYPTO_ALG_ALLOCATES_MEMORY	0x00008000
+
+/*
  * Don't trigger module loading
  */
-#define CRYPTO_NOLOAD			0x00008000
+#define CRYPTO_NOLOAD			0x00010000
 
 /*
  * Transform masks and values (for crt_flags).
Index: linux-2.6/drivers/md/dm-crypt.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-crypt.c
+++ linux-2.6/drivers/md/dm-crypt.c
@@ -419,7 +419,7 @@  static int crypt_iv_lmk_ctr(struct crypt
 		return -EINVAL;
 	}
 
-	lmk->hash_tfm = crypto_alloc_shash("md5", 0, 0);
+	lmk->hash_tfm = crypto_alloc_shash("md5", 0, CRYPTO_ALG_ALLOCATES_MEMORY);
 	if (IS_ERR(lmk->hash_tfm)) {
 		ti->error = "Error initializing LMK hash";
 		return PTR_ERR(lmk->hash_tfm);
@@ -581,7 +581,7 @@  static int crypt_iv_tcw_ctr(struct crypt
 		return -EINVAL;
 	}
 
-	tcw->crc32_tfm = crypto_alloc_shash("crc32", 0, 0);
+	tcw->crc32_tfm = crypto_alloc_shash("crc32", 0, CRYPTO_ALG_ALLOCATES_MEMORY);
 	if (IS_ERR(tcw->crc32_tfm)) {
 		ti->error = "Error initializing CRC32 in TCW";
 		return PTR_ERR(tcw->crc32_tfm);
@@ -768,7 +768,7 @@  static int crypt_iv_elephant_ctr(struct
 	struct iv_elephant_private *elephant = &cc->iv_gen_private.elephant;
 	int r;
 
-	elephant->tfm = crypto_alloc_skcipher("ecb(aes)", 0, 0);
+	elephant->tfm = crypto_alloc_skcipher("ecb(aes)", 0, CRYPTO_ALG_ALLOCATES_MEMORY);
 	if (IS_ERR(elephant->tfm)) {
 		r = PTR_ERR(elephant->tfm);
 		elephant->tfm = NULL;
@@ -2088,7 +2088,7 @@  static int crypt_alloc_tfms_skcipher(str
 		return -ENOMEM;
 
 	for (i = 0; i < cc->tfms_count; i++) {
-		cc->cipher_tfm.tfms[i] = crypto_alloc_skcipher(ciphermode, 0, 0);
+		cc->cipher_tfm.tfms[i] = crypto_alloc_skcipher(ciphermode, 0, CRYPTO_ALG_ALLOCATES_MEMORY);
 		if (IS_ERR(cc->cipher_tfm.tfms[i])) {
 			err = PTR_ERR(cc->cipher_tfm.tfms[i]);
 			crypt_free_tfms(cc);
@@ -2114,7 +2114,7 @@  static int crypt_alloc_tfms_aead(struct
 	if (!cc->cipher_tfm.tfms)
 		return -ENOMEM;
 
-	cc->cipher_tfm.tfms_aead[0] = crypto_alloc_aead(ciphermode, 0, 0);
+	cc->cipher_tfm.tfms_aead[0] = crypto_alloc_aead(ciphermode, 0, CRYPTO_ALG_ALLOCATES_MEMORY);
 	if (IS_ERR(cc->cipher_tfm.tfms_aead[0])) {
 		err = PTR_ERR(cc->cipher_tfm.tfms_aead[0]);
 		crypt_free_tfms(cc);
@@ -2565,7 +2565,7 @@  static int crypt_ctr_auth_cipher(struct
 		return -ENOMEM;
 	strncpy(mac_alg, start, end - start);
 
-	mac = crypto_alloc_ahash(mac_alg, 0, 0);
+	mac = crypto_alloc_ahash(mac_alg, 0, CRYPTO_ALG_ALLOCATES_MEMORY);
 	kfree(mac_alg);
 
 	if (IS_ERR(mac))