[PATCH] cifs: fix crash due to race in hmac(md5) handling

[PATCH] cifs: fix crash due to race in hmac(md5) handling

[Rabin Vincent]

From: Rabin Vincent <rabinv-VrBV9hrLPhE@public.gmane.org>

The secmech hmac(md5) structures are present in the TCP_Server_Info
struct and can be shared among multiple CIFS sessions.  However, the
server mutex is not currently held when these structures are allocated
and used, which can lead to a kernel crashes, as in the scenario below:

mount.cifs(8) #1				mount.cifs(8) #2

Is secmech.sdeschmaccmd5 allocated?
// false

						Is secmech.sdeschmaccmd5 allocated?
						// false

secmech.hmacmd = crypto_alloc_shash..
secmech.sdeschmaccmd5 = kzalloc..
sdeschmaccmd5->shash.tfm = &secmec.hmacmd;

						secmech.sdeschmaccmd5 = kzalloc
						// sdeschmaccmd5->shash.tfm
						// not yet assigned

crypto_shash_update()
 deref NULL sdeschmaccmd5->shash.tfm

 Unable to handle kernel paging request at virtual address 00000030
 epc   : 8027ba34 crypto_shash_update+0x38/0x158
 ra    : 8020f2e8 setup_ntlmv2_rsp+0x4bc/0xa84
 Call Trace:
  crypto_shash_update+0x38/0x158
  setup_ntlmv2_rsp+0x4bc/0xa84
  build_ntlmssp_auth_blob+0xbc/0x34c
  sess_auth_rawntlmssp_authenticate+0xac/0x248
  CIFS_SessSetup+0xf0/0x178
  cifs_setup_session+0x4c/0x84
  cifs_get_smb_ses+0x2c8/0x314
  cifs_mount+0x38c/0x76c
  cifs_do_mount+0x98/0x440
  mount_fs+0x20/0xc0
  vfs_kern_mount+0x58/0x138
  do_mount+0x1e8/0xccc
  SyS_mount+0x88/0xd4
  syscall_common+0x30/0x54

Fix this by locking the srv_mutex around the code which uses these
hmac(md5) structures.  All the other secmech algos already have similar
locking.

Fixes: 95dc8dd14e2e84cc ("Limit allocation of crypto mechanisms to dialect which requires")
Signed-off-by: Rabin Vincent <rabinv-VrBV9hrLPhE@public.gmane.org>
---
 fs/cifs/cifsencrypt.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 6aeb8d4..8347c90 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -743,24 +743,26 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const struct nls_table *nls_cp)
 
 	memcpy(ses->auth_key.response + baselen, tiblob, tilen);
 
+	mutex_lock(&ses->server->srv_mutex);
+
 	rc = crypto_hmacmd5_alloc(ses->server);
 	if (rc) {
 		cifs_dbg(VFS, "could not crypto alloc hmacmd5 rc %d\n", rc);
-		goto setup_ntlmv2_rsp_ret;
+		goto unlock;
 	}
 
 	/* calculate ntlmv2_hash */
 	rc = calc_ntlmv2_hash(ses, ntlmv2_hash, nls_cp);
 	if (rc) {
 		cifs_dbg(VFS, "could not get v2 hash rc %d\n", rc);
-		goto setup_ntlmv2_rsp_ret;
+		goto unlock;
 	}
 
 	/* calculate first part of the client response (CR1) */
 	rc = CalcNTLMv2_response(ses, ntlmv2_hash);
 	if (rc) {
 		cifs_dbg(VFS, "Could not calculate CR1 rc: %d\n", rc);
-		goto setup_ntlmv2_rsp_ret;
+		goto unlock;
 	}
 
 	/* now calculate the session key for NTLMv2 */
