[net-next,v4,19/20] security/keys: rewrite big_key crypto to use Zinc
diff mbox series

Message ID 20180914162240.7925-20-Jason@zx2c4.com
State New
Headers show
Series
  • WireGuard: Secure Network Tunnel
Related show

Commit Message

Jason A. Donenfeld Sept. 14, 2018, 4:22 p.m. UTC
A while back, I noticed that the crypto and crypto API usage in big_keys
were entirely broken in multiple ways, so I rewrote it. Now, I'm
rewriting it again, but this time using Zinc's ChaCha20Poly1305
function. This makes the file considerably more simple; the diffstat
alone should justify this commit. It also should be faster, since it no
longer requires a mutex around the "aead api object" (nor allocations),
allowing us to encrypt multiple items in parallel. We also benefit from
being able to pass any type of pointer to Zinc, so we can get rid of the
ridiculously complex custom page allocator that big_key really doesn't
need.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Samuel Neves <sneves@dei.uc.pt>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Jean-Philippe Aumasson <jeanphilippe.aumasson@gmail.com>
Cc: Eric Biggers <ebiggers3@gmail.com>
Cc: David Howells <dhowells@redhat.com>
---
 security/keys/Kconfig   |   4 +-
 security/keys/big_key.c | 230 +++++-----------------------------------
 2 files changed, 28 insertions(+), 206 deletions(-)

Comments

kbuild test robot Sept. 15, 2018, 11:52 p.m. UTC | #1
Hi Jason,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Jason-A-Donenfeld/WireGuard-Secure-Network-Tunnel/20180916-043623
config: arm64-defconfig
compiler: aarch64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        GCC_VERSION=7.2.0 make.cross ARCH=arm64  defconfig
        GCC_VERSION=7.2.0 make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

>> drivers/acpi/Kconfig:9:error: recursive dependency detected!
   drivers/acpi/Kconfig:9: symbol ACPI depends on ARCH_SUPPORTS_ACPI
   drivers/acpi/Kconfig:6: symbol ARCH_SUPPORTS_ACPI is selected by EFI
   arch/arm64/Kconfig:1253: symbol EFI depends on KERNEL_MODE_NEON
   arch/arm64/Kconfig:262: symbol KERNEL_MODE_NEON is implied by ZINC_ARCH_ARM
   lib/zinc/Kconfig:42: symbol ZINC_ARCH_ARM depends on ZINC
>> lib/zinc/Kconfig:1: symbol ZINC is selected by ZINC_CHACHA20
>> lib/zinc/Kconfig:4: symbol ZINC_CHACHA20 is selected by ZINC_CHACHA20POLY1305
>> lib/zinc/Kconfig:13: symbol ZINC_CHACHA20POLY1305 is selected by BIG_KEYS
>> security/keys/Kconfig:44: symbol BIG_KEYS depends on KEYS
>> security/keys/Kconfig:5: symbol KEYS is selected by FS_ENCRYPTION
>> fs/crypto/Kconfig:1: symbol FS_ENCRYPTION is selected by UBIFS_FS_ENCRYPTION
>> fs/ubifs/Kconfig:65: symbol UBIFS_FS_ENCRYPTION depends on MISC_FILESYSTEMS
>> fs/Kconfig:218: symbol MISC_FILESYSTEMS is selected by ACPI_APEI
>> drivers/acpi/apei/Kconfig:8: symbol ACPI_APEI depends on ACPI
   For a resolution refer to Documentation/kbuild/kconfig-language.txt
   subsection "Kconfig recursive dependency limitations"

vim +4 lib/zinc/Kconfig

