All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] security/keys: rewrite big_key crypto to use Zinc
@ 2020-05-01 22:23 ` Jason A. Donenfeld
  0 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-01 22:23 UTC (permalink / raw)
  To: dhowells, keyrings
  Cc: Jason A. Donenfeld, Andy Lutomirski, Greg KH, Linus Torvalds,
	kernel-hardening

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, so we can get rid of the
ridiculously complex custom page allocator that big_key really doesn't
need.

Cc: David Howells <dhowells@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: kernel-hardening@lists.openwall.com
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
I finally got around to updating this patch from the mailing list posts
back in 2017-2018, using the library interface that we eventually merged
in 2019. I haven't retested this for functionality, but nothing much has
changed, so I suspect things should still be good to go.

 security/keys/Kconfig   |   4 +-
 security/keys/big_key.c | 230 +++++-----------------------------------
 2 files changed, 28 insertions(+), 206 deletions(-)

diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index 47c041563d41..5aa442490d52 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -60,9 +60,7 @@ config BIG_KEYS
 	bool "Large payload keys"
 	depends on KEYS
 	depends on TMPFS
-	select CRYPTO
-	select CRYPTO_AES
-	select CRYPTO_GCM
+	select CRYPTO_LIB_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 82008f900930..0b5f61ea09f1 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /* Large capacity key type
  *
- * Copyright (C) 2017 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
+ * Copyright (C) 2017-2020 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)
  */
@@ -12,20 +12,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 <crypto/chacha20poly1305.h>
 
 /*
  * Layout of key payload words.
@@ -37,14 +27,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
@@ -52,16 +34,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
@@ -75,136 +47,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_AUTHTAG_SIZE;
 	int ret;
 
 	if (datalen <= 0 || datalen > 1024 * 1024 || !prep->data)
@@ -220,28 +76,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_KEY_SIZE, GFP_KERNEL);
 		if (!enckey) {
 			ret = -ENOMEM;
 			goto error;
 		}
-		ret = get_random_bytes_wait(enckey, ENC_KEY_SIZE);
+		ret = get_random_bytes_wait(enckey, CHACHA20POLY1305_KEY_SIZE);
 		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);
@@ -250,7 +106,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)
@@ -265,7 +121,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);
@@ -283,7 +139,7 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 err_enckey:
 	kzfree(enckey);
 error:
-	big_key_free_buffer(buf);
+	kvfree(buf);
 	return ret;
 }
 
@@ -361,14 +217,13 @@ long big_key_read(const struct key *key, char *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_AUTHTAG_SIZE;
 		loff_t pos = 0;
 
-		buf = big_key_alloc_buffer(enclen);
+		buf = kvmalloc(enclen, GFP_KERNEL);
 		if (!buf)
 			return -ENOMEM;
 
@@ -379,25 +234,26 @@ long big_key_read(const struct key *key, char *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 out decrypted data */
-		memcpy(buffer, buf->virt, datalen);
+		memcpy(buffer, buf, datalen);
 
 err_fput:
 		fput(file);
 error:
-		big_key_free_buffer(buf);
+		kvfree(buf);
 	} else {
 		ret = datalen;
 		memcpy(buffer, key->payload.data[big_key_data], datalen);
@@ -411,39 +267,7 @@ long big_key_read(const struct key *key, char *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);
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH] security/keys: rewrite big_key crypto to use Zinc
@ 2020-05-01 22:23 ` Jason A. Donenfeld
  0 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-01 22:23 UTC (permalink / raw)
  To: dhowells, keyrings
  Cc: Jason A. Donenfeld, Andy Lutomirski, Greg KH, Linus Torvalds,
	kernel-hardening

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, so we can get rid of the
ridiculously complex custom page allocator that big_key really doesn't
need.

Cc: David Howells <dhowells@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: kernel-hardening@lists.openwall.com
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
I finally got around to updating this patch from the mailing list posts
back in 2017-2018, using the library interface that we eventually merged
in 2019. I haven't retested this for functionality, but nothing much has
changed, so I suspect things should still be good to go.

 security/keys/Kconfig   |   4 +-
 security/keys/big_key.c | 230 +++++-----------------------------------
 2 files changed, 28 insertions(+), 206 deletions(-)

diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index 47c041563d41..5aa442490d52 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -60,9 +60,7 @@ config BIG_KEYS
 	bool "Large payload keys"
 	depends on KEYS
 	depends on TMPFS
-	select CRYPTO
-	select CRYPTO_AES
-	select CRYPTO_GCM
+	select CRYPTO_LIB_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 82008f900930..0b5f61ea09f1 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /* Large capacity key type
  *
- * Copyright (C) 2017 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
+ * Copyright (C) 2017-2020 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)
  */
@@ -12,20 +12,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 <crypto/chacha20poly1305.h>
 
 /*
  * Layout of key payload words.
@@ -37,14 +27,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
@@ -52,16 +34,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
@@ -75,136 +47,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_AUTHTAG_SIZE;
 	int ret;
 
 	if (datalen <= 0 || datalen > 1024 * 1024 || !prep->data)
@@ -220,28 +76,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_KEY_SIZE, GFP_KERNEL);
 		if (!enckey) {
 			ret = -ENOMEM;
 			goto error;
 		}
-		ret = get_random_bytes_wait(enckey, ENC_KEY_SIZE);
+		ret = get_random_bytes_wait(enckey, CHACHA20POLY1305_KEY_SIZE);
 		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);
@@ -250,7 +106,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)
@@ -265,7 +121,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);
@@ -283,7 +139,7 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 err_enckey:
 	kzfree(enckey);
 error:
-	big_key_free_buffer(buf);
+	kvfree(buf);
 	return ret;
 }
 
@@ -361,14 +217,13 @@ long big_key_read(const struct key *key, char *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_AUTHTAG_SIZE;
 		loff_t pos = 0;
 
-		buf = big_key_alloc_buffer(enclen);
+		buf = kvmalloc(enclen, GFP_KERNEL);
 		if (!buf)
 			return -ENOMEM;
 
@@ -379,25 +234,26 @@ long big_key_read(const struct key *key, char *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 out decrypted data */
-		memcpy(buffer, buf->virt, datalen);
+		memcpy(buffer, buf, datalen);
 
 err_fput:
 		fput(file);
 error:
-		big_key_free_buffer(buf);
+		kvfree(buf);
 	} else {
 		ret = datalen;
 		memcpy(buffer, key->payload.data[big_key_data], datalen);
@@ -411,39 +267,7 @@ long big_key_read(const struct key *key, char *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);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH] security/keys: rewrite big_key crypto to use Zinc
  2020-05-01 22:23 ` Jason A. Donenfeld
@ 2020-05-01 23:09   ` Eric Biggers
  -1 siblings, 0 replies; 28+ messages in thread
From: Eric Biggers @ 2020-05-01 23:09 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: dhowells, keyrings, Andy Lutomirski, Greg KH, Linus Torvalds,
	kernel-hardening

On Fri, May 01, 2020 at 04:23:57PM -0600, Jason A. Donenfeld wrote:
> 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, so we can get rid of the
> ridiculously complex custom page allocator that big_key really doesn't
> need.
> 
> Cc: David Howells <dhowells@redhat.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: kernel-hardening@lists.openwall.com
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> I finally got around to updating this patch from the mailing list posts
> back in 2017-2018, using the library interface that we eventually merged
> in 2019. I haven't retested this for functionality, but nothing much has
> changed, so I suspect things should still be good to go.
> 
>  security/keys/Kconfig   |   4 +-
>  security/keys/big_key.c | 230 +++++-----------------------------------
>  2 files changed, 28 insertions(+), 206 deletions(-)
> 
> diff --git a/security/keys/Kconfig b/security/keys/Kconfig
> index 47c041563d41..5aa442490d52 100644
> --- a/security/keys/Kconfig
> +++ b/security/keys/Kconfig
> @@ -60,9 +60,7 @@ config BIG_KEYS
>  	bool "Large payload keys"
>  	depends on KEYS
>  	depends on TMPFS
> -	select CRYPTO
> -	select CRYPTO_AES
> -	select CRYPTO_GCM
> +	select CRYPTO_LIB_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

