crypto: cmac - clone fixes

PATCH In review Dmitry Safonov 3 participants 11 messages Jun 14, 2023 – Jun 23, 2023 Crypto API

Share & tools

Dmitry Safonov

author

2023-06-14 17:47 UTC #0 lore raw

cover crypto: cmac - clone fixes

Make cipher cloning possible in atomic contexts + prevent use-after-free on the crypto algorithm.
Those seems to be all pitfalls I found while adapting TCP-AO patches to use crypto clone-tfm and dropping per-CPU requests allocations.

Cc Bob Gilligan <[email protected]>

Cc David Ahern <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Dmitry Safonov <[email protected]>

Cc Eric Dumazet <[email protected]>

Cc "Eric W. Biederman" <[email protected]>

Cc Francesco Ruggeri <[email protected]>

Cc Jakub Kicinski <[email protected]>

Cc Herbert Xu <[email protected]>

Cc Paolo Abeni <[email protected]>

Cc Salam Noureddine <[email protected]>

Cc [email protected]

Thanks,
Dmitry

Dmitry Safonov (3):
crypto: api - Remove crypto_init_ops()
crypto: api - Provide gfp mask for tfm allocation
crypto: cipher - On clone do crypto_mod_get()

 crypto/algapi.c         |  2 +-
 crypto/api.c            | 20 +++-----------------
 crypto/cipher.c         |  9 +++++++--
 crypto/internal.h       |  2 +-
 include/crypto/algapi.h |  1 -
 5 files changed, 12 insertions(+), 22 deletions(-)

base-commit: b16049b21162bb649cdd8519642a35972b7910fe

Signature · 1 line

2.40.0

Dmitry Safonov

author

2023-06-14 17:47 UTC #1 lore raw

PATCH 1/3 crypto: api - Remove crypto_init_ops()

Purge crypto_type::init() as well.
The last user seems to be gone with commit d63007eb954e ("crypto:
ablkcipher - remove deprecated and unused ablkcipher support").

Signed-off-by Dmitry Safonov <[email protected]>

---
 crypto/api.c            | 14 --------------
 include/crypto/algapi.h |  1 -
 2 files changed, 15 deletions(-)

crypto/api.c +0 ~~-14~~

index d375e8cd770d..a94bd0695719 100644
@@ -345,15 +345,6 @@ struct crypto_alg *crypto_alg_mod_lookup(const char *name, u32 type, u32 mask)
 }
 EXPORT_SYMBOL_GPL(crypto_alg_mod_lookup);
 
