[PATCH] KEYS: Sort out big_key initialisation

[PATCH] KEYS: Sort out big_key initialisation

[Kirill Marinushkin]

big_key has two separate initialisation functions, one that registers the
key type and one that registers the crypto.  If the key type fails to
register, there's no problem if the crypto registers successfully because
there's no way to reach the crypto except through the key type.

However, if the key type registers successfully but the crypto does not,
big_key_rng and big_key_blkcipher may end up set to NULL - but the code
neither checks for this nor unregisters the big key key type.

Furthermore, since the key type is registered before the crypto, it is
theoretically possible for the kernel to try adding a big_key before the
crypto is set up, leading to the same effect.

Fix this by merging big_key_crypto_init() and big_key_init() and calling
the resulting function late.  If they're going to be encrypted, we
shouldn't be creating big_keys before we have the facilities to do the
encryption available.  The key type registration is also moved after the
crypto initialisation.

The fix also includes message printing on failure.

If the big_key type isn't correctly set up, simply doing:

    dd if=/dev/zero bs=4096 count=1 | keyctl padd big_key a @s

ought to cause an oops.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Peter Hlavaty <zer0mem@yahoo.com>
cc: stable@vger.kernel.org
Signed-off-by: Kirill Marinushkin <k.marinushkin@gmail.com>
---
 security/keys/Kconfig   |  2 +-
 security/keys/big_key.c | 62 +++++++++++++++++++++++++++----------------------
 2 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index f826e87..8213221 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -44,7 +44,7 @@ config BIG_KEYS
 	select CRYPTO
 	select CRYPTO_AES
 	select CRYPTO_ECB
-	select CRYPTO_RNG
+	select CRYPTO_ANSI_CPRNG
 	help
 	  This option provides support for holding large keys within the kernel
 	  (for example Kerberos ticket caches).  The data may be stored out to
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index c0b3030..fce1df9 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -9,6 +9,7 @@
  * 2 of the Licence, or (at your option) any later version.
  */
 
+#define pr_fmt(fmt) "big_key: "fmt
 #include <linux/init.h>
 #include <linux/seq_file.h>
 #include <linux/file.h>
