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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> - 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).
------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
On Nov 5, 2016 8:13 AM, "Kent Overstreet" <[email protected]> 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 <[email protected]>
>
> > - 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.
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 <[email protected]>
> ---
> 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 <[email protected]>
Thanks,
//richard
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 <[email protected]>
> ---
> 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 <[email protected]>
Thanks,
//richard
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.
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 <[email protected]>
This commit is on the fscrypt and dev branches on ext4.git.
- Ted
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 <[email protected]>
This commit is on the fscrypt and dev branches on ext4.git.
- Ted
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 <[email protected]>
>
> 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