The 'select CRYPTO' is actually still needed because CRYPTO_LIB_CHACHA20POLY1305
is under the CRYPTO menuconfig:

make allnoconfig
cat >> .config << EOF
CONFIG_SHMEM=y
CONFIG_TMPFS=y
CONFIG_KEYS=y
CONFIG_BIG_KEYS=y
EOF
make olddefconfig

WARNING: unmet direct dependencies detected for CRYPTO_LIB_CHACHA20POLY1305
  Depends on [n]: CRYPTO [=n] && (CRYPTO_ARCH_HAVE_LIB_CHACHA [=n] || !CRYPTO_ARCH_HAVE_LIB_CHACHA [=n]) && (CRYPTO_ARCH_HAVE_LIB_POLY1305 [=n] || !CRYPTO_ARCH_HAVE_LIB_POLY1305 [=n])
  Selected by [y]:
  - BIG_KEYS [=y] && KEYS [=y] && TMPFS [=y]


Maybe the 'source "lib/crypto/Kconfig"' in crypto/Kconfig should be moved to
lib/Kconfig so that it's under "Library routines" and the crypto library options
can be selected without the full CRYPTO framework?

But lib/crypto/libchacha.c uses crypto_xor_cpy(), and
lib/crypto/chacha20poly1305.c uses crypto_memneq().  So those functions would
first need to be pulled into lib/crypto/ too.

> @@ -265,7 +121,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);
> @@ -283,7 +139,7 @@ int big_key_preparse(struct key_preparsed_payload *prep)
>  err_enckey:
>  	kzfree(enckey);
>  error:
> -	big_key_free_buffer(buf);
> +	kvfree(buf);
>  	return ret;
>  }

There should be a 'memzero_explicit(buf, enclen);' before the above two calls to
kvfree().

Or even better these should use kvfree_sensitive(), but that hasn't been merged
yet.  It was under discussion last month:
https://lkml.kernel.org/lkml/20200407200318.11711-1-longman@redhat.com/

>  
> -		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;

-EINVAL is often unclear, since everyone uses it for everything.  How about
using -EBADMSG, which is what was used before via crypto_aead_decrypt()?

>  
>  		ret = datalen;
>  
>  		/* copy out decrypted data */
> -		memcpy(buffer, buf->virt, datalen);
> +		memcpy(buffer, buf, datalen);
>  
>  err_fput:
>  		fput(file);
>  error:
> -		big_key_free_buffer(buf);
> +		kvfree(buf);

Likewise, the buffer should be zeroed before freeing here.

- Eric

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] security/keys: rewrite big_key crypto to use Zinc
@ 2020-05-01 23:09   ` Eric Biggers
  0 siblings, 0 replies; 28+ messages in thread
From: Eric Biggers @ 2020-05-01 23:09 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: dhowells, keyrings, Andy Lutomirski, Greg KH, Linus Torvalds,
	kernel-hardening

On Fri, May 01, 2020 at 04:23:57PM -0600, Jason A. Donenfeld wrote:
> 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, so we can get rid of the
> ridiculously complex custom page allocator that big_key really doesn't
> need.
> 
> Cc: David Howells <dhowells@redhat.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: kernel-hardening@lists.openwall.com
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> I finally got around to updating this patch from the mailing list posts
> back in 2017-2018, using the library interface that we eventually merged
> in 2019. I haven't retested this for functionality, but nothing much has
> changed, so I suspect things should still be good to go.
> 
>  security/keys/Kconfig   |   4 +-
>  security/keys/big_key.c | 230 +++++-----------------------------------
>  2 files changed, 28 insertions(+), 206 deletions(-)
> 
> diff --git a/security/keys/Kconfig b/security/keys/Kconfig
> index 47c041563d41..5aa442490d52 100644
> --- a/security/keys/Kconfig
> +++ b/security/keys/Kconfig
> @@ -60,9 +60,7 @@ config BIG_KEYS
>  	bool "Large payload keys"
>  	depends on KEYS
>  	depends on TMPFS
> -	select CRYPTO
> -	select CRYPTO_AES
> -	select CRYPTO_GCM
> +	select CRYPTO_LIB_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

The 'select CRYPTO' is actually still needed because CRYPTO_LIB_CHACHA20POLY1305
is under the CRYPTO menuconfig:

make allnoconfig
cat >> .config << EOF
CONFIG_SHMEM=y
CONFIG_TMPFS=y
CONFIG_KEYS=y
CONFIG_BIG_KEYS=y
EOF
make olddefconfig

WARNING: unmet direct dependencies detected for CRYPTO_LIB_CHACHA20POLY1305
  Depends on [n]: CRYPTO [=n] && (CRYPTO_ARCH_HAVE_LIB_CHACHA [=n] || !CRYPTO_ARCH_HAVE_LIB_CHACHA [=n]) && (CRYPTO_ARCH_HAVE_LIB_POLY1305 [=n] || !CRYPTO_ARCH_HAVE_LIB_POLY1305 [=n])
  Selected by [y]:
  - BIG_KEYS [=y] && KEYS [=y] && TMPFS [=y]


Maybe the 'source "lib/crypto/Kconfig"' in crypto/Kconfig should be moved to
lib/Kconfig so that it's under "Library routines" and the crypto library options
can be selected without the full CRYPTO framework?

But lib/crypto/libchacha.c uses crypto_xor_cpy(), and
lib/crypto/chacha20poly1305.c uses crypto_memneq().  So those functions would
first need to be pulled into lib/crypto/ too.

> @@ -265,7 +121,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);
> @@ -283,7 +139,7 @@ int big_key_preparse(struct key_preparsed_payload *prep)
>  err_enckey:
>  	kzfree(enckey);
>  error:
> -	big_key_free_buffer(buf);
> +	kvfree(buf);
>  	return ret;
>  }

There should be a 'memzero_explicit(buf, enclen);' before the above two calls to
kvfree().

Or even better these should use kvfree_sensitive(), but that hasn't been merged
yet.  It was under discussion last month:
https://lkml.kernel.org/lkml/20200407200318.11711-1-longman@redhat.com/

>  
> -		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;

-EINVAL is often unclear, since everyone uses it for everything.  How about
using -EBADMSG, which is what was used before via crypto_aead_decrypt()?

>  
>  		ret = datalen;
>  
>  		/* copy out decrypted data */
> -		memcpy(buffer, buf->virt, datalen);
> +		memcpy(buffer, buf, datalen);
>  
>  err_fput:
>  		fput(file);
>  error:
> -		big_key_free_buffer(buf);
> +		kvfree(buf);

Likewise, the buffer should be zeroed before freeing here.

- Eric

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] security/keys: rewrite big_key crypto to use Zinc
  2020-05-01 23:09   ` Eric Biggers
@ 2020-05-02  0:06     ` Jason A. Donenfeld
  -1 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-02  0:06 UTC (permalink / raw)
  To: Eric Biggers
  Cc: David Howells, keyrings, Andy Lutomirski, Greg KH,
	Linus Torvalds, kernel-hardening

Hey Eric,

Thanks for the review.

I'll add `select CONFIG` as you suggested. I agree about trying to
move as much as possible out of crypto and into lib/crypto. Breaking
those dependency cycles won't be easy but perhaps it'll be possible to
chip away at that gradually. (I'd also lib a
lib/crypto/arch/{arch}/..., but I guess that's a separate discussion.)

I'll also change -EINVAL to -EBADMSG. Nice catch.

Regarding the buffer zeroing... are you sure? These buffers are
already being copied into filesystem caches and all sorts of places
over which we have zero control. At that point, does it matter? Or do
you argue that because it's still technically key material, we should
zero out both the plaintext and ciphertext everywhere we can, and
hopefully at some point the places where we can't will go away? IOW,
I'm fine doing that, but would like to learn your explicit reasoning
before.

Jason

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] security/keys: rewrite big_key crypto to use Zinc
@ 2020-05-02  0:06     ` Jason A. Donenfeld
  0 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-02  0:06 UTC (permalink / raw)
  To: Eric Biggers
  Cc: David Howells, keyrings, Andy Lutomirski, Greg KH,
	Linus Torvalds, kernel-hardening