32bbe22e Jason A. Donenfeld 2018-09-14  @1  config ZINC
32bbe22e Jason A. Donenfeld 2018-09-14   2  	tristate
32bbe22e Jason A. Donenfeld 2018-09-14   3  
35f45248 Jason A. Donenfeld 2018-09-14  @4  config ZINC_CHACHA20
35f45248 Jason A. Donenfeld 2018-09-14   5  	bool
35f45248 Jason A. Donenfeld 2018-09-14   6  	select ZINC
35f45248 Jason A. Donenfeld 2018-09-14   7  	select CRYPTO_ALGAPI
35f45248 Jason A. Donenfeld 2018-09-14   8  
0a36c146 Jason A. Donenfeld 2018-09-14   9  config ZINC_POLY1305
0a36c146 Jason A. Donenfeld 2018-09-14  10  	bool
0a36c146 Jason A. Donenfeld 2018-09-14  11  	select ZINC
0a36c146 Jason A. Donenfeld 2018-09-14  12  
1b5dbb86 Jason A. Donenfeld 2018-09-14 @13  config ZINC_CHACHA20POLY1305
1b5dbb86 Jason A. Donenfeld 2018-09-14  14  	bool
1b5dbb86 Jason A. Donenfeld 2018-09-14  15  	select ZINC
1b5dbb86 Jason A. Donenfeld 2018-09-14  16  	select ZINC_CHACHA20
1b5dbb86 Jason A. Donenfeld 2018-09-14  17  	select ZINC_POLY1305
1b5dbb86 Jason A. Donenfeld 2018-09-14  18  	select CRYPTO_BLKCIPHER
1b5dbb86 Jason A. Donenfeld 2018-09-14  19  
a740374c Jason A. Donenfeld 2018-09-14  20  config ZINC_BLAKE2S
a740374c Jason A. Donenfeld 2018-09-14  21  	bool
a740374c Jason A. Donenfeld 2018-09-14  22  	select ZINC
a740374c Jason A. Donenfeld 2018-09-14  23  
cec5aa7c Jason A. Donenfeld 2018-09-14  24  config ZINC_CURVE25519
cec5aa7c Jason A. Donenfeld 2018-09-14  25  	bool
cec5aa7c Jason A. Donenfeld 2018-09-14  26  	select ZINC
cec5aa7c Jason A. Donenfeld 2018-09-14  27  	select CONFIG_CRYPTO
cec5aa7c Jason A. Donenfeld 2018-09-14  28  
32bbe22e Jason A. Donenfeld 2018-09-14  29  config ZINC_DEBUG
32bbe22e Jason A. Donenfeld 2018-09-14  30  	bool "Zinc cryptography library debugging and self-tests"
32bbe22e Jason A. Donenfeld 2018-09-14  31  	depends on ZINC
32bbe22e Jason A. Donenfeld 2018-09-14  32  	help
32bbe22e Jason A. Donenfeld 2018-09-14  33  	  This builds a series of self-tests for the Zinc crypto library, which
32bbe22e Jason A. Donenfeld 2018-09-14  34  	  help diagnose any cryptographic algorithm implementation issues that
32bbe22e Jason A. Donenfeld 2018-09-14  35  	  might be at the root cause of potential bugs. It also adds various
32bbe22e Jason A. Donenfeld 2018-09-14  36  	  debugging traps.
32bbe22e Jason A. Donenfeld 2018-09-14  37  
32bbe22e Jason A. Donenfeld 2018-09-14  38  	  Unless you're developing and testing cryptographic routines, or are
32bbe22e Jason A. Donenfeld 2018-09-14  39  	  especially paranoid about correctness on your hardware, you may say
32bbe22e Jason A. Donenfeld 2018-09-14  40  	  N here.
32bbe22e Jason A. Donenfeld 2018-09-14  41  
32bbe22e Jason A. Donenfeld 2018-09-14 @42  config ZINC_ARCH_ARM
32bbe22e Jason A. Donenfeld 2018-09-14  43  	def_bool y
32bbe22e Jason A. Donenfeld 2018-09-14  44  	depends on ARM
32bbe22e Jason A. Donenfeld 2018-09-14  45  	depends on ZINC
32bbe22e Jason A. Donenfeld 2018-09-14  46  	imply VFP
32bbe22e Jason A. Donenfeld 2018-09-14  47  	imply VFPv3 if CPU_V7
32bbe22e Jason A. Donenfeld 2018-09-14  48  	imply NEON if CPU_V7
32bbe22e Jason A. Donenfeld 2018-09-14  49  	imply KERNEL_MODE_NEON if CPU_V7
32bbe22e Jason A. Donenfeld 2018-09-14  50  

:::::: The code at line 4 was first introduced by commit
:::::: 35f45248597b5a2c80f0f4a680344c22c86efe7d zinc: ChaCha20 generic C implementation

:::::: TO: Jason A. Donenfeld <Jason@zx2c4.com>
:::::: CC: 0day robot <lkp@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Jason A. Donenfeld Sept. 16, 2018, 12:29 a.m. UTC | #2
Hey again Ro,

> 32bbe22e Jason A. Donenfeld 2018-09-14  49      imply KERNEL_MODE_NEON if CPU_V7

This shouldn't have ever been there anyway. Fixed now for v5.