@@ -341,44 +342,49 @@ error:
  */
 static int __init big_key_init(void)
 {
-	return register_key_type(&key_type_big_key);
-}
-
-/*
- * Initialize big_key crypto and RNG algorithms
- */
-static int __init big_key_crypto_init(void)
-{
-	int ret = -EINVAL;
+	struct crypto_skcipher *cipher;
+	struct crypto_rng *rng;
+	int ret;
 
-	/* init RNG */
-	big_key_rng = crypto_alloc_rng(big_key_rng_name, 0, 0);
-	if (IS_ERR(big_key_rng)) {
-		big_key_rng = NULL;
-		return -EFAULT;
+	rng = crypto_alloc_rng(big_key_rng_name, 0, 0);
+	if (IS_ERR(rng)) {
+		pr_err("Can't alloc rng: %ld\n", PTR_ERR(rng));
+		return PTR_ERR(rng);
 	}
 
+	big_key_rng = rng;
+
 	/* seed RNG */
-	ret = crypto_rng_reset(big_key_rng, NULL, crypto_rng_seedsize(big_key_rng));
-	if (ret)
-		goto error;
+	ret = crypto_rng_reset(rng, NULL, crypto_rng_seedsize(rng));
+	if (ret) {
+		pr_err("Can't reset rng: %d\n", ret);
+		goto error_rng;
+	}
 
-	/* init block cipher */
-	big_key_skcipher = crypto_alloc_skcipher(big_key_alg_name,
-						 0, CRYPTO_ALG_ASYNC);
-	if (IS_ERR(big_key_skcipher)) {
-		big_key_skcipher = NULL;
-		ret = -EFAULT;
-		goto error;
+	cipher = crypto_alloc_skcipher(big_key_alg_name,
+				       0, CRYPTO_ALG_ASYNC);
+	if (IS_ERR(cipher)) {
+		ret = PTR_ERR(cipher);
+		pr_err("Can't alloc crypto: %d\n", ret);
+		goto error_rng;
+	}
+
+	big_key_skcipher = cipher;
+
+	ret = register_key_type(&key_type_big_key);
+	if (ret < 0) {
+		pr_err("Can't register key type: %d\n", ret);
+		goto error_cipher;
 	}
 
 	return 0;
 
-error:
+error_cipher:
+	crypto_free_skcipher(big_key_skcipher);
+error_rng:
 	crypto_free_rng(big_key_rng);
-	big_key_rng = NULL;
+
 	return ret;
 }
 
-device_initcall(big_key_init);
-late_initcall(big_key_crypto_init);
+late_initcall(big_key_init);
-- 
1.9.1

Re: [PATCH] KEYS: Sort out big_key initialisation

[Joe Perches]

On Sat, 2016-07-30 at 22:03 +0200, Kirill Marinushkin wrote:
> big_key has two separate initialisation functions, one that registers the
> key type and one that registers the crypto.  If the key type fails to
> register, there's no problem if the crypto registers successfully because
> there's no way to reach the crypto except through the key type.

trivia:

> diff --git a/security/keys/big_key.c b/security/keys/big_key.c
[]
> @@ -9,6 +9,7 @@
>   * 2 of the Licence, or (at your option) any later version.
>   */
>  
> +#define pr_fmt(fmt) "big_key: "fmt

It's much more common to use

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

RE: [PATCH] KEYS: Sort out big_key initialisation

[Kirill Marinushkin]

> It's much more common to use
> 
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

This happened to be a wrong email thread and a wrong kernel tag;
I resent a proper patch in-reply-to a proper thread.

Re: [PATCH] KEYS: Sort out big_key initialisation

[Greg KH]

On Sat, Jul 30, 2016 at 06:21:41PM +0200, Kirill Marinushkin wrote:
> big_key has two separate initialisation functions, one that registers the
> key type and one that registers the crypto.  If the key type fails to
> register, there's no problem if the crypto registers successfully because
> there's no way to reach the crypto except through the key type.
> 
> However, if the key type registers successfully but the crypto does not,
> big_key_rng and big_key_blkcipher may end up set to NULL - but the code
> neither checks for this nor unregisters the big key key type.
> 
> Furthermore, since the key type is registered before the crypto, it is
> theoretically possible for the kernel to try adding a big_key before the
> crypto is set up, leading to the same effect.
> 
> Fix this by merging big_key_crypto_init() and big_key_init() and calling
> the resulting function late.  If they're going to be encrypted, we
> shouldn't be creating big_keys before we have the facilities to do the
> encryption available.  The key type registration is also moved after the
> crypto initialisation.
> 
> The fix also includes message printing on failure.
> 
> If the big_key type isn't correctly set up, simply doing:
> 
>     dd if=/dev/zero bs=4096 count=1 | keyctl padd big_key a @s
> 
> ought to cause an oops.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Kirill Marinushkin <k.marinushkin@gmail.com>
> ---
>  security/keys/Kconfig   |  2 +-
>  security/keys/big_key.c | 62 +++++++++++++++++++++++++++----------------------
>  2 files changed, 35 insertions(+), 29 deletions(-)


<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

</formletter>

[PATCH] KEYS: Sort out big_key initialisation

[Kirill Marinushkin]

big_key has two separate initialisation functions, one that registers the
key type and one that registers the crypto.  If the key type fails to
register, there's no problem if the crypto registers successfully because
there's no way to reach the crypto except through the key type.

However, if the key type registers successfully but the crypto does not,
big_key_rng and big_key_blkcipher may end up set to NULL - but the code
neither checks for this nor unregisters the big key key type.

Furthermore, since the key type is registered before the crypto, it is
theoretically possible for the kernel to try adding a big_key before the
crypto is set up, leading to the same effect.

Fix this by merging big_key_crypto_init() and big_key_init() and calling
the resulting function late.  If they're going to be encrypted, we
shouldn't be creating big_keys before we have the facilities to do the
encryption available.  The key type registration is also moved after the
crypto initialisation.

The fix also includes message printing on failure.

If the big_key type isn't correctly set up, simply doing:

    dd if=/dev/zero bs=4096 count=1 | keyctl padd big_key a @s

ought to cause an oops.

Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Kirill Marinushkin <k.marinushkin@gmail.com>
---
 security/keys/Kconfig   |  2 +-
 security/keys/big_key.c | 62 +++++++++++++++++++++++++++----------------------
 2 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index f826e87..8213221 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -44,7 +44,7 @@ config BIG_KEYS
 	select CRYPTO
 	select CRYPTO_AES
 	select CRYPTO_ECB
-	select CRYPTO_RNG
+	select CRYPTO_ANSI_CPRNG
 	help
 	  This option provides support for holding large keys within the kernel
 	  (for example Kerberos ticket caches).  The data may be stored out to
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index c0b3030..fce1df9 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -9,6 +9,7 @@
  * 2 of the Licence, or (at your option) any later version.
  */
 
+#define pr_fmt(fmt) "big_key: "fmt
 #include <linux/init.h>
 #include <linux/seq_file.h>
 #include <linux/file.h>
@@ -341,44 +342,49 @@ error:
  */
 static int __init big_key_init(void)
 {
-	return register_key_type(&key_type_big_key);
-}
-
-/*
- * Initialize big_key crypto and RNG algorithms
- */
-static int __init big_key_crypto_init(void)
-{
-	int ret = -EINVAL;
+	struct crypto_skcipher *cipher;
+	struct crypto_rng *rng;
+	int ret;
 
-	/* init RNG */
-	big_key_rng = crypto_alloc_rng(big_key_rng_name, 0, 0);
-	if (IS_ERR(big_key_rng)) {
-		big_key_rng = NULL;
-		return -EFAULT;
+	rng = crypto_alloc_rng(big_key_rng_name, 0, 0);
+	if (IS_ERR(rng)) {
+		pr_err("Can't alloc rng: %ld\n", PTR_ERR(rng));
+		return PTR_ERR(rng);
 	}
 
+	big_key_rng = rng;
+
 	/* seed RNG */
-	ret = crypto_rng_reset(big_key_rng, NULL, crypto_rng_seedsize(big_key_rng));
-	if (ret)
-		goto error;
+	ret = crypto_rng_reset(rng, NULL, crypto_rng_seedsize(rng));
+	if (ret) {
+		pr_err("Can't reset rng: %d\n", ret);
+		goto error_rng;
+	}
 
-	/* init block cipher */
-	big_key_skcipher = crypto_alloc_skcipher(big_key_alg_name,
-						 0, CRYPTO_ALG_ASYNC);
-	if (IS_ERR(big_key_skcipher)) {
-		big_key_skcipher = NULL;
-		ret = -EFAULT;
-		goto error;
+	cipher = crypto_alloc_skcipher(big_key_alg_name,
+				       0, CRYPTO_ALG_ASYNC);
+	if (IS_ERR(cipher)) {
+		ret = PTR_ERR(cipher);
+		pr_err("Can't alloc crypto: %d\n", ret);
+		goto error_rng;
+	}
+
+	big_key_skcipher = cipher;
+
+	ret = register_key_type(&key_type_big_key);
+	if (ret < 0) {
+		pr_err("Can't register key type: %d\n", ret);
+		goto error_cipher;
 	}
 
 	return 0;
 
-error:
+error_cipher:
+	crypto_free_skcipher(big_key_skcipher);
+error_rng:
 	crypto_free_rng(big_key_rng);
-	big_key_rng = NULL;
+
 	return ret;
 }
 
-device_initcall(big_key_init);
-late_initcall(big_key_crypto_init);
+late_initcall(big_key_init);
-- 
1.9.1