Hey Eric,

Thanks for the review.

I'll add `select CONFIG` as you suggested. I agree about trying to
move as much as possible out of crypto and into lib/crypto. Breaking
those dependency cycles won't be easy but perhaps it'll be possible to
chip away at that gradually. (I'd also lib a
lib/crypto/arch/{arch}/..., but I guess that's a separate discussion.)

I'll also change -EINVAL to -EBADMSG. Nice catch.

Regarding the buffer zeroing... are you sure? These buffers are
already being copied into filesystem caches and all sorts of places
over which we have zero control. At that point, does it matter? Or do
you argue that because it's still technically key material, we should
zero out both the plaintext and ciphertext everywhere we can, and
hopefully at some point the places where we can't will go away? IOW,
I'm fine doing that, but would like to learn your explicit reasoning
before.

Jason

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] security/keys: rewrite big_key crypto to use Zinc
  2020-05-02  0:06     ` Jason A. Donenfeld
@ 2020-05-02  0:14       ` Eric Biggers
  -1 siblings, 0 replies; 28+ messages in thread
From: Eric Biggers @ 2020-05-02  0:14 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: David Howells, keyrings, Andy Lutomirski, Greg KH,
	Linus Torvalds, kernel-hardening

On Fri, May 01, 2020 at 06:06:17PM -0600, Jason A. Donenfeld wrote:
> Hey Eric,
> 
> Thanks for the review.
> 
> I'll add `select CONFIG` as you suggested. I agree about trying to
> move as much as possible out of crypto and into lib/crypto. Breaking
> those dependency cycles won't be easy but perhaps it'll be possible to
> chip away at that gradually. (I'd also lib a
> lib/crypto/arch/{arch}/..., but I guess that's a separate discussion.)
> 
> I'll also change -EINVAL to -EBADMSG. Nice catch.
> 
> Regarding the buffer zeroing... are you sure? These buffers are
> already being copied into filesystem caches and all sorts of places
> over which we have zero control. At that point, does it matter? Or do
> you argue that because it's still technically key material, we should
> zero out both the plaintext and ciphertext everywhere we can, and
> hopefully at some point the places where we can't will go away? IOW,
> I'm fine doing that, but would like to learn your explicit reasoning
> before.

It's true that the buffer zeroing doesn't matter in big_key_preparse() because
the buffer only holds the encrypted key (which is what the shmem file will
contain).  But in big_key_read(), the buffer holds the decrypted key.  So it's
at least needed there.  Having it in both places for consistency might be a good
idea.

- Eric

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] security/keys: rewrite big_key crypto to use Zinc
@ 2020-05-02  0:14       ` Eric Biggers
  0 siblings, 0 replies; 28+ messages in thread
From: Eric Biggers @ 2020-05-02  0:14 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: David Howells, keyrings, Andy Lutomirski, Greg KH,
	Linus Torvalds, kernel-hardening

On Fri, May 01, 2020 at 06:06:17PM -0600, Jason A. Donenfeld wrote:
> Hey Eric,
> 
> Thanks for the review.
> 
> I'll add `select CONFIG` as you suggested. I agree about trying to
> move as much as possible out of crypto and into lib/crypto. Breaking
> those dependency cycles won't be easy but perhaps it'll be possible to
> chip away at that gradually. (I'd also lib a
> lib/crypto/arch/{arch}/..., but I guess that's a separate discussion.)
> 
> I'll also change -EINVAL to -EBADMSG. Nice catch.
> 
> Regarding the buffer zeroing... are you sure? These buffers are
> already being copied into filesystem caches and all sorts of places
> over which we have zero control. At that point, does it matter? Or do
> you argue that because it's still technically key material, we should
> zero out both the plaintext and ciphertext everywhere we can, and
> hopefully at some point the places where we can't will go away? IOW,
> I'm fine doing that, but would like to learn your explicit reasoning
> before.

It's true that the buffer zeroing doesn't matter in big_key_preparse() because
the buffer only holds the encrypted key (which is what the shmem file will
contain).  But in big_key_read(), the buffer holds the decrypted key.  So it's
at least needed there.  Having it in both places for consistency might be a good
idea.

- Eric

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] security/keys: rewrite big_key crypto to use Zinc
  2020-05-02  0:14       ` Eric Biggers
@ 2020-05-02  0:15         ` Jason A. Donenfeld
  -1 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-02  0:15 UTC (permalink / raw)
  To: Eric Biggers
  Cc: David Howells, keyrings, Andy Lutomirski, Greg KH,
	Linus Torvalds, kernel-hardening

On Fri, May 1, 2020 at 6:14 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Fri, May 01, 2020 at 06:06:17PM -0600, Jason A. Donenfeld wrote:
> > Hey Eric,
> >
> > Thanks for the review.
> >
> > I'll add `select CONFIG` as you suggested. I agree about trying to
> > move as much as possible out of crypto and into lib/crypto. Breaking
> > those dependency cycles won't be easy but perhaps it'll be possible to
> > chip away at that gradually. (I'd also lib a
> > lib/crypto/arch/{arch}/..., but I guess that's a separate discussion.)
> >
> > I'll also change -EINVAL to -EBADMSG. Nice catch.
> >
> > Regarding the buffer zeroing... are you sure? These buffers are
> > already being copied into filesystem caches and all sorts of places
> > over which we have zero control. At that point, does it matter? Or do
> > you argue that because it's still technically key material, we should
> > zero out both the plaintext and ciphertext everywhere we can, and
> > hopefully at some point the places where we can't will go away? IOW,
> > I'm fine doing that, but would like to learn your explicit reasoning
> > before.
>
> It's true that the buffer zeroing doesn't matter in big_key_preparse() because
> the buffer only holds the encrypted key (which is what the shmem file will
> contain).  But in big_key_read(), the buffer holds the decrypted key.  So it's
> at least needed there.  Having it in both places for consistency might be a good
> idea.

Alright. v2 coming your way shortly.


Jason

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] security/keys: rewrite big_key crypto to use Zinc
@ 2020-05-02  0:15         ` Jason A. Donenfeld
  0 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-02  0:15 UTC (permalink / raw)
  To: Eric Biggers
  Cc: David Howells, keyrings, Andy Lutomirski, Greg KH,
	Linus Torvalds, kernel-hardening

On Fri, May 1, 2020 at 6:14 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Fri, May 01, 2020 at 06:06:17PM -0600, Jason A. Donenfeld wrote:
> > Hey Eric,
> >
> > Thanks for the review.
> >
> > I'll add `select CONFIG` as you suggested. I agree about trying to
> > move as much as possible out of crypto and into lib/crypto. Breaking
> > those dependency cycles won't be easy but perhaps it'll be possible to
> > chip away at that gradually. (I'd also lib a
> > lib/crypto/arch/{arch}/..., but I guess that's a separate discussion.)
> >
> > I'll also change -EINVAL to -EBADMSG. Nice catch.
> >
> > Regarding the buffer zeroing... are you sure? These buffers are
> > already being copied into filesystem caches and all sorts of places
> > over which we have zero control. At that point, does it matter? Or do
> > you argue that because it's still technically key material, we should
> > zero out both the plaintext and ciphertext everywhere we can, and
> > hopefully at some point the places where we can't will go away? IOW,
> > I'm fine doing that, but would like to learn your explicit reasoning
> > before.
>
> It's true that the buffer zeroing doesn't matter in big_key_preparse() because
> the buffer only holds the encrypted key (which is what the shmem file will
> contain).  But in big_key_read(), the buffer holds the decrypted key.  So it's
> at least needed there.  Having it in both places for consistency might be a good
> idea.

Alright. v2 coming your way shortly.


Jason

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2] security/keys: rewrite big_key crypto to use Zinc
  2020-05-02  0:15         ` Jason A. Donenfeld
@ 2020-05-02  0:19           ` Jason A. Donenfeld
  -1 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-02  0:19 UTC (permalink / raw)
  To: dhowells, keyrings
  Cc: Jason A. Donenfeld, Andy Lutomirski, Greg KH, Linus Torvalds,
	Eric Biggers, kernel-hardening

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, so we can get rid of the
ridiculously complex custom page allocator that big_key really doesn't
need.