Thanks,
Jason

Patch
diff mbox series

diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index 6462e6654ccf..66ff26298fb3 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -45,9 +45,7 @@  config BIG_KEYS
 	bool "Large payload keys"
 	depends on KEYS
 	depends on TMPFS
-	select CRYPTO
-	select CRYPTO_AES
-	select CRYPTO_GCM
+	select ZINC_CHACHA20POLY1305
 	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 2806e70d7f8f..934497ecbf65 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -1,6 +1,6 @@ 
 /* Large capacity key type
  *
- * Copyright (C) 2017 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
+ * Copyright (C) 2017-2018 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
  * Copyright (C) 2013 Red Hat, Inc. All Rights Reserved.
  * Written by David Howells (dhowells@redhat.com)
  *
@@ -16,20 +16,10 @@ 
 #include <linux/file.h>
 #include <linux/shmem_fs.h>
 #include <linux/err.h>
-#include <linux/scatterlist.h>
 #include <linux/random.h>
-#include <linux/vmalloc.h>
 #include <keys/user-type.h>
 #include <keys/big_key-type.h>
-#include <crypto/aead.h>
-#include <crypto/gcm.h>
-
-struct big_key_buf {
-	unsigned int		nr_pages;
-	void			*virt;
-	struct scatterlist	*sg;
-	struct page		*pages[];
-};
+#include <zinc/chacha20poly1305.h>
 
 /*
  * Layout of key payload words.
@@ -41,14 +31,6 @@  enum {
 	big_key_len,
 };
 
-/*
- * Crypto operation with big_key data
- */
-enum big_key_op {
-	BIG_KEY_ENC,
-	BIG_KEY_DEC,
-};
-
 /*
  * If the data is under this limit, there's no point creating a shm file to
  * hold it as the permanently resident metadata for the shmem fs will be at
@@ -56,16 +38,6 @@  enum big_key_op {
  */
 #define BIG_KEY_FILE_THRESHOLD (sizeof(struct inode) + sizeof(struct dentry))
 
-/*
- * Key size for big_key data encryption
- */
-#define ENC_KEY_SIZE 32
-
-/*
- * Authentication tag length
- */
-#define ENC_AUTHTAG_SIZE 16
-
 /*
  * big_key defined keys take an arbitrary string as the description and an
  * arbitrary blob of data as the payload
@@ -79,136 +51,20 @@  struct key_type key_type_big_key = {
 	.destroy		= big_key_destroy,
 	.describe		= big_key_describe,
 	.read			= big_key_read,
-	/* no ->update(); don't add it without changing big_key_crypt() nonce */
+	/* no ->update(); don't add it without changing chacha20poly1305's nonce */
 };
 
