[PATCH] [CIFS] [1/4] NTLM authentication and signing - Calculate auth response per smb session

[PATCH] [CIFS] [1/4] NTLM authentication and signing - Calculate auth response per smb session

[shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w]

From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Start calculation auth response within a session.  Move/Add pertinet
data structures like session key, server challenge and ntlmv2_hash in
a session structure.  We should do the calculations within a session
before copying session key and response over to server data
structures because a session setup can fail.

Only after a very first smb session succeeds, it copies/makes its
session key, session key of smb connection.  This key stays with
the smb connection throughout its life.


Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/cifsencrypt.c |   16 ++++++++--------
 fs/cifs/cifsglob.h    |    6 ++++--
 fs/cifs/cifssmb.c     |    4 ++--
 fs/cifs/connect.c     |   16 +++++++++++++---
 fs/cifs/sess.c        |   28 ++++++++--------------------
 5 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index e3edd8a..7ac0056 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -458,7 +458,7 @@ calc_exit_1:
 calc_exit_2:
 	/* BB FIXME what about bytes 24 through 40 of the signing key?
 	   compare with the NTLM example */
-	hmac_md5_final(ses->server->ntlmv2_hash, pctxt);
+	hmac_md5_final(ses->ntlmv2_hash, pctxt);
 
 	kfree(pctxt);
 	return rc;
@@ -502,14 +502,14 @@ setup_ntlmv2_rsp(struct cifsSesInfo *ses, char *resp_buf,
 	}
 	CalcNTLMv2_response(ses, resp_buf);
 
-	/* now calculate the MAC key for NTLMv2 */
-	hmac_md5_init_limK_to_64(ses->server->ntlmv2_hash, 16, &context);
+	/* now calculate the session key for NTLMv2 */
+	hmac_md5_init_limK_to_64(ses->ntlmv2_hash, 16, &context);
 	hmac_md5_update(resp_buf, 16, &context);
-	hmac_md5_final(ses->server->session_key.data.ntlmv2.key, &context);
+	hmac_md5_final(ses->auth_key.data.ntlmv2.key, &context);
 