Cc: David Howells <dhowells@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Eric Biggers <ebiggers@kernel.org>
Cc: kernel-hardening@lists.openwall.com
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Changes v1->v2:
 - [Eric] Return -EBADMSG instead of -EINVAL if the authtag fails.
 - [Eric] Select CONFIG_CRYPTO, since it's required by the LIB selection.
 - [Eric] Zero out buffers that formerly contained either plaintext or
   ciphertext keys.
 - [Jason] If kernel_read() fails, return that error value, instead of
   relying on the subsequent decryption to fail.

Note v1:
 I finally got around to updating this patch from the mailing list posts
 back in 2017-2018, using the library interface that we eventually
 merged in 2019. I haven't retested this for functionality, but nothing
 much has changed, so I suspect things should still be good to go.

 security/keys/Kconfig   |   3 +-
 security/keys/big_key.c | 235 ++++++----------------------------------
 2 files changed, 33 insertions(+), 205 deletions(-)

diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index 47c041563d41..7da6c1b496f9 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -61,8 +61,7 @@ config BIG_KEYS
 	depends on KEYS
 	depends on TMPFS
 	select CRYPTO
-	select CRYPTO_AES
-	select CRYPTO_GCM
+	select CRYPTO_LIB_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 82008f900930..3879fe5a5e94 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /* Large capacity key type
  *
- * Copyright (C) 2017 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
+ * Copyright (C) 2017-2020 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)
  */
@@ -12,20 +12,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 <crypto/chacha20poly1305.h>
 
 /*
  * Layout of key payload words.
@@ -37,14 +27,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
@@ -52,16 +34,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
@@ -75,136 +47,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_AUTHTAG_SIZE;
 	int ret;
 
 	if (datalen <= 0 || datalen > 1024 * 1024 || !prep->data)
@@ -220,28 +76,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_KEY_SIZE, GFP_KERNEL);
 		if (!enckey) {
 			ret = -ENOMEM;
 			goto error;
 		}
-		ret = get_random_bytes_wait(enckey, ENC_KEY_SIZE);
+		ret = get_random_bytes_wait(enckey, CHACHA20POLY1305_KEY_SIZE);
 		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);
@@ -250,7 +106,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)
@@ -265,7 +121,8 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 		*path = file->f_path;
 		path_get(path);
 		fput(file);
-		big_key_free_buffer(buf);
+		memzero_explicit(buf, enclen);
+		kvfree(buf);
 	} else {
 		/* Just store the data in a buffer */
 		void *data = kmalloc(datalen, GFP_KERNEL);
@@ -283,7 +140,8 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 err_enckey:
 	kzfree(enckey);
 error:
-	big_key_free_buffer(buf);
+	memzero_explicit(buf, enclen);
+	kvfree(buf);
 	return ret;
 }
 
