* [PATCH] cifs: fix crash due to race in hmac(md5) handling
@ 2016-07-19 7:26 Rabin Vincent
[not found] ` <1468913181-28163-1-git-send-email-rabin.vincent-VrBV9hrLPhE@public.gmane.org>
0 siblings, 1 reply; 5+ messages in thread
From: Rabin Vincent @ 2016-07-19 7:26 UTC (permalink / raw)
To: sfrench-eUNUBHrolfbYtjvyW6yDsg
Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] cifs: fix crash due to race in hmac(md5) handling
[not found] ` <1468913181-28163-1-git-send-email-rabin.vincent-VrBV9hrLPhE@public.gmane.org>
@ 2016-07-20 13:57 ` Sachin Prabhu
[not found] ` <1469023049.4942.21.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-07-20 15:54 ` Steve French
1 sibling, 1 reply; 5+ messages in thread
From: Sachin Prabhu @ 2016-07-20 13:57 UTC (permalink / raw)
To: Rabin Vincent, sfrench-eUNUBHrolfbYtjvyW6yDsg
Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA, Rabin Vincent
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);
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cifs: fix crash due to race in hmac(md5) handling
[not found] ` <1468913181-28163-1-git-send-email-rabin.vincent-VrBV9hrLPhE@public.gmane.org>
2016-07-20 13:57 ` Sachin Prabhu
@ 2016-07-20 15:54 ` Steve French
1 sibling, 0 replies; 5+ messages in thread
From: Steve French @ 2016-07-20 15:54 UTC (permalink / raw)
To: Rabin Vincent
Cc: Steve French, linux-cifs-u79uwXL29TY76Z2rM5mHXA, Rabin Vincent
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cifs: fix crash due to race in hmac(md5) handling
[not found] ` <1469023049.4942.21.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2016-07-21 7:30 ` Rabin Vincent
[not found] ` <20160721073053.GA31832-VrBV9hrLPhE@public.gmane.org>
0 siblings, 1 reply; 5+ messages in thread
From: Rabin Vincent @ 2016-07-21 7:30 UTC (permalink / raw)
To: Sachin Prabhu
Cc: sfrench-eUNUBHrolfbYtjvyW6yDsg, linux-cifs-u79uwXL29TY76Z2rM5mHXA
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)
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] cifs: fix crash due to race in hmac(md5) handling
[not found] ` <20160721073053.GA31832-VrBV9hrLPhE@public.gmane.org>
@ 2016-07-25 9:51 ` Sachin Prabhu
0 siblings, 0 replies; 5+ messages in thread
From: Sachin Prabhu @ 2016-07-25 9:51 UTC (permalink / raw)
To: Rabin Vincent
Cc: sfrench-eUNUBHrolfbYtjvyW6yDsg, linux-cifs-u79uwXL29TY76Z2rM5mHXA
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)
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-07-25 9:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-19 7:26 [PATCH] cifs: fix crash due to race in hmac(md5) handling Rabin Vincent
[not found] ` <1468913181-28163-1-git-send-email-rabin.vincent-VrBV9hrLPhE@public.gmane.org>
2016-07-20 13:57 ` Sachin Prabhu
[not found] ` <1469023049.4942.21.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-07-21 7:30 ` Rabin Vincent
[not found] ` <20160721073053.GA31832-VrBV9hrLPhE@public.gmane.org>
2016-07-25 9:51 ` Sachin Prabhu
2016-07-20 15:54 ` Steve French
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.