[PATCH] crypto/mcryptd: Check mcryptd algorithm compatability

[PATCH] crypto/mcryptd: Check mcryptd algorithm compatability

[Tim Chen]

Algorithms not compatible with mcryptd could be spawned by mcryptd
with a direct crypto_alloc_tfm invocation using a "mcryptd(alg)"
name construct.  This causes mcryptd to crash the kernel if
"alg" is incompatible and not intended to be used with mcryptd.

A flag CRYPTO_ALG_MCRYPT is being added to mcryptd compatible
algorithms' cra_flags. The compatability is checked when mcryptd spawn
off an algorithm.

Link: http://marc.info/?l=linux-crypto-vger&m=148063683310477&w=2
Cc: stable@vger.kernel.org
Reported-by: Mikulas Patocka <mpatocka@redhat.com>
Tested-by: Megha Dey <megha.dey@linux.intel.com>
Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 arch/x86/crypto/sha1-mb/sha1_mb.c     | 3 ++-
 arch/x86/crypto/sha256-mb/sha256_mb.c | 3 ++-
 arch/x86/crypto/sha512-mb/sha512_mb.c | 3 ++-
 crypto/mcryptd.c                      | 6 ++++++
 include/linux/crypto.h                | 6 ++++++
 5 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/arch/x86/crypto/sha1-mb/sha1_mb.c b/arch/x86/crypto/sha1-mb/sha1_mb.c