@@ -769,13 +771,13 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const struct nls_table *nls_cp)
 	if (rc) {
 		cifs_dbg(VFS, "%s: Could not set NTLMV2 Hash as a key\n",
 			 __func__);
-		goto setup_ntlmv2_rsp_ret;
+		goto unlock;
 	}
 
 	rc = crypto_shash_init(&ses->server->secmech.sdeschmacmd5->shash);
 	if (rc) {
 		cifs_dbg(VFS, "%s: Could not init hmacmd5\n", __func__);
-		goto setup_ntlmv2_rsp_ret;
+		goto unlock;
 	}
 
 	rc = crypto_shash_update(&ses->server->secmech.sdeschmacmd5->shash,
@@ -783,7 +785,7 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const struct nls_table *nls_cp)
 		CIFS_HMAC_MD5_HASH_SIZE);
 	if (rc) {
 		cifs_dbg(VFS, "%s: Could not update with response\n", __func__);
-		goto setup_ntlmv2_rsp_ret;
+		goto unlock;
 	}
 
 	rc = crypto_shash_final(&ses->server->secmech.sdeschmacmd5->shash,
@@ -791,6 +793,8 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const struct nls_table *nls_cp)
 	if (rc)
 		cifs_dbg(VFS, "%s: Could not generate md5 hash\n", __func__);
 
+unlock:
+	mutex_unlock(&ses->server->srv_mutex);
 setup_ntlmv2_rsp_ret:
 	kfree(tiblob);
 
-- 
2.1.4

Re: [PATCH] cifs: fix crash due to race in hmac(md5) handling

[Sachin Prabhu]

On Tue, 2016-07-19 at 09:26 +0200, Rabin Vincent wrote:
> From: Rabin Vincent <rabinv-VrBV9hrLPhE@public.gmane.org>
> 
> The secmech hmac(md5) structures are present in the TCP_Server_Info
> struct and can be shared among multiple CIFS sessions.  However, the
> server mutex is not currently held when these structures are
> allocated
> and used, which can lead to a kernel crashes, as in the scenario
> below:
> 
> mount.cifs(8) #1				mount.cifs(8) #2
> 
> Is secmech.sdeschmaccmd5 allocated?
> // false
> 
> 						Is
> secmech.sdeschmaccmd5 allocated?
> 						// false
> 
> secmech.hmacmd = crypto_alloc_shash..
> secmech.sdeschmaccmd5 = kzalloc..
> sdeschmaccmd5->shash.tfm = &secmec.hmacmd;
> 
> 						secmech.sdeschmaccmd5 =
> kzalloc
> 						// sdeschmaccmd5-
> >shash.tfm
> 						// not yet assigned
> 
> crypto_shash_update()
>  deref NULL sdeschmaccmd5->shash.tfm
> 
>  Unable to handle kernel paging request at virtual address 00000030
>  epc   : 8027ba34 crypto_shash_update+0x38/0x158
>  ra    : 8020f2e8 setup_ntlmv2_rsp+0x4bc/0xa84
>  Call Trace:
>   crypto_shash_update+0x38/0x158
>   setup_ntlmv2_rsp+0x4bc/0xa84
>   build_ntlmssp_auth_blob+0xbc/0x34c
>   sess_auth_rawntlmssp_authenticate+0xac/0x248
>   CIFS_SessSetup+0xf0/0x178
>   cifs_setup_session+0x4c/0x84
>   cifs_get_smb_ses+0x2c8/0x314
>   cifs_mount+0x38c/0x76c
>   cifs_do_mount+0x98/0x440
>   mount_fs+0x20/0xc0
>   vfs_kern_mount+0x58/0x138
>   do_mount+0x1e8/0xccc
>   SyS_mount+0x88/0xd4
>   syscall_common+0x30/0x54
> 
> Fix this by locking the srv_mutex around the code which uses these
> hmac(md5) structures.  All the other secmech algos already have
> similar
> locking.
> 
> Fixes: 95dc8dd14e2e84cc ("Limit allocation of crypto mechanisms to
> dialect which requires")
> Signed-off-by: Rabin Vincent <rabinv-VrBV9hrLPhE@public.gmane.org>

Looks correct. 

Acked-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Rabin, do you have a reliable reproducer for this case? If yes, can you
please point me to it.

Sachin Prabhu
> ---
>  fs/cifs/cifsencrypt.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
> index 6aeb8d4..8347c90 100644
> --- a/fs/cifs/cifsencrypt.c
> +++ b/fs/cifs/cifsencrypt.c
> @@ -743,24 +743,26 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const
> struct nls_table *nls_cp)
>  
>  	memcpy(ses->auth_key.response + baselen, tiblob, tilen);
>  
> +	mutex_lock(&ses->server->srv_mutex);
> +
>  	rc = crypto_hmacmd5_alloc(ses->server);
>  	if (rc) {
>  		cifs_dbg(VFS, "could not crypto alloc hmacmd5 rc
> %d\n", rc);
> -		goto setup_ntlmv2_rsp_ret;
> +		goto unlock;
>  	}
>  
>  	/* calculate ntlmv2_hash */
>  	rc = calc_ntlmv2_hash(ses, ntlmv2_hash, nls_cp);
>  	if (rc) {
>  		cifs_dbg(VFS, "could not get v2 hash rc %d\n", rc);
> -		goto setup_ntlmv2_rsp_ret;
> +		goto unlock;
>  	}
>  
>  	/* calculate first part of the client response (CR1) */
>  	rc = CalcNTLMv2_response(ses, ntlmv2_hash);
>  	if (rc) {
>  		cifs_dbg(VFS, "Could not calculate CR1 rc: %d\n",
> rc);
> -		goto setup_ntlmv2_rsp_ret;
> +		goto unlock;
>  	}
>  
>  	/* now calculate the session key for NTLMv2 */
> @@ -769,13 +771,13 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const
> struct nls_table *nls_cp)
>  	if (rc) {
>  		cifs_dbg(VFS, "%s: Could not set NTLMV2 Hash as a
> key\n",
>  			 __func__);
> -		goto setup_ntlmv2_rsp_ret;
> +		goto unlock;
>  	}
>  
>  	rc = crypto_shash_init(&ses->server->secmech.sdeschmacmd5-
> >shash);
>  	if (rc) {
>  		cifs_dbg(VFS, "%s: Could not init hmacmd5\n",
> __func__);
> -		goto setup_ntlmv2_rsp_ret;
> +		goto unlock;
>  	}
>  
>  	rc = crypto_shash_update(&ses->server->secmech.sdeschmacmd5-
> >shash,
> @@ -783,7 +785,7 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const
> struct nls_table *nls_cp)
>  		CIFS_HMAC_MD5_HASH_SIZE);
>  	if (rc) {
>  		cifs_dbg(VFS, "%s: Could not update with
> response\n", __func__);
> -		goto setup_ntlmv2_rsp_ret;
> +		goto unlock;
>  	}
>  
>  	rc = crypto_shash_final(&ses->server->secmech.sdeschmacmd5-
> >shash,
> @@ -791,6 +793,8 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const
> struct nls_table *nls_cp)
>  	if (rc)
>  		cifs_dbg(VFS, "%s: Could not generate md5 hash\n",
> __func__);
>  
> +unlock:
> +	mutex_unlock(&ses->server->srv_mutex);
>  setup_ntlmv2_rsp_ret:
>  	kfree(tiblob);
>  

Re: [PATCH] cifs: fix crash due to race in hmac(md5) handling

[Rabin Vincent]

On Wed, Jul 20, 2016 at 02:57:29PM +0100, Sachin Prabhu wrote:
> Rabin, do you have a reliable reproducer for this case? If yes, can you
> please point me to it.

I have only seen one report of the oops in real testing, so I don't have
a case that would reproduce easily on an unmodified kernel, but while
investigating this I made a debug patch which forces the faulty sequence
to occur and provides a 100% reliable reproducer.

The below patched forces the race condition by making two mount.cifs
processes wait for each other in the appropriate places.

With the force-race patch applied and without this fix, the crash occurs
every time when the following sequence of commands is run.  Tested in a
KVM guest on latest mainline with my "unbreak TCP session reuse" patch
applied.

With the force-race patch applied and with this fix, the mount processes
hang since the race condition can never be satisfied.

 cp /sbin/mount.cifs /sbin/cifs1
 cp /sbin/mount.cifs /sbin/cifs2
 mkdir -p cifs cifs2
 cifs1 //192.168.1.1/test cifs -o credentials=/creds &
 sleep 2
 cifs2 //192.168.1.1/test2 cifs2 -o credentials=/creds

8<-------------------------
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 8347c90..1ed6dfd 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -35,6 +35,32 @@
 #include <linux/highmem.h>
 #include <crypto/skcipher.h>
 
+static struct completion completions[100];
+
+void hack_set_state(int newstate)
+{
+	printk("%s: caller %pF sets state to %d\n", current->comm, (void *)_RET_IP_, newstate);
+	complete(&completions[newstate]);
+}
+
+void hack_wait_for_state(int state)
+{
+	printk("%s caller %pF wants state %d\n", current->comm, (void *)_RET_IP_, state);
+	wait_for_completion(&completions[state]);
+	printk("%s caller %pF got state %d\n", current->comm, (void *)_RET_IP_, state);
+}
+
+static int hack_init(void)
+{
+       int i;
+
+       for (i = 0; i < ARRAY_SIZE(completions); i++)
+               init_completion(&completions[i]);
+
+       return 0;
+}
+late_initcall(hack_init);
+
 static int
 cifs_crypto_shash_md5_allocate(struct TCP_Server_Info *server)
 {
@@ -54,6 +80,7 @@ cifs_crypto_shash_md5_allocate(struct TCP_Server_Info *server)
 
 	size = sizeof(struct shash_desc) +
 			crypto_shash_descsize(server->secmech.md5);
+
 	server->secmech.sdescmd5 = kmalloc(size, GFP_KERNEL);
 	if (!server->secmech.sdescmd5) {
 		crypto_free_shash(server->secmech.md5);
@@ -661,8 +688,10 @@ static int crypto_hmacmd5_alloc(struct TCP_Server_Info *server)
 	unsigned int size;
 
 	/* check if already allocated */
-	if (server->secmech.sdeschmacmd5)
+	if (server->secmech.sdeschmacmd5) {
+		printk("%s: server %p already allocated\n", __func__, server);
 		return 0;
+	}
 
 	server->secmech.hmacmd5 = crypto_alloc_shash("hmac(md5)", 0, 0);
 	if (IS_ERR(server->secmech.hmacmd5)) {
@@ -674,12 +703,28 @@ static int crypto_hmacmd5_alloc(struct TCP_Server_Info *server)
 
 	size = sizeof(struct shash_desc) +
 			crypto_shash_descsize(server->secmech.hmacmd5);
+
+	if (!strcmp(current->comm, "cifs1")) {
+		hack_wait_for_state(10);
+	}
+	if (!strcmp(current->comm, "cifs2")) {
+		hack_set_state(10);
+		hack_wait_for_state(20);
+	}
+
 	server->secmech.sdeschmacmd5 = kmalloc(size, GFP_KERNEL);
+	printk("%s: %s: server %p alloc sdeschmacmd5=%p\n", __func__, current->comm, server, server->secmech.sdeschmacmd5);
 	if (!server->secmech.sdeschmacmd5) {
 		crypto_free_shash(server->secmech.hmacmd5);
 		server->secmech.hmacmd5 = NULL;
 		return -ENOMEM;
 	}
+
+	if (!strcmp(current->comm, "cifs2")) {
+		hack_set_state(30);
+		hack_wait_for_state(40);
+	}
+
 	server->secmech.sdeschmacmd5->shash.tfm = server->secmech.hmacmd5;
 	server->secmech.sdeschmacmd5->shash.flags = 0x0;
 
@@ -745,6 +790,7 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const struct nls_table *nls_cp)
 
 	mutex_lock(&ses->server->srv_mutex);
 
+	printk("%s: ses %p ses->server %p\n", __func__, ses, ses->server);
 	rc = crypto_hmacmd5_alloc(ses->server);
 	if (rc) {
 		cifs_dbg(VFS, "could not crypto alloc hmacmd5 rc %d\n", rc);
@@ -780,6 +826,11 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const struct nls_table *nls_cp)
 		goto unlock;
 	}
 
+	if (!strcmp(current->comm, "cifs1")) {
+		hack_set_state(20);
+		hack_wait_for_state(30);
+	}
+
 	rc = crypto_shash_update(&ses->server->secmech.sdeschmacmd5->shash,
 		ntlmv2->ntlmv2_hash,
 		CIFS_HMAC_MD5_HASH_SIZE);
@@ -788,6 +839,8 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const struct nls_table *nls_cp)
 		goto unlock;
 	}
 