-/*
- * Crypto names for big_key data authenticated encryption
- */
-static const char big_key_alg_name[] = "gcm(aes)";
-#define BIG_KEY_IV_SIZE		GCM_AES_IV_SIZE
-
-/*
- * Crypto algorithms for big_key data authenticated encryption
- */
-static struct crypto_aead *big_key_aead;
-
-/*
- * Since changing the key affects the entire object, we need a mutex.
- */
-static DEFINE_MUTEX(big_key_aead_lock);
-
-/*
- * Encrypt/decrypt big_key data
- */
-static int big_key_crypt(enum big_key_op op, struct big_key_buf *buf, size_t datalen, u8 *key)
-{
-	int ret;
-	struct aead_request *aead_req;
-	/* We always use a zero nonce. The reason we can get away with this is
-	 * because we're using a different randomly generated key for every
-	 * different encryption. Notably, too, key_type_big_key doesn't define
-	 * an .update function, so there's no chance we'll wind up reusing the
-	 * key to encrypt updated data. Simply put: one key, one encryption.
-	 */
-	u8 zero_nonce[BIG_KEY_IV_SIZE];
-
-	aead_req = aead_request_alloc(big_key_aead, GFP_KERNEL);
-	if (!aead_req)
-		return -ENOMEM;
-
-	memset(zero_nonce, 0, sizeof(zero_nonce));
-	aead_request_set_crypt(aead_req, buf->sg, buf->sg, datalen, zero_nonce);
-	aead_request_set_callback(aead_req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
-	aead_request_set_ad(aead_req, 0);
-
-	mutex_lock(&big_key_aead_lock);
-	if (crypto_aead_setkey(big_key_aead, key, ENC_KEY_SIZE)) {
-		ret = -EAGAIN;
-		goto error;
-	}
-	if (op == BIG_KEY_ENC)
-		ret = crypto_aead_encrypt(aead_req);
-	else
-		ret = crypto_aead_decrypt(aead_req);
-error:
-	mutex_unlock(&big_key_aead_lock);
-	aead_request_free(aead_req);
-	return ret;
-}
-
-/*
- * Free up the buffer.
- */
-static void big_key_free_buffer(struct big_key_buf *buf)
-{
-	unsigned int i;
-
-	if (buf->virt) {
-		memset(buf->virt, 0, buf->nr_pages * PAGE_SIZE);
-		vunmap(buf->virt);
-	}
-
-	for (i = 0; i < buf->nr_pages; i++)
-		if (buf->pages[i])
-			__free_page(buf->pages[i]);
-
-	kfree(buf);
-}
-
-/*
- * Allocate a buffer consisting of a set of pages with a virtual mapping
- * applied over them.
- */
-static void *big_key_alloc_buffer(size_t len)
-{
-	struct big_key_buf *buf;
-	unsigned int npg = (len + PAGE_SIZE - 1) >> PAGE_SHIFT;
-	unsigned int i, l;
-
-	buf = kzalloc(sizeof(struct big_key_buf) +
-		      sizeof(struct page) * npg +
-		      sizeof(struct scatterlist) * npg,
-		      GFP_KERNEL);
-	if (!buf)
-		return NULL;
-
-	buf->nr_pages = npg;
-	buf->sg = (void *)(buf->pages + npg);
-	sg_init_table(buf->sg, npg);
-
-	for (i = 0; i < buf->nr_pages; i++) {
-		buf->pages[i] = alloc_page(GFP_KERNEL);
-		if (!buf->pages[i])
-			goto nomem;
-
-		l = min_t(size_t, len, PAGE_SIZE);
-		sg_set_page(&buf->sg[i], buf->pages[i], l, 0);
-		len -= l;
-	}
-
-	buf->virt = vmap(buf->pages, buf->nr_pages, VM_MAP, PAGE_KERNEL);
-	if (!buf->virt)
-		goto nomem;
-
-	return buf;
-
-nomem:
-	big_key_free_buffer(buf);
-	return NULL;
-}
-
 /*
  * Preparse a big key
  */
 int big_key_preparse(struct key_preparsed_payload *prep)
 {
-	struct big_key_buf *buf;
 	struct path *path = (struct path *)&prep->payload.data[big_key_path];
 	struct file *file;
-	u8 *enckey;
+	u8 *buf, *enckey;
 	ssize_t written;
-	size_t datalen = prep->datalen, enclen = datalen + ENC_AUTHTAG_SIZE;
+	size_t datalen = prep->datalen;
+	size_t enclen = datalen + CHACHA20POLY1305_AUTHTAGLEN;
 	int ret;
 
 	if (datalen <= 0 || datalen > 1024 * 1024 || !prep->data)
@@ -224,28 +80,28 @@  int big_key_preparse(struct key_preparsed_payload *prep)
 		 * to be swapped out if needed.
 		 *
 		 * File content is stored encrypted with randomly generated key.
+		 * Since the key is random for each file, we can set the nonce
+		 * to zero, provided we never define a ->update() call.
 		 */
 		loff_t pos = 0;
 
-		buf = big_key_alloc_buffer(enclen);
+		buf = kvmalloc(enclen, GFP_KERNEL);
 		if (!buf)
 			return -ENOMEM;
-		memcpy(buf->virt, prep->data, datalen);
 
 		/* generate random key */
-		enckey = kmalloc(ENC_KEY_SIZE, GFP_KERNEL);
+		enckey = kmalloc(CHACHA20POLY1305_KEYLEN, GFP_KERNEL);
 		if (!enckey) {
 			ret = -ENOMEM;
 			goto error;
 		}
-		ret = get_random_bytes_wait(enckey, ENC_KEY_SIZE);
+		ret = get_random_bytes_wait(enckey, CHACHA20POLY1305_KEYLEN);
 		if (unlikely(ret))
 			goto err_enckey;
 
-		/* encrypt aligned data */
-		ret = big_key_crypt(BIG_KEY_ENC, buf, datalen, enckey);
-		if (ret)
-			goto err_enckey;
+		/* encrypt data */
+		chacha20poly1305_encrypt(buf, prep->data, datalen, NULL, 0,
+					 0, enckey);
 
 		/* save aligned data to file */
 		file = shmem_kernel_file_setup("", enclen, 0);
@@ -254,7 +110,7 @@  int big_key_preparse(struct key_preparsed_payload *prep)
 			goto err_enckey;
 		}
 
-		written = kernel_write(file, buf->virt, enclen, &pos);
+		written = kernel_write(file, buf, enclen, &pos);
 		if (written != enclen) {
 			ret = written;
 			if (written >= 0)
@@ -269,7 +125,7 @@  int big_key_preparse(struct key_preparsed_payload *prep)
 		*path = file->f_path;
 		path_get(path);
 		fput(file);
-		big_key_free_buffer(buf);
+		kvfree(buf);
 	} else {
 		/* Just store the data in a buffer */
 		void *data = kmalloc(datalen, GFP_KERNEL);
@@ -287,7 +143,7 @@  int big_key_preparse(struct key_preparsed_payload *prep)
 err_enckey:
 	kzfree(enckey);
 error:
-	big_key_free_buffer(buf);
+	kvfree(buf);
 	return ret;
 }
 
@@ -365,14 +221,13 @@  long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
 		return datalen;
 
 	if (datalen > BIG_KEY_FILE_THRESHOLD) {
-		struct big_key_buf *buf;
 		struct path *path = (struct path *)&key->payload.data[big_key_path];
 		struct file *file;
-		u8 *enckey = (u8 *)key->payload.data[big_key_data];
-		size_t enclen = datalen + ENC_AUTHTAG_SIZE;
+		u8 *buf, *enckey = (u8 *)key->payload.data[big_key_data];
+		size_t enclen = datalen + CHACHA20POLY1305_AUTHTAGLEN;
 		loff_t pos = 0;
 
-		buf = big_key_alloc_buffer(enclen);
+		buf = kvmalloc(enclen, GFP_KERNEL);
 		if (!buf)
 			return -ENOMEM;
 
@@ -383,26 +238,27 @@  long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
 		}
 
 		/* read file to kernel and decrypt */
-		ret = kernel_read(file, buf->virt, enclen, &pos);
+		ret = kernel_read(file, buf, enclen, &pos);
 		if (ret >= 0 && ret != enclen) {
 			ret = -EIO;
 			goto err_fput;
 		}
 
-		ret = big_key_crypt(BIG_KEY_DEC, buf, enclen, enckey);
-		if (ret)
+		ret = chacha20poly1305_decrypt(buf, buf, enclen, NULL, 0, 0,
+					       enckey) ? 0 : -EINVAL;
+		if (unlikely(ret))
 			goto err_fput;
 
 		ret = datalen;
 
 		/* copy decrypted data to user */
-		if (copy_to_user(buffer, buf->virt, datalen) != 0)
+		if (copy_to_user(buffer, buf, datalen) != 0)
 			ret = -EFAULT;
 
 err_fput:
 		fput(file);
 error:
-		big_key_free_buffer(buf);
+		kvfree(buf);
 	} else {
 		ret = datalen;
 		if (copy_to_user(buffer, key->payload.data[big_key_data],
@@ -418,39 +274,7 @@  long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
  */
 static int __init big_key_init(void)
 {
-	int ret;
-
-	/* init block cipher */
-	big_key_aead = crypto_alloc_aead(big_key_alg_name, 0, CRYPTO_ALG_ASYNC);
-	if (IS_ERR(big_key_aead)) {
-		ret = PTR_ERR(big_key_aead);
-		pr_err("Can't alloc crypto: %d\n", ret);
-		return ret;
-	}
-
-	if (unlikely(crypto_aead_ivsize(big_key_aead) != BIG_KEY_IV_SIZE)) {
-		WARN(1, "big key algorithm changed?");
-		ret = -EINVAL;
-		goto free_aead;
-	}
-
-	ret = crypto_aead_setauthsize(big_key_aead, ENC_AUTHTAG_SIZE);
-	if (ret < 0) {
-		pr_err("Can't set crypto auth tag len: %d\n", ret);
-		goto free_aead;
-	}
-
-	ret = register_key_type(&key_type_big_key);
-	if (ret < 0) {
-		pr_err("Can't register type: %d\n", ret);
-		goto free_aead;
-	}
-
-	return 0;
-
-free_aead:
-	crypto_free_aead(big_key_aead);
-	return ret;
+	return register_key_type(&key_type_big_key);
 }
 
 late_initcall(big_key_init);