index acf9fdf..475959db 100644
--- a/arch/x86/crypto/sha1-mb/sha1_mb.c
+++ b/arch/x86/crypto/sha1-mb/sha1_mb.c
@@ -770,7 +770,8 @@ static struct ahash_alg sha1_mb_areq_alg = {
 			 */
 			.cra_flags	= CRYPTO_ALG_TYPE_AHASH |
 						CRYPTO_ALG_ASYNC |
-						CRYPTO_ALG_INTERNAL,
+						CRYPTO_ALG_INTERNAL |
+						CRYPTO_ALG_MCRYPT,
 			.cra_blocksize	= SHA1_BLOCK_SIZE,
 			.cra_module	= THIS_MODULE,
 			.cra_list	= LIST_HEAD_INIT
diff --git a/arch/x86/crypto/sha256-mb/sha256_mb.c b/arch/x86/crypto/sha256-mb/sha256_mb.c
index 7926a22..f33b592 100644
--- a/arch/x86/crypto/sha256-mb/sha256_mb.c
+++ b/arch/x86/crypto/sha256-mb/sha256_mb.c
@@ -768,7 +768,8 @@ static struct ahash_alg sha256_mb_areq_alg = {
 			 */
 			.cra_flags	= CRYPTO_ALG_TYPE_AHASH |
 						CRYPTO_ALG_ASYNC |
-						CRYPTO_ALG_INTERNAL,
+						CRYPTO_ALG_INTERNAL |
+						CRYPTO_ALG_MCRYPT,
 			.cra_blocksize	= SHA256_BLOCK_SIZE,
 			.cra_module	= THIS_MODULE,
 			.cra_list	= LIST_HEAD_INIT
diff --git a/arch/x86/crypto/sha512-mb/sha512_mb.c b/arch/x86/crypto/sha512-mb/sha512_mb.c
index 9c1bb6d..13aa2e6 100644
--- a/arch/x86/crypto/sha512-mb/sha512_mb.c
+++ b/arch/x86/crypto/sha512-mb/sha512_mb.c
@@ -783,7 +783,8 @@ static struct ahash_alg sha512_mb_areq_alg = {
 			 */
 			.cra_flags	= CRYPTO_ALG_TYPE_AHASH |
 						CRYPTO_ALG_ASYNC |
-						CRYPTO_ALG_INTERNAL,
+						CRYPTO_ALG_INTERNAL |
+						CRYPTO_ALG_MCRYPT,
 			.cra_blocksize	= SHA512_BLOCK_SIZE,
 			.cra_module	= THIS_MODULE,
 			.cra_list	= LIST_HEAD_INIT
diff --git a/crypto/mcryptd.c b/crypto/mcryptd.c
index 94ee44a..5c40e13 100644
--- a/crypto/mcryptd.c
+++ b/crypto/mcryptd.c
@@ -500,6 +500,12 @@ static int mcryptd_create_hash(struct crypto_template *tmpl, struct rtattr **tb,
 
 	alg = &halg->base;
 	pr_debug("crypto: mcryptd hash alg: %s\n", alg->cra_name);
+
+	if (!(alg->cra_flags & CRYPTO_ALG_MCRYPT)) {
+		err = -EINVAL;
+		goto out_put_alg;
+	}
+
 	inst = mcryptd_alloc_instance(alg, ahash_instance_headroom(),
 					sizeof(*ctx));
 	err = PTR_ERR(inst);
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 167aea2..e47d5a8 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -106,6 +106,12 @@
 #define CRYPTO_ALG_INTERNAL		0x00002000
 
 /*
+ * Mark cipher as compatible with mcryptd
+ * for multi-buffer processing
+ */
+#define CRYPTO_ALG_MCRYPT		0x00004000
+
+/*
  * Transform masks and values (for crt_flags).
  */
 #define CRYPTO_TFM_REQ_MASK		0x000fff00
-- 
2.5.5

Re: [PATCH] crypto/mcryptd: Check mcryptd algorithm compatability

[Herbert Xu]

On Fri, Dec 02, 2016 at 04:15:21PM -0800, Tim Chen wrote:
> Algorithms not compatible with mcryptd could be spawned by mcryptd
> with a direct crypto_alloc_tfm invocation using a "mcryptd(alg)"
> name construct.  This causes mcryptd to crash the kernel if
> "alg" is incompatible and not intended to be used with mcryptd.
> 
> A flag CRYPTO_ALG_MCRYPT is being added to mcryptd compatible
> algorithms' cra_flags. The compatability is checked when mcryptd spawn
> off an algorithm.
> 
> Link: http://marc.info/?l=linux-crypto-vger&m=148063683310477&w=2
> Cc: stable@vger.kernel.org
> Reported-by: Mikulas Patocka <mpatocka@redhat.com>
> Tested-by: Megha Dey <megha.dey@linux.intel.com>
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>

Tim, I think we should instead make mcryptd refuse to generate
a non-internal algorithm.  This way the user would not be able
to access it at all since they can only request for non-internal
algorithms.

Basically you want to check at the start of mcryptd_create_hash
that the INTERNAL bit is set on both the type and mask as returned
by crypto_get_attr_type.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto/mcryptd: Check mcryptd algorithm compatability

[Tim Chen]

On Mon, 2016-12-05 at 20:34 +0800, Herbert Xu wrote:
> On Fri, Dec 02, 2016 at 04:15:21PM -0800, Tim Chen wrote:
> > 
> > Algorithms not compatible with mcryptd could be spawned by mcryptd
> > with a direct crypto_alloc_tfm invocation using a "mcryptd(alg)"
> > name construct.  This causes mcryptd to crash the kernel if
> > "alg" is incompatible and not intended to be used with mcryptd.
> > 
> > A flag CRYPTO_ALG_MCRYPT is being added to mcryptd compatible
> > algorithms' cra_flags. The compatability is checked when mcryptd spawn
> > off an algorithm.
> > 
> > Link: http://marc.info/?l=linux-crypto-vger&m=148063683310477&w=2
> > Cc: stable@vger.kernel.org
> > Reported-by: Mikulas Patocka <mpatocka@redhat.com>
> > Tested-by: Megha Dey <megha.dey@linux.intel.com>
> > Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> Tim, I think we should instead make mcryptd refuse to generate
> a non-internal algorithm.  This way the user would not be able
> to access it at all since they can only request for non-internal
> algorithms.
> 
> Basically you want to check at the start of mcryptd_create_hash
> that the INTERNAL bit is set on both the type and mask as returned
> by crypto_get_attr_type.

I have thought about that.  However, not all internal algorithms
are compatible with mcryptd. We can still have trouble if some
random internal algorithm is paired with mcryptd.

Tim

Re: [PATCH] crypto/mcryptd: Check mcryptd algorithm compatability

[Tim Chen]

On Mon, 2016-12-05 at 08:50 -0800, Tim Chen wrote:
> On Mon, 2016-12-05 at 20:34 +0800, Herbert Xu wrote:
> > 
> > On Fri, Dec 02, 2016 at 04:15:21PM -0800, Tim Chen wrote:
> > > 
> > > 
> > > Algorithms not compatible with mcryptd could be spawned by mcryptd
> > > with a direct crypto_alloc_tfm invocation using a "mcryptd(alg)"
> > > name construct.  This causes mcryptd to crash the kernel if
> > > "alg" is incompatible and not intended to be used with mcryptd.
> > > 
> > > A flag CRYPTO_ALG_MCRYPT is being added to mcryptd compatible
> > > algorithms' cra_flags. The compatability is checked when mcryptd spawn
> > > off an algorithm.
> > > 
> > > Link: http://marc.info/?l=linux-crypto-vger&m=148063683310477&w=2
> > > Cc: stable@vger.kernel.org
> > > Reported-by: Mikulas Patocka <mpatocka@redhat.com>
> > > Tested-by: Megha Dey <megha.dey@linux.intel.com>
> > > Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> > Tim, I think we should instead make mcryptd refuse to generate
> > a non-internal algorithm.  This way the user would not be able
> > to access it at all since they can only request for non-internal
> > algorithms.
> > 
> > Basically you want to check at the start of mcryptd_create_hash
> > that the INTERNAL bit is set on both the type and mask as returned
> > by crypto_get_attr_type.
> I have thought about that.  However, not all internal algorithms
> are compatible with mcryptd. We can still have trouble if some
> random internal algorithm is paired with mcryptd.

You're right.  The mcryptd(alg) should be an internal algorithm itself.
So checking on the INTERNAL flag should be sufficient.

I'll generate another patch that uses the INTERNAL flag for checking.

Tim