+	hack_set_state(40);
+
 	rc = crypto_shash_final(&ses->server->secmech.sdeschmacmd5->shash,
 		ses->auth_key.response);
 	if (rc)

Re: [PATCH] cifs: fix crash due to race in hmac(md5) handling

[Sachin Prabhu]

On Thu, 2016-07-21 at 09:30 +0200, Rabin Vincent wrote:
> On Wed, Jul 20, 2016 at 02:57:29PM +0100, Sachin Prabhu wrote:
> > 
> > Rabin, do you have a reliable reproducer for this case? If yes, can
> > you
> > please point me to it.
> I have only seen one report of the oops in real testing, so I don't
> have
> a case that would reproduce easily on an unmodified kernel, but while
> investigating this I made a debug patch which forces the faulty
> sequence
> to occur and provides a 100% reliable reproducer.
> 
> The below patched forces the race condition by making two mount.cifs
> processes wait for each other in the appropriate places.
> 
> With the force-race patch applied and without this fix, the crash
> occurs
> every time when the following sequence of commands is run.  Tested in
> a
> KVM guest on latest mainline with my "unbreak TCP session reuse"
> patch
> applied.
> 
> With the force-race patch applied and with this fix, the mount
> processes
> hang since the race condition can never be satisfied.


Thanks Rabin.