-	memcpy(&ses->server->session_key.data.ntlmv2.resp, resp_buf,
+	memcpy(&ses->auth_key.data.ntlmv2.resp, resp_buf,
 	       sizeof(struct ntlmv2_resp));
-	ses->server->session_key.len = 16 + sizeof(struct ntlmv2_resp);
+	ses->auth_key.len = 16 + sizeof(struct ntlmv2_resp);
 
 	return 0;
 
@@ -526,8 +526,8 @@ void CalcNTLMv2_response(const struct cifsSesInfo *ses,
 {
 	struct HMACMD5Context context;
 	/* rest of v2 struct already generated */
-	memcpy(v2_session_response + 8, ses->server->cryptKey, 8);
-	hmac_md5_init_limK_to_64(ses->server->ntlmv2_hash, 16, &context);
+	memcpy(v2_session_response + 8, ses->cryptKey, 8);
+	hmac_md5_init_limK_to_64(ses->ntlmv2_hash, 16, &context);
 
 	hmac_md5_update(v2_session_response+8,
 			sizeof(struct ntlmv2_resp) - 8, &context);
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 4f85dfd..7d834b1 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -179,12 +179,10 @@ struct TCP_Server_Info {
 	int capabilities; /* allow selective disabling of caps by smb sess */
 	int timeAdj;  /* Adjust for difference in server time zone in sec */
 	__u16 CurrentMid;         /* multiplex id - rotating counter */
-	char cryptKey[CIFS_CRYPTO_KEY_SIZE];
 	/* 16th byte of RFC1001 workstation name is always null */
 	char workstation_RFC1001_name[RFC1001_NAME_LEN_WITH_NULL];
 	__u32 sequence_number; /* needed for CIFS PDU signature */
 	struct session_key session_key;
-	char ntlmv2_hash[16];
 	unsigned long lstrp; /* when we got last response from this server */
 	u16 dialect; /* dialect index that server chose */
 	/* extended security flavors that server supports */
@@ -192,6 +190,7 @@ struct TCP_Server_Info {
 	bool	sec_mskerberos;		/* supports legacy MS Kerberos */
 	bool	sec_kerberosu2u;	/* supports U2U Kerberos */
 	bool	sec_ntlmssp;		/* supports NTLMSSP */
+	bool session_estab; /* mark when very first sess is established */
 #ifdef CONFIG_CIFS_FSCACHE
 	struct fscache_cookie   *fscache; /* client index cache cookie */
 #endif
@@ -223,6 +222,9 @@ struct cifsSesInfo {
 	char userName[MAX_USERNAME_SIZE + 1];
 	char *domainName;
 	char *password;
+	char cryptKey[CIFS_CRYPTO_KEY_SIZE];
+	struct session_key auth_key;
+	char ntlmv2_hash[16];
 	unsigned int tilen; /* length of the target info blob */
 	unsigned char *tiblob; /* target info blob in challenge response */
 	bool need_reconnect:1; /* connection reset, uid now invalid */
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 54bd83a..a420c7b 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -503,7 +503,7 @@ CIFSSMBNegotiate(unsigned int xid, struct cifsSesInfo *ses)
 
 		if (rsp->EncryptionKeyLength ==
 				cpu_to_le16(CIFS_CRYPTO_KEY_SIZE)) {
-			memcpy(server->cryptKey, rsp->EncryptionKey,
+			memcpy(ses->cryptKey, rsp->EncryptionKey,
 				CIFS_CRYPTO_KEY_SIZE);
 		} else if (server->secMode & SECMODE_PW_ENCRYPT) {
 			rc = -EIO; /* need cryptkey unless plain text */
@@ -574,7 +574,7 @@ CIFSSMBNegotiate(unsigned int xid, struct cifsSesInfo *ses)
 	server->timeAdj = (int)(__s16)le16_to_cpu(pSMBr->ServerTimeZone);
 	server->timeAdj *= 60;
 	if (pSMBr->EncryptionKeyLength == CIFS_CRYPTO_KEY_SIZE) {
-		memcpy(server->cryptKey, pSMBr->u.EncryptionKey,
+		memcpy(ses->cryptKey, pSMBr->u.EncryptionKey,
 		       CIFS_CRYPTO_KEY_SIZE);
 	} else if ((pSMBr->hdr.Flags2 & SMBFLG2_EXT_SEC)
 			&& (pSMBr->EncryptionKeyLength == 0)) {
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 4944fc8..3606ba8 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -206,6 +206,7 @@ cifs_reconnect(struct TCP_Server_Info *server)
 			if (server->tcpStatus != CifsExiting)
 				server->tcpStatus = CifsGood;
 			server->sequence_number = 0;
+			server->session_estab = false;
 			spin_unlock(&GlobalMid_Lock);
 	/*		atomic_set(&server->inFlight,0);*/
 			wake_up(&server->response_q);
@@ -1631,6 +1632,7 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
 		volume_info->source_rfc1001_name, RFC1001_NAME_LEN_WITH_NULL);
 	memcpy(tcp_ses->server_RFC1001_name,
 		volume_info->target_rfc1001_name, RFC1001_NAME_LEN_WITH_NULL);
+	tcp_ses->session_estab = false;
 	tcp_ses->sequence_number = 0;
 	INIT_LIST_HEAD(&tcp_ses->tcp_ses_list);
 	INIT_LIST_HEAD(&tcp_ses->smb_ses_list);
@@ -2983,14 +2985,13 @@ CIFSTCon(unsigned int xid, struct cifsSesInfo *ses,
 #ifdef CONFIG_CIFS_WEAK_PW_HASH
 		if ((global_secflags & CIFSSEC_MAY_LANMAN) &&
 		    (ses->server->secType == LANMAN))
-			calc_lanman_hash(tcon->password, ses->server->cryptKey,
+			calc_lanman_hash(tcon->password, ses->cryptKey,
 					 ses->server->secMode &
 					    SECMODE_PW_ENCRYPT ? true : false,
 					 bcc_ptr);
 		else
 #endif /* CIFS_WEAK_PW_HASH */
-		SMBNTencrypt(tcon->password, ses->server->cryptKey,
-			     bcc_ptr);
+		SMBNTencrypt(tcon->password, ses->cryptKey, bcc_ptr);
 
 		bcc_ptr += CIFS_SESS_KEY_SIZE;
 		if (ses->capabilities & CAP_UNICODE) {
@@ -3175,6 +3176,15 @@ int cifs_setup_session(unsigned int xid, struct cifsSesInfo *ses,
 	if (rc) {
 		cERROR(1, "Send error in SessSetup = %d", rc);
 	} else {
+		mutex_lock(&ses->server->srv_mutex);
+		if (!server->session_estab) {
+			memcpy(&server->session_key.data,
+				&ses->auth_key.data, ses->auth_key.len);
+			server->session_key.len = ses->auth_key.len;
+			ses->server->session_estab = true;
+		}
+		mutex_unlock(&server->srv_mutex);
+
 		cFYI(1, "CIFS Session Established successfully");
 		spin_lock(&GlobalMid_Lock);
 		ses->status = CifsGood;
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index c926e6c..2111bed 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -402,7 +402,7 @@ static int decode_ntlmssp_challenge(char *bcc_ptr, int blob_len,
 		return -EINVAL;
 	}
 
-	memcpy(ses->server->cryptKey, pblob->Challenge, CIFS_CRYPTO_KEY_SIZE);
+	memcpy(ses->cryptKey, pblob->Challenge, CIFS_CRYPTO_KEY_SIZE);
 	/* BB we could decode pblob->NegotiateFlags; some may be useful */
 	/* In particular we can examine sign flags */
 	/* BB spec says that if AvId field of MsvAvTimestamp is populated then
@@ -591,17 +591,12 @@ CIFS_SessSetup(unsigned int xid, struct cifsSesInfo *ses,
 	int bytes_remaining;
 	struct key *spnego_key = NULL;
 	__le32 phase = NtLmNegotiate; /* NTLMSSP, if needed, is multistage */
-	bool first_time;
 	int blob_len;
 	char *ntlmsspblob = NULL;
 
 	if (ses == NULL)
 		return -EINVAL;
 
-	read_lock(&cifs_tcp_ses_lock);
-	first_time = is_first_ses_reconnect(ses);
-	read_unlock(&cifs_tcp_ses_lock);
-
 	type = ses->server->secType;
 
 	cFYI(1, "sess setup type %d", type);
@@ -672,7 +667,7 @@ ssetup_ntlmssp_authenticate:
 		/* BB calculate hash with password */
 		/* and copy into bcc */
 
-		calc_lanman_hash(ses->password, ses->server->cryptKey,
+		calc_lanman_hash(ses->password, ses->cryptKey,
 				 ses->server->secMode & SECMODE_PW_ENCRYPT ?
 					true : false, lnm_session_key);
 
@@ -699,15 +694,11 @@ ssetup_ntlmssp_authenticate:
 			cpu_to_le16(CIFS_SESS_KEY_SIZE);
 
 		/* calculate session key */
-		SMBNTencrypt(ses->password, ses->server->cryptKey,
-			     ntlm_session_key);
+		SMBNTencrypt(ses->password, ses->cryptKey, ntlm_session_key);
 
-		if (first_time) /* should this be moved into common code
-				  with similar ntlmv2 path? */
-			cifs_calculate_session_key(&ses->server->session_key,
-				ntlm_session_key, ses->password);
+		cifs_calculate_session_key(&ses->auth_key,
+					ntlm_session_key, ses->password);
 		/* copy session key */
-
 		memcpy(bcc_ptr, (char *)ntlm_session_key, CIFS_SESS_KEY_SIZE);
 		bcc_ptr += CIFS_SESS_KEY_SIZE;
 		memcpy(bcc_ptr, (char *)ntlm_session_key, CIFS_SESS_KEY_SIZE);
@@ -794,17 +785,14 @@ ssetup_ntlmssp_authenticate:
 		}
 		/* bail out if key is too long */
 		if (msg->sesskey_len >
-		    sizeof(ses->server->session_key.data.krb5)) {
+		    sizeof(ses->auth_key.data.krb5)) {
 			cERROR(1, "Kerberos signing key too long (%u bytes)",
 				msg->sesskey_len);
 			rc = -EOVERFLOW;
 			goto ssetup_exit;
 		}
-		if (first_time) {
-			ses->server->session_key.len = msg->sesskey_len;
-			memcpy(ses->server->session_key.data.krb5,
-				msg->data, msg->sesskey_len);
-		}
+		ses->auth_key.len = msg->sesskey_len;
+		memcpy(ses->auth_key.data.krb5, msg->data, msg->sesskey_len);
 		pSMB->req.hdr.Flags2 |= SMBFLG2_EXT_SEC;
 		capabilities |= CAP_EXTENDED_SECURITY;
 		pSMB->req.Capabilities = cpu_to_le32(capabilities);
-- 
1.6.0.2

Re: [PATCH] [CIFS] [1/4] NTLM authentication and signing - Calculate auth response per smb session

[Jeff Layton]

On Tue, 12 Oct 2010 14:45:03 -0500
shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:

> From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Start calculation auth response within a session.  Move/Add pertinet
> data structures like session key, server challenge and ntlmv2_hash in
> a session structure.  We should do the calculations within a session
> before copying session key and response over to server data
> structures because a session setup can fail.
> 
> Only after a very first smb session succeeds, it copies/makes its
> session key, session key of smb connection.  This key stays with
> the smb connection throughout its life.
> 
> 
> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>


>  		bcc_ptr += CIFS_SESS_KEY_SIZE;
>  		if (ses->capabilities & CAP_UNICODE) {
> @@ -3175,6 +3176,15 @@ int cifs_setup_session(unsigned int xid, struct cifsSesInfo *ses,
>  	if (rc) {
>  		cERROR(1, "Send error in SessSetup = %d", rc);
>  	} else {
> +		mutex_lock(&ses->server->srv_mutex);
> +		if (!server->session_estab) {
> +			memcpy(&server->session_key.data,
> +				&ses->auth_key.data, ses->auth_key.len);
> +			server->session_key.len = ses->auth_key.len;
> +			ses->server->session_estab = true;
			^^^^
			nit: Aren't "server" and "ses->server" the
			same? Why access them differently here?

> +		}
> +		mutex_unlock(&server->srv_mutex);
> +
		^^^^^^
		What are the rules for locking around the session_estab
		flag? Here you're holding the srv_mutex when checking
		and altering it, but in cifs_reconnect you're altering
		it without holding the mutex. Is that a potential race
		condition?

>  		cFYI(1, "CIFS Session Established successfully");
>  		spin_lock(&GlobalMid_Lock);
>  		ses->status = CifsGood;

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] [CIFS] [1/4] NTLM authentication and signing - Calculate auth response per smb session

[Shirish Pargaonkar]

On Wed, Oct 13, 2010 at 7:36 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Tue, 12 Oct 2010 14:45:03 -0500
> shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>
>> From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> Start calculation auth response within a session.  Move/Add pertinet
>> data structures like session key, server challenge and ntlmv2_hash in
>> a session structure.  We should do the calculations within a session
>> before copying session key and response over to server data
>> structures because a session setup can fail.
>>
>> Only after a very first smb session succeeds, it copies/makes its
>> session key, session key of smb connection.  This key stays with
>> the smb connection throughout its life.
>>
>>
>> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
>
>>               bcc_ptr += CIFS_SESS_KEY_SIZE;
>>               if (ses->capabilities & CAP_UNICODE) {
>> @@ -3175,6 +3176,15 @@ int cifs_setup_session(unsigned int xid, struct cifsSesInfo *ses,
>>       if (rc) {
>>               cERROR(1, "Send error in SessSetup = %d", rc);
>>       } else {
>> +             mutex_lock(&ses->server->srv_mutex);
>> +             if (!server->session_estab) {
>> +                     memcpy(&server->session_key.data,
>> +                             &ses->auth_key.data, ses->auth_key.len);
>> +                     server->session_key.len = ses->auth_key.len;
>> +                     ses->server->session_estab = true;
>                        ^^^^
>                        nit: Aren't "server" and "ses->server" the
>                        same? Why access them differently here?
>
>> +             }
>> +             mutex_unlock(&server->srv_mutex);
>> +
>                ^^^^^^
>                What are the rules for locking around the session_estab
>                flag? Here you're holding the srv_mutex when checking
>                and altering it, but in cifs_reconnect you're altering
>                it without holding the mutex. Is that a potential race
>                condition?

Jeff, I do not think it is a potential race.  In cifs_reconnect, by the
time session_estab is set as false, all the sessions have been
invalidated and socket connection has been taken down
and restablished. Isn't that as good as new connection?
Setting session_estab under srv_mutex is mainly to prevent
newly established sessions from racing.

>
>>               cFYI(1, "CIFS Session Established successfully");
>>               spin_lock(&GlobalMid_Lock);
>>               ses->status = CifsGood;
>
> --
> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>

Re: [PATCH] [CIFS] [1/4] NTLM authentication and signing - Calculate auth response per smb session

[Jeff Layton]

On Wed, 13 Oct 2010 10:28:25 -0500
Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Wed, Oct 13, 2010 at 7:36 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > On Tue, 12 Oct 2010 14:45:03 -0500
> > shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> >
> >> From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>
> >> Start calculation auth response within a session.  Move/Add pertinet
> >> data structures like session key, server challenge and ntlmv2_hash in
> >> a session structure.  We should do the calculations within a session
> >> before copying session key and response over to server data
> >> structures because a session setup can fail.
> >>
> >> Only after a very first smb session succeeds, it copies/makes its
> >> session key, session key of smb connection.  This key stays with
> >> the smb connection throughout its life.
> >>
> >>
> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >
> >
> >>               bcc_ptr += CIFS_SESS_KEY_SIZE;
> >>               if (ses->capabilities & CAP_UNICODE) {
> >> @@ -3175,6 +3176,15 @@ int cifs_setup_session(unsigned int xid, struct cifsSesInfo *ses,
> >>       if (rc) {
> >>               cERROR(1, "Send error in SessSetup = %d", rc);
> >>       } else {
> >> +             mutex_lock(&ses->server->srv_mutex);
> >> +             if (!server->session_estab) {
> >> +                     memcpy(&server->session_key.data,
> >> +                             &ses->auth_key.data, ses->auth_key.len);
> >> +                     server->session_key.len = ses->auth_key.len;
> >> +                     ses->server->session_estab = true;
> >                        ^^^^
> >                        nit: Aren't "server" and "ses->server" the
> >                        same? Why access them differently here?
> >
> >> +             }
> >> +             mutex_unlock(&server->srv_mutex);
> >> +
> >                ^^^^^^
> >                What are the rules for locking around the session_estab
> >                flag? Here you're holding the srv_mutex when checking
> >                and altering it, but in cifs_reconnect you're altering
> >                it without holding the mutex. Is that a potential race
> >                condition?
> 
> Jeff, I do not think it is a potential race.  In cifs_reconnect, by the
> time session_estab is set as false, all the sessions have been
> invalidated and socket connection has been taken down
> and restablished. Isn't that as good as new connection?
> Setting session_estab under srv_mutex is mainly to prevent
> newly established sessions from racing.
> 

You may be right, but I don't immediately see what prevents the
following scenario:

The sessions have been invalidated in cifs_reconnect and we're making
our way down to the point where that flag is set to false.

Another task then calls smb_init to issue a new call, notices that the
session is invalid and reconnects it.

This succeeds, but cifs_reconnect isn't done yet. session_estab is
still true. The session setup code takes the mutex and then checks and
finds that the flag is still true so it doesn't copy the key.

Eventually cifs_reconnect flips the flag to false (maybe even while
we're holding the mutex) but now we're borked. We have a valid session
on the socket but the key is likely to be wrong.

This is admittedly unlikely, and maybe impossible, but we should
understand what makes it impossible before we wave our hands and say
there's no race.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] [CIFS] [1/4] NTLM authentication and signing - Calculate auth response per smb session

[Shirish Pargaonkar]

On Wed, Oct 13, 2010 at 12:03 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Wed, 13 Oct 2010 10:28:25 -0500
> Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Wed, Oct 13, 2010 at 7:36 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
>> > On Tue, 12 Oct 2010 14:45:03 -0500
>> > shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> >
>> >> From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >>
>> >> Start calculation auth response within a session.  Move/Add pertinet
>> >> data structures like session key, server challenge and ntlmv2_hash in
>> >> a session structure.  We should do the calculations within a session
>> >> before copying session key and response over to server data
>> >> structures because a session setup can fail.
>> >>
>> >> Only after a very first smb session succeeds, it copies/makes its
>> >> session key, session key of smb connection.  This key stays with
>> >> the smb connection throughout its life.
>> >>
>> >>
>> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >
>> >
>> >>               bcc_ptr += CIFS_SESS_KEY_SIZE;
>> >>               if (ses->capabilities & CAP_UNICODE) {
>> >> @@ -3175,6 +3176,15 @@ int cifs_setup_session(unsigned int xid, struct cifsSesInfo *ses,
>> >>       if (rc) {
>> >>               cERROR(1, "Send error in SessSetup = %d", rc);
>> >>       } else {
>> >> +             mutex_lock(&ses->server->srv_mutex);
>> >> +             if (!server->session_estab) {
>> >> +                     memcpy(&server->session_key.data,
>> >> +                             &ses->auth_key.data, ses->auth_key.len);
>> >> +                     server->session_key.len = ses->auth_key.len;
>> >> +                     ses->server->session_estab = true;
>> >                        ^^^^
>> >                        nit: Aren't "server" and "ses->server" the
>> >                        same? Why access them differently here?
>> >
>> >> +             }
>> >> +             mutex_unlock(&server->srv_mutex);
>> >> +
>> >                ^^^^^^
>> >                What are the rules for locking around the session_estab
>> >                flag? Here you're holding the srv_mutex when checking
>> >                and altering it, but in cifs_reconnect you're altering
>> >                it without holding the mutex. Is that a potential race
>> >                condition?
>>
>> Jeff, I do not think it is a potential race.  In cifs_reconnect, by the
>> time session_estab is set as false, all the sessions have been
>> invalidated and socket connection has been taken down
>> and restablished. Isn't that as good as new connection?
>> Setting session_estab under srv_mutex is mainly to prevent
>> newly established sessions from racing.
>>
>
> You may be right, but I don't immediately see what prevents the
> following scenario:
>
> The sessions have been invalidated in cifs_reconnect and we're making
> our way down to the point where that flag is set to false.
>
> Another task then calls smb_init to issue a new call, notices that the
> session is invalid and reconnects it.
>
> This succeeds, but cifs_reconnect isn't done yet. session_estab is
> still true. The session setup code takes the mutex and then checks and
> finds that the flag is still true so it doesn't copy the key.
>
> Eventually cifs_reconnect flips the flag to false (maybe even while
> we're holding the mutex) but now we're borked. We have a valid session
> on the socket but the key is likely to be wrong.
>
> This is admittedly unlikely, and maybe impossible, but we should
> understand what makes it impossible before we wave our hands and say
> there's no race.
>
> --
> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>

I think this change should take care of that
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 848e458..613741b 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -203,10 +203,12 @@ cifs_reconnect(struct TCP_Server_Info *server)
                } else {
                        atomic_inc(&tcpSesReconnectCount);
                        spin_lock(&GlobalMid_Lock);
+                       if (server->tcpStatus == CifsNeedReconnect) {
+                               server->sequence_number = 0;
+                               server->session_estab = false;
+                       }
                        if (server->tcpStatus != CifsExiting)
                                server->tcpStatus = CifsGood;
-                       server->sequence_number = 0;
-                       server->session_estab = false;
                        spin_unlock(&GlobalMid_Lock);
        /*              atomic_set(&server->inFlight,0);*/
                        wake_up(&server->response_q);

Re: [PATCH] [CIFS] [1/4] NTLM authentication and signing - Calculate auth response per smb session

[Shirish Pargaonkar]

On Wed, Oct 13, 2010 at 12:03 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Wed, 13 Oct 2010 10:28:25 -0500
> Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Wed, Oct 13, 2010 at 7:36 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
>> > On Tue, 12 Oct 2010 14:45:03 -0500
>> > shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> >
>> >> From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >>
>> >> Start calculation auth response within a session.  Move/Add pertinet
>> >> data structures like session key, server challenge and ntlmv2_hash in
>> >> a session structure.  We should do the calculations within a session
>> >> before copying session key and response over to server data
>> >> structures because a session setup can fail.
>> >>
>> >> Only after a very first smb session succeeds, it copies/makes its
>> >> session key, session key of smb connection.  This key stays with
>> >> the smb connection throughout its life.
>> >>
>> >>
>> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >
>> >
>> >>               bcc_ptr += CIFS_SESS_KEY_SIZE;
>> >>               if (ses->capabilities & CAP_UNICODE) {
>> >> @@ -3175,6 +3176,15 @@ int cifs_setup_session(unsigned int xid, struct cifsSesInfo *ses,
>> >>       if (rc) {
>> >>               cERROR(1, "Send error in SessSetup = %d", rc);
>> >>       } else {
>> >> +             mutex_lock(&ses->server->srv_mutex);
>> >> +             if (!server->session_estab) {
>> >> +                     memcpy(&server->session_key.data,
>> >> +                             &ses->auth_key.data, ses->auth_key.len);
>> >> +                     server->session_key.len = ses->auth_key.len;
>> >> +                     ses->server->session_estab = true;
>> >                        ^^^^
>> >                        nit: Aren't "server" and "ses->server" the
>> >                        same? Why access them differently here?
>> >
>> >> +             }
>> >> +             mutex_unlock(&server->srv_mutex);
>> >> +
>> >                ^^^^^^
>> >                What are the rules for locking around the session_estab
>> >                flag? Here you're holding the srv_mutex when checking
>> >                and altering it, but in cifs_reconnect you're altering
>> >                it without holding the mutex. Is that a potential race
>> >                condition?
>>
>> Jeff, I do not think it is a potential race.  In cifs_reconnect, by the
>> time session_estab is set as false, all the sessions have been
>> invalidated and socket connection has been taken down
>> and restablished. Isn't that as good as new connection?
>> Setting session_estab under srv_mutex is mainly to prevent
>> newly established sessions from racing.
>>
>
> You may be right, but I don't immediately see what prevents the
> following scenario:
>
> The sessions have been invalidated in cifs_reconnect and we're making
> our way down to the point where that flag is set to false.
>
> Another task then calls smb_init to issue a new call, notices that the
> session is invalid and reconnects it.
>
> This succeeds, but cifs_reconnect isn't done yet. session_estab is
> still true. The session setup code takes the mutex and then checks and
> finds that the flag is still true so it doesn't copy the key.
>
> Eventually cifs_reconnect flips the flag to false (maybe even while
> we're holding the mutex) but now we're borked. We have a valid session
> on the socket but the key is likely to be wrong.
>
> This is admittedly unlikely, and maybe impossible, but we should
> understand what makes it impossible before we wave our hands and say
> there's no race.
>
> --
> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>

Or we could move them earlier

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 848e458..d7e0651 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -173,6 +173,8 @@ cifs_reconnect(struct TCP_Server_Info *server)
                sock_release(server->ssocket);
                server->ssocket = NULL;
        }
+       server->sequence_number = 0;
+       server->session_estab = false;

        spin_lock(&GlobalMid_Lock);
        list_for_each(tmp, &server->pending_mid_q) {
@@ -205,8 +207,6 @@ cifs_reconnect(struct TCP_Server_Info *server)
                        spin_lock(&GlobalMid_Lock);
                        if (server->tcpStatus != CifsExiting)
                                server->tcpStatus = CifsGood;
-                       server->sequence_number = 0;
-                       server->session_estab = false;
                        spin_unlock(&GlobalMid_Lock);
        /*              atomic_set(&server->inFlight,0);*/
                        wake_up(&server->response_q);


There is no smb connection at this point and (certainly) sessions are
not established at this point.

Re: [PATCH] [CIFS] [1/4] NTLM authentication and signing - Calculate auth response per smb session

[Jeff Layton]

On Wed, 13 Oct 2010 13:42:24 -0500
Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Wed, Oct 13, 2010 at 12:03 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > On Wed, 13 Oct 2010 10:28:25 -0500
> > Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> On Wed, Oct 13, 2010 at 7:36 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> >> > On Tue, 12 Oct 2010 14:45:03 -0500
> >> > shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> >> >
> >> >> From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> >>
> >> >> Start calculation auth response within a session.  Move/Add pertinet
> >> >> data structures like session key, server challenge and ntlmv2_hash in
> >> >> a session structure.  We should do the calculations within a session
> >> >> before copying session key and response over to server data
> >> >> structures because a session setup can fail.
> >> >>
> >> >> Only after a very first smb session succeeds, it copies/makes its
> >> >> session key, session key of smb connection.  This key stays with
> >> >> the smb connection throughout its life.
> >> >>
> >> >>
> >> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> >
> >> >
> >> >>               bcc_ptr += CIFS_SESS_KEY_SIZE;
> >> >>               if (ses->capabilities & CAP_UNICODE) {
> >> >> @@ -3175,6 +3176,15 @@ int cifs_setup_session(unsigned int xid, struct cifsSesInfo *ses,
> >> >>       if (rc) {
> >> >>               cERROR(1, "Send error in SessSetup = %d", rc);
> >> >>       } else {
> >> >> +             mutex_lock(&ses->server->srv_mutex);
> >> >> +             if (!server->session_estab) {
> >> >> +                     memcpy(&server->session_key.data,
> >> >> +                             &ses->auth_key.data, ses->auth_key.len);
> >> >> +                     server->session_key.len = ses->auth_key.len;
> >> >> +                     ses->server->session_estab = true;
> >> >                        ^^^^
> >> >                        nit: Aren't "server" and "ses->server" the
> >> >                        same? Why access them differently here?
> >> >
> >> >> +             }
> >> >> +             mutex_unlock(&server->srv_mutex);
> >> >> +
> >> >                ^^^^^^
> >> >                What are the rules for locking around the session_estab
> >> >                flag? Here you're holding the srv_mutex when checking
> >> >                and altering it, but in cifs_reconnect you're altering
> >> >                it without holding the mutex. Is that a potential race
> >> >                condition?
> >>
> >> Jeff, I do not think it is a potential race.  In cifs_reconnect, by the
> >> time session_estab is set as false, all the sessions have been
> >> invalidated and socket connection has been taken down
> >> and restablished. Isn't that as good as new connection?
> >> Setting session_estab under srv_mutex is mainly to prevent
> >> newly established sessions from racing.
> >>
> >
> > You may be right, but I don't immediately see what prevents the
> > following scenario:
> >
> > The sessions have been invalidated in cifs_reconnect and we're making
> > our way down to the point where that flag is set to false.
> >
> > Another task then calls smb_init to issue a new call, notices that the
> > session is invalid and reconnects it.
> >
> > This succeeds, but cifs_reconnect isn't done yet. session_estab is
> > still true. The session setup code takes the mutex and then checks and
> > finds that the flag is still true so it doesn't copy the key.
> >
> > Eventually cifs_reconnect flips the flag to false (maybe even while
> > we're holding the mutex) but now we're borked. We have a valid session
> > on the socket but the key is likely to be wrong.
> >
> > This is admittedly unlikely, and maybe impossible, but we should
> > understand what makes it impossible before we wave our hands and say
> > there's no race.
> >
> > --
> > Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> >
> 
> Or we could move them earlier
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 848e458..d7e0651 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -173,6 +173,8 @@ cifs_reconnect(struct TCP_Server_Info *server)
>                 sock_release(server->ssocket);
>                 server->ssocket = NULL;
>         }
> +       server->sequence_number = 0;
> +       server->session_estab = false;
> 
>         spin_lock(&GlobalMid_Lock);
>         list_for_each(tmp, &server->pending_mid_q) {
> @@ -205,8 +207,6 @@ cifs_reconnect(struct TCP_Server_Info *server)
>                         spin_lock(&GlobalMid_Lock);
>                         if (server->tcpStatus != CifsExiting)
>                                 server->tcpStatus = CifsGood;
> -                       server->sequence_number = 0;
> -                       server->session_estab = false;
>                         spin_unlock(&GlobalMid_Lock);
>         /*              atomic_set(&server->inFlight,0);*/
>                         wake_up(&server->response_q);
> 
> 
> There is no smb connection at this point and (certainly) sessions are
> not established at this point.
> 

I think that's the right idea.

You want to make sure that everything is properly torn down, flags
reset, etc. before the socket is reconnected.

There may be other pieces of cifs_reconnect that violate that rule as
well. If there are, I'd suggest fixing those in a separate patch that
goes before this one.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] [CIFS] [1/4] NTLM authentication and signing - Calculate auth response per smb session

[Shirish Pargaonkar]

On Wed, Oct 13, 2010 at 1:47 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Wed, 13 Oct 2010 13:42:24 -0500
> Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Wed, Oct 13, 2010 at 12:03 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
>> > On Wed, 13 Oct 2010 10:28:25 -0500
>> > Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >
>> >> On Wed, Oct 13, 2010 at 7:36 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
>> >> > On Tue, 12 Oct 2010 14:45:03 -0500
>> >> > shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> >> >
>> >> >> From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >> >>
>> >> >> Start calculation auth response within a session.  Move/Add pertinet
>> >> >> data structures like session key, server challenge and ntlmv2_hash in
>> >> >> a session structure.  We should do the calculations within a session
>> >> >> before copying session key and response over to server data
>> >> >> structures because a session setup can fail.
>> >> >>
>> >> >> Only after a very first smb session succeeds, it copies/makes its
>> >> >> session key, session key of smb connection.  This key stays with
>> >> >> the smb connection throughout its life.
>> >> >>
>> >> >>
>> >> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >> >
>> >> >
>> >> >>               bcc_ptr += CIFS_SESS_KEY_SIZE;
>> >> >>               if (ses->capabilities & CAP_UNICODE) {
>> >> >> @@ -3175,6 +3176,15 @@ int cifs_setup_session(unsigned int xid, struct cifsSesInfo *ses,
>> >> >>       if (rc) {
>> >> >>               cERROR(1, "Send error in SessSetup = %d", rc);
>> >> >>       } else {
>> >> >> +             mutex_lock(&ses->server->srv_mutex);
>> >> >> +             if (!server->session_estab) {
>> >> >> +                     memcpy(&server->session_key.data,
>> >> >> +                             &ses->auth_key.data, ses->auth_key.len);
>> >> >> +                     server->session_key.len = ses->auth_key.len;
>> >> >> +                     ses->server->session_estab = true;
>> >> >                        ^^^^
>> >> >                        nit: Aren't "server" and "ses->server" the
>> >> >                        same? Why access them differently here?
>> >> >
>> >> >> +             }
>> >> >> +             mutex_unlock(&server->srv_mutex);
>> >> >> +
>> >> >                ^^^^^^
>> >> >                What are the rules for locking around the session_estab
>> >> >                flag? Here you're holding the srv_mutex when checking
>> >> >                and altering it, but in cifs_reconnect you're altering
>> >> >                it without holding the mutex. Is that a potential race
>> >> >                condition?
>> >>
>> >> Jeff, I do not think it is a potential race.  In cifs_reconnect, by the
>> >> time session_estab is set as false, all the sessions have been
>> >> invalidated and socket connection has been taken down
>> >> and restablished. Isn't that as good as new connection?
>> >> Setting session_estab under srv_mutex is mainly to prevent
>> >> newly established sessions from racing.
>> >>
>> >
>> > You may be right, but I don't immediately see what prevents the
>> > following scenario:
>> >
>> > The sessions have been invalidated in cifs_reconnect and we're making
>> > our way down to the point where that flag is set to false.
>> >
>> > Another task then calls smb_init to issue a new call, notices that the
>> > session is invalid and reconnects it.
>> >
>> > This succeeds, but cifs_reconnect isn't done yet. session_estab is
>> > still true. The session setup code takes the mutex and then checks and
>> > finds that the flag is still true so it doesn't copy the key.
>> >
>> > Eventually cifs_reconnect flips the flag to false (maybe even while
>> > we're holding the mutex) but now we're borked. We have a valid session
>> > on the socket but the key is likely to be wrong.
>> >
>> > This is admittedly unlikely, and maybe impossible, but we should
>> > understand what makes it impossible before we wave our hands and say
>> > there's no race.
>> >
>> > --
>> > Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>> >
>>
>> Or we could move them earlier
>>
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index 848e458..d7e0651 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -173,6 +173,8 @@ cifs_reconnect(struct TCP_Server_Info *server)
>>                 sock_release(server->ssocket);
>>                 server->ssocket = NULL;
>>         }
>> +       server->sequence_number = 0;
>> +       server->session_estab = false;
>>
>>         spin_lock(&GlobalMid_Lock);
>>         list_for_each(tmp, &server->pending_mid_q) {
>> @@ -205,8 +207,6 @@ cifs_reconnect(struct TCP_Server_Info *server)
>>                         spin_lock(&GlobalMid_Lock);
>>                         if (server->tcpStatus != CifsExiting)
>>                                 server->tcpStatus = CifsGood;
>> -                       server->sequence_number = 0;
>> -                       server->session_estab = false;
>>                         spin_unlock(&GlobalMid_Lock);
>>         /*              atomic_set(&server->inFlight,0);*/
>>                         wake_up(&server->response_q);
>>
>>
>> There is no smb connection at this point and (certainly) sessions are
>> not established at this point.
>>
>
> I think that's the right idea.
>
> You want to make sure that everything is properly torn down, flags
> reset, etc. before the socket is reconnected.
>
> There may be other pieces of cifs_reconnect that violate that rule as
> well. If there are, I'd suggest fixing those in a separate patch that
> goes before this one.
>
> --
> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>

I think we can prevent race in cifs_reconnect with this patch.
Why allow more than one reconnect call to execute rest of the code at all?


diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 848e458..3656c80 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -136,7 +136,8 @@ cifs_reconnect(struct TCP_Server_Info *server)
        struct mid_q_entry *mid_entry;

        spin_lock(&GlobalMid_Lock);
-       if (server->tcpStatus == CifsExiting) {
+       if (server->tcpStatus == CifsExiting ||
+               server->tcpStatus == CifsNeedReconnect) {
                /* the demux thread will exit normally
                next time through the loop */
                spin_unlock(&GlobalMid_Lock);