@@ -361,14 +219,13 @@ long big_key_read(const struct key *key, char *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_AUTHTAG_SIZE;
 		loff_t pos = 0;
 
-		buf = big_key_alloc_buffer(enclen);
+		buf = kvmalloc(enclen, GFP_KERNEL);
 		if (!buf)
 			return -ENOMEM;
 
@@ -379,25 +236,29 @@ long big_key_read(const struct key *key, char *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;
+		} else if (ret < 0) {
+			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 : -EBADMSG;
+		if (unlikely(ret))
 			goto err_fput;
 
 		ret = datalen;
 
 		/* copy out decrypted data */
-		memcpy(buffer, buf->virt, datalen);
+		memcpy(buffer, buf, datalen);
 
 err_fput:
 		fput(file);
 error:
-		big_key_free_buffer(buf);
+		memzero_explicit(buf, enclen);
+		kvfree(buf);
 	} else {
 		ret = datalen;
 		memcpy(buffer, key->payload.data[big_key_data], datalen);
@@ -411,39 +272,7 @@ long big_key_read(const struct key *key, char *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);
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v2] security/keys: rewrite big_key crypto to use Zinc
@ 2020-05-02  0:19           ` Jason A. Donenfeld
  0 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-02  0:19 UTC (permalink / raw)
  To: dhowells, keyrings
  Cc: Jason A. Donenfeld, Andy Lutomirski, Greg KH, Linus Torvalds,
	Eric Biggers, kernel-hardening

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, so we can get rid of the
ridiculously complex custom page allocator that big_key really doesn't
need.

Cc: David Howells <dhowells@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Eric Biggers <ebiggers@kernel.org>
Cc: kernel-hardening@lists.openwall.com
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Changes v1->v2:
 - [Eric] Return -EBADMSG instead of -EINVAL if the authtag fails.
 - [Eric] Select CONFIG_CRYPTO, since it's required by the LIB selection.
 - [Eric] Zero out buffers that formerly contained either plaintext or
   ciphertext keys.
 - [Jason] If kernel_read() fails, return that error value, instead of
   relying on the subsequent decryption to fail.

Note v1:
 I finally got around to updating this patch from the mailing list posts
 back in 2017-2018, using the library interface that we eventually
 merged in 2019. I haven't retested this for functionality, but nothing
 much has changed, so I suspect things should still be good to go.

 security/keys/Kconfig   |   3 +-
 security/keys/big_key.c | 235 ++++++----------------------------------
 2 files changed, 33 insertions(+), 205 deletions(-)

diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index 47c041563d41..7da6c1b496f9 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -61,8 +61,7 @@ config BIG_KEYS
 	depends on KEYS
 	depends on TMPFS
 	select CRYPTO
-	select CRYPTO_AES
-	select CRYPTO_GCM
+	select CRYPTO_LIB_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 82008f900930..3879fe5a5e94 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /* Large capacity key type
  *
- * Copyright (C) 2017 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
+ * Copyright (C) 2017-2020 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)
  */
@@ -12,20 +12,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 <crypto/chacha20poly1305.h>
 
 /*
  * Layout of key payload words.
@@ -37,14 +27,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
@@ -52,16 +34,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
@@ -75,136 +47,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_AUTHTAG_SIZE;
 	int ret;
 
 	if (datalen <= 0 || datalen > 1024 * 1024 || !prep->data)
@@ -220,28 +76,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_KEY_SIZE, GFP_KERNEL);
 		if (!enckey) {
 			ret = -ENOMEM;
 			goto error;
 		}
-		ret = get_random_bytes_wait(enckey, ENC_KEY_SIZE);
+		ret = get_random_bytes_wait(enckey, CHACHA20POLY1305_KEY_SIZE);
 		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);
@@ -250,7 +106,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)
@@ -265,7 +121,8 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 		*path = file->f_path;
 		path_get(path);
 		fput(file);
-		big_key_free_buffer(buf);
+		memzero_explicit(buf, enclen);
+		kvfree(buf);
 	} else {
 		/* Just store the data in a buffer */
 		void *data = kmalloc(datalen, GFP_KERNEL);
@@ -283,7 +140,8 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 err_enckey:
 	kzfree(enckey);
 error:
-	big_key_free_buffer(buf);
+	memzero_explicit(buf, enclen);
+	kvfree(buf);
 	return ret;
 }
 
@@ -361,14 +219,13 @@ long big_key_read(const struct key *key, char *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_AUTHTAG_SIZE;
 		loff_t pos = 0;
 
-		buf = big_key_alloc_buffer(enclen);
+		buf = kvmalloc(enclen, GFP_KERNEL);
 		if (!buf)
 			return -ENOMEM;
 
@@ -379,25 +236,29 @@ long big_key_read(const struct key *key, char *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;
+		} else if (ret < 0) {
+			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 : -EBADMSG;
+		if (unlikely(ret))
 			goto err_fput;
 
 		ret = datalen;
 
 		/* copy out decrypted data */
-		memcpy(buffer, buf->virt, datalen);
+		memcpy(buffer, buf, datalen);
 
 err_fput:
 		fput(file);
 error:
-		big_key_free_buffer(buf);
+		memzero_explicit(buf, enclen);
+		kvfree(buf);
 	} else {
 		ret = datalen;
 		memcpy(buffer, key->payload.data[big_key_data], datalen);
@@ -411,39 +272,7 @@ long big_key_read(const struct key *key, char *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);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v2] security/keys: rewrite big_key crypto to use Zinc
  2020-05-02  0:19           ` Jason A. Donenfeld
@ 2020-05-10 21:17             ` Eric Biggers
  -1 siblings, 0 replies; 28+ messages in thread
From: Eric Biggers @ 2020-05-10 21:17 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: dhowells, keyrings, Andy Lutomirski, Greg KH, Linus Torvalds,
	kernel-hardening

On Fri, May 01, 2020 at 06:19:42PM -0600, Jason A. Donenfeld wrote:
> 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, so we can get rid of the
> ridiculously complex custom page allocator that big_key really doesn't
> need.
> 
> Cc: David Howells <dhowells@redhat.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Eric Biggers <ebiggers@kernel.org>
> Cc: kernel-hardening@lists.openwall.com
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

You can add:

	Reviewed-by: Eric Biggers <ebiggers@google.com>

But, a couple minor suggestions:

The commit message should say "lib/crypto", not Zinc.  Nothing in the source
tree actually says Zinc, so it will confuse people who haven't read all the
previous discussions.

>  		/* 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;
> +		} else if (ret < 0) {
> +			goto err_fput;
>  		}

It would make sense to write this as the following, to make it consistent with
how the return value of kernel_write() is checked in big_key_preparse():

		ret = kernel_read(file, buf, enclen, &pos);
		if (ret != enclen) {
			if (ret >= 0)
				ret = -ENOMEM;
			goto err_fput;
		}

- Eric

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2] security/keys: rewrite big_key crypto to use Zinc
@ 2020-05-10 21:17             ` Eric Biggers
  0 siblings, 0 replies; 28+ messages in thread
From: Eric Biggers @ 2020-05-10 21:17 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: dhowells, keyrings, Andy Lutomirski, Greg KH, Linus Torvalds,
	kernel-hardening

On Fri, May 01, 2020 at 06:19:42PM -0600, Jason A. Donenfeld wrote:
> 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, so we can get rid of the
> ridiculously complex custom page allocator that big_key really doesn't
> need.
> 
> Cc: David Howells <dhowells@redhat.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Eric Biggers <ebiggers@kernel.org>
> Cc: kernel-hardening@lists.openwall.com
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

You can add:

	Reviewed-by: Eric Biggers <ebiggers@google.com>

But, a couple minor suggestions:

The commit message should say "lib/crypto", not Zinc.  Nothing in the source
tree actually says Zinc, so it will confuse people who haven't read all the
previous discussions.

>  		/* 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;
> +		} else if (ret < 0) {
> +			goto err_fput;
>  		}

It would make sense to write this as the following, to make it consistent with
how the return value of kernel_write() is checked in big_key_preparse():

		ret = kernel_read(file, buf, enclen, &pos);
		if (ret != enclen) {
			if (ret >= 0)
				ret = -ENOMEM;
			goto err_fput;
		}

- Eric

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2] security/keys: rewrite big_key crypto to use Zinc
  2020-05-10 21:17             ` Eric Biggers
@ 2020-05-11 21:41               ` Jason A. Donenfeld
  -1 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-11 21:41 UTC (permalink / raw)
  To: Eric Biggers
  Cc: David Howells, keyrings, Andy Lutomirski, Greg KH,
	Linus Torvalds, kernel-hardening

On Sun, May 10, 2020 at 3:17 PM Eric Biggers <ebiggers@kernel.org> wrote:
> The commit message should say "lib/crypto", not Zinc.  Nothing in the source
> tree actually says Zinc, so it will confuse people who haven't read all the
> previous discussions.

Old commit message from a few years ago. Will adjust.

>
> >               /* 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;
> > +             } else if (ret < 0) {
> > +                     goto err_fput;
> >               }
>
> It would make sense to write this as the following, to make it consistent with
> how the return value of kernel_write() is checked in big_key_preparse():
>
>                 ret = kernel_read(file, buf, enclen, &pos);
>                 if (ret != enclen) {
>                         if (ret >= 0)
>                                 ret = -ENOMEM;
>                         goto err_fput;
>                 }

Will do, and will send a v3 with your reviewed-by.

Jason

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2] security/keys: rewrite big_key crypto to use Zinc
@ 2020-05-11 21:41               ` Jason A. Donenfeld
  0 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-11 21:41 UTC (permalink / raw)
  To: Eric Biggers
  Cc: David Howells, keyrings, Andy Lutomirski, Greg KH,
	Linus Torvalds, kernel-hardening

On Sun, May 10, 2020 at 3:17 PM Eric Biggers <ebiggers@kernel.org> wrote:
> The commit message should say "lib/crypto", not Zinc.  Nothing in the source
> tree actually says Zinc, so it will confuse people who haven't read all the
> previous discussions.

Old commit message from a few years ago. Will adjust.

>
> >               /* 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;
> > +             } else if (ret < 0) {
> > +                     goto err_fput;
> >               }
>
> It would make sense to write this as the following, to make it consistent with
> how the return value of kernel_write() is checked in big_key_preparse():
>
>                 ret = kernel_read(file, buf, enclen, &pos);
>                 if (ret != enclen) {
>                         if (ret >= 0)
>                                 ret = -ENOMEM;
>                         goto err_fput;
>                 }

Will do, and will send a v3 with your reviewed-by.

Jason

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3] security/keys: rewrite big_key crypto to use library interface
  2020-05-11 21:41               ` Jason A. Donenfeld
@ 2020-05-11 21:51                 ` Jason A. Donenfeld
  -1 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-11 21:51 UTC (permalink / raw)
  To: dhowells, keyrings, linux-kernel
  Cc: Jason A. Donenfeld, Andy Lutomirski, Greg KH, Linus Torvalds,
	kernel-hardening, Eric Biggers

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 the simpler ChaCha20Poly1305
library 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, so we can get rid
of the ridiculously complex custom page allocator that big_key really
doesn't need.

Cc: David Howells <dhowells@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: kernel-hardening@lists.openwall.com
Reviewed-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Changes v2->v3:
 - [Eric] Unify kernel_read/write handling in big_key_preparse and
   big_key_read.
 - [Eric] Update commit message.

Changes v1->v2:
 - [Eric] Return -EBADMSG instead of -EINVAL if the authtag fails.
 - [Eric] Select CONFIG_CRYPTO, since it's required by the LIB selection.
 - [Eric] Zero out buffers that formerly contained either plaintext or
   ciphertext keys.
 - [Jason] If kernel_read() fails, return that error value, instead of
   relying on the subsequent decryption to fail.

Note v1:
 I finally got around to updating this patch from the mailing list posts
 back in 2017-2018, using the library interface that we eventually
 merged in 2019. I haven't retested this for functionality, but nothing
 much has changed, so I suspect things should still be good to go.

 security/keys/Kconfig   |   3 +-
 security/keys/big_key.c | 240 ++++++----------------------------------
 2 files changed, 35 insertions(+), 208 deletions(-)

diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index 47c041563d41..7da6c1b496f9 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -61,8 +61,7 @@ config BIG_KEYS
 	depends on KEYS
 	depends on TMPFS
 	select CRYPTO
-	select CRYPTO_AES
-	select CRYPTO_GCM
+	select CRYPTO_LIB_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 82008f900930..d43f3daab2b8 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /* Large capacity key type
  *
- * Copyright (C) 2017 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
+ * Copyright (C) 2017-2020 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)
  */
@@ -12,20 +12,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 <crypto/chacha20poly1305.h>
 
 /*
  * Layout of key payload words.
@@ -37,14 +27,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
@@ -52,16 +34,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
@@ -75,136 +47,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_AUTHTAG_SIZE;
 	int ret;
 
 	if (datalen <= 0 || datalen > 1024 * 1024 || !prep->data)
@@ -220,28 +76,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_KEY_SIZE, GFP_KERNEL);
 		if (!enckey) {
 			ret = -ENOMEM;
 			goto error;
 		}
-		ret = get_random_bytes_wait(enckey, ENC_KEY_SIZE);
+		ret = get_random_bytes_wait(enckey, CHACHA20POLY1305_KEY_SIZE);
 		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);
@@ -250,11 +106,11 @@ 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)
-				ret = -ENOMEM;
+				ret = -EIO;
 			goto err_fput;
 		}
 
@@ -265,7 +121,8 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 		*path = file->f_path;
 		path_get(path);
 		fput(file);
-		big_key_free_buffer(buf);
+		memzero_explicit(buf, enclen);
+		kvfree(buf);
 	} else {
 		/* Just store the data in a buffer */
 		void *data = kmalloc(datalen, GFP_KERNEL);
@@ -283,7 +140,8 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 err_enckey:
 	kzfree(enckey);
 error:
-	big_key_free_buffer(buf);
+	memzero_explicit(buf, enclen);
+	kvfree(buf);
 	return ret;
 }
 