Sachin Prabhu

> 
>  cp /sbin/mount.cifs /sbin/cifs1
>  cp /sbin/mount.cifs /sbin/cifs2
>  mkdir -p cifs cifs2
>  cifs1 //192.168.1.1/test cifs -o credentials=/creds &
>  sleep 2
>  cifs2 //192.168.1.1/test2 cifs2 -o credentials=/creds
> 
> 8<-------------------------
> diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
> index 8347c90..1ed6dfd 100644
> --- a/fs/cifs/cifsencrypt.c
> +++ b/fs/cifs/cifsencrypt.c
> @@ -35,6 +35,32 @@
>  #include <linux/highmem.h>
>  #include <crypto/skcipher.h>
>  
> +static struct completion completions[100];
> +
> +void hack_set_state(int newstate)
> +{
> +	printk("%s: caller %pF sets state to %d\n", current->comm,
> (void *)_RET_IP_, newstate);
> +	complete(&completions[newstate]);
> +}
> +
> +void hack_wait_for_state(int state)
> +{
> +	printk("%s caller %pF wants state %d\n", current->comm,
> (void *)_RET_IP_, state);
> +	wait_for_completion(&completions[state]);
> +	printk("%s caller %pF got state %d\n", current->comm, (void
> *)_RET_IP_, state);
> +}
> +
> +static int hack_init(void)
> +{
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(completions); i++)
> +               init_completion(&completions[i]);
> +
> +       return 0;
> +}
> +late_initcall(hack_init);
> +
>  static int
>  cifs_crypto_shash_md5_allocate(struct TCP_Server_Info *server)
>  {
> @@ -54,6 +80,7 @@ cifs_crypto_shash_md5_allocate(struct
> TCP_Server_Info *server)
>  
>  	size = sizeof(struct shash_desc) +
>  			crypto_shash_descsize(server->secmech.md5);
> +
>  	server->secmech.sdescmd5 = kmalloc(size, GFP_KERNEL);
>  	if (!server->secmech.sdescmd5) {
>  		crypto_free_shash(server->secmech.md5);
> @@ -661,8 +688,10 @@ static int crypto_hmacmd5_alloc(struct
> TCP_Server_Info *server)
>  	unsigned int size;
>  
>  	/* check if already allocated */
> -	if (server->secmech.sdeschmacmd5)
> +	if (server->secmech.sdeschmacmd5) {
> +		printk("%s: server %p already allocated\n",
> __func__, server);
>  		return 0;
> +	}
>  
>  	server->secmech.hmacmd5 = crypto_alloc_shash("hmac(md5)", 0,
> 0);
>  	if (IS_ERR(server->secmech.hmacmd5)) {
> @@ -674,12 +703,28 @@ static int crypto_hmacmd5_alloc(struct
> TCP_Server_Info *server)
>  
>  	size = sizeof(struct shash_desc) +
>  			crypto_shash_descsize(server-
> >secmech.hmacmd5);
> +
> +	if (!strcmp(current->comm, "cifs1")) {
> +		hack_wait_for_state(10);
> +	}
> +	if (!strcmp(current->comm, "cifs2")) {
> +		hack_set_state(10);
> +		hack_wait_for_state(20);
> +	}
> +
>  	server->secmech.sdeschmacmd5 = kmalloc(size, GFP_KERNEL);
> +	printk("%s: %s: server %p alloc sdeschmacmd5=%p\n",
> __func__, current->comm, server, server->secmech.sdeschmacmd5);
>  	if (!server->secmech.sdeschmacmd5) {
>  		crypto_free_shash(server->secmech.hmacmd5);
>  		server->secmech.hmacmd5 = NULL;
>  		return -ENOMEM;
>  	}
> +
> +	if (!strcmp(current->comm, "cifs2")) {
> +		hack_set_state(30);
> +		hack_wait_for_state(40);
> +	}
> +
>  	server->secmech.sdeschmacmd5->shash.tfm = server-
> >secmech.hmacmd5;
>  	server->secmech.sdeschmacmd5->shash.flags = 0x0;
>  
> @@ -745,6 +790,7 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const
> struct nls_table *nls_cp)
>  
>  	mutex_lock(&ses->server->srv_mutex);
>  
> +	printk("%s: ses %p ses->server %p\n", __func__, ses, ses-
> >server);
>  	rc = crypto_hmacmd5_alloc(ses->server);
>  	if (rc) {
>  		cifs_dbg(VFS, "could not crypto alloc hmacmd5 rc
> %d\n", rc);
> @@ -780,6 +826,11 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const
> struct nls_table *nls_cp)
>  		goto unlock;
>  	}
>  
> +	if (!strcmp(current->comm, "cifs1")) {
> +		hack_set_state(20);
> +		hack_wait_for_state(30);
> +	}
> +
>  	rc = crypto_shash_update(&ses->server->secmech.sdeschmacmd5-
> >shash,
>  		ntlmv2->ntlmv2_hash,
>  		CIFS_HMAC_MD5_HASH_SIZE);
> @@ -788,6 +839,8 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const
> struct nls_table *nls_cp)
>  		goto unlock;
>  	}
>  
> +	hack_set_state(40);
> +
>  	rc = crypto_shash_final(&ses->server->secmech.sdeschmacmd5-
> >shash,
>  		ses->auth_key.response);
>  	if (rc)

