[PATCH 1/2] fscrypto: don't use on-stack buffer for filename encryption

[PATCH 1/2] fscrypto: don't use on-stack buffer for filename encryption

[Eric Biggers]

With the new (in 4.9) option to use a virtually-mapped stack
(CONFIG_VMAP_STACK), stack buffers cannot be used as input/output for
the scatterlist crypto API because they may not be directly mappable to
struct page.  For short filenames, fname_encrypt() was encrypting a
stack buffer holding the padded filename.  Fix it by encrypting the
filename in-place in the output buffer, thereby making the temporary
buffer unnecessary.

This bug could most easily be observed in a CONFIG_DEBUG_SG kernel
because this allowed the BUG in sg_set_buf() to be triggered.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fname.c | 53 +++++++++++++++++++++--------------------------------
 1 file changed, 21 insertions(+), 32 deletions(-)

diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 9a28133..9b774f4 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -39,65 +39,54 @@ static void fname_crypt_complete(struct crypto_async_request *req, int res)
 static int fname_encrypt(struct inode *inode,
 			const struct qstr *iname, struct fscrypt_str *oname)
 {
-	u32 ciphertext_len;
 	struct skcipher_request *req = NULL;
 	DECLARE_FS_COMPLETION_RESULT(ecr);
 	struct fscrypt_info *ci = inode->i_crypt_info;
 	struct crypto_skcipher *tfm = ci->ci_ctfm;
 	int res = 0;
 	char iv[FS_CRYPTO_BLOCK_SIZE];
-	struct scatterlist src_sg, dst_sg;
+	struct scatterlist sg;
 	int padding = 4 << (ci->ci_flags & FS_POLICY_FLAGS_PAD_MASK);
-	char *workbuf, buf[32], *alloc_buf = NULL;
-	unsigned lim;
+	unsigned int lim;
+	unsigned int cryptlen;
 
 	lim = inode->i_sb->s_cop->max_namelen(inode);
 	if (iname->len <= 0 || iname->len > lim)
 		return -EIO;
 
-	ciphertext_len = max(iname->len, (u32)FS_CRYPTO_BLOCK_SIZE);
-	ciphertext_len = round_up(ciphertext_len, padding);
-	ciphertext_len = min(ciphertext_len, lim);
+	/*
+	 * Copy the filename to the output buffer for encrypting in-place and
+	 * pad it with the needed number of NUL bytes.
+	 */
+	cryptlen = max_t(unsigned int, iname->len, FS_CRYPTO_BLOCK_SIZE);
+	cryptlen = round_up(cryptlen, padding);
+	cryptlen = min(cryptlen, lim);
+	memcpy(oname->name, iname->name, iname->len);
+	memset(oname->name + iname->len, 0, cryptlen - iname->len);
 
-	if (ciphertext_len <= sizeof(buf)) {
-		workbuf = buf;
-	} else {
-		alloc_buf = kmalloc(ciphertext_len, GFP_NOFS);
-		if (!alloc_buf)
-			return -ENOMEM;
-		workbuf = alloc_buf;
-	}
+	/* Initialize the IV */
+	memset(iv, 0, FS_CRYPTO_BLOCK_SIZE);
 
-	/* Allocate request */
+	/* Set up the encryption request */
 	req = skcipher_request_alloc(tfm, GFP_NOFS);
 	if (!req) {
 		printk_ratelimited(KERN_ERR
-			"%s: crypto_request_alloc() failed\n", __func__);
-		kfree(alloc_buf);
+			"%s: skcipher_request_alloc() failed\n", __func__);
 		return -ENOMEM;
 	}
 	skcipher_request_set_callback(req,
 			CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
 			fname_crypt_complete, &ecr);
+	sg_init_one(&sg, oname->name, cryptlen);
+	skcipher_request_set_crypt(req, &sg, &sg, cryptlen, iv);
 
-	/* Copy the input */
-	memcpy(workbuf, iname->name, iname->len);
-	if (iname->len < ciphertext_len)
-		memset(workbuf + iname->len, 0, ciphertext_len - iname->len);
-
-	/* Initialize IV */
-	memset(iv, 0, FS_CRYPTO_BLOCK_SIZE);
-
-	/* Create encryption request */
-	sg_init_one(&src_sg, workbuf, ciphertext_len);
-	sg_init_one(&dst_sg, oname->name, ciphertext_len);
-	skcipher_request_set_crypt(req, &src_sg, &dst_sg, ciphertext_len, iv);
+	/* Do the encryption */
 	res = crypto_skcipher_encrypt(req);
 	if (res == -EINPROGRESS || res == -EBUSY) {
+		/* Request is being completed asynchronously; wait for it */
 		wait_for_completion(&ecr.completion);
 		res = ecr.res;
 	}