-static int crypto_init_ops(struct crypto_tfm *tfm, u32 type, u32 mask)
-{
-	const struct crypto_type *type_obj = tfm->__crt_alg->cra_type;
-
-	if (type_obj)
-		return type_obj->init(tfm, type, mask);
-	return 0;
-}
-
 static void crypto_exit_ops(struct crypto_tfm *tfm)
 {
 	const struct crypto_type *type = tfm->__crt_alg->cra_type;

@@ -410,10 +401,6 @@ struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type,
 	tfm->__crt_alg = alg;
 	refcount_set(&tfm->refcnt, 1);
 
-	err = crypto_init_ops(tfm, type, mask);
-	if (err)
-		goto out_free_tfm;
-
 	if (!tfm->exit && alg->cra_init && (err = alg->cra_init(tfm)))
 		goto cra_init_failed;

@@ -421,7 +408,6 @@ struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type,
 
 cra_init_failed:
 	crypto_exit_ops(tfm);
-out_free_tfm:
 	if (err == -EAGAIN)
 		crypto_shoot_alg(alg);
 	kfree(tfm);

include/crypto/algapi.h +0 -1

index 016d5a302b84..6156161b181f 100644
@@ -56,7 +56,6 @@ struct sk_buff;
 struct crypto_type {
 	unsigned int (*ctxsize)(struct crypto_alg *alg, u32 type, u32 mask);
 	unsigned int (*extsize)(struct crypto_alg *alg);
-	int (*init)(struct crypto_tfm *tfm, u32 type, u32 mask);
 	int (*init_tfm)(struct crypto_tfm *tfm);
 	void (*show)(struct seq_file *m, struct crypto_alg *alg);
 	int (*report)(struct sk_buff *skb, struct crypto_alg *alg);

Signature · 1 line

2.40.0

Eric Biggers

Reviewed-by on v1 1/3

2023-06-15 23:32 UTC #2 lore raw

Re: [PATCH-next 1/3] crypto: api - Remove crypto_init_ops()

Quoted 10 lines · Dmitry Safonov

Purge crypto_type::init() as well.
The last user seems to be gone with commit d63007eb954e ("crypto:
ablkcipher - remove deprecated and unused ablkcipher support").
Signed-off-by Dmitry Safonov <[email protected]>
---
 crypto/api.c            | 14 --------------
 include/crypto/algapi.h |  1 -
 2 files changed, 15 deletions(-)

Reviewed-by Eric Biggers <[email protected]>

- Eric

Dmitry Safonov

author

2023-06-14 17:47 UTC #3 lore raw

PATCH 2/3 crypto: api - Provide gfp mask for tfm allocation

In reply to Dmitry Safonov

Use it straight away in crypto_clone_cipher(), as that is not meant to sleep.

Signed-off-by Dmitry Safonov <[email protected]>

---
 crypto/algapi.c   | 2 +-
 crypto/api.c      | 6 +++---
 crypto/cipher.c   | 2 +-
 crypto/internal.h | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

crypto/algapi.c +1 -1

index 5e7cd603d489..8d7d9cc008ff 100644
@@ -798,7 +798,7 @@ struct crypto_tfm *crypto_spawn_tfm(struct crypto_spawn *spawn, u32 type,
 	if (unlikely((alg->cra_flags ^ type) & mask))
 		goto out_put_alg;
 
-	tfm = __crypto_alloc_tfm(alg, type, mask);
+	tfm = __crypto_alloc_tfm(alg, type, mask, GFP_KERNEL);
 	if (IS_ERR(tfm))
 		goto out_put_alg;

crypto/api.c +3 -3

index a94bd0695719..54bf7c71b482 100644
@@ -387,14 +387,14 @@ void crypto_shoot_alg(struct crypto_alg *alg)
 EXPORT_SYMBOL_GPL(crypto_shoot_alg);
 
 struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type,
-				      u32 mask)
+				      u32 mask, gfp_t gfp)
 {
 	struct crypto_tfm *tfm = NULL;
 	unsigned int tfm_size;
 	int err = -ENOMEM;
 
 	tfm_size = sizeof(*tfm) + crypto_ctxsize(alg, type, mask);
-	tfm = kzalloc(tfm_size, GFP_KERNEL);
+	tfm = kzalloc(tfm_size, gfp);
 	if (tfm == NULL)
 		goto out_err;

@@ -454,7 +454,7 @@ struct crypto_tfm *crypto_alloc_base(const char *alg_name, u32 type, u32 mask)
 			goto err;
 		}
 
-		tfm = __crypto_alloc_tfm(alg, type, mask);
+		tfm = __crypto_alloc_tfm(alg, type, mask, GFP_KERNEL);
 		if (!IS_ERR(tfm))
 			return tfm;

crypto/cipher.c +1 -1

index d39ef5f72ab8..184188339a4a 100644
@@ -102,7 +102,7 @@ struct crypto_cipher *crypto_clone_cipher(struct crypto_cipher *cipher)
 		return ERR_PTR(-ENOSYS);
 
 	ntfm = __crypto_alloc_tfm(alg, CRYPTO_ALG_TYPE_CIPHER,
-				  CRYPTO_ALG_TYPE_MASK);
+				  CRYPTO_ALG_TYPE_MASK, GFP_ATOMIC);
 	if (IS_ERR(ntfm))
 		return ERR_CAST(ntfm);

crypto/internal.h +1 -1

index 8dd746b1130b..eba723a57689 100644
@@ -103,7 +103,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 void crypto_remove_final(struct list_head *list);
 void crypto_shoot_alg(struct crypto_alg *alg);
 struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type,
-				      u32 mask);
+				      u32 mask, gfp_t gfp);
 void *crypto_create_tfm_node(struct crypto_alg *alg,
 			const struct crypto_type *frontend, int node);
 void *crypto_clone_tfm(const struct crypto_type *frontend,

Signature · 1 line

2.40.0

Herbert Xu

maintainer

2023-06-15 09:01 UTC #4 lore raw

PATCH v2 1/1 crypto: api - Add __crypto_alloc_tfmgfp

Quoted 10 lines · Dmitry Safonov

Use it straight away in crypto_clone_cipher(), as that is not meant to sleep.
Signed-off-by Dmitry Safonov <[email protected]>
---
 crypto/algapi.c   | 2 +-
 crypto/api.c      | 6 +++---
 crypto/cipher.c   | 2 +-
 crypto/internal.h | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

Good catch. Though I'd rather add the gfp argument to a separate function because I'm in the process of replacing ciphers with something that uses the new crypto_types API.

Once that happens ciphers will switch over to the normal cloning call and this can be removed.

---8<---
Use it straight away in crypto_clone_cipher(), as that is not meant to sleep.

Fixes 51d8d6d0f4be ("crypto: cipher - Add crypto_clone_cipher")

Signed-off-by Dmitry Safonov <[email protected]>

Signed-off-by Herbert Xu <[email protected]>

crypto/api.c +10 -3

index d375e8cd770d..9007b33e1108 100644
@@ -395,15 +395,15 @@ void crypto_shoot_alg(struct crypto_alg *alg)
 }
 EXPORT_SYMBOL_GPL(crypto_shoot_alg);
 
-struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type,
-				      u32 mask)
+struct crypto_tfm *__crypto_alloc_tfmgfp(struct crypto_alg *alg, u32 type,
+					 u32 mask, gfp_t gfp)
 {
 	struct crypto_tfm *tfm = NULL;
 	unsigned int tfm_size;
 	int err = -ENOMEM;
 
 	tfm_size = sizeof(*tfm) + crypto_ctxsize(alg, type, mask);
-	tfm = kzalloc(tfm_size, GFP_KERNEL);
+	tfm = kzalloc(tfm_size, gfp);
 	if (tfm == NULL)
 		goto out_err;

@@ -430,6 +430,13 @@ struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type,
 out:
 	return tfm;
 }
+EXPORT_SYMBOL_GPL(__crypto_alloc_tfmgfp);
+
+struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type,
+				      u32 mask)
+{
+	return __crypto_alloc_tfmgfp(alg, type, mask, GFP_KERNEL);
+}
 EXPORT_SYMBOL_GPL(__crypto_alloc_tfm);
 
 /*

crypto/cipher.c +2 -2

index d39ef5f72ab8..a5a88038f0d6 100644
@@ -101,8 +101,8 @@ struct crypto_cipher *crypto_clone_cipher(struct crypto_cipher *cipher)
 	if (alg->cra_init)
 		return ERR_PTR(-ENOSYS);
 
-	ntfm = __crypto_alloc_tfm(alg, CRYPTO_ALG_TYPE_CIPHER,
-				  CRYPTO_ALG_TYPE_MASK);
+	ntfm = __crypto_alloc_tfmgfp(alg, CRYPTO_ALG_TYPE_CIPHER,
+				     CRYPTO_ALG_TYPE_MASK, GFP_ATOMIC);
 	if (IS_ERR(ntfm))
 		return ERR_CAST(ntfm);

crypto/internal.h +2 -0

index 024c2c795f59..12c50b7e7d87 100644
@@ -118,6 +118,8 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 			  struct crypto_alg *nalg);
 void crypto_remove_final(struct list_head *list);
 void crypto_shoot_alg(struct crypto_alg *alg);
+struct crypto_tfm *__crypto_alloc_tfmgfp(struct crypto_alg *alg, u32 type,
+					 u32 mask, gfp_t gfp);
 struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type,
 				      u32 mask);
 void *crypto_create_tfm_node(struct crypto_alg *alg,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Dmitry Safonov

author

2023-06-15 16:20 UTC #5 lore raw

Re: [v2 PATCH] crypto: api - Add __crypto_alloc_tfmgfp

On 6/15/23 10:00, Herbert Xu wrote:
[..]

Quoted 7 lines

Good catch. Though I'd rather add the gfp argument to a separate function because I'm in the process of replacing ciphers with something that uses the new crypto_types API.
Once that happens ciphers will switch over to the normal cloning call and this can be removed.

LGTM, thanks!

crypto/api.c

Quoted 10 lines

---8<---
Use it straight away in crypto_clone_cipher(), as that is not meant to sleep.
Fixes 51d8d6d0f4be ("crypto: cipher - Add crypto_clone_cipher")
Signed-off-by Dmitry Safonov <[email protected]>
Signed-off-by Herbert Xu <[email protected]>
crypto/api.c +0 -0

[..]

Thanks,
Dmitry

Dmitry Safonov

author

2023-06-20 16:59 UTC #6 lore raw

Re: [v2 PATCH] crypto: api - Add __crypto_alloc_tfmgfp

Hi Herbert,

Quoted 11 lines · Dmitry Safonov

On 6/15/23 10:00, Herbert Xu wrote:
[..]
Quoted 7 lines
Good catch. Though I'd rather add the gfp argument to a separate function because I'm in the process of replacing ciphers with something that uses the new crypto_types API.
Once that happens ciphers will switch over to the normal cloning call and this can be removed.
LGTM, thanks!

Would you prefer me to resend this v2 or you're happy to apply with your proposed changes?

Quoted 10 lines

Quoted 9 lines
---8<---
Use it straight away in crypto_clone_cipher(), as that is not meant to sleep.
Fixes 51d8d6d0f4be ("crypto: cipher - Add crypto_clone_cipher")
Signed-off-by Dmitry Safonov <[email protected]>
Signed-off-by Herbert Xu <[email protected]>
crypto/api.c +0 -0
[..]

Thanks,
Dmitry

Eric Biggers

2023-06-15 23:38 UTC #7 lore raw

Re: [PATCH-next 2/3] crypto: api - Provide gfp mask for tfm allocation

In reply to Dmitry Safonov

crypto/cipher.c struct crypto_cipher *crypto_clone_cipher(struct crypto_cipher *cipher) · line 108

Quoted 13 lines · Dmitry Safonov

crypto/cipher.c +1 -1

index d39ef5f72ab8..184188339a4a 100644
@@ -102,7 +102,7 @@ struct crypto_cipher *crypto_clone_cipher(struct crypto_cipher *cipher)
 		return ERR_PTR(-ENOSYS);
 
 	ntfm = __crypto_alloc_tfm(alg, CRYPTO_ALG_TYPE_CIPHER,
-				  CRYPTO_ALG_TYPE_MASK);
+				  CRYPTO_ALG_TYPE_MASK, GFP_ATOMIC);
 	if (IS_ERR(ntfm))
 		return ERR_CAST(ntfm);

Should crypto_clone_cipher() not have a gfp_t argument itself?

I'm wondering if any users of the crypto_clone_*() functions will need anything other than GFP_ATOMIC, such as GFP_NOFS or GFP_NOIO.

FWIW, btrfs's support for fscrypt is planned to use per-extent keys. It's challenging to implement. I've been thinking it might need a crypto_clone_skcipher() function that it can use during filesystem I/O. That use case would want GFP_NOFS, I think.

- Eric

Herbert Xu

maintainer

2023-06-16 00:26 UTC #8 lore raw

Re: [PATCH-next 2/3] crypto: api - Provide gfp mask for tfm allocation

Quoted 10 lines · Eric Biggers

Should crypto_clone_cipher() not have a gfp_t argument itself?
I'm wondering if any users of the crypto_clone_*() functions will need anything other than GFP_ATOMIC, such as GFP_NOFS or GFP_NOIO.
FWIW, btrfs's support for fscrypt is planned to use per-extent keys. It's challenging to implement. I've been thinking it might need a crypto_clone_skcipher() function that it can use during filesystem I/O. That use case would want GFP_NOFS, I think.

This is usually a small allocation (< 1 page). But if you do need it then we should add it to the generic cloning interface crypto_clone_tfm.

Thanks,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Dmitry Safonov

author

2023-06-14 17:47 UTC #9 lore raw

PATCH 3/3 crypto: cipher - On clone do crypto_mod_get()

In reply to Dmitry Safonov

The refcounter of underlying algorithm should be incremented, otherwise it'll be destroyed with the cloned cipher, wrecking the original cipher.

Signed-off-by Dmitry Safonov <[email protected]>

---
 crypto/cipher.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

crypto/cipher.c +6 -1

index 184188339a4a..b53bf3cda826 100644
@@ -101,10 +101,15 @@ struct crypto_cipher *crypto_clone_cipher(struct crypto_cipher *cipher)
 	if (alg->cra_init)
 		return ERR_PTR(-ENOSYS);
 
+	if (unlikely(!crypto_mod_get(alg)))
+		return ERR_PTR(-ESTALE);
+
 	ntfm = __crypto_alloc_tfm(alg, CRYPTO_ALG_TYPE_CIPHER,
 				  CRYPTO_ALG_TYPE_MASK, GFP_ATOMIC);
-	if (IS_ERR(ntfm))
+	if (IS_ERR(ntfm)) {
+		crypto_mod_put(alg);
 		return ERR_CAST(ntfm);
+	}
 
 	ntfm->crt_flags = tfm->crt_flags;

Signature · 1 line

2.40.0

Herbert Xu

maintainer

2023-06-23 08:23 UTC #10 lore raw

In reply to Dmitry Safonov

Quoted 35 lines · Dmitry Safonov

Make cipher cloning possible in atomic contexts + prevent use-after-free on the crypto algorithm.
Those seems to be all pitfalls I found while adapting TCP-AO patches to use crypto clone-tfm and dropping per-CPU requests allocations.
Cc Bob Gilligan <[email protected]>
Cc David Ahern <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc Dmitry Safonov <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc "Eric W. Biederman" <[email protected]>
Cc Francesco Ruggeri <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Herbert Xu <[email protected]>
Cc Paolo Abeni <[email protected]>
Cc Salam Noureddine <[email protected]>
Cc [email protected]
Cc [email protected]
Thanks,
Dmitry
Dmitry Safonov (3):
crypto: api - Remove crypto_init_ops()
crypto: api - Provide gfp mask for tfm allocation
crypto: cipher - On clone do crypto_mod_get()
 crypto/algapi.c         |  2 +-
 crypto/api.c            | 20 +++-----------------
 crypto/cipher.c         |  9 +++++++--
 crypto/internal.h       |  2 +-
 include/crypto/algapi.h |  1 -
 5 files changed, 12 insertions(+), 22 deletions(-)

Quoted 3 lines

base-commit: b16049b21162bb649cdd8519642a35972b7910fe
Signature · 1 line
2.40.0

All appleed with patch 2 replaced by my version. Thanks.

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt