diff mbox

Fix BUG() in calc_seckey()

Message ID 1476736822-30098-1-git-send-email-sprabhu@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sachin Prabhu Oct. 17, 2016, 8:40 p.m. UTC
Andy Lutromirski's new virtually mapped kernel stack allocations moves
kernel stacks the vmalloc area. This triggers the bug
 kernel BUG at ./include/linux/scatterlist.h:140!
at calc_seckey()->sg_init()

Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
---
 fs/cifs/cifsencrypt.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Sachin Prabhu Oct. 18, 2016, 12:46 p.m. UTC | #1
A bit more reference for this patch

Information about the patch series which seems to have caused this
problem is available at

http://lwn.net/Articles/692208/

For the reasons described in the article, the kernel now allocates the
stack from the vmalloc region. The buffers used in calc_seckey() are
therefore now situated in the vmalloc-ed region. This triggers the
check in 

static inline void sg_set_buf(struct scatterlist *sg, const void *buf,
                              unsigned int buflen)
{
#ifdef CONFIG_DEBUG_SG
        BUG_ON(!virt_addr_valid(buf));
#endif
..
}

This issue was also addressed in Linus's release notes at
http://lwn.net/Articles/703664/

"
The virtual stack mapping also happens to mean that people who try to
do DMA from temporary buffers on the stack ("Don't do it!") now really
need to change their evil ways. So there is some fallout from this,
and I expect a couple of drivers to need minor fixes. But it's all for
a good cause, really (and it isn't all that common, because doing DMA
from the stack really has never been a good idea, and is generally not
even workable in most situations).
"

Sachin Prabhu

On Mon, 2016-10-17 at 16:40 -0400, Sachin Prabhu wrote:
> Andy Lutromirski's new virtually mapped kernel stack allocations
> moves
> kernel stacks the vmalloc area. This triggers the bug
>  kernel BUG at ./include/linux/scatterlist.h:140!
> at calc_seckey()->sg_init()
> 
> Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
> ---
>  fs/cifs/cifsencrypt.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
> index 8347c90..5eb0412 100644
> --- a/fs/cifs/cifsencrypt.c
> +++ b/fs/cifs/cifsencrypt.c
> @@ -808,7 +808,11 @@ calc_seckey(struct cifs_ses *ses)
>  	struct crypto_skcipher *tfm_arc4;
>  	struct scatterlist sgin, sgout;
>  	struct skcipher_request *req;
> -	unsigned char sec_key[CIFS_SESS_KEY_SIZE]; /* a nonce */
> +	unsigned char *sec_key;
> +
> +	sec_key = kmalloc(CIFS_SESS_KEY_SIZE, GFP_KERNEL);
> +	if (sec_key == NULL)
> +		return -ENOMEM;
>  
>  	get_random_bytes(sec_key, CIFS_SESS_KEY_SIZE);
>  
> @@ -816,7 +820,7 @@ calc_seckey(struct cifs_ses *ses)
>  	if (IS_ERR(tfm_arc4)) {
>  		rc = PTR_ERR(tfm_arc4);
>  		cifs_dbg(VFS, "could not allocate crypto API
> arc4\n");
> -		return rc;
> +		goto out;
>  	}
>  
>  	rc = crypto_skcipher_setkey(tfm_arc4, ses-
> >auth_key.response,
> @@ -854,7 +858,8 @@ calc_seckey(struct cifs_ses *ses)
>  
>  out_free_cipher:
>  	crypto_free_skcipher(tfm_arc4);
> -
> +out:
> +	kfree(sec_key);
>  	return rc;
>  }
>  

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Layton Oct. 18, 2016, 1:57 p.m. UTC | #2
On Mon, 2016-10-17 at 16:40 -0400, Sachin Prabhu wrote:
> Andy Lutromirski's new virtually mapped kernel stack allocations moves
> kernel stacks the vmalloc area. This triggers the bug
>  kernel BUG at ./include/linux/scatterlist.h:140!
> at calc_seckey()->sg_init()
> 
> Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
> ---
>  fs/cifs/cifsencrypt.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
> index 8347c90..5eb0412 100644
> --- a/fs/cifs/cifsencrypt.c
> +++ b/fs/cifs/cifsencrypt.c
> @@ -808,7 +808,11 @@ calc_seckey(struct cifs_ses *ses)
>  	struct crypto_skcipher *tfm_arc4;
>  	struct scatterlist sgin, sgout;
>  	struct skcipher_request *req;
> -	unsigned char sec_key[CIFS_SESS_KEY_SIZE]; /* a nonce */
> +	unsigned char *sec_key;
> +
> +	sec_key = kmalloc(CIFS_SESS_KEY_SIZE, GFP_KERNEL);
> +	if (sec_key == NULL)
> +		return -ENOMEM;
>  
>  	get_random_bytes(sec_key, CIFS_SESS_KEY_SIZE);
>  
> @@ -816,7 +820,7 @@ calc_seckey(struct cifs_ses *ses)
>  	if (IS_ERR(tfm_arc4)) {
>  		rc = PTR_ERR(tfm_arc4);
>  		cifs_dbg(VFS, "could not allocate crypto API arc4\n");
> -		return rc;
> +		goto out;
>  	}
>  
>  	rc = crypto_skcipher_setkey(tfm_arc4, ses->auth_key.response,
> @@ -854,7 +858,8 @@ calc_seckey(struct cifs_ses *ses)
>  
>  out_free_cipher:
>  	crypto_free_skcipher(tfm_arc4);
> -
> +out:
> +	kfree(sec_key);
>  	return rc;
>  }
>  

Reviewed-by: Jeff Layton <jlayton@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 8347c90..5eb0412 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -808,7 +808,11 @@  calc_seckey(struct cifs_ses *ses)
 	struct crypto_skcipher *tfm_arc4;
 	struct scatterlist sgin, sgout;
 	struct skcipher_request *req;
-	unsigned char sec_key[CIFS_SESS_KEY_SIZE]; /* a nonce */
+	unsigned char *sec_key;
+
+	sec_key = kmalloc(CIFS_SESS_KEY_SIZE, GFP_KERNEL);
+	if (sec_key == NULL)
+		return -ENOMEM;
 
 	get_random_bytes(sec_key, CIFS_SESS_KEY_SIZE);
 
@@ -816,7 +820,7 @@  calc_seckey(struct cifs_ses *ses)
 	if (IS_ERR(tfm_arc4)) {
 		rc = PTR_ERR(tfm_arc4);
 		cifs_dbg(VFS, "could not allocate crypto API arc4\n");
-		return rc;
+		goto out;
 	}
 
 	rc = crypto_skcipher_setkey(tfm_arc4, ses->auth_key.response,
@@ -854,7 +858,8 @@  calc_seckey(struct cifs_ses *ses)
 
 out_free_cipher:
 	crypto_free_skcipher(tfm_arc4);
-
+out:
+	kfree(sec_key);
 	return rc;
 }