-	kfree(alloc_buf);
 	skcipher_request_free(req);
 	if (res < 0) {
 		printk_ratelimited(KERN_ERR
@@ -105,7 +94,7 @@ static int fname_encrypt(struct inode *inode,
 		return res;
 	}
 
-	oname->len = ciphertext_len;
+	oname->len = cryptlen;
 	return 0;
 }
 
-- 
2.8.0.rc3.226.g39d4020

[PATCH 2/2] fscrypto: don't use on-stack buffer for key derivation

[Eric Biggers]

With the new (in 4.9) option to use a virtually-mapped stack
(CONFIG_VMAP_STACK), stack buffers cannot be used as input/output for
the scatterlist crypto API because they may not be directly mappable to
struct page.  get_crypt_info() was using a stack buffer to hold the
output from the encryption operation used to derive the per-file key.
Fix it by using a heap buffer.

This bug could most easily be observed in a CONFIG_DEBUG_SG kernel
because this allowed the BUG in sg_set_buf() to be triggered.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/keyinfo.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 82f0285..67fb6d8 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -185,7 +185,7 @@ int get_crypt_info(struct inode *inode)
 	struct crypto_skcipher *ctfm;
 	const char *cipher_str;
 	int keysize;
-	u8 raw_key[FS_MAX_KEY_SIZE];
+	u8 *raw_key = NULL;
 	int res;
 
 	res = fscrypt_initialize();
@@ -238,6 +238,15 @@ int get_crypt_info(struct inode *inode)
 	if (res)
 		goto out;
 
+	/*
+	 * This cannot be a stack buffer because it is passed to the scatterlist
+	 * crypto API as part of key derivation.
+	 */
+	res = -ENOMEM;
+	raw_key = kmalloc(FS_MAX_KEY_SIZE, GFP_NOFS);
+	if (!raw_key)
+		goto out;
+
 	if (fscrypt_dummy_context_enabled(inode)) {
 		memset(raw_key, 0x42, FS_AES_256_XTS_KEY_SIZE);
 		goto got_key;
@@ -276,7 +285,8 @@ int get_crypt_info(struct inode *inode)
 	if (res)
 		goto out;
 
-	memzero_explicit(raw_key, sizeof(raw_key));
+	kzfree(raw_key);
+	raw_key = NULL;
 	if (cmpxchg(&inode->i_crypt_info, NULL, crypt_info) != NULL) {
 		put_crypt_info(crypt_info);
 		goto retry;
@@ -287,7 +297,7 @@ int get_crypt_info(struct inode *inode)
 	if (res == -ENOKEY)
 		res = 0;
 	put_crypt_info(crypt_info);
-	memzero_explicit(raw_key, sizeof(raw_key));
+	kzfree(raw_key);
 	return res;
 }
 
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 1/2] fscrypto: don't use on-stack buffer for filename encryption

[Kent Overstreet]

On Thu, Nov 03, 2016 at 03:03:01PM -0700, Eric Biggers wrote:
> With the new (in 4.9) option to use a virtually-mapped stack
> (CONFIG_VMAP_STACK), stack buffers cannot be used as input/output for
> the scatterlist crypto API because they may not be directly mappable to
> struct page.  For short filenames, fname_encrypt() was encrypting a
> stack buffer holding the padded filename.  Fix it by encrypting the
> filename in-place in the output buffer, thereby making the temporary
> buffer unnecessary.
> 
> This bug could most easily be observed in a CONFIG_DEBUG_SG kernel
> because this allowed the BUG in sg_set_buf() to be triggered.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

> -		alloc_buf = kmalloc(ciphertext_len, GFP_NOFS);
> -		if (!alloc_buf)
> -			return -ENOMEM;
> -		workbuf = alloc_buf;

Vmalloc memory does have struct pages - you just need to use vmalloc_to_page()
instead of virt_to_page. Look at drivers/md/bcache/util.c bch_bio_map() if you
want an example.

It would be better to just fix the sg code to handle vmalloc memory, instead of
adding a kmalloc() that can fail (and an error path that inevitably won't be
tested).

Re: [PATCH 1/2] fscrypto: don't use on-stack buffer for filename encryption

[Andy Lutomirski]

On Nov 5, 2016 8:13 AM, "Kent Overstreet" <kent.overstreet@gmail.com> wrote:
>
> On Thu, Nov 03, 2016 at 03:03:01PM -0700, Eric Biggers wrote:
> > With the new (in 4.9) option to use a virtually-mapped stack
> > (CONFIG_VMAP_STACK), stack buffers cannot be used as input/output for
> > the scatterlist crypto API because they may not be directly mappable to
> > struct page.  For short filenames, fname_encrypt() was encrypting a
> > stack buffer holding the padded filename.  Fix it by encrypting the
> > filename in-place in the output buffer, thereby making the temporary
> > buffer unnecessary.
> >
> > This bug could most easily be observed in a CONFIG_DEBUG_SG kernel
> > because this allowed the BUG in sg_set_buf() to be triggered.
> >
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
>
> > -             alloc_buf = kmalloc(ciphertext_len, GFP_NOFS);
> > -             if (!alloc_buf)
> > -                     return -ENOMEM;
> > -             workbuf = alloc_buf;
>
> Vmalloc memory does have struct pages - you just need to use vmalloc_to_page()
> instead of virt_to_page. Look at drivers/md/bcache/util.c bch_bio_map() if you
> want an example.
>
> It would be better to just fix the sg code to handle vmalloc memory, instead of
> adding a kmalloc() that can fail (and an error path that inevitably won't be
> tested).

Probably not, because (a) vmalloc_to_page is slow and (b) stack
buffers can span physically noncontiguous pages.

I think it's best to either avoid stack buffers or to teach crypto about kiov.

Re: [PATCH 1/2] fscrypto: don't use on-stack buffer for filename encryption

[Richard Weinberger]

On 03.11.2016 23:03, Eric Biggers wrote:
> With the new (in 4.9) option to use a virtually-mapped stack
> (CONFIG_VMAP_STACK), stack buffers cannot be used as input/output for
> the scatterlist crypto API because they may not be directly mappable to
> struct page.  For short filenames, fname_encrypt() was encrypting a

As Kent and Andy pointed out, they are but here are dragons.
The pages can be non-linear and on platforms with different cache architectures
extra flush operations may be needed.

> stack buffer holding the padded filename.  Fix it by encrypting the
> filename in-place in the output buffer, thereby making the temporary
> buffer unnecessary.
> 
> This bug could most easily be observed in a CONFIG_DEBUG_SG kernel
> because this allowed the BUG in sg_set_buf() to be triggered.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/crypto/fname.c | 53 +++++++++++++++++++++--------------------------------
>  1 file changed, 21 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 9a28133..9b774f4 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -39,65 +39,54 @@ static void fname_crypt_complete(struct crypto_async_request *req, int res)
>  static int fname_encrypt(struct inode *inode,
>  			const struct qstr *iname, struct fscrypt_str *oname)
>  {
> -	u32 ciphertext_len;
>  	struct skcipher_request *req = NULL;
>  	DECLARE_FS_COMPLETION_RESULT(ecr);
>  	struct fscrypt_info *ci = inode->i_crypt_info;
>  	struct crypto_skcipher *tfm = ci->ci_ctfm;
>  	int res = 0;
>  	char iv[FS_CRYPTO_BLOCK_SIZE];
> -	struct scatterlist src_sg, dst_sg;
> +	struct scatterlist sg;
>  	int padding = 4 << (ci->ci_flags & FS_POLICY_FLAGS_PAD_MASK);
> -	char *workbuf, buf[32], *alloc_buf = NULL;
> -	unsigned lim;
> +	unsigned int lim;
> +	unsigned int cryptlen;
>  
>  	lim = inode->i_sb->s_cop->max_namelen(inode);
>  	if (iname->len <= 0 || iname->len > lim)
>  		return -EIO;
>  
> -	ciphertext_len = max(iname->len, (u32)FS_CRYPTO_BLOCK_SIZE);
> -	ciphertext_len = round_up(ciphertext_len, padding);
> -	ciphertext_len = min(ciphertext_len, lim);
> +	/*
> +	 * Copy the filename to the output buffer for encrypting in-place and
> +	 * pad it with the needed number of NUL bytes.
> +	 */
> +	cryptlen = max_t(unsigned int, iname->len, FS_CRYPTO_BLOCK_SIZE);
> +	cryptlen = round_up(cryptlen, padding);
> +	cryptlen = min(cryptlen, lim);
> +	memcpy(oname->name, iname->name, iname->len);
> +	memset(oname->name + iname->len, 0, cryptlen - iname->len);
>  
> -	if (ciphertext_len <= sizeof(buf)) {
> -		workbuf = buf;
> -	} else {
> -		alloc_buf = kmalloc(ciphertext_len, GFP_NOFS);
> -		if (!alloc_buf)
> -			return -ENOMEM;
> -		workbuf = alloc_buf;
> -	}
> +	/* Initialize the IV */
> +	memset(iv, 0, FS_CRYPTO_BLOCK_SIZE);

You can initialize it with iv = {0} at the beginning such that you don't
need the memset here.

> -	/* Allocate request */
> +	/* Set up the encryption request */
>  	req = skcipher_request_alloc(tfm, GFP_NOFS);
>  	if (!req) {
>  		printk_ratelimited(KERN_ERR
> -			"%s: crypto_request_alloc() failed\n", __func__);
> -		kfree(alloc_buf);
> +			"%s: skcipher_request_alloc() failed\n", __func__);
>  		return -ENOMEM;
>  	}
>  	skcipher_request_set_callback(req,
>  			CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
>  			fname_crypt_complete, &ecr);
> +	sg_init_one(&sg, oname->name, cryptlen);
> +	skcipher_request_set_crypt(req, &sg, &sg, cryptlen, iv);
>  
> -	/* Copy the input */
> -	memcpy(workbuf, iname->name, iname->len);
> -	if (iname->len < ciphertext_len)
> -		memset(workbuf + iname->len, 0, ciphertext_len - iname->len);
> -
> -	/* Initialize IV */
> -	memset(iv, 0, FS_CRYPTO_BLOCK_SIZE);
> -
> -	/* Create encryption request */
> -	sg_init_one(&src_sg, workbuf, ciphertext_len);
> -	sg_init_one(&dst_sg, oname->name, ciphertext_len);
> -	skcipher_request_set_crypt(req, &src_sg, &dst_sg, ciphertext_len, iv);
> +	/* Do the encryption */
>  	res = crypto_skcipher_encrypt(req);
>  	if (res == -EINPROGRESS || res == -EBUSY) {
> +		/* Request is being completed asynchronously; wait for it */
>  		wait_for_completion(&ecr.completion);
>  		res = ecr.res;
>  	}
> -	kfree(alloc_buf);
>  	skcipher_request_free(req);
>  	if (res < 0) {
>  		printk_ratelimited(KERN_ERR
> @@ -105,7 +94,7 @@ static int fname_encrypt(struct inode *inode,
>  		return res;
>  	}
>  
> -	oname->len = ciphertext_len;
> +	oname->len = cryptlen;
>  	return 0;
>  }

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH 2/2] fscrypto: don't use on-stack buffer for key derivation

[Richard Weinberger]

On 03.11.2016 23:03, Eric Biggers wrote:
> With the new (in 4.9) option to use a virtually-mapped stack
> (CONFIG_VMAP_STACK), stack buffers cannot be used as input/output for
> the scatterlist crypto API because they may not be directly mappable to
> struct page.  get_crypt_info() was using a stack buffer to hold the

See 1/2. :-)