Re: [PATCH] cifs: fix crash due to race in hmac(md5) handling

[Steve French]

pushed to cifs-2.6.git for-next

added cc: stable

On Tue, Jul 19, 2016 at 2:26 AM, Rabin Vincent <rabin.vincent-VrBV9hrLPhE@public.gmane.org> wrote:
> From: Rabin Vincent <rabinv-VrBV9hrLPhE@public.gmane.org>
>
> The secmech hmac(md5) structures are present in the TCP_Server_Info
> struct and can be shared among multiple CIFS sessions.  However, the
> server mutex is not currently held when these structures are allocated
> and used, which can lead to a kernel crashes, as in the scenario below:
>
> mount.cifs(8) #1                                mount.cifs(8) #2
>
> Is secmech.sdeschmaccmd5 allocated?
> // false
>
>                                                 Is secmech.sdeschmaccmd5 allocated?
>                                                 // false
>
> secmech.hmacmd = crypto_alloc_shash..
> secmech.sdeschmaccmd5 = kzalloc..
> sdeschmaccmd5->shash.tfm = &secmec.hmacmd;
>
>                                                 secmech.sdeschmaccmd5 = kzalloc
>                                                 // sdeschmaccmd5->shash.tfm
>                                                 // not yet assigned
>
> crypto_shash_update()
>  deref NULL sdeschmaccmd5->shash.tfm
>
>  Unable to handle kernel paging request at virtual address 00000030
>  epc   : 8027ba34 crypto_shash_update+0x38/0x158
>  ra    : 8020f2e8 setup_ntlmv2_rsp+0x4bc/0xa84
>  Call Trace:
>   crypto_shash_update+0x38/0x158
>   setup_ntlmv2_rsp+0x4bc/0xa84
>   build_ntlmssp_auth_blob+0xbc/0x34c
>   sess_auth_rawntlmssp_authenticate+0xac/0x248
>   CIFS_SessSetup+0xf0/0x178
>   cifs_setup_session+0x4c/0x84
>   cifs_get_smb_ses+0x2c8/0x314
>   cifs_mount+0x38c/0x76c
>   cifs_do_mount+0x98/0x440
>   mount_fs+0x20/0xc0
>   vfs_kern_mount+0x58/0x138
>   do_mount+0x1e8/0xccc
>   SyS_mount+0x88/0xd4
>   syscall_common+0x30/0x54
>
> Fix this by locking the srv_mutex around the code which uses these
> hmac(md5) structures.  All the other secmech algos already have similar
> locking.
>
> Fixes: 95dc8dd14e2e84cc ("Limit allocation of crypto mechanisms to dialect which requires")
> Signed-off-by: Rabin Vincent <rabinv-VrBV9hrLPhE@public.gmane.org>
> ---
>  fs/cifs/cifsencrypt.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
> index 6aeb8d4..8347c90 100644
> --- a/fs/cifs/cifsencrypt.c
> +++ b/fs/cifs/cifsencrypt.c
> @@ -743,24 +743,26 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const struct nls_table *nls_cp)
>
>         memcpy(ses->auth_key.response + baselen, tiblob, tilen);
>
> +       mutex_lock(&ses->server->srv_mutex);
> +
>         rc = crypto_hmacmd5_alloc(ses->server);
>         if (rc) {
>                 cifs_dbg(VFS, "could not crypto alloc hmacmd5 rc %d\n", rc);
> -               goto setup_ntlmv2_rsp_ret;
> +               goto unlock;
>         }
>
>         /* calculate ntlmv2_hash */
>         rc = calc_ntlmv2_hash(ses, ntlmv2_hash, nls_cp);
>         if (rc) {
>                 cifs_dbg(VFS, "could not get v2 hash rc %d\n", rc);
> -               goto setup_ntlmv2_rsp_ret;
> +               goto unlock;
>         }
>
>         /* calculate first part of the client response (CR1) */
>         rc = CalcNTLMv2_response(ses, ntlmv2_hash);
>         if (rc) {
>                 cifs_dbg(VFS, "Could not calculate CR1 rc: %d\n", rc);
> -               goto setup_ntlmv2_rsp_ret;
> +               goto unlock;
>         }
>
>         /* now calculate the session key for NTLMv2 */
> @@ -769,13 +771,13 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const struct nls_table *nls_cp)
>         if (rc) {
>                 cifs_dbg(VFS, "%s: Could not set NTLMV2 Hash as a key\n",
>                          __func__);
> -               goto setup_ntlmv2_rsp_ret;
> +               goto unlock;
>         }
>
>         rc = crypto_shash_init(&ses->server->secmech.sdeschmacmd5->shash);
>         if (rc) {
>                 cifs_dbg(VFS, "%s: Could not init hmacmd5\n", __func__);
> -               goto setup_ntlmv2_rsp_ret;
> +               goto unlock;
>         }
>
>         rc = crypto_shash_update(&ses->server->secmech.sdeschmacmd5->shash,
> @@ -783,7 +785,7 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const struct nls_table *nls_cp)
>                 CIFS_HMAC_MD5_HASH_SIZE);
>         if (rc) {
>                 cifs_dbg(VFS, "%s: Could not update with response\n", __func__);
> -               goto setup_ntlmv2_rsp_ret;
> +               goto unlock;
>         }
>
>         rc = crypto_shash_final(&ses->server->secmech.sdeschmacmd5->shash,
> @@ -791,6 +793,8 @@ setup_ntlmv2_rsp(struct cifs_ses *ses, const struct nls_table *nls_cp)
>         if (rc)
>                 cifs_dbg(VFS, "%s: Could not generate md5 hash\n", __func__);
>
> +unlock:
> +       mutex_unlock(&ses->server->srv_mutex);
>  setup_ntlmv2_rsp_ret:
>         kfree(tiblob);
>
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,

Steve