@@ -361,14 +219,13 @@ long big_key_read(const struct key *key, char *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_AUTHTAG_SIZE;
 		loff_t pos = 0;
 
-		buf = big_key_alloc_buffer(enclen);
+		buf = kvmalloc(enclen, GFP_KERNEL);
 		if (!buf)
 			return -ENOMEM;
 
@@ -379,25 +236,28 @@ long big_key_read(const struct key *key, char *buffer, size_t buflen)
 		}
 
 		/* read file to kernel and decrypt */
-		ret = kernel_read(file, buf->virt, enclen, &pos);
-		if (ret >= 0 && ret != enclen) {
-			ret = -EIO;
+		ret = kernel_read(file, buf, enclen, &pos);
+		if (ret != enclen) {
+			if (ret >= 0)
+				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 : -EBADMSG;
+		if (unlikely(ret))
 			goto err_fput;
 
 		ret = datalen;
 
 		/* copy out decrypted data */
-		memcpy(buffer, buf->virt, datalen);
+		memcpy(buffer, buf, datalen);
 
 err_fput:
 		fput(file);
 error:
-		big_key_free_buffer(buf);
+		memzero_explicit(buf, enclen);
+		kvfree(buf);
 	} else {
 		ret = datalen;
 		memcpy(buffer, key->payload.data[big_key_data], datalen);
@@ -411,39 +271,7 @@ long big_key_read(const struct key *key, char *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);
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v3] security/keys: rewrite big_key crypto to use library interface
@ 2020-05-11 21:51                 ` Jason A. Donenfeld
  0 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-11 21:51 UTC (permalink / raw)
  To: dhowells, keyrings, linux-kernel
  Cc: Jason A. Donenfeld, Andy Lutomirski, Greg KH, Linus Torvalds,
	kernel-hardening, Eric Biggers

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 the simpler ChaCha20Poly1305
library 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, so we can get rid
of the ridiculously complex custom page allocator that big_key really
doesn't need.

Cc: David Howells <dhowells@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: kernel-hardening@lists.openwall.com
Reviewed-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Changes v2->v3:
 - [Eric] Unify kernel_read/write handling in big_key_preparse and
   big_key_read.
 - [Eric] Update commit message.

Changes v1->v2:
 - [Eric] Return -EBADMSG instead of -EINVAL if the authtag fails.
 - [Eric] Select CONFIG_CRYPTO, since it's required by the LIB selection.
 - [Eric] Zero out buffers that formerly contained either plaintext or
   ciphertext keys.
 - [Jason] If kernel_read() fails, return that error value, instead of
   relying on the subsequent decryption to fail.

Note v1:
 I finally got around to updating this patch from the mailing list posts
 back in 2017-2018, using the library interface that we eventually
 merged in 2019. I haven't retested this for functionality, but nothing
 much has changed, so I suspect things should still be good to go.

 security/keys/Kconfig   |   3 +-
 security/keys/big_key.c | 240 ++++++----------------------------------
 2 files changed, 35 insertions(+), 208 deletions(-)

diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index 47c041563d41..7da6c1b496f9 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -61,8 +61,7 @@ config BIG_KEYS
 	depends on KEYS
 	depends on TMPFS
 	select CRYPTO
-	select CRYPTO_AES
-	select CRYPTO_GCM
+	select CRYPTO_LIB_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 82008f900930..d43f3daab2b8 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /* Large capacity key type
  *
- * Copyright (C) 2017 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
+ * Copyright (C) 2017-2020 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)
  */
@@ -12,20 +12,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 <crypto/chacha20poly1305.h>
 
 /*
  * Layout of key payload words.
@@ -37,14 +27,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
@@ -52,16 +34,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
@@ -75,136 +47,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_AUTHTAG_SIZE;
 	int ret;
 
 	if (datalen <= 0 || datalen > 1024 * 1024 || !prep->data)
@@ -220,28 +76,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_KEY_SIZE, GFP_KERNEL);
 		if (!enckey) {
 			ret = -ENOMEM;
 			goto error;
 		}
-		ret = get_random_bytes_wait(enckey, ENC_KEY_SIZE);
+		ret = get_random_bytes_wait(enckey, CHACHA20POLY1305_KEY_SIZE);
 		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);
@@ -250,11 +106,11 @@ 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)
-				ret = -ENOMEM;
+				ret = -EIO;
 			goto err_fput;
 		}
 
@@ -265,7 +121,8 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 		*path = file->f_path;
 		path_get(path);
 		fput(file);
-		big_key_free_buffer(buf);
+		memzero_explicit(buf, enclen);
+		kvfree(buf);
 	} else {
 		/* Just store the data in a buffer */
 		void *data = kmalloc(datalen, GFP_KERNEL);
@@ -283,7 +140,8 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 err_enckey:
 	kzfree(enckey);
 error:
-	big_key_free_buffer(buf);
+	memzero_explicit(buf, enclen);
+	kvfree(buf);
 	return ret;
 }
 
@@ -361,14 +219,13 @@ long big_key_read(const struct key *key, char *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_AUTHTAG_SIZE;
 		loff_t pos = 0;
 
-		buf = big_key_alloc_buffer(enclen);
+		buf = kvmalloc(enclen, GFP_KERNEL);
 		if (!buf)
 			return -ENOMEM;
 
@@ -379,25 +236,28 @@ long big_key_read(const struct key *key, char *buffer, size_t buflen)
 		}
 
 		/* read file to kernel and decrypt */
-		ret = kernel_read(file, buf->virt, enclen, &pos);
-		if (ret >= 0 && ret != enclen) {
-			ret = -EIO;
+		ret = kernel_read(file, buf, enclen, &pos);
+		if (ret != enclen) {
+			if (ret >= 0)
+				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 : -EBADMSG;
+		if (unlikely(ret))
 			goto err_fput;
 
 		ret = datalen;
 
 		/* copy out decrypted data */
-		memcpy(buffer, buf->virt, datalen);
+		memcpy(buffer, buf, datalen);
 
 err_fput:
 		fput(file);
 error:
-		big_key_free_buffer(buf);
+		memzero_explicit(buf, enclen);
+		kvfree(buf);
 	} else {
 		ret = datalen;
 		memcpy(buffer, key->payload.data[big_key_data], datalen);
@@ -411,39 +271,7 @@ long big_key_read(const struct key *key, char *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);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v3] security/keys: rewrite big_key crypto to use library interface
  2020-05-11 21:41               ` Jason A. Donenfeld
@ 2020-05-12 13:17                 ` David Howells
  -1 siblings, 0 replies; 28+ messages in thread
From: David Howells @ 2020-05-12 13:17 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: dhowells, keyrings, linux-kernel, Andy Lutomirski, Greg KH,
	Linus Torvalds, kernel-hardening, Eric Biggers

Jason A. Donenfeld <Jason@zx2c4.com> wrote:

> -	/* no ->update(); don't add it without changing big_key_crypt() nonce */
> +	/* no ->update(); don't add it without changing chacha20poly1305's nonce

Note that ->update() doesn't have to modify the contents of the key, but can
just rather replace them entirely.  See attached.  The actual work is done in
big_key_preparse(); all big_key_update() has to do is apply it and dispose of
the old payload.

David
---
commit 724e76c185d517f35ead4f72f9958850c9218f4d
Author: David Howells <dhowells@redhat.com>
Date:   Tue May 12 14:03:53 2020 +0100

    keys: Implement update for the big_key type
    
    Implement the ->update op for the big_key type.
    
    Signed-off-by: David Howells <dhowells@redhat.com>

diff --git a/include/keys/big_key-type.h b/include/keys/big_key-type.h
index 3fee04f81439..988d90d77f53 100644
--- a/include/keys/big_key-type.h
+++ b/include/keys/big_key-type.h
@@ -18,5 +18,6 @@ extern void big_key_revoke(struct key *key);
 extern void big_key_destroy(struct key *key);
 extern void big_key_describe(const struct key *big_key, struct seq_file *m);
 extern long big_key_read(const struct key *key, char *buffer, size_t buflen);
+extern int big_key_update(struct key *key, struct key_preparsed_payload *prep);
 
 #endif /* _KEYS_BIG_KEY_TYPE_H */
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index d43f3daab2b8..dd708e8f13c0 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -47,7 +47,7 @@ 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 chacha20poly1305's nonce */
+	.update			= big_key_update,
 };
 
 /*
@@ -191,6 +191,23 @@ void big_key_destroy(struct key *key)
 	key->payload.data[big_key_data] = NULL;
 }
 
+/*
+ * Update a big key
+ */
+int big_key_update(struct key *key, struct key_preparsed_payload *prep)
+{
+	int ret;
+
+	ret = key_payload_reserve(key, prep->datalen);
+	if (ret < 0)
+		return ret;
+
+	if (key_is_positive(key))
+		big_key_destroy(key);
+
+	return generic_key_instantiate(key, prep);
+}
+
 /*
  * describe the big_key key
  */

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v3] security/keys: rewrite big_key crypto to use library interface
@ 2020-05-12 13:17                 ` David Howells
  0 siblings, 0 replies; 28+ messages in thread
From: David Howells @ 2020-05-12 13:17 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: dhowells, keyrings, linux-kernel, Andy Lutomirski, Greg KH,
	Linus Torvalds, kernel-hardening, Eric Biggers

Jason A. Donenfeld <Jason@zx2c4.com> wrote:

> -	/* no ->update(); don't add it without changing big_key_crypt() nonce */
> +	/* no ->update(); don't add it without changing chacha20poly1305's nonce

Note that ->update() doesn't have to modify the contents of the key, but can
just rather replace them entirely.  See attached.  The actual work is done in
big_key_preparse(); all big_key_update() has to do is apply it and dispose of
the old payload.

David
---
commit 724e76c185d517f35ead4f72f9958850c9218f4d
Author: David Howells <dhowells@redhat.com>
Date:   Tue May 12 14:03:53 2020 +0100

    keys: Implement update for the big_key type
    
    Implement the ->update op for the big_key type.
    
    Signed-off-by: David Howells <dhowells@redhat.com>

diff --git a/include/keys/big_key-type.h b/include/keys/big_key-type.h
index 3fee04f81439..988d90d77f53 100644
--- a/include/keys/big_key-type.h
+++ b/include/keys/big_key-type.h
@@ -18,5 +18,6 @@ extern void big_key_revoke(struct key *key);
 extern void big_key_destroy(struct key *key);
 extern void big_key_describe(const struct key *big_key, struct seq_file *m);
 extern long big_key_read(const struct key *key, char *buffer, size_t buflen);
+extern int big_key_update(struct key *key, struct key_preparsed_payload *prep);
 
 #endif /* _KEYS_BIG_KEY_TYPE_H */
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index d43f3daab2b8..dd708e8f13c0 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -47,7 +47,7 @@ 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 chacha20poly1305's nonce */
+	.update			= big_key_update,
 };
 
 /*
@@ -191,6 +191,23 @@ void big_key_destroy(struct key *key)
 	key->payload.data[big_key_data] = NULL;
 }
 
+/*
+ * Update a big key
+ */
+int big_key_update(struct key *key, struct key_preparsed_payload *prep)
+{
+	int ret;
+
+	ret = key_payload_reserve(key, prep->datalen);
+	if (ret < 0)
+		return ret;
+
+	if (key_is_positive(key))
+		big_key_destroy(key);
+
+	return generic_key_instantiate(key, prep);
+}
+
 /*
  * describe the big_key key
  */


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v3] security/keys: rewrite big_key crypto to use library interface
  2020-05-12 13:17                 ` David Howells
  (?)
@ 2020-05-12 21:38                   ` Jason A. Donenfeld
  -1 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-12 21:38 UTC (permalink / raw)
  To: David Howells
  Cc: keyrings, LKML, Andy Lutomirski, Greg KH, Linus Torvalds,
	kernel-hardening, Eric Biggers