> output from the encryption operation used to derive the per-file key.
> Fix it by using a heap buffer.
> 
> This bug could most easily be observed in a CONFIG_DEBUG_SG kernel
> because this allowed the BUG in sg_set_buf() to be triggered.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/crypto/keyinfo.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
> index 82f0285..67fb6d8 100644
> --- a/fs/crypto/keyinfo.c
> +++ b/fs/crypto/keyinfo.c
> @@ -185,7 +185,7 @@ int get_crypt_info(struct inode *inode)
>  	struct crypto_skcipher *ctfm;
>  	const char *cipher_str;
>  	int keysize;
> -	u8 raw_key[FS_MAX_KEY_SIZE];
> +	u8 *raw_key = NULL;
>  	int res;
>  
>  	res = fscrypt_initialize();
> @@ -238,6 +238,15 @@ int get_crypt_info(struct inode *inode)
>  	if (res)
>  		goto out;
>  
> +	/*
> +	 * This cannot be a stack buffer because it is passed to the scatterlist
> +	 * crypto API as part of key derivation.
> +	 */
> +	res = -ENOMEM;
> +	raw_key = kmalloc(FS_MAX_KEY_SIZE, GFP_NOFS);
> +	if (!raw_key)
> +		goto out;
> +
>  	if (fscrypt_dummy_context_enabled(inode)) {
>  		memset(raw_key, 0x42, FS_AES_256_XTS_KEY_SIZE);
>  		goto got_key;
> @@ -276,7 +285,8 @@ int get_crypt_info(struct inode *inode)
>  	if (res)
>  		goto out;
>  
> -	memzero_explicit(raw_key, sizeof(raw_key));
> +	kzfree(raw_key);
> +	raw_key = NULL;
>  	if (cmpxchg(&inode->i_crypt_info, NULL, crypt_info) != NULL) {
>  		put_crypt_info(crypt_info);
>  		goto retry;
> @@ -287,7 +297,7 @@ int get_crypt_info(struct inode *inode)
>  	if (res == -ENOKEY)
>  		res = 0;
>  	put_crypt_info(crypt_info);
> -	memzero_explicit(raw_key, sizeof(raw_key));
> +	kzfree(raw_key);
>  	return res;
>  }

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH 1/2] fscrypto: don't use on-stack buffer for filename encryption

