| Message ID | 1478210582-86338-2-git-send-email-ebiggers@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-)