Hi David,

So long as that ->update function:
1. Deletes the old on-disk data.
2. Deletes the old key from the inode.
3. Generates a new key using get_random_bytes.
4. Stores that new key in the inode.
5. Encrypts the updated data afresh with the new key.
6. Puts the updated data onto disk,

then this is fine with me, and feel free to have my Acked-by if you
want. But if it doesn't do that -- i.e. if it tries to reuse the old
key or similar -- then this isn't fine. But it sounds like from what
you've described that things are actually fine, in which case, I guess
it makes sense to apply your patch ontop of mine and commit these.

Jason

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3] security/keys: rewrite big_key crypto to use library interface
@ 2020-05-12 21:38                   ` Jason A. Donenfeld
  0 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-12 21:38 UTC (permalink / raw)
  To: David Howells
  Cc: keyrings, LKML, Andy Lutomirski, Greg KH, Linus Torvalds,
	kernel-hardening, Eric Biggers

Hi David,

So long as that ->update function:
1. Deletes the old on-disk data.
2. Deletes the old key from the inode.
3. Generates a new key using get_random_bytes.
4. Stores that new key in the inode.
5. Encrypts the updated data afresh with the new key.
6. Puts the updated data onto disk,

then this is fine with me, and feel free to have my Acked-by if you
want. But if it doesn't do that -- i.e. if it tries to reuse the old
key or similar -- then this isn't fine. But it sounds like from what
you've described that things are actually fine, in which case, I guess
it makes sense to apply your patch ontop of mine and commit these.

Jason

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3] security/keys: rewrite big_key crypto to use library interface
@ 2020-05-12 21:38                   ` Jason A. Donenfeld
  0 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-12 21:38 UTC (permalink / raw)
  To: David Howells
  Cc: keyrings, LKML, Andy Lutomirski, Greg KH, Linus Torvalds,
	kernel-hardening, Eric Biggers

Hi David,

So long as that ->update function:
1. Deletes the old on-disk data.
2. Deletes the old key from the inode.
3. Generates a new key using get_random_bytes.
4. Stores that new key in the inode.
5. Encrypts the updated data afresh with the new key.
6. Puts the updated data onto disk,

then this is fine with me, and feel free to have my Acked-by if you
want. But if it doesn't do that -- i.e. if it tries to reuse the old
key or similar -- then this isn't fine. But it sounds like from what
you've described that things are actually fine, in which case, I guess
it makes sense to apply your patch ontop of mine and commit these.

Jason

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3] security/keys: rewrite big_key crypto to use library interface
  2020-05-12 13:17                 ` David Howells
@ 2020-05-12 22:03                   ` David Howells
  -1 siblings, 0 replies; 28+ messages in thread
From: David Howells @ 2020-05-12 22:03 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: dhowells, keyrings, LKML, Andy Lutomirski, Greg KH,
	Linus Torvalds, kernel-hardening, Eric Biggers

Jason A. Donenfeld <Jason@zx2c4.com> wrote:

> So long as that ->update function:
> 1. Deletes the old on-disk data.
> 2. Deletes the old key from the inode.
> 3. Generates a new key using get_random_bytes.
> 4. Stores that new key in the inode.
> 5. Encrypts the updated data afresh with the new key.
> 6. Puts the updated data onto disk,
> 
> then this is fine with me, and feel free to have my Acked-by if you
> want. But if it doesn't do that -- i.e. if it tries to reuse the old
> key or similar -- then this isn't fine. But it sounds like from what
> you've described that things are actually fine, in which case, I guess
> it makes sense to apply your patch ontop of mine and commit these.

Yep.  It calls big_key_destroy(), which clears away the old stuff just as when
a key is being destroyed, then generic_key_instantiate() just as when a key is
being set up.