[Christoph Hellwig]

On Sat, Nov 05, 2016 at 07:13:49AM -0800, Kent Overstreet wrote:
> Vmalloc memory does have struct pages - you just need to use vmalloc_to_page()
> instead of virt_to_page. Look at drivers/md/bcache/util.c bch_bio_map() if you
> want an example.

That example seems to be clearly broken on virtually index caches
due to the lack of flush_kernel_vmap_range and
invalidate_kernel_vmap_range calls.

Re: [PATCH 1/2] fscrypto: don't use on-stack buffer for filename encryption

[Theodore Ts'o]

On Thu, Nov 03, 2016 at 03:03:01PM -0700, Eric Biggers wrote:
> With the new (in 4.9) option to use a virtually-mapped stack
> (CONFIG_VMAP_STACK), stack buffers cannot be used as input/output for
> the scatterlist crypto API because they may not be directly mappable to
> struct page.  For short filenames, fname_encrypt() was encrypting a
> stack buffer holding the padded filename.  Fix it by encrypting the
> filename in-place in the output buffer, thereby making the temporary
> buffer unnecessary.
> 
> This bug could most easily be observed in a CONFIG_DEBUG_SG kernel
> because this allowed the BUG in sg_set_buf() to be triggered.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

This commit is on the fscrypt and dev branches on ext4.git.

     	       	      	      	      - Ted

Re: [PATCH 2/2] fscrypto: don't use on-stack buffer for key derivation

[Theodore Ts'o]

On Thu, Nov 03, 2016 at 03:03:02PM -0700, Eric Biggers wrote:
> With the new (in 4.9) option to use a virtually-mapped stack
> (CONFIG_VMAP_STACK), stack buffers cannot be used as input/output for
> the scatterlist crypto API because they may not be directly mappable to
> struct page.  get_crypt_info() was using a stack buffer to hold the
> output from the encryption operation used to derive the per-file key.
> Fix it by using a heap buffer.
> 
> This bug could most easily be observed in a CONFIG_DEBUG_SG kernel
> because this allowed the BUG in sg_set_buf() to be triggered.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

This commit is on the fscrypt and dev branches on ext4.git.

     	       	      	      	  - Ted

Re: [PATCH 2/2] fscrypto: don't use on-stack buffer for key derivation

[Eric Biggers]

On Tue, Nov 15, 2016 at 11:47:04AM -0500, Theodore Ts'o wrote:
> On Thu, Nov 03, 2016 at 03:03:02PM -0700, Eric Biggers wrote:
> > With the new (in 4.9) option to use a virtually-mapped stack
> > (CONFIG_VMAP_STACK), stack buffers cannot be used as input/output for
> > the scatterlist crypto API because they may not be directly mappable to
> > struct page.  get_crypt_info() was using a stack buffer to hold the
> > output from the encryption operation used to derive the per-file key.
> > Fix it by using a heap buffer.
> > 
> > This bug could most easily be observed in a CONFIG_DEBUG_SG kernel
> > because this allowed the BUG in sg_set_buf() to be triggered.
> > 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> 
> This commit is on the fscrypt and dev branches on ext4.git.
> 
>      	       	      	      	  - Ted

Hi Ted,

Would it make any sense to send these two patches to Linus for v4.9-rc6, given
that they fix bugs introduced in 4.9 with the virtually-mapped stack feature?
Or would you prefer to wait and have them go to 4.9 via stable?
Note that CONFIG_VMAP_STACK defaults to y on x86_64.

Thanks,

Eric