The key ID and the key metadata (ownership, perms, expiry) are maintained, but
the payload is just completely replaced.

David

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3] security/keys: rewrite big_key crypto to use library interface
@ 2020-05-12 22:03                   ` David Howells
  0 siblings, 0 replies; 28+ messages in thread
From: David Howells @ 2020-05-12 22:03 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: dhowells, keyrings, LKML, Andy Lutomirski, Greg KH,
	Linus Torvalds, kernel-hardening, Eric Biggers

Jason A. Donenfeld <Jason@zx2c4.com> wrote:

> So long as that ->update function:
> 1. Deletes the old on-disk data.
> 2. Deletes the old key from the inode.
> 3. Generates a new key using get_random_bytes.
> 4. Stores that new key in the inode.
> 5. Encrypts the updated data afresh with the new key.
> 6. Puts the updated data onto disk,
> 
> then this is fine with me, and feel free to have my Acked-by if you
> want. But if it doesn't do that -- i.e. if it tries to reuse the old
> key or similar -- then this isn't fine. But it sounds like from what
> you've described that things are actually fine, in which case, I guess
> it makes sense to apply your patch ontop of mine and commit these.

Yep.  It calls big_key_destroy(), which clears away the old stuff just as when
a key is being destroyed, then generic_key_instantiate() just as when a key is
being set up.

The key ID and the key metadata (ownership, perms, expiry) are maintained, but
the payload is just completely replaced.

David


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3] security/keys: rewrite big_key crypto to use library interface
  2020-05-12 22:03                   ` David Howells
  (?)
@ 2020-05-13  2:33                     ` Jason A. Donenfeld
  -1 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-13  2:33 UTC (permalink / raw)
  To: David Howells
  Cc: keyrings, LKML, Andy Lutomirski, Greg KH, Linus Torvalds,
	kernel-hardening, Eric Biggers

On Tue, May 12, 2020 at 4:03 PM David Howells <dhowells@redhat.com> wrote:
>
> Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> > So long as that ->update function:
> > 1. Deletes the old on-disk data.
> > 2. Deletes the old key from the inode.
> > 3. Generates a new key using get_random_bytes.
> > 4. Stores that new key in the inode.
> > 5. Encrypts the updated data afresh with the new key.
> > 6. Puts the updated data onto disk,
> >
> > then this is fine with me, and feel free to have my Acked-by if you
> > want. But if it doesn't do that -- i.e. if it tries to reuse the old
> > key or similar -- then this isn't fine. But it sounds like from what
> > you've described that things are actually fine, in which case, I guess
> > it makes sense to apply your patch ontop of mine and commit these.
>
> Yep.  It calls big_key_destroy(), which clears away the old stuff just as when
> a key is being destroyed, then generic_key_instantiate() just as when a key is
> being set up.
>
> The key ID and the key metadata (ownership, perms, expiry) are maintained, but
> the payload is just completely replaced.

Okay, in that case, take my:

    Acked-by: Jason A. Donenfeld <Jason@zx2c4.com>

And then perhaps you can take both my patch and your addendum into keys-next.

Jason

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3] security/keys: rewrite big_key crypto to use library interface
@ 2020-05-13  2:33                     ` Jason A. Donenfeld
  0 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-13  2:33 UTC (permalink / raw)
  To: David Howells
  Cc: keyrings, LKML, Andy Lutomirski, Greg KH, Linus Torvalds,
	kernel-hardening, Eric Biggers

On Tue, May 12, 2020 at 4:03 PM David Howells <dhowells@redhat.com> wrote:
>
> Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> > So long as that ->update function:
> > 1. Deletes the old on-disk data.
> > 2. Deletes the old key from the inode.
> > 3. Generates a new key using get_random_bytes.
> > 4. Stores that new key in the inode.
> > 5. Encrypts the updated data afresh with the new key.
> > 6. Puts the updated data onto disk,
> >
> > then this is fine with me, and feel free to have my Acked-by if you
> > want. But if it doesn't do that -- i.e. if it tries to reuse the old
> > key or similar -- then this isn't fine. But it sounds like from what
> > you've described that things are actually fine, in which case, I guess
> > it makes sense to apply your patch ontop of mine and commit these.
>
> Yep.  It calls big_key_destroy(), which clears away the old stuff just as when
> a key is being destroyed, then generic_key_instantiate() just as when a key is
> being set up.
>
> The key ID and the key metadata (ownership, perms, expiry) are maintained, but
> the payload is just completely replaced.

Okay, in that case, take my:

    Acked-by: Jason A. Donenfeld <Jason@zx2c4.com>

And then perhaps you can take both my patch and your addendum into keys-next.

Jason

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v3] security/keys: rewrite big_key crypto to use library interface
@ 2020-05-13  2:33                     ` Jason A. Donenfeld
  0 siblings, 0 replies; 28+ messages in thread
From: Jason A. Donenfeld @ 2020-05-13  2:33 UTC (permalink / raw)
  To: David Howells
  Cc: keyrings, LKML, Andy Lutomirski, Greg KH, Linus Torvalds,
	kernel-hardening, Eric Biggers

On Tue, May 12, 2020 at 4:03 PM David Howells <dhowells@redhat.com> wrote:
>
> Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> > So long as that ->update function:
> > 1. Deletes the old on-disk data.
> > 2. Deletes the old key from the inode.
> > 3. Generates a new key using get_random_bytes.
> > 4. Stores that new key in the inode.
> > 5. Encrypts the updated data afresh with the new key.
> > 6. Puts the updated data onto disk,
> >
> > then this is fine with me, and feel free to have my Acked-by if you
> > want. But if it doesn't do that -- i.e. if it tries to reuse the old
> > key or similar -- then this isn't fine. But it sounds like from what
> > you've described that things are actually fine, in which case, I guess
> > it makes sense to apply your patch ontop of mine and commit these.
>
> Yep.  It calls big_key_destroy(), which clears away the old stuff just as when
> a key is being destroyed, then generic_key_instantiate() just as when a key is
> being set up.
>
> The key ID and the key metadata (ownership, perms, expiry) are maintained, but
> the payload is just completely replaced.

Okay, in that case, take my:

    Acked-by: Jason A. Donenfeld <Jason@zx2c4.com>

And then perhaps you can take both my patch and your addendum into keys-next.

Jason

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2020-05-13  2:33 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-01 22:23 [PATCH] security/keys: rewrite big_key crypto to use Zinc Jason A. Donenfeld
2020-05-01 22:23 ` Jason A. Donenfeld
2020-05-01 23:09 ` Eric Biggers
2020-05-01 23:09   ` Eric Biggers
2020-05-02  0:06   ` Jason A. Donenfeld
2020-05-02  0:06     ` Jason A. Donenfeld
2020-05-02  0:14     ` Eric Biggers
2020-05-02  0:14       ` Eric Biggers
2020-05-02  0:15       ` Jason A. Donenfeld
2020-05-02  0:15         ` Jason A. Donenfeld
2020-05-02  0:19         ` [PATCH v2] " Jason A. Donenfeld
2020-05-02  0:19           ` Jason A. Donenfeld
2020-05-10 21:17           ` Eric Biggers
2020-05-10 21:17             ` Eric Biggers
2020-05-11 21:41             ` Jason A. Donenfeld
2020-05-11 21:41               ` Jason A. Donenfeld
2020-05-11 21:51               ` [PATCH v3] security/keys: rewrite big_key crypto to use library interface Jason A. Donenfeld
2020-05-11 21:51                 ` Jason A. Donenfeld
2020-05-12 13:17               ` David Howells
2020-05-12 13:17                 ` David Howells
2020-05-12 21:38                 ` Jason A. Donenfeld
2020-05-12 21:38                   ` Jason A. Donenfeld
2020-05-12 21:38                   ` Jason A. Donenfeld
2020-05-12 22:03                 ` David Howells
2020-05-12 22:03                   ` David Howells
2020-05-13  2:33                   ` Jason A. Donenfeld
2020-05-13  2:33                     ` Jason A. Donenfeld
2020-05-13  2:33                     ` Jason A. Donenfeld

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.